Package relay (p2p)
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
mempoolMinFee, and “mempool min fee”
- Look at
CheckFeeRatein src/validation.cpp::517 (that was added by the ATMP refactoring PR)
- Class member
bool m_cpfpablerefers to the state of being CPFP-able, e.g. eligible for Child Pays For Parent
Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)
What steps did you take, beyond reading the code and running the tests?
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?
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.”
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!