refactor: consolidate MempoolAcceptResult processing #29619
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Note to reviewers: this PR isn't supposed to change behavior at all. I have a spreadsheet here enumerating what we do on master, and this PR should match that. |
Deduplicate code that exists in both tx processing and ProcessOrphanTx. Additionally, this can be reused in a function that handles multiple MempoolAcceptResults from package submission.
glozow
force-pushed
the
2024-03-atmp-refactors
branch
from
9696d66
to
a407f10
Compare
Deduplicate code that exists in both tx processing and ProcessOrphanTx. Additionally, this can be reused in a function that handles multiple MempoolAcceptResults from package submission.
glozow
force-pushed
the
2024-03-atmp-refactors
branch
from
a407f10
to
07cd510
Compare
|
thanks for doing this! concept ACK |
There was a problem hiding this comment.
ACK 07cd510
I can't find any behavior differences in this attempt, and it seems to be straight forwardly useful in the follow-up 1p1c PR.
Looking forward to making better unit test coverage; it's really quite lacking sadly.
| RelayTransaction(tx.GetHash(), tx.GetWitnessHash()); | ||
| m_orphanage.AddChildrenToWorkSet(tx); | ||
|
|
||
| ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value()); |
There was a problem hiding this comment.
value_or here as well for belt and suspenders?
There was a problem hiding this comment.
Adding in the next PR, or will squash if I retouch 👍
| // We only add the txid if it differs from the wtxid, to | ||
| // avoid wasting entries in the rolling bloom filter. |
There was a problem hiding this comment.
In 07cd510 "[refactor] consolidate invalid MempoolAcceptResult processing"
This comment is lost.
| @@ -3037,6 +3046,63 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, | |||
| return; | |||
| } | |||
|
|
|||
| void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state, | |||
There was a problem hiding this comment.
Nit: The var names differ from the declaration for CTransactionsRef& and TxValidationState&. Is there a reason for the p in ptx?
There was a problem hiding this comment.
p as in "pointer". I usually use CMutableTransaction mtx and CTransaction tx. Though I agree there's a mix...
e518a8b [functional test] opportunistic 1p1c package submission (glozow) 87c5c52 [p2p] opportunistically accept 1-parent-1-child packages (glozow) 6c51e1d [p2p] add separate rejections cache for reconsiderable txns (glozow) 410ebd6 [fuzz] break out parent functions and add GetChildrenFrom* coverage (glozow) d095316 [unit test] TxOrphanage::GetChildrenFrom* (glozow) 2f51cd6 [txorphanage] add method to get all orphans spending a tx (glozow) 092c978 [txpackages] add canonical way to get hash of package (glozow) c3c1e15 [doc] restore comment about why we check if ptx HasWitness before caching rejected txid (glozow) 6f4da19 guard against MempoolAcceptResult::m_replaced_transactions (glozow) Pull request description: This enables 1p1c packages to propagate in the "happy case" (i.e. not reliable if there are adversaries) and contains a lot of package relay-related code. See #27463 for overall package relay tracking. Rationale: This is "non-robust 1-parent-1-child package relay" which is immediately useful. - Relaying 1-parent-1-child CPFP when mempool min feerate is high would be a subset of all package relay use cases, but a pretty significant improvement over what we have today, where such transactions don't propagate at all. [1] - Today, a miner can run this with a normal/small maxmempool to get revenue from 1p1c CPFP'd transactions without losing out on the ones with parents below mempool minimum feerate. - The majority of this code is useful for building more featureful/robust package relay e.g. see the code in #27742. The first 2 commits are followups from #29619: - #29619 (comment) - #29619 (comment) Q: What makes this short of a more full package relay feature? (1) it only supports packages in which 1 of the parents needs to be CPFP'd by the child. That includes 1-parent-1-child packages and situations in which the other parents already pay for themselves (and are thus in mempool already when the package is submitted). More general package relay is a future improvement that requires more engineering in mempool and validation - see #27463. (2) We rely on having kept the child in orphanage, and don't make any attempt to protect it while we wait to receive the parent. If we are experiencing a lot of orphanage churn (e.g. an adversary is purposefully sending us a lot of transactions with missing inputs), we will fail to submit packages. This limitation has been around for 12+ years, see #27742 which adds a token bucket scheme for protecting package-related orphans at a limited rate per peer. (3) Our orphan-handling logic is somewhat opportunistic; we don't make much effort to resolve an orphan beyond asking the child's sender for the parents. This means we may miss packages if the first sender fails to give us the parent (intentionally or unintentionally). To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. This is also an existing problem which has a proposed solution in #28031. [1]: see this writeup and its links https://github.com/bitcoin/bips/blob/02ec218c7857ef60914e9a3d383b68caf987f70b/bip-0331.mediawiki#propagate-high-feerate-transactions ACKs for top commit: sr-gi: tACK e518a8b instagibbs: reACK e518a8b theStack: Code-review ACK e518a8b 📦 dergoegge: light Code review ACK e518a8b achow101: ACK e518a8b Tree-SHA512: 632579fbe7160cb763bbec6d82ca0dab484d5dbbc7aea90c187c0b9833b8d7c1e5d13b8587379edd3a3b4a02a5a1809020369e9cd09a4ebaf729921f65c15943
Every time we try to
ProcessTransaction(i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached inm_recent_rejectsso we don't try to download/validate it again.There are 2 current places and at least 1 future place where we need to process
MempoolAcceptResult:ProcessMessagelogic after receiving and validating a txProcessOrphanTxwhere we retry orphansConsolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR.