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

jonatack · 2020-11-05T06:26:20Z

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 wallet, rpc: explicit fee rate follow-ups/fixes for 0.21 #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

maflcko · 2020-11-05T07:18:33Z

How does this differ/compare to #20250 ?

maflcko · 2020-11-05T07:25:36Z

I guess this one is adding a new separate option, #20250 is adding named aliases and keeping the existing options.

jonatack · 2020-11-05T07:42:28Z

Yes, this one removes the conf_target/estimate_mode overloading and introduces a standard fee rate param.

#20250 keeps the current overloading and makes it more consistent between the six RPCs. Some of what it does (feeRate -> fee_rate) is compatible or orthogonal.

ISTM the question is, do we want to release with the overloading or with a standard feerate param. (It would be simpler if we decide before the branch-off. After, this PR would need to be changed as we would have to support both.)

DrahtBot · 2020-11-05T10:09:57Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391 (wallet: introduce setfeerate (an improved settxfee, in sat/vB) by jonatack)
test: Implicitly sync after generate* to preempt races and intermittent test failures #20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)
wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418 (wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 by fjahr)
Add Single Random Draw as an additional coin selection algorithm #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
Use effective values throughout coin selection #17331 (Use effective values throughout coin selection by achow101)
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.

luke-jr · 2020-11-06T19:26:16Z

AFAICT, the only "fee_rate" in 0.20 was bumpfee. Specifying BTC/vkB (current value) in place of the new sat/vB would always be a much lower feerate. So it'd most likely be too low and error, or worst case lower than you intended and you can just bump it again to fix.

With that in mind, I think the option here should just be renamed to "fee_rate" and break the compatibility for bumpfee.

jonatack · 2020-11-06T19:42:18Z

@luke-jr SGTM and that would further simplify the implementation too.

achow101 · 2020-11-06T22:14:11Z

Agree with renaming to fee_rate. However having two fee rate options for some RPCs is kinda weird. I suppose it's too late to fix that for this release, but it'd be nice to not do that in the future.

Concept ACK.

meshcollider · 2020-11-07T06:13:12Z

Concept ACK for fee_rate

promag

Why fee_rate_sat_vb and not just fee_rate? Also, should not change verbose parameter index?

jonatack · 2020-11-09T10:42:11Z

Why fee_rate_sat_vb and not just fee_rate?

I initially began with fee_rate in #20231. Then, based on #20220 (comment) I changed to fee_rate_sat_vb here. Today, based on the feedback at last Friday's wallet meeting and here, I will push an update to use fee_rate.

Also, should not change verbose parameter index?

For sendtoaddress and sendmany, I hesitated on this because I thought that users are used to seeing the verbose argument placed last, e.g. like gettransaction. If positional true or false are passed for the fee_rate, a type error will be raised and the transaction won't proceed. Is it better to put fee_rate after verbose instead?

maflcko · 2020-11-09T10:51:48Z

Has verbose been in a release already?

jonatack · 2020-11-09T11:19:17Z

Per d5863c0 it looks like verbose was added to sendtoaddress and sendmany after the 0.20 release. Not a bugfix, so probably not backported. Checking. Edit: nope, they aren't in 0.20.0 or 0.20.1. We're good.

murchandamus

Partial review up to ~3/4 of 53b35be. Will continue in approx 60 minutes.

src/wallet/rpcwallet.cpp

test/functional/wallet_send.py

murchandamus · 2020-11-09T17:23:40Z

src/wallet/rpcwallet.cpp

-
-        cc.m_feerate = CFeeRate(fee_rate);
+        CAmount feerate{AmountFromValue(estimate_param)};
+        cc.m_feerate = cc.m_fee_mode == FeeEstimateMode::SAT_B ? CFeeRate(feerate, COIN) : CFeeRate(feerate);


It's not immediately obvious to me why CFeeRate(feerate, COIN) is equivalent to fee_rate /= WALLET_BTC_KB_TO_SAT_B; CFeeRate(fee_rate). As far as I see, COIN equals 1e8, but WALLET_BTC_KB_TO_SAT_B equals 1e5.

Upon inspection of ./src/policy/feerate.cpp and ./src/policy/feerate.h, CFeeRate(…) with two arguments appears to construct a fee rate in sat/kvB from fees paid in sats divided by transaction weight in vB:

