Package relay (p2p)

Host: jonatack  -  PR author: sdaftuar


Today’s PR is a Draft WIP implementation of the “package relay” concept first introduced in December 2018 with issue #14895 “Package relay design questions”, proposed by Suhas Daftuar who has been working to improve the privacy and resilience of Bitcoin’s peer-to-peer network.

There is also a BIP draft: Transaction Package Relay.

In the PR description, Suhas described today’s PR as a proof-of-concept motivation for refactoring AcceptToMemoryPoolWorker (ATMP), which he did in PR #16400 “Rewrite AcceptToMemoryPoolWorker() using smaller parts” that was merged two weeks ago and which lays a foundation for multiple transactions for package relay.

Today’s PR was opened at the same time as the ATMP refactoring. It is a stand-alone change, requiring no p2p protocol changes – including those made by the ATMP refactoring.

Suhas’ motivation for it begins: Accepting a single transaction to the mempool only succeeds if (among other things) the feerate of the transaction is greater than both the min relay fee and the mempool min fee. Consequently, a transaction below the minimum fee may not be accepted to the mempool, even if we later learn of a transaction with a high fee that depends on it.

  • You can find the minimum relay fee and mempool minimum fee in the codebase by git grepping for CheckFeeRate, relayMinFee, DEFAULT_MIN_RELAY_TX_FEE, mempoolMinFee, and “mempool min fee”
  • Look at CheckFeeRate in src/validation.cpp::517 (that was added by the ATMP refactoring PR)
  • Class member bool m_cpfpable refers to the state of being CPFP-able, e.g. eligible for Child Pays For Parent


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)

  2. What steps did you take, beyond reading the code and running the tests?

  3. Did you review the functional test, or address the author’s TODO in it?

Other questions you may want to consider:

  • Who is responsible for initiating package relay, sender or recipient?

  • Should we update the p2p protocol to accommodate package relay, or rely on existing functionality?

  • What Denial-of-Service concerns do we need to address?

  • One of the conditions in the PR is that “No transactions in the mempool conflict with any transactions in the package.” What is meant here by conflict, and for what reasons could a parent transaction evict a transaction from the mempool that another child depends on?

  • From the PR: “The ancestor/descendant size limits are calculated assuming that any mempool ancestor of any candidate transaction is an ancestor of all the candidate transactions.” Do you agree that this assumption can be made?

Going further

A related PR opened recently by Anthony Towns (ajtowns) that you may wish to review: PR #16851 “Continue relaying transactions after they expire from mapRelay.”

