Merge bitcoin#32646: p2p: Add witness mutation check inside FillBlock

Backport of bitcoin#32646 (commits bac9ee4, 28299ce), slimmed for Dash. PartiallyDownloadedBlock::FillBlock now calls IsBlockMutated() instead of CheckBlock() as a belt-and-suspenders mutation check on reconstructed compact blocks, and the vestigial READ_STATUS_CHECKBLOCK_FAILED status is removed.

Dash adaptations: no segwit, so FillBlock keeps its 2-arg signature (no segwit_active parameter) and the mutation mock hook becomes IsBlockMutatedFn = std::function<bool(const CBlock&)> (m_check_block_mutated_mock). The fuzz harness is updated to mock IsBlockMutated instead of CheckBlock.

Author: PastaPastaPasta

src/blockencodings.cpp

@@ -203,16 +203,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
     if (vtx_missing.size() != tx_missing_offset)
         return READ_STATUS_INVALID;
 
-    BlockValidationState state;
-    CheckBlockFn check_block = m_check_block_mock ? m_check_block_mock : CheckBlock;
-    if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true)) {
-        // TODO: We really want to just check merkle tree manually here,
-        // but that is expensive, and CheckBlock caches a block's
-        // "checked-status" (in the CBlock?). CBlock should be able to
-        // check its own merkle root and cache that check.
-        if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED)
-            return READ_STATUS_FAILED; // Possible Short ID collision
-        return READ_STATUS_CHECKBLOCK_FAILED;
+    // Check for possible mutations early now that we have a seemingly good block
+    IsBlockMutatedFn check_mutated{m_check_block_mutated_mock ? m_check_block_mutated_mock : IsBlockMutated};
+    if (check_mutated(/*block=*/block)) {
+        return READ_STATUS_FAILED; // Possible Short ID collision
     }
 
     LogPrint(BCLog::CMPCTBLOCK, "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool (incl at least %lu from extra pool) and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size());

src/blockencodings.h

@@ -84,8 +84,6 @@ typedef enum ReadStatus_t
     READ_STATUS_OK,
     READ_STATUS_INVALID, // Invalid object, peer is sending bogus crap
     READ_STATUS_FAILED, // Failed to process object
-    READ_STATUS_CHECKBLOCK_FAILED, // Used only by FillBlock to indicate a
-                                   // failure in CheckBlock.
 } ReadStatus;
 
 class CBlockHeaderAndShortTxIDs {
@@ -136,8 +134,8 @@ class PartiallyDownloadedBlock {
     CBlockHeader header;
 
     // Can be overriden for testing
-    using CheckBlockFn = std::function<bool(const CBlock&, BlockValidationState&, const Consensus::Params&, bool, bool)>;
-    CheckBlockFn m_check_block_mock{nullptr};
+    using IsBlockMutatedFn = std::function<bool(const CBlock&)>;
+    IsBlockMutatedFn m_check_block_mutated_mock{nullptr};
 
     explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
 

src/net_processing.cpp

@@ -3674,23 +3674,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
                 return;
             }
         } else {
-            // Block is either okay, or possibly we received
-            // READ_STATUS_CHECKBLOCK_FAILED.
-            // Note that CheckBlock can only fail for one of a few reasons:
-            // 1. bad-proof-of-work (impossible here, because we've already
-            //    accepted the header)
-            // 2. merkleroot doesn't match the transactions given (already
-            //    caught in FillBlock with READ_STATUS_FAILED, so
-            //    impossible here)
-            // 3. the block is otherwise invalid (eg invalid coinbase,
-            //    block is too big, too many legacy sigops, etc).
-            // So if CheckBlock failed, #3 is the only possibility.
-            // Under BIP 152, we don't discourage the peer unless proof of work is
-            // invalid (we don't require all the stateless checks to have
-            // been run).  This is handled below, so just treat this as
-            // though the block was successfully read, and rely on the
-            // handling in ProcessNewBlock to ensure the block index is
-            // updated, etc.
+            // Block is okay for further processing
             RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // it is now an empty pointer
             fBlockRead = true;
             // mapBlockSource is used for potentially punishing peers and

src/test/fuzz/partially_downloaded_block.cpp

@@ -28,14 +28,10 @@ void initialize_pdb()
     g_setup = testing_setup.get();
 }
 
-PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValidationResult> result)
+PartiallyDownloadedBlock::IsBlockMutatedFn FuzzedIsBlockMutated(bool result)
 {
-    return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) {
-        if (result) {
-            return state.Invalid(*result);
-        }
-
-        return true;
+    return [result](const CBlock& block) {
+        return result;
     };
 }
 
@@ -103,37 +99,20 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
         skipped_missing |= (!pdb.IsTxAvailable(i) && skip);
     }
 
-    // Mock CheckBlock
-    bool fail_check_block{fuzzed_data_provider.ConsumeBool()};
-    auto validation_result =
-        fuzzed_data_provider.PickValueInArray(
-            {BlockValidationResult::BLOCK_RESULT_UNSET,
-             BlockValidationResult::BLOCK_CONSENSUS,
-             BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE,
-             BlockValidationResult::BLOCK_CACHED_INVALID,
-             BlockValidationResult::BLOCK_INVALID_HEADER,
-             BlockValidationResult::BLOCK_MUTATED,
-             BlockValidationResult::BLOCK_MISSING_PREV,
-             BlockValidationResult::BLOCK_INVALID_PREV,
-             BlockValidationResult::BLOCK_TIME_FUTURE,
-             BlockValidationResult::BLOCK_CHECKPOINT,
-             BlockValidationResult::BLOCK_CHAINLOCK});
-    pdb.m_check_block_mock = FuzzedCheckBlock(
-        fail_check_block ?
-            std::optional<BlockValidationResult>{validation_result} :
-            std::nullopt);
+    // Mock IsBlockMutated
+    bool fail_block_mutated{fuzzed_data_provider.ConsumeBool()};
+    pdb.m_check_block_mutated_mock = FuzzedIsBlockMutated(fail_block_mutated);
 
     CBlock reconstructed_block;
     auto fill_status{pdb.FillBlock(reconstructed_block, missing)};
     switch (fill_status) {
     case READ_STATUS_OK:
         assert(!skipped_missing);
-        assert(!fail_check_block);
+        assert(!fail_block_mutated);
         assert(block->GetHash() == reconstructed_block.GetHash());
         break;
-    case READ_STATUS_CHECKBLOCK_FAILED: [[fallthrough]];
     case READ_STATUS_FAILED:
-        assert(fail_check_block);
+        assert(fail_block_mutated);
         break;
     case READ_STATUS_INVALID:
         break;