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] 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); } }