#16401 Package relay (p2p)

https://github.com/bitcoin/bitcoin/16401

Host: jonatack

Notes

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

Questions

  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

19:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club.
19: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!
19:01 <jkczyz> hi
19:01 <sosthene> hi
19:01 <jonatack> This week, we're talking about PR #16401 "Package relay" by sdaftuar
19:01 <lightlike> hi
19:01 <pinheadmz> hi
19:02 <jonatack> which is part of his ongoing work to improve the privacy and resilience of Bitcoin's peer-to-peer network.
19:02 <fjahr> hi
19:02 <jonatack> sdaftuar: (Would you like to say anything about this PR or the next steps? Feel free to jump in.)
19:02 <zenogais> hi
19:03 <jonatack> As summarised by Dave Harding in Bitcoin Optech newsletter #65 at https://bitcoinops.org/en/newsletters/2019/09/25/
19: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.
19:03 <jonatack> If widely deployed, package relay would allow users who create transactions a
19:03 <jonatack> long time before broadcasting them (e.g. timelocked transactions or LN
19:03 <jonatack> commitment transactions) to safely pay the minimum possible fee.
19: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."
19:04 <jonatack> A resource that I will add to the notes:
19:04 <jonatack> Suhas began a BIP draft entitled "Transaction Package Relay" here:
19:04 <jonatack> https://gist.github.com/sdaftuar/8756699bfcad4d3806ba9f3396d4e66a
19:04 <jonatack> Much to unpack this week.
19: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.
19:05 <jonatack> Who reviewed the PR?
19:06 <pinheadmz> i did the best i could :-) mempool stuff is the most complicated!
19:06 <pinheadmz> but i read the bip as well and the test is very easy to follow
19:06 <kanzure> hi
19:06 <zenogais> I did, it was definitely a more difficult one.
19:06 <fjahr> I did and I enjoyed it although I needed to through it multiple times
19:07 <sosthene> I didn't have much time today, I'm building it and hopefully will run the test before the end of session
19: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.
19:08 <zenogais> The BIP draft here provides a lot of userful higher-level context on this PR.
19:09 <jonatack> Question: How to have your running bitcoind output the net logging?
19:09 <pinheadmz> -debug net
19:09 <jonatack> Hint: see bitcoin-cli help logging
19: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
19:10 <fjahr> backwards.
19:11 <fjahr> Anyone feel free to correct me if you disagree with this, may be just an anecdotal observation ;)
19:11 <pinheadmz> oh yeah i forgot theres a command to enable while already running: bitcoin-cli logging debug
19:11 <pinheadmz> sorry logging (["net"])
19:11 <jonatack> pinheadmz: yes, when bitcoind is already running
19:12 <jonatack> pinheadmz: right! bitcoin-cli logging '["net"]'
19:12 <jonatack> fjahr: Interesting thought!
19:14 <jonatack> Did anyone attempt to fix the test TODO?
19:14 <zenogais> Yes
19:14 <zenogais> I also added another test around 3-tx packages: https://gist.github.com/etscrivner/19d5f942a973940aaaeb397bc5e0e0d9
19:14 <zenogais> fixed the TODO using assert_raises_rpc_error
19:15 <jonatack> zenogais: Nice! Can you share a gist?
19:15 <zenogais> Posted above I think?
19:16 <zenogais> https://gist.github.com/etscrivner/19d5f942a973940aaaeb397bc5e0e0d9
19:16 <jonatack> zenogais: thank you.
19:16 <pinheadmz> quick Q about the test framework: is there always a BaseNode? and then num_nodes adds two additional nodes?
19:17 <jonatack> Proposing additional tests can be a good way to help a PR author.
19: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
19:17 <fjahr> jonatack: true
19:17 <jonatack> It's generally a well-appreciated contribution.
19:18 <jonatack> fjahr: Good point about the Draft status.
19:18 <jonatack> WIP and draft PRs are likely mainly looking for Concept and approach ACKs.
19:20 <jonatack> pinheadmz: In the test setup you can specify the number of nodes.
19: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.
19:20 <jonatack> pinheadmz: Have a look at test/functional/example_test.py
19:20 <jonatack> pinheadmz: It's a functioning tutorial test
19: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 ?
19:22 <jonatack> pinheadmz: lightlike: Yes. It depends on the includes at the top of the test fgile.
19:23 <zenogais> The second add_p2p_connection is there for its return value.
19:23 <zenogais> Which is used to invoke send_and_ping
19:24 <pinheadmz> ok thats what i thought - it doesnt disconenect/ reconnect
19:24 <pinheadmz> so that p2p = could be up on line 67
19:24 <zenogais> I believe so, let me try it
19:24 <zenogais> yep, that works
19:25 <pinheadmz> confirmed as well
19:25 <pinheadmz> hey maybe ill comment!
19:26 <jonatack> Question: Who ought to be responsible for initiating package relay, the sender or the recipient?
19:26 <zenogais> I believe it's the sender, since they have to specify the package to relay.
19:27 <pinheadmz> the recipient first must signal they allow package messages
19:27 <pinheadmz> `sendpackages`
19:28 <pinheadmz> like how `sendheaders` is used to initialize headers-first sync from a peer
19:28 <jonatack> IIUC, it's a trade-off
19:28 <jonatack> The sender is well-positioned to know if package relay is needed
19:28 <jonatack> but a naive version could be bandwidth wasteful, and likely need p2p changes
19:29 <jonatack> The recipient can tell if a transaction's parents are missing
19:29 <jonatack> and request package relay from the sending peer
19:29 <jonatack> so package relay would only occur as needed
19:30 <jonatack> eg to/from low-memory-mempool nodes
19:30 <lightlike> in the current draft implementation, it seems that the recipient figures out when it can use package relay by themselves.
19:31 <jonatack> lightlike: Yes. I think recipient-initiated is the preferred approach.
19:32 <jonatack> Question: Should we update the p2p protocol to accommodate package relay, or rely on existing functionality?
19:32 <zenogais> Makes sense
19:32 <jonatack> (eg what are the trade-offs)
19: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
19: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
19:36 <jonatack> On one hand, not updating the protocol means ease of upgrading the network.
19:37 <zenogais> Not chaning p2p also means decreased attack surface, especially since some of this verification logic is expensive.
19: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.
19:38 <jonatack> zenogais: true. And more code and features can mean more attack surface.
19: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)
19:38 <zenogais> ^ Yeah, which is why this looks sender initiated in the Draft PR
19:39 <jonatack> lightlike: Right. PR 16401 is proof of concept written before 16400 (ATMP refactoring) was merged.
19:39 <zenogais> One other trade-off here is that the functionality is a bit redundant with RBF
19:40 <fjahr> I think it is a worthwhile improvement without new messages but with new messages we get much higher efficiency
19:41 <jonatack> From what I've understood, the advantage of updating the p2p protocol would be potentially optimising network efficiency and computational complexity
19:41 <jonatack> fjahr: right
19:41 <zenogais> Yeah, otherwise I think there would be a lot more logic around detecting potential relay packages
19:41 <zenogais> With P2P message it's explicit
19:42 <jonatack> Yes. My money would be on updating. It's too tempting not to try :D
19:43 <zenogais> On the plus side, this also opens up some neat possibilities as pointed out in OpTech summary
19:44 <zenogais> Increases possibilities for reducing fee-rates
19:45 <jonatack> What about DoS vectors and the additional logic added with AcceptMultipleTransactions?
19: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."
19:46 <jonatack> Seems like more checks are involved. PreChecks maybe twice? etc.
19: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
19:47 <unstaffed-skylig> zenogais where is the comment "We will end up needing..."
19:48 <zenogais> DoS potential is a little worrying without there being some limit on package size.
19:48 <zenogais> src/validation.cpp Line 1107
19:48 <jonatack> Yes. For the complexity reason alone, upgrading/refactoring to simplify/make efficient looks necessary.
19: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
19: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
19:50 <jonatack> Lots to do!
19: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.
19:51 <jonatack> unstaffed-skylig: zenogais: yes
19:51 <jonatack> zenogais: I agree.
19:52 <jonatack> And yet we're talking about arguably the most critical sections of the codebase
19:54 <zenogais> Is there chain-split potential with this change as well?
19:54 <jonatack> Will need eyes and much more review... experienced reviewers. Why this club exists.
19:55 <jonatack> 5 minutes. Let's wrap up.
19: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
19: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.
19:56 <jonatack> Any PR suggestions for next week?
19:57 <zenogais> Looking through PRs right now
19:57 <jonatack> lightlike: A systematic framework for simulating/testing p2p might be valuable for assessing this.
19:58 <lightlike> i think #16981 will be really interesting, but it's still WIP.
19: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.
20:00 <jonatack> Please add your PR suggestions and volunteer for hosting meetings on the club repository!
20:01 <zenogais> Will do, thanks jonatack
20:01 <jonatack> The link is: https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14
20:01 <jonatack> Thanks, all!
20:02 <pinheadmz> thanks jonatack !
20:02 <fjahr> thanks jon!
20:02 <jonatack> Thanks to zenogais, fjahr, and pinheadmz for reviewing the PR!
20:02 <lightlike> thanks!
20:04 <jonatack> zenogais: thanks for adding tests!
20:05 <zenogais> Of course!