From be4e64d9e4051f7272c75c2819ceb075ed5452c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 15 Jul 2026 12:36:49 -0700 Subject: [PATCH 1/2] test: characterize extra transaction miscount --- src/test/blockencodings_tests.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index e4200cace25..954febb0313 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -144,6 +144,12 @@ public: SERIALIZE_METHODS(TestHeaderAndShortIDs, obj) { READWRITE(obj.header, obj.nonce, Using>>(obj.shorttxids), obj.prefilledtxn); } }; +struct TestPartiallyDownloadedBlock : PartiallyDownloadedBlock { + using PartiallyDownloadedBlock::PartiallyDownloadedBlock; + + size_t GetExtraCount() const { return extra_count; } +}; + BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) { CTxMemPool& pool = *Assert(m_node.mempool); @@ -319,6 +325,13 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) { const CTransactionRef non_block_tx = MakeTransactionRef(std::move(mtx)); CBlock block(BuildBlockTestCase(rand_ctx)); + // Leave one transaction missing so scanning doesn't stop before the collision. + mtx = BuildTransactionTestCase(); + mtx.vin[0].prevout.hash = Txid::FromUint256(rand_ctx.rand256()); + block.vtx.push_back(MakeTransactionRef(std::move(mtx))); + block.hashMerkleRoot = BlockMerkleRoot(block); + while (!CheckProofOfWork(block.GetHash(), block.nBits, Params().GetConsensus())) ++block.nNonce; + std::vector> extra_txn; extra_txn.resize(10); @@ -352,6 +365,14 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) { // This transaction is now available via extra_txn: BOOST_CHECK(partial_block_with_extra.IsTxAvailable(1)); BOOST_CHECK(partial_block_with_extra.IsTxAvailable(2)); + + // Simulate a mempool collision after finding an unrelated extra transaction. + extra_txn[2] = {block.vtx[2]->GetWitnessHash(), non_block_tx}; + TestPartiallyDownloadedBlock partial_block_with_extra_collision{&pool}; + BOOST_CHECK_EQUAL(partial_block_with_extra_collision.InitData(cmpctblock, extra_txn), READ_STATUS_OK); + BOOST_CHECK(partial_block_with_extra_collision.IsTxAvailable(1)); + BOOST_CHECK(!partial_block_with_extra_collision.IsTxAvailable(2)); + BOOST_CHECK_EQUAL(partial_block_with_extra_collision.GetExtraCount(), 0U); // TODO: This should be 1 } } From 6aa5d8d9481f5e06b10095df7f46f0532f7ecdb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Wed, 15 Jul 2026 12:37:48 -0700 Subject: [PATCH 2/2] blockencodings: fix extra transaction count A short ID collision can invalidate a mempool-sourced transaction after an unrelated transaction was found in extra_txn. Track each slot's source so extra_count is decremented only when the invalidated slot came from extra_txn. Mark collided slots explicitly so later candidates do not refill them. --- src/blockencodings.cpp | 33 +++++++++++++++---------------- src/test/blockencodings_tests.cpp | 23 ++++++++++++++++++++- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index c2846539bdb..afa9df4fa7c 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -113,25 +113,25 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c if (shorttxids.size() != cmpctblock.shorttxids.size()) return READ_STATUS_FAILED; // Short ID collision - std::vector have_txn(txn_available.size()); + enum class TxSource : uint8_t { NONE, MEMPOOL, EXTRA, COLLIDED }; + std::vector tx_source(txn_available.size(), TxSource::NONE); { LOCK(pool->cs); for (const auto& [wtxid, txit] : pool->txns_randomized) { uint64_t shortid = cmpctblock.GetShortID(wtxid); std::unordered_map::iterator idit = shorttxids.find(shortid); if (idit != shorttxids.end()) { - if (!have_txn[idit->second]) { + if (tx_source[idit->second] == TxSource::NONE) { txn_available[idit->second] = txit->GetSharedTx(); - have_txn[idit->second] = true; + tx_source[idit->second] = TxSource::MEMPOOL; mempool_count++; - } else { + } else if (tx_source[idit->second] != TxSource::COLLIDED) { // If we find two mempool txn that match the short id, just request it. // This should be rare enough that the extra bandwidth doesn't matter, // but eating a round-trip due to FillBlock failure would be annoying - if (txn_available[idit->second]) { - txn_available[idit->second].reset(); - mempool_count--; - } + txn_available[idit->second].reset(); + mempool_count--; + tx_source[idit->second] = TxSource::COLLIDED; } } // Though ideally we'd continue scanning for the two-txn-match-shortid case, @@ -146,24 +146,23 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c uint64_t shortid = cmpctblock.GetShortID(extra_txn[i].first); std::unordered_map::iterator idit = shorttxids.find(shortid); if (idit != shorttxids.end()) { - if (!have_txn[idit->second]) { + if (tx_source[idit->second] == TxSource::NONE) { txn_available[idit->second] = extra_txn[i].second; - have_txn[idit->second] = true; + tx_source[idit->second] = TxSource::EXTRA; mempool_count++; extra_count++; - } else { + } else if (tx_source[idit->second] != TxSource::COLLIDED && + txn_available[idit->second]->GetWitnessHash() != extra_txn[i].second->GetWitnessHash()) { // If we find two mempool/extra txn that match the short id, just // request it. // This should be rare enough that the extra bandwidth doesn't matter, // but eating a round-trip due to FillBlock failure would be annoying // Note that we don't want duplication between extra_txn and mempool to // trigger this case, so we compare witness hashes first - if (txn_available[idit->second] && - txn_available[idit->second]->GetWitnessHash() != extra_txn[i].second->GetWitnessHash()) { - txn_available[idit->second].reset(); - mempool_count--; - extra_count--; - } + txn_available[idit->second].reset(); + mempool_count--; + extra_count -= (tx_source[idit->second] == TxSource::EXTRA); + tx_source[idit->second] = TxSource::COLLIDED; } } // Though ideally we'd continue scanning for the two-txn-match-shortid case, diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 954febb0313..60cb73d240e 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -147,6 +147,7 @@ public: struct TestPartiallyDownloadedBlock : PartiallyDownloadedBlock { using PartiallyDownloadedBlock::PartiallyDownloadedBlock; + size_t GetMempoolCount() const { return mempool_count; } size_t GetExtraCount() const { return extra_count; } }; @@ -372,7 +373,27 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) { BOOST_CHECK_EQUAL(partial_block_with_extra_collision.InitData(cmpctblock, extra_txn), READ_STATUS_OK); BOOST_CHECK(partial_block_with_extra_collision.IsTxAvailable(1)); BOOST_CHECK(!partial_block_with_extra_collision.IsTxAvailable(2)); - BOOST_CHECK_EQUAL(partial_block_with_extra_collision.GetExtraCount(), 0U); // TODO: This should be 1 + BOOST_CHECK_EQUAL(partial_block_with_extra_collision.GetMempoolCount(), 1U); + BOOST_CHECK_EQUAL(partial_block_with_extra_collision.GetExtraCount(), 1U); + + // Now also collide the extra-sourced slot: both counters decrement exactly once. + extra_txn[3] = {block.vtx[1]->GetWitnessHash(), non_block_tx}; + TestPartiallyDownloadedBlock partial_block_with_extra_source_collision{&pool}; + BOOST_CHECK_EQUAL(partial_block_with_extra_source_collision.InitData(cmpctblock, extra_txn), READ_STATUS_OK); + BOOST_CHECK(!partial_block_with_extra_source_collision.IsTxAvailable(1)); + BOOST_CHECK(!partial_block_with_extra_source_collision.IsTxAvailable(2)); + BOOST_CHECK_EQUAL(partial_block_with_extra_source_collision.GetMempoolCount(), 0U); + BOOST_CHECK_EQUAL(partial_block_with_extra_source_collision.GetExtraCount(), 0U); + + // Collided slots are terminal: not even the genuine transactions refill them. + extra_txn[4] = {block.vtx[2]->GetWitnessHash(), block.vtx[2]}; + extra_txn[5] = {block.vtx[1]->GetWitnessHash(), block.vtx[1]}; + TestPartiallyDownloadedBlock partial_block_no_refill{&pool}; + BOOST_CHECK_EQUAL(partial_block_no_refill.InitData(cmpctblock, extra_txn), READ_STATUS_OK); + BOOST_CHECK(!partial_block_no_refill.IsTxAvailable(1)); + BOOST_CHECK(!partial_block_no_refill.IsTxAvailable(2)); + BOOST_CHECK_EQUAL(partial_block_no_refill.GetMempoolCount(), 0U); + BOOST_CHECK_EQUAL(partial_block_no_refill.GetExtraCount(), 0U); } }