v3 transaction policy for anti-pinning

[glozow]

See #27463 for overall package relay tracking.

Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418

Rationale:

• There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see Cluster size 2 package rbf #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
• Switching to a cluster-based mempool (see Proposal for a new mempool design #27677 and [WIP] Cluster mempool implementation #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.

V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.

Immediate benefits:

• You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
• Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.

This also enables some other cool things (again see #27463 for overall roadmap):

• Ephemeral Anchors
• Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
• We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
• We can switch to a cluster-based mempool [5] (Proposal for a new mempool design #27677 [WIP] Cluster mempool implementation #28676), which removes CPFP carve out [6].

[1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
[2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
[3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
[4]: Original PR #25038 also contains a lot of the discussion
[5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
[6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12

[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
ACK	sdaftuar, instagibbs, achow101
Concept NACK	petertodd
Concept ACK	ismaelsadeeq

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:

• #29325 (consensus: Store transaction nVersion as uint32_t by achow101)
• #29306 (policy: enable sibling eviction for v3 transactions by glozow)
• #29242 (Mempool util: Add RBF diagram checks for single chunks against clusters of size 2 by instagibbs)
• #28984 (Cluster size 2 package rbf by instagibbs)
• #28960 (kernel: Remove dependency on CScheduler by TheCharlatan)
• #28886 (refactor: Replace sets of txiter with CTxMemPoolEntryRefs by TheCharlatan)
• #28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

[dergoegge]

Would be good to have some fuzzing for the new policy rules. Maybe amending the existing fuzz targets (tx_pool, tx_package_eval) to include v3 txs is enough? A standalone harness for the new rules would probably also be good regardless.

[glozow]

Changed tx_pool and tx_package_eval fuzzer to sometimes create v3 transactions, and check v3 invariants at the end of each iteration.

[dergoegge]

$ echo "ACVL/gA+Pj4+Pj4+3D4AAAAAAAAAAAAAAAAAAAAAAAAAAAAAPT09//8AAP/8AAAAigAAAAAAAAAAAAAAAAAAAAAAAAD6AAAAAAAIAQk9AAAAAAAAAAAAAAAAAAAAABMA9wAAAAYiACkAAAAAAAAAAAD///8AAPkAAAAkAAAAAACiAGxpbWl0YW739/f3Y2VzdG9yc2lSemUAAAAmAKMAAP9kZQBi4nUxZ2xvZ2ZpbGUAJgD/AA==" | base64 --decode > tx_pool-bbe7bc3c2a6d0cb1a21c48506cd16fdb807ae3cf.crash
$ FUZZ=tx_pool ./src/test/fuzz/fuzz tx_pool-bbe7bc3c2a6d0cb1a21c48506cd16fdb807ae3cf.crash
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1136977254
INFO: Loaded 1 modules   (572973 inline 8-bit counters): 572973 [0x563877bfae70, 0x563877c86c9d), 
INFO: Loaded 1 PC tables (572973 PCs): 572973 [0x563877c86ca0,0x563878544f70), 
/workdir/fuzz_bins/fuzz_libfuzzer: Running 1 inputs 1 time(s) each.
Running: /workdir/crashes/crash-bbe7bc3c2a6d0cb1a21c48506cd16fdb807ae3cf
test/fuzz/tx_pool.cpp:114 Finish: Assertion `entry.GetModFeesWithDescendants() > 0' failed.
==1877== ERROR: libFuzzer: deadly signal
    #0 0x5638764b4c88 in __sanitizer_print_stack_trace (/workdir/fuzz_bins/fuzz_libfuzzer+0x14b4c88) (BuildId: 7fbfcc32a58adde3cb3dcfe8229731b5bb30d71d)
    #1 0x56387648c04c in fuzzer::PrintStackTrace() crtstuff.c
    #2 0x563876471e67 in fuzzer::Fuzzer::CrashCallback() crtstuff.c
    #3 0x7f168cc0850f  (/lib/x86_64-linux-gnu/libc.so.6+0x3c50f) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #4 0x7f168cc560fb  (/lib/x86_64-linux-gnu/libc.so.6+0x8a0fb) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #5 0x7f168cc08471 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x3c471) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #6 0x7f168cbf24b1 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x264b1) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #7 0x5638778c6167 in assertion_fail(std::basic_string_view<char, std::char_traits<char>>, int, std::basic_string_view<char, std::char_traits<char>>, std::basic_string_view<char, std::char_traits<char>>) check.cpp
    #8 0x56387682def1 in (anonymous namespace)::Finish(FuzzedDataProvider&, (anonymous namespace)::MockedTxPool&, Chainstate&) tx_pool.cpp
    #9 0x563876832e79 in (anonymous namespace)::tx_pool_fuzz_target(Span<unsigned char const>) tx_pool.cpp
    #10 0x5638768972c7 in LLVMFuzzerTestOneInput fuzz.cpp
    #11 0x563876473334 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) crtstuff.c
    #12 0x56387645c263 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) crtstuff.c
    #13 0x563876461e86 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) crtstuff.c
    #14 0x56387648c9d6 in main crtstuff.c
    #15 0x7f168cbf36c9  (/lib/x86_64-linux-gnu/libc.so.6+0x276c9) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #16 0x7f168cbf3784 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x27784) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #17 0x563876456cd0 in _start (/workdir/fuzz_bins/fuzz_libfuzzer+0x1456cd0) (BuildId: 7fbfcc32a58adde3cb3dcfe8229731b5bb30d71d)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal

[ariard]

Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.

I don’t think adopting cluster limits instead of max ancestors / descendants limits change anything for V3 packages. What matters is the overall weight limit of the package (4000 WU for a child), as this limit draws an anti-pinning security bound on the lowest off-chain payment that one can afford to burn as an absolute fee . Limit to be evaluated in function of network mempools congestion. Removing the absolute fee replacement rules I think would simplify current pinning analysis - though sounds this is beyond the scope of this proposal.

[glozow]

CI is green. I've added a rule for maximum sigops and updated the doc.
(cc @sdaftuar)

[glozow]

To unblock progress on other work, my suggestion is that we do the following:

* modify this PR to introduce the v3 policy and validation rules, but not make v3 transactions standard for relay yet -- this should be a very small code change I think

Pushed to address @instagibbs review and make v3 not-yet-standard.

[ariard]

To be clear, package relay with package RBF is meant to be resistant to these kinds of network topology aware (NTA) pinning scenarios. With correctly implemented package relay it doesn't matter that a CPFP tx did a CPFP on top of a commitment transaction that no-one but the originator has: provided the fee (or with RBFR, fee-rate) outbids the competing commitment transaction, it will be replaced as nodes evaluate the new package as a whole.

i’m not sure if the adversary injects malicious package A in target’s mempool, and package B in other network mempools.
a v3 parent is not necessarily a LN commitment transaction however CPFP can be in conflict between A / B.
see the ugly NTA pinning scenario described in my old mail.

Yes, I wrote this branch a few months ago: https://github.com/glozow/bitcoin/commits/2023-11-1p1c-v3-ea/
I also included a generic (i.e. not LN-specific) version of this scenario as part of the package RBF + package relay test suite. See the "[functional test] package RBF network" commit.

thanks interesting to test both NTA and “loophole” pinnings on this branch described earlier to t-bast here

If the issue is about alice_commitment_tx + alice_cpfp_on_alice_tx replacing bob_commitment_tx, this proposal (v3 + package RBF + 1p1c package relay) does resolve it and is in fact designed primarily for that purpose.

sure package relay fixes this, less sure even if you add package RBF if you combine with some leeway given by 1000vb limit

Updating my concept ACK. Reading through the numerous comments on this PR, the objections that have been raised have nothing to do with v3, and instead have to do with applications that might use it, and what their best design is. I think applications are free to optimize further, but this PR is not the place to do spec design of layer 2 protocols.

no, if we follow this line of reasoning imo it’s like saying it’s okay to have IPsec or TLS shipping with weak crypto primitives
the objections which have been raised are very related to the v3, at least the 1000vb, see the “loophole” here
secp256k1 dev are spending amount of time to make its high-level api safe for “dumb” users, we should do the same.

So far, we've had some support from LN spec developers (@t-bast, @TheBlueMatt) for the idea that the v3 rules meet their use case. The others have been silent so far, so it's not yet clear that v3 will be adopted; but it's also not yet clear that it won't be.

i broke enough time LN spec dev stuff to take their technical opinions at their fair prices.

Moreover, there is another issue besides pinning that is relevant -- the CPFP carveout rule. I brought up in #29319 that in order to eventually be able to deploy cluster mempool without breaking current use cases, we'll need some alternative to the carveout rule. It sounds like v3 + sibling eviction (proposed in #29306) may be sufficient -- again, we have concept ACKs from @t-bast and @TheBlueMatt and we're waiting to hear from others who have been silent so far. To unblock progress on other work, my suggestion is that we do the following:

the CPFP carveout rule can be removed with or without v3, its security benefits quasi-null in my opinion.
see comment other issues.

I've been re-reviewing this PR myself today, and I see that @instagibbs has left a few more comments, and my overall sense is that the code here is pretty well reviewed and tested, and should be ready for merge soon.

@sdaftuar do you have a lightning node running somewhere on signet (maybe instagibbs has some v3 CLN branch) ?
my pleasure to try to break it with “loophole” or NTA scenarios, at least try the “loophole” one.
if i or anyone fail to exploit it, great my pinning concerns are unfounded
if i or succeed to exploit, we'll learn and can think more about anti-pinning mitigations.

[sdaftuar]

Code review ACK. Left some comments that are just nits, nothing that I think needs to be addressed unless you have to fix more things anyway.

Will do another round of testing shortly...

[sdaftuar]

ACK 29029df

In addition to my code review and running a slightly earlier version of this branch on a year's worth of historical data, I also did a bunch of mutation testing to confirm code coverage for every component of the new logic (found one block that I think is unreachable, which I commented on above, but doesn't seem like an issue either way).

[instagibbs]

looks like if I revert TX_MAX_STANDARD_VERSION to 3, all unit/functional tests pass?

ACK 29029df modulo that

[glozow]

looks like if I revert TX_MAX_STANDARD_VERSION to 3, all unit/functional tests pass?

I suppose my change at https://github.com/bitcoin/bitcoin/pull/28948/files#diff-cc0c6a9039a1c9fe38b8a21fe28391fffbac9b8531dfda0f658919a9f74b46baR793 is partially to blame. I made it TX_MAX_STANDARD_VERSION + 1 instead of 3, thinking it might be easier to flip in the future.

I'm working on a followup to address the comments, and can incorporate if I need to retouch.

[achow101]

ACK 29029df

[ismaelsadeeq]

Post merge ACK

[ariard]

@sdaftuar @achow101 @instagibbs

Congrats guys to ACK and merge a +1100 LoC PR a Friday evening with still standing objections on the very heart of the PR.
What a lack of professionalism…Still my pleasure to break v3 anti-pinning mitigations in the future, with a pinning toolkit.
If script kiddies don’t make it before me and go to savage the LN ecosystem, who would have naively thought they’re safe.

@glozow

I think there is medium difficulty follow-up to this PR to make pinning far harder to exploit.
Namely a) moving 1000 kb limit on the client-side and b) making it cover the whole package, not the child only.
This can be embedded in bip331 changes or in any future p2p extensions.
That way LN channels could adjust their pinning exposure in the level of trust they have towards their counterparties.
And L2s won’t have to bargain the 1000 kb limit that doesn’t fit their use-case.

[instagibbs]

@ariard

I'll let people decide for themselves our professionalism or lack thereof.

I asked hundreds of comments ago to stop
using this PR as a venue for debating the finer points of the LN spec, obviously unheeded.
There's a BOLT repo, which is an appropriate place on github to discuss these things.

You are also free to lobby different projects to not use these primitives moving forward and push
whatever solutions you think are necessary, even consensus changes. Meanwhile people
who find this useful and want to, they can use it, including use-cases which fall
outside of your sphere of interest.

As was noted already: We're nearing feature freeze, which means if we miss the window the rebasing would continue
on for yet another release cycle. It's left as non-standard which leaves some room for
finding a slightly better local optimum, perhaps informed by @sdaftuar's investigating work
and more feedback.

To end a positive note, I'm looking forward to iterative improvements,
hoping to make things more generally useful for systems deployed today,
improve incentive compatibility, without policing their use-cases:

1. 1p1c relay: p2p: opportunistically accept 1-parent-1-child packages #28970
2. sibling eviction for v3: policy: enable sibling eviction for v3 transactions #29306
3. limited package rbf: Cluster size 2 package rbf #28984
4. cluster mempool: [WIP] Cluster mempool implementation #28676

and leveraging a totally ordered mempool in interesting ways beyond.

[ariard]

@instagibbs

I maintain your review and the ones of @sdaftuar and @achow101 are lacking professionalism.

For a PR which is called "anti-pinning" (it's all in the title), there has been no substantial answer on my concerns that it does not address neither rbf rule3 pinning (the 1000 vbyte rule limit offers to give a way, see "loophole" scenario exposed above), neither topological-based scenario at the tx-relay network level (NTA one), and that actually "hard" limits are a source of "no-one-fits-all" bargainging for use-cases operations (as we have seen with mempoolfullrbf).

On the review process itself and calling to stop using this PR to debate the robustness of the mitigation, this is insulting and a flagrant proof of your lack of system engineering culture. When software engineers are designing new cryptopgraphic primtives or kernel mechanism (e.g linux cgroups), they are of course considering how those mechanisms are used by downstream use-cases.

The current design and review mentality you're exposing and I understanding you're advocating has been proven multiple times a source of potential security failures (e.g CVE-2021-31876 or the 1 CSV added on non-anchor outputs commitment transactions as original carve out mechanism semantics were unclear). Beyond, you're falling in the trap of the protocol design process communication silos I've been pointong out before and I aimed to address in the past with the "L2s Onchain Support IRC Workshop"

On the remark that people who find this useful and want to, they can use it, which I understanding that LN maintainers or anyone else (e.g exchange batching) are free to level up their bitcoin use-cases with nversion=3 semantics. Sadly one has to be very realistic and observe that LN security track record is not great. When you're doing adversarial reviews on financial software you cannot trust the incentives of the developers "in defense", as they have always a moral interest to claim attack X or attack Y is "mitigated", playing some kind of security theater.

Finally, on the call of approaching feature freeze, this is neither a convicining argument in my eyes. In critical system design, development and maintenance there is a social phenomena called process fatigue. Namely, when engineers are getting in front of some deadlines after lengthy work cycles, they are rushing to deliver a technical project, provoking downstream major failure (e.g known historic example is Space Shuttle Challenger disaster).

There is no magical reason that Bitcoin Core review process is not affected by such phenomena of "process fatigue" and as such when there is no review consensus on security-first contribution in one of the part of the codebaee like the mempool, practical wisdom would recommend to delay it to the next release cycle. This is better than yet-another "poor hot fixes" in production were we have to mobilize half-of the engineering, vendors and users ecosystem time, and jeopardize their funds.

To conclude your answer is a poor one sustaining my precedent accusation of lacking professionalism. You're still free to communicate any example or demonstration infrastructure running with v3 semantics that I can target with demo pinning attacks. And as such back up your Bitcoin Core technical review with your credibility and reputation, in a skin-in-the-game approach.

[ariard]

@glozow are you still interested if I do a proof-of-concept bitcoin core branch or idea draft of a) moving the “package limit” on the use-case side and b) make it dynamic (in the limit of absolute core DoS robustness) ? I think it can be backup in future bip331 changes. v3 is your work and this is your PR making anti-pinning claims after-all.

[ariard]

are you still interested if I do a proof-of-concept bitcoin core branch or idea draft of a) moving the “package limit” on the use-case side and b) make it dynamic (in the limit of absolute core DoS robustness) ? I think it can be backup in future bip331 changes. v3 is your work and this is your PR making anti-pinning claims after-all.

did it with #29448

[hebasto]

Considering the current code organisation, I can't see any reasons to include policy/v3_policy.cpp into the libbitcoin_common library. Both its symbols, i.e., SingleV3Checks and PackageV3Checks are referenced in the validation.cpp only. As such, policy/v3_policy.cpp inclusion into the libbitcoin_node library is enough.

[hebasto]

nit: This stuff can be placed into a anonymous namespace.

See SF.22: Use an unnamed (anonymous) namespace for all internal/non-exported entities.