validation: Move package acceptance size limit from KvB to WU

ariard commented at 9:37 pm on May 28, 2021: member

Class of second-layers applications interested by the package acceptance API are mostly relying on trustless chain of transactions which are necessarily made of segwit input. As such, most of them are making their internal propagation sanity checks and feerate computations already in weight units (at least I can confirm for Lightning, it’s mandatory in the spec to announce feerate_per_kw to your counterparty).

To reduce odds of bugs in second-layers implementations, such as the one mentioned in #13283, this PR changes the package acceptance size limit from kilo-bytes to weight units.

E.g an internal Lightning onchain logic with an already-computed package weight of 404_002 (100_000 bytes of regular fields, 4002 bytes of witness fields), dividing by WITNESS_SCALE_FACTOR, obtaining 101_000 virtual bytes, checking against MAX_PACKAGE_SIZE and then wrongly evaluating the package as valid for mempool acceptance.

It also matches our max transaction size check already expressed in weight units (MAX_STANDARD_TX_WEIGHT).

DrahtBot added the label TX fees and policy on May 28, 2021

DrahtBot added the label Validation on May 28, 2021

DrahtBot commented at 3:59 am on May 29, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23381 (validation/refactor: refactoring for package submission by glozow)
#22674 (validation: mempool validation and submission for packages of 1 child + parents by glozow)

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.

DrahtBot added the label Needs rebase on Jun 10, 2021

murchandamus commented at 4:49 pm on June 11, 2021: contributor

404_002 (100_000 bytes of regular fields, 1002 bytes of witness fields), dividing by WITNESS_SCALE_FACTOR, obtaining 101_000 virtual bytes

Did you mean 4,002 bytes of witness fields?

murchandamus commented at 4:52 pm on June 11, 2021: contributor

Concept ACK

jonatack commented at 4:54 pm on June 11, 2021: contributor

ping @rustyrussell

harding commented at 8:44 pm on June 11, 2021: contributor

Wouldn’t this cause the package acceptance code to accept packages that wouldn’t be accepted as a series of successively-relayed transactions? E.g., Alice submits a 400,001 weight transaction (100,001 vbytes) followed by a 3999 weight child transaction (1,000 vbytes).

I don’t think implementing this (with a bug fix for the above, if I’m correct) would be a disaster, but I think we should try to be consistent in the units we use in our external interfaces—so we either use vbytes everywhere or we use weight everywhere. When choosing among those two units, we eventually bump into the feerate interfaces where we have a choice between trying to retrain a large number of users who currently think about fees in “bytes” per BTC (or sats), or instead training a a few dozen developers[1] that, to get vbytes from weight units, they need to put a ceil() around weight/4. To me, the decision to be made there seems obvious.

[1] Most wallets should probably be using a library that provides low-level functions for calculating sizes and feerates, so it’s only the authors of those libraries who should need to read BIP141’s simple and clear description of how to convert weight into vbytes.

murchandamus commented at 9:50 pm on June 11, 2021: contributor

Good point, although I am wondering whether backend code should generally move towards weight units everywhere even if the feerate interfaces remain denominated in vbytes for the userbase.

validation: Move package size limit from virtual bytes to weight units 4fa72eedd1

ariard force-pushed on Jun 13, 2021

ariard commented at 5:15 pm on June 13, 2021: member

@Xekyo

Did you mean 4,002 bytes of witness fields?

Yep, good catch, corrected in PR description. @harding

Wouldn’t this cause the package acceptance code to accept packages that wouldn’t be accepted as a series of successively-relayed transactions? E.g., Alice submits a 400,001 weight transaction (100,001 vbytes) followed by a 3999 weight child transaction (1,000 vbytes).

I think the parent transaction would be rejected by our current check against MAX_STANDARD_TX_WEIGHT (400_000 WU), which still apply at package acceptance (in AcceptMultipleTransactions) ? Also, I believe one of the outcome of the lenghty discussion in #20833 was that package acceptance doesn’t imply that the corresponding series of transactions will be accepted if submitted. Ultimately, package-level checks aren’t even going to be a superset of transaction-level one, some checks might more liberal (evaluate package-level feerate to decide mempool min feerate), some more conservative (evaluate union of package ancestors to decide mempool ancestors limits) ?

I don’t think implementing this (with a bug fix for the above, if I’m correct) would be a disaster, but I think we should try to be consistent in the units we use in our external interfaces—so we either use vbytes everywhere or we use weight everywhere.

W.r.t to our mempool limits interface, I think we’re already using weight (MAX_STANDARD_TX_WEIGHT) and I don’t think we could move backward to vbytes. The reasoning being that if we had MAX_STANDARD_TX_SIZE, we could accept transaction which are ultimately bigger than other ones rejected, when it’s time to evaluate them for block inclusion (and I believe this phenomena could already happen with our current approach of evaluating mempool ancestors/descendants limits in vbytes, DEFAULT_DESCENDANT_LIMITS ?). This change avoids this pitfall and normalizes our package acceptance interface with the transaction-level one.

When choosing among those two units, we eventually bump into the feerate interfaces where we have a choice between trying to retrain a large number of users who currently think about fees in “bytes” per BTC (or sats), or instead training a a few dozen developers[1] that, to get vbytes from weight units, they need to put a ceil() around weight/4.

But do we have the leisury to avoid retraining users, or at least a subset of the userspace, to think fees in terms of “weights” ? With the upcoming deployment of Taproot, you might have the choice among multiples tapscripts, offering differing feerates/interactivity trade-offs. Sticking to vbytes, such wallets would be misleading when branches differs in weights but are displayed as “equal” in vbytes. Further, LN operators and users are going to think in term of weights, as it’s far more granularity for liquidity operations (e.g participating in dual-funded channel). For the rest of the users, I believe they’re going to care more about the cost of basic operations like spend from a P2WSH to one party, spend from a P2SH to two parties, a 2-of-3 multisig, rather than the unit into which it’s displayed.

That said, I’m not a UX wallet expert, and I think this change is motivated enough from a mempool acceptance interface viewpoint rather than wandering into a feerate interface discussion.

ariard commented at 5:17 pm on June 13, 2021: member

@glozow I guess you would be interested, it’s the answer to your question here.

DrahtBot removed the label Needs rebase on Jun 13, 2021

harding commented at 7:17 pm on June 13, 2021: contributor

@ariard

I think the parent transaction would be rejected by our current check against MAX_STANDARD_TX_WEIGHT (400_000 WU), which still apply at package acceptance (in AcceptMultipleTransactions) ?

Oh, of course. The example would then be:

0In [1]: x = 400000; y = 3001; z = 999
1
2In [2]: x+y+z <= 404000
3Out[2]: True
4
5In [3]: ceil(x/4)+ceil(y/4)+ceil(z/4) <= 101000
6Out[3]: False

I believe one of the outcome of the lenghty discussion in #20833 was that package acceptance doesn’t imply that the corresponding series of transactions will be accepted if submitted.

Yeah, but it would be nice to follow the same rules when there’s no significant benefit to changing them. That said, I don’t really care about tiny rounding issues that don’t impact our DoS protections at all; it just seems incongruous to me.

do we have the leisury to avoid retraining users, or at least a subset of the userspace, to think fees in terms of “weights” ?

Yes, of course we do. Your argument in this paragraph creates an artificial dilemma between vbytes and weight units based on granularity, but it’s possible to use vbytes with more granularity when making comparisons. Developers writing their own low-level functions need to know that vbytes at the transaction level is ceil(x/4), but for everyone else vbytes can be expressed for comparison purposes as float(x/4). For example, see https://bitcoinops.org/en/tools/calc-size/

I think this change is motivated enough from a mempool acceptance interface viewpoint rather than wandering into a feerate interface discussion.

The user-visible interface for mempool acceptance limits is currently defined in “kilobytes”:

0$ bitcoind -help-debug | grep limit.*size
1  -limitancestorsize=<n>
2  -limitdescendantsize=<n>

Using weight units in the code seems to create unnecessary confusion, e.g. the example at the top of this post. If you instead propose to change the user-facing size parameter for package size, that would imply also changing other size parameters to use weight, which brings us to the fee interface.

I think there’s a very simple rule that’s previously been applied to the code, which is that weight units are for consensus checks and vbytes are for everything else. I’m not sure we should be changing that because significant parts of the early LN protocol specification drafts were written by someone trying to make sipa an eponym for weight, and so now they’re stuck using feerate units nobody else uses. Personally, my response to #13283 isn’t “downstream developer had a problem, we should spend a bunch of time refactoring our code (and maybe our interfaces) to fix it” but rather “downstream developer did something weird, we should buy them a drink and tell them a story about a time we also did something weird and suffered for it.”

ariard commented at 3:57 pm on June 14, 2021: member

@harding

Yes your last example works.

