We currently concentrate the validation of all the assigned peer’s reconsiderable orphans that are still missing inputs in a single message-processing turn. This ties orphan processing to validation result, and adds latency to processing other peers' messages.
Always process a single reconsiderable orphan per turn instead. This removes the need for an immediate validation result, and shifts the latency from other peers to the assigned peer.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fametrano, ViniciusCestarii, furszy |
| Concept ACK | instagibbs, mzumsande |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
712 | @@ -713,13 +713,12 @@ class PeerManagerImpl final : public PeerManager
713 | /**
714 | * Reconsider orphan transactions after a parent has been accepted to the mempool.
715 | *
commit msg: citing the wrong hash, should be 866c8058a7 ?
Yeah, looks like a copy paste mistake.
Corrected commit message, thanks.
concept ACK
probably slap on a unit test or smth and/or fuzz to cap the amount of reconsidered stuff in a step
Orphan processing used to be uninterruptible, which could be exploited
as a severe CPU DoS (see CVE-2024-52914). 866c8058a7 mitigated this by
interrupting orphan processing after the first transaction validation
that did not result in a missing input error.
This commit moves the interruption point to after each transaction
validation, whatever the result. This unties it from the validation
result and reduces the latency before processing other peers' messages,
at the cost of increasing it for the messages of the peer the orphans
are assigned to.
Added a unit test that demonstrates the behaviour: the test will fail on master, as both child would be reconsidered there (since validation returns a missing-input error).
Concept ACK
Was the missing-inputs exception in 866c8058a706931f025335b3e794ed2f4d287918 there because failing in PreChecks was considered cheap? The only hint I found is "after every actual transaction validation attempt" in the 866c8058a706931f025335b3e794ed2f4d287918 commit message.
It seems the focus was likely on avoiding too much script validation, and wiping from the orphanage eagerly to avoid a single announcer dominating the orphanage. The orphanage and how announcements are handled across peers has changed quite a bit since then.
tACK 83003cafaa81bb116526419246608658ede824b6
I measured the trade-off the description names with a scenario of my own. One peer sends four orphans that each spend a parent output and an outpoint that does not exist, then sends the parent and an unrelated valid transaction back to back. On master the unrelated transaction is processed on the peer's second turn, because all four orphans are retried in one turn. With this PR it takes six turns: the parent, one per orphan, then the transaction. So the wait moves onto the assigned peer, as described, and each turn validates at most one orphan.
The new case in denialofservice_tests fails on master, at the ProcessMessagesOnce check in its loop, and passes here.
I tested the head merged into master a773e1e33b, in a Debug build, with the denialofservice, orphanage, txdownload and txpackage unit tests and the p2p_orphan_handling and p2p_opportunistic_1p1c functional tests. I ran them again with #36437 and #36578 merged on top; the three PRs merge without conflicts. I can share the scenario, a Boost test case, as a gist.
Concept ACK
This removes the need for an immediate validation result
what do you mean by that? Looking at the code, there still seems to be the need for it, to call ProcessValidTx/ProcessInvalidTx/neither.
ACK 83003cafaa81bb116526419246608658ede824b6
I like how this also simplifies the code by replacing the while loop with an if
This removes the need for an immediate validation result
what do you mean by that? Looking at the code, there still seems to be the need for it, to call
ProcessValidTx/ProcessInvalidTx/neither.
I've been experimenting with a net_processing / validation split, whereby the message handling and chainstate validation threads talk via message passing. This type of pattern whereby the message handling thread decides how to make progress based on an immediate result it gets from validation ("is this a missing inputs error or something else?") requires a more clumsy API: instead of asking for validation of a transaction, and acting on it once the job comes back, conserving something close to today's behaviour would require passing all reconsiderable transaction in bulk, then the chainstate validation thread to perform today's "validate until result is anything but a missing inputs error", and then return the set of not-yet-reconsidered transactions to net_processing. I think this is a further hint that today's bulk processing is a suboptimal design.
3485 | @@ -3491,7 +3486,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) 3486 | AssertLockHeld(g_msgproc_mutex); 3487 | LOCK2(::cs_main, m_tx_download_mutex); 3488 | 3489 | - while (CTransactionRef porphanTx = m_txdownloadman.GetTxToReconsider(peer.m_id)) { 3490 | + if (CTransactionRef porphanTx = m_txdownloadman.GetTxToReconsider(peer.m_id)) {
pedantic nano nit: I know this doesn't worth it, and don't want you to do it here but.. my inner self would be happy to read this as
CTransactionRef porphanTx = m_txdownloadman.GetTxToReconsider(peer.m_id);
if (!porphanTx) return false;
// ... process orphan..
712 | @@ -713,13 +713,12 @@ class PeerManagerImpl final : public PeerManager 713 | /** 714 | * Reconsider orphan transactions after a parent has been accepted to the mempool. 715 | * 716 | - * @peer[in] peer The peer whose orphan transactions we will reconsider. Generally only 717 | + * @peer[in] peer The peer whose orphan transactions we will reconsider. At most
Unrelated to this PR but still wrong
* [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] peer The peer whose orphan transactions we will reconsider. At most
536 | @@ -537,6 +537,33 @@ BOOST_FIXTURE_TEST_CASE(orphan_reconsideration_interruptible, TestChain100Setup) 537 | BOOST_CHECK_EQUAL(mempool.size(), 1U + num_children / 2); 538 | BOOST_CHECK(!connman.ProcessMessagesOnce(peer)); 539 | 540 | + // Create a new parent for the purpose of testing a single orphan is reconsidered per call.
tiny nit: as an extra sanity check, could check we still process a third valid child after rejecting the invalid ones.
<details> <summary>Click to expand the diff</summary>
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -538,30 +538,44 @@
BOOST_CHECK(!connman.ProcessMessagesOnce(peer));
// Create a new parent for the purpose of testing a single orphan is reconsidered per call.
- mineBlocks(1);
- const int child_count{2};
+ mineBlocks(1); // Mature a second coinbase output for the new parent to spend.
+ const int num_missing_input_children{2};
+ const int num_new_children{num_missing_input_children + 1}; // Plus a valid child.
const auto new_parent{MakeTransactionRef(CreateValidMempoolTransaction(
{m_coinbase_txns[1]}, {COutPoint{m_coinbase_txns[1]->GetHash(), 0}}, /*input_height=*/2, {coinbaseKey},
- std::vector<CTxOut>(child_count, CTxOut{5 * COIN, script}), /*submit=*/false))};
+ std::vector<CTxOut>(num_new_children, CTxOut{5 * COIN, script}), /*submit=*/false))};
- // Now create 2 children. Each spends the parent and a non-existing input.
- for (int i{0}; i < child_count; ++i) {
+ // Now create the missing-input children. Each spends the parent and a non-existing input.
+ for (int i{0}; i < num_missing_input_children; ++i) {
CMutableTransaction mtx;
mtx.vin.emplace_back(COutPoint{new_parent->GetHash(), uint32_t(i)});
mtx.vin.emplace_back(COutPoint{Txid::FromUint256(m_rng.rand256()), 0});
mtx.vout.emplace_back(COIN, script);
send_tx(MakeTransactionRef(mtx));
}
- BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), child_count);
+ // Then a valid child, spending the parent's last output. Orphans are reconsidered oldest
+ // announcement first, so it is queued behind the missing-input children.
+ const auto valid_child{MakeTransactionRef(CreateValidMempoolTransaction(
+ new_parent, /*input_vout=*/num_missing_input_children, /*input_height=*/101, coinbaseKey, script, 4 * COIN, /*submit=*/false))};
+ send_tx(valid_child);
+ BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), num_new_children);
// Processing the parent will lead to processing a single orphan per run, even though each fails
- // on a missing-input error (orphanage size does not decrease).
+ // on a missing-input error (orphanage size does not decrease). The valid child must wait for a
+ // call of its own.
BOOST_CHECK(send_tx(new_parent));
BOOST_CHECK(mempool.exists(new_parent->GetHash()));
- for (int i{0}; i < child_count; ++i) {
+ for (int i{0}; i < num_missing_input_children; ++i) {
BOOST_CHECK(connman.ProcessMessagesOnce(peer));
- BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), child_count);
+ BOOST_CHECK(!mempool.exists(valid_child->GetHash()));
+ BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), num_new_children);
}
+
+ // The next call accepts the valid child.
+ BOOST_CHECK(connman.ProcessMessagesOnce(peer));
+ BOOST_CHECK(mempool.exists(valid_child->GetHash()));
+ BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), num_missing_input_children);
+ // No more orphans to process.
BOOST_CHECK(!connman.ProcessMessagesOnce(peer));
peerman.FinalizeNode(peer);
</details>
ACK 83003cafaa81bb116526419246608658ede824b6