policy: nVersion=3 and Package RBF

[glozow]

Note: this PR was superseded by #28948 (v3) and #28984 (package RBF). Also, v3 became TRUC and includes other changes, see BIP 431 for more up-to-date documentation.

See #27463 for overall package relay tracking.

This PR contains 2 projects: v3 policy and package RBF. Mailing list posts: package RBF 1 and V3 + package RBF 2. It stems from a long discussion about RBF pinning, across a mailing list thread and gist.

V3 Policy: A set of policy rules applied to transactions with their nVersion field set to 3. Namely, it allows users to opt in to more restrictive descendant limits for shared transactions. If adopted by many nodes in the network, V3 mitigates various RBF pinning attacks. See doc/policy/version3_transactions.md for the exact rules and rationale, and these review club notes for more background and discussion.

Package RBF: In addition to allowing a child to pay for its parents within the package, also allow the child to pay for replacing the parent's conflicts. For example, this allows LN users to replace commitment transactions existing in the mempool, simply by broadcasting their respective commitment transactions with a high-fee child. The commitment transactions can be signed with 0 fees, which means no overpaying.

FAQ: is v3 still helpful even with cluster mempool (#27677) ?

• Rule 3 pinning: This is addressed with v3 but not really with cluster mempool (descendant allowance is still too permissive).
• Package RBF and ACP pinning: This PR allows for package RBF with v3 packages. V3 has an effective "cluster limit" of 2 which makes it very cheap to calculate the mining score of a v3 transaction. With cluster mempool, which also makes it easier to calculate mining score, we could have package RBF for non-v3 transactions.
• Allowing 0fee transactions: This PR allows v3 transactions to be below minimum relay feerate, provided they are CPFP'd. This is because the simplified topology allows us to avoid situations like the ones described in mempool: disallow txns under min relay fee, even in packages #26933. With cluster mempool, we can allow this for non-v3 transactions.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28972 (test: Add and use option for tx-version in MiniWallet methods by maflcko)
• #28960 (kernel: Remove dependency on CScheduler by TheCharlatan)
• #28950 (RPC: Add maxfeerate and maxburnamount args to submitpackage by instagibbs)
• #28948 (v3 transaction policy for anti-pinning by glozow)
• #28886 (refactor: Replace sets of txiter with CTxMemPoolEntryRefs by TheCharlatan)
• #28863 (wallet, mempool: propagete checkChainLimits error message to wallet by ismaelsadeeq)
• #28690 (build: Introduce internal kernel library by TheCharlatan)
• #28368 (Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq)
• #28335 (RFC: Remove boost usage from kernel api / headers by TheCharlatan)
• #27432 (contrib: add tool to convert compact-serialized UTXO set to SQLite database by theStack)
• #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)

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.

[jonatack]

$ ./test/lint/lint-python.py
test/functional/feature_package_rbf.py:18:1: F401 'test_framework.util.assert_equal' imported but unused

[ariard]

Just a first parse

[ariard]

Reason to not turn PackageTestAccept as true too ?

[glozow]

This RBF policy is essentially treating the package as "one big transaction," which works because everything is in the ancestor set of the child. PackageTestAccept doesn't impose any topology restrictions - it even allows unrelated transactions to be submitted as a group. So it wouldn't make sense to use each other's fees.

[ariard]

IIUC given that PackageMempoolChecks happens after package dedup there should not be remaining already-in-mempool transactions at that stage in txns ? So no descendants that could be to-be-replaced mempool entries to subtract or are you thinking to something else ?

[glozow]

I mean a situation like this:

{A, B, C} are in the mempool, and A is already at the descendant limit (101KvB). {X, Y, Z} is a package where X conflicts with B and will replace {B, C}. However, when we run descendant limit checks, it will look like A's descendants include {B, C, X, Z} and reject it. But actually A's descendants will only include {X, Z} after the replacement.

[ariard]

Depending on the design of p2p packages, we might relay the "full-non-dedup" version of the package as our peers might not have the same mempool composition. So the additional fees might have to be paid on the non-dedup package. I don't think we have to settle the question now though we might have to rework package-RBF in function of p2p design ?

[t-bast]

My head hurts 😆

I'm not 100% sure I'm understanding this rule correctly, can you let me know if the following example makes sense?

Let's imagine we have {commitTxAlice, anchorTxAlice} in the mempool that we want to replace.
What we really want to do is to replace commitTxAlice by commitTxBob (the child is just a tool to achieve that).
We ask bitcoind to fundrawtransaction to ensure our anchorTxBob adds enough funds.
bitcoind adds an unconfirmed (but safe) wallet input:

+-------------+
| commitTxBob |---------+
+-------------+         |      +-------------+
                        |      |             |
+------------------+    +----->| anchorTxBob |
| previousWalletTx |---------->|             |
+------------------+           +-------------+

If we include previousWalletTx in the package, this package-rbf would work, right?
But if we don't include previousWalletTx in the package, this would be rejected?

But what if previousWalletTx itself has an unconfirmed parent?
We cannot include this grand-parent in the package (since packages are only parents-with-single-child).

This isn't directly bringing an unconfirmed input, but it is indirectly bringing one. Is that ok? If that's ok, why wouldn't it be ok to just allow adding new unconfirmed inputs in the first place?

[t-bast]

Gotcha, thanks for the confirmation, we'll deal with that limitation then, it's already a good start. And hopefully we'll eventually figure out better RBF rules for both single txs and packages!

[glozow]

(Sorry for my earlier comment, I was confused)

If we include previousWalletTx in the package, this package-rbf would work, right?

Yes. It's fine because it's one of the direct parents of anchorTxBob.

But if we don't include previousWalletTx in the package, this would be rejected?

If you mean anchorTxBob now only has 1 unconfirmed input, commitTxBob, actually this would be accepted.

If you mean that previousWalletTx was submitted ahead of time, that should still be okay because the code will remember to exempt previousWalletTx from the "no new unconfirmed" rule.

But what if previousWalletTx itself has an unconfirmed parent?

Based on this rule, if previousWalletTx has another unconfirmed parent, this would be rejected.

[t-bast]

Thanks for the clarification! I'll create extensive tests of this behavior and will report back.

[t-bast]

It's looking good with the current state of the PR, I am able to include new unconfirmed inputs that themselves have an unconfirmed parent as shown by this test 👍

[instagibbs]

@t-bast

to have 100 outputs and broadcast one child tx per output, you're already hitting the 100 transactions limit...

They cannot create this chain to begin with as from any single unconfirmed transaction with confirmed parents, you can only make 25 transactions.

So if you for example had >4 commitment transactions that you want to CPFP with a single bump transaction, this is where the limit can be hit.

[t-bast]

They cannot create this chain to begin with as from any single unconfirmed transaction with confirmed parents, you can only make 25 transactions.

Right, I had forgotten that restriction! However, this 25 descendants chain limit is only a configuration parameter on bitcoind, not a BIP 125 requirement, right? So in theory we cannot fully rely on it, nothing prevents an attacker to inject a 100 descendants chain in miners mempools if they have configured their bitcoind differently (or even use a different implementation)?

[instagibbs]

@t-bast unlikely but true, seems like something to consider scaling with other policies to be more robust?

[ajtowns]

this 25 descendants chain limit is only a configuration parameter on bitcoind, not a BIP 125 requirement

Note that BIP 125 enforcement could be considered a configuration parameter too -- eg knots nodes will allow replacement of any tx, whether they signal or not.

Perhaps something we could consider is defining a service bit to correspond with a relay policy, and have nodes preferential peer with other nodes who set that bit; that way you're more likely for nodes that support a particular policy to form a fully connected graph, and thus have a path to any miners that also accept that relay policy. Knots defines an experimental service bit for full-RBF nodes https://github.com/bitcoinknots/bitcoin/blob/23.x-knots/src/init.cpp#L1103 .

[sdaftuar]

Since we are introducing a new form of RBF that doesn't currently exist, now might be the best time to at least improve the BIP 125 rules to be incentive compatible in all situations (though this comes at the cost of making replacements potentially more expensive).

In particular what I have in mind is that we require the ancestor feerate of the child transaction to be higher than the actual feerate of all transactions that would be evicted. This is conservative but I think would ensure that we don't have any situations where there are transactions being replaced that might appear in the next block, while the new transactions do not.

Would this be too conservative for the use cases that we have in mind?

[instagibbs]

Can you elaborate with an example? It's possible I don't have the same mental mapping to terms and do not understand the scenario.

[sdaftuar]

Sure -- consider the case where you have a transaction A(100vb, 1sat/vb) and child B (100vb, 10sat/vb) in the mempool. Total fees = 1100 sat.

A package comes in with parent A'(100vb, 1sat/vb) and child B'(1000vb, 2.5sat/vb). Total package fees = 2600 sat.

Under the rules described here, package (A', B') could replace (A, B) in the mempool, because total fees are paid for and there's enough left over fee to pay the incremental relay fee of the new package (1500 sat/1100 vb). Also the package feerate is higher than that of the direct conflicts[1].

The deficiency in the logic is that we're not looking at the feerates of the transactions that would be evicted -- even though their absolute fee is being paid, their feerate could be even higher than the transactions that would replace them. Calculating the effective feerate that such a transaction would have in our mining code is tricky (because a sibling could pay for an ancestor, it's not quite right to just consider ancestor feerate), but using its feerate is a conservative check here -- any descendant that is paying for its confirmation would be caught in this logic, and its (higher) feerate considered if necessary.