Yeah, but it would be nice to follow the same rules when there’s no significant benefit to changing them. That said, I don’t really care about tiny rounding issues that don’t impact our DoS protections at all; it just seems incongruous to me.

Beyond the reduction of rounding-bugs risks in critical paths of downstream codebases, I think there is a relevant benefit to changing them. To make my argument clear, I believe that using vbytes units for our mempool acceptance limits isn’t economically optimal.

Assuming two absolute-fee equal packages A and B, respectively of sizes 100_999 vbytes and 101_001 vbytes. The first one is currently accepted by our package logic, the second one rejected. Scaled up to weight units, it turns out package A has a size of 300_999 WU and package B has a size of 104_004 WU. Package B is far more interesting than A for block inclusion, however it won’t be present in our mempool with currently vbytes expressed limits.

Developers writing their own low-level functions need to know that vbytes at the transaction level is ceil(x/4), but for everyone else vbytes can be expressed for comparison purposes as float(x/4). For example, see https://bitcoinops.org/en/tools/calc-size/

Of course, you can use vbytes with far more granularity when making comparisons if your wallet developer is aware of all the differing accounting scales to use among transaction components, i.e when to display float(x/4). And this accounting complexity might extend in the future, if we rely on taproot annex to artificially increase the cost of new opcodes or if we introduce new consensus fields. Beyond, I would argue that reasoning on integers is far easier to do than floats, which kind of reduce the end-user cognitive burden from a UX standpoint.

Using weight units in the code seems to create unnecessary confusion, e.g. the example at the top of this post. If you instead propose to change the user-facing size parameter for package size, that would imply also changing other size parameters to use weight, which brings us to the fee interface.

If my reasoning above about economical optimality holds, yes I think that logically implies changing other size parameters to weight. W.r.t to mempool acceptance limits, that’s a minor change, which can be done without overhaul of mempool internal interface from sat/KvB to sat/kWU. I agree that this later mentioned change would imply a lot of careful refactoring which might restrain us to do it in the short-term. But we don’t have this concern with newer interfaces.

I think there’s a very simple rule that’s previously been applied to the code, which is that weight units are for consensus checks and vbytes are for everything else.

At least we have one instance where this rule isn’t consistently applied, MAX_STANDARD_TX_WEIGHT ? So should we modify back this variable to vbytes units ?

Personally, my response to #13283 isn’t “downstream developer had a problem, we should spend a bunch of time refactoring our code (and maybe our interfaces) to fix it” but rather “downstream developer did something weird, we should buy them a drink and tell them a story about a time we also did something weird and suffered for it.”

I hear you there, but please note I think we’re both advocating package-relay to make downstream projects support better, which is extensively time-consuming as this discussion and other related-package PRs are illustrating it. If you want to abandon this serie of works in the name of downstream weirdness, I think you’ll have a lot of drinks to buy to the LN ecosystem :)

Beyond the joke, if you have a strong opinion against this specific PR, please mark a clear Concept NACK, or let me know if I didn’t address your arguments or you feel I need to clarify mine. Otherwise, I fear we’re going to fall in an endless nerdy discussion ?

harding commented at 6:51 pm on June 16, 2021: contributor

@ariard thanks for your detailed response and sorry for bike shedding.

I’m concept NACK on using different rules for packages of transactions than we would use for the same transactions submitted separately unless there’s a significant benefit motivating the difference. I don’t think a less-than 0.001% difference in economic rationality (which only matters in rare edge cases) is anywhere significant enough motivation. I don’t think downstream developers should be performing relay-related calculations using weight units—they should be using vbytes—so I don’t think there’s any benefit at all for them to us using weight units here; indeed, it might be harmful in giving them mistaken impression that Bitcoin Core is going to eventually switch to using weight units in public interfaces.

If this PR were changed to eliminate the behavior change, I’d have no conceptual objection to it.

As for the general idea of using weight units in internal code, I think that’s a decision best made by the people most frequently working on that code; I’ll try to keep my nose out of it. But I will object strongly to any later attempts to change the public interfaces to use weight units based on the argument that the internal code now uses weight units. If vbytes are to die and weight units are to become part of how everyday users measure feerates, please let that change be established in other popular software first before we consider it.

I think you’ll have a lot of drinks to buy to the LN ecosystem :)

I think I already have quite a few obligations in that area from other messes I’ve made, some of which you’ve be instrumental in discovering. :-)

darosior commented at 8:21 am on June 18, 2021: member

