The PR branch HEAD was ed414f6 at the time of this review club meeting.
Notes
Today’s review club is A Tale of Two Fee Rate Units, and the ability starting
from Bitcoin Core 0.21 to set one of them explicitly in Bitcoin Core’s various
send RPCs.
Bitcoin Core has historically used fee rates denominated in bitcoins per 1000
bytes (BTC/kB); post-segwit, the unit is 1000 virtual bytes (BTC/kvB).
You can see this by running ./src/bitcoin-cli help settxfee and
./src/bitcoind -help-debug | grep fee
Nevertheless, users have expressed what appears to be a long-standing
preference for fee rates to be expressed in satoshis per vbyte (sat/vB)
instead, which is a commonly used unit in other wallets. This demand can be
seen, for instance, in Bitcoin Core
issues and in twitter
polls.
Background
PR 11413, “Add explicit
feerate option,” was merged in June 2020. That pull enabled users to
specify for the first time a specific fee rate in some send RPCs. However, it
did so by overloading the integer conf_target param/option to also accept
floats or strings. Then, if a float or string was passed to represent a fee
rate instead, the unit was taken from the estimate_mode param/option that
the PR also overloaded. See this
comment
for more context.
Issue 19543, “Normalize fee
units for RPC (BTC/kB and sat/B),” was opened in July 2020 to address the
problem.
PR 20220, “Explicit fee rate
follow-ups/fixes for 0.21,” was merged in November 2020 to provide a spec and
regression coverage for the potential next step of moving to a universal fee
rate argument in satoshis per vbyte (sat/vB) as well as immediate coverage and
fixes for 0.21.
PR 20305, “Introduce
fee_rate sat/vB param/option,” was merged in November 2020 to address issue
19543 and is part of the 0.21 release. It replaces overloading the
conf_target and estimate_mode params/options with a new fee_rate
param/option denominated in sat/vB in the sendtoaddress, sendmany, send,
fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs, as well as other
fixes. It was followed by PR
20246, “Allow zero-fee
fundrawtransaction/walletcreatefundedpsbt and other fixes,” that was
backported to 0.21.
Current situation
RPCs sendtoaddress, sendmany, send, fundrawtransaction,
walletcreatefundedpsbt, and bumpfee all allow specifying an explicit fee
rate in sat/vB starting from Bitcoin Core v0.21 released in January 2021.
It would be ideal
for all fee rates to be in the same units. However, if you run git grep
feeRate, you will see that we still have a similarly spelled feeRate
(BTC/kvB) option in RPCs fundrawtransaction and walletcreatefundedpsbt
that could confuse users, as fee_rate and feeRate have different units and
similar spellings.
PR 20483, “deprecate feeRate
in fundrawtransaction/walletcreatefundedpsbt,” was opened to deprecate the
feeRate option but cannot yet be considered. (Why?)
All of the fee rate configuration options are currently still denominated in
BTC/kvB units and need to be updated.
Issue 20534, “sat/b values
aren’t validated to be in range,” was opened in November 2020 and remains
unresolved.
Code
Today’s PR is an accessible change to resolve issue 20534 while doing some
CFeeRate refactoring (see the pull request description for details).
The CFeeRate class in src/policy/feerate.{h,cpp} is responsible for
constructing fee rates of different units. It also contains comparison
operators and a ToString() helper function.
SetFeeEstimateMode() in src/wallet/rpcwallet.cpp is the central function
for setting fee estimation or the fee rate that is used by the wallet
transaction RPCs and by FundTransaction() that some of the RPCs call
through. These are all located in the same file, rpcwallet.cpp.
AmountFromValue() in src/rpc/util.cpp is a frequently used utility that
converts UniValue amounts from user input to CAmount values suitable for
constructing CFeeRate objects.
Questions
Explain the difference between BTC/kvB and sat/vB units. A fee rate of 1
BTC/kvB is equivalent to what fee rate in sat/vB?
What is the difference between an RPC parameter and an RPC option?
When making transactions, what fee practice does the ability to specify an
explicit fee rate replace?
At the time of this writing, why is it too early to deprecate the feeRate
(BTC/kvB) option in fundrawtransaction and walletcreatefundedpsbt to
avoid confusion with fee_rate (sat/vB)?
After some discussion, a plan was made to complete the migration from
BTC/kvB to sat/vB units. What is it? Does it make sense to you?
In the meantime, if users confuse fee_rate with feeRate when making a
transaction or fee bumping, why was the risk of losing funds considered low
enough to be temporarily acceptable?
What range of values is non-representable by CFeeRate? How does this PR
resolve that? Is there a better approach?
What is the “Named Constructor Idiom”? Why does this PR use it?
The new utility function, FeeRateFromValueInSatB(), calls another utility
function named AmountFromValue(). Why are there two of them in the
codebase (one in src/rpc/util.cpp and one in src/bitcoin-tx.cpp)?
Running ./src/bitcoind -help-debug | grep fee returns the fee rate
configuration options. After setfeerate and estimatefeerate in sat/vB
are added and settxfee and estimatesmartfee in BTC/kvB become hidden
RPCs, what would be the best way to migrate the fee rate units in these
configuration options from BTC/kvB to sat/vB?
<jonatack> We usually start Bitcoin Core IRC meetings with a 'hi' to know who is here. Feel free to say hi! If you have a signet address, append it your "hi" to receive some sBTC (signet coins) for good answers. 🍰
<jonatack> This week, we're talking about PR #20564 "Check for non-representable CFeeRates (tx fees and policy, wallet, refactoring)" which is a bugfix and part of our ongoing migration of fee rates units from BTC/kvB to sat/vB units (by popular demand). 🚀
<jonatack> Here is an RPC command template you can use to try sending coins to people (or to yourself) from your signet node using the new fee_rate param (just add their address):
<sishir> “difference between BTC/kvB and sat/vB units means that a transaction with a fee rate mistakenly calculated in BTC/kvB rather than sat/vB should raise an error due to the fee rate being set too low.”
<jonatack> Yes! Explicit fee rate is an alternative to using fee estimation with the conf_target and estimate_mode params/options, to allow Bitcoin Core to set the fee rate for you.
<jonatack> 4. Why is it too early to deprecate the `feeRate` (BTC/kvB) option in `fundrawtransaction` and `walletcreatefundedpsbt` to avoid confusion with `fee_rate` (sat/vB)?
<sishir> The estimatesmartfee is still in BTC/kB, so merging this would force all users to do the conversion themselves (or force to pass -deprecatedrpc).
<jonatack> I proposed to deprecate feeRate but because people may use RPC estimatesmartfee to estimate the fee needed, but that RPC returns the fee rate in BTC/kvB and not in sat/vB, it was considered to early.
<jonatack> It seemed the best way to normalize and transition to sat/vB per user demand, protect users by not having feeRate/feerate in BTC/kvB and fee_rate in sat/vB in the same RPCs, and minimize the amount of breaking API changes.
<jonatack> 6. In the meantime, if users confuse fee_rate with feeRate when making a transaction or fee bumping, why is the risk of losing funds considered low enough to be temporarily acceptable?
<jonatack> 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.
<jonatack> Or as Murch wrote, "In the worst case, someone is going to get upped to the minRelayTxFee silently and send at 1 sat/vB. Since RBF is on by default, they should be able to bump when they notice."
<AnthonyRonning> 0 to 0.001, non inclusive. It checks if, after conversion, if the value is zero but the initial value was not zero. Checking for `> 0 && < 0.001` instead of allocating a `CFeeRate` could be another approach.
<AnthonyRonning> i typically would use the range check. Is there a reason why allocating `CFeeRate` is done here? Is there a hidden benefit i'm missing?
<AnthonyRonning> I still see some places in code that use `CFeeRate(X, 1000)`, is there further refactoring that could be implemented to be consistent?
<jonatack> I found it in isocpp.org/wiki/faq/ctors#named-ctor-idiom by Stroustrup, Cline, Sutter and Alexandrescu. Its goal is to provide more intuitive and/or safer construction operations for users of your class.
<larryruane_> Seems like named constructors are nice when the argument list can't be used to distinguish between different forms of initializers (e.g. both take uint64_t but the units are different, as is the case here)... nice! I wasn't aware of this!
<jonatack> 9. FeeRateFromValueInSatB calls another utility function named AmountFromValue. Why are there two AmountFromValue functions in the codebase (one in src/rpc/util.cpp and one in src/bitcoin-tx.cpp)?
<AnthonyRonning> Because you can work with transactions through the RPC and the command line (from what i can tell), and `AmountFromValue` should always be used when inputing numerical values from the user.
<AnthonyRonning> ah yeah, better question. I'm not sure, is there not a better place for utility functions to exist? Or are they slightly different, need to check again.
<Murch> Given that there are quite a few types of amounts in the codebase that have distinct ranges and shouldn't be easily added together, have there been more attempts to make classes for specific types of amounts?
<larryruane_> if this doesn't sidetrack the discussion too much (feel free to ignore), what's the reason we even check for circular dependencies? what goes wrong if we don't? there are exceptions anyway....
<AnthonyRonning> oh didn't notice that, yeah i see that now. It could have been better to be more explicit between the two in the first place, in retrospect
<Murch> AnthonyRonning: The downside with making parameter names even more explicit is that you're then stuck with that name even when the old one is long forgotten