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.
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
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?
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.”
<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.
<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."
<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.
<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
<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
<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.
<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
<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)
<jonatack> From what I've understood, the advantage of updating the p2p protocol would be potentially optimising network efficiency and computational complexity
<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."
<jonatack> Also: 1) We would want the ability to perform policy checks on the whole package *before* signature validation to avoid CPU-exhaustion attacks
<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
<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.
<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.