policy: nVersion=3 and Package RBF

[glozow]

Part of #22290.

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. In particular, this allows LN users to replace commitment transactions existing in the mempool, simply by broadcasting the commitment transaction with a high-fee child (provided it pays enough fees to replace the existing tx along with its descendants).

This is similar to what was proposed on the mailing list.

[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

[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 .

[t-bast]

I have added a first set of tests to eclair that exercise the package-rbf logic: https://github.com/ACINQ/eclair/commit/4f583b5725cba6594388d54c0b31affc1bf8cddf

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 https://github.com/bitcoin/bitcoin/pull/25038#discussion_r907576864
• Adding a set of rules for nVersion=3, i.e. stricter ancestor/descendant limits, inheritance, signaling for RBF, etc. https://github.com/bitcoin/bitcoin/pull/25038#discussion_r908996095 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]

we should probably bikeshed the protocol hard-coded value and use the lowest value we can to minimize the harm a malicious child tx can do.

Here's my reasoning for the current value (4000vB), but open to feedback and I will of course add the rationale to the version3_transactions.md doc:

• Upper bound: the larger we make this limit, the more vbytes we may need to replace. With default limits, if the child is e.g. 100,000vB, that might be an additional 100,000sat or more, depending on the feerate.
• Lower bound: the smaller we make this limit, the fewer UTXOs we can use to fund this fee-bump. If we require all fee-bumps to only use 1 UTXO, for example, we're be requiring wallets to always maintain a pool of high-value UTXOs, one for each channel they might close.

If you're broadcasting commitment tx commit_a and 4000vB is the largest output your counterparty can attach to their commitment tx commit_b, you can easily meet (commit_b.fees + 4000 * minfeerate) + (incrementalfeerate * commit_a.size) fee to replace.

4000vB should give you at least 50 inputs depending on the output type, which I'm guessing is a reasonable restriction. If you say, hey, 10 inputs is enough, then we could lower this to 1000vB.

[t-bast]

Here's my reasoning for the current value (4000vB), but open to feedback and I will of course add the rationale to the version3_transactions.md doc

Thanks for the explanation!

4000vB should give you at least 50 inputs depending on the output type, which I'm guessing is a reasonable restriction. If you say, hey, 10 inputs is enough, then we could lower this to 1000vB.

Since that child transaction is only bringing fees, it shouldn't need a big input amount, so I'd say 50 inputs is clearly too much, 10 inputs should be more than enough, shouldn't it?

[instagibbs]

@t-bast and I realized that under this proposal, even excluding ephemeral outputs, we can do all the anchor improvements up/to the "ephemeral" part for ln-penatly.

i.e. Change to an OP_TRUE single anchor output for commitment transaction, but right at the relay output dust level.

Wouldn't be applicable to eltoo-like constructs, but these are more rare it seems. Most contract types can survive bleeding off a few sats once. e.g. batched payouts, coinjoins, ln-penalty

Scope creep to OP_TRUE becoming a standard output(up to 1 a tx?) to relay might be worth it? We can figure out the ephemeral part as a nice cleanup of current protocols further in future, and fix for eltoo-like stuff.

[glozow]

Since that child transaction is only bringing fees, it shouldn't need a big input amount, so I'd say 50 inputs is clearly too much, 10 inputs should be more than enough, shouldn't it?

Oh great! Then I'll limit the v3 child to 1000vB. Very simple :ok_hand: :grinning:

[glozow]

Ready for review! Writing a mailing list post at the moment. You can also look at doc/policy/packages.md and doc/policy/version3_transactions.md.

[glozow]

Rebase + deleted some unused code + tweak to the signaling logic. Posted to the mailing list just now https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html

[JeremyRubin]

Adding a note here:

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

[glozow]

This is mentioned in a few other places, but Rule 3 pinning via additional ancestor is still not resolved with the multi-parent-1-child case. Quick diagram to illustrate - this was pointed out to me by @instagibbs.

Working on further restricting the rules to 1-parent-1-child since that eliminates the possibility of a conflict's high fees diluted by ancestors. Will add more tests as well, and then will take out of draft when finished.

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

Will consider adding this if people feel strongly. Wondering if others on this thread have thoughts on it?

[glozow]

This PR is open for review but leaving as draft, attempting to shave off the beginning commits in #26646 to make this smaller and v3-focused.

[glozow]

Last push is just a rebase. Still need to do:

• Not allowing non-v3 below minrelayfee in package CPFP. Currently working on test util to raise mempool min feerate for this.
• Removing below-minrelayfee-and-no-longer-bumped transactions after replacement.

[instagibbs]

Not allowing non-v3 below minrelayfee in package CPFP. Currently working on test util to raise mempool min feerate for this.

Implication being a v1/2 transaction cannot be 0-fee and bumped by child?

[glozow]

Implication being a v1/2 transaction cannot be 0-fee and bumped by child?

Yes unfortunately, unless there is a better alternative... see ~08:16 in https://gnusha.org/bitcoin-core-dev/2022-12-16.log

[ariard]

Lightning legacy channel types (before BOLT9's option_anchors_output / option_anchors_zero_fee_htlc_tx) presents the issue of counterparty's second-stage HTLC transactons malleable enough to opt-out from BIP125 replacement rules (the preimage path on the local commitment transaction offered output and the timeout path on the local commitment transaction accepted output). This path can be leveraged by a counterarty not only to do opt-out pinning but also for rule #3 style of pinning, where the second-stage HTLC transaction is malleated to a high-fee/low-feerate.

This repleaceability issue is moving away with option_anchors_zero_fee_htlc_tx, where all the second-stage spends must signal replacement due to the new CSV 1 to satisfy. This anchor states are currently signed under nversion=2, assuming we have dynamic upgrades, a new channel type with states committing to nversion=3 could be deployed in-flight (i.e without confirming on-chain new funding outputs). Those channels would be safe against rule #3 style of pinning, as if old RBF opt-in nversion=2 states are used for pinning, under the proposed policy rules ("All directly conflicting transactions signal replaceability explicitly, either through BIP125 or V3.", the incentive-compatible enhanced RBF rules should apply.

As of today, option_anchors_zero_fee_htlc_tx is enabled by default by Eclair (since v0.7.0) and LND (since v.0.12.0). This is not deployed by default by CLN and LDK. So we would still have probably tens of thousands of legacy channels forever exposed to pinning attacks mitigated by nversion=3. For this subset, this is a wonder if we would like to introduce some "retro-active" policy effect, where nversion=3 effects are granted to the legacy channel types with some at-broadcast opt-in mechanism. E.g a signature from a pubkey of the funding output committing to a package version bit. Of course, this would be more technical complexity on the Core-side, at the security and economic benefit of Lightning channels, so trade-off.

Lightning node operators of legacy channel types could be also just force-close all channels, and re-open new safer ones. Assuming it's done in an asynchronous fashion (to avoid accidental flood&loot situations) and mempool fees, this should be okay. On the other hand, I can see the operators incentives to keep unsafe channel open to preserve good reputation accumulated in routing algorithms.

Personally, I think the deployment cost are low enough on the Lightning-side to not bother with dedicated "retro-active" policy mechanism on the Core-side, at least in this case. Though this is only my own appreciation of the deployment cost and it's highly probable we'll have newer policy to deploy in the future and interactions with channel types, so I think it can valuable to think about it.

[ajtowns]

For this subset, this is a wonder if we would like to introduce some "retro-active" policy effect

The alternative approach would be for early lightning adopters to update their channel on-chain to permanently invalidate any old states that might enable pinning (eg, by splicing some funds in or out)... Simple is better than complicated.

[ariard]

The alternative approach would be for early lightning adopters to update their channel on-chain to permanently invalidate any old states that might enable pinning (eg, by splicing some funds in or out)... Simple is better than complicated.

On this present case, this is my thinking too ("I think the deployment cost are low enough on the Lightning-side to not bother with dedicated "retro-active" policy mechanism on the Core-side"). However, deployment workaround for new channel types have already been proposed in the past, and to avoid hard synchronisation issue (i.e provoking mempool backlogs) we might have to rely on "retro-active" police deployment in the future. That said, we probably still have one or two orders of magnitude of the number of Lightning channels growth before this starts to be a concern.

[glozow]

Rebased

[instagibbs]

Looks like this lost TrimToSize changes?

commit ed6045a8f99f14987977e13faa1865c3bf56890f
Author: glozow <[email protected]>
Date:   Tue Jan 17 13:43:27 2023 +0000

    [mempool] evict everything below min relay fee in TrimToSize()
    
    At this point it's not expected that there are any such transactions,
    except from reorgs and possibly when loading from mempool.dat.

I rely on this for ephemeral anchors, trying to rebase.

[glozow]

Fixed accidental drop of TrimToSize commit

[glozow]

Rebased on top of #28251 for the issue posted in #26403

[ariard]

Looked on the proposed doc/policy/version3_transactions.md changes with other Lightning eyes on a whiteboard, and as far as I can tell it works well for Lightning usual flow and also upcoming dual-funding / splicing: https://github.com/lightning/bolts/pull/863 (to be checked again when the specs are finalized)

The replacement rule for signed version=3 transactions is the following as of 06e41874, it can replaces nversion=2 signaling replaceability or opt-out transactions in a node with mempoolfullrbf=1. It cannot replace non-signaling nversion=2 (L785 in src/validation.cpp). In term of types of Lightning channels:

• old legacy channels: second-stage HTLC txn on counterparty commitment can be still be pinned (cf. mailing list post https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-April/002639.html)
• anchor-outputs channels: commitment and second-stage HTLC are explicitly signaling RBF on both asymmetric states

Once nversion=3 is deployed, the new nversion=3 should be able to to replace all the anchor-outputs channels transactions without rejection. I think old legacy channels Lightning node operators would be advised to upgrade to anchor-outputs channels as soon as it’s ready in their Lightning implementations, and they’ve practical understanding of fee-bumping reserves management and swallow the bullet in term of on-chain fees. In the current state of things, I don’t think there is a need to introduce unilateral retroactive policy rules effect as suggested above, though this can be a welcome mechanism in the far future to massively save on on-chain fees for the Lightning ecosystem.

I think missing test coverage for v3 transactions not replacing not-bip125-signaling v2 transaction

commit aec6ea3a378d532cd86f8812006e4eb42e185662
Author: Antoine Riard <[email protected]>
Date:   Thu Sep 7 04:27:49 2023 +0100

    Add non-v3 trasnsactions not signaling not replaceable test

diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index e969b0249b..dd1c55be05 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -143,6 +143,24 @@ class PackageRBFTest(BitcoinTestFramework):
         self.assert_mempool_contents(expected=package_txns_v3, unexpected=[tx_bip125_v2["tx"]])
         self.generate(node, 1)
 
+        self.log.info("Test that non-V3 transactions not signaling BIP125 are not replaceable")
+        coin = self.coins.pop()
+
+        tx_nobip125_v2 = self.wallet.create_self_transfer(
+            fee=DEFAULT_FEE,
+            utxo_to_spend=coin,
+            sequence=MAX_BIP125_RBF_SEQUENCE +1,
+            version=2
+        )
+        node.sendrawtransaction(tx_nobip125_v2["hex"])
+
+        self.assert_mempool_contents(expected=[tx_nobip125_v2["tx"]])
+        assert not node.getmempoolentry(tx_nobip125_v2["tx"].rehash())["bip125-replaceable"]
+        assert tx_bip125_v2["tx"].nVersion == 2
+        package_hex_v3, package_txns_v3 = self.create_simple_package(coin, parent_fee=0, child_fee=DEFAULT_FEE * 3, version=3)
+        assert all([tx.nVersion == 3 for tx in package_txns_v3])
+        assert_raises_rpc_error(-26, "txn-mempool-conflict", node.submitpackage, package_hex_v3)
+
     def test_package_rbf_additional_fees(self):
         self.log.info("Check Package RBF must increase the absolute fee")
         node = self.nodes[0]

[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 https://github.com/bitcoin/bitcoin/pull/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