policy: Trim Packages when transaction with same txid exists in mempool

glozow commented at 2:23 pm on June 15, 2021: member

[WIP] This requires #22253 and is built on top of #21800, please review them first.

This PR implements deduplication of package transactions that have the same txid as transaction(s) in the mempool. In that sense, our policy changes because it is no longer all-or-nothing. I would still call it “atomic” because it either decides (1) all transactions may be submitted to mempool or (2) there should be no change to the mempool. Importantly, we deduplicate by txid rather than wtxid - a transaction with the same nonwitness data should be sufficient to validate the descendants in the package.

I consider this a prerequisite to submission of packages to mempool, because it would be a rather annoying limitation if we couldn’t tell that the package was already partially in the mempool. It could also be problematic if someone could submit the transaction with a different witness to cause the package to be rejected.

misc doc improvements 409f7647b6

[mempool] extend CalculateMemPoolAncestors for packages

When calculating ancestor/descendant counts for transactions in the
package, as a heuristic, count every transaction in the package as an
ancestor and descendant of every other transaction in the package.

This may overestimate, but will not underestimate, the
ancestor/descendant counts. Most use cases of submitting as a package
involve two transactions in a parent-child relationship, so this
shortcut still produces an accurate count.

f3504c8615

[policy] ancestor/descendant limits for packages 451c79a9c3

[test] package mempool limits ef6004ca88

[validation] distinguish same txid different wtxid in mempool 1d9ca2934c

[test] submit same txid different wtxid as mempool tx

Co-authored-by: Antoine Riard <ariard@student.42.fr>
Co-authored-by: Antoine Riard <antoine.riard@gmail.com>

c23f72a239

[validation] include vsize in MempoolAcceptResult 7ebb8fe366

[rpc] use MempoolAcceptResult vsize in testmempoolaccept

In the future we might return a result for a same-txid-different-wtxid
transaction and should be consistent about which transaction we're
referring to.

7a8617aaf9

[policy] trim package when tx is already in mempool

Packages are still validated atomically as in, there are only 2
possibilities: (1) All transactions would be accepted to mempool OR (2)
The mempool would not change at all.

b9942821da

[test] package trimming 06a4d498c4

[rpc] handle package trimming 188cab0af2

[test] pin package using same-txid-different-wtxid tx 0eb938b4ea

glozow force-pushed on Jun 15, 2021

glozow added the label Validation on Jun 15, 2021

DrahtBot commented at 8:27 pm on June 15, 2021: 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:

#22290 by glozow
#22261 by jnewbery
#22253 by glozow
#21800 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.

ariard commented at 1:01 am on June 16, 2021: member

Concept NACK. At least for now.

Firstly, package trimming might be handled at the p2p-layer instead of the mempool acceptance one. I believe we have not decided yet on a package-announcement protocol, one of the solution could be something INV(“utxo:feerate”)-based, which would assume the exact feerate package is evaluated by the receiver (or discarded from fetching is inferior to the best known in-mempool package for this utxo). Another solution could be to announce package_ids or another one could be to announce the raw serie of transaction identifier corresponding to a package.

Secondly, i don’t think this policy complexity is required for a v0.1 package-relay aiming to solve the pre-signed feerate problem for L2s. If the root transaction is already in the mempool, it doesn’t offer a benefit for those protocols to reevaluate a later-received proposal. The package can still be bumped as a CPFP, not need to swallow additional complexity on our side.

Thirdly, i think this opens a new pinning vector (though not for LN use-case) in the case of the in-mempool same-txid-different-wtxid’s feerate is far-lower than the same-txid-different-wtxid inside the package candidate. E.g Alice might expected to have pre-signed transaction with a feerate of 10 sat/vbyte and a CPFP of 20 sat/vbyte. Malicia has previously broadcast the pre-signed transaction, with a different witness, offering a feerate of 5 sat/vbyte ?

jnewbery commented at 10:08 am on June 16, 2021: member

Concept ACK. This makes sense for both local package acceptance and for package relay.

ariard commented at 9:27 pm on June 16, 2021: member

@jnewbery

Can you explain in detail why “it makes sense” for package acceptance and package relay ? Especially answering to my third point about introducing a new pinning vector, I’m happy to write a functional test if it helps the discussion :)

glozow commented at 10:22 am on June 17, 2021: member

@ariard

i think this opens a new pinning vector (though not for LN use-case) in the case of the in-mempool same-txid-different-wtxid’s feerate is far-lower than the same-txid-different-wtxid inside the package candidate.

This pinning attack is precisely what this PR aims to [begin to] address - that’s why there is a very similar functional test for it in this PR. The only difference is that it’s not based on feerates since that is irrelevant at the moment: without package trimming, the entire package would be rejected as a “conflict” with the mempool. If/when someone implements replacement of same-txid-different-wtxid transactions (i.e. what you trying to do in #19645), that will be even better and 100% compatible with this. #22253 is attempting to make that work a bit faster.

jnewbery commented at 10:32 am on June 17, 2021: member

@ariard if an RPC client or peer sends a package {A,B}, and your mempool already contains {A}, then we shouldn’t reject the package. Similarly your mempool already contains {A’} (where txid(A) = txid(A’) but wtxid(A) != wtxid(A’)), then we also shouldn’t reject the package.

If A’ is already in the mempool, then its feerate can’t make B and more or less viable for mempool acceptance, since the viability of a transaction is based on its descendant feerate. So:

if A’ has too low a feerate to enter the mempool and {A,B} is later submitted, then {A,B} is processed as a package - no change
if A’ has sufficent feerate to enter the mempool and {A,B} is later submitted, then A is trimmed, {B} is processed individually, and the feerate of A and A’ is irrelevant.

I’m happy to write a functional test if it helps the discussion :)

Yes please - code speaks more precisely than text. If there’s some subtlety that I’m missing, then a concrete example would clarify that.

ariard commented at 6:25 pm on June 17, 2021: member

@glozow

This pinning attack is precisely what this PR aims to [begin to] address - that’s why there is a very similar functional test for it in this PR. The only difference is that it’s not based on feerates since that is irrelevant at the moment: without package trimming, the entire package would be rejected as a “conflict” with the mempool. If/when someone implements replacement of same-txid-different-wtxid transactions (i.e. what you trying to do in #19645), that will be even better and 100% compatible with this. #22253 is attempting to make that work a bit faster.

Sorry but I don’t understand the exact pinning scenario you’re aiming to solve ? I would be glad if you can detail it for the sake of discussion clarity and see if we agree on the problem terms. In the context of L2s, or multi-party transactions we have few types of tx-relay jammings to care about.

One of them is witness malleability as described here. Another one is transaction pinning. The intentional attempt with #19645 was to start the preliminary work to solve witness malleability by authorizing our mempool to accept wtxid. But ultimately addressing it for singe-tx was relying on a change of our RBF rules, IIRC. What I’m concerned of with this PR, is while attempting to solve witness malleability, it’s opening a new vector of transaction pinning, namely downgrading the feerate of a honest package at package evaluation by our ProcessNewPackage, if you’ve an already in-mempool first package component with a lower, malicious feerate.

ariard commented at 6:34 pm on June 17, 2021: member

@jnewbery

if A’ has sufficent feerate to enter the mempool and {A,B} is later submitted, then A is trimmed, {B} is processed individually, and the feerate of A and A’ is irrelevant.

I think the feerate of A/A’ is still relevant as you have to consider the whole package feerate for block inclusion (see addPackageTxs), and a lower, malicious A feerate is decreasing the odds of inclusion. Which is one of the purpose of the pinning, not only block mempool acceptance, but also downgrade odds of soon-confimation (an outcome we considered in the past, see discussion about carve-out design : https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html iirc)

Yes please - code speaks more precisely than text. If there’s some subtlety that I’m missing, then a concrete example would clarify that.

Yep, will do in the coming weeks, when I’ve time. Otherwise, if my concern is realized, and this change introduces a flaw in our mempool logic, I’ll tread this a security issue, as usual, not forgetting to point it was a contentious change w.r.t to our review process.

I think we have time with such context-rich work (mempool, second-layer implications, an array of attacks scenarios to condisder, …), so I believe this PR isn’t ready at all. Also I think it’s relevantess is still function of deciding of a package-announcement p2p mechanism, as ihmo we might achieve at a lower-layer than the mempool. Do you have a sound package relay specification draft available somewhere against which we can evaluate this proposal ?

ariard commented at 11:24 pm on June 18, 2021: member

John, while digging recently through the archives of #10823, I saw your engagement standing as an example of patience, listening with the potentially affected Bitcoin users, and the implications for their businesses. Genuinely, I think that was a worthy example of how project contributors should consider policy change when the implications are significant and ecosystem-wise.

I’ve always been eager of what Optech has done for the ecosystem. This team pouring thousands of hours of work, not only to weekly inform Bitcoin devs/business without reprieve, but also pulling up Bitcoin OSS standards, everytime the opportunity was met, such as the occasion of the Taproot public review. Inspired by this action, I’ve recently setup a serie of workshops to the awareness of the always-growing Lightning/L2 community. Driven first by a goal of sharing back knowledge around the safety and performance issues they’re encountering in the last years about our policy but also to observe if a consensus (or a lack of it) was emerging among L2s users and the wider community about ongoing and future changes of our policy, as they’re going to be the first ones impacted in case of bugs or subtle safety issues in our mempool.

Note, maybe I should have start those workshops as early this serie of works was started with #20833, but at that time I wasn’t sure the direction of this work, and which exact problem it was trying to solve, either laying the ground of hopefully a future libbstandardness for Core or laying the ground for package-relay and I did say so.

Building on top of this, and hoping to clear miscommunications among us on this serie of work, i explained the problems that L2s are encountering on the base-layer in this new published mail : https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-June/003057.html, on which I hope you’ll take time to think about. Until so, I’ll maintain my Concept NACK on this PR as I don’t think it’s grounded enough to pass the bar of our downstream projects expectations.

With the always-ongoing growth of the Bitcoin ecosystem, IMO such standards of development, where we carefully listen to our users and if the feedback provided is relevant, delay engineering works in consequence is slowly going to become the norm, even if it takes years to get there. I did appreciate how #21528 sets a step in this direction, where the PR author took considerable time to reach out potentially affected downstream projects to minimize service-disruption.

W.r.t to my previous comment expressing in-advance safety concerns with this PR, I’ve poured time in the last months browsing our mempool logics correctness and I’ll be glad to not have regressions on this side.

Finally, I hope in this serie of L2-related works you’ll keep standing as the public example you have been for this Bitcoin community in the past years and I always kindly remember the John I used to know at Chaincode, diligently catching up my countless wallet refactos bugs. We’re humans after all, and as such we have all good and bad days, an assumption I believe true for any contributors to this project.

glozow closed this on Aug 23, 2021

DrahtBot locked this on Aug 23, 2022

policy: Trim Packages when transaction with same txid exists in mempool #22252

Conflicts