/** Constructor for a fee rate in satoshis per kB. The size in bytes must not exceed (2^63 - 1)*/ CFeeRate(const CAmount& nFeePaid, size_t nBytes);

CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_) { assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max())); int64_t nSize = int64_t(nBytes_); if (nSize > 0) nSatoshisPerK = nFeePaid * 1000 / nSize; else nSatoshisPerK = 0; }

and either results in the feerate having been divided by 100,000 (1e5). ✔️

Yes 👍 and I was more confident about making this change after all the test coverage we added in #20220.

It would be nice to have a comment explaining this because it is not immediately clear why this works and future work on this code may not see this explanation.

Done, added Doxygen documentation in a3eac6e

This feels like a hack, unless I misunderstand the original purpose of this constructor. Maybe it's better to add a separate constructor with a boolean to distinguish SAT_VB from BTC/kvB.

I asked myself why else the two-argument ctor might have been written and didn't come up with an answer...I'm not against reviewing a refactoring of it.

Looking at settxfee in wallet/rpcwallet.cpp, it uses this ctor in a similar manner.

CAmount nAmount = AmountFromValue(request.params[0]); CFeeRate tx_fee_rate(nAmount, 1000);

This feels like a hack, unless I misunderstand the original purpose of this constructor. Maybe it's better to add a separate constructor with a boolean to distinguish SAT_VB from BTC/kvB.

@Sjors done, proposed a separate constructor in 9c479bf

src/wallet/rpcwallet.cpp

murchandamus · 2020-11-09T18:41:39Z

test/functional/rpc_psbt.py

+        assert_raises_rpc_error(-8, "Invalid fee_rate 0.00000000 BTC/kB (must be greater than 0)",
+            self.nodes[1].walletcreatefundedpsbt, inputs, outputs, 0, {"feeRate": 0, "add_inputs": True})
+        for param, value in {("fee_rate", 100000), ("feeRate", 1.1)}:
+            assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)",


Probably out of scope, but it would be kinda nice if an error like this told the user what said limit is.

Agree--punting on this for the next push but would be good.

test/functional/wallet_basic.py

murchandamus

Looks great to me! Gonna circle back tomorrow after I get up. :)

murchandamus · 2020-11-10T01:28:47Z

test/functional/wallet_bumpfee.py

-            assert_raises_rpc_error(-8, "Insufficient total fee", rbf_node.bumpfee, rbfid, options)
+        # Bumping to just above minrelay should fail to increase the total fee enough.
+        assert_raises_rpc_error(-8, "Insufficient total fee 0.00000141, must be at least 0.00001704 (oldFee 0.00000999 + incrementalFee 0.00000705)",


Getting hung up on all the wrong things, but if minRelayTxFee × txsize is 141 sats, I'm surprised that the incrementalFee is 705. I thought that minimum increments are equal to minRelayTxFee.

I think that's from wallet/wallet.h, though the different fee constants and config options are a bit confusing to me.

//! minimum recommended increment for BIP 125 replacement txs static const CAmount WALLET_INCREMENTAL_RELAY_FEE = 5000;

and wallet/feebumper.cpp

if (new_total_fee < minTotalFee) { errors.push_back(strprintf(Untranslated("Insufficient total fee %s, must be at least %s (oldFee %s + incrementalFee %s)"), FormatMoney(new_total_fee), FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxTxSize)), FormatMoney(incrementalRelayFee.GetFee(maxTxSize))));

Thanks for tracking that down!

murchandamus · 2020-11-10T01:31:46Z

test/functional/wallet_bumpfee.py

