refactor: consolidate MempoolAcceptResult processing

glozow commented at 10:43 am on March 11, 2024: member

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 in m_recent_rejects so 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:

In the ProcessMessage logic after receiving and validating a tx
In ProcessOrphanTx where we retry orphans
With #28970, after processing a package of transactions, we should do these updates for each tx in the package.

Consolidate this code so it isn’t repeated in 2 places and so we can reuse it in a future PR.

glozow added the label Refactoring on Mar 11, 2024

DrahtBot commented at 10:43 am on March 11, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, achow101, dergoegge, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#28970 (p2p: opportunistically accept 1-parent-1-child packages by glozow)

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.

in src/net_processing.cpp:592 in 9696d66d14 outdated

586+     * @param[in]   was_rejected    Whether this tx has already been rejected before and is being
587+     *                              reevaluated as an orphan. Used to avoid adding duplicate entries to
588+     *                              vExtraTxnForCompact.
589+     * Updates m_txrequest, m_recent_rejects, m_orphanage, and vExtraTxnForCompact. */
590+    void ProcessInvalidTx(const CTransactionRef& tx, const TxValidationState& result, bool was_rejected, NodeId nodeid)
591+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);

dergoegge commented at 10:49 am on March 11, 2024:

0    void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result, bool was_rejected)
1        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);

nit: This is more inline with other method sigs in peerman.

glozow commented at 11:09 am on March 11, 2024:

done

in src/net_processing.cpp:595 in 9696d66d14 outdated

590+    void ProcessInvalidTx(const CTransactionRef& tx, const TxValidationState& result, bool was_rejected, NodeId nodeid)
591+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
592+
593+    /** Handle a transaction whose result was MempoolAcceptResult::ResultType::VALID.
594+     * Updates m_txrequest, m_orphanage, and vExtraTxnForCompact. Also queues the tx for relay. */
595+    void ProcessValidTx(const CTransactionRef& tx, NodeId nodeid, const std::list<CTransactionRef>& replaced_transactions)

dergoegge commented at 10:50 am on March 11, 2024:

0    void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)

glozow commented at 11:09 am on March 11, 2024:

done

in src/net_processing.cpp:586 in 9696d66d14 outdated

581@@ -582,6 +582,19 @@ class PeerManagerImpl final : public PeerManager
582      */
583     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
584 
585+    /** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID.
586+     * @param[in]   was_rejected    Whether this tx has already been rejected before and is being

dergoegge commented at 10:52 am on March 11, 2024:

Maybe was_orphan would be a better name?

glozow commented at 10:56 am on March 11, 2024:

I was calling it this, but in the future it could also be was_failed_for_too_low_fee, so I changed it to was_rejected to be more generic.

dergoegge commented at 11:01 am on March 11, 2024:

Ok, how about add_to_extra_compact_txs?

glozow commented at 11:41 am on March 11, 2024:

Ok :handshake: maybe_add_extra_compact_tx

dergoegge commented at 10:55 am on March 11, 2024: member

Concept ACK

just inline nits for now

glozow commented at 11:03 am on March 11, 2024: member

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. For tests, p2p_orphan_handling.py was written to hit some of these codepaths. Unit/fuzz tests for this are pretty difficult to achieve right now, but I would like to add some in the future after modularization - see #28031 (e.g.https://github.com/bitcoin/bitcoin/pull/28031/commits/ffa4d60bf11abac7aeeb9712e77245c67c15d3f6).

[refactor] consolidate valid MempoolAcceptResult processing

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.

9353aa4066

glozow force-pushed on Mar 11, 2024

[refactor] consolidate invalid MempoolAcceptResult processing

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.

07cd510ffe

glozow force-pushed on Mar 11, 2024

instagibbs commented at 2:01 pm on March 11, 2024: member

thanks for doing this! concept ACK

in src/net_processing.cpp:4304 in 9353aa4066 outdated

4306-            m_txrequest.ForgetTxHash(tx.GetHash());
4307-            m_txrequest.ForgetTxHash(tx.GetWitnessHash());
4308-            RelayTransaction(tx.GetHash(), tx.GetWitnessHash());
4309-            m_orphanage.AddChildrenToWorkSet(tx);
4310-
4311+            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value());

instagibbs commented at 2:29 pm on March 11, 2024:

value_or here as well for belt and suspenders?

glozow commented at 9:42 am on March 12, 2024:

Adding in the next PR, or will squash if I retouch :+1:

glozow commented at 11:57 am on March 13, 2024:

done in #28970

in src/net_processing.cpp:3097 in 07cd510ffe

3092+        if (maybe_add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) {
3093+            AddToCompactExtraTransactions(ptx);
3094+        }
3095+    }
3096+
3097+    MaybePunishNodeForTx(nodeid, state);

instagibbs commented at 3:38 pm on March 11, 2024:

now that it’s one call-site, do we need this subroutine?

glozow commented at 9:41 am on March 12, 2024:

I thought about this as well, but didn’t think I should do anything with it in this PR. Seems like there are some ideas in the air wrt misbehavior and I’m not sure where it’s going to land / don’t want to conflict with them (#29575, #29530).

instagibbs approved

instagibbs commented at 4:35 pm on March 11, 2024: member

ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b

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.

DrahtBot requested review from dergoegge on Mar 11, 2024

in src/net_processing.cpp:3130 in 07cd510ffe

3191-                // processing of this transaction in the event that child
3192-                // transactions are later received (resulting in
3193-                // parent-fetching by txid via the orphan-handling logic).
3194-                if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->HasWitness()) {
3195-                    // We only add the txid if it differs from the wtxid, to
3196-                    // avoid wasting entries in the rolling bloom filter.

achow101 commented at 3:13 pm on March 12, 2024:

In 07cd510ffe791a4dfc1824c67fb440be780a4e2b “[refactor] consolidate invalid MempoolAcceptResult processing”

This comment is lost.

glozow commented at 11:57 am on March 13, 2024:

added back in #28970

achow101 commented at 3:20 pm on March 12, 2024: member

ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b

dergoegge approved

dergoegge commented at 4:09 pm on March 12, 2024: member

Code review ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b

in src/net_processing.cpp:3049 in 07cd510ffe

3045@@ -3037,6 +3046,63 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
3046     return;
3047 }
3048 
3049+void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state,

TheCharlatan commented at 11:01 am on March 13, 2024:

Nit: The var names differ from the declaration for CTransactionsRef& and TxValidationState&. Is there a reason for the p in ptx?

glozow commented at 11:44 am on March 13, 2024:

p as in “pointer”. I usually use CMutableTransaction mtx and CTransaction tx. Though I agree there’s a mix…

TheCharlatan approved

TheCharlatan commented at 11:13 am on March 13, 2024: contributor

ACK 07cd510ffe791a4dfc1824c67fb440be780a4e2b

achow101 merged this on Mar 13, 2024

achow101 closed this on Mar 13, 2024

glozow deleted the branch on Mar 13, 2024

refactor: consolidate MempoolAcceptResult processing #29619

Code Coverage

Reviews

Conflicts