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...
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.