Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package Relay Project Tracking #27463

Open
48 of 56 tasks
glozow opened this issue Apr 14, 2023 · 15 comments
Open
48 of 56 tasks

Package Relay Project Tracking #27463

glozow opened this issue Apr 14, 2023 · 15 comments

Comments

@glozow
Copy link
Member

glozow commented Apr 14, 2023

This issue will be edited frequently to reflect the current status of the project.

What is ready for review now?

👇 👇 👇 👇 👇 👇 👇
p2p: #30111
mempool: #28984
☝️ ☝️ ☝️ ☝️ ☝️ ☝️ ☝️

Why does the roadmap include cluster mempool?

Handling arbitrary ancestor packages (i.e. beyond 1p1c or child-with-parents-tree) is very complex.
There are various higher-priority issues with mempool that make accepting packages more difficult to reason about, and that we should fix before adding ancestor package relay.
We are building the full feature set (package relay, package RBF, bumping 0-fee parents, etc.) for 1p1c packages as we know that is very useful and it is a topology that we can easily handle without cluster mempool.

Why does the roadmap include TRUCs (aka v3 aka BIP 431)?

TRUCs helps deliver some of the 1p1c package relay features like bumping 0-fee parents, and is generally more robust for applications that would seek to use 1p1c package relay.
Also see #29319 for its role in cluster mempool.

Tasks and PRs

(1) multi-parent-1-child package validation

What we get: the ability to validate multiple transactions, including CPFP of transactions below the mempool minimum feerate. An RPC to submit things locally.

See PRs

(2a) Topologically Restricted Until Confirmation (v3) transaction policy

What we get: an opt-in policy for anti-pinning in single transaction or 1-parent-1-child package scenarios. Also, package CPFP of 0-fee parent and package RBF for restricted topologies prior to cluster mempool.

(2b) Package RBF for cluster size 2

(3) 1-parent-1-child package relay

What we get: package relay of 1-parent-1-child packages.
Protocols like LN can use this to create 0-fee presigned transactions with a single, 0-value anchor output; 0-fee commitment transactions can replace each other using the fees of the child attached to the anchor.
This should provide an adequate replacement for CPFP carve out, which is helpful for the next step (see #29319).

(4) cluster mempool

What we get: the ability to quickly assess the incentive compatibility of transactions, safer eviction, more incentive-compatible and pinning-free RBF rules.

(5) more general package relay

Goals: propagate incentive-compatible packages that are more compelx than 1p1c, safely evaluate replacements within packages, handle orphans better.

See also:

Superseded/Deferred Work

Prehistory

@instagibbs
Copy link
Member

Very helpful. I think this helps define "sprints" that can be achieved to hit next milestones.

@ariard
Copy link
Member

ariard commented Apr 17, 2023

I think one more sub-category to track is the feedback collected on how package relay/nversion=3 are fitting multi-party applications and contracting protocols

  • Lightning: legacy channels / anchor output / taproot types
  • Peerswap: claim with preimage + refund cooperatively + refund after csv passes
  • Collaborative transaction: splicing + dual funding flows

The 0-conf flow of Lightning can be also considered. As deployed today by some LSPs where both incoming/outgoing HTLCs are allowed during the pending confirmation period, there is a risk in case of mempool congestions of the inbound funding transaction not confirming before the timelock of the offered HTLC output expires. The current nversion=3 size is 1000 vb which might be too low if LSPs would like to do batching of 0-confs. Note, I think all the pinning risks are not the same for 0-conf funding transactions than for time-sensitive confirmation of LN commitment+HTLCs transactions.

Mentioning 0-conf, and as raised on #27643 at some point in the development cycle of nversion=3 it might be good communication practice to publish than all 0confs users might have to upgrade their mempool monitoring stuff to reject nversion=3, or chain of transactions with them, as otherwise genuine upgrade to the version of Core with package relay might lower their security.

For Lightning, there is the question if the p2p package format is flexible enough for the Lightning channels to apply "new mempool rules" on "old in-flight channnel type". Like backward compatible effect of the mempools rules without off-chain channels having to cooperate on-resigning transactions. I believe that would require a Lightning node to sign the p2p package with the same pubkey than used with the channel (though I start to think we might have multiple types of backward/forward compatible application of mempools rules like the ones mentioned in #25038, like there is some matrix ?)

For the second-layers listed, I'm just considering the ones which have a) a mature specification and b) some substantial deployment though should we consider more second-layers or multi-party applications like vaults (non-deployed) ?

From a reviewer viewpoint, I think it could be very valuable if you can highlight the 2/3 PRs or mail posts you consider as top of the stack for reviews ? Thanks for a package-relay context-tracking meta-issue.

(Feel free to integrate what you find relevant of this comment to ease context tracking of package relay as a project)

@ajtowns
Copy link
Contributor

ajtowns commented Apr 17, 2023

I think at a high level, the "package relay" part looks like:

  • make package mempool acceptance available via rpc in regtest
  • fix up package acceptance/retention/mining policies to be free from DoS vectors and to behave sensibly
  • behave sensibly with arbitrary txs in a package
  • implement bip331 to expose package relay over p2p not just rpc

And relatedly:

  • version 3 acceptance/rbf policies to limit pinnability
  • ephemeral anchors

Particularly if #26933 is applied, is there any reason not to make submitpackage available on all chains? It should behave no differently to submitting the transactions individually with a non-full mempool, no?

@ajtowns
Copy link
Contributor

ajtowns commented Apr 17, 2023

FWIW, I don't think "incentive compatibility" is a good description of our goal here -- I think there's three goals we want:

  • for most normal use cases (including newly invented ones) just submitting your txs over p2p should work fine
    • (otherwise people will tend to use centralised tx submission methods, which creates a chokepoint that will attract censorship)
  • running a plain bitcoind should get you block templates within 90%-99% of optimal, depending on the resources you can allocate to your node
    • (otherwise miners will outsource block template construction to proprietary services in order to be competitive, which again creates a censorship chokepoint)
  • mempool/relay behaviour should be as simple to understand as possible -- for wallet designers, bitcoin users, node operators and miners
    • (otherwise bitcoin will only be usable in theory, not in practice)

I think most of the "incentive compatible" things above aren't really making anything more incentive compatible (miners can just raise their mempool limits to keep the transactions; non-mining nodes aren't receiving any incentives in the first place) but rather making mempool behaviour easier to understand (eg "we removed this tx because there was a race condition when loading the mempool" vs "we removed this tx because it wouldn't be included in any of the next 100 optimal blocks, even if no new txs are submitted").

@glozow
Copy link
Member Author

glozow commented Apr 17, 2023

I think at a high level, the "package relay" part looks like:

Yes, that is how I think of it.

Particularly if #26933 is applied, is there any reason not to make submitpackage available on
all chains? It should behave no differently to submitting the transactions individually with a non-full mempool, no?

Somewhat. It's still a little bit weird when you submit a child-with-parents where a parent relies on the other (grep "Check that validation isn't quitting too early" in #26711, currently at this line). I would say it's safe after the "submit with subpackages" changes.

Also, mempools are full on mainnet (great for testing but means there is a difference), and we'll definitely want to warn them that the transactions are not necessarily relayed etc.

@ajtowns
Copy link
Contributor

ajtowns commented Apr 19, 2023

Also, mempools are full on mainnet

My mempool isn't full, and (I think) hasn't been full this year. (It has a 1GB limit; though there's only ~40MB of txs now anyway)

Submitting a low-feerate tx with a high mempool limit already means your tx won't relay without a cpfp tx, but we expect people to be who are clever enough to do that to also deal with the consequences. (If you do cpfp the low feerate tx, it'll be broadcast via orphan handling, so won't appear in the unbroadcast set any longer, but also won't successfully relay without package processing, so you'll end up in the exact same state, I think)

glozow added a commit that referenced this issue Apr 26, 2023
…kages

bf77fc9 [test] mempool full in package accept (glozow)
b51ebcc [validation] set PackageValidationState when mempool full (glozow)
563a2ee [policy] disallow transactions under min relay fee, even in packages (glozow)
c4554fe [test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee (glozow)
ac463e8 [test util] mock mempool minimum feerate (glozow)

Pull request description:

  Part of package relay, see #27463.

  Note that this still allows packages to bump transactions that are below the dynamic mempool minimum feerate, which means this still solves the "mempool is congested and my presigned 1sat/vB tx is screwed" problem for all transactions.

  On master, the package policy (only accessible through regtest-only RPC submitpackage) allows 0-fee (or otherwise below min relay feerate) transactions if they are bumped by a child. However, with default package limits, we don't yet have a DoS-resistant way of ensuring these transactions remain bumped throughout their time in the mempool. Primarily, the fee-bumping child may later be replaced by another transaction that doesn't bump the parent(s). The parent(s) could potentially stay bumped by other transactions, but not enough to ever be selected by the `BlockAssembler` (due to `blockmintxfee`).

  For example, (tested [here](https://github.com/glozow/bitcoin/commits/26933-motivation)):
  - The mempool accepts 24 below-minrelayfeerate transactions ("0-fee parents"), all bumped by a single high-fee transaction ("the fee-bumping child"). The fee-bumping child also spends a confirmed UTXO.
  - Two additional children are added to each 0-fee parent. These children each pay a feerate slightly above the minimum relay feerate (e.g. 1.9sat/vB) such that, for each 0-fee parent, the total fees of its two children divided by the total size of the children and parent is above the minimum relay feerate.
  - If a block template is built now, all transactions would be selected.
  - A transaction replaces the the fee-bumping child, spending only the confirmed UTXO and not any of the outputs from the 0-fee parents.
   - The 0-fee parents now each have 2 children. Their descendant feerates are above minrelayfeerate, which means that they remain in the mempool, even if the mempool evicts all below-minrelayfeerate packages.
   - If a block template is built now, none of the 0-fee parents or their children would be selected.
   - Even more low-feerate descendants can be added to these below-minrelayfeerate packages and they will not be evicted until they expire or the mempool reaches capacity.

  Unless we have a DoS-resistant way of ensuring package CPFP-bumped transactions are always bumped, allowing package CPFP to bump below-minrelayfeerate transactions can result in these problematic situations. See #27018 which proposes a partial solution with some limitations, and contains discussion about potential improvements to eviction strategy. While no adequate solution exists, for now, avoid these situations by requiring all transactions to meet min relay feerate.

ACKs for top commit:
  ajtowns:
    reACK bf77fc9
  instagibbs:
    re-ACK bf77fc9

Tree-SHA512: 28940f41493a9e280b010284316fb8caf1ed7b2090ba9a4ef8a3b2eafc5933601074b142f4f7d4e3c6c4cce99d3146f5c8e1393d9406c6f2070dd41c817985c9
@ariard
Copy link
Member

ariard commented May 1, 2023

If the state-of-art implementation of BIP331 can be linked here or in the BIP (Like there is glozow#8 and glozow#9) ?

From a reviewer perspective, ideally it’s better to have the proposed specification changes, the proposed Core code changes and the backend code of broadcaster client (e.g Lightning) to grasp a correct mental model of all the interactions.

@glozow
Copy link
Member Author

glozow commented Jun 2, 2023

If the state-of-art implementation of BIP331 can be linked here or in the BIP (Like there is glozow#8 and glozow#9) ?
From a reviewer perspective, ideally it’s better to have the proposed specification changes, the proposed Core code changes and the backend code of broadcaster client (e.g Lightning) to grasp a correct mental model of all the interactions.

Forgot to respond to this earlier - #27742 is the branch you're looking for :)

@AndriiHlukhovskyi
Copy link

Super

@ariard
Copy link
Member

ariard commented Jul 7, 2023

From a reviewing standpoint on #27742, I think it would benefit if the release aim for current package version (BIP331 + nversion=3 policy regime) is explicitly bounded to Lightning time-sensitive confirmation flows, especially in matters of DoS protection:

  • broadcast of revoked transaction output spend
  • broadcast of a commitment transaction with pending HTLC outputs and its associated second-stage transactions

I think this covers the most sensitive Lightning flows impacted by the pinning attacks mentioned as a design goal by BIP331. However this left of out considerations the following multi-party contract protocols and subsidiary Lightning flows:

  • submarine swaps e.g peerswap
  • LN dual-funding and splicing (package relay has been discussed as a griefing mitigation on the lightning dev mailing list iirc)
  • Taprooted wallet with time-sensitive script path

I think this is opportune to say so on the mailing list and therefore avoid multi-party applications or contracting protocols having to complaint than this deployment of package relay doesn’t fit their use-case (kind of like we have with the mempoolfullrbf debate). Beyond, I would say it’s valuable to announce latest BIP331 (minor) changes, if it does change something for the p2p network, that we would have missed from the Bitcoin Core-side.

@bitcoin bitcoin deleted a comment from mehran636311 Aug 17, 2023
@bitcoin bitcoin deleted a comment from AndriiHlukhovskyi Aug 17, 2023
achow101 added a commit that referenced this issue Aug 31, 2023
a3b55c9 [doc] move comment about AlreadyHaveTx DoS score to the right place (glozow)
3b8c178 [log] add more logs related to orphan handling (glozow)
51b3275 [log] add category TXPACKAGES for orphanage and package relay (glozow)
a33dde1 [log] include wtxid in tx {relay,validation,orphanage} logging (glozow)

Pull request description:

  This was taken from #28031 (see #27463 for project tracking).

  - Log wtxids in addition to txids when possible. This allows us to track the fate of a transaction from inv to mempool accept/reject through logs.
  - Additional orphan-related logging to make testing and debugging easier. Suggested in #28031 (review) and #28031 (comment)
  - Add `TXPACKAGES` category for logging.
  - Move a nearby comment block that was in the wrong place.

ACKs for top commit:
  instagibbs:
    reACK a3b55c9
  achow101:
    ACK a3b55c9
  brunoerg:
    crACK a3b55c9
  mzumsande:
    Code review ACK a3b55c9

Tree-SHA512: 21884ef7c2ea2fd006e715574a9dd3e6cbbe8f82d62c6187fe1d39aad5a834051203fda5f355a06ca40c3e2b9561aec50d7c922a662b1edc96f7b552c9f4b24d
@glozow
Copy link
Member Author

glozow commented Sep 20, 2023

Status update: I've put all the p2p PRs in draft to focus on getting the mempool/validation things done (I was struggling to context-switch and iterate quickly with the 5+ PRs). The v3 stuff also can't make progress until the AcceptPackage interface is more stable. After #26711 and more fuzzing, it also might be safe to open submitpackage on mainnet.

I'll continue working on the p2p code and write a lot more tests before opening them again. Please review the mempool stuff if you want to help with the project.

@tolex469
Copy link

Add new features

@achow101 achow101 unpinned this issue Oct 5, 2023
fanquake added a commit that referenced this issue Nov 3, 2023
b5a60ab MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a86 [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceb [refactor] move package checks into helper functions (glozow)

Pull request description:

  This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in #26711 (comment).

  - Split package sanitization in policy/packages.h into helper functions
    - Add some tests for its quirks (#26711 (comment))
  - Rename `CheckPackage` to `IsPackageWellFormed`
  - Improve the `CreateValidTransaction` unit test utility to:
    - Configure the target feerate and return the fee paid
    - Signal BIP125 on transactions to enable RBF tests
    - Allow the specification of multiple inputs and outputs
  - Move `CleanupTemporaryCoins` into its own function to be reused later without duplication

ACKs for top commit:
  dergoegge:
    Code review ACK b5a60ab
  instagibbs:
    ACK b5a60ab

Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
achow101 added a commit that referenced this issue Nov 3, 2023
d9cc99d [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a37 [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24 [MiniMiner] track inclusion order and add Linearize() function (glozow)
0040759 [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c [MiniMiner] make target_feerate optional (glozow)
5a83f55 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e63 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b7 [lint] update expected boost includes (glozow)

Pull request description:

  This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in #26711 (comment).

  - Allow using `MiniMiner` on transactions that aren't in the mempool.
  - Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
    - Add clarification for how this is different from `target_feerate=0` (#26711 (comment))
  - Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
  - Tests

  Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.

ACKs for top commit:
  TheCharlatan:
    ACK d9cc99d
  kevkevinpal:
    reACK [d9cc99d](d9cc99d)
  achow101:
    re-ACK d9cc99d

Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
fanquake added a commit that referenced this issue Nov 8, 2023
…ble fee failures and skipped transactions

1147e00 [validation] change package-fee-too-low, return wtxid(s) and effective feerate (glozow)
10dd9f2 [test] use CheckPackageMempoolAcceptResult in previous tests (glozow)
3979f1a [validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN (glozow)
5c786a0 [refactor] use Wtxid for m_wtxids_fee_calculations (glozow)

Pull request description:

  Split off from #26711 (suggested in #26711 (comment)). This is part of #27463.

  - Add 2 new TxValidationResults
    - `TX_RECONSIDERABLE` helps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished from `TX_MEMPOOL_POLICY` so that we re-validate a transaction if and only if it is eligible for package CPFP. In the future, we will have a separate cache for reconsiderable rejects so these transactions don't go in `m_recent_rejects`.
    - `TX_UNKNOWN` helps us communicate that we aborted package validation and didn't finish looking at this transaction: it's not valid but it's also not invalid (i.e. don't cache it as a rejected tx)
  - Return effective feerate and the wtxids of transactions used to calculate that effective feerate when the error is `TX_SINGLE_FAILURE`. Previously, we would only provide this information if the transaction passed. Now that we have package validation, it's much more helpful to the caller to know how the failing feerate was calculated. This can also be used to improve our submitpackage RPC result (which is currently a bit unhelpful when things fail).
  - Use the newly added `CheckPackageMempoolAcceptResult` for existing package validation tests. This increases test coverage and helps test the changes made in this PR.

ACKs for top commit:
  instagibbs:
    reACK 1147e00
  achow101:
    ACK 1147e00
  murchandamus:
    reACK 1147e00
  ismaelsadeeq:
    ACK 1147e00

Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
@achow101
Copy link
Member

#26711 was closed. Could we get a longer explanation of why and what the plan is going forward? My understanding is that the plan is to do a limited package relay carveout for just CPFP, then cluster mempool, then the rest of package relay?

@instagibbs
Copy link
Member

I'll update Ephemeral Anchors PR/BIP once current set of PRs are merged, since I build on #28848 for testing

@glozow
Copy link
Member Author

glozow commented Nov 14, 2023

There have been discussions on what package validation and RBF rules will look like in a post-cluster mempool world. #26711 either doesn't work with cluster mempool or should not be considered until after cluster mempool. #26711 supports certain types of package submission that we might not be able to continue to support with cluster mempool depending on what approach is taken. There are different ideas on how to consider subsets of packages to accept when it doesn't make sense to take all or none. IIUC the approach taken in #26711 (which is to do subpackage evaluation) no longer has conceptual support which is why I closed it.

Another issue is a circular dependency between cluster mempool and package relay. (1) We want cluster mempool before package RBF since it allows us to assess incentive compatibility much more easily. (2) Cluster mempool requires the removal of CPFP carve out in order to switch from ancestor/descendant-based package limits to cluster limits. But there are people who rely on CPFP carve out for bumping presigned transactions; we need to build a replacement (i.e. package relay + package RBF).

So the new plan of attack is to first create 1-parent-1-child package relay, including package RBF for v3 transactions. This allows the users of CPFP carve out to switch, thus allowing us to remove CPFP carve out and then add cluster mempool. This also allows us to defer the decision-making on what package evaluation and package RBF will look like in a post-cluster mempool world.

achow101 added a commit that referenced this issue Feb 10, 2024
29029df [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea7 [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5 [functional test] v3 transaction submission (glozow)
27c8786 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea5 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2 [policy] add v3 policy rules (glozow)
9a29d47 [rpc] return full string for package_msg and package-error (glozow)
158623b [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)

Pull request description:

  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 #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 #27677 and #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] (#27677 #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

ACKs for top commit:
  sdaftuar:
    ACK 29029df
  achow101:
    ACK 29029df
  instagibbs:
    ACK 29029df modulo that

Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
achow101 added a commit that referenced this issue Apr 30, 2024
e518a8b [functional test] opportunistic 1p1c package submission (glozow)
87c5c52 [p2p] opportunistically accept 1-parent-1-child packages (glozow)
6c51e1d [p2p] add separate rejections cache for reconsiderable txns (glozow)
410ebd6 [fuzz] break out parent functions and add GetChildrenFrom* coverage (glozow)
d095316 [unit test] TxOrphanage::GetChildrenFrom* (glozow)
2f51cd6 [txorphanage] add method to get all orphans spending a tx (glozow)
092c978 [txpackages] add canonical way to get hash of package (glozow)
c3c1e15 [doc] restore comment about why we check if ptx HasWitness before caching rejected txid (glozow)
6f4da19 guard against MempoolAcceptResult::m_replaced_transactions (glozow)

Pull request description:

  This enables 1p1c packages to propagate in the "happy case" (i.e. not reliable if there are adversaries) and contains a lot of package relay-related code. See #27463 for overall package relay tracking.

  Rationale: This is "non-robust 1-parent-1-child package relay" which is immediately useful.
  - Relaying 1-parent-1-child CPFP when mempool min feerate is high would be a subset of all package relay use cases, but a pretty significant improvement over what we have today, where such transactions don't propagate at all. [1]
  - Today, a miner can run this with a normal/small maxmempool to get revenue from 1p1c CPFP'd transactions without losing out on the ones with parents below mempool minimum feerate.
  - The majority of this code is useful for building more featureful/robust package relay e.g. see the code in #27742.

  The first 2 commits are followups from #29619:
  - #29619 (comment)
  - #29619 (comment)

  Q: What makes this short of a more full package relay feature?

  (1) it only supports packages in which 1 of the parents needs to be CPFP'd by the child. That includes 1-parent-1-child packages and situations in which the other parents already pay for themselves (and are thus in mempool already when the package is submitted). More general package relay is a future improvement that requires more engineering in mempool and validation - see #27463.

  (2) We rely on having kept the child in orphanage, and don't make any attempt to protect it while we wait to receive the parent. If we are experiencing a lot of orphanage churn (e.g. an adversary is purposefully sending us a lot of transactions with missing inputs), we will fail to submit packages. This limitation has been around for 12+ years, see #27742 which adds a token bucket scheme for protecting package-related orphans at a limited rate per peer.

  (3) Our orphan-handling logic is somewhat opportunistic; we don't make much effort to resolve an orphan beyond asking the child's sender for the parents. This means we may miss packages if the first sender fails to give us the parent (intentionally or unintentionally). To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. This is also an existing problem which has a proposed solution in #28031.

  [1]: see this writeup and its links https://github.com/bitcoin/bips/blob/02ec218c7857ef60914e9a3d383b68caf987f70b/bip-0331.mediawiki#propagate-high-feerate-transactions

ACKs for top commit:
  sr-gi:
    tACK e518a8b
  instagibbs:
    reACK e518a8b
  theStack:
    Code-review ACK e518a8b 📦
  dergoegge:
    light Code review ACK e518a8b
  achow101:
    ACK e518a8b

Tree-SHA512: 632579fbe7160cb763bbec6d82ca0dab484d5dbbc7aea90c187c0b9833b8d7c1e5d13b8587379edd3a3b4a02a5a1809020369e9cd09a4ebaf729921f65c15943
ryanofsky added a commit that referenced this issue May 15, 2024
…e txid

0fb17bf [log] updates in TxOrphanage (glozow)
b16da7e [functional test] attackers sending mutated orphans (glozow)
6675f64 [unit test] TxOrphanage handling of same-txid-different-witness txns (glozow)
8923edf [p2p] allow entries with the same txid in TxOrphanage (glozow)
c31f148 [refactor] TxOrphanage::EraseTx by wtxid (glozow)
efcc593 [refactor] TxOrphanage::HaveTx only by wtxid (glozow)
7e475b9 [p2p] don't query orphanage by txid (glozow)

Pull request description:

  Part of #27463 in the "make orphan handling more robust" section.

  Currently the main map in `TxOrphanage` is indexed by txid; we do not allow 2 transactions with the same txid into TxOrphanage. This means that if we receive a transaction and want to store it in orphanage, we'll fail to do so if a same-txid-different-witness version of the tx already exists in the orphanage. The existing orphanage entry can stay until it expires 20 minutes later, or until we find that it is invalid.

  This means an attacker can try to block/delay us accepting an orphan transaction by sending a mutated version of the child ahead of time. See included test.

  Prior to #28970, we don't rely on the orphanage for anything and it would be relatively difficult to guess what transaction will go to a node's orphanage. After the parent(s) are accepted, if anybody sends us the correct transaction, we'll end up accepting it. However, this is a bit more painful for 1p1c: it's easier for an attacker to tell when a tx is going to hit a node's orphanage, and we need to store the correct orphan + receive the parent before we'll consider the package. If we start out with a bad orphan, we can't evict it until we receive the parent + try the 1p1c, and then we'll need to download the real child, put it in orphanage, download the parent again, and then retry 1p1c.

ACKs for top commit:
  AngusP:
    ACK 0fb17bf
  itornaza:
    trACK 0fb17bf
  instagibbs:
    ACK 0fb17bf
  theStack:
    ACK 0fb17bf
  sr-gi:
    crACK [0fb17bf](0fb17bf)
  stickies-v:
    ACK 0fb17bf

Tree-SHA512: edcbac7287c628bc27036920c2d4e4f63ec65087fbac1de9319c4f541515d669fc4e5fdc30c8b9a248b720da42b89153d388e91c7bf5caf4bc5b3b931ded1f59
achow101 added a commit that referenced this issue Jun 7, 2024
30a0113 [doc] update bips.md for 431 (glozow)
9dbe6a0 [test] wallet uses CURRENT_VERSION which is 2 (glozow)
539404f [policy] make v3 transactions standard (glozow)
052ede7 [refactor] use TRUC_VERSION in place of 3 (glozow)

Pull request description:

  Make `nVersion=3` (which is currently nonstandard on mainnet) standard.

  Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873

  See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.

ACKs for top commit:
  sdaftuar:
    utACK 30a0113
  achow101:
    ACK 30a0113
  instagibbs:
    utACK 30a0113
  murchandamus:
    ACK 30a0113
  ismaelsadeeq:
    ACK 30a0113 🛰️

Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Priority Projects
Development

No branches or pull requests

10 participants