Merge bitcoin/bitcoin#26898: fuzz: Add PartiallyDownloadedBlock target

a1c36275b5a27ae685f49ff02dabff0adbf51aa1 [fuzz] Assert that omitting missing transactions always fails block reconstruction (dergoegge)
a8ac61ab5e1805df61f4dc94ded44a874725484c [fuzz] Add PartiallyDownloadedBlock target (dergoegge)
42bd4c746824e3b2adf2c696cf4a705fa43d1fa8 [block encodings] Avoid fuzz blocking asserts in PartiallyDownloadedBlock (dergoegge)
1429f8377017c0029cb87c4d355c37b796432611 [block encodings] Make CheckBlock mockable for PartiallyDownloadedBlock (dergoegge)

Pull request description:

  This PR adds a fuzz target for `PartiallyDownloadedBlock`, which we currently do not have any coverage for.

ACKs for top commit:
  mzumsande:
    Code Review ACK a1c36275b5a27ae685f49ff02dabff0adbf51aa1
  MarcoFalke:
    re-ACK a1c36275b5a27ae685f49ff02dabff0adbf51aa1  🎼

Tree-SHA512: 01ae452fe457da0c8f2b28c72091d40807c56a9e5d0f80b55f166b67be50baf80a02f53d4cbe9736bb22424cca1758b87e4e471b8a24e756c22563a2640e9a5f

4 files changed, 165 insertions, 6 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 1a29e9a47a..4d867fdc2f 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -293,6 +293,7 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/parse_numbers.cpp \
  test/fuzz/parse_script.cpp \
  test/fuzz/parse_univalue.cpp \
+ test/fuzz/partially_downloaded_block.cpp \
  test/fuzz/policy_estimator.cpp \
  test/fuzz/policy_estimator_io.cpp \
  test/fuzz/pow.cpp \
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index bcb86d75cc..0d5575e5d5 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -52,7 +52,8 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
     if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MAX_BLOCK_WEIGHT / MIN_SERIALIZABLE_TRANSACTION_WEIGHT)
         return READ_STATUS_INVALID;
 
-    assert(header.IsNull() && txn_available.empty());
+    if (!header.IsNull() || !txn_available.empty()) return READ_STATUS_INVALID;
+
     header = cmpctblock.header;
     txn_available.resize(cmpctblock.BlockTxCount());
 
@@ -167,14 +168,18 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
     return READ_STATUS_OK;
 }
 
-bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const {
-    assert(!header.IsNull());
+bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const
+{
+    if (header.IsNull()) return false;
+
     assert(index < txn_available.size());
     return txn_available[index] != nullptr;
 }
 
-ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) {
-    assert(!header.IsNull());
+ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing)
+{
+    if (header.IsNull()) return READ_STATUS_INVALID;
+
     uint256 hash = header.GetHash();
     block = header;
     block.vtx.resize(txn_available.size());
@@ -197,7 +202,8 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
         return READ_STATUS_INVALID;
 
     BlockValidationState state;
-    if (!CheckBlock(block, state, Params().GetConsensus())) {
+    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
diff --git a/src/blockencodings.h b/src/blockencodings.h
index e60c1e3db4..7207ff1ae2 100644
--- a/src/blockencodings.h
+++ b/src/blockencodings.h
@@ -7,8 +7,13 @@
 
 #include <primitives/block.h>
 
+#include <functional>
 
 class CTxMemPool;
+class BlockValidationState;
+namespace Consensus {
+struct Params;
+};
 
 // Transaction compression schemes for compact block relay can be introduced by writing
 // an actual formatter here.
@@ -129,6 +134,11 @@ protected:
     const CTxMemPool* pool;
 public:
     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};
+
     explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
 
     // extra_txn is a list of extra transactions to look at, in <witness hash, reference> form
diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp
new file mode 100644
index 0000000000..f8ba4f08d9
--- /dev/null
+++ b/src/test/fuzz/partially_downloaded_block.cpp
@@ -0,0 +1,142 @@
+#include <blockencodings.h>
+#include <consensus/merkle.h>
+#include <consensus/validation.h>
+#include <primitives/block.h>
+#include <primitives/transaction.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/util.h>
+#include <test/fuzz/util/mempool.h>
+#include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
+#include <txmempool.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <limits>
+#include <memory>
+#include <optional>
+#include <set>
+#include <vector>
+
+namespace {
+const TestingSetup* g_setup;
+} // namespace
+
+void initialize_pdb()
+{
+    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    g_setup = testing_setup.get();
+}
+
+PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValidationResult> result)
+{
+    return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) {
+        if (result) {
+            return state.Invalid(*result);
+        }
+
+        return true;
+    };
+}
+
+FUZZ_TARGET_INIT(partially_downloaded_block, initialize_pdb)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+
+    auto block{ConsumeDeserializable<CBlock>(fuzzed_data_provider)};
+    if (!block || block->vtx.size() == 0 ||
+        block->vtx.size() >= std::numeric_limits<uint16_t>::max()) {
+        return;
+    }
+
+    CBlockHeaderAndShortTxIDs cmpctblock{*block};
+
+    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
+    PartiallyDownloadedBlock pdb{&pool};
+
+    // Set of available transactions (mempool or extra_txn)
+    std::set<uint16_t> available;
+    // The coinbase is always available
+    available.insert(0);
+
+    std::vector<std::pair<uint256, CTransactionRef>> extra_txn;
+    for (size_t i = 1; i < block->vtx.size(); ++i) {
+        auto tx{block->vtx[i]};
+
+        bool add_to_extra_txn{fuzzed_data_provider.ConsumeBool()};
+        bool add_to_mempool{fuzzed_data_provider.ConsumeBool()};
+
+        if (add_to_extra_txn) {
+            extra_txn.emplace_back(tx->GetWitnessHash(), tx);
+            available.insert(i);
+        }
+
+        if (add_to_mempool) {
+            LOCK2(cs_main, pool.cs);
+            pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx));
+            available.insert(i);
+        }
+    }
+
+    auto init_status{pdb.InitData(cmpctblock, extra_txn)};
+
+    std::vector<CTransactionRef> missing;
+    // Whether we skipped a transaction that should be included in `missing`.
+    // FillBlock should never return READ_STATUS_OK if that is the case.
+    bool skipped_missing{false};
+    for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) {
+        // If init_status == READ_STATUS_OK then a available transaction in the
+        // compact block (i.e. IsTxAvailable(i) == true) implies that we marked
+        // that transaction as available above (i.e. available.count(i) > 0).
+        // The reverse is not true, due to possible compact block short id
+        // collisions (i.e. available.count(i) > 0 does not imply
+        // IsTxAvailable(i) == true).
+        if (init_status == READ_STATUS_OK) {
+            assert(!pdb.IsTxAvailable(i) || available.count(i) > 0);
+        }
+
+        bool skip{fuzzed_data_provider.ConsumeBool()};
+        if (!pdb.IsTxAvailable(i) && !skip) {
+            missing.push_back(block->vtx[i]);
+        }
+
+        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_HEADER_LOW_WORK});
+    pdb.m_check_block_mock = FuzzedCheckBlock(
+        fail_check_block ?
+            std::optional<BlockValidationResult>{validation_result} :
+            std::nullopt);
+
+    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(block->GetHash() == reconstructed_block.GetHash());
+        break;
+    case READ_STATUS_CHECKBLOCK_FAILED: [[fallthrough]];
+    case READ_STATUS_FAILED:
+        assert(fail_check_block);
+        break;
+    case READ_STATUS_INVALID:
+        break;
+    }
+}