Meeting Log

  119:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club.
  219:00 <jonatack> We usually start Bitcoin Core IRC meetings with a 'hi' so it's clear who's at keyboard. Feel free to say hi!
  319:01 <jkczyz> hi
  419:01 <sosthene> hi
  519:01 <jonatack> This week, we're talking about PR #16401 "Package relay" by sdaftuar
  619:01 <lightlike> hi
  719:01 <pinheadmz> hi
  819:02 <jonatack> which is part of his ongoing work to improve the privacy and resilience of Bitcoin's peer-to-peer network.
  919:02 <fjahr> hi
 1019:02 <jonatack> sdaftuar: (Would you like to say anything about this PR or the next steps? Feel free to jump in.)
 1119:02 <zenogais> hi
 1219:03 <jonatack> As summarised by Dave Harding in Bitcoin Optech newsletter #65 at
 1319:03 <jonatack> "Package relay could allow nodes to accept a transaction below the node’s minimum feerate if the transaction came bundled with a child transaction whose fee was high enough to pay the minimum feerate for both it and its parent.
 1419:03 <jonatack> If widely deployed, package relay would allow users who create transactions a
 1519:03 <jonatack> long time before broadcasting them (e.g. timelocked transactions or LN
 1619:03 <jonatack> commitment transactions) to safely pay the minimum possible fee.
 1719:03 <jonatack> When it came time to broadcast the transaction, they could use Child-Pays-For-Parent (CPFP) fee bumping to set an appropriate fee for the current network conditions."
 1819:04 <jonatack> A resource that I will add to the notes:
 1919:04 <jonatack> Suhas began a BIP draft entitled "Transaction Package Relay" here:
 2019:04 <jonatack>
 2119:04 <jonatack> Much to unpack this week.
 2219:04 <jonatack> A good strategy for situations like this is to break it down to smaller useful steps, while keeping the larger vision in mind.
 2319:05 <jonatack> Who reviewed the PR?
 2419:06 <pinheadmz> i did the best i could :-) mempool stuff is the most complicated!
 2519:06 <pinheadmz> but i read the bip as well and the test is very easy to follow
 2619:06 <kanzure> hi
 2719:06 <zenogais> I did, it was definitely a more difficult one.
 2819:06 <fjahr> I did and I enjoyed it although I needed to through it multiple times
 2919:07 <sosthene> I didn't have much time today, I'm building it and hopefully will run the test before the end of session
 3019:08 <jonatack> TBH I spent the afternoon reviewing 16400 and 16401 and am not nearly done. There's a lot to look at and the ATMP refactoring in 16400 pretty much has to be reviewed before this PR.
 3119:08 <zenogais> The BIP draft here provides a lot of userful higher-level context on this PR.
 3219:09 <jonatack> Question: How to have your running bitcoind output the net logging?
 3319:09 <pinheadmz> -debug net
 3419:09 <jonatack> Hint: see bitcoin-cli help logging
 3519:10 <fjahr> A meta comment for newcomers: a pattern of commits in Bitcoin PRs that is very clear here and that I have seen several times now is: first lower level stuff, then higher level/usage of the lower level, then tests. This happens almost automatically when commits are well structured but each one should not have failing tests on it's own. As a results when you see this is makes sense to review the commits
 3619:10 <fjahr> backwards.
 3719:11 <fjahr> Anyone feel free to correct me if you disagree with this, may be just an anecdotal observation ;)
 3819:11 <pinheadmz> oh yeah i forgot theres a command to enable while already running: bitcoin-cli logging debug
 3919:11 <pinheadmz> sorry logging (["net"])
 4019:11 <jonatack> pinheadmz: yes, when bitcoind is already running
 4119:12 <jonatack> pinheadmz: right! bitcoin-cli logging '["net"]'
 4219:12 <jonatack> fjahr: Interesting thought!
 4319:14 <jonatack> Did anyone attempt to fix the test TODO?
 4419:14 <zenogais> Yes
 4519:14 <zenogais> I also added another test around 3-tx packages:
 4619:14 <zenogais> fixed the TODO using assert_raises_rpc_error
 4719:15 <jonatack> zenogais: Nice! Can you share a gist?
 4819:15 <zenogais> Posted above I think?
 4919:16 <zenogais>
 5019:16 <jonatack> zenogais: thank you.
 5119:16 <pinheadmz> quick Q about the test framework: is there always a BaseNode? and then num_nodes adds two additional nodes?
 5219:17 <jonatack> Proposing additional tests can be a good way to help a PR author.
 5319:17 <fjahr> I was not sure how much I should do with/comment on the tests since they were half-baked (still had comments from the example test) so stuck to the code. Also first time I have reviewed a draft PR tbh
 5419:17 <fjahr> jonatack: true
 5519:17 <jonatack> It's generally a well-appreciated contribution.
 5619:18 <jonatack> fjahr: Good point about the Draft status.
 5719:18 <jonatack> WIP and draft PRs are likely mainly looking for Concept and approach ACKs.
 5819:20 <jonatack> pinheadmz: In the test setup you can specify the number of nodes.
 5919:20 <lightlike> pinheadmz: not always: "BaseNode" is just a name; these derived classes are used when you need some extra functionality (such as extra logging etc.) beyond the basic P2PInterface class.
 6019:20 <jonatack> pinheadmz: Have a look at test/functional/
 6119:20 <jonatack> pinheadmz: It's a functioning tutorial test
 6219:22 <pinheadmz> I sorta dont understand why we need one more add_p2p_connection on line 102 -- shoudlnt the connection already be established on line 67 ?
 6319:22 <jonatack> pinheadmz: lightlike: Yes. It depends on the includes at the top of the test fgile.
 6419:23 <zenogais> The second add_p2p_connection is there for its return value.
 6519:23 <zenogais> Which is used to invoke send_and_ping
 6619:24 <pinheadmz> ok thats what i thought - it doesnt disconenect/ reconnect
 6719:24 <pinheadmz> so that p2p = could be up on line 67
 6819:24 <zenogais> I believe so, let me try it
 6919:24 <zenogais> yep, that works
 7019:25 <pinheadmz> confirmed as well
 7119:25 <pinheadmz> hey maybe ill comment!
 7219:26 <jonatack> Question: Who ought to be responsible for initiating package relay, the sender or the recipient?
 7319:26 <zenogais> I believe it's the sender, since they have to specify the package to relay.
 7419:27 <pinheadmz> the recipient first must signal they allow package messages
 7519:27 <pinheadmz> `sendpackages`
 7619:28 <pinheadmz> like how `sendheaders` is used to initialize headers-first sync from a peer
 7719:28 <jonatack> IIUC, it's a trade-off
 7819:28 <jonatack> The sender is well-positioned to know if package relay is needed
 7919:28 <jonatack> but a naive version could be bandwidth wasteful, and likely need p2p changes
 8019:29 <jonatack> The recipient can tell if a transaction's parents are missing
 8119:29 <jonatack> and request package relay from the sending peer
 8219:29 <jonatack> so package relay would only occur as needed
 8319:30 <jonatack> eg to/from low-memory-mempool nodes
 8419:30 <lightlike> in the current draft implementation, it seems that the recipient figures out when it can use package relay by themselves.
 8519:31 <jonatack> lightlike: Yes. I think recipient-initiated is the preferred approach.
 8619:32 <jonatack> Question: Should we update the p2p protocol to accommodate package relay, or rely on existing functionality?
 8719:32 <zenogais> Makes sense
 8819:32 <jonatack> (eg what are the trade-offs)
 8919:34 <jonatack> zenogais: IIUC, recipient-initiated adds the step of requesting the sender return the list, but saves the need for the sender to do broadcast it all the time
 9019:35 <pinheadmz> i thought it was interesting that the author added new p2p messages - at first i just expected it to be an expanded CPFP policy
 9119:36 <jonatack> On one hand, not updating the protocol means ease of upgrading the network.
 9219:37 <zenogais> Not chaning p2p also means decreased attack surface, especially since some of this verification logic is expensive.
 9319:37 <zenogais> jonatack: Yeah, I hadn't fully grasped these packages were being received, but reading through the BIP draft it makes more sense now.
 9419:38 <jonatack> zenogais: true. And more code and features can mean more attack surface.
 9519:38 <lightlike> there are no new p2p messages in the draft implementation though. What was changed there is only the behaviour if we receive a TX message with a too-low fee (searching for a fitting orphan and trying to build a 2-tx package)
 9619:38 <zenogais> ^ Yeah, which is why this looks sender initiated in the Draft PR
 9719:39 <jonatack> lightlike: Right. PR 16401 is proof of concept written before 16400 (ATMP refactoring) was merged.
 9819:39 <zenogais> One other trade-off here is that the functionality is a bit redundant with RBF
 9919:40 <fjahr> I think it is a worthwhile improvement without new messages but with new messages we get much higher efficiency
