Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely

luke-jr commented at 11:31 pm on October 26, 2020: member

Expects fee_mode for explicit feerate modes instead of overloading conf_target (positional args still accept either for now; that can be restricted once we finally have #17356).

fundrawtransaction used to expect its option to be called feeRate, diverging from other fee_rate option names. It now accepts either, with feeRate soft-deprecated.

fundrawtransaction and bumpfee still accept fee_rate without estimate_mode for backward compatibility.

Borrows tests from #20220 (adapted for interface changes). (NOTE: NOT including 9ea6d390c26975c98a9cf28cf816bcbded9aa3d1 77a4f98897ab4430311cff3f2226f274fad20d6a f97a3cd3e6e229305a4451d611d77cbf3892381f)

RPC/Wallet: Move nonzero feerate check to SetFeeEstimateMode

Was only in bumpfee's helper

75e54e388b

RPC/Wallet: sendtoaddress & sendmany: Accept fee_rate as an alternative to conf_target 0c194fb706

luke-jr commented at 11:33 pm on October 26, 2020: member

@jonatack What do you think of this approach? (Actually what I thought it already was!)

fanquake added the label Wallet on Oct 26, 2020

DrahtBot commented at 5:30 am on October 27, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20220 (wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 by jonatack)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/rpcwallet.cpp:228 in 75e54e388b outdated

222@@ -223,6 +223,9 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U
223         }
224 
225         cc.m_feerate = CFeeRate(fee_rate);
226+        if (*cc.m_feerate <= CFeeRate(0)) {
227+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid fee_rate %s (must be greater than 0)", cc.m_feerate->ToString()));
228+        }

jonatack commented at 9:38 pm on October 27, 2020:

It could maybe just be removed. The tests I added in #20220 for the other 5 RPCs check that the code detects this and raises with "Fee rate (0.00000000 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)

luke-jr commented at 10:12 pm on October 27, 2020:

IMO these are fundamentally different error types, and should probably be handled differently.

in src/wallet/rpcwallet.cpp:4030 in eeae8bbf77 outdated

4023@@ -4024,9 +4024,10 @@ static RPCHelpMan send()
4024                     {"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The bitcoin address to receive the change"},
4025                     {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
4026                     {"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if change_address is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
4027-                    {"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)"},
4028+                    {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks)"},
4029                     {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
4030             "       \"" + FeeModes("\"\n\"") + "\""},
4031+                    {"fee_rate", RPCArg::Type::NUM, /* default */ "wallet default", "Fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},

jonatack commented at 9:47 pm on October 27, 2020:

eeae8bbf7785133f0df3c7b731a452204eb03b15 I don’t think this works at this commit? Maybe as feeRate but then you see Cannot specify both estimate_mode and feeRate. Also, wallet_send.py has no test coverage for it currently.

luke-jr commented at 1:41 pm on October 28, 2020:

It should…?

luke-jr commented at 8:38 pm on October 28, 2020:

Fixed

in src/wallet/rpcwallet.cpp:3141 in 2fad6c55c0 outdated

3134@@ -3134,8 +3135,11 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3135             lockUnspents = (options.exists("lock_unspents") ? options["lock_unspents"] : options["lockUnspents"]).get_bool();
3136         }
3137 
3138-        if (options.exists("feeRate"))
3139-        {
3140+        if (options.exists("feeRate") && options.exists("fee_rate")) {
3141+            throw JSONRPCError(RPC_INVALID_PARAMETER, "feeRate and fee_rate options should not both be set. Use fee_rate (feeRate is deprecated).");
3142+        }
3143+        auto fee_rate = options.exists("feeRate") ? options["feeRate"] : options["fee_rate"];

jonatack commented at 9:50 pm on October 27, 2020:

Concept ACK on moving away from feeRate

jonatack commented at 10:08 pm on October 27, 2020: member

I agree this is more sane than currently, especially aliasing feeRate to fee_rate. Didn’t test the commit “RPC/Wallet: sendtoaddress & sendmany: Accept fee_rate as an alternatve to conf_target”; not sure it works as-is but it would need test coverage.

Bugfix: RPC/Wallet: Pass both conf_target and fee_rate into SetFeeEstimateMode separately to replace broken code duplication

For compatibility, bumpfee and fundraw accept fee_rate without estimate_mode

856a1a824b

RPC/Wallet: Deprecate fundrawtransaction feeRate and replace with fee_rate 8e07bb59ce

Bugfix: RPC/Wallet: Fix help for "send" method 508aae16b3

QA: Test bumpfee RPC with BTC/kB and sat/B estimation modes c630bb3de5

wallet: add bumpfee feerate coverage, improve error/help 994f87860b

wallet: fundrawtransaction feerate coverage af5fdaea85

test: refactor rpc_psbt.py for walletcreatefundedpsbt feerate coverage 081955b421

wallet: add walletcreatefundedpsbt feerate coverage 681ce7d324

QA: wallet_basic: Adapt sendtoaddress/sendmany tests to use fee_rate named param 7bef214862

luke-jr commented at 8:39 pm on October 28, 2020: member

Fixed review comments including adapting tests

luke-jr force-pushed on Oct 28, 2020

in src/wallet/rpcwallet.cpp:236 in 856a1a824b outdated

231@@ -229,8 +232,13 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U
232 
233         // default RBF to true for explicit fee rate modes
234         if (cc.m_signal_bip125_rbf == nullopt) cc.m_signal_bip125_rbf = true;
235-    } else if (!estimate_param.isNull()) {
236-        cc.m_confirm_target = ParseConfirmTarget(estimate_param, pwallet->chain().estimateMaxBlocks());
237+    } else {
238+        if (&conf_target_param != &fee_rate_param && !fee_rate_param.isNull()) {

sipa commented at 3:55 am on November 4, 2020:

I had to look this up as this is comparing pointers to unrelated objects, but my reading of https://en.cppreference.com/w/cpp/language/operator_comparison is that this is indeed well defined.

sipa commented at 4:02 am on November 4, 2020: member

Concept/approach ACK, also superficial code review ACK. Needs linter fix.

DrahtBot commented at 4:25 am on November 4, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Nov 4, 2020

MarcoFalke added this to the milestone 0.21.0 on Nov 4, 2020

in src/wallet/rpcwallet.cpp:455 in 7bef214862

450@@ -440,7 +451,7 @@ static RPCHelpMan sendtoaddress()
451                     {"subtractfeefromamount", RPCArg::Type::BOOL, /* default */ "false", "The fee will be deducted from the amount being sent.\n"
452                                          "The recipient will receive less bitcoins than you enter in the amount field."},
453                     {"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"},
454-                    {"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)"},
455+                    {"conf_target|fee_rate", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
456                     {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"

MarcoFalke commented at 7:24 am on November 5, 2020:

should this also be aliased to something? Like fee_rate_type?

luke-jr commented at 7:24 pm on November 6, 2020: member

Closing in favour of #20305

luke-jr closed this on Nov 6, 2020

DrahtBot locked this on Feb 15, 2022

Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely #20250

Conflicts