Package relay (
p2p) Oct 2, 2019
Today’s PR is a Draft WIP implementation of the “package relay” concept first
introduced in December 2018 with
#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:
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”
CheckFeeRate in src/validation.cpp::517 (that was added by the ATMP
bool m_cpfpable refers 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
(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
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
PR #16851 “Continue
relaying transactions after they expire from mapRelay.” Meeting Log
1 19:00 <jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club.
2 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!
5 19:01 <jonatack> This week, we're talking about PR #16401 "Package relay" by sdaftuar
8 19:02 <jonatack> which is part of his ongoing work to improve the privacy and resilience of Bitcoin's peer-to-peer network.
10 19:02 <jonatack> sdaftuar: (Would you like to say anything about this PR or the next steps? Feel free to jump in.)
13 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.
14 19:03 <jonatack> If widely deployed, package relay would allow users who create transactions a
15 19:03 <jonatack> long time before broadcasting them (e.g. timelocked transactions or LN
16 19:03 <jonatack> commitment transactions) to safely pay the minimum possible fee.
17 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."
18 19:04 <jonatack> A resource that I will add to the notes:
19 19:04 <jonatack> Suhas began a BIP draft entitled "Transaction Package Relay" here:
21 19:04 <jonatack> Much to unpack this week.
22 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.
23 19:05 <jonatack> Who reviewed the PR?
24 19:06 <pinheadmz> i did the best i could :-) mempool stuff is the most complicated!
25 19:06 <pinheadmz> but i read the bip as well and the test is very easy to follow
27 19:06 <zenogais> I did, it was definitely a more difficult one.
28 19:06 <fjahr> I did and I enjoyed it although I needed to through it multiple times
29 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
30 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.
31 19:08 <zenogais> The BIP draft here provides a lot of userful higher-level context on this PR.
32 19:09 <jonatack> Question: How to have your running bitcoind output the net logging?
33 19:09 <pinheadmz> -debug net
34 19:09 <jonatack> Hint: see bitcoin-cli help logging
35 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
36 19:10 <fjahr> backwards.
37 19:11 <fjahr> Anyone feel free to correct me if you disagree with this, may be just an anecdotal observation ;)
38 19:11 <pinheadmz> oh yeah i forgot theres a command to enable while already running: bitcoin-cli logging debug
39 19:11 <pinheadmz> sorry logging (["net"])
40 19:11 <jonatack> pinheadmz: yes, when bitcoind is already running
41 19:12 <jonatack> pinheadmz: right! bitcoin-cli logging '["net"]'
42 19:12 <jonatack> fjahr: Interesting thought!
43 19:14 <jonatack> Did anyone attempt to fix the test TODO?
46 19:14 <zenogais> fixed the TODO using assert_raises_rpc_error
47 19:15 <jonatack> zenogais: Nice! Can you share a gist?
48 19:15 <zenogais> Posted above I think?
50 19:16 <jonatack> zenogais: thank you.
51 19:16 <pinheadmz> quick Q about the test framework: is there always a BaseNode? and then num_nodes adds two additional nodes?
52 19:17 <jonatack> Proposing additional tests can be a good way to help a PR author.
53 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
54 19:17 <fjahr> jonatack: true
55 19:17 <jonatack> It's generally a well-appreciated contribution.
56 19:18 <jonatack> fjahr: Good point about the Draft status.
57 19:18 <jonatack> WIP and draft PRs are likely mainly looking for Concept and approach ACKs.
58 19:20 <jonatack> pinheadmz: In the test setup you can specify the number of nodes.
59 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.
60 19:20 <jonatack> pinheadmz: Have a look at test/functional/example_test.py
61 19:20 <jonatack> pinheadmz: It's a functioning tutorial test
62 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 ?
63 19:22 <jonatack> pinheadmz: lightlike: Yes. It depends on the includes at the top of the test fgile.
64 19:23 <zenogais> The second add_p2p_connection is there for its return value.
65 19:23 <zenogais> Which is used to invoke send_and_ping
66 19:24 <pinheadmz> ok thats what i thought - it doesnt disconenect/ reconnect
67 19:24 <pinheadmz> so that p2p = could be up on line 67
68 19:24 <zenogais> I believe so, let me try it
69 19:24 <zenogais> yep, that works
70 19:25 <pinheadmz> confirmed as well
71 19:25 <pinheadmz> hey maybe ill comment!
72 19:26 <jonatack> Question: Who ought to be responsible for initiating package relay, the sender or the recipient?
73 19:26 <zenogais> I believe it's the sender, since they have to specify the package to relay.
74 19:27 <pinheadmz> the recipient first must signal they allow package messages
75 19:27 <pinheadmz> `sendpackages`
76 19:28 <pinheadmz> like how `sendheaders` is used to initialize headers-first sync from a peer
77 19:28 <jonatack> IIUC, it's a trade-off
78 19:28 <jonatack> The sender is well-positioned to know if package relay is needed
79 19:28 <jonatack> but a naive version could be bandwidth wasteful, and likely need p2p changes
80 19:29 <jonatack> The recipient can tell if a transaction's parents are missing
81 19:29 <jonatack> and request package relay from the sending peer
82 19:29 <jonatack> so package relay would only occur as needed
83 19:30 <jonatack> eg to/from low-memory-mempool nodes
84 19:30 <lightlike> in the current draft implementation, it seems that the recipient figures out when it can use package relay by themselves.
85 19:31 <jonatack> lightlike: Yes. I think recipient-initiated is the preferred approach.
86 19:32 <jonatack> Question: Should we update the p2p protocol to accommodate package relay, or rely on existing functionality?
87 19:32 <zenogais> Makes sense
88 19:32 <jonatack> (eg what are the trade-offs)
89 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
90 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
91 19:36 <jonatack> On one hand, not updating the protocol means ease of upgrading the network.
92 19:37 <zenogais> Not chaning p2p also means decreased attack surface, especially since some of this verification logic is expensive.
93 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.
94 19:38 <jonatack> zenogais: true. And more code and features can mean more attack surface.
95 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)
96 19:38 <zenogais> ^ Yeah, which is why this looks sender initiated in the Draft PR
97 19:39 <jonatack> lightlike: Right. PR 16401 is proof of concept written before 16400 (ATMP refactoring) was merged.
98 19:39 <zenogais> One other trade-off here is that the functionality is a bit redundant with RBF
99 19:40 <fjahr> I think it is a worthwhile improvement without new messages but with new messages we get much higher efficiency
100 19:41 <jonatack> From what I've understood, the advantage of updating the p2p protocol would be potentially optimising network efficiency and computational complexity
101 19:41 <jonatack> fjahr: right
102 19:41 <zenogais> Yeah, otherwise I think there would be a lot more logic around detecting potential relay packages
103 19:41 <zenogais> With P2P message it's explicit
104 19:42 <jonatack> Yes. My money would be on updating. It's too tempting not to try :D
105 19:43 <zenogais> On the plus side, this also opens up some neat possibilities as pointed out in OpTech summary
106 19:44 <zenogais> Increases possibilities for reducing fee-rates
107 19:45 <jonatack> What about DoS vectors and the additional logic added with AcceptMultipleTransactions?
108 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."
109 19:46 <jonatack> Seems like more checks are involved. PreChecks maybe twice? etc.
110 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
111 19:47 <unstaffed-skylig> zenogais where is the comment "We will end up needing..."
112 19:48 <zenogais> DoS potential is a little worrying without there being some limit on package size.
113 19:48 <zenogais> src/validation.cpp Line 1107
114 19:48 <jonatack> Yes. For the complexity reason alone, upgrading/refactoring to simplify/make efficient looks necessary.
115 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
116 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
117 19:50 <jonatack> Lots to do!
118 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.
119 19:51 <jonatack> unstaffed-skylig: zenogais: yes
120 19:51 <jonatack> zenogais: I agree.
121 19:52 <jonatack> And yet we're talking about arguably the most critical sections of the codebase
122 19:54 <zenogais> Is there chain-split potential with this change as well?
123 19:54 <jonatack> Will need eyes and much more review... experienced reviewers. Why this club exists.
124 19:55 <jonatack> 5 minutes. Let's wrap up.
125 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
126 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.
127 19:56 <jonatack> Any PR suggestions for next week?
128 19:57 <zenogais> Looking through PRs right now
129 19:57 <jonatack> lightlike: A systematic framework for simulating/testing p2p might be valuable for assessing this.
130 19:58 <lightlike> i think #16981 will be really interesting, but it's still WIP.
131 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.
132 20:00 <jonatack> Please add your PR suggestions and volunteer for hosting meetings on the club repository!
133 20:01 <zenogais> Will do, thanks jonatack
135 20:01 <jonatack> Thanks, all!
136 20:02 <pinheadmz> thanks jonatack !
137 20:02 <fjahr> thanks jon!
138 20:02 <jonatack> Thanks to zenogais, fjahr, and pinheadmz for reviewing the PR!
139 20:02 <lightlike> thanks!
140 20:04 <jonatack> zenogais: thanks for adding tests!
141 20:05 <zenogais> Of course!