Low 48 Bitcoin Committed Jul 17, 2026 · 14:57 UTC

blockencodings: fix extra transaction count

6aa5d8d9 View source ↗

Public commit record

What the developer wrote

Authored by Lőrinc

68/100 · Adequate

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.

✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context

The short version

What changed, and why it matters

This commit fixes a bookkeeping bug in Bitcoin Core's compact block handling. When receiving a compressed block, the node tries to match short transaction IDs against its own mempool and a small cache of recently seen transactions ('extra_txn'). A bug caused the node to incorrectly decrement the 'extra_count' counter when a mempool-sourced transaction collided with a short ID, even though no extra_txn transaction was actually lost. This could lead to wrong internal counts and, in edge cases, prevent a collided slot from being correctly marked as unusable, potentially allowing later transactions to refill a slot that should stay empty. The fix tracks whether each matched slot came from the mempool, extra cache, or a collision, and only adjusts the appropriate counters.

Recommended action

Reviewers should verify that the TxSource state machine covers all transition paths (NONE -> MEMPOOL/EXTRA -> COLLIDED) and that no path allows a COLLIDED slot to be refilled. The added unit tests should be run to confirm the fix. Consider whether this bug could be triggered remotely via crafted short ID collisions and whether additional hardening is warranted.

Security signals we found

Incorrect internal accounting in compact block reconstruction

Short ID collision handling could misclassify transaction source

Potential for collided slots to be refilled by later candidates

Fix includes regression tests for counter correctness and collision terminal state

Technical analysis

Evidence from the diff

In PartiallyDownloadedBlock::InitData, the previous code used a single std::vector have_txn to track filled slots. When a short ID collision occurred (two transactions mapping to the same short ID), the code would reset the slot and decrement both mempool_count and extra_count unconditionally if a transaction was present. This was incorrect when the slot had been filled from the mempool, because extra_count would be decremented even though no extra_txn entry was consumed. The patch replaces the boolean with a TxSource enum (NONE, MEMPOOL, EXTRA, COLLIDED), decrements extra_count only when the invalidated slot’s source was EXTRA, and marks collided slots explicitly so subsequent candidates cannot refill them. Tests are updated to verify the corrected counters and the terminal nature of collided slots.

Changed components

src/blockencodings.cppsrc/test/blockencodings_tests.cppPartiallyDownloadedBlock::InitDatacompact block reconstruction logic

Inspect captured patch +38 / −18

diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index c2846539..afa9df4f 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<bool> have_txn(txn_available.size());
+    enum class TxSource : uint8_t { NONE, MEMPOOL, EXTRA, COLLIDED };
+    std::vector<TxSource> 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<uint64_t, uint16_t>::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<uint64_t, uint16_t>::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 954febb0..60cb73d2 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);
     }
 }

Risk score

Why this scored 48/100

Our methodology →

Potential impact 12/30

Exploitability 8/25

Stealth signal 7/15

Affected reach 10/15

Confidence 7/10

Evidence quality 4/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 48/100

Community notes

Related watches

Merge bitcoin/bitcoin#35928: doc: mention -DWITH_ZMQ=ON in macOS build guide

Merge bitcoin/bitcoin#35482: fuzz: exercise the transaction-handling path in process_message(s)

Merge bitcoin/bitcoin#35914: test, fuzz: Remove unused variables