wallet, rpc: explicit fee rate follow-ups/fixes for 0.21

jonatack commented at 4:57 pm on October 22, 2020: member

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.

in src/wallet/rpcwallet.cpp:3464 in 59222c6e22 outdated

3460@@ -3461,7 +3461,6 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
3461             if (options.exists("fee_rate")) {
3462                 throw JSONRPCError(RPC_INVALID_PARAMETER, "conf_target can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
3463             }
3464-            coin_control.m_confirm_target = ParseConfirmTarget(conf_target, pwallet->chain().estimateMaxBlocks());

jonatack commented at 5:06 pm on October 22, 2020:

This action is performed in SetFeeEstimateMode() and calling it in bumpfee beforehand causes an error when explicit feerates are used (issue #20219).

DrahtBot added the label RPC/REST/ZMQ on Oct 22, 2020

DrahtBot added the label Wallet on Oct 22, 2020

jonatack renamed this:
~~rpc, test, doc: explicit feerate follow-ups~~
wallet, rpc, test: explicit feerate follow-ups
on Oct 22, 2020

laanwj added this to the milestone 0.21.0 on Oct 22, 2020

MarcoFalke renamed this:
~~wallet, rpc, test: explicit feerate follow-ups~~
wallet, rpc: explicit feerate follow-ups
on Oct 23, 2020

jonatack force-pushed on Oct 23, 2020

jonatack commented at 10:57 am on October 23, 2020: member

Pushed a few improvements. Should be ready for review.

in test/functional/wallet_bumpfee.py:164 in 995dd4a6e7 outdated

133@@ -132,6 +134,13 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
134     if mode == "fee_rate":
135         bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"fee_rate": NORMAL})
136         bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": NORMAL})
137+    elif mode == BTC_MODE:

Xekyo commented at 8:13 pm on October 23, 2020:

