Merge bitcoin/bitcoin#36262: test: cover orphan reconsideration interruptibility
What the developer wrote
Authored by merge-script
Merge bitcoin/bitcoin#36262: test: cover orphan reconsideration interruptibility
16bff77a5aaf7a9740b859f74b8898cee6de5588 test: cover orphan reconsideration interruptibility (ViniciusCestarii)
Pull request description:
Add test checking that ProcessOrphanTx reconsiders at most one orphan per ProcessMessages(). This test prevents regression of [CVE-2024-52914](https://bitcoincore.org/en/2024/07/03/disclose-orphan-dos/) which currently no existing test checks.
The test kills 2 mutants found with https://github.com/ViniciusCestarii/mutant-harness:
1. Remove `return true` after an orphan is accepted
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3494,7 +3494,6 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
LogDebug(BCLog::TXPACKAGES, " accepted orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString());
ProcessValidTx(peer.m_id, porphanTx, result.m_replaced_transactions);
- return true;
} else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
```
2. Remove `return true` after an orphan is rejected for a reason other than `TX_MISSING_INPUTS`
```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3508,7 +3508,6 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
state.GetResult() != TxValidationResult::TX_RESULT_UNSET)) {
ProcessInvalidTx(peer.m_id, porphanTx, state, /*first_time_failure=*/false);
}
- return true;
}
}
```
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/36262/commits/16bff77a5aaf7a9740b859f74b8898cee6de5588
fametrano:
tACK 16bff77a5aaf7a9740b859f74b8898cee6de5588
Tree-SHA512: 3d00bf7aa023c47d335b0f4c9d720c914611962272b895d91c8a542739f3b9c0d35c1dba5becdfd7167122c3f3f2be87b366d296732fa2765538467e6c0af6d7
What changed, and why it matters
This commit only adds a new automated test to Bitcoin Core. It does not change any production code. The test verifies that a previously fixed denial-of-service bug (CVE-2024-52914) stays fixed by checking that orphan transactions are reconsidered one at a time per network message. It is a regression test, not a security patch.
No security action needed. Treat as normal test-only maintenance. Reviewers may optionally confirm the test correctly exercises the CVE-2024-52914 fix and passes in CI.
Security signals we found
Adds regression test for previously disclosed CVE-2024-52914
No changes to src/net_processing.cpp or other production code
Test targets interruptibility of orphan transaction reconsideration
Commit message explicitly describes test purpose and mutant killing
Evidence from the diff
The change is a single new BOOST_FIXTURE_TEST_CASE named orphan_reconsideration_interruptible in src/test/denialofservice_tests.cpp. It creates six child transactions that arrive before their parent, making them orphans. Half pay no fee and will later be rejected. After the parent arrives, the test asserts that each ProcessMessagesOnce() call processes exactly one orphan, so the orphan reconsideration loop remains interruptible. The commit references CVE-2024-52914 and the Bitcoin Core disclosure page, but the actual vulnerability was already patched; this commit only adds missing test coverage.
Changed components
src/test/denialofservice_tests.cppInspect captured patch +69 / −0
### src/test/denialofservice_tests.cpp
@@ -7,11 +7,14 @@
#include <banman.h>
#include <chainparams.h>
#include <common/args.h>
+#include <key.h>
#include <net.h>
#include <net_processing.h>
+#include <primitives/transaction.h>
#include <pubkey.h>
#include <script/sign.h>
#include <script/signingprovider.h>
+#include <script/solver.h>
#include <serialize.h>
#include <test/util/net.h>
#include <test/util/random.h>
@@ -471,4 +474,70 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
peerLogic->FinalizeNode(dummyNode);
}
+// Reconsidering orphans must be interruptible: at most one orphan is accepted
+// or rejected per ProcessMessages() call.
+// See https://bitcoincore.org/en/2024/07/03/disclose-orphan-dos.
+BOOST_FIXTURE_TEST_CASE(orphan_reconsideration_interruptible, TestChain100Setup)
+{
+ LOCK(NetEventsInterface::g_msgproc_mutex);
+
+ ConnmanTestMsg& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+ PeerManager& peerman = *m_node.peerman;
+ const CTxMemPool& mempool = *m_node.mempool;
+
+ CNode peer{/*id=*/0,
+ /*sock=*/nullptr,
+ CAddress(ip(0xa0b0c001), NODE_NONE),
+ /*nKeyedNetGroupIn=*/0,
+ /*nLocalHostNonceIn=*/0,
+ CAddress(),
+ /*addrNameIn=*/"",
+ ConnectionType::OUTBOUND_FULL_RELAY,
+ /*inbound_onion=*/false,
+ /*network_key=*/0};
+ connman.Handshake(
+ /*node=*/peer,
+ /*successfully_connected=*/true,
+ /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+ /*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
+ /*version=*/PROTOCOL_VERSION,
+ /*relay_txs=*/true);
+
+ const CScript script{GetScriptForRawPubKey(coinbaseKey.GetPubKey())};
+ const int num_children{6};
+ const auto parent{MakeTransactionRef(CreateValidMempoolTransaction(
+ {m_coinbase_txns[0]}, {COutPoint{m_coinbase_txns[0]->GetHash(), 0}}, /*input_height=*/1, {coinbaseKey},
+ std::vector<CTxOut>(num_children, CTxOut{5 * COIN, script}), /*submit=*/false))};
+
+ const auto send_tx{[&](const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
+ connman.FlushSendBuffer(peer); // Drop messages we sent, the transport is shared with the fake peer.
+ BOOST_REQUIRE(connman.ReceiveMsgFrom(peer, NetMsg::Make(NetMsgType::TX, TX_WITH_WITNESS(*tx))));
+ peer.fPauseSend = false;
+ return connman.ProcessMessagesOnce(peer);
+ }};
+
+ // Children arrive first and become orphans. Half of them pay no fee and will be rejected.
+ for (int i{0}; i < num_children; ++i) {
+ const CAmount output_value{i % 2 ? 5 * COIN : 4 * COIN};
+ send_tx(MakeTransactionRef(CreateValidMempoolTransaction(parent, i, /*input_height=*/101, coinbaseKey, script, output_value, /*submit=*/false)));
+ }
+ BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), num_children);
+ BOOST_CHECK_EQUAL(mempool.size(), 0U);
+
+ // Accepting the parent schedules all children for reconsideration.
+ BOOST_CHECK(send_tx(parent));
+ BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), num_children);
+ BOOST_CHECK_EQUAL(mempool.size(), 1U);
+
+ // Each call accepts or rejects exactly one orphan, whether it is valid or not.
+ for (int i{1}; i <= num_children; ++i) {
+ BOOST_CHECK(connman.ProcessMessagesOnce(peer));
+ BOOST_CHECK_EQUAL(peerman.GetOrphanTransactions().size(), num_children - i);
+ }
+ BOOST_CHECK_EQUAL(mempool.size(), 1U + num_children / 2);
+ BOOST_CHECK(!connman.ProcessMessagesOnce(peer));
+
+ peerman.FinalizeNode(peer);
+}
+
BOOST_AUTO_TEST_SUITE_END()Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.