Adds a fuzz target that directly calls ConnectBlock with fJustCheck set to true, so it hits block/transaction validation without writing undo data or updating the chainstate.
This PR is essentially #34651 with some minor tweaks and style cleanups. Additional validation harnesses (e.g. #34895) could build on this test's setup.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35850.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | nervana21 |
| Concept ACK | Crypt-iQ, ismaelsadeeq, brunoerg |
| Stale ACK | dergoegge |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
ConsumeTransaction(fuzzed_data_provider, additional_txins, true, target_height) in src/test/fuzz/connect_block.cpp<sup>2026-08-13 17:09:09</sup>
utACK 020bede7c2d3e7a764ed51344925fcfe2300b3cf
A follow up could be to investigate if we can make this harness find https://bitcoincore.org/en/2026/05/05/disclose-cve-2024-52911/ (perhaps the only thing missing here is to actually use the multi-threaded checkqueue).
Concept ACK, will review
Concept ACK, thanks for picking it up.
Concept ACK
374 | + 375 | + if (!coinbase) { 376 | + // Create spending scripts for all CTxOuts so they can be spent in later 377 | + // transactions. Do it here as the transaction hash is definitive. 378 | + for (int i = 0; i < num_outputs; i++) { 379 | + additional_txins.emplace_back(GetSpendingScript(*res, i));
In LoadCurrentBlock we filter before adding to g_spend_candidate_txins. Could we use the same idea here? Otherwise ConsumeTransaction can pollute additional_txins with scripts that are not spendable.
Also, the existing LoadCurrentBlock skip only checks the OP_RETURN prefix, so oversized unspendable scripts can be added there. Prefer scriptPubKey.IsUnspendable() at both insert sites so spend candidates match what CCoinsViewCache::AddCoin would keep.
<details><summary>suggested patch</summary>
diff --git a/src/test/fuzz/connect_block.cpp b/src/test/fuzz/connect_block.cpp
index 86eccde4f1..08ab22550b 100644
--- a/src/test/fuzz/connect_block.cpp
+++ b/src/test/fuzz/connect_block.cpp
@@ -42,7 +42,7 @@ TestingSetup* g_setup;
static std::vector<std::shared_ptr<CBlock>> g_blocks;
/** Set of block hashes in g_blocks */
static std::set<uint256> g_existing_block_hashes;
-/** CTxIns for spending outputs (excluding OP_RETURN), which can be unspent, already spent, or an immature coinbase. */
+/** CTxIns for spending outputs (excluding unspendable outputs), which can be unspent, already spent, or an immature coinbase. */
static std::vector<CTxIn> g_spend_candidate_txins;
/** Static P2SH_OP_TRUE script */
static const CScript P2SH_OP_TRUE = CScript() << OP_HASH160 << ToByteVector(ScriptHash(CScript() << OP_TRUE)) << OP_EQUAL;
@@ -71,18 +71,17 @@ static void InitTaprootScript()
}
/**
- * Given a transaction and an output index, create a CTxIn that can be used to
- * spend it (if possible).
+ * Given a transaction and a spendable output index, create a CTxIn that can be
+ * used to spend it.
*/
static CTxIn GetSpendingScript(const CTransaction& tx, uint32_t vout_index)
{
Assert(vout_index < tx.vout.size());
const CTxOut& output = tx.vout[vout_index];
- CTxIn res{COutPoint(tx.GetHash(), vout_index)};
- if (output.scriptPubKey.size() >= 1 && output.scriptPubKey[0] == OP_RETURN)
- return res;
+ Assert(!output.scriptPubKey.IsUnspendable());
+ CTxIn res{COutPoint(tx.GetHash(), vout_index)};
if (output.scriptPubKey == P2WSH_OP_TRUE) {
res.scriptSig = CScript();
res.scriptWitness.stack.push_back(WITNESS_STACK_ELEM_OP_TRUE);
@@ -98,6 +97,13 @@ static CTxIn GetSpendingScript(const CTransaction& tx, uint32_t vout_index)
return res;
}
+/** Add a spend-candidate CTxIn unless the output is unspendable. */
+static void MaybeAddSpendCandidate(std::vector<CTxIn>& pool, const CTransaction& tx, uint32_t vout_index)
+{
+ Assert(vout_index < tx.vout.size());
+ if (tx.vout[vout_index].scriptPubKey.IsUnspendable()) return;
+ pool.push_back(GetSpendingScript(tx, vout_index));
+}
/**
* Read the block from the BlockManager and add it to g_blocks and g_existing_block_hashes.
@@ -119,11 +125,7 @@ static void LoadCurrentBlock(Chainstate& chainstate, CBlockIndex* current_block)
// Iterate all transaction outputs.
for (const auto& tx : g_blocks[current_block->nHeight]->vtx) {
for (uint32_t vout_index{0}; vout_index < tx->vout.size(); ++vout_index) {
- auto& vout = tx->vout[vout_index];
- // Do not keep OP_RETURN outputs as they are not spendable.
- if (vout.scriptPubKey.size() >= 1 && vout.scriptPubKey[0] == OP_RETURN) continue;
- // Create the CTxIn that can be used to spend this output.
- g_spend_candidate_txins.push_back(GetSpendingScript(*tx, vout_index));
+ MaybeAddSpendCandidate(g_spend_candidate_txins, *tx, vout_index);
}
}
}
@@ -373,10 +375,10 @@ CTransactionRef ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider,
auto res = MakeTransactionRef(tx);
if (!coinbase) {
- // Create spending scripts for all CTxOuts so they can be spent in later
+ // Create spending scripts for spendable CTxOuts so they can be spent in later
// transactions. Do it here as the transaction hash is definitive.
for (int i = 0; i < num_outputs; i++) {
- additional_txins.emplace_back(GetSpendingScript(*res, i));
+ MaybeAddSpendCandidate(additional_txins, *res, i);
}
}
</details>
Took this suggestion, thanks. This should also work a bit better for the two activate best chain targets I'm planning to add later.
tACK 020bede7c2d3e7a764ed51344925fcfe2300b3cf
Left a non-blocking suggestion that can also be considered as a follow-up.
343 | + // Read CAmount to spend. 344 | + tx.vout[i].nValue = fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(-10, 50 * COIN + 10); 345 | + 346 | + // Read scriptPubKey type into one of the valid types. 347 | + switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 4)) { 348 | + case 0:
CallOneOf should be shorter and clearer, no?
True. Fixed, thanks.
431 | + block.hashMerkleRoot = BlockMerkleRoot(block); 432 | + // Let the fuzzer mutate hashMerkleRoot. 433 | + if (fuzzed_data_provider.ConsumeBool()) { 434 | + block.hashMerkleRoot = ConsumeUInt256(fuzzed_data_provider); 435 | + } 436 | +
From the linked coverage, (misread the coverage)HaveCoin always returns true. So would it also make sense to mutate the view here to hit the false-y case?
Might still be interesting to mutate the view directly
ConnectBlock assumes the coins view matches the parent's UTXO set, so by mutating it we'd be testing connecting a block with a "dishonest" UTXO set, yes?
I played with this idea a bit, but couldn't arrive at anything that seemed worth including for this harness specifically. We do hit some missing coin coverage already by mutating the prevouts. And then all the coins_view targets fuzz the views directly.
I can try some different variations and run them to see what comes up. Also, let me know if I'm not understanding and you have something in mind.
ConnectBlock assumes the coins view matches the parent's UTXO set, so by mutating it we'd be testing connecting a block with a "dishonest" UTXO set, yes?
By parent do you mean parent block? I wasn't trying to go for anything like a dishonest UTXO set -- I just noticed that this branch was uncovered and was wondering how we'd go about hitting it. The only thing I could come up with is if the harness could insert into the view as well?
Never mind, I was mixing up CoinsTip and the view (active_coins). What you're saying makes sense now.
The only thing I could come up with is if the harness could insert into the view as well?
So after consuming a block, we could sometimes AddCoin() on active_coins for an output of a tx in that block, so HaveCoin sees that outpoint as unspent. The other option could be to add to the fuzzed block itself. Not in ConsumeBlock() but after that before building new_index. Something like:
if (fuzzed_data_provider.ConsumeBool()) {
const auto& duplicates = g_blocks.back()->vtx;
block.vtx.push_back(duplicates[fuzzed_data_provider.ConsumeIntegralInRange<size_t>(1, duplicates.size() - 1)]);
}
That should work too if I'm not missing something. And feels more straightforward to me. What do you think?
Up to you, either way works. I tested using a duplicate from g_blocks.back()->vtx and checked it works.
471 | + BlockValidationState state; 472 | + bool connected = active_chainstate.ConnectBlock(block, 473 | + state, 474 | + &new_index, 475 | + active_coins, 476 | + /*fJustCheck=*/true);
deterministic-fuzz-coverage reports this as fully deterministic, AFL++ reports ~78% stability. Maybe this is due to some fields that are carried over that don't affect branching (at least in my corpus of ~55K inputs): num_blocks_total, time_check, time_forks, signature cache?, etc
Perhaps setting min_validation_cache=true on TestOps can solve this stability issue? My guess is that without it, chain manager's script exec cache might be persisting?
I checked this out (including min_validation_cache), and none of these are responsible for the low stability. So I think I'll run a pass to find the unstable branches in AFL++
Pretty sure @brunoerg has the right idea here actually. If I force fCacheResults to be false, the instability goes away (99.1%). So it is related to the script validation cache.
The problem is that min_validation_cache=true doesn't fully disable the cache because the setup in cuckoocache.h sets the table size to a minimum of 2. So iterations can still end up using the stored script checks and the AFL++ instability is still there.
Afaict, this instability wouldn't result in non-reproducible test cases so I was planning to leave this as is. If you guys prefer, I can add some sort of test-only clear/reset method (in cuckoocache.h I think) to use between every iteration.
If I force fCacheResults to be false, the instability goes away (99.1%).
I checked this and you are right, nice catch.
Afaict, this instability wouldn't result in non-reproducible test cases so I was planning to leave this as is.
I guess this is because the cache only stores validity? I'm a bit confused why the rust script didn't detect this since I would expect the check here to cause different branch counts?
If you guys prefer, I can add some sort of test-only clear/reset method (in cuckoocache.h I think) to use between every iteration.
I'm in favor of this even if it makes the diff bigger for the sole purpose of not messing up the coverage feedback. Currently setValid in SignatureCache is private though...
Looking at it more, the signature cache doesn't need to be reset since we're not dealing with signatures. Diff is small:
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index e25f691341..b75e8fe78f 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -483,6 +483,13 @@ public:
}
return false;
}
+
+ void TestOnlyReset()
+ {
+ table.clear();
+ epoch_flags.clear();
+ setup(0);
+ }
};
} // namespace CuckooCache
diff --git a/src/test/fuzz/connect_block.cpp b/src/test/fuzz/connect_block.cpp
index ca6b443811..0d4fd7efd7 100644
--- a/src/test/fuzz/connect_block.cpp
+++ b/src/test/fuzz/connect_block.cpp
@@ -467,6 +467,8 @@ FUZZ_TARGET(connect_block, .init = initialize_connect_block)
active_coins,
/*fJustCheck=*/true);
Assert(connected == state.IsValid());
+
+ g_setup->m_node.chainman->m_validation_cache.m_script_execution_cache.TestOnlyReset();
}
} // namespace
Lgtm, happy to add that. I'll document in setup() that this test-only reset is the exception to calling it once.
I guess this is because the cache only stores validity? I'm a bit confused why the rust script didn't detect this since I would expect the check here to cause different branch counts?
Yeah it only caches that the scripts succeeded. Re-running an input just does the checks again and should get the same result.
I think the coverage determinism script doesn’t see a change in branch counts because it never re-runs the same input in the same process, unlike AFL++ persistent mode. For the individual input runs, the cache starts empty both times. For the shuffled corpus runs, it seems the total cache hits/misses doesn't depend on the order the inputs are run. Each successful hashCacheEntry just misses once and hits every time after that.
I see, makes perfect sense. Thanks for the explanation.
187 | + * coins from mature blocks. Otherwise the mined chain only contains 188 | + * coinbase transactions. 189 | + */ 190 | +void AddExtraTxsToMempool(TestingSetup& setup) 191 | +{ 192 | + Assert(Assert(Assert(setup.m_node.chainman)->ActiveChainstate().GetMempool())->size() == 0);
nit: what about removing the two inner Assert, letting them panic if something is nullptr, and Assert on the size?
284 | + // Some harnesses want to explicitly read coinbase transactions from input. 285 | + if (coinbase) { 286 | + // vin size is hardcoded. 287 | + tx.vin.resize(1); 288 | + tx.vin[0].prevout.SetNull(); 289 | + tx.vin[0].nSequence = CTxIn::MAX_SEQUENCE_NONFINAL;
Good catch. Dropped it for now. I can figure out what to do with this and nLockTime later when the forceValidBlock option is introduced.
422 | + block.vtx.push_back(ConsumeTransaction(fuzzed_data_provider, additional_txins)); 423 | + } 424 | + 425 | + // Commit witness. 426 | + if (num_tx > 0) { 427 | + g_setup->m_node.chainman->GenerateCoinbaseCommitment(block, nullptr);
nit: this could be set only sometimes, since it's optional if all the other tx don't have a witness
Yeah makes sense. Switched to ConsumeBool().
434 | + block.hashMerkleRoot = ConsumeUInt256(fuzzed_data_provider); 435 | + } 436 | + 437 | + // Read the nonce from the input and avoid reusing a setup block hash. 438 | + block.nNonce = fuzzed_data_provider.ConsumeIntegral<uint32_t>(); 439 | + while (g_existing_block_hashes.contains(block.GetHash())) {
Is this possible? I could be mis-reading: if block builds off g_blocks.back() it should be impossible for hashes to match?
Somehow got confused here and thought the fuzzer would maybe come up with duplicates. But yeah that's not gonna happen. I'm not yet sure if I'll even need this check for the later two harnesses either...
This has good coverage, still need to grok the taproot logic a little better.
Co-authored-by: marcofleon <marleo23@proton.me>
Thanks everyone for the feedback and sorry for taking a bit to respond. I took most suggestions. I'm running the target again to get a shiny new coverage report.
A follow up could be to investigate if we can make this harness find https://bitcoincore.org/en/2026/05/05/disclose-cve-2024-52911/ @dergoegge I'll look into this more, but would this not be prevented by our fuzz determinism in the testing setup? If so, maybe we can try it after adding a little
FUZZ_NONDETERMINISM=1.
Here is the coverage after re-running.
tACK 2900767806c22edca99e2e4fe212bbdcf42962a0
Thanks for taking the review comments.
Since last tACK:
IsUnspendable() at both spend candidate insert sitesCallOneOfGenerateCoinbaseCommitment optionalnSequence pinThere are still some open discussions. Happy to review again after those land or are dropped.
I'll look into this more, but would this not be prevented by our fuzz determinism in the testing setup? If so, maybe we can try it after adding a little
FUZZ_NONDETERMINISM=1.
Yes, see also #31841#pullrequestreview-2702662698