From d530b7301638a768412acc32db6db4e04754e5d4 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 16 Jun 2020 13:46:06 -0400 Subject: [PATCH 01/11] Merge #18275: wallet: error if an explicit fee rate was given but the needed fee rate differed 44cc75f80ee7805a117e9298a182af1a44bcbff4 wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm) Pull request description: This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for #11413. ACKs for top commit: Sjors: utACK 44cc75f80ee7805a117e9298a182af1a44bcbff4 (rebased) fjahr: re-ACK 44cc75f80ee7805a117e9298a182af1a44bcbff4 Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483 --- src/coinjoin/util.cpp | 2 +- src/wallet/wallet.cpp | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/coinjoin/util.cpp b/src/coinjoin/util.cpp index 50a8e6727d94b..f337473c60624 100644 --- a/src/coinjoin/util.cpp +++ b/src/coinjoin/util.cpp @@ -116,7 +116,7 @@ CTransactionBuilder::CTransactionBuilder(std::shared_ptr pwalletIn, con // Generate a feerate which will be used to consider if the remainder is dust and will go into fees or not coinControl.m_discard_feerate = ::GetDiscardRate(*pwallet); // Generate a feerate which will be used by calculations of this class and also by CWallet::CreateTransaction - coinControl.m_feerate = std::max(pwallet->chain().estimateSmartFee(int(pwallet->m_confirm_target), true, nullptr), pwallet->m_pay_tx_fee); + coinControl.m_feerate = std::max(GetRequiredFeeRate(*pwallet), pwallet->m_pay_tx_fee); // Change always goes back to origin coinControl.destChange = tallyItemIn.txdest; // Only allow tallyItems inputs for tx creation diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 14ba64998d6ac..298fa3739a5d4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3549,6 +3549,16 @@ bool CWallet::CreateTransactionInternal( CMutableTransaction txNew; FeeCalculation feeCalc; CFeeRate discard_rate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this); + + // Get the fee rate to use effective values in coin selection + CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc); + // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly + // provided one + if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) { + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(), nFeeRateNeeded.ToString()); + return false; + } + int nBytes{0}; { std::vector vecCoins; From 0fa19226cb04e6599ca37ff15e9e36236489ff23 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 4 Nov 2020 15:45:01 +1300 Subject: [PATCH 02/11] Merge #20220: wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 0be29000c011dec0722481dbebb159873da6fa54 rpc: update conf_target helps for correctness/consistency (Jon Atack) 778b9be40667876c705e586849ea9c9e44cf451c wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack) 603c0050837ec65765208dd54dde354145fbe098 wallet: add rpc send explicit fee rate coverage (Jon Atack) dd341e602d5160fc621c0299179b91403756b61d wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack) 44e7bfa60313e4ae67da49e5ba4535038b71b453 wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack) 6e1ea4273e52fdcd86c87628aa595c03a071ca8c test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack) 3ac7b0c6f1c68e74a84d868a454f508bada6b09d wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack) 2d8eba8f8425a2515022d51f1f5b4911329fbf55 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack) 1697a40b6f841a54ee0d9744ed7fd09034b0ddad wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack) fc5721723d34f76f9e1ffd2e31f274ea6b22f894 wallet: fix SetFeeEstimateMode() error message (Jon Atack) 052427eef1c9da84c474c5161b1910d3328ef0da wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack) Pull request description: Follow-up to #11413 providing a base to build on for #19543: - bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #20219 - adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany` - improves a few related RPC error messages and `ParseConfirmTarget()` / error message - fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see #19543), as well as immediate coverage and minimum fixes for 0.21. ACKs for top commit: kallewoof: Concept/Tested ACK 0be29000c011dec0722481dbebb159873da6fa54 meshcollider: Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54 Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0 --- src/rpc/util.cpp | 9 ++-- src/wallet/rpcwallet.cpp | 26 +++++---- test/functional/rpc_fundrawtransaction.py | 55 +++++++++++++++++++ test/functional/rpc_psbt.py | 65 +++++++++++++++++++---- test/functional/wallet_basic.py | 32 +++++++---- test/functional/wallet_send.py | 63 ++++++++++++++++++++-- 6 files changed, 212 insertions(+), 38 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 3b02da632f693..d1790c351c077 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -319,11 +319,12 @@ UniValue DescribeAddress(const CTxDestination& dest) unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target) { - int target = value.get_int(); - if (target < 1 || (unsigned int)target > max_target) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u - %u", 1, max_target)); + const int target{value.get_int()}; + const unsigned int unsigned_target{static_cast(target)}; + if (target < 1 || unsigned_target > max_target) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u and %u", 1, max_target)); } - return (unsigned int)target; + return unsigned_target; } /** diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index fdea130b65c7a..4f39fd5fd5ef1 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -221,7 +221,7 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un if (cc.m_fee_mode == FeeEstimateMode::DASH_KB || cc.m_fee_mode == FeeEstimateMode::DUFF_B) { if (estimate_param.isNull()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected estimate_mode requires a fee rate"); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Selected estimate_mode %s requires a fee rate to be specified in conf_target", estimate_mode.get_str())); } CAmount fee_rate = AmountFromValue(estimate_param); @@ -426,7 +426,8 @@ static RPCHelpMan sendtoaddress() "The recipient will receive less amount of Dash than you enter in the amount field."}, {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"}, {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" + "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n" @@ -921,7 +922,8 @@ static RPCHelpMan sendmany() }, {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"}, {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" + "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, {"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."}, @@ -3378,7 +3380,7 @@ static RPCHelpMan listunspent() }; } -void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl) +void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now @@ -3533,7 +3535,8 @@ static RPCHelpMan fundrawtransaction() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" + "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, }, @@ -4177,7 +4180,8 @@ static RPCHelpMan send() }, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" + "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", @@ -4189,7 +4193,8 @@ static RPCHelpMan send() {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"}, {"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The Dash address to receive the change"}, {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" + "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, {"include_watching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n" @@ -4205,7 +4210,7 @@ static RPCHelpMan send() {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"}, {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"}, {"psbt", RPCArg::Type::BOOL, /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."}, - {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A JSON array of integers.\n" + {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "Outputs to subtract the fee from, specified as integer indices.\n" "The fee will be equally deducted from the amount of each specified output.\n" "Those recipients will receive less funds than you enter in their corresponding amount field.\n" "If no outputs are specified here, the sender pays the fee.", @@ -4279,7 +4284,7 @@ static RPCHelpMan send() CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"]); CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can - // be overriden by options.add_inputs. + // be overridden by options.add_inputs. coin_control.m_add_inputs = rawTx.vin.size() == 0; FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control); @@ -4534,7 +4539,8 @@ static RPCHelpMan walletcreatefundedpsbt() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "Fallback to wallet's confirmation target", "Confirmation target (in blocks)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" + "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, }, diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index bf33d1092d119..6ac389e53f10b 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -8,6 +8,7 @@ from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( + assert_approx, assert_equal, assert_fee_amount, assert_greater_than, @@ -93,6 +94,7 @@ def run_test(self): self.test_op_return() self.test_watchonly() self.test_all_watched_funds() + self.test_feerate_with_conf_target_and_estimate_mode() self.test_option_feerate() self.test_address_reuse() self.test_option_subtract_fee_from_outputs() @@ -718,6 +720,59 @@ def test_option_feerate(self): assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) + def test_feerate_with_conf_target_and_estimate_mode(self): + self.log.info("Test fundrawtxn passing an explicit fee rate using conf_target and estimate_mode") + node = self.nodes[3] + # Make sure there is exactly one input so coin selection can't skew the result. + assert_equal(len(node.listunspent(1)), 1) + inputs = [] + outputs = {node.getnewaddress() : 1} + rawtx = node.createrawtransaction(inputs, outputs) + + for unit, fee_rate in {"dash/kb": 0.1, "duff/b": 10000}.items(): + self.log.info("Test fundrawtxn with conf_target {} estimate_mode {} produces expected fee".format(fee_rate, unit)) + # With no arguments passed, expect fee of 225 sats/b. + assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000225, vspan=0.00000001) + # Expect fee to be 10,000x higher when explicit fee 10,000x greater is specified. + result = node.fundrawtransaction(rawtx, {"conf_target": fee_rate, "estimate_mode": unit}) + assert_approx(result["fee"], vexp=0.0225, vspan=0.0001) + + for field, fee_rate in {"conf_target": 0.1, "estimate_mode": "duff/b"}.items(): + self.log.info("Test fundrawtxn raises RPC error if both feeRate and {} are passed".format(field)) + assert_raises_rpc_error( + -8, "Cannot specify both {} and feeRate".format(field), + lambda: node.fundrawtransaction(rawtx, {"feeRate": 0.1, field: fee_rate})) + + self.log.info("Test fundrawtxn with invalid estimate_mode settings") + for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): + assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k), + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": v, "conf_target": 0.1})) + for mode in ["foo", Decimal("3.141592")]: + assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": 0.1})) + + self.log.info("Test fundrawtxn with invalid conf_target settings") + for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + self.log.debug("{}".format(mode)) + for k, v in {"string": "", "object": {"foo": "bar"}}.items(): + assert_raises_rpc_error(-3, "Expected type number for conf_target, got {}".format(k), + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": v})) + if mode in ["dash/kb", "duff/b"]: + assert_raises_rpc_error(-3, "Amount out of range", + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": -1})) + assert_raises_rpc_error(-4, "Fee rate (0.00000000 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": 0})) + else: + for n in [-1, 0, 1009]: + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": n})) + + for unit, fee_rate in {"duff/B": 0.99999999, "DASH/kB": 0.00000999}.items(): + self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) + assert_raises_rpc_error(-4, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", + lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True})) + + def test_address_reuse(self): """Test no address reuse occurs.""" self.log.info("Test fundrawtxn does not reuse addresses") diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index cb5bd668a8f1b..e6753c1564935 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -94,8 +94,11 @@ def run_test(self): elif out['scriptPubKey']['address'] == p2pkh: p2pkh_pos = out['n'] + inputs = [{"txid":txid,"vout":p2pkh_pos}] + outputs = [{self.nodes[1].getnewaddress(): 9.99}] + # spend single key from node 1 - created_psbt = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}) + created_psbt = self.nodes[1].walletcreatefundedpsbt(inputs, outputs) walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(created_psbt['psbt']) # Make sure it has both types of UTXOs decoded = self.nodes[1].decodepsbt(walletprocesspsbt_out['psbt']) @@ -105,18 +108,62 @@ def run_test(self): assert_equal(walletprocesspsbt_out['complete'], True) self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex']) - # feeRate of 0.1 DASH / KB produces a total fee slightly below -maxtxfee (~0.06650000): - res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}, 0, {"feeRate": 0.1, "add_inputs": True}, False) + self.log.info("Test walletcreatefundedpsbt feeRate of 0.1 DASH/kB produces a total fee at or slightly below -maxtxfee (~0.06650000)") + res = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True}) assert_approx(res["fee"], 0.04, 0.03) - decoded_psbt = self.nodes[0].decodepsbt(res['psbt']) - for psbt_in in decoded_psbt["inputs"]: - assert "bip32_derivs" not in psbt_in - # feeRate of 10 DASH / KB produces a total fee well above -maxtxfee + self.log.info("Test walletcreatefundedpsbt explicit fee rate with conf_target and estimate_mode") + for unit, fee_rate in {"dash/kb": 0.1, "duff/b": 10000}.items(): + fee = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"conf_target": fee_rate, "estimate_mode": unit, "add_inputs": True})["fee"] + self.log.info("- conf_target {}, estimate_mode {} produces fee {} at or slightly below -maxtxfee (~0.05290000)".format(fee_rate, unit, fee)) + assert_approx(fee, vexp=0.04, vspan=0.03) + + for field, fee_rate in {"conf_target": 0.1, "estimate_mode": "duff/b"}.items(): + self.log.info("- raises RPC error if both feeRate and {} are passed".format(field)) + assert_raises_rpc_error(-8, "Cannot specify both {} and feeRate".format(field), + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, field: fee_rate, "add_inputs": True})) + + self.log.info("- raises RPC error with invalid estimate_mode settings") + for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): + assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k), + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": v, "conf_target": 0.1, "add_inputs": True})) + for mode in ["foo", Decimal("3.141592")]: + assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": 0.1, "add_inputs": True})) + + self.log.info("- raises RPC error if estimate_mode is passed without a conf_target") + for unit in ["DUFF/B", "DASH/KB"]: + assert_raises_rpc_error(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit), + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": unit})) + + self.log.info("- raises RPC error with invalid conf_target settings") + for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + self.log.debug("{}".format(mode)) + for k, v in {"string": "", "object": {"foo": "bar"}}.items(): + assert_raises_rpc_error(-3, "Expected type number for conf_target, got {}".format(k), + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": v, "add_inputs": True})) + if mode in ["dash/kb", "duff/b"]: + assert_raises_rpc_error(-3, "Amount out of range", + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": -1, "add_inputs": True})) + assert_raises_rpc_error(-4, "Fee rate (0.00000000 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": 0, "add_inputs": True})) + else: + for n in [-1, 0, 1009]: + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": n, "add_inputs": True})) + + for unit, fee_rate in {"DUFF/B": 0.99999999, "DASH/KB": 0.00000999}.items(): + self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) + assert_raises_rpc_error(-4, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", + lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True})) + + self.log.info("Test walletcreatefundedpsbt feeRate of 10 DASH/kB produces total fee well above -maxtxfee and raises RPC error") # previously this was silently capped at -maxtxfee - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}, 0, {"feeRate": 10, "add_inputs": True}) - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():1}, 0, {"feeRate": 10, "add_inputs": True}) + for bool_add, outputs_array in {True: outputs, False: [{self.nodes[1].getnewaddress(): 1}]}.items(): + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs_array, 0, {"feeRate": 10, "add_inputs": bool_add}) + self.log.info("Test various PSBT operations") # partially sign multisig things with node 1 psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2sh_pos}], outputs={self.nodes[1].getnewaddress():9.99}, options={'changeAddress': self.nodes[1].getrawchangeaddress()})['psbt'] walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(psbtx) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index ea2b49b994946..7621dd05a6e39 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -221,6 +221,8 @@ def run_test(self): assert_equal(self.nodes[2].getbalance(), node_2_bal) node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), Decimal('200'), fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex'])) + self.log.info("Test sendmany") + # Sendmany 100 DASH txid = self.nodes[2].sendmany('', {address: 100}, 0, False, "", []) self.nodes[2].generate(1) @@ -239,9 +241,9 @@ def run_test(self): self.start_node(3, self.nodes[3].extra_args) self.connect_nodes(0, 3) - # Sendmany with explicit fee (DASH/kB) + self.log.info("Test case-insensitive explicit fee rate (sendmany as DASH/kB)") # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + assert_raises_rpc_error(-8, "Selected estimate_mode dash/kB requires a fee rate to be specified in conf_target", self.nodes[2].sendmany, amounts={ address: 10 }, estimate_mode='dash/kB') @@ -265,9 +267,9 @@ def run_test(self): node_0_bal += Decimal('10') assert_equal(self.nodes[0].getbalance(), node_0_bal) - # Sendmany with explicit fee (DUFF/B) + self.log.info("Test case-insensitive explicit fee rate (sendmany as duff/B)") # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + assert_raises_rpc_error(-8, "Selected estimate_mode duff/b requires a fee rate to be specified in conf_target", self.nodes[2].sendmany, amounts={ address: 10 }, estimate_mode='duff/b') @@ -293,6 +295,11 @@ def run_test(self): node_0_bal += Decimal('10') assert_equal(self.nodes[0].getbalance(), node_0_bal) + # Test setting explicit fee rate just below the minimum. + for unit, fee_rate in {"DASH/kB": 0.00000999, "duff/B": 0.99999999}.items(): + self.log.info("Test sendmany raises 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) + assert_raises_rpc_error(-6, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", + self.nodes[2].sendmany, amounts={address: 10}, estimate_mode=unit, conf_target=fee_rate) # check if we can list zero value tx as available coins # 1. create raw_tx @@ -423,15 +430,14 @@ def run_test(self): self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) - # send with explicit dash/kb fee - self.log.info("test explicit fee (sendtoaddress as dash/kb)") + self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as DASH/kB)") self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) prebalance = self.nodes[2].getbalance() assert prebalance > 2 address = self.nodes[1].getnewaddress() # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + assert_raises_rpc_error(-8, "Selected estimate_mode dash/Kb requires a fee rate to be specified in conf_target", self.nodes[2].sendtoaddress, address=address, amount=1.0, @@ -457,15 +463,15 @@ def run_test(self): fee = prebalance - postbalance - Decimal('1') assert_fee_amount(fee, tx_size, Decimal('0.00002500')) - # send with explicit duff/b fee self.sync_all(self.nodes[0:3]) - self.log.info("test explicit fee (sendtoaddress as duff/b)") + + self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as duff/B)") self.nodes[0].generate(1) prebalance = self.nodes[2].getbalance() assert prebalance > 2 address = self.nodes[1].getnewaddress() # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", + assert_raises_rpc_error(-8, "Selected estimate_mode duff/b requires a fee rate to be specified in conf_target", self.nodes[2].sendtoaddress, address=address, amount=1.0, @@ -491,6 +497,12 @@ def run_test(self): fee = prebalance - postbalance - Decimal('1') assert_fee_amount(fee, tx_size, Decimal('0.00002000')) + # Test setting explicit fee rate just below the minimum. + for unit, fee_rate in {"DASH/kB": 0.00000999, "sat/B": 0.99999999}.items(): + self.log.info("Test sendtoaddress raises 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) + assert_raises_rpc_error(-6, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", + self.nodes[2].sendtoaddress, address=address, amount=1, estimate_mode=unit, conf_target=fee_rate) + # 2. Import address from node2 to node1 self.nodes[1].importaddress(address_to_import) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 78ffc4bb33aa3..734ed9d13dcae 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -97,6 +97,8 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, except AssertionError: # Provide debug info if the test fails self.log.error("Unexpected successful result:") + self.log.error(arg_conf_target) + self.log.error(arg_estimate_mode) self.log.error(options) res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) self.log.error(res) @@ -271,8 +273,10 @@ def run_test(self): assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"]) # but not at the same time - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical", - conf_target=1, estimate_mode="economical", add_to_wallet=False, expect_error=(-8,"Use either conf_target and estimate_mode or the options dictionary to control fee rate")) + for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical", + conf_target=1, estimate_mode=mode, add_to_wallet=False, + expect_error=(-8, "Use either conf_target and estimate_mode or the options dictionary to control fee rate")) self.log.info("Create PSBT from watch-only wallet w3, sign with w2...") res = self.test_send(from_wallet=w3, to_wallet=w1, amount=1) @@ -297,12 +301,61 @@ def run_test(self): res = w2.walletprocesspsbt(res["psbt"]) assert res["complete"] - self.log.info("Set fee rate...") + self.log.info("Test setting explicit fee rate") + res1 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical", add_to_wallet=False) + res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=1, estimate_mode="economical", add_to_wallet=False) + assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"]) + + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.00007, estimate_mode="dash/kb", add_to_wallet=False) + fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] + assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00007")) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=2, estimate_mode="duff/b", add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002")) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode="duff/b", - expect_error=(-3, "Amount out of range")) + + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.00004531, arg_estimate_mode="dash/kb", add_to_wallet=False) + fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] + assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00004531")) + + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=3, arg_estimate_mode="duff/b", add_to_wallet=False) + fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] + assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00003")) + + # TODO: This test should pass with all modes, e.g. with the next line uncommented, for consistency with the other explicit feerate RPCs. + # for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + for mode in ["dash/kb", "duff/b"]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode=mode, + expect_error=(-3, "Amount out of range")) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0, estimate_mode=mode, + expect_error=(-4, "Fee rate (0.00000000 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)")) + + for mode in ["foo", Decimal("3.141592")]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode=mode, + expect_error=(-8, "Invalid estimate_mode parameter")) + # TODO: these 2 equivalent sends with an invalid estimate_mode arg should both fail, but they do not...why? + # self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode, + # expect_error=(-8, "Invalid estimate_mode parameter")) + # assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w1.getnewaddress(): 1}, 0.1, mode)) + + # TODO: These tests should pass for consistency with the other explicit feerate RPCs, but they do not. + # for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + # self.log.debug("{}".format(mode)) + # for k, v in {"string": "", "object": {"foo": "bar"}}.items(): + # self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode, + # expect_error=(-3, "Expected type number for conf_target, got {}".format(k))) + + # TODO: error should use duff/B instead of DASH/kB if duff/B is selected. + # Test setting explicit fee rate just below the minimum. + for unit, fee_rate in {"duff/B": 0.99999999, "DASH/kB": 0.00000999}.items(): + self.log.info("Explicit fee rate raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=fee_rate, estimate_mode=unit, + expect_error=(-4, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)")) + + self.log.info("Explicit fee rate raises RPC error if estimate_mode is passed without a conf_target") + for unit, fee_rate in {"duff/B": 100, "DASH/kB": 0.001}.items(): + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, estimate_mode=unit, + expect_error=(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit))) # TODO: Return hex if fee rate is below -maxmempool # res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="duff/b", add_to_wallet=False) From f436c20bc44197b3c99c7b74eedab20fb6f3f17c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 17 Nov 2020 13:49:04 +0100 Subject: [PATCH 03/11] Merge #20305: wallet: introduce fee_rate sat/vB param/option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 05e82d86b09d914ebce05dbc92a7299cb026847b wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack) 9a670b4f07a6140de809d73cbd7f3e614eb6ea74 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack) be481b72e24fb6834bd674cd8daee67c6938b42d wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack) 449b730579566459e350703611629e63e54657ed wallet: provide valid values if invalid estimate mode passed (Jon Atack) 6da3afbaee5809ebf6d88efaa3958c505c2d71c7 wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack) 173b5b5fe07d45be5a1e5bc7a5df996f20ab1e85 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack) 7f9835a05abf3e168ad93e7195cbaa4bf61b9b07 wallet: remove fee rates from conf_target helps (Jon Atack) b7994c01e9a3251536fe6538a22f614774eec82d wallet: add fee_rate unit warnings to bumpfee (Jon Atack) 410e471fa42d3db04e8879c71f8c824dcc151a83 wallet: remove redundant bumpfee fee_rate checks (Jon Atack) a0d495747320c79b27a83c216dcc526ac8df8f24 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack) e21212f01b7c41eba13b0479b252053cf482bc1f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack) 6112cf20d43b0be34fe0edce2ac3e6b27cae1bbe wallet: add CFeeRate ctor doxygen documentation (Jon Atack) 3f7279161347543ce4e997d78ea89a4043491145 wallet: fix bug in RPC send options (Jon Atack) Pull request description: This PR builds on #11413 and #20220 to address #19543. - replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs - allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument - fix a bug in the experimental send RPC described in https://github.com/bitcoin/bitcoin/pull/20220#discussion_r513789526 where args were not being passed correctly into the options values - update the feerate error message units for these RPCs from BTC/kB to sat/vB - update the test coverage, help docs, doxygen docs, and some of the RPC examples - other changes to address the excellent review feedback See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309 ACKs for top commit: achow101: re-ACK 05e82d8 MarcoFalke: review ACK 05e82d86b0 did not test and found a few style nits, which can be fixed later 🍯 Xekyo: tACK 05e82d86b09d914ebce05dbc92a7299cb026847b Sjors: utACK 05e82d86b09d914ebce05dbc92a7299cb026847b Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c --- src/policy/feerate.h | 11 +- src/rpc/client.cpp | 9 +- src/rpc/mining.cpp | 2 +- src/test/amount_tests.cpp | 2 + src/util/fees.cpp | 7 +- src/util/fees.h | 1 + src/wallet/rpcwallet.cpp | 183 ++++++++++++--------- src/wallet/wallet.cpp | 2 +- test/functional/rpc_estimatefee.py | 2 +- test/functional/rpc_fundrawtransaction.py | 137 ++++++++++------ test/functional/rpc_psbt.py | 100 +++++++----- test/functional/wallet_basic.py | 189 ++++++++-------------- test/functional/wallet_send.py | 96 +++++------ 13 files changed, 393 insertions(+), 348 deletions(-) diff --git a/src/policy/feerate.h b/src/policy/feerate.h index e4bed40fd84b6..67b124d7972e4 100644 --- a/src/policy/feerate.h +++ b/src/policy/feerate.h @@ -19,8 +19,8 @@ enum class FeeEstimateMode { UNSET, //!< Use default settings based on other criteria ECONOMICAL, //!< Force estimateSmartFee to use non-conservative estimates CONSERVATIVE, //!< Force estimateSmartFee to use conservative estimates - DASH_KB, //!< Use explicit DASH/kB fee given in coin control - DUFF_B, //!< Use explicit duff/B fee given in coin control + DASH_KB, //!< Use DASH/kB fee rate unit + DUFF_B, //!< Use duff/B fee rate unit }; /** @@ -39,7 +39,12 @@ class CFeeRate // We've previously had bugs creep in from silent double->int conversion... static_assert(std::is_integral::value, "CFeeRate should be used without floats"); } - /** Constructor for a fee rate in satoshis per kB. The size in bytes must not exceed (2^63 - 1)*/ + /** Constructor for a fee rate in satoshis per kB (duff/kB). + * + * Passing a num_bytes value of COIN (1e8) returns a fee rate in satoshis per B (sat/B), + * e.g. (nFeePaid * 1e8 / 1e3) == (nFeePaid / 1e5), + * where 1e5 is the ratio to convert from DASH/kB to sat/B. + */ CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes); /** * Return the fee in satoshis for the given size in bytes. diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 8af1bcf4410cf..86b7c2f950357 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -44,7 +44,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "sendtoaddress", 6, "use_cj" }, { "sendtoaddress", 7, "conf_target" }, { "sendtoaddress", 9, "avoid_reuse" }, - { "sendtoaddress", 10, "verbose"}, + { "sendtoaddress", 10, "fee_rate"}, + { "sendtoaddress", 11, "verbose"}, { "settxfee", 0, "amount" }, { "sethdseed", 0, "newkeypool" }, { "getreceivedbyaddress", 1, "minconf" }, @@ -91,7 +92,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "sendmany", 6, "use_is" }, { "sendmany", 7, "use_cj" }, { "sendmany", 8, "conf_target" }, - { "sendmany", 10, "verbose" }, + { "sendmany", 10, "fee_rate" }, + { "sendmany", 11, "verbose" }, { "deriveaddresses", 1, "range" }, { "scantxoutset", 1, "scanobjects" }, { "addmultisigaddress", 0, "nrequired" }, @@ -152,7 +154,8 @@ static const CRPCConvertParam vRPCConvertParams[] = { "lockunspent", 1, "transactions" }, { "send", 0, "outputs" }, { "send", 1, "conf_target" }, - { "send", 3, "options" }, + { "send", 3, "fee_rate"}, + { "send", 4, "options" }, { "importprivkey", 2, "rescan" }, { "importelectrumwallet", 1, "index" }, { "importaddress", 2, "rescan" }, diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 026ceb2fc2696..d0e9d5db8386c 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1160,7 +1160,7 @@ static RPCHelpMan estimatesmartfee() if (!request.params[1].isNull()) { FeeEstimateMode fee_mode; if (!FeeModeFromString(request.params[1].get_str(), fee_mode)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); + throw JSONRPCError(RPC_INVALID_PARAMETER, InvalidEstimateModeErrorMessage()); } if (fee_mode == FeeEstimateMode::ECONOMICAL) conservative = false; } diff --git a/src/test/amount_tests.cpp b/src/test/amount_tests.cpp index 34d89ef7bde7b..efcc372d5bbc7 100644 --- a/src/test/amount_tests.cpp +++ b/src/test/amount_tests.cpp @@ -109,6 +109,8 @@ BOOST_AUTO_TEST_CASE(ToStringTest) CFeeRate feeRate; feeRate = CFeeRate(1); BOOST_CHECK_EQUAL(feeRate.ToString(), "0.00000001 DASH/kB"); + BOOST_CHECK_EQUAL(feeRate.ToString(FeeEstimateMode::DASH_KB), "0.00000001 DASH/kB"); + BOOST_CHECK_EQUAL(feeRate.ToString(FeeEstimateMode::DUFF_B), "0.001 duff/B"); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/fees.cpp b/src/util/fees.cpp index 48df56be42ef5..cbefe18dbb918 100644 --- a/src/util/fees.cpp +++ b/src/util/fees.cpp @@ -40,8 +40,6 @@ const std::vector>& FeeModeMap() {"unset", FeeEstimateMode::UNSET}, {"economical", FeeEstimateMode::ECONOMICAL}, {"conservative", FeeEstimateMode::CONSERVATIVE}, - {(CURRENCY_UNIT + "/kB"), FeeEstimateMode::DASH_KB}, - {(CURRENCY_ATOM + "/B"), FeeEstimateMode::DUFF_B}, }; return FEE_MODES; } @@ -51,6 +49,11 @@ std::string FeeModes(const std::string& delimiter) return Join(FeeModeMap(), delimiter, [&](const std::pair& i) { return i.first; }); } +const std::string InvalidEstimateModeErrorMessage() +{ + return "Invalid estimate_mode parameter, must be one of: \"" + FeeModes("\", \"") + "\""; +} + bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode) { auto searchkey = ToUpper(mode_string); diff --git a/src/util/fees.h b/src/util/fees.h index 1ae8b8e32226e..9ef2389d3e706 100644 --- a/src/util/fees.h +++ b/src/util/fees.h @@ -13,5 +13,6 @@ enum class FeeReason; bool FeeModeFromString(const std::string& mode_string, FeeEstimateMode& fee_estimate_mode); std::string StringForFeeReason(FeeReason reason); std::string FeeModes(const std::string& delimiter); +const std::string InvalidEstimateModeErrorMessage(); #endif // BITCOIN_UTIL_FEES_H diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4f39fd5fd5ef1..3f00992cf8a6a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -52,8 +52,6 @@ using interfaces::FoundBlock; static const std::string WALLET_ENDPOINT_BASE = "/wallet/"; -static const uint32_t WALLET_DASH_KB_TO_DUFF_B = COIN / 1000; // 1 duff / B = 0.00001 DASH / kB - static inline bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param) { bool can_avoid_reuse = wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); bool avoid_reuse = param.isNull() ? can_avoid_reuse : param.get_bool(); @@ -205,34 +203,42 @@ static std::string LabelFromValue(const UniValue& value) /** * Update coin control with fee estimation based on the given parameters * - * @param[in] wallet Wallet reference - * @param[in,out] cc Coin control which is to be updated - * @param[in] estimate_mode String value (e.g. "ECONOMICAL") - * @param[in] estimate_param Parameter (blocks to confirm, explicit fee rate, etc) - * @throws a JSONRPCError if estimate_mode is unknown, or if estimate_param is missing when required + * @param[in] wallet Wallet reference + * @param[in,out] cc Coin control to be updated + * @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h; + * if a fee_rate is present, 0 is allowed here as a no-op positional placeholder + * @param[in] estimate_mode UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative"; + * if a fee_rate is present, "" is allowed here as a no-op positional placeholder + * @param[in] fee_rate UniValue real; fee rate in sat/B; + * if a fee_rate is present, both conf_target and estimate_mode must either be null, or no-op + * @param[in] override_min_fee bool; whether to set fOverrideFeeRate to true to disable minimum fee rate checks and instead + verify only that fee_rate is greater than 0 + * @throws a JSONRPCError if conf_target, estimate_mode, or fee_rate contain invalid values or are in conflict */ -static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const UniValue& estimate_mode, const UniValue& estimate_param) +static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, bool override_min_fee) { - if (!estimate_mode.isNull()) { - if (!FeeModeFromString(estimate_mode.get_str(), cc.m_fee_mode)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter"); + if (!fee_rate.isNull()) { + if (!conf_target.isNull() && conf_target.get_int() > 0) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate."); } - } - - if (cc.m_fee_mode == FeeEstimateMode::DASH_KB || cc.m_fee_mode == FeeEstimateMode::DUFF_B) { - if (estimate_param.isNull()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Selected estimate_mode %s requires a fee rate to be specified in conf_target", estimate_mode.get_str())); + if (!estimate_mode.isNull() && !estimate_mode.get_str().empty()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate"); } - - CAmount fee_rate = AmountFromValue(estimate_param); - if (cc.m_fee_mode == FeeEstimateMode::DUFF_B) { - fee_rate /= WALLET_DASH_KB_TO_DUFF_B; + CFeeRate fee_rate_in_sat_vb{CFeeRate(AmountFromValue(fee_rate), COIN)}; + if (override_min_fee) { + if (fee_rate_in_sat_vb <= CFeeRate(0)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate_in_sat_vb.ToString(FeeEstimateMode::DUFF_B))); + } + cc.fOverrideFeeRate = true; } - - cc.m_feerate = CFeeRate(fee_rate); - - } else if (!estimate_param.isNull()) { - cc.m_confirm_target = ParseConfirmTarget(estimate_param, wallet.chain().estimateMaxBlocks()); + cc.m_feerate = fee_rate_in_sat_vb; + return; + } + if (!estimate_mode.isNull() && !FeeModeFromString(estimate_mode.get_str(), cc.m_fee_mode)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, InvalidEstimateModeErrorMessage()); + } + if (!conf_target.isNull()) { + cc.m_confirm_target = ParseConfirmTarget(conf_target, wallet.chain().estimateMaxBlocks()); } } @@ -426,12 +432,12 @@ static RPCHelpMan sendtoaddress() "The recipient will receive less amount of Dash than you enter in the amount field."}, {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"}, {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" - "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n" " dirty if they have previously been used in a transaction."}, + {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, {"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra information about the transaction."}, }, { @@ -445,15 +451,20 @@ static RPCHelpMan sendtoaddress() {RPCResult::Type::STR, "fee reason", "The transaction fee reason."} }, }, - }, - RPCExamples{ - HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1") - + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"donation\" \"seans outpost\"") - + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"\" \"\" true") - + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"\" \"\" false true 0.00002 " + (CURRENCY_UNIT + "/kB")) - + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"\" \"\" false true 2 " + (CURRENCY_ATOM + "/B")) - + HelpExampleRpc("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\", 0.1, \"donation\", \"seans outpost\"") - }, + }, + RPCExamples{ + "\nSend 0.1 Dash\n" + + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1") + + "\nSend 0.1 Dash with a confirmation target of 6 blocks in economical fee estimate mode using positional arguments\n" + + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"donation\" \"sean's outpost\" false false false 6 economical") + + "\nSend 0.1 Dash with a fee rate of 1 " + CURRENCY_ATOM + "/B, subtract fee from amount, use CoinJoin funds only, using positional arguments\n" + + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"drinks\" \"room77\" true false true 0 \"\" 1") + + "\nSend 0.2 Dash with a confirmation target of 6 blocks in economical fee estimate mode using named arguments\n" + + HelpExampleCli("-named sendtoaddress", "address=\"" + EXAMPLE_ADDRESS[0] + "\" amount=0.2 conf_target=6 estimate_mode=\"economical\"") + + "\nSend 0.5 Dash with a fee rate of 25 " + CURRENCY_ATOM + "/B using named arguments\n" + + HelpExampleCli("-named sendtoaddress", "address=\"" + EXAMPLE_ADDRESS[0] + "\" amount=0.5 fee_rate=25") + + HelpExampleCli("-named sendtoaddress", "address=\"" + EXAMPLE_ADDRESS[0] + "\" amount=0.5 fee_rate=25 subtractfeefromamount=false avoid_reuse=true comment=\"2 pizzas\" comment_to=\"jeremy\" verbose=true") + }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); @@ -487,7 +498,7 @@ static RPCHelpMan sendtoaddress() // We also enable partial spend avoidance if reuse avoidance is set. coin_control.m_avoid_partial_spends |= coin_control.m_avoid_address_reuse; - SetFeeEstimateMode(*pwallet, coin_control, request.params[8], request.params[7]); + SetFeeEstimateMode(*pwallet, coin_control, /* conf_target */ request.params[7], /* estimate_mode */ request.params[8], /* fee_rate */ request.params[10], /* override_min_fee */ false); EnsureWalletIsUnlocked(*pwallet); @@ -501,7 +512,7 @@ static RPCHelpMan sendtoaddress() std::vector recipients; ParseRecipients(address_amounts, subtractFeeFromAmount, recipients); - bool verbose = request.params[10].isNull() ? false: request.params[10].get_bool(); + const bool verbose{request.params[11].isNull() ? false : request.params[11].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose); }, @@ -922,10 +933,10 @@ static RPCHelpMan sendmany() }, {"use_is", RPCArg::Type::BOOL, /* default */ "false", "Deprecated and ignored"}, {"use_cj", RPCArg::Type::BOOL, /* default */ "false", "Use CoinJoin funds only"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" - "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, + {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, {"verbose", RPCArg::Type::BOOL, /* default */ "false", "If true, return extra infomration about the transaction."}, }, { @@ -981,11 +992,11 @@ static RPCHelpMan sendmany() coin_control.UseCoinJoin(request.params[7].get_bool()); } - SetFeeEstimateMode(*pwallet, coin_control, request.params[9], request.params[8]); + SetFeeEstimateMode(*pwallet, coin_control, /* conf_target */ request.params[8], /* estimate_mode */ request.params[9], /* fee_rate */ request.params[10], /* override_min_fee */ false); std::vector recipients; ParseRecipients(sendTo, subtractFeeFromAmount, recipients); - bool verbose = request.params[10].isNull() ? false : request.params[10].get_bool(); + const bool verbose{request.params[11].isNull() ? false : request.params[11].get_bool()}; return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose); }, @@ -3380,7 +3391,7 @@ static RPCHelpMan listunspent() }; } -void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl) +void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, const UniValue& options, CCoinControl& coinControl, bool override_min_fee) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now @@ -3413,7 +3424,8 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, {"lockUnspents", UniValueType(UniValue::VBOOL)}, {"lock_unspents", UniValueType(UniValue::VBOOL)}, {"locktime", UniValueType(UniValue::VNUM)}, - {"feeRate", UniValueType()}, // will be checked below + {"fee_rate", UniValueType()}, // will be checked by AmountFromValue() in SetFeeEstimateMode() + {"feeRate", UniValueType()}, // will be checked by AmountFromValue() below {"psbt", UniValueType(UniValue::VBOOL)}, {"subtractFeeFromOutputs", UniValueType(UniValue::VARR)}, {"subtract_fee_from_outputs", UniValueType(UniValue::VARR)}, @@ -3453,21 +3465,27 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, } if (options.exists("feeRate")) { + if (options.exists("fee_rate")) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both fee_rate (" + CURRENCY_ATOM + "/B) and feeRate (" + CURRENCY_UNIT + "/kB)"); + } if (options.exists("conf_target")) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate"); + throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate."); } if (options.exists("estimate_mode")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and feeRate"); } - coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"])); + CFeeRate fee_rate(AmountFromValue(options["feeRate"])); + if (fee_rate <= CFeeRate(0)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid feeRate %s (must be greater than 0)", fee_rate.ToString(FeeEstimateMode::DASH_KB))); + } + coinControl.m_feerate = fee_rate; coinControl.fOverrideFeeRate = true; } if (options.exists("subtractFeeFromOutputs") || options.exists("subtract_fee_from_outputs") ) subtractFeeFromOutputs = (options.exists("subtract_fee_from_outputs") ? options["subtract_fee_from_outputs"] : options["subtractFeeFromOutputs"]).get_array(); - SetFeeEstimateMode(wallet, coinControl, options["estimate_mode"], options["conf_target"]); - + SetFeeEstimateMode(wallet, coinControl, options["conf_target"], options["estimate_mode"], options["fee_rate"], override_min_fee); } } else { // if options is null and not a bool @@ -3526,7 +3544,8 @@ static RPCHelpMan fundrawtransaction() "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, {"lockUnspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"}, - {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set: makes wallet determine the fee", "Set a specific fee rate in " + CURRENCY_UNIT + "/kB"}, + {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, + {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_UNIT + "/kB."}, {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* default */ "empty array", "The integers.\n" "The fee will be equally deducted from the amount of each specified output.\n" "Those recipients will receive less Dash than you enter in their corresponding amount field.\n" @@ -3535,8 +3554,7 @@ static RPCHelpMan fundrawtransaction() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" - "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, }, @@ -3578,7 +3596,7 @@ static RPCHelpMan fundrawtransaction() CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. coin_control.m_add_inputs = true; - FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control); + FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /* override_min_fee */ true); UniValue result(UniValue::VOBJ); result.pushKV("hex", EncodeHexTx(CTransaction(tx))); @@ -4180,10 +4198,10 @@ static RPCHelpMan send() }, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" - "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, + {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", { {"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "If inputs are specified, automatically include more if they are not enough."}, @@ -4193,10 +4211,10 @@ static RPCHelpMan send() {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"}, {"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The Dash address to receive the change"}, {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" - "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, + {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, {"include_watching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n" "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n" "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."}, @@ -4231,36 +4249,53 @@ static RPCHelpMan send() } }, RPCExamples{"" - "\nSend with a fee rate of 1 " + CURRENCY_ATOM + "/B\n" - + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 " + CURRENCY_ATOM + "/B\n") + - "\nCreate a transaction that should confirm the next block, with a specific input, and return result without adding to wallet or broadcasting to the network\n" + "\nSend 0.1 Dash with a confirmation target of 6 blocks in economical fee estimate mode\n" + + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 6 economical\n") + + "Send 0.2 Dash with a fee rate of 1 " + CURRENCY_ATOM + "/B using positional arguments\n" + + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.2}' 0 \"\" 1\n") + + "Send 0.2 Dash with a fee rate of 1 " + CURRENCY_ATOM + "/B using the options argument\n" + + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.2}' '{\"fee_rate\": 1}'\n") + + "Send 0.3 Dash with a fee rate of 25 " + CURRENCY_ATOM + "/B using named arguments\n" + + HelpExampleCli("-named send", "outputs='{\"" + EXAMPLE_ADDRESS[0] + "\": 0.3}' fee_rate=25\n") + + "Create a transaction that should confirm the next block, with a specific input, and return result without adding to wallet or broadcasting to the network\n" + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 economical '{\"add_to_wallet\": false, \"inputs\": [{\"txid\":\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\", \"vout\":1}]}'") }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue -{ - RPCTypeCheck(request.params, { - UniValueType(), // ARR or OBJ, checked later - UniValue::VNUM, - UniValue::VSTR, - UniValue::VOBJ - }, true - ); + { + RPCTypeCheck(request.params, { + UniValueType(), // outputs (ARR or OBJ, checked later) + UniValue::VNUM, // conf_target + UniValue::VSTR, // estimate_mode + UniValue::VNUM, // fee_rate + UniValue::VOBJ, // options + }, true + ); std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; - UniValue options = request.params[3]; - if (options.exists("feeRate") || options.exists("fee_rate") || options.exists("estimate_mode") || options.exists("conf_target")) { + UniValue options{request.params[4].isNull() ? UniValue::VOBJ : request.params[4]}; + if (options.exists("estimate_mode") || options.exists("conf_target")) { if (!request.params[1].isNull() || !request.params[2].isNull()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Use either conf_target and estimate_mode or the options dictionary to control fee rate"); + throw JSONRPCError(RPC_INVALID_PARAMETER, "Pass conf_target and estimate_mode either as arguments or in the options object, but not both"); } } else { options.pushKV("conf_target", request.params[1]); options.pushKV("estimate_mode", request.params[2]); } + if (options.exists("fee_rate")) { + if (!request.params[3].isNull()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Pass the fee_rate either as an argument, or in the options object, but not both"); + } + } else { + options.pushKV("fee_rate", request.params[3]); + } if (!options["conf_target"].isNull() && (options["estimate_mode"].isNull() || (options["estimate_mode"].get_str() == "unset"))) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify estimate_mode"); } + if (options.exists("feeRate")) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Use fee_rate (" + CURRENCY_ATOM + "/B) instead of feeRate"); + } if (options.exists("changeAddress")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Use change_address"); } @@ -4286,7 +4321,7 @@ static RPCHelpMan send() // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_add_inputs = rawTx.vin.size() == 0; - FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control); + FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /* override_min_fee */ false); bool add_to_wallet = true; if (options.exists("add_to_wallet")) { @@ -4530,6 +4565,7 @@ static RPCHelpMan walletcreatefundedpsbt() {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, {"includeWatching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only"}, {"lockUnspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"}, + {"fee_rate", RPCArg::Type::AMOUNT, /* default */ "not set, fall back to wallet fee estimation", "Specify a fee rate in " + CURRENCY_ATOM + "/B."}, {"feeRate", RPCArg::Type::AMOUNT, /* default */ "not set: makes wallet determine the fee", "Set a specific fee rate in " + CURRENCY_UNIT + "/kB"}, {"subtractFeeFromOutputs", RPCArg::Type::ARR, /* default */ "empty array", "The outputs to subtract the fee from.\n" "The fee will be equally deducted from the amount of each specified output.\n" @@ -4539,8 +4575,7 @@ static RPCHelpMan walletcreatefundedpsbt() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, - {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n" - "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"}, + {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, }, @@ -4585,7 +4620,7 @@ static RPCHelpMan walletcreatefundedpsbt() // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_add_inputs = rawTx.vin.size() == 0; - FundTransaction(*pwallet, rawTx, fee, change_position, request.params[3], coin_control); + FundTransaction(*pwallet, rawTx, fee, change_position, request.params[3], coin_control, /* override_min_fee */ true); // Make a blank psbt PartiallySignedTransaction psbtx{rawTx}; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 298fa3739a5d4..9419b14708bdd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3555,7 +3555,7 @@ bool CWallet::CreateTransactionInternal( // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly // provided one if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) { - error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(), nFeeRateNeeded.ToString()); + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), nFeeRateNeeded.ToString(FeeEstimateMode::DUFF_B)); return false; } diff --git a/test/functional/rpc_estimatefee.py b/test/functional/rpc_estimatefee.py index a76d9eaf8e200..51b7efb4c31a3 100755 --- a/test/functional/rpc_estimatefee.py +++ b/test/functional/rpc_estimatefee.py @@ -27,7 +27,7 @@ def run_test(self): # wrong type for estimatesmartfee(estimate_mode) assert_raises_rpc_error(-3, "Expected type string, got number", self.nodes[0].estimatesmartfee, 1, 1) - assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", self.nodes[0].estimatesmartfee, 1, 'foo') + assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', self.nodes[0].estimatesmartfee, 1, 'foo') # wrong type for estimaterawfee(threshold) assert_raises_rpc_error(-3, "Expected type number, got string", self.nodes[0].estimaterawfee, 1, 'foo') diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 6ac389e53f10b..eb78dfb395dc0 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -94,7 +94,6 @@ def run_test(self): self.test_op_return() self.test_watchonly() self.test_all_watched_funds() - self.test_feerate_with_conf_target_and_estimate_mode() self.test_option_feerate() self.test_address_reuse() self.test_option_subtract_fee_from_outputs() @@ -704,74 +703,86 @@ def test_all_watched_funds(self): wwatch.unloadwallet() def test_option_feerate(self): - self.log.info("Test fundrawtxn feeRate option") - - # Make sure there is exactly one input so coin selection can't skew the result. - assert_equal(len(self.nodes[3].listunspent(1)), 1) - - inputs = [] - outputs = {self.nodes[3].getnewaddress() : 1} - rawtx = self.nodes[3].createrawtransaction(inputs, outputs) - result = self.nodes[3].fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) - result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) - result3 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 10 * self.min_relay_tx_fee}) - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[3].fundrawtransaction, rawtx, {"feeRate": 1}) - result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) - assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) - assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) - - def test_feerate_with_conf_target_and_estimate_mode(self): - self.log.info("Test fundrawtxn passing an explicit fee rate using conf_target and estimate_mode") + self.log.info("Test fundrawtxn with explicit fee rates (fee_rate duff/B and feeRate DASH/kB)") node = self.nodes[3] # Make sure there is exactly one input so coin selection can't skew the result. - assert_equal(len(node.listunspent(1)), 1) + assert_equal(len(self.nodes[3].listunspent(1)), 1) inputs = [] outputs = {node.getnewaddress() : 1} rawtx = node.createrawtransaction(inputs, outputs) - for unit, fee_rate in {"dash/kb": 0.1, "duff/b": 10000}.items(): - self.log.info("Test fundrawtxn with conf_target {} estimate_mode {} produces expected fee".format(fee_rate, unit)) - # With no arguments passed, expect fee of 225 sats/b. - assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000225, vspan=0.00000001) - # Expect fee to be 10,000x higher when explicit fee 10,000x greater is specified. - result = node.fundrawtransaction(rawtx, {"conf_target": fee_rate, "estimate_mode": unit}) - assert_approx(result["fee"], vexp=0.0225, vspan=0.0001) + result = node.fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) + btc_kvb_to_sat_vb = 100000 # (1e5) + result1 = node.fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) + result2 = node.fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) + result3 = node.fundrawtransaction(rawtx, {"fee_rate": 10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) + result4 = node.fundrawtransaction(rawtx, {"feeRate": 10 * self.min_relay_tx_fee}) + result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) + assert_fee_amount(result1['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) + assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) + assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) + assert_fee_amount(result4['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) - for field, fee_rate in {"conf_target": 0.1, "estimate_mode": "duff/b"}.items(): - self.log.info("Test fundrawtxn raises RPC error if both feeRate and {} are passed".format(field)) - assert_raises_rpc_error( - -8, "Cannot specify both {} and feeRate".format(field), - lambda: node.fundrawtransaction(rawtx, {"feeRate": 0.1, field: fee_rate})) + # With no arguments passed, expect fee of 225 satoshis. + assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000225, vspan=0.00000001) + # Expect fee to be 10,000x higher when an explicit fee rate 10,000x greater is specified. + result = node.fundrawtransaction(rawtx, {"fee_rate": 10000}) + assert_approx(result["fee"], vexp=0.0225, vspan=0.0001) self.log.info("Test fundrawtxn with invalid estimate_mode settings") for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k), - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": v, "conf_target": 0.1})) - for mode in ["foo", Decimal("3.141592")]: - assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": 0.1})) + node.fundrawtransaction, rawtx, {"estimate_mode": v, "conf_target": 0.1, "add_inputs": True}) + for mode in ["", "foo", Decimal("3.141592")]: + assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', + node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": 0.1, "add_inputs": True}) self.log.info("Test fundrawtxn with invalid conf_target settings") - for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + for mode in ["unset", "economical", "conservative"]: self.log.debug("{}".format(mode)) for k, v in {"string": "", "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, "Expected type number for conf_target, got {}".format(k), - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": v})) - if mode in ["dash/kb", "duff/b"]: - assert_raises_rpc_error(-3, "Amount out of range", - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": -1})) - assert_raises_rpc_error(-4, "Fee rate (0.00000000 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": 0})) - else: - for n in [-1, 0, 1009]: - assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": n})) - - for unit, fee_rate in {"duff/B": 0.99999999, "DASH/kB": 0.00000999}.items(): - self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) - assert_raises_rpc_error(-4, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", - lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True})) - + node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": v, "add_inputs": True}) + for n in [-1, 0, 1009]: + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h + node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": n, "add_inputs": True}) + + self.log.info("Test invalid fee rate settings") + assert_raises_rpc_error(-8, "Invalid fee_rate 0.000 duff/B (must be greater than 0)", + node.fundrawtransaction, rawtx, {"fee_rate": 0, "add_inputs": True}) + assert_raises_rpc_error(-8, "Invalid feeRate 0.00000000 DASH/kB (must be greater than 0)", + node.fundrawtransaction, rawtx, {"feeRate": 0, "add_inputs": True}) + for param, value in {("fee_rate", 100000), ("feeRate", 1.000)}: + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", + node.fundrawtransaction, rawtx, {param: value, "add_inputs": True}) + assert_raises_rpc_error(-3, "Amount out of range", + node.fundrawtransaction, rawtx, {"fee_rate": -1, "add_inputs": True}) + assert_raises_rpc_error(-3, "Amount is not a number or string", + node.fundrawtransaction, rawtx, {"fee_rate": {"foo": "bar"}, "add_inputs": True}) + assert_raises_rpc_error(-3, "Invalid amount", + node.fundrawtransaction, rawtx, {"fee_rate": "", "add_inputs": True}) + + self.log.info("Test min fee rate checks are bypassed with fundrawtxn, e.g. a fee_rate under 1 sat/vB is allowed") + node.fundrawtransaction(rawtx, {"fee_rate": 0.99999999, "add_inputs": True}) + node.fundrawtransaction(rawtx, {"feeRate": 0.00000999, "add_inputs": True}) + + self.log.info("- raises RPC error if both feeRate and fee_rate are passed") + assert_raises_rpc_error(-8, "Cannot specify both fee_rate (duff/B) and feeRate (DASH/kB)", + node.fundrawtransaction, rawtx, {"fee_rate": 0.1, "feeRate": 0.1, "add_inputs": True}) + + self.log.info("- raises RPC error if both feeRate and estimate_mode passed") + assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and feeRate", + node.fundrawtransaction, rawtx, {"estimate_mode": "economical", "feeRate": 0.1, "add_inputs": True}) + + for param in ["feeRate", "fee_rate"]: + self.log.info("- raises RPC error if both {} and conf_target are passed".format(param)) + assert_raises_rpc_error(-8, "Cannot specify both conf_target and {}. Please provide either a confirmation " + "target in blocks for automatic fee estimation, or an explicit fee rate.".format(param), + node.fundrawtransaction, rawtx, {param: 1, "conf_target": 1, "add_inputs": True}) + + self.log.info("- raises RPC error if both fee_rate and estimate_mode are passed") + assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and fee_rate", + node.fundrawtransaction, rawtx, {"fee_rate": 1, "estimate_mode": "economical", "add_inputs": True}) def test_address_reuse(self): """Test no address reuse occurs.""" @@ -799,12 +810,32 @@ def test_option_subtract_fee_from_outputs(self): outputs = {self.nodes[2].getnewaddress(): 1} rawtx = self.nodes[3].createrawtransaction(inputs, outputs) + # Test subtract fee from outputs with feeRate (BTC/kvB) result = [self.nodes[3].fundrawtransaction(rawtx), # uses self.min_relay_tx_fee (set by settxfee) self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee) self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}), self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee, "subtractFeeFromOutputs": [0]}),] + dec_tx = [self.nodes[3].decoderawtransaction(tx_['hex']) for tx_ in result] + output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)] + change = [d['vout'][r['changepos']]['value'] for d, r in zip(dec_tx, result)] + + assert_equal(result[0]['fee'], result[1]['fee'], result[2]['fee']) + assert_equal(result[3]['fee'], result[4]['fee']) + assert_equal(change[0], change[1]) + assert_equal(output[0], output[1]) + assert_equal(output[0], output[2] + result[2]['fee']) + assert_equal(change[0] + result[0]['fee'], change[2]) + assert_equal(output[3], output[4] + result[4]['fee']) + assert_equal(change[3] + result[3]['fee'], change[4]) + # Test subtract fee from outputs with fee_rate (sat/vB) + btc_kvb_to_sat_vb = 100000 # (1e5) + result = [self.nodes[3].fundrawtransaction(rawtx), # uses self.min_relay_tx_fee (set by settxfee) + self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list + self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee) + self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}), + self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee, "subtractFeeFromOutputs": [0]}),] dec_tx = [self.nodes[3].decoderawtransaction(tx_['hex']) for tx_ in result] output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)] change = [d['vout'][r['changepos']]['value'] for d, r in zip(dec_tx, result)] diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index e6753c1564935..2bdf1d8b7fbe5 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -108,60 +108,74 @@ def run_test(self): assert_equal(walletprocesspsbt_out['complete'], True) self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex']) - self.log.info("Test walletcreatefundedpsbt feeRate of 0.1 DASH/kB produces a total fee at or slightly below -maxtxfee (~0.06650000)") - res = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True}) - assert_approx(res["fee"], 0.04, 0.03) - - self.log.info("Test walletcreatefundedpsbt explicit fee rate with conf_target and estimate_mode") - for unit, fee_rate in {"dash/kb": 0.1, "duff/b": 10000}.items(): - fee = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"conf_target": fee_rate, "estimate_mode": unit, "add_inputs": True})["fee"] - self.log.info("- conf_target {}, estimate_mode {} produces fee {} at or slightly below -maxtxfee (~0.05290000)".format(fee_rate, unit, fee)) - assert_approx(fee, vexp=0.04, vspan=0.03) - - for field, fee_rate in {"conf_target": 0.1, "estimate_mode": "duff/b"}.items(): - self.log.info("- raises RPC error if both feeRate and {} are passed".format(field)) - assert_raises_rpc_error(-8, "Cannot specify both {} and feeRate".format(field), - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, field: fee_rate, "add_inputs": True})) + self.log.info("Test walletcreatefundedpsbt fee rate of 10000 sat/vB and 0.1 BTC/kvB produces a total fee at or slightly below -maxtxfee (~0.05290000)") + res1 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 10000, "add_inputs": True}) + assert_approx(res1["fee"], 0.04, 0.005) + res2 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True}) + assert_approx(res2["fee"], 0.04, 0.005) + self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed, e.g. a fee_rate under 1 sat/vB is allowed") + res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 0.99999999, "add_inputs": True}) + assert_approx(res3["fee"], 0.00000224, 0.0000001) + res4 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.00000999, "add_inputs": True}) + assert_approx(res4["fee"], 0.00000224, 0.0000001) + + self.log.info("Test invalid fee rate settings") + assert_raises_rpc_error(-8, "Invalid fee_rate 0.000 duff/B (must be greater than 0)", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": 0, "add_inputs": True}) + assert_raises_rpc_error(-8, "Invalid feeRate 0.00000000 DASH/kB (must be greater than 0)", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"feeRate": 0, "add_inputs": True}) + for param, value in {("fee_rate", 100000), ("feeRate", 1)}: + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: value, "add_inputs": True}) + assert_raises_rpc_error(-3, "Amount out of range", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": -1, "add_inputs": True}) + assert_raises_rpc_error(-3, "Amount is not a number or string", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": {"foo": "bar"}, "add_inputs": True}) + assert_raises_rpc_error(-3, "Invalid amount", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": "", "add_inputs": True}) + + self.log.info("- raises RPC error if both feeRate and fee_rate are passed") + assert_raises_rpc_error(-8, "Cannot specify both fee_rate (duff/B) and feeRate (DASH/kB)", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": 0.1, "feeRate": 0.1, "add_inputs": True}) + + self.log.info("- raises RPC error if both feeRate and estimate_mode passed") + assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and feeRate", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"estimate_mode": "economical", "feeRate": 0.1, "add_inputs": True}) + + for param in ["feeRate", "fee_rate"]: + self.log.info("- raises RPC error if both {} and conf_target are passed".format(param)) + assert_raises_rpc_error(-8, "Cannot specify both conf_target and {}. Please provide either a confirmation " + "target in blocks for automatic fee estimation, or an explicit fee rate.".format(param), + self.nodes[1].walletcreatefundedpsbt ,inputs, outputs, 0, {param: 1, "conf_target": 1, "add_inputs": True}) + + self.log.info("- raises RPC error if both fee_rate and estimate_mode are passed") + assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and fee_rate", + self.nodes[1].walletcreatefundedpsbt ,inputs, outputs, 0, {"fee_rate": 1, "estimate_mode": "economical", "add_inputs": True}) self.log.info("- raises RPC error with invalid estimate_mode settings") for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k), - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": v, "conf_target": 0.1, "add_inputs": True})) - for mode in ["foo", Decimal("3.141592")]: - assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": 0.1, "add_inputs": True})) - - self.log.info("- raises RPC error if estimate_mode is passed without a conf_target") - for unit in ["DUFF/B", "DASH/KB"]: - assert_raises_rpc_error(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit), - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": unit})) + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"estimate_mode": v, "conf_target": 0.1, "add_inputs": True}) + for mode in ["", "foo", Decimal("3.141592")]: + assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"estimate_mode": mode, "conf_target": 0.1, "add_inputs": True}) self.log.info("- raises RPC error with invalid conf_target settings") - for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + for mode in ["unset", "economical", "conservative"]: self.log.debug("{}".format(mode)) for k, v in {"string": "", "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, "Expected type number for conf_target, got {}".format(k), - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": v, "add_inputs": True})) - if mode in ["dash/kb", "duff/b"]: - assert_raises_rpc_error(-3, "Amount out of range", - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": -1, "add_inputs": True})) - assert_raises_rpc_error(-4, "Fee rate (0.00000000 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": 0, "add_inputs": True})) - else: - for n in [-1, 0, 1009]: - assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": mode, "conf_target": n, "add_inputs": True})) - - for unit, fee_rate in {"DUFF/B": 0.99999999, "DASH/KB": 0.00000999}.items(): - self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) - assert_raises_rpc_error(-4, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", - lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True})) - - self.log.info("Test walletcreatefundedpsbt feeRate of 10 DASH/kB produces total fee well above -maxtxfee and raises RPC error") + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"estimate_mode": mode, "conf_target": v, "add_inputs": True}) + for n in [-1, 0, 1009]: + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"estimate_mode": mode, "conf_target": n, "add_inputs": True}) + + self.log.info("Test walletcreatefundedpsbt with too-high fee rate produces total fee well above -maxtxfee and raises RPC error") # previously this was silently capped at -maxtxfee for bool_add, outputs_array in {True: outputs, False: [{self.nodes[1].getnewaddress(): 1}]}.items(): - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs_array, 0, {"feeRate": 10, "add_inputs": bool_add}) + msg = "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)" + assert_raises_rpc_error(-4, msg, self.nodes[1].walletcreatefundedpsbt, inputs, outputs_array, 0, {"fee_rate": 1000000, "add_inputs": bool_add}) + assert_raises_rpc_error(-4, msg, self.nodes[1].walletcreatefundedpsbt, inputs, outputs_array, 0, {"feeRate": 1, "add_inputs": bool_add}) self.log.info("Test various PSBT operations") # partially sign multisig things with node 1 diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 7621dd05a6e39..e3b48264d4bef 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -4,6 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the wallet.""" from decimal import Decimal +from itertools import product from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework @@ -16,6 +17,8 @@ ) from test_framework.wallet_util import test_address +OUT_OF_RANGE = "Amount out of range" + class WalletTest(BitcoinTestFramework): def set_test_params(self): @@ -50,6 +53,9 @@ def check_fee_amount(self, curr_balance, balance_with_fee, fee_per_byte, tx_size assert_fee_amount(fee, tx_size, fee_per_byte * 1000) return curr_balance + def get_vsize(self, txn): + return self.nodes[0].decoderawtransaction(txn)['size'] + def run_test(self): # Check that there's no UTXO on none of the nodes @@ -79,7 +85,7 @@ def run_test(self): assert_equal(len(self.nodes[1].listunspent()), 1) assert_equal(len(self.nodes[2].listunspent()), 0) - self.log.info("test gettxout") + self.log.info("Test gettxout") confirmed_txid, confirmed_index = utxos[0]["txid"], utxos[0]["vout"] # First, outputs that are unspent both in the chain and in the # mempool should appear with or without include_mempool @@ -93,7 +99,7 @@ def run_test(self): self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 110) mempool_txid = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 100) - self.log.info("test gettxout (second part)") + self.log.info("Test gettxout (second part)") # utxo spent in mempool should be visible if you exclude mempool # but invisible if you include mempool txout = self.nodes[0].gettxout(confirmed_txid, confirmed_index, False) @@ -239,67 +245,45 @@ def run_test(self): assert_equal(self.nodes[2].getbalance(), node_2_bal) node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('100'), fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex'])) - self.start_node(3, self.nodes[3].extra_args) - self.connect_nodes(0, 3) - self.log.info("Test case-insensitive explicit fee rate (sendmany as DASH/kB)") - # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode dash/kB requires a fee rate to be specified in conf_target", - self.nodes[2].sendmany, - amounts={ address: 10 }, - estimate_mode='dash/kB') - # Throw if negative feerate - assert_raises_rpc_error(-3, "Amount out of range", - self.nodes[2].sendmany, - amounts={ address: 10 }, - conf_target=-1, - estimate_mode='dash/kB') - fee_per_kb = 0.0002500 - explicit_fee_per_byte = Decimal(fee_per_kb) / 1000 - txid = self.nodes[2].sendmany( - amounts={ address: 10 }, - conf_target=fee_per_kb, - estimate_mode='dash/kB', - ) - self.nodes[2].generate(1) - self.sync_all(self.nodes[0:3]) - node_2_bal = self.check_fee_amount(self.nodes[2].getbalance(), node_2_bal - Decimal('10'), explicit_fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex'])) - assert_equal(self.nodes[2].getbalance(), node_2_bal) - node_0_bal += Decimal('10') - assert_equal(self.nodes[0].getbalance(), node_0_bal) + self.log.info("Test sendmany with fee_rate param (explicit fee rate in duff/B)") + fee_rate_sat_vb = 2 + fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 + explicit_fee_rate_btc_kvb = Decimal(fee_rate_btc_kvb) / 1000 - self.log.info("Test case-insensitive explicit fee rate (sendmany as duff/B)") - # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode duff/b requires a fee rate to be specified in conf_target", - self.nodes[2].sendmany, - amounts={ address: 10 }, - estimate_mode='duff/b') - # Throw if negative feerate - assert_raises_rpc_error(-3, "Amount out of range", - self.nodes[2].sendmany, - amounts={ address: 10 }, - conf_target=-1, - estimate_mode='duff/b') - fee_duff_per_b = 2 - fee_per_kb = fee_duff_per_b / 100000.0 - explicit_fee_per_byte = Decimal(fee_per_kb) / 1000 - txid = self.nodes[2].sendmany( - amounts={ address: 10 }, - conf_target=fee_duff_per_b, - estimate_mode='duff/b', - ) + # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. + txid = self.nodes[2].sendmany(amounts={address: 10}, conf_target=0, estimate_mode="", fee_rate=fee_rate_sat_vb) self.nodes[2].generate(1) self.sync_all(self.nodes[0:3]) balance = self.nodes[2].getbalance() - node_2_bal = self.check_fee_amount(balance, node_2_bal - Decimal('10'), explicit_fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex'])) + node_2_bal = self.check_fee_amount(balance, node_2_bal - Decimal('10'), explicit_fee_rate_btc_kvb, self.get_vsize(self.nodes[2].gettransaction(txid)['hex'])) assert_equal(balance, node_2_bal) node_0_bal += Decimal('10') assert_equal(self.nodes[0].getbalance(), node_0_bal) + for key in ["totalFee", "feeRate"]: + assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1) + # Test setting explicit fee rate just below the minimum. - for unit, fee_rate in {"DASH/kB": 0.00000999, "duff/B": 0.99999999}.items(): - self.log.info("Test sendmany raises 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) - assert_raises_rpc_error(-6, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", - self.nodes[2].sendmany, amounts={address: 10}, estimate_mode=unit, conf_target=fee_rate) + self.log.info("Test sendmany raises 'fee rate too low' if fee_rate of 0.99999999 is passed") + assert_raises_rpc_error(-6, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", + self.nodes[2].sendmany, amounts={address: 10}, fee_rate=0.99999999) + + self.log.info("Test sendmany raises if fee_rate of 0 or -1 is passed") + assert_raises_rpc_error(-6, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", + self.nodes[2].sendmany, amounts={address: 10}, fee_rate=0) + assert_raises_rpc_error(-3, OUT_OF_RANGE, self.nodes[2].sendmany, amounts={address: 10}, fee_rate=-1) + + self.log.info("Test sendmany raises if an invalid conf_target or estimate_mode is passed") + for target, mode in product([-1, 0, 1009], ["economical", "conservative"]): + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h + self.nodes[2].sendmany, amounts={address: 1}, conf_target=target, estimate_mode=mode) + for target, mode in product([-1, 0], ["btc/kb", "sat/b"]): + assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', + self.nodes[2].sendmany, amounts={address: 1}, conf_target=target, estimate_mode=mode) + + self.start_node(3, self.nodes[3].extra_args) + self.connect_nodes(0, 3) + self.sync_all() # check if we can list zero value tx as available coins # 1. create raw_tx @@ -328,7 +312,7 @@ def run_test(self): assert_equal(uTx['amount'], Decimal('0')) assert found - # do some -walletbroadcast tests + self.log.info("Test -walletbroadcast") self.stop_nodes() self.start_node(0, ["-walletbroadcast=0"]) self.start_node(1, ["-walletbroadcast=0"]) @@ -388,7 +372,7 @@ def run_test(self): # General checks for errors from incorrect inputs # This will raise an exception because the amount is negative - assert_raises_rpc_error(-3, "Amount out of range", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "-1") + assert_raises_rpc_error(-3, OUT_OF_RANGE, self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "-1") # This will raise an exception because the amount type is wrong assert_raises_rpc_error(-3, "Invalid amount", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "1f-4") @@ -430,78 +414,43 @@ def run_test(self): self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) - self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as DASH/kB)") - self.nodes[0].generate(1) - self.sync_all(self.nodes[0:3]) + self.log.info("Test sendtoaddress with fee_rate param (explicit fee rate in sat/vB)") prebalance = self.nodes[2].getbalance() assert prebalance > 2 address = self.nodes[1].getnewaddress() - # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode dash/Kb requires a fee rate to be specified in conf_target", - self.nodes[2].sendtoaddress, - address=address, - amount=1.0, - estimate_mode='dash/Kb') - # Throw if negative feerate - assert_raises_rpc_error(-3, "Amount out of range", - self.nodes[2].sendtoaddress, - address=address, - amount=1.0, - conf_target=-1, - estimate_mode='dash/kb') - txid = self.nodes[2].sendtoaddress( - address=address, - amount=1.0, - conf_target=0.00002500, - estimate_mode='dash/kb', - ) - tx_size = count_bytes(self.nodes[2].gettransaction(txid)['hex']) - self.sync_all(self.nodes[0:3]) - self.nodes[0].generate(1) - self.sync_all(self.nodes[0:3]) - postbalance = self.nodes[2].getbalance() - fee = prebalance - postbalance - Decimal('1') - assert_fee_amount(fee, tx_size, Decimal('0.00002500')) - - self.sync_all(self.nodes[0:3]) + amount = 3 + fee_rate_sat_vb = 2 + fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 - self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as duff/B)") - self.nodes[0].generate(1) - prebalance = self.nodes[2].getbalance() - assert prebalance > 2 - address = self.nodes[1].getnewaddress() - # Throw if no conf_target provided - assert_raises_rpc_error(-8, "Selected estimate_mode duff/b requires a fee rate to be specified in conf_target", - self.nodes[2].sendtoaddress, - address=address, - amount=1.0, - estimate_mode='duff/b') - # Throw if negative feerate - assert_raises_rpc_error(-3, "Amount out of range", - self.nodes[2].sendtoaddress, - address=address, - amount=1.0, - conf_target=-1, - estimate_mode='duff/b') - txid = self.nodes[2].sendtoaddress( - address=address, - amount=1.0, - conf_target=2, - estimate_mode='duff/B', - ) - tx_size = count_bytes(self.nodes[2].gettransaction(txid)['hex']) - self.sync_all(self.nodes[0:3]) + # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. + txid = self.nodes[2].sendtoaddress(address=address, amount=amount, conf_target=0, estimate_mode="", fee_rate=fee_rate_sat_vb) + tx_size = self.get_vsize(self.nodes[2].gettransaction(txid)['hex']) self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) postbalance = self.nodes[2].getbalance() - fee = prebalance - postbalance - Decimal('1') - assert_fee_amount(fee, tx_size, Decimal('0.00002000')) + fee = prebalance - postbalance - Decimal(amount) + assert_fee_amount(fee, tx_size, Decimal(fee_rate_btc_kvb)) + + for key in ["totalFee", "feeRate"]: + assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1) # Test setting explicit fee rate just below the minimum. - for unit, fee_rate in {"DASH/kB": 0.00000999, "sat/B": 0.99999999}.items(): - self.log.info("Test sendtoaddress raises 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) - assert_raises_rpc_error(-6, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)", - self.nodes[2].sendtoaddress, address=address, amount=1, estimate_mode=unit, conf_target=fee_rate) + self.log.info("Test sendtoaddress raises 'fee rate too low' if fee_rate of 0.99999999 is passed") + assert_raises_rpc_error(-6, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", + self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=0.999) + + self.log.info("Test sendtoaddress raises if fee_rate of 0 or -1 is passed") + assert_raises_rpc_error(-6, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", + self.nodes[2].sendtoaddress, address=address, amount=10, fee_rate=0) + assert_raises_rpc_error(-3, OUT_OF_RANGE, self.nodes[2].sendtoaddress, address=address, amount=1.0, fee_rate=-1) + + self.log.info("Test sendtoaddress raises if an invalid conf_target or estimate_mode is passed") + for target, mode in product([-1, 0, 1009], ["economical", "conservative"]): + assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h + self.nodes[2].sendtoaddress, address=address, amount=1, conf_target=target, estimate_mode=mode) + for target, mode in product([-1, 0], ["dash/kb", "duff/b"]): + assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', + self.nodes[2].sendtoaddress, address=address, amount=1, conf_target=target, estimate_mode=mode) # 2. Import address from node2 to node1 self.nodes[1].importaddress(address_to_import) @@ -559,7 +508,7 @@ def run_test(self): ] chainlimit = 6 for m in maintenance: - self.log.info("check " + m) + self.log.info("Test " + m) self.stop_nodes() # set lower ancestor limit for later self.start_node(0, [m, "-limitancestorcount=" + str(chainlimit)]) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 734ed9d13dcae..1eef86be90ecd 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -5,6 +5,8 @@ """Test the send RPC command.""" from decimal import Decimal, getcontext +from itertools import product + from test_framework.authproxy import JSONRPCException from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework @@ -12,7 +14,7 @@ assert_equal, assert_fee_amount, assert_greater_than, - assert_raises_rpc_error + assert_raises_rpc_error, ) class WalletSendTest(BitcoinTestFramework): @@ -29,8 +31,8 @@ def skip_test_if_missing_module(self): self.skip_if_no_wallet() def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, - arg_conf_target=None, arg_estimate_mode=None, - conf_target=None, estimate_mode=None, add_to_wallet=None, psbt=None, + arg_conf_target=None, arg_estimate_mode=None, arg_fee_rate=None, + conf_target=None, estimate_mode=None, fee_rate=None, add_to_wallet=None, psbt=None, inputs=None, add_inputs=None, include_unsafe=None, change_address=None, change_position=None, include_watching=None, locktime=None, lock_unspents=None, subtract_fee_from_outputs=None, expect_error=None): @@ -66,6 +68,8 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, options["conf_target"] = conf_target if estimate_mode is not None: options["estimate_mode"] = estimate_mode + if fee_rate is not None: + options["fee_rate"] = fee_rate if inputs is not None: options["inputs"] = inputs if add_inputs is not None: @@ -89,18 +93,19 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, options = None if expect_error is None: - res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) + res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, fee_rate=arg_fee_rate, options=options) else: try: assert_raises_rpc_error(expect_error[0], expect_error[1], from_wallet.send, - outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) + outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, fee_rate=arg_fee_rate, options=options) except AssertionError: # Provide debug info if the test fails self.log.error("Unexpected successful result:") self.log.error(arg_conf_target) self.log.error(arg_estimate_mode) + self.log.error(arg_fee_rate) self.log.error(options) - res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, options=options) + res = from_wallet.send(outputs=outputs, conf_target=arg_conf_target, estimate_mode=arg_estimate_mode, fee_rate=arg_fee_rate, options=options) self.log.error(res) if "txid" in res and add_to_wallet: self.log.error("Transaction details:") @@ -273,10 +278,10 @@ def run_test(self): assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"]) # but not at the same time - for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: + for mode in ["unset", "economical", "conservative"]: self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical", conf_target=1, estimate_mode=mode, add_to_wallet=False, - expect_error=(-8, "Use either conf_target and estimate_mode or the options dictionary to control fee rate")) + expect_error=(-8, "Pass conf_target and estimate_mode either as arguments or in the options object, but not both")) self.log.info("Create PSBT from watch-only wallet w3, sign with w2...") res = self.test_send(from_wallet=w3, to_wallet=w1, amount=1) @@ -302,60 +307,57 @@ def run_test(self): assert res["complete"] self.log.info("Test setting explicit fee rate") - res1 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical", add_to_wallet=False) - res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=1, estimate_mode="economical", add_to_wallet=False) + res1 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=1, add_to_wallet=False) + res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=1, add_to_wallet=False) assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"]) - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.00007, estimate_mode="dash/kb", add_to_wallet=False) + # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0, estimate_mode="", fee_rate=7, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00007")) - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=2, estimate_mode="duff/b", add_to_wallet=False) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=2, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002")) - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.00004531, arg_estimate_mode="dash/kb", add_to_wallet=False) + # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0, arg_estimate_mode="", arg_fee_rate=4.531, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00004531")) - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=3, arg_estimate_mode="duff/b", add_to_wallet=False) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=3, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00003")) - # TODO: This test should pass with all modes, e.g. with the next line uncommented, for consistency with the other explicit feerate RPCs. - # for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: - for mode in ["dash/kb", "duff/b"]: - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode=mode, - expect_error=(-3, "Amount out of range")) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0, estimate_mode=mode, - expect_error=(-4, "Fee rate (0.00000000 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)")) - - for mode in ["foo", Decimal("3.141592")]: - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode=mode, - expect_error=(-8, "Invalid estimate_mode parameter")) - # TODO: these 2 equivalent sends with an invalid estimate_mode arg should both fail, but they do not...why? - # self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode, - # expect_error=(-8, "Invalid estimate_mode parameter")) - # assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w1.getnewaddress(): 1}, 0.1, mode)) - - # TODO: These tests should pass for consistency with the other explicit feerate RPCs, but they do not. - # for mode in ["unset", "economical", "conservative", "dash/kb", "duff/b"]: - # self.log.debug("{}".format(mode)) - # for k, v in {"string": "", "object": {"foo": "bar"}}.items(): - # self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode, - # expect_error=(-3, "Expected type number for conf_target, got {}".format(k))) - - # TODO: error should use duff/B instead of DASH/kB if duff/B is selected. + # Test that passing fee_rate as both an argument and an option raises. + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=1, fee_rate=1, add_to_wallet=False, + expect_error=(-8, "Pass the fee_rate either as an argument, or in the options object, but not both")) + + assert_raises_rpc_error(-8, "Use fee_rate (duff/B) instead of feeRate", w0.send, {w1.getnewaddress(): 1}, 6, "conservative", 1, {"feeRate": 0.01}) + + assert_raises_rpc_error(-3, "Unexpected key totalFee", w0.send, {w1.getnewaddress(): 1}, 6, "conservative", 1, {"totalFee": 0.01}) + + for target, mode in product([-1, 0, 1009], ["economical", "conservative"]): + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=target, estimate_mode=mode, + expect_error=(-8, "Invalid conf_target, must be between 1 and 1008")) # max value of 1008 per src/policy/fees.h + msg = 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"' + for target, mode in product([-1, 0], ["dash/kb", "dufff/b"]): + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=target, estimate_mode=mode, expect_error=(-8, msg)) + for mode in ["", "foo", Decimal("3.141592")]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode=mode, expect_error=(-8, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode, expect_error=(-8, msg)) + assert_raises_rpc_error(-8, msg, w0.send, {w1.getnewaddress(): 1}, 0.1, mode) + + for mode in ["economical", "conservative", "dash/kb", "duff/b"]: + self.log.debug("{}".format(mode)) + for k, v in {"string": "true", "object": {"foo": "bar"}}.items(): + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode, + expect_error=(-3, "Expected type number for conf_target, got {}".format(k))) + # Test setting explicit fee rate just below the minimum. - for unit, fee_rate in {"duff/B": 0.99999999, "DASH/kB": 0.00000999}.items(): - self.log.info("Explicit fee rate raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit)) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=fee_rate, estimate_mode=unit, - expect_error=(-4, "Fee rate (0.00000999 DASH/kB) is lower than the minimum fee rate setting (0.00001000 DASH/kB)")) - - self.log.info("Explicit fee rate raises RPC error if estimate_mode is passed without a conf_target") - for unit, fee_rate in {"duff/B": 100, "DASH/kB": 0.001}.items(): - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, estimate_mode=unit, - expect_error=(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit))) + self.log.info("Explicit fee rate raises RPC error 'fee rate too low' if fee_rate of 0.99999999 is passed") + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0.99999999, + expect_error=(-4, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) # TODO: Return hex if fee rate is below -maxmempool # res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="duff/b", add_to_wallet=False) From 01e41aa1fb40a1185f9e36c4c5c06036e54c9b83 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 21 Nov 2020 07:47:06 +0100 Subject: [PATCH 04/11] Merge #20426: wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9f08780dd7946b63476e9736745131db8e7f4e93 Use the correct incremental fee constant in bumpfee help (Jon Atack) 3f1e10b2b1cd11f7112fbad6355464bd4adbbc5c Update feeRate (BTC/kvB) to fee_rate (sat/vB) in wallet_bumpfee (Jon Atack) 1b3d7009280595108eb22ac1188bc4367804fc5d Allow zero-fee fundrawtxn and walletcreatefundedpsbt calls (Jon Atack) Pull request description: - Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525406176. A check to raise an error on zero-fee txns was mistakenly extended in a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include fundrawtransaction and walletcreatefundedpsbt. This commit re-overrides zero fee rate checking for these two RPCs, not only for the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new fee_rate (sat/vB) arg. Negative fee rates will still raise "amount out of range" by the MoneyRange check in src/bitcoin-tx.cpp::AmountFromValue. - Updates a wallet bumpfee test from feeRate (BTC/kvB) to fee_rate (sat/vB) - Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525405363 to use the correct incremental fee rate constant in the bumpfee help (thanks Marco Falke for the catch) and rectifies "1.000 sat/vB sat/vB" in the help to "1.000 sat/vB" ACKs for top commit: MarcoFalke: review ACK 9f08780dd7946b63476e9736745131db8e7f4e93 🐾 promag: Code review ACK 9f08780dd7946b63476e9736745131db8e7f4e93. Xekyo: Code review reACK 9f08780dd7946b63476e9736745131db8e7f4e93. Tree-SHA512: 413dfb4f23ebaf3d2ef210dd04610a843272e64eabba428699f5de4d646a86ac4911dab66b5e2f5ebea53b76e4be8347ef40824c1592c750d5eaa12579d3cdf6 --- src/wallet/rpcwallet.cpp | 17 +++-------------- test/functional/rpc_fundrawtransaction.py | 16 +++++++++------- test/functional/rpc_psbt.py | 15 ++++++++------- test/functional/wallet_send.py | 8 +++++++- 4 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 3f00992cf8a6a..fe7250201b9a4 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include #include @@ -224,14 +223,8 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un if (!estimate_mode.isNull() && !estimate_mode.get_str().empty()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate"); } - CFeeRate fee_rate_in_sat_vb{CFeeRate(AmountFromValue(fee_rate), COIN)}; - if (override_min_fee) { - if (fee_rate_in_sat_vb <= CFeeRate(0)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", fee_rate_in_sat_vb.ToString(FeeEstimateMode::DUFF_B))); - } - cc.fOverrideFeeRate = true; - } - cc.m_feerate = fee_rate_in_sat_vb; + cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN); + if (override_min_fee) cc.fOverrideFeeRate = true; return; } if (!estimate_mode.isNull() && !FeeModeFromString(estimate_mode.get_str(), cc.m_fee_mode)) { @@ -3474,11 +3467,7 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, if (options.exists("estimate_mode")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and feeRate"); } - CFeeRate fee_rate(AmountFromValue(options["feeRate"])); - if (fee_rate <= CFeeRate(0)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid feeRate %s (must be greater than 0)", fee_rate.ToString(FeeEstimateMode::DASH_KB))); - } - coinControl.m_feerate = fee_rate; + coinControl.m_feerate = CFeeRate(AmountFromValue(options["feeRate"])); coinControl.fOverrideFeeRate = true; } diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index eb78dfb395dc0..d183ae8cae35c 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -717,11 +717,17 @@ def test_option_feerate(self): result2 = node.fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) result3 = node.fundrawtransaction(rawtx, {"fee_rate": 10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) result4 = node.fundrawtransaction(rawtx, {"feeRate": 10 * self.min_relay_tx_fee}) + # Test that funding non-standard "zero-fee" transactions is valid. + result5 = self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 0}) + result6 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 0}) + result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) assert_fee_amount(result1['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) assert_fee_amount(result4['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) + assert_fee_amount(result5['fee'], count_bytes(result5['hex']), 0) + assert_fee_amount(result6['fee'], count_bytes(result6['hex']), 0) # With no arguments passed, expect fee of 225 satoshis. assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000225, vspan=0.00000001) @@ -748,19 +754,15 @@ def test_option_feerate(self): node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": n, "add_inputs": True}) self.log.info("Test invalid fee rate settings") - assert_raises_rpc_error(-8, "Invalid fee_rate 0.000 duff/B (must be greater than 0)", - node.fundrawtransaction, rawtx, {"fee_rate": 0, "add_inputs": True}) - assert_raises_rpc_error(-8, "Invalid feeRate 0.00000000 DASH/kB (must be greater than 0)", - node.fundrawtransaction, rawtx, {"feeRate": 0, "add_inputs": True}) for param, value in {("fee_rate", 100000), ("feeRate", 1.000)}: assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", node.fundrawtransaction, rawtx, {param: value, "add_inputs": True}) assert_raises_rpc_error(-3, "Amount out of range", - node.fundrawtransaction, rawtx, {"fee_rate": -1, "add_inputs": True}) + node.fundrawtransaction, rawtx, {param: -1, "add_inputs": True}) assert_raises_rpc_error(-3, "Amount is not a number or string", - node.fundrawtransaction, rawtx, {"fee_rate": {"foo": "bar"}, "add_inputs": True}) + node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True}) assert_raises_rpc_error(-3, "Invalid amount", - node.fundrawtransaction, rawtx, {"fee_rate": "", "add_inputs": True}) + node.fundrawtransaction, rawtx, {param: "", "add_inputs": True}) self.log.info("Test min fee rate checks are bypassed with fundrawtxn, e.g. a fee_rate under 1 sat/vB is allowed") node.fundrawtransaction(rawtx, {"fee_rate": 0.99999999, "add_inputs": True}) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 2bdf1d8b7fbe5..8e73e3b8d8234 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -113,26 +113,27 @@ def run_test(self): assert_approx(res1["fee"], 0.04, 0.005) res2 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True}) assert_approx(res2["fee"], 0.04, 0.005) + self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed, e.g. a fee_rate under 1 sat/vB is allowed") res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 0.99999999, "add_inputs": True}) assert_approx(res3["fee"], 0.00000224, 0.0000001) res4 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.00000999, "add_inputs": True}) assert_approx(res4["fee"], 0.00000224, 0.0000001) + self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed and that funding non-standard 'zero-fee' transactions is valid") + for param in ["fee_rate", "feeRate"]: + assert_equal(self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {param: 0, "add_inputs": True})["fee"], 0) + self.log.info("Test invalid fee rate settings") - assert_raises_rpc_error(-8, "Invalid fee_rate 0.000 duff/B (must be greater than 0)", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": 0, "add_inputs": True}) - assert_raises_rpc_error(-8, "Invalid feeRate 0.00000000 DASH/kB (must be greater than 0)", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"feeRate": 0, "add_inputs": True}) for param, value in {("fee_rate", 100000), ("feeRate", 1)}: assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: value, "add_inputs": True}) assert_raises_rpc_error(-3, "Amount out of range", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": -1, "add_inputs": True}) + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: -1, "add_inputs": True}) assert_raises_rpc_error(-3, "Amount is not a number or string", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": {"foo": "bar"}, "add_inputs": True}) + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: {"foo": "bar"}, "add_inputs": True}) assert_raises_rpc_error(-3, "Invalid amount", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": "", "add_inputs": True}) + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: "", "add_inputs": True}) self.log.info("- raises RPC error if both feeRate and fee_rate are passed") assert_raises_rpc_error(-8, "Cannot specify both fee_rate (duff/B) and feeRate (DASH/kB)", diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 1eef86be90ecd..b73dbb1540e35 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -354,10 +354,16 @@ def run_test(self): self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode, expect_error=(-3, "Expected type number for conf_target, got {}".format(k))) - # Test setting explicit fee rate just below the minimum. + # Test setting explicit fee rate just below the minimum and at zero. self.log.info("Explicit fee rate raises RPC error 'fee rate too low' if fee_rate of 0.99999999 is passed") self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0.99999999, expect_error=(-4, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0.99999999, + expect_error=(-4, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0, + expect_error=(-4, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0, + expect_error=(-4, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) # TODO: Return hex if fee rate is below -maxmempool # res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="duff/b", add_to_wallet=False) From db4a2169bb0057145a3d86a458e7e2f27ecc7b73 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 25 Nov 2020 08:02:11 +0100 Subject: [PATCH 05/11] Merge #20410: wallet: Do not treat default constructed types as None-type fa69c2c78455fd0dc436018fece9ff7fc83a180d wallet: Do not treat default constructed types as None-type (MarcoFalke) fac4e136fa3d0fab7fde900a6be921313e16e7a6 refactor: Change pointer to reference because it can not be null (MarcoFalke) Pull request description: Equating `0==None` and `""==None` is confusing, unneeded and undocumented ACKs for top commit: jonatack: ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d achow101: ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d Sjors: tACK fa69c2c78455fd0dc436018fece9ff7fc83a180d modulo `unset` Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac --- src/wallet/rpcwallet.cpp | 28 +++++++++++++--------------- test/functional/wallet_basic.py | 6 ++---- test/functional/wallet_send.py | 9 ++++----- 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index fe7250201b9a4..4784d7339cda5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -203,24 +203,22 @@ static std::string LabelFromValue(const UniValue& value) * Update coin control with fee estimation based on the given parameters * * @param[in] wallet Wallet reference - * @param[in,out] cc Coin control to be updated - * @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h; - * if a fee_rate is present, 0 is allowed here as a no-op positional placeholder - * @param[in] estimate_mode UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative"; - * if a fee_rate is present, "" is allowed here as a no-op positional placeholder - * @param[in] fee_rate UniValue real; fee rate in sat/B; - * if a fee_rate is present, both conf_target and estimate_mode must either be null, or no-op + * @param[in,out] cc Coin control to be updated + * @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h; + * @param[in] estimate_mode UniValue string; fee estimation mode, valid values are "unset", "economical" or "conservative"; + * @param[in] fee_rate UniValue real; fee rate in sat/B; + * if present, both conf_target and estimate_mode must either be null, or no-op or "unset" * @param[in] override_min_fee bool; whether to set fOverrideFeeRate to true to disable minimum fee rate checks and instead - verify only that fee_rate is greater than 0 + * verify only that fee_rate is greater than 0 * @throws a JSONRPCError if conf_target, estimate_mode, or fee_rate contain invalid values or are in conflict */ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, bool override_min_fee) { if (!fee_rate.isNull()) { - if (!conf_target.isNull() && conf_target.get_int() > 0) { + if (!conf_target.isNull()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate."); } - if (!estimate_mode.isNull() && !estimate_mode.get_str().empty()) { + if (!estimate_mode.isNull() && estimate_mode.get_str() != "unset") { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate"); } cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN); @@ -450,8 +448,8 @@ static RPCHelpMan sendtoaddress() + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1") + "\nSend 0.1 Dash with a confirmation target of 6 blocks in economical fee estimate mode using positional arguments\n" + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"donation\" \"sean's outpost\" false false false 6 economical") + - "\nSend 0.1 Dash with a fee rate of 1 " + CURRENCY_ATOM + "/B, subtract fee from amount, use CoinJoin funds only, using positional arguments\n" - + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"drinks\" \"room77\" true false true 0 \"\" 1") + + "\nSend 0.1 Dash with a fee rate of 1.1 " + CURRENCY_ATOM + "/B, subtract fee from amount, use CoinJoin funds only, using positional arguments\n" + + HelpExampleCli("sendtoaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0.1 \"drinks\" \"room77\" true false true 0 \"\" 1.1") + "\nSend 0.2 Dash with a confirmation target of 6 blocks in economical fee estimate mode using named arguments\n" + HelpExampleCli("-named sendtoaddress", "address=\"" + EXAMPLE_ADDRESS[0] + "\" amount=0.2 conf_target=6 estimate_mode=\"economical\"") + "\nSend 0.5 Dash with a fee rate of 25 " + CURRENCY_ATOM + "/B using named arguments\n" @@ -4240,10 +4238,10 @@ static RPCHelpMan send() RPCExamples{"" "\nSend 0.1 Dash with a confirmation target of 6 blocks in economical fee estimate mode\n" + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 6 economical\n") + - "Send 0.2 Dash with a fee rate of 1 " + CURRENCY_ATOM + "/B using positional arguments\n" - + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.2}' 0 \"\" 1\n") + + "Send 0.2 Dash with a fee rate of 1.1 " + CURRENCY_ATOM + "/B using positional arguments\n" + + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.2}' null \"unset\" 1.1\n") + "Send 0.2 Dash with a fee rate of 1 " + CURRENCY_ATOM + "/B using the options argument\n" - + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.2}' '{\"fee_rate\": 1}'\n") + + + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.2}' null \"unset\" null '{\"fee_rate\": 1}'\n") + "Send 0.3 Dash with a fee rate of 25 " + CURRENCY_ATOM + "/B using named arguments\n" + HelpExampleCli("-named send", "outputs='{\"" + EXAMPLE_ADDRESS[0] + "\": 0.3}' fee_rate=25\n") + "Create a transaction that should confirm the next block, with a specific input, and return result without adding to wallet or broadcasting to the network\n" diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index e3b48264d4bef..9af82d19acce4 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -250,8 +250,7 @@ def run_test(self): fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 explicit_fee_rate_btc_kvb = Decimal(fee_rate_btc_kvb) / 1000 - # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. - txid = self.nodes[2].sendmany(amounts={address: 10}, conf_target=0, estimate_mode="", fee_rate=fee_rate_sat_vb) + txid = self.nodes[2].sendmany(amounts={address: 10}, fee_rate=fee_rate_sat_vb) self.nodes[2].generate(1) self.sync_all(self.nodes[0:3]) balance = self.nodes[2].getbalance() @@ -422,8 +421,7 @@ def run_test(self): fee_rate_sat_vb = 2 fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 - # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. - txid = self.nodes[2].sendtoaddress(address=address, amount=amount, conf_target=0, estimate_mode="", fee_rate=fee_rate_sat_vb) + txid = self.nodes[2].sendtoaddress(address=address, amount=amount, fee_rate=fee_rate_sat_vb) tx_size = self.get_vsize(self.nodes[2].gettransaction(txid)['hex']) self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index b73dbb1540e35..ea2d8e2b10891 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -311,17 +311,16 @@ def run_test(self): res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=1, add_to_wallet=False) assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"]) - # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0, estimate_mode="", fee_rate=7, add_to_wallet=False) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=7, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00007")) - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=2, add_to_wallet=False) + # "unset" and None are treated the same for estimate_mode + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=2, estimate_mode="unset", add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002")) - # Passing conf_target 0, estimate_mode "" as placeholder arguments should allow fee_rate to apply. - res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0, arg_estimate_mode="", arg_fee_rate=4.531, add_to_wallet=False) + res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=4.531, add_to_wallet=False) fee = self.nodes[1].decodepsbt(res["psbt"])["fee"] assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00004531")) From 5ad8a489a5fc6f1ef2acb9f297ea325a81866cae Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 10 Dec 2020 08:45:52 +0100 Subject: [PATCH 06/11] Merge #20573: wallet, bugfix: allow send with string fee_rate amounts 6fa72ceb8021c3b5aea62f6cfe92665c29212923 test: add coverage for passing fee rate as a string (Jon Atack) ce207d6b93d35bc02fcd2dd28f1fd95869261d43 wallet, bugfix: allow send to take string fee rate values (Jon Atack) Pull request description: RPC send currently only accepts fee rates as numbers, which is a user-facing bug. It should accept fee rates as an amount, e.g. a string or a number, as documented in its help and like sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, and bumpfee. Provide a fix and regression test coverage. ACKs for top commit: MarcoFalke: review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923 achow101: Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923 promag: Code review ACK 6fa72ceb8021c3b5aea62f6cfe92665c29212923. Tree-SHA512: 735f9269cb1b81953764b5283449c0b154bd62de034225be5bcedc515c84faf767fe8fe0741008679fe412922c847b00d116cb11aab775236b779c847ba87167 --- src/wallet/rpcwallet.cpp | 2 +- test/functional/rpc_fundrawtransaction.py | 4 ++-- test/functional/rpc_psbt.py | 4 ++-- test/functional/wallet_basic.py | 29 +++++++++++++++++++++-- test/functional/wallet_send.py | 4 ++-- 5 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4784d7339cda5..40ede2a706290 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4253,7 +4253,7 @@ static RPCHelpMan send() UniValueType(), // outputs (ARR or OBJ, checked later) UniValue::VNUM, // conf_target UniValue::VSTR, // estimate_mode - UniValue::VNUM, // fee_rate + UniValueType(), // fee_rate, will be checked by AmountFromValue() in SetFeeEstimateMode() UniValue::VOBJ, // options }, true ); diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index d183ae8cae35c..f1054c4aff51d 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -713,10 +713,10 @@ def test_option_feerate(self): result = node.fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) btc_kvb_to_sat_vb = 100000 # (1e5) - result1 = node.fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) + result1 = node.fundrawtransaction(rawtx, {"fee_rate": str(2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee)}) result2 = node.fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) result3 = node.fundrawtransaction(rawtx, {"fee_rate": 10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) - result4 = node.fundrawtransaction(rawtx, {"feeRate": 10 * self.min_relay_tx_fee}) + result4 = node.fundrawtransaction(rawtx, {"feeRate": str(10 * self.min_relay_tx_fee)}) # Test that funding non-standard "zero-fee" transactions is valid. result5 = self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 0}) result6 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 0}) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 8e73e3b8d8234..db3500089b4e0 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -111,11 +111,11 @@ def run_test(self): self.log.info("Test walletcreatefundedpsbt fee rate of 10000 sat/vB and 0.1 BTC/kvB produces a total fee at or slightly below -maxtxfee (~0.05290000)") res1 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 10000, "add_inputs": True}) assert_approx(res1["fee"], 0.04, 0.005) - res2 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True}) + res2 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": "0.1", "add_inputs": True}) assert_approx(res2["fee"], 0.04, 0.005) self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed, e.g. a fee_rate under 1 sat/vB is allowed") - res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": 0.99999999, "add_inputs": True}) + res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": "0.99999999", "add_inputs": True}) assert_approx(res3["fee"], 0.00000224, 0.0000001) res4 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.00000999, "add_inputs": True}) assert_approx(res4["fee"], 0.00000224, 0.0000001) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 9af82d19acce4..013a77078ec11 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -250,7 +250,8 @@ def run_test(self): fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 explicit_fee_rate_btc_kvb = Decimal(fee_rate_btc_kvb) / 1000 - txid = self.nodes[2].sendmany(amounts={address: 10}, fee_rate=fee_rate_sat_vb) + # Test passing fee_rate as a string + txid = self.nodes[2].sendmany(amounts={address: 10}, fee_rate=str(fee_rate_sat_vb)) self.nodes[2].generate(1) self.sync_all(self.nodes[0:3]) balance = self.nodes[2].getbalance() @@ -259,6 +260,17 @@ def run_test(self): node_0_bal += Decimal('10') assert_equal(self.nodes[0].getbalance(), node_0_bal) + # Test passing fee_rate as an integer + amount = Decimal("0.0001") + txid = self.nodes[2].sendmany(amounts={address: amount}, fee_rate=fee_rate_sat_vb) + self.nodes[2].generate(1) + self.sync_all(self.nodes[0:3]) + balance = self.nodes[2].getbalance() + node_2_bal = self.check_fee_amount(balance, node_2_bal - amount, explicit_fee_rate_btc_kvb, self.get_vsize(self.nodes[2].gettransaction(txid)['hex'])) + assert_equal(balance, node_2_bal) + node_0_bal += amount + assert_equal(self.nodes[0].getbalance(), node_0_bal) + for key in ["totalFee", "feeRate"]: assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1) @@ -420,7 +432,7 @@ def run_test(self): amount = 3 fee_rate_sat_vb = 2 fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 - + # Test passing fee_rate as an integer txid = self.nodes[2].sendtoaddress(address=address, amount=amount, fee_rate=fee_rate_sat_vb) tx_size = self.get_vsize(self.nodes[2].gettransaction(txid)['hex']) self.nodes[0].generate(1) @@ -429,6 +441,19 @@ def run_test(self): fee = prebalance - postbalance - Decimal(amount) assert_fee_amount(fee, tx_size, Decimal(fee_rate_btc_kvb)) + prebalance = self.nodes[2].getbalance() + amount = Decimal("0.001") + fee_rate_sat_vb = 1.23 + fee_rate_btc_kvb = fee_rate_sat_vb * 1e3 / 1e8 + # Test passing fee_rate as a string + txid = self.nodes[2].sendtoaddress(address=address, amount=amount, fee_rate=str(fee_rate_sat_vb)) + tx_size = self.get_vsize(self.nodes[2].gettransaction(txid)['hex']) + self.nodes[0].generate(1) + self.sync_all(self.nodes[0:3]) + postbalance = self.nodes[2].getbalance() + fee = prebalance - postbalance - amount + assert_fee_amount(fee, tx_size, Decimal(fee_rate_btc_kvb)) + for key in ["totalFee", "feeRate"]: assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index ea2d8e2b10891..89f9b554a26f2 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -307,8 +307,8 @@ def run_test(self): assert res["complete"] self.log.info("Test setting explicit fee rate") - res1 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=1, add_to_wallet=False) - res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=1, add_to_wallet=False) + res1 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate="1", add_to_wallet=False) + res2 = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate="1", add_to_wallet=False) assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"], self.nodes[1].decodepsbt(res2["psbt"])["fee"]) res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=7, add_to_wallet=False) From 9e9975f83b9635955f8d022004c6de584cc897eb Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Fri, 19 Feb 2021 11:37:57 +1300 Subject: [PATCH 07/11] Merge #21201: rpc: Disallow sendtoaddress and sendmany when private keys disabled 6bfbc97d716faad38c87603ac6049d222236d623 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack) 0997019e7681efb00847a7246c15ac8f235128d8 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow) Pull request description: Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made. Fixes #21104 ACKs for top commit: jonatack: ACK 6bfbc97d716faad38c87603ac6049d222236d623 meshcollider: utACK 6bfbc97d716faad38c87603ac6049d222236d623 kristapsk: ACK 6bfbc97d716faad38c87603ac6049d222236d623. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API. Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb --- src/wallet/rpcwallet.cpp | 8 +++++++- test/functional/wallet_watchonly.py | 7 ++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 40ede2a706290..79343ff86830b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -382,6 +382,12 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto { EnsureWalletIsUnlocked(wallet); + // This function is only used by sendtoaddress and sendmany. + // This should always try to sign, if we don't have private keys, don't try to do anything here. + if (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet"); + } + if (coin_control.IsUsingCoinJoin()) { map_value["DS"] = "1"; } @@ -392,7 +398,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto bilingual_str error; CTransactionRef tx; FeeCalculation fee_calc_out; - bool fCreated = wallet.CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); + const bool fCreated = wallet.CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, true); if (!fCreated) { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original); } diff --git a/test/functional/wallet_watchonly.py b/test/functional/wallet_watchonly.py index 2b91edf22fcfd..c3ac93229f844 100755 --- a/test/functional/wallet_watchonly.py +++ b/test/functional/wallet_watchonly.py @@ -2,7 +2,7 @@ # Copyright (c) 2018-2019 The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. -"""Test createwallet arguments. +"""Test createwallet watchonly arguments. """ from test_framework.blocktools import COINBASE_MATURITY @@ -51,6 +51,11 @@ def run_test(self): assert_equal(len(wo_wallet.listtransactions()), 1) assert_equal(wo_wallet.getbalance(include_watchonly=False), 0) + self.log.info('Test sending from a watch-only wallet raises RPC error') + msg = "Error: Private keys are disabled for this wallet" + assert_raises_rpc_error(-4, msg, wo_wallet.sendtoaddress, a1, 0.1) + assert_raises_rpc_error(-4, msg, wo_wallet.sendmany, amounts={a1: 0.1}) + self.log.info('Testing listreceivedbyaddress watch-only defaults') result = wo_wallet.listreceivedbyaddress() assert_equal(len(result), 1) From ccac35c89c19a20273b1aba5ebb6c4754d533286 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 17 Mar 2021 11:46:08 +1300 Subject: [PATCH 08/11] Merge #21083: wallet: Avoid requesting fee rates multiple times during coin selection f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow) bdd0c2934b7f389ffcfae3b602ee3ecee8581acd wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow) 448d04b931f86941903e855f831249ff5ec77485 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow) e2f429e6bbf7098f278c0247b954ecd3ba53cf37 wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow) 1a6a0b0dfb90f9ebd4b86d7934c6aa5594974f5f wallet: Use existing feerate instead of getting a new one (Andrew Chow) Pull request description: During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail. Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed. While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same. Fixes #19229 ACKs for top commit: glozow: reACK https://github.com/bitcoin/bitcoin/commit/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 fjahr: Code review re-ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 Xekyo: tACK https://github.com/bitcoin/bitcoin/pull/21083/commits/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 meshcollider: Code review + test run ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863 --- src/bench/coin_selection.cpp | 5 ++++- src/wallet/test/coinselector_tests.cpp | 30 +++++++++++++++----------- src/wallet/wallet.cpp | 29 +++++++++++-------------- src/wallet/wallet.h | 16 ++++++++++---- 4 files changed, 45 insertions(+), 35 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 9f1a75a553425..e53755c046c19 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -48,7 +48,10 @@ static void CoinSelection(benchmark::Bench& bench) coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */); } const CoinEligibilityFilter filter_standard(1, 6, 0); - const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0, false); + const CoinSelectionParams coin_selection_params(/* use_bnb= */ true, /* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); bench.run([&] { std::set setCoinsRet; CAmount nValueRet; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 3ec2f7a094dfc..88eb119d7bb8b 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -33,9 +33,10 @@ static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); -CoinSelectionParams coin_selection_params(/* use_bnb = */ false, /* change_output_size = */ 0, - /* change_spend_size = */ 0, /* effective_fee = */ CFeeRate(0), - /* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); +CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0, + /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { @@ -254,9 +255,10 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } // Make sure that effective value is working in SelectCoinsMinConf when BnB is used - CoinSelectionParams coin_selection_params_bnb(/* use_bnb = */ true, /* change_output_size = */ 0, - /* change_spend_size = */ 0, /* effective_fee = */ CFeeRate(3000), - /* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); + CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 0, + /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), + /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); { std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); @@ -298,7 +300,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CCoinControl coin_control; coin_control.fAllowOtherInputs = true; coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i)); - coin_selection_params_bnb.effective_fee = CFeeRate(0); + coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); BOOST_CHECK(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used)); BOOST_CHECK(bnb_used); BOOST_CHECK(coin_selection_params_bnb.use_bnb); @@ -642,12 +644,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) CAmount target = rand.randrange(balance - 1000) + 1000; // Perform selection - CoinSelectionParams coin_selection_params_knapsack(/* use_bnb = */ false, /* change_output_size = */ 34, - /* change_spend_size = */ 148, /* effective_fee = */ CFeeRate(0), - /* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); - CoinSelectionParams coin_selection_params_bnb(/* use_bnb = */ true, /* change_output_size = */ 34, - /* change_spend_size = */ 148, /* effective_fee = */ CFeeRate(0), - /* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); + CoinSelectionParams coin_selection_params_knapsack(/* use_bnb= */ false, /* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); CoinSet out_set; CAmount out_value = 0; bool bnb_used = false; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9419b14708bdd..414eeb16aade7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2872,26 +2872,20 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil nValueRet = 0; if (coin_selection_params.use_bnb) { - // Get long term estimate - FeeCalculation feeCalc; - CCoinControl temp; - temp.m_confirm_target = 1008; - CFeeRate long_term_feerate = GetMinimumFeeRate(*this, temp, &feeCalc); - // Get the feerate for effective value. // When subtracting the fee from the outputs, we want the effective feerate to be 0 CFeeRate effective_feerate{0}; if (!coin_selection_params.m_subtract_fee_outputs) { - effective_feerate = coin_selection_params.effective_fee; + effective_feerate = coin_selection_params.m_effective_feerate; } - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, long_term_feerate, eligibility_filter, true /* positive_only */); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); // Calculate cost of change - CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); + CAmount cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); // Calculate the fees for things that aren't inputs - CAmount not_input_fees = coin_selection_params.effective_fee.GetFee(coin_selection_params.tx_noinputs_size); + CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); bnb_used = true; return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { @@ -2959,7 +2953,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (coin.m_input_bytes <= 0) { return false; // Not solvable, can't estimate size for fee } - coin.effective_value = coin.txout.nValue - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes); + coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); if (coin_selection_params.use_bnb) { value_to_select -= coin.effective_value; } else { @@ -3548,14 +3542,16 @@ bool CWallet::CreateTransactionInternal( CMutableTransaction txNew; FeeCalculation feeCalc; - CFeeRate discard_rate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this); + + CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy + coin_selection_params.m_discard_feerate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this); // Get the fee rate to use effective values in coin selection - CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc); + coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly // provided one - if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) { - error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), nFeeRateNeeded.ToString(FeeEstimateMode::DUFF_B)); + if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::DUFF_B)); return false; } @@ -3568,7 +3564,6 @@ bool CWallet::CreateTransactionInternal( CAmount nAmountAvailable{0}; std::vector vAvailableCoins; AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); - CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy coin_selection_params.use_bnb = false; // never use BnB for (auto out : vAvailableCoins) { @@ -3756,7 +3751,7 @@ bool CWallet::CreateTransactionInternal( // Never create dust outputs; if we would, just // add the dust to the fee. - if (IsDust(newTxOut, discard_rate)) + if (IsDust(newTxOut, coin_selection_params.m_discard_feerate)) { nFeeRet = nFeePrev; nChangePosInOut = -1; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 194d734061595..11824d79ee5ec 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -683,8 +683,13 @@ struct CoinSelectionParams size_t change_output_size = 0; /** Size of the input to spend a change output in virtual bytes. */ size_t change_spend_size = 0; - /** The targeted feerate of the transaction being built. */ - CFeeRate effective_fee = CFeeRate(0); + /** The fee to spend these UTXOs at the long term feerate. */ + CFeeRate m_effective_feerate; + /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend + * the change output sometime in the future. */ + CFeeRate m_long_term_feerate; + /** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */ + CFeeRate m_discard_feerate; size_t tx_noinputs_size = 0; /** Indicate that we are subtracting the fee from outputs */ bool m_subtract_fee_outputs = false; @@ -693,11 +698,14 @@ struct CoinSelectionParams * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */ bool m_avoid_partial_spends = false; - CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size, bool avoid_partial) : + CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate, + CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) : use_bnb(use_bnb), change_output_size(change_output_size), change_spend_size(change_spend_size), - effective_fee(effective_fee), + m_effective_feerate(effective_feerate), + m_long_term_feerate(long_term_feerate), + m_discard_feerate(discard_feerate), tx_noinputs_size(tx_noinputs_size), m_avoid_partial_spends(avoid_partial) {} From 22435f1898b1468b623e61e9574d071c88c4ac2c Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Wed, 5 May 2021 15:21:51 +0200 Subject: [PATCH 09/11] Merge bitcoin/bitcoin#21787: test: fix off-by-ones in rpc_fundrawtransaction assertions f09e6b2585200040be2e8ee44fa79b86b1970d70 test: fix off-by-ones in rpc_fundrawtransaction (Jon Atack) Pull request description: The variables in these assertions should be the same within each line. ACKs for top commit: laanwj: ACK f09e6b2585200040be2e8ee44fa79b86b1970d70 theStack: ACK f09e6b2585200040be2e8ee44fa79b86b1970d70 Tree-SHA512: 7ac754eaadd8cb00a725afa55bccbb8de7547dedac9350d79a9a470918245617e075c56a91adc36fb653bbe8a0a325d59b00443155a7e1a81ebf22e4e4cf56d9 --- test/functional/rpc_fundrawtransaction.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index f1054c4aff51d..809769fbfcd9f 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -722,10 +722,10 @@ def test_option_feerate(self): result6 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 0}) result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) - assert_fee_amount(result1['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) + assert_fee_amount(result1['fee'], count_bytes(result1['hex']), 2 * result_fee_rate) assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) - assert_fee_amount(result4['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) + assert_fee_amount(result4['fee'], count_bytes(result4['hex']), 10 * result_fee_rate) assert_fee_amount(result5['fee'], count_bytes(result5['hex']), 0) assert_fee_amount(result6['fee'], count_bytes(result6['hex']), 0) From 3ba99b9c42b581105b5ba09dda65ec2abf65a8d4 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 10 May 2021 09:02:42 +0200 Subject: [PATCH 10/11] Merge bitcoin/bitcoin#21786: wallet: ensure sat/vB feerates are in range (mantissa of 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 847288df07b45ca535c849e518b22818ab492896 test: fee rate values that cannot be represented as sat/vB (Jon Atack) 06a90fa0381c790f7bde2ab9bf47d2b22acef4a5 rpc: for sat/vB fee rates, limit ParseFixedPoint decimals to 3 (Jon Atack) 0742c7840f03505597fd2de87db97f12597ef667 rpc: enable passing decimals to AmountFromValue, add doxygen (Jon Atack) 8ce3ef57a3e9ad13c0aaa4648e8584241d53592d test: ParseFixedPoint with 3 decimals for sat/vB fee rates (Jon Atack) b5033275979a2a495b02b25f70cadbdcc8b6eb6a test: type error and out of range fee rates where missing (Jon Atack) c5fd4344f7fcc257062a610c8ff26ffcc9b53953 test: explicit fee rates with invalid amounts (Jon Atack) ea6f76b66ecc52360719053489e0ec9f9a673eab test: improve zero-value explicit fee rate coverage (Jon Atack) Pull request description: - Improve/close gaps in existing test coverage before making the change - Enable passing `decimals` to `ParseFixedPoint()` when calling `AmountFromValue()` - Limit explicit fee rates in sat/vB passed in by users to 3 decimals, and raise otherwise - Add regression test coverage Closes #20534. ACKs for top commit: MarcoFalke: review ACK 847288df07b45ca535c849e518b22818ab492896 🔷 Tree-SHA512: c539d07ae9b21c0d6c8ea460beb9c8dad5559445518aace560abc3c05c588907bae189b6fd7602b3b397de4a42356136c3ec6f960d3dcf2d5d16377aef4ab5a2 --- src/rpc/util.cpp | 4 +- src/rpc/util.h | 9 ++++- src/test/util_tests.cpp | 9 +++++ src/wallet/rpcwallet.cpp | 3 +- test/functional/rpc_fundrawtransaction.py | 20 ++++++---- test/functional/rpc_psbt.py | 16 ++++++-- test/functional/wallet_basic.py | 45 ++++++++++++++++++----- test/functional/wallet_send.py | 45 ++++++++++++++++------- 8 files changed, 114 insertions(+), 37 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index d1790c351c077..5f14b67ba0275 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -72,12 +72,12 @@ void RPCTypeCheckObj(const UniValue& o, } } -CAmount AmountFromValue(const UniValue& value) +CAmount AmountFromValue(const UniValue& value, int decimals) { if (!value.isNum() && !value.isStr()) throw JSONRPCError(RPC_TYPE_ERROR, "Amount is not a number or string"); CAmount amount; - if (!ParseFixedPoint(value.getValStr(), 8, &amount)) + if (!ParseFixedPoint(value.getValStr(), decimals, &amount)) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount"); if (!MoneyRange(amount)) throw JSONRPCError(RPC_TYPE_ERROR, "Amount out of range"); diff --git a/src/rpc/util.h b/src/rpc/util.h index 189551cd03793..b39c7acf8e318 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -84,7 +84,14 @@ int64_t ParseInt64V(const UniValue& v, const std::string &strName); double ParseDoubleV(const UniValue& v, const std::string &strName); bool ParseBoolV(const UniValue& v, const std::string &strName); -CAmount AmountFromValue(const UniValue& value); +/** + * Validate and return a CAmount from a UniValue number or string. + * + * @param[in] value UniValue number or string to parse. + * @param[in] decimals Number of significant digits (default: 8). + * @returns a CAmount if the various checks pass. + */ +CAmount AmountFromValue(const UniValue& value, int decimals = 8); using RPCArgList = std::vector>; std::string HelpExampleCli(const std::string& methodname, const std::string& args); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index a796888a8765f..bc6269f03c83f 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -2044,6 +2044,15 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint) BOOST_CHECK(!ParseFixedPoint("1.1e", 8, &amount)); BOOST_CHECK(!ParseFixedPoint("1.1e-", 8, &amount)); BOOST_CHECK(!ParseFixedPoint("1.", 8, &amount)); + + // Test with 3 decimal places for fee rates in sat/vB. + BOOST_CHECK(ParseFixedPoint("0.001", 3, &amount)); + BOOST_CHECK_EQUAL(amount, CAmount{1}); + BOOST_CHECK(!ParseFixedPoint("0.0009", 3, &amount)); + BOOST_CHECK(!ParseFixedPoint("31.00100001", 3, &amount)); + BOOST_CHECK(!ParseFixedPoint("31.0011", 3, &amount)); + BOOST_CHECK(!ParseFixedPoint("31.99999999", 3, &amount)); + BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount)); } static void TestOtherThread(fs::path dirname, std::string lockname, bool *result) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 79343ff86830b..b308579e41f1a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -221,7 +221,8 @@ static void SetFeeEstimateMode(const CWallet& wallet, CCoinControl& cc, const Un if (!estimate_mode.isNull() && estimate_mode.get_str() != "unset") { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both estimate_mode and fee_rate"); } - cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN); + // Fee rates in sat/vB cannot represent more than 3 significant digits. + cc.m_feerate = CFeeRate{AmountFromValue(fee_rate, /* decimals */ 3)}; if (override_min_fee) cc.fOverrideFeeRate = true; return; } diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 809769fbfcd9f..a1c1334e94140 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -5,6 +5,8 @@ """Test the fundrawtransaction RPC.""" from decimal import Decimal +from itertools import product + from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -717,17 +719,16 @@ def test_option_feerate(self): result2 = node.fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) result3 = node.fundrawtransaction(rawtx, {"fee_rate": 10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) result4 = node.fundrawtransaction(rawtx, {"feeRate": str(10 * self.min_relay_tx_fee)}) - # Test that funding non-standard "zero-fee" transactions is valid. - result5 = self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 0}) - result6 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 0}) result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) assert_fee_amount(result1['fee'], count_bytes(result1['hex']), 2 * result_fee_rate) assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) assert_fee_amount(result4['fee'], count_bytes(result4['hex']), 10 * result_fee_rate) - assert_fee_amount(result5['fee'], count_bytes(result5['hex']), 0) - assert_fee_amount(result6['fee'], count_bytes(result6['hex']), 0) + + # Test that funding non-standard "zero-fee" transactions is valid. + for param, zero_value in product(["fee_rate", "feeRate"], [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]): + assert_equal(self.nodes[3].fundrawtransaction(rawtx, {param: zero_value})["fee"], 0) # With no arguments passed, expect fee of 225 satoshis. assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000225, vspan=0.00000001) @@ -761,11 +762,16 @@ def test_option_feerate(self): node.fundrawtransaction, rawtx, {param: -1, "add_inputs": True}) assert_raises_rpc_error(-3, "Amount is not a number or string", node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True}) + # Test fee rate values that don't pass fixed-point parsing checks. + for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: + assert_raises_rpc_error(-3, "Invalid amount", node.fundrawtransaction, rawtx, {param: invalid_value, "add_inputs": True}) + # Test fee_rate values that cannot be represented in sat/vB. + for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: assert_raises_rpc_error(-3, "Invalid amount", - node.fundrawtransaction, rawtx, {param: "", "add_inputs": True}) + node.fundrawtransaction, rawtx, {"fee_rate": invalid_value, "add_inputs": True}) self.log.info("Test min fee rate checks are bypassed with fundrawtxn, e.g. a fee_rate under 1 sat/vB is allowed") - node.fundrawtransaction(rawtx, {"fee_rate": 0.99999999, "add_inputs": True}) + node.fundrawtransaction(rawtx, {"fee_rate": 0.999, "add_inputs": True}) node.fundrawtransaction(rawtx, {"feeRate": 0.00000999, "add_inputs": True}) self.log.info("- raises RPC error if both feeRate and fee_rate are passed") diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index db3500089b4e0..e05ccad04ff72 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -6,6 +6,8 @@ """ from decimal import Decimal +from itertools import product + from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_approx, @@ -115,14 +117,14 @@ def run_test(self): assert_approx(res2["fee"], 0.04, 0.005) self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed, e.g. a fee_rate under 1 sat/vB is allowed") - res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": "0.99999999", "add_inputs": True}) + res3 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"fee_rate": "0.999", "add_inputs": True}) assert_approx(res3["fee"], 0.00000224, 0.0000001) res4 = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.00000999, "add_inputs": True}) assert_approx(res4["fee"], 0.00000224, 0.0000001) self.log.info("Test min fee rate checks with walletcreatefundedpsbt are bypassed and that funding non-standard 'zero-fee' transactions is valid") - for param in ["fee_rate", "feeRate"]: - assert_equal(self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {param: 0, "add_inputs": True})["fee"], 0) + for param, zero_value in product(["fee_rate", "feeRate"], [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]): + assert_equal(0, self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {param: zero_value, "add_inputs": True})["fee"]) self.log.info("Test invalid fee rate settings") for param, value in {("fee_rate", 100000), ("feeRate", 1)}: @@ -132,8 +134,14 @@ def run_test(self): self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: -1, "add_inputs": True}) assert_raises_rpc_error(-3, "Amount is not a number or string", self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: {"foo": "bar"}, "add_inputs": True}) + # Test fee rate values that don't pass fixed-point parsing checks. + for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: + assert_raises_rpc_error(-3, "Invalid amount", + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: invalid_value, "add_inputs": True}) + # Test fee_rate values that cannot be represented in sat/vB. + for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: assert_raises_rpc_error(-3, "Invalid amount", - self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {param: "", "add_inputs": True}) + self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"fee_rate": invalid_value, "add_inputs": True}) self.log.info("- raises RPC error if both feeRate and fee_rate are passed") assert_raises_rpc_error(-8, "Cannot specify both fee_rate (duff/B) and feeRate (DASH/kB)", diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 013a77078ec11..6ba8a44e2406f 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -17,6 +17,7 @@ ) from test_framework.wallet_util import test_address +NOT_A_NUMBER_OR_STRING = "Amount is not a number or string" OUT_OF_RANGE = "Amount out of range" @@ -277,12 +278,25 @@ def run_test(self): # Test setting explicit fee rate just below the minimum. self.log.info("Test sendmany raises 'fee rate too low' if fee_rate of 0.99999999 is passed") assert_raises_rpc_error(-6, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", - self.nodes[2].sendmany, amounts={address: 10}, fee_rate=0.99999999) - - self.log.info("Test sendmany raises if fee_rate of 0 or -1 is passed") - assert_raises_rpc_error(-6, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", - self.nodes[2].sendmany, amounts={address: 10}, fee_rate=0) + self.nodes[2].sendmany, amounts={address: 10}, fee_rate=0.999) + + self.log.info("Test sendmany raises if an invalid fee_rate is passed") + # Test fee_rate with zero values. + msg = "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)" + for zero_value in [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]: + assert_raises_rpc_error(-6, msg, self.nodes[2].sendmany, amounts={address: 1}, fee_rate=zero_value) + msg = "Invalid amount" + # Test fee_rate values that don't pass fixed-point parsing checks. + for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: + assert_raises_rpc_error(-3, msg, self.nodes[2].sendmany, amounts={address: 1.0}, fee_rate=invalid_value) + # Test fee_rate values that cannot be represented in duff/B. + for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: + assert_raises_rpc_error(-3, msg, self.nodes[2].sendmany, amounts={address: 10}, fee_rate=invalid_value) + # Test fee_rate out of range (negative number). assert_raises_rpc_error(-3, OUT_OF_RANGE, self.nodes[2].sendmany, amounts={address: 10}, fee_rate=-1) + # Test type error. + for invalid_value in [True, {"foo": "bar"}]: + assert_raises_rpc_error(-3, NOT_A_NUMBER_OR_STRING, self.nodes[2].sendmany, amounts={address: 10}, fee_rate=invalid_value) self.log.info("Test sendmany raises if an invalid conf_target or estimate_mode is passed") for target, mode in product([-1, 0, 1009], ["economical", "conservative"]): @@ -425,7 +439,7 @@ def run_test(self): self.nodes[0].generate(1) self.sync_all(self.nodes[0:3]) - self.log.info("Test sendtoaddress with fee_rate param (explicit fee rate in sat/vB)") + self.log.info("Test sendtoaddress with fee_rate param (explicit fee rate in duff/B)") prebalance = self.nodes[2].getbalance() assert prebalance > 2 address = self.nodes[1].getnewaddress() @@ -462,10 +476,23 @@ def run_test(self): assert_raises_rpc_error(-6, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=0.999) - self.log.info("Test sendtoaddress raises if fee_rate of 0 or -1 is passed") - assert_raises_rpc_error(-6, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)", - self.nodes[2].sendtoaddress, address=address, amount=10, fee_rate=0) + self.log.info("Test sendtoaddress raises if an invalid fee_rate is passed") + # Test fee_rate with zero values. + msg = "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)" + for zero_value in [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]: + assert_raises_rpc_error(-6, msg, self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=zero_value) + msg = "Invalid amount" + # Test fee_rate values that don't pass fixed-point parsing checks. + for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: + assert_raises_rpc_error(-3, msg, self.nodes[2].sendtoaddress, address=address, amount=1.0, fee_rate=invalid_value) + # Test fee_rate values that cannot be represented in duff/B. + for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: + assert_raises_rpc_error(-3, msg, self.nodes[2].sendtoaddress, address=address, amount=10, fee_rate=invalid_value) + # Test fee_rate out of range (negative number). assert_raises_rpc_error(-3, OUT_OF_RANGE, self.nodes[2].sendtoaddress, address=address, amount=1.0, fee_rate=-1) + # Test type error. + for invalid_value in [True, {"foo": "bar"}]: + assert_raises_rpc_error(-3, NOT_A_NUMBER_OR_STRING, self.nodes[2].sendtoaddress, address=address, amount=1.0, fee_rate=invalid_value) self.log.info("Test sendtoaddress raises if an invalid conf_target or estimate_mode is passed") for target, mode in product([-1, 0, 1009], ["economical", "conservative"]): diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 89f9b554a26f2..81292298cdca6 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -347,22 +347,41 @@ def run_test(self): self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode, expect_error=(-8, msg)) assert_raises_rpc_error(-8, msg, w0.send, {w1.getnewaddress(): 1}, 0.1, mode) - for mode in ["economical", "conservative", "dash/kb", "duff/b"]: - self.log.debug("{}".format(mode)) - for k, v in {"string": "true", "object": {"foo": "bar"}}.items(): + for mode in ["economical", "conservative"]: + for k, v in {"string": "true", "bool": True, "object": {"foo": "bar"}}.items(): self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode, - expect_error=(-3, "Expected type number for conf_target, got {}".format(k))) + expect_error=(-3, f"Expected type number for conf_target, got {k}")) - # Test setting explicit fee rate just below the minimum and at zero. + # Test setting explicit fee rate just below the minimum of 1 duff/B. self.log.info("Explicit fee rate raises RPC error 'fee rate too low' if fee_rate of 0.99999999 is passed") - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0.99999999, - expect_error=(-4, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0.99999999, - expect_error=(-4, "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0, - expect_error=(-4, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) - self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0, - expect_error=(-4, "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)")) + msg = "Fee rate (0.999 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)" + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=0.999, expect_error=(-4, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=0.999, expect_error=(-4, msg)) + + self.log.info("Explicit fee rate raises if invalid fee_rate is passed") + # Test fee_rate with zero values. + msg = "Fee rate (0.000 duff/B) is lower than the minimum fee rate setting (1.000 duff/B)" + for zero_value in [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=zero_value, expect_error=(-4, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=zero_value, expect_error=(-4, msg)) + msg = "Invalid amount" + # Test fee_rate values that don't pass fixed-point parsing checks. + for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=invalid_value, expect_error=(-3, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=invalid_value, expect_error=(-3, msg)) + # Test fee_rate values that cannot be represented in duff/B. + for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=invalid_value, expect_error=(-3, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=invalid_value, expect_error=(-3, msg)) + # Test fee_rate out of range (negative number). + msg = "Amount out of range" + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=-1, expect_error=(-3, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=-1, expect_error=(-3, msg)) + # Test type error. + msg = "Amount is not a number or string" + for invalid_value in [True, {"foo": "bar"}]: + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, fee_rate=invalid_value, expect_error=(-3, msg)) + self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_fee_rate=invalid_value, expect_error=(-3, msg)) # TODO: Return hex if fee rate is below -maxmempool # res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode="duff/b", add_to_wallet=False) From d6946aaba1550f1f2e81a965f8ec4be1604e1f3a Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 2 Sep 2024 14:57:19 +0700 Subject: [PATCH 11/11] fix: offset fee for 1 duff in commission in wallet_basic.py due to missing bitcoin/bitcoin#22949 --- test/functional/wallet_basic.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 6ba8a44e2406f..f9d82ed4a86ee 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -466,7 +466,9 @@ def run_test(self): self.sync_all(self.nodes[0:3]) postbalance = self.nodes[2].getbalance() fee = prebalance - postbalance - amount - assert_fee_amount(fee, tx_size, Decimal(fee_rate_btc_kvb)) + # TODO: remove workaround after bitcoin/bitcoin#22949 done + workaround_offset = 1 + assert_fee_amount(fee, tx_size - workaround_offset, Decimal(fee_rate_btc_kvb)) for key in ["totalFee", "feeRate"]: assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1)