+        self.log.info("Test invalid fee rate settings")
+        assert_raises_rpc_error(-8, "Invalid fee_rate 0.00000000 BTC/kB (must be greater than 0)",
+            rbf_node.bumpfee, rbfid, {"fee_rate": 0})
+        assert_raises_rpc_error(-4, "Specified or calculated fee 0.141 is too high (cannot be higher than -maxtxfee 0.10",


woah, damn. I think we may want to have a look at the maxtxfee that's a lot of money.

Could be a good follow-up.

I proposed that once: #16539

I proposed that once: #16539

TIL, thanks. Interesting discussion in that PR.

murchandamus · 2020-11-10T01:40:07Z

src/wallet/rpcwallet.cpp

        "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
-        "returned by getnetworkinfo) to enter the node's mempool.\n",
+        "returned by getnetworkinfo) to enter the node's mempool.\n"
+        "* WARNING: before version 0.21, fee_rate was in " + CURRENCY_UNIT + "/kvB. As of 0.21, fee_rate is in " + CURRENCY_ATOM + "/vB. *\n",


Luckily, this is very benign. In the worst case, someone is going to get upped to the minRelayTxFee silently and sends at 1 sat/vB. Since RBF is on by default, they should be able to bump when they notice. 👍

murchandamus · 2020-11-10T01:41:15Z

src/wallet/rpcwallet.cpp

@@ -4369,8 +4363,7 @@ static RPCHelpMan walletcreatefundedpsbt()
                            },
                            {"replaceable", RPCArg::Type::BOOL, /* default */ "wallet default", "Marks this transaction as BIP125 replaceable.\n"
                                                          "Allows this transaction to be replaced by a transaction with higher fees"},
-                            {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target (in blocks)\n"
-                                                          "or fee rate (for " + CURRENCY_UNIT + "/kB and " + CURRENCY_ATOM + "/B estimate modes)"},
+                            {"conf_target", RPCArg::Type::NUM, /* default */ "wallet -txconfirmtarget", "Confirmation target in blocks"},


Overloading, begone! 🎆

src/policy/feerate.h

murchandamus · 2020-11-10T01:51:17Z

src/wallet/rpcwallet.cpp

