Switch to weight units for all feerates computation

darosior commented at 9:29 pm on November 22, 2019: member

This changes the current feerate computation from using virtual bytes to weight units for transaction size.

The transaction size being stored and used as virtual bytes introduced some non-negligible rounding and approximations (See #13283 for an example.). This patch set is an attempt to switch to using weight units in place of virtual bytes internally for a more accurate feerate computation, while leaving intact the vbyte interface.

There is, I think, a big cleanup needed (for example renaming feerate constants as xx_FEERATE would really improve readability) but I wanted to keep changes as minimal as possible.

Fixes #13283.

fanquake added the label Wallet on Nov 22, 2019

fanquake added the label TX fees and policy on Nov 22, 2019

darosior force-pushed on Nov 22, 2019

DrahtBot commented at 11:39 pm on November 22, 2019: 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:

#18120 (Change UpdateForDescendants to use Epochs by JeremyRubin)
#18098 (scripted-diff: Add missing spaces in RPCResult, Normalize type names by MarcoFalke)
#18063 (Improve UpdateForDescendants by using Epochs and Removing CacheMap by JeremyRubin)
#17997 (refactor: Remove mempool global from net by MarcoFalke)
#17809 (rpc: Auto-format RPCResult by MarcoFalke)
#17791 (Remove UBSan suppressions for CTxMemPool* by hebasto)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#17331 (Use effective values throughout coin selection by achow101)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#17208 (Make all tests pass UBSan without using any UBSan suppressions by practicalswift)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#16224 (gui: Bilingual GUI error messages by hebasto)
#14582 (wallet: always do avoid partial spends if fees are within a specified range by kallewoof)
#13990 (Allow fee estimation to work with lower fees by ajtowns)
#13693 ([test] Add coverage to estimaterawfee and estimatesmartfee by Empact)
#12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

darosior force-pushed on Nov 23, 2019

darosior force-pushed on Nov 25, 2019

darosior commented at 9:03 pm on November 25, 2019: member

Rebased now that appveyor seems to be fixed.

darosior force-pushed on Nov 25, 2019

darosior force-pushed on Nov 26, 2019

darosior commented at 10:45 pm on December 2, 2019: member

This might be “too late” but the fee estimation can be floored to 253 sat/kWU, so that the wallet doesn’t create transaction below old feerate until most of the network relay 250 sat/kWU txs..

DrahtBot added the label Needs rebase on Dec 3, 2019

darosior force-pushed on Dec 3, 2019

DrahtBot removed the label Needs rebase on Dec 3, 2019

darosior force-pushed on Dec 4, 2019

darosior commented at 1:29 pm on December 4, 2019: member

Rebased.

jb55 commented at 6:25 pm on December 5, 2019: member

I can’t help but wonder if there’s a better abstraction here instead of random conversions happening everywhere…

darosior commented at 8:56 pm on December 9, 2019: member

I can’t help but wonder if there’s a better abstraction here instead of random conversions happening everywhere…

Conversion is needed anyway for every user input (and even for JSONRPC outputs) to not break the API..

jb55 commented at 6:34 pm on December 11, 2019: member

I can’t help but wonder if there’s a better abstraction here instead of random conversions happening everywhere…

Conversion is needed anyway for every user input (and even for JSONRPC outputs) to not break the API..

yes I understand that conversions are needed… I’m just referring to the fact these are two different fee types, but that fact is not captured in the type system. If we had a better abstraction that captures both of these types, it would be less error prone and we wouldn’t have to opencode conversions everywhere.

DrahtBot added the label Needs rebase on Jan 7, 2020

policy: switch to weight units for feerate computation

Since Segregated Witness activation and the transaction size being determined in
wieght units, our internal logic for transaction size (and thus feerates) has relied
on virtual bytes.
Virtual bytes are useful for non-technical end-user experience, but can cause some issues
if the whole logic rely on this raw division.

For instance, if stored as an integer, the transaction size will be truncated, which can
lead to wrong feerate calculation. In order to patch this, GetVirtualTransactionSize()
rounds up.
This led to not only protecting the users from paying too-low fees and seeing their
transaction rejected, but this also implicitly increased the minimum feerate for
relaying transactions. For instance a minrelayfee(rate) of 1000sat/kvbyte is of 253sat/kWU,
while we could expect it to be 250sat/perkWU (see issue # 13283).

Using constants in weight units will allow us to avoid raw roundings for feerate computation
and to get an accurate feerate estimation (and a deterministic relay policy), while keeping
the cosiness of vbytes for non-technical end-users.

67376daaf5

txmempool: Introduce weigh unit counters in addition to vbyte ones

This adds transaction size utils in weight where there were only virtual
byte ones. This will allow to use weight units for feerate computation
later on while keeping accurate vbyte computation for not breaking the
API.

d592eb333e

darosior force-pushed on Jan 17, 2020

darosior commented at 3:15 pm on January 17, 2020: member

Rebased.

Will rework the approach and include remarks from jb55 if there are some acks on the general concept of using / exposing weight units..

DrahtBot removed the label Needs rebase on Jan 17, 2020

txmempool: use weight units to measure transaction size

Most constants are unchanged but timed by 4 at initialization, mainly to avoid a big diff by rewriting all 'GetArg's because we need to still expose parameters as taking vbytes.

3f37f614d6

miner: switch to weight-based accounting for packages

This adds weight units counter to the 'CTxMemPoolModifiedEntry' struct
to make weights the packages size unit.

This also treats block min feerate as per weight units, while leaving
intact the vbyte shroud for the user.

4451345484

wallet: use weight units for fee computation

The functional tests in wallet_basic.py are also modified to check feerates
using weight units, as we don't round up anymore for transaction size computation.

67e2c768ec

policy/fees: use weight units for fee estimation c845d41f3b

wallet/rpcwallet: convert user feerate I/O (from/to) virtual bytes

We now use weight units for an accurate fee computation internally, but
we don't want to break the API, so apply the vbyte factor for both RPC
inputs and outputs.

56d25201bc

wallet/rpcwallet: allow to set feerate ('settxfee') in weight units 9f1e569aa1

rpc/mining: allow "estimatesmartfee" to return feerate in weight units

This adds a parameter to get the resulting feerate in satoshis per kilo
weight units instead of kilo virtual bytes.

01c749f990

rpc/mining: make "estimaterawfee" return feerate in weight units

There has been warnings since its inception that this API was not stable.

Since we now estimate fees in weight units internally, it makes sense
that the raw estimatefee API returns the feerate in weight units.

b1e95b7115

rpc/net: expose feerates in virtual bytes

We now use weight units internally, but keep exposing virtual bytes
to avoid breaking the API.

501cf1d698

rpc/rawtransaction: rename the MAX_FEERATE constant

This makes it clearer that the constant is in sat/vkb, as the feerate
parameters  currently exposed by the API are in sat/vkb.

bedaf84928

rpc/rawtransaction: add transaction weight to 'analyzepsbt' output 708b2e3ed9

rpc/rawtransaction: add a "feerate in weight" parameter to 'analyzepsbt'

This makes the feerate calculation in weight in node/psbt, then allows
to deactivate the virtual bytes transformation when using the "analyzepsbt"
RPC call.

8289007b0b

rpc/blockchain: convert feerate outputs to vbytes, expose transaction weight

This adds new fields to calls response returning transaction size and
convert already present feerate fields back to vbyte.

51ad78b50b

rpc/blockchain: allow 'getblockstats' to return feerate in weight units b165fd71b5

test/functional: Add a test to check minimum relay feerate in weight 3dc4f5d078

darosior force-pushed on Jan 18, 2020

DrahtBot commented at 1:59 am on February 17, 2020: member

DrahtBot added the label Needs rebase on Feb 17, 2020

ariard commented at 9:31 am on June 22, 2020: member

Concept ACK.

Correctness of fee computation is safety-critical for any time-sensitive protocol like LN. Overpaying by default isn’t the solution as you may have to save value for period of congestion. With this regards easing implementation of segwit fee-estimators should be highly considered.

As Rusty points out in related issue, this should be part of a wider discussion on a stable API for protocol applications on top of Bitcoin Core.

darosior commented at 10:41 am on June 22, 2020: member

Thank you for the feedback, I’ll rebase this and change the approach as per jb55’s comments soon. I wanted to go for the minimal changes, but something cleaner is definitely better. This makes me wonder if I should break this into smaller PRs, too.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ Le lundi, juin 22, 2020 11:31 AM, Antoine Riard notifications@github.com a écrit :

Concept ACK.

Correctness of fee computation is safety-critical for any time-sensitive protocol like LN. Overpaying by default isn’t the solution as you may have to save value for period of congestion. With this regards easing implementation of segwit fee-estimators should be highly considered.

As Rusty points out in related issue, this should be part of a wider discussion on a stable API for protocol applications on top of Bitcoin Core.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

adamjonas commented at 12:54 pm on August 14, 2020: member

I gathered some opinions on this from a long time contributor about the concept, he replied: “seems like a fine thing to do. also seems like not a high priority”

After looking at the code, however, his reaction was that the patch was too big for the benefit. Putting more data in the CTxMemPoolEntry especially stood out as an issue. @darosior I think it’s worth getting more conceptual review before you put in more effort. I believe it’s clear that more discussions need to happen around ways to improve the linkages between Bitcoin Core and LN, but based on the opinions I gathered, the approach on this PR could use some more feedback.

darosior commented at 1:02 pm on August 14, 2020: member

@adamjonas this PR is a bad approach. It’s been known and discussed above, so I don’t think it’s worth discussing it any more. However for the sake of the argument: I locally have a version which is an about 3x smaller diff, and regarding adding a field to the CTxMempoolEntry it’s necessary anyway as to use weight-mesured feerates we need to track, well, the weight of the tx.

I agree it needs more conceptual feedback, but i don’t want to ask for it before this patchset is reworked (will push soon :tm: ) as i think the boutique approach here could degrade the conceptual review.

Nonetheless, thank you for your interest.

jnewbery commented at 9:01 am on September 29, 2020: member

This has required rebase since February, and there are still conceptual questions outstanding. I’m going to close it for now. @darosior - feel free to reopen (or open another PR) if you have something that’s ready for review/discussion.

jnewbery closed this on Sep 29, 2020

DrahtBot locked this on Feb 15, 2022

Switch to weight units for all feerates computation #17566

Conflicts