For evaluating the mining suitability of the new transactions coming in, I think we can just look at the ancestor feerate of the package, which is a lower bound on the attractiveness of the new transactions. (I think this would also allow us to relax the rule against new unconfirmed inputs.)

[1] In looking at this example, I noticed that BIP 125 as drafted seems to misstate an important check we perform in our current RBF policy, namely that we compare the feerate of a new transaction coming in to the feerate of the existing transactions that it directly conflicts with. (Without this check, a small high fee transaction could be replaced by a large low fee transaction!). From my first glance at the code in this PR, it looks like the code itself does a similar check for packages (comparing the package feerate to the feerate of the direct conflicts) so that is good at least, but the description of the logic in the documentation should point out that there is some feerate check happening as well.

[instagibbs]

It's ancient lore by now, but I'm not sure it's a "mis-statement" or just incentive incompatible stuff no one implements(becase feerate check is cheap). I agree with your reading of current texts either way and agree pulling that into the text is the right thing to do.

[glozow]

require the ancestor feerate of the child transaction to be higher than the actual feerate of all transactions that would be evicted. This is conservative but I think would ensure that we don't have any situations where there are transactions being replaced that might appear in the next block, while the new transactions do not.

I think this is a sensible addition. I cannot think of any easy attacks since it's only the directly conflicting transactions and not their descendants (i.e. I don't have the concerns stated in #23121 (comment)).

