fuzz: Add PartiallyDownloadedBlock target #26898

dergoegge commented at 3:39 PM on January 16, 2023: member

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

DrahtBot commented at 3:39 PM on January 16, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, MarcoFalke
Concept ACK	sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jan 16, 2023

in src/test/fuzz/partially_downloaded_block.cpp:67 in 1105efe461 outdated

  62 | +
  63 | +    std::vector<std::pair<uint256, CTransactionRef>> extra_txn;
  64 | +    for (size_t i = 1; i < block->vtx.size(); ++i) {
  65 | +        auto tx{block->vtx[i]};
  66 | +
  67 | +        bool add_to_etxra_txn{fuzzed_data_provider.ConsumeBool()};

mzumsande commented at 7:00 PM on January 18, 2023:

nit: etxra -> extra

in src/blockencodings.cpp:201 in f463998ccb outdated

 196 | @@ -197,7 +197,8 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
 197 |          return READ_STATUS_INVALID;
 198 |  
 199 |      BlockValidationState state;
 200 | -    if (!CheckBlock(block, state, Params().GetConsensus())) {
 201 | +    CheckBlockFn check_block = check_block_mock ? check_block_mock : CheckBlock;
 202 | +    if (!check_block(block, state, Params().GetConsensus(), true, true)) {

mzumsande commented at 7:31 PM on January 18, 2023:

maybe add named arguments for the bools

in src/test/fuzz/partially_downloaded_block.cpp:101 in 1105efe461 outdated

  85 | +    for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) {
  86 | +        bool skip{fuzzed_data_provider.ConsumeBool()};
  87 | +        assert(init_status != READ_STATUS_OK ||
  88 | +               (available.count(i) == 1) == pdb.IsTxAvailable(i));
  89 | +        if (!pdb.IsTxAvailable(i) && !skip && i < block->vtx.size()) {
  90 | +            missing.push_back(block->vtx[i]);

mzumsande commented at 11:28 PM on January 18, 2023:

maybe we could memorize if we decided to skip an unavailable tx (i.e. one we were supposed to include in missing), and later assert that the FillBlock result must not be READ_STATUS_OK in this case?

dergoegge commented at 12:00 PM on January 20, 2023:

Good idea, added an extra commit for this

mzumsande commented at 11:32 PM on January 18, 2023: contributor

Concept ACK, only minor comments

dergoegge force-pushed on Jan 20, 2023

sipa commented at 4:27 PM on January 20, 2023: member

Concept ACK

in src/blockencodings.h:140 in c41f50d389 outdated

 133 | @@ -129,6 +134,11 @@ class PartiallyDownloadedBlock {
 134 |      const CTxMemPool* pool;
 135 |  public:
 136 |      CBlockHeader header;
 137 | +
 138 | +    // Can be overriden for testing
 139 | +    using CheckBlockFn = std::function<bool(const CBlock&, BlockValidationState&, const Consensus::Params&, bool, bool)>;
 140 | +    CheckBlockFn check_block_mock{nullptr};

maflcko commented at 10:07 AM on January 23, 2023:

nit: m_ prefix?

in src/test/fuzz/partially_downloaded_block.cpp:34 in 1136999e0c outdated

  29 | +    g_setup = testing_setup.get();
  30 | +}
  31 | +
  32 | +PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValidationResult> result)
  33 | +{
  34 | +    return [result](const CBlock& block, BlockValidationState& state, const Consensus::Params& params, bool check_pow, bool check_merkle_root) {

maflcko commented at 2:01 PM on January 23, 2023:

nit 1136999e0ce7b13e209ac3c3153ed698938b6f6e: Any reason to mention the unused names, other than state? Seems fragile, given that the names may go out-of-sync in the future or may have compilers warn/error on the dummy symbols.

dergoegge commented at 4:34 PM on January 23, 2023:

Dropped the unused names

in src/test/fuzz/partially_downloaded_block.cpp:92 in 1136999e0c outdated

  87 | +        // compact block (i.e. IsTxAvailable(i) == true) implies that we marked
  88 | +        // that transaction as available above (i.e. available.count(i) > 0).
  89 | +        // The reverse is not true, due to possible compact block short id
  90 | +        // collisions (i.e. available.count(i) > 0 does not imply
  91 | +        // IsTxAvailable(i) == true).
  92 | +        assert(init_status != READ_STATUS_OK ||

maflcko commented at 2:49 PM on January 23, 2023:

1136999e0ce7b13e209ac3c3153ed698938b6f6e nit: Why not if (init_status == READ_STATUS_OK) assert(...? Seems less confusing to have less checks in one assert.

in src/test/fuzz/partially_downloaded_block.cpp:59 in 1136999e0c outdated

  54 | +
  55 | +    CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node)};
  56 | +    PartiallyDownloadedBlock pdb{&pool};
  57 | +
  58 | +    // Set of available transactions (mempool or extra_txn)
  59 | +    std::set<uint16_t> available;

maflcko commented at 2:50 PM on January 23, 2023:

Not that it matters, but a bitvector might use less space and time

dergoegge commented at 4:34 PM on January 23, 2023:

I don't think this is worth it for now, but I might open a follow up if this turns out to significantly improve performance.

in src/test/fuzz/partially_downloaded_block.cpp:96 in 1136999e0c outdated

  91 | +        // IsTxAvailable(i) == true).
  92 | +        assert(init_status != READ_STATUS_OK ||
  93 | +               !pdb.IsTxAvailable(i) || available.count(i) > 0);
  94 | +
  95 | +        bool skip{fuzzed_data_provider.ConsumeBool()};
  96 | +        if (!pdb.IsTxAvailable(i) && !skip && i < block->vtx.size()) {

maflcko commented at 2:51 PM on January 23, 2023:

Why would i ever be larger or equal to vtx.size()? Also, why would BlockTxCount ever be not equal to vtx.size?

dergoegge commented at 4:33 PM on January 23, 2023:

Yea this is not needed, i suspect I this is a remnant of an earlier version of the target.

in src/test/fuzz/partially_downloaded_block.cpp:106 in 1136999e0c outdated

 101 | +    // Mock CheckBlock
 102 | +    bool fail_check_block{fuzzed_data_provider.ConsumeBool()};
 103 | +    auto validation_result{static_cast<BlockValidationResult>(
 104 | +        fuzzed_data_provider.ConsumeIntegralInRange<int>(
 105 | +            (int)BlockValidationResult::BLOCK_RESULT_UNSET,
 106 | +            (int)BlockValidationResult::BLOCK_HEADER_LOW_WORK))};

maflcko commented at 2:55 PM on January 23, 2023:

Using PickValue, like the other enums, would be using less casts

maflcko commented at 2:56 PM on January 23, 2023:

If you want to use casts, I'd recommend std::underlying_type

in src/test/fuzz/partially_downloaded_block.cpp:103 in b350ffd07c outdated

  98 | @@ -96,6 +99,8 @@ FUZZ_TARGET_INIT(partially_downloaded_block, initialize_pdb)
  99 |          if (!pdb.IsTxAvailable(i) && !skip && i < block->vtx.size()) {
 100 |              missing.push_back(block->vtx[i]);
 101 |          }
 102 | +
 103 | +        skipped_missing = skipped_missing || (!pdb.IsTxAvailable(i) && skip);

maflcko commented at 3:00 PM on January 23, 2023:

nit: Use |=?

maflcko approved

maflcko commented at 3:02 PM on January 23, 2023: member

Thanks for working on improved fuzz targets!

I've left some nano-nits that can be ignored. Please let us know if you want this merged or want to address the nits.

review ACK b350ffd07c04d3b499f5960973d664d699341ec1 🎧

<details><summary>Show signature</summary>

Signature:

</details>

[block encodings] Make CheckBlock mockable for PartiallyDownloadedBlock 1429f83770

[block encodings] Avoid fuzz blocking asserts in PartiallyDownloadedBlock 42bd4c7468

[fuzz] Add PartiallyDownloadedBlock target a8ac61ab5e

[fuzz] Assert that omitting missing transactions always fails block reconstruction a1c36275b5

dergoegge force-pushed on Jan 23, 2023

dergoegge commented at 5:18 PM on January 23, 2023: member

@MarcoFalke addressed all your comments. CI failures seem unrelated (restarted the failing jobs).

mzumsande commented at 9:03 PM on January 23, 2023: contributor

Code Review ACK a1c36275b5a27ae685f49ff02dabff0adbf51aa1

in src/test/fuzz/partially_downloaded_block.cpp:121 in a1c36275b5

 116 | +             BlockValidationResult::BLOCK_MUTATED,
 117 | +             BlockValidationResult::BLOCK_MISSING_PREV,
 118 | +             BlockValidationResult::BLOCK_INVALID_PREV,
 119 | +             BlockValidationResult::BLOCK_TIME_FUTURE,
 120 | +             BlockValidationResult::BLOCK_CHECKPOINT,
 121 | +             BlockValidationResult::BLOCK_HEADER_LOW_WORK});

maflcko commented at 11:33 AM on January 24, 2023:

nit: Could move to a common header file?

maflcko approved

maflcko commented at 11:34 AM on January 24, 2023: member

lgtm. Follow-ups can be done in new pulls, if there are any.

re-ACK a1c36275b5a27ae685f49ff02dabff0adbf51aa1 🎼

<details><summary>Show signature</summary>

Signature:

</details>

maflcko merged this on Jan 24, 2023

maflcko closed this on Jan 24, 2023

sidhujag referenced this in commit a8897f6e62 on Jan 24, 2023

in src/test/fuzz/partially_downloaded_block.cpp:80 in a1c36275b5

  75 | +        if (add_to_mempool) {
  76 | +            LOCK2(cs_main, pool.cs);
  77 | +            pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx));
  78 | +            available.insert(i);
  79 | +        }
  80 | +    }

maflcko commented at 9:14 AM on January 25, 2023:

nit: Could make sense to include some extraneous txs that are not in the block in the extra txn or mempool?

in src/test/fuzz/partially_downloaded_block.cpp:96 in a1c36275b5

  91 | +        // that transaction as available above (i.e. available.count(i) > 0).
  92 | +        // The reverse is not true, due to possible compact block short id
  93 | +        // collisions (i.e. available.count(i) > 0 does not imply
  94 | +        // IsTxAvailable(i) == true).
  95 | +        if (init_status == READ_STATUS_OK) {
  96 | +            assert(!pdb.IsTxAvailable(i) || available.count(i) > 0);

maflcko commented at 9:18 AM on January 25, 2023:

nit:

        if (init_status == READ_STATUS_OK && pdb.IsTxAvailable(i)) {
            assert(available.count(i) > 0);

Also, in the comment replace "then" with "and"?

in src/test/fuzz/partially_downloaded_block.cpp:104 in a1c36275b5

  99 | +        bool skip{fuzzed_data_provider.ConsumeBool()};
 100 | +        if (!pdb.IsTxAvailable(i) && !skip) {
 101 | +            missing.push_back(block->vtx[i]);
 102 | +        }
 103 | +
 104 | +        skipped_missing |= (!pdb.IsTxAvailable(i) && skip);

maflcko commented at 9:22 AM on January 25, 2023:

nit:

        if (!pdb.IsTxAvailable(i)) {
            if (skip) { skipped_missing = true; }
            else { missing.push_back(block->vtx[i]); }
        }

maflcko commented at 9:33 AM on January 25, 2023: member

left more nits :sweat_smile:

bitcoin locked this on Jan 25, 2024