10019:41 <jonatack> From what I've understood, the advantage of updating the p2p protocol would be potentially optimising network efficiency and computational complexity
10119:41 <jonatack> fjahr: right
10219:41 <zenogais> Yeah, otherwise I think there would be a lot more logic around detecting potential relay packages
10319:41 <zenogais> With P2P message it's explicit
10419:42 <jonatack> Yes. My money would be on updating. It's too tempting not to try :D
10519:43 <zenogais> On the plus side, this also opens up some neat possibilities as pointed out in OpTech summary
10619:44 <zenogais> Increases possibilities for reducing fee-rates
10719:45 <jonatack> What about DoS vectors and the additional logic added with AcceptMultipleTransactions?
10819:46 <zenogais> From the comment: "We will end up needing to recalculate setAncestors for each transaction prior to calling Finalize, but we should do the correct package-size calculations before we call ScriptChecks(), to avoid CPU-DoS."
10919:46 <jonatack> Seems like more checks are involved. PreChecks maybe twice? etc.
11019:46 <unstaffed-skylig> DoS was something I was trying see if it had been discussed on GH. seems like resource exhaustion potential is much higher
11119:47 <unstaffed-skylig> zenogais where is the comment "We will end up needing..."
11219:48 <zenogais> DoS potential is a little worrying without there being some limit on package size.
11319:48 <zenogais> src/validation.cpp Line 1107
11419:48 <jonatack> Yes. For the complexity reason alone, upgrading/refactoring to simplify/make efficient looks necessary.
11519:49 <jonatack> Also: 1) We would want the ability to perform policy checks on the whole package *before* signature validation to avoid CPU-exhaustion attacks
11619:49 <jonatack> and (2) we need to be attentive of order-dependencies on mempool acceptance of (a) transaction relay and (b) transaction arrival to the node
11719:50 <jonatack> Lots to do!
11819:51 <zenogais> Yeah, would love to see the package policy validation in a single method so it could be tested against multiple types of packages. The logic here seems particularly tricky.
11919:51 <jonatack> unstaffed-skylig: zenogais: yes
12019:51 <jonatack> zenogais: I agree.
12119:52 <jonatack> And yet we're talking about arguably the most critical sections of the codebase
12219:54 <zenogais> Is there chain-split potential with this change as well?
12319:54 <jonatack> Will need eyes and much more review... experienced reviewers. Why this club exists.
12419:55 <jonatack> 5 minutes. Let's wrap up.
12519:55 <unstaffed-skylig> zenogais: chain split beyond DoS'ing a mining node to take out effective hash power? That's the only scenario I can invision
12619:55 <lightlike> i am not completely convinced yet if the use cases for package relay justify a non-minimal version of it with large and more complicated packages.
12719:56 <jonatack> Any PR suggestions for next week?
12819:57 <zenogais> Looking through PRs right now
12919:57 <jonatack> lightlike: A systematic framework for simulating/testing p2p might be valuable for assessing this.
13019:58 <lightlike> i think #16981 will be really interesting, but it's still WIP.
13119:59 <jonatack> From what I understand, people like Giula Fanti, Suhas, and Gleb (among others would be interested in any contribution to such a framework.
13220:00 <jonatack> Please add your PR suggestions and volunteer for hosting meetings on the club repository!
13320:01 <zenogais> Will do, thanks jonatack
13420:01 <jonatack> The link is:
13520:01 <jonatack> Thanks, all!
13620:02 <pinheadmz> thanks jonatack !
13720:02 <fjahr> thanks jon!
13820:02 <jonatack> Thanks to zenogais, fjahr, and pinheadmz for reviewing the PR!
13920:02 <lightlike> thanks!
14020:04 <jonatack> zenogais: thanks for adding tests!
14120:05 <zenogais> Of course!