Btw, don't we want min(package feerate, child ancestor feerate)?

In pseudocode for a child-with-unconfirmed-parents package:

for each tx in directly_conflicting_transactions:
    if min(package_feerate, child.ancestor_feerate) <= tx.individual_feerate
        fail

With this, do you think it would be acceptable to remove the "no new unconfirmed inputs" rule in package RBF?

we compare the feerate of a new transaction coming in to the feerate of the existing transactions that it directly conflicts with

Right, this wasn't in BIP 125. I also misinterpreted that part of the code to be part of the other fee-related rules and just a "fail fast" piece of logic, and didn't include it in # #23711 either (gah!). Just opened #25382 to add test coverage for it in isolation + add it to doc/policy/replacements.md

[sdaftuar]

Assuming we can figure out something sensible, it seems better to have both ancestor and descendant limits? Adding descendant limits resolves the "you must pay for all conflicts" type of pinning. But adding an ancestor limit could be beneficial in resolving the "create a replacement tx with a giant ancestor" type pinning.

Yeah I was thinking in this direction as well. I wonder if we could find some super minimal use case, like a 2-transaction package replacing another 2-transaction package, and just implement that to start. I think the rough idea would be:

• some opt-in flag (eg nVersion=3, or anything else) indicates that a transaction can't have more than 1 in-mempool descendant with no more than X vbytes of total size. Any descendant is also not allowed to have more than 1 ancestor.
• When a 1-child, 1-parent package comes in, if it conflicts with at most 1 opt-in package, allow for RBF semantics along the lines of what we're discussing. In this case, I think we would require the package feerate to be greater than the ancestor feerates of both transactions that would be replaced (along with the total fee check, and the incremental relay fee check).

Would there be value to any layer 2 projects if something this minimal were to be implemented?

[t-bast]

This would be very valuable for lightning. @instagibbs has explored that space recently.

