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

jonatack · 2020-10-22T16:57:06Z

Follow-up to #11413 providing a base to build on for #19543:

bugfix for bumpfee raising a JSON error with explicit feerates, fixes issue RPC bumpfee error with explicit feerate #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.

jonatack · 2020-10-22T17:06:14Z

src/wallet/rpcwallet.cpp

@@ -3461,7 +3461,6 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
            if (options.exists("fee_rate")) {
                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.");
            }
-            coin_control.m_confirm_target = ParseConfirmTarget(conf_target, pwallet->chain().estimateMaxBlocks());


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

jonatack · 2020-10-23T10:57:10Z

Pushed a few improvements. Should be ready for review.

murchandamus · 2020-10-23T20:13:59Z

test/functional/wallet_bumpfee.py

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


Isn't the BTC_MODE then essentially a duplicate of the {"fee_rate": …}` option?

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.

test/functional/wallet_bumpfee.py

murchandamus · 2020-10-23T20:20:13Z

src/wallet/rpcwallet.cpp

@@ -3459,7 +3459,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)

        if (!conf_target.isNull()) {
            if (options.exists("fee_rate")) {
-                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.");
+                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.");


So much clearer! 👍

test/functional/wallet_bumpfee.py

murchandamus · 2020-10-23T20:32:37Z

test/functional/wallet_bumpfee.py


+    self.log.info("Test explicit feerate raises RPC error if estimate_mode is passed without a conf_target")
+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL, "estimate_mode": BTC_MODE})
+    assert_raises_rpc_error(-8, "Selected estimate_mode requires a fee rate", rbf_node.bumpfee, rbfid, {"fee_rate": 10, "estimate_mode": SAT_MODE})


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?

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.

+++ b/src/wallet/rpcwallet.cpp @@ -214,7 +214,7 @@ static void SetFeeEstimateMode(const CWallet* pwallet, CCoinControl& cc, const U if (cc.m_fee_mode == FeeEstimateMode::BTC_KB || cc.m_fee_mode == FeeEstimateMode::SAT_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())); }

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

Done in fc57217

test/functional/rpc_psbt.py

murchandamus · 2020-10-23T21:22:52Z

test/functional/rpc_psbt.py

        # 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":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})
-        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})
+        for bool_add, addr in {True: addr, False: {self.nodes[1].getnewaddress(): 1}}.items():


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

murchandamus · 2020-10-23T21:28:43Z

test/functional/rpc_psbt.py

        res = self.nodes[1].walletcreatefundedpsbt(inputs, addr, 0, {"feeRate": 0.1, "add_inputs": True})
        assert_approx(res["fee"], 0.055, 0.005)

+        self.log.info("Test passing walletcreatefundedpsbt explicit feerate with conf_target and estimate_mode")
+        for unit, feerate in {"btc/kb": 0.1, "sat/b": 10000}.items():


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.

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.

test/functional/rpc_psbt.py

murchandamus · 2020-10-23T21:35:37Z

src/wallet/rpcwallet.cpp

@@ -4034,7 +4034,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"


Oh my, great improvement!

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

jonatack · 2020-10-26T20:47:28Z

@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 · 2020-10-27T05:48:46Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

might be best reviewed with: git show COMMIT_HASH -w --color-moved=dimmed-zebra

jonatack · 2020-10-27T21:25:21Z

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.

murchandamus · 2020-10-28T03:38:09Z

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

murchandamus

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.

test/functional/wallet_basic.py

test/functional/rpc_psbt.py

murchandamus · 2020-10-28T20:32:04Z

test/functional/rpc_psbt.py

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


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

murchandamus · 2020-10-28T20:38:56Z

test/functional/wallet_send.py

-        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", "btc/kb", "sat/b"]:
+            self.test_send(from_wallet=w0, to_wallet=w1, amount=1, arg_conf_target=1, arg_estimate_mode="economical",


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.

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 (comment). 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.

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.

murchandamus · 2020-10-28T20:43:32Z

test/functional/wallet_send.py

+            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,


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?

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.

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.

murchandamus · 2020-10-28T20:46:14Z

src/wallet/rpcwallet.cpp

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


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.

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 :)

e.g. before

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

after

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

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

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

jonatack · 2020-10-28T23:27:44Z

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

git diff c80e227 0be2900

diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
index a0e1d1b8a3..85ecb40354 100755
--- a/test/functional/rpc_fundrawtransaction.py
+++ b/test/functional/rpc_fundrawtransaction.py
@@ -721,7 +721,7 @@ class RawTransactionsTest(BitcoinTestFramework):
                     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 {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():
+        for unit, fee_rate in {"sat/B": 0.99999999, "BTC/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 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
                 lambda: self.nodes[1].fundrawtransaction(rawtx, {"estimate_mode": unit, "conf_target": fee_rate, "add_inputs": True}))
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 31a28d6ae7..28bcc516c6 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -238,7 +238,7 @@ class PSBTTest(BitcoinTestFramework):
             assert_raises_rpc_error(-4, "Fee rate (0.00000999 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/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 BTC/KB produces total fee well above -maxtxfee and raises RPC error")
+        self.log.info("Test walletcreatefundedpsbt feeRate of 10 BTC/kB 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)",
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index 584be1da94..411ae3db29 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -228,7 +228,7 @@ class WalletTest(BitcoinTestFramework):
         assert_equal(self.nodes[2].getbalance(), node_2_bal)
         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']))
 
-        self.log.info("Test explicit fee (sendmany as BTC/kB)")
+        self.log.info("Test case-insensitive explicit fee rate (sendmany as BTC/kB)")
         # Throw if no conf_target provided
         assert_raises_rpc_error(-8, "Selected estimate_mode bTc/kB requires a fee rate to be specified in conf_target",
             self.nodes[2].sendmany,
@@ -254,7 +254,7 @@ class WalletTest(BitcoinTestFramework):
         node_0_bal += Decimal('10')
         assert_equal(self.nodes[0].getbalance(), node_0_bal)
 
-        self.log.info("Test explicit fee (sendmany as sat/B)")
+        self.log.info("Test case-insensitive explicit fee rate (sendmany as sat/B)")
         # Throw if no conf_target provided
         assert_raises_rpc_error(-8, "Selected estimate_mode sat/b requires a fee rate to be specified in conf_target",
             self.nodes[2].sendmany,
@@ -283,7 +283,7 @@ class WalletTest(BitcoinTestFramework):
         assert_equal(self.nodes[0].getbalance(), node_0_bal)
 
         # Test setting explicit fee rate just below the minimum.
-        for unit, fee_rate in {"BTC/KB": 0.00000999, "SAT/B": 0.99999999}.items():
+        for unit, fee_rate in {"BTC/kB": 0.00000999, "sat/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 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
                 self.nodes[2].sendmany, amounts={address: 10}, estimate_mode=unit, conf_target=fee_rate)
@@ -421,7 +421,7 @@ class WalletTest(BitcoinTestFramework):
             self.nodes[0].generate(1)
             self.sync_all(self.nodes[0:3])
 
-            self.log.info("Test explicit fee (sendtoaddress as BTC/kB)")
+            self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as BTC/kB)")
             self.nodes[0].generate(1)
             self.sync_all(self.nodes[0:3])
             prebalance = self.nodes[2].getbalance()
@@ -456,7 +456,7 @@ class WalletTest(BitcoinTestFramework):
 
             self.sync_all(self.nodes[0:3])
 
-            self.log.info("Test explicit fee (sendtoaddress as sat/B)")
+            self.log.info("Test case-insensitive explicit fee rate (sendtoaddress as sat/B)")
             self.nodes[0].generate(1)
             prebalance = self.nodes[2].getbalance()
             assert prebalance > 2
@@ -489,7 +489,7 @@ class WalletTest(BitcoinTestFramework):
             assert_fee_amount(fee, tx_size, Decimal('0.00002000'))
 
             # Test setting explicit fee rate just below the minimum.
-            for unit, fee_rate in {"BTC/KB": 0.00000999, "SAT/B": 0.99999999}.items():
+            for unit, fee_rate in {"BTC/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 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)",
                     self.nodes[2].sendtoaddress, address=address, amount=1, estimate_mode=unit, conf_target=fee_rate)
diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py
index a6002b67f0..5840a24404 100755
--- a/test/functional/wallet_send.py
+++ b/test/functional/wallet_send.py
@@ -271,7 +271,7 @@ class WalletSendTest(BitcoinTestFramework):
         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.
+        # 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", "btc/kb", "sat/b"]:
         for mode in ["btc/kb", "sat/b"]:
             self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode=mode,
@@ -285,24 +285,24 @@ class WalletSendTest(BitcoinTestFramework):
             # 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({w0.getnewaddress(): 1}, 0.1, mode))
+            # assert_raises_rpc_error(-8, "Invalid estimate_mode parameter", lambda: w0.send({w1.getnewaddress(): 1}, 0.1, mode))
 
-        # TODO: These tests should pass but they do not.
+        # TODO: These tests should pass for consistency with the other explicit feerate RPCs, but they do not.
         # for mode in ["unset", "economical", "conservative", "btc/kb", "sat/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 sat/b
+        # TODO: error should use sat/B instead of BTC/kB if sat/B is selected.
         # Test setting explicit fee rate just below the minimum.
-        for unit, fee_rate in {"SAT/B": 0.99999999, "BTC/KB": 0.00000999}.items():
+        for unit, fee_rate in {"sat/B": 0.99999999, "BTC/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 BTC/kB) is lower than the minimum fee rate setting (0.00001000 BTC/kB)"))
 
         self.log.info("Explicit fee rate raises RPC error if estimate_mode is passed without a conf_target")
-        for unit, fee_rate in {"SAT/B": 100, "BTC/KB": 0.001}.items():
+        for unit, fee_rate in {"sat/B": 100, "BTC/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)))

murchandamus · 2020-10-29T00:09:29Z

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. 😆

murchandamus · 2020-10-29T00:17:01Z

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

sipa · 2020-10-29T00:25:35Z

@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 · 2020-10-29T00:34:06Z

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.

murchandamus · 2020-10-29T00:37:33Z

tack (functional tests only) 0be2900

kallewoof

Concept/Tested ACK 0be2900

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.)

src/wallet/rpcwallet.cpp

jonatack · 2020-11-02T12:50:52Z

@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

Code review + functional test run ACK 0be2900

Very nice improvements & cleanups, thanks jonatack!

…for 0.21

jonatack · 2020-11-04T08:42:07Z

Thank you @xekyo, @kallewoof and @meshcollider!

05e82d8 wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack) 9a670b4 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack) be481b7 wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack) 449b730 wallet: provide valid values if invalid estimate mode passed (Jon Atack) 6da3afb wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack) 173b5b5 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack) 7f9835a wallet: remove fee rates from conf_target helps (Jon Atack) b7994c0 wallet: add fee_rate unit warnings to bumpfee (Jon Atack) 410e471 wallet: remove redundant bumpfee fee_rate checks (Jon Atack) a0d4957 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack) e21212f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack) 6112cf2 wallet: add CFeeRate ctor doxygen documentation (Jon Atack) 3f72791 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 #20220 (comment) 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 05e82d8 did not test and found a few style nits, which can be fixed later 🍯 Xekyo: tACK 05e82d8 Sjors: utACK 05e82d8 Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c