@@ -2303,7 +2303,7 @@ static RPCHelpMan settxfee()
                "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n"
                "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n",
                {
-                    {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The transaction fee in " + CURRENCY_UNIT + "/kB"},
+                    {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The transaction fee in " + CURRENCY_UNIT + "/kvB"},


I assume that you considered and deliberately passed on also replacing these. I would surmise that it's it just too much of a can of worms? Otherwise it would be kinda odd that we are using sat/vB for the fee rate in all the send variants, but then use BTC/kvB for settxfee.

Nit: settxfee sets not a fee, but a feerate. ;)

Yep, scope creep on an already-wide PR. Could be a next step.

Done in #20391.

src/wallet/rpcwallet.cpp

achow101 · 2020-11-10T18:11:48Z

ACK 5660dd3

murchandamus · 2020-11-13T23:46:37Z

tACK 05e82d8

Sjors · 2020-11-14T14:08:21Z

If there's still block explorers (that people use) using real bytes instead of virtual bytes in their fee calculation, then indeed adding the v might be necessary. Either way, don't let it hold back this PR.

murchandamus · 2020-11-16T14:41:33Z

Just to provide two examples:

This first is from Blockchair providing the fee per raw size as [sat/B] next to the actual feerate:

This second is Blockchain.com, providing fee per raw size as [sat/B] next to actual [sat/WU]:

luke-jr · 2020-11-16T20:02:46Z

Block explorer websites have been confusing users with bogus "information" forever. Trying to match them doesn't make sense.

maflcko · 2020-11-17T12:48:48Z

review ACK 05e82d8 did not test and found a few style nits, which can be fixed later 🍯

Show signature and timestamp

Signature:

python: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory

Sjors · 2020-11-17T12:56:15Z

I contacted one such explorer and nagged them to switch from bytes to vbytes :-)

maflcko · 2020-11-17T18:38:12Z

This probably needs release notes?

maflcko · 2020-11-17T18:50:45Z

Btw, the first commit adds tests that pass without the code changes. I haven't tried the other added tests, but as a general rule, I'd prefer if new tests were added in a separate commit and not in the same commit that also changes behaviour.

src/wallet/rpcwallet.cpp

jonatack · 2020-11-17T18:53:47Z

This probably needs release notes?

Yes, will update the wiki.

maflcko · 2020-11-17T18:55:42Z

Turns out the style nits are regressions. See https://github.com/bitcoin/bitcoin/pull/20305/files#r525406176 and #20410

MONIMAKER365

#18261

jonatack · 2020-11-26T16:48:25Z

Release notes added to the wiki at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft.

The Needs release note tag can be removed.

in RPCs fundrawtransaction and walletcreatefundedpsbt only. This provides the existing feeRate (BTC/kvB) behavior in these two RPCs to the new fee_rate (sat/vB) param also. See these two GitHub review discussions for more info: bitcoin/bitcoin#10706 bitcoin/bitcoin#20305 (comment)

jonatack mentioned this pull request Nov 5, 2020

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

Merged

fanquake added the Wallet label Nov 5, 2020

jonatack mentioned this pull request Nov 5, 2020

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

Closed

DrahtBot mentioned this pull request Nov 5, 2020

wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418

Merged

maflcko added this to the 0.21.0 milestone Nov 5, 2020

jonatack force-pushed the fee_rate_sat_vb branch from 078b8fb to 2b8d681 Compare November 6, 2020 18:33

luke-jr mentioned this pull request Nov 6, 2020

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

Closed

promag reviewed Nov 9, 2020

View reviewed changes

jonatack changed the title ~~wallet: introduce fee_rate_sat_vb param/option~~ wallet: introduce fee_rate param/option Nov 9, 2020

jonatack force-pushed the fee_rate_sat_vb branch from 2b8d681 to 9c680e7 Compare November 9, 2020 15:26

jonatack changed the title ~~wallet: introduce fee_rate param/option~~ wallet: introduce fee_rate sat/vB param/option Nov 9, 2020

jonatack force-pushed the fee_rate_sat_vb branch from 9c680e7 to 5660dd3 Compare November 9, 2020 15:59

murchandamus reviewed Nov 9, 2020

View reviewed changes

This was referenced Nov 10, 2020

Add Single Random Draw as an additional coin selection algorithm #17526

Merged

Use effective values throughout coin selection #17331

Merged

murchandamus reviewed Nov 10, 2020

View reviewed changes

DrahtBot mentioned this pull request Nov 10, 2020

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362

Closed

jonatack force-pushed the fee_rate_sat_vb branch from 5660dd3 to bcb8e0d Compare November 10, 2020 20:12

jonatack mentioned this pull request Nov 14, 2020

wallet: introduce setfeerate (an improved settxfee, in sat/vB) #20391

Closed

DrahtBot mentioned this pull request Nov 15, 2020

Descriptor Wallets BIP39 recovery #20361

Closed

jonatack mentioned this pull request Nov 17, 2020

rpc: Allow named and positional arguments to be used together #19762

Merged

maflcko merged commit 80e32e1 into bitcoin:master Nov 17, 2020

jonatack deleted the fee_rate_sat_vb branch November 17, 2020 13:04

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

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

8518ab9

maflcko added the Needs release note label Nov 17, 2020

maflcko reviewed Nov 17, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

This was referenced Nov 18, 2020

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

Closed

wallet: allow zero-fee fundrawtransaction/walletcreatefundedpsbt and other fixes #20426

Merged

MONIMAKER365 reviewed Nov 22, 2020

View reviewed changes

jonatack mentioned this pull request Nov 24, 2020

Newsletters: add 125 (2020-11-25) bitcoinops/bitcoinops.github.io#486

Merged

5 tasks

fanquake removed the Needs release note label Nov 26, 2020

This was referenced Nov 30, 2020

wallet: deprecate feeRate in fundrawtransaction/walletcreatefundedpsbt #20483

Closed

Add explicit feeRate option for sendmany RPC method #19662

Closed

jonatack mentioned this pull request Dec 28, 2020

policy, refactor: CFeeRate::FromSatB/FromBtcKb named constructors #20790

Closed

jonatack mentioned this pull request Jan 14, 2021

wallet: check for non-representable CFeeRates #20546

Closed

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

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

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

Uh oh!

Conversation

jonatack commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 5, 2020

Uh oh!

maflcko commented Nov 5, 2020

Uh oh!

jonatack commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr commented Nov 6, 2020

Uh oh!

jonatack commented Nov 6, 2020

Uh oh!

achow101 commented Nov 6, 2020

Uh oh!

meshcollider commented Nov 7, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 9, 2020

Uh oh!

jonatack commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jonatack commented Nov 5, 2020 •

edited

Loading

jonatack commented Nov 5, 2020 •

edited

Loading

DrahtBot commented Nov 5, 2020 •

edited

Loading

jonatack commented Nov 9, 2020 •

edited

Loading

jonatack commented Nov 9, 2020 •

edited

Loading

jonatack Nov 13, 2020 •

edited

Loading

jonatack Nov 10, 2020 •

edited

Loading

murchandamus Nov 10, 2020 •

edited

Loading

jonatack Nov 10, 2020 •

edited

Loading