Code wise, please note that the virtual size isn’t computed as a simple /4 (rounded up) from the weight units but also factors in the signature OPs cost. As implemented a specially-crafted transaction’s size could largely diverge between the WU size there and the virtual size in the rest of the code.

Conceptually, i’m a big fan of moving to using WU in Bitcoin Core but i think it would need to be an invasive change through the codebase to keep everything consistent. One approach i had considered to minimize the change was to overload CFeeRate to optionally take the transaction size as WUs.

FWIW to respond to the previous conceptual concerns, most (all?) of the downstream projects i know of use WUs and then use some hacks to avoid the caveat of bitcoind’s rounding due to its use of virtual bytes.

harding commented at 9:14 am on June 18, 2021: contributor

Code wise, please note that the virtual size isn’t computed as a simple /4 (rounded up) from the weight units but also factors in the signature OPs cost. As implemented a specially-crafted transaction’s size could largely diverge between the WU size there and the virtual size in the rest of the code.

I don’t think that’s accurate. The number of sigops in a transaction has no direct effect on its weight or vsize; there’s just a rule that says a transaction is invalid if the number of sigops per weight exceeds a certain budget, e.g. as defined in BIP342.

darosior commented at 9:30 am on June 18, 2021: member

I don’t think that’s accurate. The number of sigops in a transaction has no direct effect on its weight or vsize

The transaction virtual size is increased if its number of sigops is abnormally high compared to its size: https://github.com/bitcoin/bitcoin/blob/da69d9965a112c6421fce5649b5a18beb7513526/src/policy/policy.cpp#L280-L283

This is how the size is computed in ATMP. However you are right that for packages we give a number of sigops (and cost) of 0 and therefore the specifc changes of this PR are not affected. ~~But we probably should compute the transaction size for packages the same way we do for single transactions in the first place?~~ EDIT: so this is actually done for package transactions too, as we’ll call PreChecks for each transaction. But it’s really confusing to first check the transaction virtual size without the sig op cost penalty, to then re-compute it with the penalty set.

murchandamus commented at 6:16 pm on June 18, 2021: contributor

AFAIU, sigops and weight are two independent limits per consensus. The treatment of transactions with high sigops as having a high weight is a simplification to avoid a multidimensional optimization problem and only applies to policy checks.

harding commented at 1:57 am on June 20, 2021: contributor

@darosior

The transaction virtual size is increased if its number of sigops is abnormally high compared to its size

Thanks for looking up and linking me to the code. I was able to follow it back to #8356 where this was added and so I think I now understand the source of confusion: We allow users to exceed the policy limits on sigops by pretending their transactions are the same vsize as transactions that would have the same number of sigops but an appropriate amount of weight to contain those sigops.

As long as developers are creating transactions in a normal way, they shouldn’t run into this problem and can just use the standard vbytes formula from BIP141.

t’s really confusing to first check the transaction virtual size without the sig op cost penalty, to then re-compute it with the penalty set.

Indeed. I think we still need the above function for legacy and v0 segwit transactions, but it should always produce the same result as simple BIP141 vbytes for any valid taproot transaction.

darosior commented at 8:41 pm on June 21, 2021: member

As long as developers are creating transactions in a normal way

Yes, of course. But my point there was just to be careful to not drop the DOS protection rule with a refactoring to WU (but this patch specifically doesn’t drop it as it was not present in the first).

DrahtBot commented at 5:06 pm on November 9, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Nov 9, 2021

DrahtBot commented at 12:17 pm on February 22, 2022: contributor

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

ariard commented at 0:00 am on July 9, 2022: member

I’m closing this PR. While I still think moving from KvB to WU numerous policy checks would remove some non-propagation risk for second-layers and also improve the economic efficiency of the mempool acceptance flow, it would be a lot of engineering and communication work across the ecosystem to make WU the standard unit of transaction size.

I still think it could be an interesting research project to make a WU-based mempool acceptance code and run simulations out of that from historical data to observe if the feerate yielded is better. If you’re a miner in 2032 reading that and you’re looking for tricks to improve the average fees reward from your mined blocks over your competitors you might be interested.

Can be tagged “up to grabs” if someone is interested to work on it modulo conceptual caveats.

ariard closed this on Jul 9, 2022

bitcoin locked this on Jul 9, 2023

glozow commented at 2:00 pm on September 15, 2023: member

Just remembered this - the same change has been picked up in #28471 if reviewers here want to review that one

validation: Move package acceptance size limit from KvB to WU #22097

Conflicts