Summary: This is a partial backport of [[bitcoin/bitcoin#20220 | core#20220]] Most of the PR is not applicable because we do not use `estimate_mode` nor `conf_target` for fees. List of changes and source commits: - Use a const ref in `FundTransaction` for `options`: bitcoin/bitcoin@1697a40 - Minor rpc_psbt.py refactoring: bitcoin/bitcoin@6e1ea42 - Add a `log.info` in wallet_basic.py: bitcoin/bitcoin@dd341e6 - Improve RPC doc for `subtract_fee_from_outputs`: bitcoin/bitcoin@778b9be Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10716

jonatack mentioned this pull request Oct 22, 2020

rpc: remove deprecated getaddressinfo fields #19200

Merged

jonatack commented Oct 22, 2020

View reviewed changes

This was referenced Oct 22, 2020

RPC bumpfee error with explicit feerate #20219

Closed

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #11413

Merged

Normalize fee units for RPC ("BTC/kB" and "sat/B) #19543

Closed

DrahtBot added RPC/REST/ZMQ Wallet labels Oct 22, 2020

jonatack changed the title ~~rpc, test, doc: explicit feerate follow-ups~~ wallet, rpc, test: explicit feerate follow-ups Oct 22, 2020

laanwj added this to the 0.21.0 milestone Oct 22, 2020

maflcko changed the title ~~wallet, rpc, test: explicit feerate follow-ups~~ wallet, rpc: explicit feerate follow-ups Oct 23, 2020

jonatack force-pushed the explicit-feerate-follow-ups branch 2 times, most recently from 9bd1253 to f97a3cd Compare October 23, 2020 10:55

jonatack mentioned this pull request Oct 23, 2020

wallet, rpc: universal feerate (sat/vB) param/option #20231

Closed

murchandamus reviewed Oct 23, 2020

View reviewed changes

jonatack added 2 commits October 24, 2020 22:03

wallet, bugfix: fix bumpfee with explicit fee rate modes

052427e

wallet: fix SetFeeEstimateMode() error message

fc57217

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

luke-jr mentioned this pull request Oct 26, 2020

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

Closed

jonatack added 2 commits October 27, 2020 21:33

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

1697a40

wallet: combine redundant bumpfee invalid params and args tests

2d8eba8

might be best reviewed with: git show COMMIT_HASH -w --color-moved=dimmed-zebra

jonatack force-pushed the explicit-feerate-follow-ups branch from f97a3cd to c80e227 Compare October 27, 2020 21:16

jonatack changed the title ~~wallet, rpc: explicit feerate follow-ups~~ wallet, rpc: explicit fee rate follow-ups for 0.21 Oct 27, 2020

DrahtBot mentioned this pull request Oct 28, 2020

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

murchandamus reviewed Oct 28, 2020

View reviewed changes

jonatack force-pushed the explicit-feerate-follow-ups branch from c80e227 to c9fccaf Compare October 28, 2020 23:13

wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget()

3ac7b0c

jonatack added 6 commits October 29, 2020 00:22

test: refactor for walletcreatefundedpsbt fee rate coverage

6e1ea42

wallet: add walletcreatefundedpsbt explicit fee rate coverage

44e7bfa

wallet: add sendtoaddress/sendmany explicit fee rate coverage

dd341e6

wallet: add rpc send explicit fee rate coverage

603c005

wallet, rpc: fix send subtract_fee_from_outputs help

778b9be

rpc: update conf_target helps for correctness/consistency

0be2900

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

jonatack force-pushed the explicit-feerate-follow-ups branch from c9fccaf to 0be2900 Compare October 28, 2020 23:24

murchandamus mentioned this pull request Oct 29, 2020

test: consistently use BTC/kB and sat/B in wallet_basic test #20041

Closed

DrahtBot mentioned this pull request Oct 31, 2020

Add missing thread safety annotations #20272

Closed

jonatack changed the title ~~wallet, rpc: explicit fee rate follow-ups for 0.21~~ wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 Nov 1, 2020

kallewoof reviewed Nov 2, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

meshcollider reviewed Nov 4, 2020

View reviewed changes

meshcollider merged commit 5d32009 into bitcoin:master Nov 4, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 4, 2020

Merge bitcoin#20220: wallet, rpc: explicit fee rate follow-ups/fixes …

9a76be1

…for 0.21

jonatack deleted the explicit-feerate-follow-ups branch November 4, 2020 08:40

jonatack mentioned this pull request Nov 5, 2020

wallet: introduce fee_rate sat/vB param/option #20305

Merged

jonatack mentioned this pull request Jan 20, 2021

wallet: check for non-representable CFeeRates #20546

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

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

Conversation

jonatack commented Oct 22, 2020 • edited

jonatack Oct 22, 2020 • edited

Choose a reason for hiding this comment

jonatack commented Oct 23, 2020

Choose a reason for hiding this comment

jonatack Oct 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Oct 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Oct 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Oct 26, 2020

DrahtBot commented Oct 27, 2020 • edited

Conflicts

jonatack commented Oct 27, 2020

murchandamus commented Oct 28, 2020

murchandamus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Oct 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack Oct 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Oct 28, 2020

murchandamus commented Oct 29, 2020

murchandamus commented Oct 29, 2020

sipa commented Oct 29, 2020 • edited

jonatack commented Oct 29, 2020

murchandamus commented Oct 29, 2020

kallewoof left a comment

Choose a reason for hiding this comment

jonatack commented Nov 2, 2020

meshcollider left a comment

Choose a reason for hiding this comment

jonatack commented Nov 4, 2020

jonatack commented Oct 22, 2020 •

edited

jonatack Oct 22, 2020 •

edited

jonatack Oct 24, 2020 •

edited

jonatack Oct 24, 2020 •

edited

jonatack Oct 23, 2020 •

edited

DrahtBot commented Oct 27, 2020 •

edited

jonatack Oct 28, 2020 •

edited

jonatack Oct 28, 2020 •

edited

sipa commented Oct 29, 2020 •

edited