Isn’t the BTC_MODE then essentially a duplicate of the {“fee_rate”: …}` option?

jonatack commented at 2:20 pm on October 24, 2020:

I agree, it’s a redundant feature here that adds to the confusion…and it also uses a different code path so the test is needed.

in test/functional/wallet_bumpfee.py:171 in 70b793c2b3 outdated

167@@ -168,24 +168,54 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address):
168 
169 
170 def test_feerate_args(self, rbf_node, peer_node, dest_address):
171-    self.log.info('Test fee_rate args')
172+    self.log.info('Test feerate args')

Xekyo commented at 8:16 pm on October 23, 2020:

I don’t understand this change. Is this not referring to the “fee_rate” argument below?

jonatack commented at 2:54 pm on October 24, 2020:

Reverted. This line goes away in the following commit.

in src/wallet/rpcwallet.cpp:3466 in 70b793c2b3 outdated

3458@@ -3459,7 +3459,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
3459 
3460         if (!conf_target.isNull()) {
3461             if (options.exists("fee_rate")) {
3462-                throw JSONRPCError(RPC_INVALID_PARAMETER, "conf_target can't be set with fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.");
3463+                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.");

Xekyo commented at 8:20 pm on October 23, 2020:

So much clearer! :+1:

in test/functional/wallet_bumpfee.py:184 in 70b793c2b3 outdated

186-
187     assert_raises_rpc_error(-4, "is too high (cannot be higher than", rbf_node.bumpfee, rbfid, {"fee_rate": TOO_HIGH})
188-    self.clear_mempool()
189 
190+    self.log.info("Test explicit feerate raises RPC error if estimate_mode is passed without a conf_target")
191+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL, "estimate_mode": BTC_MODE})

Xekyo commented at 8:25 pm on October 23, 2020:

Style nit: line 183 uses “feerate”, 184 uses “fee rate”. I’d prefer “feerate” per Optech style guide, but if the error message currently is “fee rate”, it might be nicer to keep it homogeneous.

jonatack commented at 5:16 pm on October 24, 2020:

Updated the log message to “fee rate”.

in test/functional/wallet_bumpfee.py:185 in 70b793c2b3 outdated

187     assert_raises_rpc_error(-4, "is too high (cannot be higher than", rbf_node.bumpfee, rbfid, {"fee_rate": TOO_HIGH})
188-    self.clear_mempool()
189 
190+    self.log.info("Test explicit feerate raises RPC error if estimate_mode is passed without a conf_target")
191+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL, "estimate_mode": BTC_MODE})
192+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": 10, "estimate_mode": SAT_MODE})

Xekyo commented at 8:32 pm on October 23, 2020:

That’s an odd name clash, that the error complains about the missing “fee rate”, but it’s actually conf_target that’s missing. I assume that was part of the items that #19543 was meant to address?

jonatack commented at 3:52 pm on October 24, 2020:

Good find. Fixed the error message to now print Selected estimate_mode <MODE> requires a fee rate to be specified in conf_target and updated the tests.

0+++ b/src/wallet/rpcwallet.cpp
1@@ -214,7 +214,7 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U
2 
3     if (cc.m_fee_mode == FeeEstimateMode::BTC_KB || cc.m_fee_mode == FeeEstimateMode::SAT_B) {
4         if (estimate_param.isNull()) {
5-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected estimate_mode requires a fee rate");
6+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Selected estimate_mode %s requires a fee rate to be specified in conf_target", estimate_mode.get_str()));
7         }

0+++ b/test/functional/wallet_bumpfee.py
1-        assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": HIGH, "estimate_mode": BTC_MODE})
2-        assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": 1000, "estimate_mode": SAT_MODE})
3+        for unit, fee_rate in {"SAT/B": 100, "BTC/KB": NORMAL}.items():
4+            assert_raises_rpc_error(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit),
5+                                    rbf_node.bumpfee, rbfid, {"fee_rate": fee_rate, "estimate_mode": unit})

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

Done in fc572172

in test/functional/wallet_bumpfee.py:190 in 70b793c2b3 outdated

192+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": 10, "estimate_mode": SAT_MODE})
193+
194+    self.log.info("Test explicit feerate raises RPC error if both fee_rate and conf_target are passed")
195+    msg = "Cannot specify both conf_target and fee_rate. Please provide either a confirmation " \
196+          "target in blocks for automatic fee estimation, or an explicit fee rate."
197+    assert_raises_rpc_error(-8, msg, rbf_node.bumpfee, rbfid, {"conf_target": NORMAL, "fee_rate": NORMAL})

Xekyo commented at 8:36 pm on October 23, 2020:

Okay, I guess this is a prime example of what is meant to get addressed in #19543. conf_target actually being a feerate is fairly icky.

in test/functional/wallet_bumpfee.py:188 in 70b793c2b3 outdated

190+    self.log.info("Test explicit feerate raises RPC error if estimate_mode is passed without a conf_target")
191+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL, "estimate_mode": BTC_MODE})
192+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": 10, "estimate_mode": SAT_MODE})
193+
194+    self.log.info("Test explicit feerate raises RPC error if both fee_rate and conf_target are passed")
195+    msg = "Cannot specify both conf_target and fee_rate. Please provide either a confirmation " \

Xekyo commented at 8:40 pm on October 23, 2020:

Nit: msg is a bit unspecific. Could be for example msg_both or error_both

jonatack commented at 2:58 pm on October 24, 2020:

Done, changed to error_both.

in test/functional/wallet_bumpfee.py:197 in 70b793c2b3 outdated

199+    self.log.info("Test invalid conf_target settings")
200+    for field in ["confTarget", "conf_target"]:
201+        assert_raises_rpc_error(-8, msg, rbf_node.bumpfee, rbfid, {field: 1, "fee_rate": NORMAL})
202+    too_high = "is too high (cannot be higher than -maxtxfee"
203+    assert_raises_rpc_error(-4, too_high, lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": BTC_MODE, "conf_target": 2009}))
204+    assert_raises_rpc_error(-4, too_high, lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": SAT_MODE, "conf_target": 2009 * 10000}))

Xekyo commented at 8:44 pm on October 23, 2020:

Potential missing test case: what would happen if we specified something lower than minRelayTxFeerate here?

jonatack commented at 3:14 pm on October 24, 2020:

Nice find! added:

0-        assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, {"fee_rate": INSUFFICIENT})
1+        for options in [{"fee_rate": INSUFFICIENT}, {"conf_target": INSUFFICIENT, "estimate_mode": BTC_MODE}, {"conf_target": 1, "estimate_mode": SAT_MODE}]:
2+            assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, options)

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

Done in 1697a40

Xekyo commented at 8:03 pm on October 28, 2020:

Wonderful, thanks!

in test/functional/wallet_bumpfee.py:200 in 70b793c2b3 outdated

202+    too_high = "is too high (cannot be higher than -maxtxfee"
203+    assert_raises_rpc_error(-4, too_high, lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": BTC_MODE, "conf_target": 2009}))
204+    assert_raises_rpc_error(-4, too_high, lambda: rbf_node.bumpfee(rbfid, {"estimate_mode": SAT_MODE, "conf_target": 2009 * 10000}))
205+
206+    self.log.info("Test invalid estimate_mode settings")
207+    for k, v in {"number": 42, "object": {"foo": "bar"}}.items():

Xekyo commented at 8:48 pm on October 23, 2020:

TIL how to iterate through a dictionary in python. :)

in test/functional/wallet_bumpfee.py:102 in 9ea6d390c2 outdated

119-        # confTarget and conf_target
120-        assert_raises_rpc_error(-8, "confTarget and conf_target options should not both be set", node.bumpfee, txid, {
121-            "confTarget": 123,
122-            "conf_target": 456,
123-        })
124+    def test_invalid_parameters(self, rbf_node, peer_node, dest_address):

Xekyo commented at 9:00 pm on October 23, 2020:

This is a bit hard to review. My approach would currently be to check line by line whether all test cases are accounted for but that would probably be easier if the reordering of code and the refactoring to traverse the test cases had been separated into two commits. What would a reviewer usually be expected to do here?

Alternatively, I suspect that there might be better diff tools that can follow a change like this. :thinking:

jonatack commented at 2:41 pm on October 24, 2020:

Sorry about that. Yes, there are a few options. In this case, a helpful one might be

git show 6fa5686 -w --color-moved=dimmed-zebra

For other cases, maybe git show or git diff with --word-diff.

Or to ignore all added lines having a matching identical removed line (empty result proves move-only):

People like practicalswift, promag, MarcoFalke, jnewbery, and harding likely know some more good ones.

Xekyo commented at 7:26 pm on October 28, 2020:

Okay, lemme check this out locally and run this incantation on it. :)

Xekyo commented at 7:48 pm on October 28, 2020:

Alright, the git show 2d8eba8 -w --color-moved=dimmed_zebra did the trick for me. Thanks for teaching me!

in src/rpc/util.cpp:275 in 84fc42f32c outdated

271@@ -272,11 +272,12 @@ UniValue DescribeAddress(const CTxDestination& dest)
272 
273 unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target)
274 {
275-    int target = value.get_int();
276-    if (target < 1 || (unsigned int)target > max_target) {
277-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u - %u", 1, max_target));
278+    const int target{value.get_int()};

Xekyo commented at 9:02 pm on October 23, 2020:

What is the advantage of this construction over the previous line?

jonatack commented at 9:49 pm on October 23, 2020:

Little: constness, and while changing the line, updated the style. It’s not essential, to be sure. Initially, the function was touched to fix the error message. Then, I saw the function has two C-style type conversions and replaced them with one preferred static cast (per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast). Then, made the non-mutated variables const.

in src/rpc/util.cpp:277 in 84fc42f32c outdated

271@@ -272,11 +272,12 @@ UniValue DescribeAddress(const CTxDestination& dest)
272 
273 unsigned int ParseConfirmTarget(const UniValue& value, unsigned int max_target)
274 {
275-    int target = value.get_int();
276-    if (target < 1 || (unsigned int)target > max_target) {
277-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid conf_target, must be between %u - %u", 1, max_target));
278+    const int target{value.get_int()};
279+    const unsigned int unsigned_target{static_cast<unsigned int>(target)};
280+    if (target < 1 || unsigned_target > max_target) {

Xekyo commented at 9:09 pm on October 23, 2020:

Would this not fail if the value of the input was between MAX_INT if max_target were between MAX_INT and MAX_UNSIGNED_INT?

Xekyo commented at 9:10 pm on October 23, 2020:

I assume this is safe, because max_target is 1000 blocks, but I did spend a few minutes staring at this.

in test/functional/rpc_fundrawtransaction.py:697 in 84fc42f32c outdated

692+            assert_approx(result["fee"], vexp=0.0141, vspan=0.0001)
693+
694+        for field, feerate in {"conf_target": 0.1, "estimate_mode": "sat/b"}.items():
695+            self.log.info("Test fundrawtxn raises RPC error if both feeRate and {} are passed".format(field))
696+            assert_raises_rpc_error(
697+                -8, "Cannot specify both {} and feeRate".format(field),

Xekyo commented at 9:14 pm on October 23, 2020:

So now we have fee rate, feeRate and fee_rate in one PR. The horrors. :sweat_smile:

in test/functional/rpc_psbt.py:178 in 1b498b9b6a outdated

173@@ -174,8 +174,11 @@ def run_test(self):
174             elif out['scriptPubKey']['addresses'][0] == p2pkh:
175                 p2pkh_pos = out['n']
176 
177+        inputs = [{"txid": txid, "vout": p2wpkh_pos}, {"txid": txid, "vout": p2sh_p2wpkh_pos}, {"txid": txid, "vout": p2pkh_pos}]
178+        addr = {self.nodes[1].getnewaddress(): 29.99}

Xekyo commented at 9:17 pm on October 23, 2020:

Nit: since this is an address and amount, how about “receiver_output”?

jonatack commented at 2:50 pm on October 24, 2020:

Good idea. Changed it to an outputs array like in the help:

0         inputs = [{"txid": txid, "vout": p2wpkh_pos}, {"txid": txid, "vout": p2sh_p2wpkh_pos}, {"txid": txid, "vout": p2pkh_pos}]
1-        addr = {self.nodes[1].getnewaddress(): 29.99}
2+        outputs = [{self.nodes[1].getnewaddress(): 29.99}]

in test/functional/rpc_psbt.py:192 in 1b498b9b6a outdated

188@@ -186,14 +189,15 @@ def run_test(self):
189         assert_equal(walletprocesspsbt_out['complete'], True)
190         self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex'])
191 
192-        # feeRate of 0.1 BTC / KB produces a total fee slightly below -maxtxfee (~0.05280000):
193-        res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1, "add_inputs": True})
194+        self.log.info("Test feeRate of 0.1 BTC / KB produces a total fee slightly below -maxtxfee (~0.05280000)")

Xekyo commented at 9:19 pm on October 23, 2020:

Nit: “K” is the symbol for Kelvin, it’s “kB”, also remove spacing like below for consistency. :nerd_face:

jonatack commented at 2:52 pm on October 24, 2020:

Done.

in test/functional/rpc_psbt.py:198 in 1b498b9b6a outdated

198-        # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
199+        self.log.info("Test feeRate of 10 BTC/KB produces total fee well above -maxtxfee and raises RPC error")
200         # previously this was silently capped at -maxtxfee
201-        assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10, "add_inputs": True})
202-        assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():1}, 0, {"feeRate": 10, "add_inputs": False})
203+        for bool_add, addr in {True: addr, False: {self.nodes[1].getnewaddress(): 1}}.items():

Xekyo commented at 9:22 pm on October 23, 2020:

Still getting used to this, but totally see what you’re doing now. :)

in test/functional/rpc_psbt.py:197 in 6ad143c294 outdated

193+        self.log.info("Test feeRate of 0.1 BTC / KB produces a total fee at or slightly below -maxtxfee (~0.05290000)")
194         res = self.nodes[1].walletcreatefundedpsbt(inputs, addr, 0, {"feeRate": 0.1, "add_inputs": True})
195         assert_approx(res["fee"], 0.055, 0.005)
196 
197+        self.log.info("Test passing walletcreatefundedpsbt explicit feerate with conf_target and estimate_mode")
198+        for unit, feerate in {"btc/kb": 0.1, "sat/b": 10000}.items():

Xekyo commented at 9:28 pm on October 23, 2020:

Probably not the right place to bring this up, but the explicit specifying of the unit does seem really error prone as mentioned in the comments of #19543 before.

jonatack commented at 9:54 pm on October 23, 2020:

Yes. It would be good to replace this with the fixed-unit feerate_sat_vb argument before the 0.21 branch-off. Edit: I have a changeset for that but I guess it’s too late though.

in test/functional/rpc_psbt.py:226 in 6ad143c294 outdated

222+                    lambda: self.nodes[1].walletcreatefundedpsbt(inputs, addr, 0, {"estimate_mode": mode, "conf_target": v, "add_inputs": True}))
223+            if mode in ["btc/kb", "sat/b"]:
224+                assert_raises_rpc_error(-3, "Amount out of range",
225+                    lambda: self.nodes[1].walletcreatefundedpsbt(inputs, addr, 0, {"estimate_mode": mode, "conf_target": -1, "add_inputs": True}))
226+                assert_raises_rpc_error(-4, "Fee rate (0.00000000 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
227+                    lambda: self.nodes[1].walletcreatefundedpsbt(inputs, addr, 0, {"estimate_mode": mode, "conf_target": 0, "add_inputs": True}))

Xekyo commented at 9:34 pm on October 23, 2020:

Zero often being some sort of special case, what happens with values between 0 and 0.00001000? Maybe we should also test 0.00000999.

jonatack commented at 9:56 pm on October 23, 2020:

Agreed–started doing just that today for the new arg in 0f2eb16 (#20231), will update here.

jonatack commented at 3:01 pm on October 24, 2020:

Added

0for unit, feerate in {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():
1    self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(feerate, unit))
2    assert_raises_rpc_error(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
3        lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": unit, "conf_target": feerate, "add_inputs": True}))

in src/wallet/rpcwallet.cpp:4043 in 77a4f98897 outdated

4033@@ -4034,7 +4034,7 @@ static RPCHelpMan send()
4034                     {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
4035                     {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
4036                     {"psbt", RPCArg::Type::BOOL,  /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."},
4037-                    {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A JSON array of integers.\n"
4038+                    {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "Outputs to subtract the fee from, specified as integer indices.\n"

Xekyo commented at 9:35 pm on October 23, 2020:

Oh my, great improvement!

wallet, bugfix: fix bumpfee with explicit fee rate modes 052427eef1

wallet: fix SetFeeEstimateMode() error message

to clarify for the user the confusing error message that the missing fee rate
needs to be set in the conf_target param/option.

fc5721723d

jonatack commented at 8:47 pm on October 26, 2020: member

@Xekyo thanks for the excellent feedback–I have been building on it, writing more tests, finding a few more things, will push an update tomorrow.

DrahtBot commented at 5:48 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:

#20250 (Bugfix: RPC/Wallet: Make BTC/kB and sat/B fee modes work sanely by luke-jr)
#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.

wallet: improve bumpfee error/help, add explicit fee rate coverage 1697a40b6f

wallet: combine redundant bumpfee invalid params and args tests

might be best reviewed with:

git show COMMIT_HASH -w --color-moved=dimmed-zebra

2d8eba8f84

jonatack force-pushed on Oct 27, 2020

jonatack commented at 9:25 pm on October 27, 2020: member

Hopefully addressed the (excellent) review feedback and also added missing explicit fee rate coverage in wallet_basic.py for RPCs sendtoaddress and sendmany and in wallet_send.py for RPC send. Found some bugs and inconsistent behavior in RPC send, some of which I notated with TODO comments in the tests, but left the fixes for follow-ups as I’ve spent a ton of time here already going through all these RPCs and the release branch-off isn’t far away.

jonatack renamed this:
~~wallet, rpc: explicit feerate follow-ups~~
wallet, rpc: explicit fee rate follow-ups for 0.21
on Oct 27, 2020

Xekyo commented at 3:38 am on October 28, 2020: member

Just seeing this now. Aiming to take a look tomorrow.

in test/functional/wallet_basic.py:233 in c80e2276fb outdated

227@@ -226,9 +228,9 @@ def run_test(self):
228         assert_equal(self.nodes[2].getbalance(), node_2_bal)
229         node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex']))
230 
231-        # Sendmany with explicit fee (BTC/kB)
232+        self.log.info("Test explicit fee (sendmany as BTC/kB)")
233         # Throw if no conf_target provided
234-        assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate",
235+        assert_raises_rpc_error(-8, "Selected estimate_mode bTc/kB requires a fee rate to be specified in conf_target",

Xekyo commented at 7:17 pm on October 28, 2020:

Nit: bTc?

in test/functional/wallet_basic.py:229 in c80e2276fb outdated

227@@ -226,9 +228,9 @@ def run_test(self):
228         assert_equal(self.nodes[2].getbalance(), node_2_bal)
229         node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex']))
230 
231-        # Sendmany with explicit fee (BTC/kB)

Xekyo commented at 7:19 pm on October 28, 2020:

Nit: I think that’s a fee rate, not an absolute fee.

jonatack commented at 10:14 pm on October 28, 2020:

Good eye! Done

in test/functional/wallet_basic.py:257 in c80e2276fb outdated

253@@ -252,9 +254,9 @@ def run_test(self):
254         node_0_bal += Decimal('10')
255         assert_equal(self.nodes[0].getbalance(), node_0_bal)
256 
257-        # Sendmany with explicit fee (SAT/B)
258+        self.log.info("Test explicit fee (sendmany as sat/B)")

Xekyo commented at 7:19 pm on October 28, 2020:

This should say “fee rate” rather than “fee”.

jonatack commented at 10:14 pm on October 28, 2020:

done

in test/functional/wallet_basic.py:424 in c80e2276fb outdated

420@@ -413,15 +421,14 @@ def run_test(self):
421             self.nodes[0].generate(1)
422             self.sync_all(self.nodes[0:3])
423 
424-            # send with explicit btc/kb fee
425-            self.log.info("test explicit fee (sendtoaddress as btc/kb)")
426+            self.log.info("Test explicit fee (sendtoaddress as BTC/kB)")

Xekyo commented at 7:20 pm on October 28, 2020:

This should say “fee rate” rather than “fee”.

jonatack commented at 10:14 pm on October 28, 2020:

done

in test/functional/wallet_basic.py:259 in c80e2276fb outdated

253@@ -252,9 +254,9 @@ def run_test(self):
254         node_0_bal += Decimal('10')
255         assert_equal(self.nodes[0].getbalance(), node_0_bal)
256 
257-        # Sendmany with explicit fee (SAT/B)
258+        self.log.info("Test explicit fee (sendmany as sat/B)")
259         # Throw if no conf_target provided
260-        assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate",
261+        assert_raises_rpc_error(-8, "Selected estimate_mode sat/b requires a fee rate to be specified in conf_target",

Xekyo commented at 7:22 pm on October 28, 2020:

Nit: Inconsistency between 257 and 259, once it’s sat/B once sat/b.

There seem to be a lot of case variations in the following code on the sat/b and BTc/Kb. I’m not sure whether that is on purpose to test the case insensitivity. I’ll err on pointing them out when I notice. Feel free to ignore if that’s on purpose and I’m just not grokking the reason.

Xekyo commented at 8:01 pm on October 28, 2020:

I just saw the value below in the estimate_mode the input in 260 is matching. I conclude that the string is case-insensitive, and the purpose was to test the insensitivity. I’d prefer an approach where a single style was used across all tests, and then one test checked the case insensitivity of the estimate_mode explicitly, but I don’t think it’s pertinent to make the actual change in this PR.

jonatack commented at 10:13 pm on October 28, 2020:

good eye, done

in test/functional/wallet_basic.py:459 in c80e2276fb outdated

453@@ -447,15 +454,15 @@ def run_test(self):
454             fee = prebalance - postbalance - Decimal('1')
455             assert_fee_amount(fee, tx_size, Decimal('0.00002500'))
456 
457-            # send with explicit sat/b fee
458             self.sync_all(self.nodes[0:3])
459-            self.log.info("test explicit fee (sendtoaddress as sat/b)")
460+
461+            self.log.info("Test explicit fee (sendtoaddress as sat/B)")

Xekyo commented at 7:23 pm on October 28, 2020:

This should say “fee rate” rather than “fee”.

The phrase “explicit fee” would imply to me that an absolute amount of satoshis is used rather than a fee rate. That’s why I’m pointing it out.

jonatack commented at 10:13 pm on October 28, 2020:

agree, done

in test/functional/wallet_basic.py:465 in c80e2276fb outdated

463             prebalance = self.nodes[2].getbalance()
464             assert prebalance > 2
465             address = self.nodes[1].getnewaddress()
466             # Throw if no conf_target provided
467-            assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate",
468+            assert_raises_rpc_error(-8, "Selected estimate_mode SAT/b requires a fee rate to be specified in conf_target",

Xekyo commented at 7:24 pm on October 28, 2020:

Nit: SAT/b

in test/functional/wallet_basic.py:424 in fc5721723d outdated

420@@ -421,7 +421,7 @@ def run_test(self):
421             assert prebalance > 2
422             address = self.nodes[1].getnewaddress()
423             # Throw if no conf_target provided
424-            assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate",
425+            assert_raises_rpc_error(-8, "Selected estimate_mode BTc/Kb requires a fee rate to be specified in conf_target",

Xekyo commented at 7:55 pm on October 28, 2020:

Nit: BTc/Kb?

jonatack commented at 10:12 pm on October 28, 2020:

I suppose I’ll make them all proper-cased in wallet_basic, and lowercased in fundrawtransaction, so they are less jarring while still testing the case insensitivity.

jonatack commented at 10:53 pm on October 28, 2020:

Oh wait, in wallet_basic.py I’d rather not touch the units in the original tests by @kallewoof. They were set this way on purpose. See closed PR #20041 that wanted to change them.

jonatack commented at 10:55 pm on October 28, 2020:

I’ll add “case insensitive” to the logging so it’s clear to future readers of the test code.

Xekyo commented at 0:19 am on October 29, 2020:

Cool, thanks for digging up the context. I still think it would be better to test case insensitivity of an argument in one dedicated test and then make all the other tests use the canonical writing, but it’s definitely out of scope here.

jonatack commented at 0:31 am on October 29, 2020:

Agree, and in a way that’s what we’ve done: The added tests use canonical units and the original tests in wallet_basic.py do case insensitive testing of the units and are now described as such.

in test/functional/wallet_bumpfee.py:179 in 1697a40b6f outdated

180-
181-    # Bumping to just above minrelay should fail to increase total fee enough, at least
182-    assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, {"fee_rate": INSUFFICIENT})
183 
184+    # For each fee mode, bumping to just above minrelay should fail to increase the total fee enough.
185+    for options in [{"fee_rate": INSUFFICIENT}, {"conf_target": INSUFFICIENT, "estimate_mode": BTC_MODE}, {"conf_target": 1, "estimate_mode": SAT_MODE}]:

Xekyo commented at 8:12 pm on October 28, 2020:

I was originally thinking here:

I’m confused by the {"conf_target": 1, "estimate_mode": SAT_MODE}. Fitting the theme, I would expect the conf_target to be below minRelayTxFeeRate. But IIUC, 1 sat/B would be exactly the minRelayTxFeeRate. Is it possible that this should be … oh. … INSUFFICIENT also appears to be exactly the minRelayTxFeeRate. … Oh, this is for bumping. Duh.

Nevermind, carry on! :ok_hand: Thanks for adding this.

in test/functional/rpc_fundrawtransaction.py:724 in 3f52eafa40 outdated

719+            else:
720+                for n in [-1, 0, 1009]:
721+                    assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008",
722+                        lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": n}))
723+
724+        for unit, fee_rate in {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():

Xekyo commented at 8:25 pm on October 28, 2020:

Nit: Since this is new code, I’m surprised by the variations in the estimate_mode case, but not feeling strongly about it.

jonatack commented at 10:10 pm on October 28, 2020:

made them all the same

in test/functional/rpc_fundrawtransaction.py:726 in 3f52eafa40 outdated

721+                    assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008",
722+                        lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": n}))
723+
724+        for unit, fee_rate in {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():
725+            self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit))
726+            assert_raises_rpc_error(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",

Xekyo commented at 8:26 pm on October 28, 2020:

:+1:

in test/functional/rpc_psbt.py:196 in 36b6ab7494 outdated

194+        self.log.info("Test walletcreatefundedpsbt feeRate of 0.1 BTC/kB produces a total fee at or slightly below -maxtxfee (~0.05290000)")
195+        res = self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"feeRate": 0.1, "add_inputs": True})
196         assert_approx(res["fee"], 0.055, 0.005)
197 
198-        # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
199+        self.log.info("Test walletcreatefundedpsbt feeRate of 10 BTC/KB produces total fee well above -maxtxfee and raises RPC error")

Xekyo commented at 8:27 pm on October 28, 2020:

Nit: kelvinbytes

I know, I know, “kilo” is the odd one out because generally the prefixes for multiples are capitalized. Just being a bit of a unit nerd here.

(To be clear, when I mark a comment with “Nit:” I consider it optional for the author to address it.)

jonatack commented at 10:10 pm on October 28, 2020:

done

in test/functional/rpc_psbt.py:178 in 36b6ab7494 outdated

173@@ -174,8 +174,11 @@ def run_test(self):
174             elif out['scriptPubKey']['addresses'][0] == p2pkh:
175                 p2pkh_pos = out['n']
176 
177+        inputs = [{"txid": txid, "vout": p2wpkh_pos}, {"txid": txid, "vout": p2sh_p2wpkh_pos}, {"txid": txid, "vout": p2pkh_pos}]
178+        outputs = [{self.nodes[1].getnewaddress(): 29.99}]

Xekyo commented at 8:32 pm on October 28, 2020:

Love how much more readable the tests below become. Good improvement.

in test/functional/wallet_send.py:230 in 328ac2b0a3 outdated

225@@ -224,8 +226,10 @@ def run_test(self):
226         assert_equal(self.nodes[1].decodepsbt(res1["psbt"])["fee"],
227                      self.nodes[1].decodepsbt(res2["psbt"])["fee"])
228         # but not at the same time
229-        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical",
230-                       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"))
231+        for mode in ["unset", "economical", "conservative", "btc/kb", "sat/b"]:
232+            self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical",

Xekyo commented at 8:38 pm on October 28, 2020:

I noticed that both arg_estimate_mode and estimate_mode appear in these test parameters. That seems odd. I wonder whether both pertain to the same value. If one of them would supersede the other, and that happened to be arg_estimate_mode, we would actually only be testing for economical here. If they actually do refer both to the same value, they should probably both be set to =mode.

jonatack commented at 10:03 pm on October 28, 2020:

Good point. In the send RPC, conf_target and estimate_mode can be both args (arg_estimate_mode in this test file) and options (estimate_mode in this test file).

There seem to be some oddities or bugs in the send RPC related to this, as described in #20220 (review). The send RPC is marked as experimental, but these should be fixed or made consistent with the other RPCs after this is merged. I started to look into it but didn’t find a fix quickly, and the PR is already quite large and the deadline for 0.21 branch-off this weekend is looming.

Xekyo commented at 0:01 am on October 29, 2020:

Okay, since this is only in a test, it may be fine to keep track of it, but not get too hung up on fixing every little bit before the merge.

in test/functional/wallet_send.py:286 in 328ac2b0a3 outdated

290+
291+        for mode in ["foo", Decimal("3.141592")]:
292+            self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=0.1, estimate_mode=mode,
293+                expect_error=(-8, "Invalid estimate_mode parameter"))
294+            # TODO: these 2 equivalent sends with an invalid estimate_mode arg should both fail, but they do not...why?
295+            # self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode,

Xekyo commented at 8:43 pm on October 28, 2020:

The only difference I see in this line is that it now uses arg_conf_target where conf_target was used in 284 and arg_estimate_mode where estimate_mode was used. Potentially, the sanitation for those parameters are different?

jonatack commented at 10:04 pm on October 28, 2020:

Yes, there seems to be a bug in the send RPC: per the code in wallet/rpcwallet.cpp, these should behave the same whether passed as an arg or an option, but they do not.

jonatack commented at 6:26 am on November 5, 2020:

Yes, there seems to be a bug in the send RPC: per the code in wallet/rpcwallet.cpp, these should behave the same whether passed as an arg or an option, but they do not.

Fixed in #20305.

in src/wallet/rpcwallet.cpp:443 in c80e2276fb outdated

439@@ -440,7 +440,8 @@ static RPCHelpMan sendtoaddress()
440                     {"subtractfeefromamount", RPCArg::Type::BOOL, /* default */ "false", "The fee will be deducted from the amount being sent.\n"
441                                          "The recipient will receive less bitcoins than you enter in the amount field."},
442                     {"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Allow this transaction to be replaced by a transaction with higher fees via BIP 125"},
443-                    {"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)"},
444+                    {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"

Xekyo commented at 8:46 pm on October 28, 2020:

I see that the third argument here has changed from "wallet default" to "wallet -txconfirmtarget", but it’s not clear to me what the effect of that would be.

jonatack commented at 10:09 pm on October 28, 2020:

This is the RPC help output the user sees. The idea was to make the conf_target help correct (some were missing the sat/B mention), consistent and helpful across the 6 RPCs. The txconfirmtarget mention in some of the conf_target helps seems more useful info than just “wallet default”, so I made them all the same/consistent. Open to disagreement about that :)

jonatack commented at 10:31 pm on October 28, 2020:

e.g. before

07. conf_target      (numeric, optional, default=wallet default) Confirmation target (in blocks), or fee rate (for BTC/kB or sat/B estimate modes)

after

07. conf_target      (numeric, optional, default=wallet -txconfirmtarget) Confirmation target (in blocks)
1                    or fee rate (for BTC/kB and sat/B estimate modes)

Xekyo commented at 0:03 am on October 29, 2020:

I see. That looks like a good change to me.

Xekyo commented at 8:52 pm on October 28, 2020: member

Looks good to me. There are two spots where I wonder whether I found an actual issue, the rest are just nits that I consider optional.

jonatack force-pushed on Oct 28, 2020

wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() 3ac7b0c6f1

test: refactor for walletcreatefundedpsbt fee rate coverage 6e1ea4273e

wallet: add walletcreatefundedpsbt explicit fee rate coverage 44e7bfa603

wallet: add sendtoaddress/sendmany explicit fee rate coverage dd341e602d

wallet: add rpc send explicit fee rate coverage 603c005083

wallet, rpc: fix send subtract_fee_from_outputs help 778b9be406

rpc: update conf_target helps for correctness/consistency

for sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, bumpfee

0be29000c0

jonatack force-pushed on Oct 28, 2020

jonatack commented at 11:27 pm on October 28, 2020: member

Thanks @Xekyo, updated units and comments per git diff c80e227 0be2900.

  0diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
  1index a0e1d1b8a3..85ecb40354 100755
  2--- a/test/functional/rpc_fundrawtransaction.py
  3+++ b/test/functional/rpc_fundrawtransaction.py
  4@@ -721,7 +721,7 @@ class RawTransactionsTest(BitcoinTestFramework):
  5                     assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008",
  6                         lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": mode, "conf_target": n}))
  7 
  8-        for unit, fee_rate in {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():
  9+        for unit, fee_rate in {"sat/B": 0.99999999, "BTC/kB": 0.00000999}.items():
 10             self.log.info("- raises RPC error 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit))
 11             assert_raises_rpc_error(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
 12                 lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True}))
 13diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
 14index 31a28d6ae7..28bcc516c6 100755
 15--- a/test/functional/rpc_psbt.py
 16+++ b/test/functional/rpc_psbt.py
 17@@ -238,7 +238,7 @@ class PSBTTest(BitcoinTestFramework):
 18             assert_raises_rpc_error(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
 19                 lambda: self.nodes[1].walletcreatefundedpsbt(inputs, outputs, 0, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True}))
 20 
 21-        self.log.info("Test walletcreatefundedpsbt feeRate of 10 BTC/KB produces total fee well above -maxtxfee and raises RPC error")
 22+        self.log.info("Test walletcreatefundedpsbt feeRate of 10 BTC/kB produces total fee well above -maxtxfee and raises RPC error")
 23         # previously this was silently capped at -maxtxfee
 24         for bool_add, outputs_array in {True: outputs, False: [{self.nodes[1].getnewaddress(): 1}]}.items():
 25             assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)",
 26diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
 27index 584be1da94..411ae3db29 100755
 28--- a/test/functional/wallet_basic.py
 29+++ b/test/functional/wallet_basic.py
 30@@ -228,7 +228,7 @@ class WalletTest(BitcoinTestFramework):
 31         assert_equal(self.nodes[2].getbalance(), node_2_bal)
 32         node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].gettransaction(txid)['hex']))
 33 
 34-        self.log.info("Test explicit fee (sendmany as BTC/kB)")
 35+        self.log.info("Test case-insensitive explicit fee rate (sendmany as BTC/kB)")
 36         # Throw if no conf_target provided
 37         assert_raises_rpc_error(-8, "Selected estimate_mode bTc/kB requires a fee rate to be specified in conf_target",
 38             self.nodes[2].sendmany,
 39@@ -254,7 +254,7 @@ class WalletTest(BitcoinTestFramework):
 40         node_0_bal += Decimal('10')
 41         assert_equal(self.nodes[0].getbalance(), node_0_bal)
 42 
 43-        self.log.info("Test explicit fee (sendmany as sat/B)")
 44+        self.log.info("Test case-insensitive explicit fee rate (sendmany as sat/B)")
 45         # Throw if no conf_target provided
 46         assert_raises_rpc_error(-8, "Selected estimate_mode sat/b requires a fee rate to be specified in conf_target",
 47             self.nodes[2].sendmany,
 48@@ -283,7 +283,7 @@ class WalletTest(BitcoinTestFramework):
 49         assert_equal(self.nodes[0].getbalance(), node_0_bal)
 50 
 51         # Test setting explicit fee rate just below the minimum.
 52-        for unit, fee_rate in {"BTC/KB": 0.00000999, "SAT/B": 0.99999999}.items():
 53+        for unit, fee_rate in {"BTC/kB": 0.00000999, "sat/B": 0.99999999}.items():
 54             self.log.info("Test sendmany raises 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit))
 55             assert_raises_rpc_error(-6, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
 56                 self.nodes[2].sendmany, amounts={address: 10}, estimate_mode=unit, conf_target=fee_rate)
 57@@ -421,7 +421,7 @@ class WalletTest(BitcoinTestFramework):
 58             self.nodes[0].generate(1)
 59             self.sync_all(self.nodes[0:3])
 60 
 61-            self.log.info("Test explicit fee (sendtoaddress as BTC/kB)")
 62+            self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as BTC/kB)")
 63             self.nodes[0].generate(1)
 64             self.sync_all(self.nodes[0:3])
 65             prebalance = self.nodes[2].getbalance()
 66@@ -456,7 +456,7 @@ class WalletTest(BitcoinTestFramework):
 67 
 68             self.sync_all(self.nodes[0:3])
 69 
 70-            self.log.info("Test explicit fee (sendtoaddress as sat/B)")
 71+            self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as sat/B)")
 72             self.nodes[0].generate(1)
 73             prebalance = self.nodes[2].getbalance()
 74             assert prebalance > 2
 75@@ -489,7 +489,7 @@ class WalletTest(BitcoinTestFramework):
 76             assert_fee_amount(fee, tx_size, Decimal('0.00002000'))
 77 
 78             # Test setting explicit fee rate just below the minimum.
 79-            for unit, fee_rate in {"BTC/KB": 0.00000999, "SAT/B": 0.99999999}.items():
 80+            for unit, fee_rate in {"BTC/kB": 0.00000999, "sat/B": 0.99999999}.items():
 81                 self.log.info("Test sendtoaddress raises 'fee rate too low' if conf_target {} and estimate_mode {} are passed".format(fee_rate, unit))
 82                 assert_raises_rpc_error(-6, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
 83                     self.nodes[2].sendtoaddress, address=address, amount=1, estimate_mode=unit, conf_target=fee_rate)
 84diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py
 85index a6002b67f0..5840a24404 100755
 86--- a/test/functional/wallet_send.py
 87+++ b/test/functional/wallet_send.py
 88@@ -271,7 +271,7 @@ class WalletSendTest(BitcoinTestFramework):
 89         fee = self.nodes[1].decodepsbt(res["psbt"])["fee"]
 90         assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00003"))
 91 
 92-        # TODO: This test should pass with all modes, e.g. with the next line uncommented.
 93+        # TODO: This test should pass with all modes, e.g. with the next line uncommented, for consistency with the other explicit feerate RPCs.
 94         # for mode in ["unset", "economical", "conservative", "btc/kb", "sat/b"]:
 95         for mode in ["btc/kb", "sat/b"]:
 96             self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode=mode,
 97@@ -285,24 +285,24 @@ class WalletSendTest(BitcoinTestFramework):
 98             # TODO: these 2 equivalent sends with an invalid estimate_mode arg should both fail, but they do not...why?
 99             # self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=0.1, arg_estimate_mode=mode,
100             #     expect_error=(-8, "Invalid estimate_mode parameter"))
101-            # assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w0.getnewaddress(): 1}, 0.1, mode))
102+            # assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w1.getnewaddress(): 1}, 0.1, mode))
103 
104-        # TODO: These tests should pass but they do not.
105+        # TODO: These tests should pass for consistency with the other explicit feerate RPCs, but they do not.
106         # for mode in ["unset", "economical", "conservative", "btc/kb", "sat/b"]:
107         #     self.log.debug("{}".format(mode))
108         #     for k, v in {"string": "", "object": {"foo": "bar"}}.items():
109         #         self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=v, estimate_mode=mode,
110         #             expect_error=(-3, "Expected type number for conf_target, got {}".format(k)))
111 
112-        # TODO: error should use sat/b
113+        # TODO: error should use sat/B instead of BTC/kB if sat/B is selected.
114         # Test setting explicit fee rate just below the minimum.
115-        for unit, fee_rate in {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():
116+        for unit, fee_rate in {"sat/B": 0.99999999, "BTC/kB": 0.00000999}.items():
117             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))
118             self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=fee_rate, estimate_mode=unit,
119                 expect_error=(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)"))
120 
121         self.log.info("Explicit fee rate raises RPC error if estimate_mode is passed without a conf_target")
122-        for unit, fee_rate in {"SAT/B": 100, "BTC/KB": 0.001}.items():
123+        for unit, fee_rate in {"sat/B": 100, "BTC/kB": 0.001}.items():
124             self.test_send(from_wallet=w0, to_wallet=w1, amount=1, estimate_mode=unit,
125                 expect_error=(-8, "Selected estimate_mode {} requires a fee rate to be specified in conf_target".format(unit)))

Xekyo commented at 0:09 am on October 29, 2020: member

Just ran the functional tests (for the previous commit I reviewed), which takes a surprisingly long time. Luckily, the first hit on an internet search for how to run the functional tests was this guide by some Jon Atack, made it really easy to get set up. :laughing:

Xekyo commented at 0:17 am on October 29, 2020: member

Changes look great to me. utack 0be2900 currently running functional tests.

sipa commented at 0:25 am on October 29, 2020: member

@Xekyo You can run them in parallel; if you have sufficient RAM pretty extremely even. test_runner.py -j60 works fine on my 4-core 32 GiB RAM system, taking 3m46s. A lot of the time consists of processes waiting for each other, so it’s not actually CPU bound.

jonatack commented at 0:34 am on October 29, 2020: member

You can run them in parallel; if you have sufficient RAM pretty extremely even. test_runner.py -j60 works fine on my 4-core 32 GIB RAM system

Good point; I’ll mention that in the guide.

Xekyo commented at 0:37 am on October 29, 2020: member

tack (functional tests only) 0be29000c011dec0722481dbebb159873da6fa54

jonatack renamed this:
~~wallet, rpc: explicit fee rate follow-ups for 0.21~~
wallet, rpc: explicit fee rate follow-ups/fixes for 0.21
on Nov 1, 2020

in src/wallet/rpcwallet.cpp:3379 in 0be29000c0

3375@@ -3373,7 +3376,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
3376         "The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist.\n"
3377         "All inputs in the original transaction will be included in the replacement transaction.\n"
3378         "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
3379-        "By default, the new fee will be calculated automatically using estimatesmartfee.\n"
3380+        "By default, the new fee will be calculated automatically using the estimatesmartfee RPC.\n"

kallewoof commented at 4:01 am on November 2, 2020:

I like the original better on this one, but I’m fine with the change.

jonatack commented at 12:45 pm on November 2, 2020:

Thanks @kallewoof–the idea was to help users know what estimatesmartfee is and where to look for more info without grepping the codebase, even though it’s less exact than writing, say, “using CBlockPolicyEstimator::estimateSmartFee in src/fees.cpp”. I don’t feel strongly about it though.

kallewoof commented at 11:55 am on November 2, 2020: member

Concept/Tested ACK 0be29000c011dec0722481dbebb159873da6fa54

It feels like this touches a fair bit of code that is unrelated to the explicit fee rate stuff, but maybe it’s related and I’m just not seeing it. Otherwise looks good.

(I was unable to run test_runner with any kind of -j flags, though both machines had bitcoin daemons running which it claims could cause problems.)

jonatack commented at 12:50 pm on November 2, 2020: member

@Xekyo You can run them in parallel; if you have sufficient RAM pretty extremely even. test_runner.py -j60 works fine on my 4-core 32 GiB RAM system, taking 3m46s. A lot of the time consists of processes waiting for each other, so it’s not actually CPU bound.

I have a sloow 4-core 32GB RAM laptop and test/functional/test_runner.py -j60 does run much faster–good tip.

meshcollider commented at 3:35 am on November 4, 2020: contributor

Code review + functional test run ACK 0be29000c011dec0722481dbebb159873da6fa54

Very nice improvements & cleanups, thanks jonatack!

meshcollider merged this on Nov 4, 2020

meshcollider closed this on Nov 4, 2020

sidhujag referenced this in commit 9a76be1d46 on Nov 4, 2020

jonatack deleted the branch on Nov 4, 2020

jonatack commented at 8:42 am on November 4, 2020: member

Thank you @Xekyo, @kallewoof and @meshcollider!

MarcoFalke referenced this in commit 80e32e120e on Nov 17, 2020

deadalnix referenced this in commit dbd80d6673 on Dec 22, 2021

DrahtBot locked this on Feb 15, 2022

wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 #20220

Conflicts