A single descendant is too limiting though: in the lightning case, each participant has its own anchor output, so we need to allow 2 descendants if we apply that rule to the commitment transaction.

[ajtowns]

[1] In looking at this example, I noticed that BIP 125 as drafted seems to misstate an important check we perform in our current RBF policy, namely that we compare the feerate of a new transaction coming in to the feerate of the existing transactions that it directly conflicts with.

Perhaps we should aim for a new rbf bip that describes bitcoin core's actual policy for both rbf of single txs and packages of txs, and document that we implement the new bip, and not 125?

[instagibbs]

To recap my thoughts:

0. I really think this direction is somewhere we need to go. The fact that in my sketches I can get rid of carveout, and naively support N-party contracts speaks to me that we're on the right path. The fact it makes simple systems like batched payouts potentially more robust is also a win.

1. I am 99% sure we want to limit entire package limits to make rule#3 pinning not a kill shot for replacement replacement

2. @t-bast Ephemeral dust, or dust values that are immediately spent within the package, enable LN and other systems to switch over to a single anchor output, obviating the requirement for carveout rules, and would fit within the one-parent-one-child pattern.

We could expand this to two direct children limit, and preserve the carveout to make sure currently deployed systems get benefits, assuming it's not otherwise a DoS/incentive compatible issue

3. Another nice-to-have would be forced signaling of replacability of the child tx, since the single-anchor spend is mostly freeform. We can work around this via "0 CSV OP_1ADD" scriptpubkey which forces RBF signaling, but something to consider. Full RBF solves this longer term as well.

[t-bast]

@t-bast Ephemeral dust, or dust values that are immediately spent within the package, enable LN and other systems to switch over to a single anchor output, obviating the requirement for carveout rules, and would fit within the one-parent-one-child pattern.

💯

We could expand this to two direct children limit, and preserve the carveout to make sure currently deployed systems get benefits, assuming it's not otherwise a DoS/incentive compatible issue

Don't bother if it's only for lightning, since we'll need a (small) change on the commitment transaction format to benefit from it, we might as well directly switch to a single anchor output.

[t-bast]

I have added a first set of tests to eclair that exercise the package-rbf logic: ACINQ/eclair@4f583b5

I extracted the transactions as test vectors here: https://gist.github.com/t-bast/7c553e61ff2bee3720ff4f7db04cc1b3
I'm not sure how easy it will be to translate them to bitcoin core tests, you'll need to re-generate some of the inputs and re-sign the commit transactions. The file should have all the required data, but it may be quite painful still...

The third test vector triggers the following error: Internal bug detected: "it != package_result.m_tx_results.end()"

[glozow]

Apologies for the delay, I am rebasing + working on the following:

• Some unit tests for RBF code
• Adding an ancestor feerate rule policy: nVersion=3 and Package RBF #25038 (comment)
• Adding a set of rules for nVersion=3, i.e. stricter ancestor/descendant limits, inheritance, signaling for RBF, etc. policy: nVersion=3 and Package RBF #25038 (comment) Won't be a finalized proposal, I'm hoping that having code written will help us iterate on what those rules should be.

Will update soon!

[glozow]

if we're doing nVersion 3, we should also fix the lack of discouragement for uninterpreted nSequence values and arguments to CSV/CLTV. see #22871

(cc @instagibbs) decided not to do this for a few different reasons:

• It's not intended for everyone to stop using v2 and switch to v3; this isn't really a "standardness rules version bump" so making something nonstandard here doesn't have the effect of discouraging it.
• From speaking to a few folks, it doesn't seem like there are any short- or long-term plans to upgrade using these fields. So the work would likely not pay off.
• It seems the branch as it's written may have the effect of making some LN transactions nonstandard.

[instagibbs]

@glozow I think at best you'd get ~7 bits reserved(without colliding with well-known protocols), and only for v3, which as you note isn't a general version bump

[glozow]

Reviewers: note that the first half (7 commits) of this PR has been opened separately, in #28948. This has been rebased and current outstanding comments addressed.

[DrahtBot]

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

[glozow]

Closing this as superseded by #28948 and #28984 (v3 and package RBF PRs respectively). Note that the new package RBF has a slightly different approach than the one implemented here, permitting txns based on topology instead of v3-only. The discussion on this PR may still be helpful for context, but the volume of comments makes it pretty hard to do any review here.