fuzz: exercise the transaction-handling path in process

HowHsu commented at 8:57 AM on June 8, 2026: contributor

Problem

process_message and process_messages keep the node in IBD (ResetIbd()) and mine their coinbases with the default bare-OP_TRUE output script. As a result net_processing returns early at the IsInitialBlockDownload() check and never reaches the transaction-handling path; and even if it did, a tx spending a bare-OP_TRUE coinbase is rejected as NONSTANDARD by ValidateInputsStandardness. The reused mempool therefore always stays empty and that path is never exercised.

Changes

Both targets now get the same treatment:

Toggle IBD from the test input — a bool decides whether to also JumpOutOfIbd(), exercising both the IBD and non-IBD paths. In process_message it is consumed last, so existing corpus entries read false and are unchanged. In process_messages the messages run in a loop, so the bool must be consumed first (see the corpus note below).
Use a spendable P2WSH_OP_TRUE coinbase — both anyone-can-spend (an OP_TRUE witness, no signature) and a standard witness output, so a fuzz-built tx spending a mature coinbase can actually be accepted into the mempool.
Reset the rng before rebuilding (preparation) — rebuilding the chainman (and, in the next commit, the mempool) consumes the global PRNG. Reset it with MakeRandDeterministicDANGEROUS() first so the rebuild is deterministic across iterations. Mirrors the cmpctblock harness.
Reset the reused mempool — now that the mempool can become non-empty, rebuild it together with the chainman in ResetChainmanAndMempool() when the block index grew or the mempool changed. A dirty mempool is detected by its sequence number rather than its size, since a tx can be added and removed within one iteration (leaving the size unchanged).

Corpus note

In process_messages the IBD bool is consumed before the message loop (first integral read), which shifts the FuzzedDataProvider layout. Existing process_messages corpus entries can be migrated by appending a single 0x00 byte at the end (read as false, keeping the IBD path); every other consumed value stays the same. This is a qa-assets change accompanying this PR.

DrahtBot added the label Fuzzing on Jun 8, 2026

DrahtBot commented at 8:57 AM on June 8, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35482.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35675 (mining: add block template manager by ismaelsadeeq)
#35581 (node: add block template manager and track waitNext fee inflow by ismaelsadeeq)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko commented at 9:09 AM on June 10, 2026: member

Hmm, what makes the fuzz target produce valid blocks, but not valid transactions? Is it that the prevhash can be solved by a fuzz engine for some reason, but the prevout can not? I guess that could be, because one may be a linear memory compare, and the other a map lookup?

Not sure what to do here, but an alternative could be to add the mempool reset logic and hope that fuzz engines in the future will be smart enough to solve a map lookup?

sedited commented at 9:33 AM on June 10, 2026: contributor

I think I would also prefer if the mempools are just reset instead. Having an empty mempool at all times shouldn't be an invariant of the test.

HowHsu force-pushed on Jun 11, 2026

maflcko commented at 8:52 AM on June 11, 2026: member

I don't think this change makes sense. Can you please answer my previous questions why this code is not hit?

I know the answer, but the burden to explain pull requests is on the author.

Hint: You'll have to look at the runtime coverage.

Absent that, I don't think this change makes sense, because it is blindly modifying dead/unreachable code without a context or goal.

<!-- 019eb5c9-2420-7d01-88dd-bb944b831d13 # in dir /tmp/tmp.a9CcqXNjti/git_dir# fuzz process_message tx. 101 history 102 git diff 103 git diff -U1 104 history root@21d4968c4719:/tmp/tmp.a9CcqXNjti/git_dir# git diff -U1 diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index f5d5989..6bfcb7a 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -6,3 +6,5 @@ #include <banman.h> +#include <coins.h> #include <consensus/consensus.h> +#include <core_io.h> #include <kernel/chainparams.h> @@ -45,2 +47,3 @@ TestingSetup* g_setup; std::string_view LIMIT_TO_MESSAGE_TYPE{}; +std::vector<COutPoint> g_mature_coinbase_outpoints; @@ -52,5 +55,10 @@ void ResetChainman(TestingSetup& setup) setup.LoadVerifyActivateChainstate(); + g_mature_coinbase_outpoints.clear(); for (int i = 0; i < 2 * COINBASE_MATURITY; i++) { node::BlockCreateOptions options; - MineBlock(setup.m_node, options); + options.coinbase_output_script = P2WSH_OP_TRUE; + const COutPoint coinbase_outpoint{MineBlock(setup.m_node, options)}; + if (i < COINBASE_MATURITY) { + g_mature_coinbase_outpoints.push_back(coinbase_outpoint); + } } @@ -89,2 +97,24 @@ FUZZ_TARGET(process_message, .init = initialize_process_message) + Assert(!g_mature_coinbase_outpoints.empty()); + + const COutPoint prevout{PickValue(fuzzed_data_provider, g_mature_coinbase_outpoints)}; + CAmount coin_value; + { + LOCK(cs_main); + coin_value = node.chainman->ActiveChainstate().CoinsTip().AccessCoin(prevout).out.nValue; + } + Assert(coin_value > 1); + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vin[0].prevout = prevout; + mtx.vin[0].scriptWitness.stack = {WITNESS_STACK_ELEM_OP_TRUE}; + mtx.vout.emplace_back(coin_value - 1, P2WSH_OP_TRUE); + const CTransactionRef tx{MakeTransactionRef(mtx)}; + + LOCK(cs_main); + const auto tx_result{node.chainman->ProcessTransaction(tx)}; + Assert(tx_result.m_result_type == MempoolAcceptResult::ResultType::VALID); + std::cout << EncodeHexTx(*tx) << std::endl; + Assert(node.mempool->size() == 0); + // Reset, so that dangling pointers can be detected by sanitizers.

HowHsu commented at 9:10 AM on June 11, 2026: contributor

Hmm, what makes the fuzz target produce valid blocks, but not valid transactions? Is it that the prevhash can be solved by a fuzz engine for some reason, but the prevout can not? I guess that could be, because one may be a linear memory compare, and the other a map lookup?

Not sure what to do here, but an alternative could be to add the mempool reset logic and hope that fuzz engines in the future will be smart enough to solve a map lookup?

I measured this on the process_message corpus: out of 2386 inputs, 90 grow the block index (so the fuzz engine does solve a valid hashPrevBlock), but 0 make the mempool non-empty. It's probably not prevhash-vs-prevout: both are unordered_map lookups (BlockMap and the UTXO set), about equally hard, and those 90 hits show the engine can solve that kind of key. The real difference is signature verification. A block reaches the block index with no script checks at all — CheckBlock/AcceptBlock only verify (simplified) PoW + merkle + prevhash, and CheckInputScripts only runs later in ConnectBlock (where coinbase is skipped) — so an empty block with just a coinbase needs no signatures. A transaction, on the other hand, must pass CheckInputScripts to enter the mempool, and a fuzz engine can't brute-force a valid signature.

maflcko commented at 9:15 AM on June 11, 2026: member

No, that is the wrong explanation and ai slop. A signature isn't needed for OP_TRUE.

Please don't use llm bots if you don't understand their output. You are neither helping yourself, nor anyone else.

HowHsu commented at 9:16 AM on June 11, 2026: contributor

No, that is the wrong explanation and ai slop. A signature isn't needed for OP_TRUE.

Please don't use llm bots if you don't understand their output. You are neither helping yourself, nor anyone else.

Need sometime to investigate it.

maflcko commented at 9:20 AM on June 11, 2026: member

Need sometime to investigate it.

Sure, take all the time you need. I am happy to answer any questions you may have.

Also, for future reference, copy-pasting LLM output without marking it as such is against the guidelines: See https://github.com/bitcoin/bitcoin/pull/35386/changes#diff-0db3c9d8a653e398b629930edc9e7dcde74514ef45c26f02a7bd51157d52dcee

HowHsu commented at 1:54 PM on June 11, 2026: contributor

No, that is the wrong explanation and ai slop. A signature isn't needed for OP_TRUE.

Please don't use llm bots if you don't understand their output. You are neither helping yourself, nor anyone else.

Still investigating. The first blocker I noticed from the coverage report is that the IBD state seems fixed. Shouldn't it be controlled by the test input so we can cover both the IBD and !IBD code paths?

The second one is the prevout, I'm gonna construct a valid one to pass it.

maflcko commented at 2:08 PM on June 11, 2026: member

Still investigating. The first blocker I noticed from the coverage report is that the IBD state seems fixed. Shouldn't it be controlled by the test input so we can cover both the IBD and !IBD code paths?

Yeah, see:

src/test/util/validation.cpp:void TestChainstateManager::ResetIbd()
src/test/util/validation.cpp:void TestChainstateManager::JumpOutOfIbd()

The second one is the prevout, I'm gonna construct a valid one to pass it.

I actually don't know if the prevout is the problem. If a fuzz engine can solve the prevhash, it should be able to solve the prevout?

I think there is at least one more other reason why it fails...

HowHsu commented at 9:05 AM on June 12, 2026: contributor

Still investigating. The first blocker I noticed from the coverage report is that the IBD state seems fixed. Shouldn't it be controlled by the test input so we can cover both the IBD and !IBD code paths?

Yeah, see:
src/test/util/validation.cpp:void TestChainstateManager::ResetIbd()
src/test/util/validation.cpp:void TestChainstateManager::JumpOutOfIbd()
The second one is the prevout, I'm gonna construct a valid one to pass it.

I actually don't know if the prevout is the problem. If a fuzz engine can solve the prevhash, it should be able to solve the prevout?

I think there is at least one more other reason why it fails...

Finally found it: ValidateInputsStandardness() --> Solver() returns TxoutType::NONSTANDARD the rootcause is the default coinbase_output_script is CScript() << OP_TRUE. From fuzz/cmpctblock.cpp, I found P2WSH_OP_TRUE is good to pass it.

maflcko commented at 9:39 AM on June 12, 2026: member

Jup.

Going further, I don't know (haven't tried) if a fuzz engine can solve this. The hash and the havoc effect makes this a bit difficult, but I think you can manually copy-paste a hex transaction into a fuzz input.

If you want to do that, I've pushed https://github.com/maflcko/DrahtBot/commit/bef76857851c4166bde3204b98d065578c89139a#diff-020ce280c4497d9badfc07d9034d427afaaed448e7289604e207e07bb03b654bR565 (untested, experimental)

HowHsu commented at 12:07 PM on June 12, 2026: contributor

Jup.

Going further, I don't know (haven't tried) if a fuzz engine can solve this. The hash and the havoc effect makes this a bit difficult, but I think you can manually copy-paste a hex transaction into a fuzz input.

If you want to do that, I've pushed maflcko/DrahtBot@bef7685#diff-020ce280c4497d9badfc07d9034d427afaaed448e7289604e207e07bb03b654bR565 (untested, experimental)

Yes, it's hard to let the fuzz engine solve it, previously I hacked the code to manually construct a Tx for connman.ProcessMessagesOnce. I 'll look into your code, thanks!

HowHsu commented at 4:09 PM on June 12, 2026: contributor

Jup.

Going further, I don't know (haven't tried) if a fuzz engine can solve this. The hash and the havoc effect makes this a bit difficult, but I think you can manually copy-paste a hex transaction into a fuzz input.

If you want to do that, I've pushed maflcko/DrahtBot@bef7685#diff-020ce280c4497d9badfc07d9034d427afaaed448e7289604e207e07bb03b654bR565 (untested, experimental)

Using your tool (plus a small local modification), I found the following input:

 74780066e604002000702b01c0004200ccf6020008005c5f02000000000101748d0a3aa658ffd81616ff63f7037a1e0bcc72b5f9bc051c903d990de2350a150000000000ffffffff0118ee052a010000002200204ae81572f06e1b88fd5c5ced7a1a000945432e83e1551e6f721ee9c00b8cc33260010151000000005c5f013a65786302ac87ffff07ffffff7c0000030301fb03ffff043b00747505000100ff0200001d0002d9270c0000fefefefafe81333461504b030104000000fb000161706900000000000000ff000000

It successfully reaches the point where a transaction is accepted into the mempool. The input depends on my local changes, though, so it's currently 205 bytes. I'll push the code tomorrow so the result can be reproduced.

HowHsu renamed this:
~~fuzz: assert the reused mempool stays empty in process_message(s)~~
fuzz: exercise the transaction-handling path in process_message
on Jun 14, 2026

HowHsu force-pushed on Jun 14, 2026

HowHsu commented at 2:31 PM on June 14, 2026: contributor

Hi @maflcko,

I ran into a problem with process_messages: adding a ConsumeBool() for the IBD state invalidates all existing process_messages corpus entries. What's the usual practice for handling this?

HowHsu force-pushed on Jun 14, 2026

DrahtBot added the label CI failed on Jun 14, 2026

DrahtBot removed the label CI failed on Jun 14, 2026

maflcko commented at 6:10 AM on June 15, 2026: member

Usually that is not a problem and just ignored, because the hope is that the fuzz engines will re-discover everything.

However, it should also be possible to manually transform all fuzz inputs with the same tool/concept I shared above. Instead of manually injecting a bool in the right place in a single fuzz input, one would have to do it for all fuzz inputs. It should be possible with a script, but I've not yet written it and thus never tried it.

maflcko commented at 6:39 AM on June 15, 2026: member

I've pushed an update to the inverse_fdp, so that the main.rs file is stand-alone and only holds fn main. Now, it should be trivial to modify the C++ code to write to stdout (when run on all old fuzz inputs) the correct main.rs file with a main function, which will then write (when run) the new correct fuzz input format for all fuzz inputs to a new folder.

HowHsu marked this as a draft on Jun 15, 2026

in src/test/fuzz/process_message.cpp:147 in 3fa4c3591e

 143 | @@ -137,8 +144,9 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
 144 |      node.validation_signals->SyncWithValidationInterfaceQueue();
 145 |      node.validation_signals->UnregisterValidationInterface(node.peerman.get());
 146 |      node.connman->StopNodes();
 147 | -    if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())) {
 148 | -        // Reuse the global chainman, but reset it when it is dirty
 149 | +    if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size()) || node.mempool->size() > 0) {

Crypt-iQ commented at 1:28 PM on June 15, 2026:

|| node.mempool->size() > 0 branch won't be taken and will lead to non-determinism if a tx is added & removed and the mempool size doesn't change (I forget how this can happen, maybe with time-based eviction?). Also, the rng needs to be reset as creating a fresh mempool will still use the dirty rng state. See for reference the cmpctblock harness: it fixes the first problem by comparing mempool sequence numbers, and the second problem by calling MakeRandDeterministicDANGEROUS.

HowHsu commented at 7:41 AM on June 17, 2026:

Checked that, you are right, thanks, I'll update it later.

HowHsu commented at 3:25 AM on June 18, 2026:

Updated.

HowHsu force-pushed on Jun 17, 2026

HowHsu renamed this:
~~fuzz: exercise the transaction-handling path in process_message~~
fuzz: exercise the transaction-handling path in process_message(s)
on Jun 17, 2026

HowHsu marked this as ready for review on Jun 18, 2026

in src/test/fuzz/process_message.cpp:130 in 88f10bdc3b outdated

 124 | @@ -125,6 +125,10 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
 125 |      connman.FlushSendBuffer(p2p_node);
 126 |      (void)connman.ReceiveMsgFrom(p2p_node, std::move(net_msg));
 127 |  
 128 | +    if (fuzzed_data_provider.ConsumeBool()) {
 129 | +        chainman.JumpOutOfIbd();
 130 | +    }

maflcko commented at 7:30 AM on June 18, 2026:

88f10bdc3bcc6ae8383ede05a525d0e3c1db22e1: I don't understand this commit message. It claims:

The targets keep the node in IBD via ResetIbd()

However, the target code literally says "any time to successfully reset ibd" from commit fa0a864b383a794910cdb215bb836dae12357bc6. So this seems like either a bug was added in commit fa0a864b383a794910cdb215bb836dae12357bc6 (or later), and you are effectively reverting that commit.

It could make sense to properly explain where the bug was added and then also mention that this reverts that commit?

maflcko commented at 7:30 AM on June 18, 2026:

Also, there are other fuzz targets that use this pattern, so if they are affected by the same bug, they should be fixed as well.

HowHsu commented at 11:02 AM on June 18, 2026:

88f10bd: I don't understand this commit message. It claims:

The targets keep the node in IBD via ResetIbd()

However, the target code literally says "any time to successfully reset ibd" from commit fa0a864. So this seems like either a bug was added in commit fa0a864 (or later), and you are effectively reverting that commit.

It could make sense to properly explain where the bug was added and then also mention that this reverts that commit?

Updated.

HowHsu commented at 11:05 AM on June 18, 2026:

Also, there are other fuzz targets that use this pattern, so if they are affected by the same bug, they should be fixed as well.

I think it can be addressed in a separate PR which focuses on this bug in other targets.

maflcko commented at 2:38 PM on June 18, 2026:

The other target is using the exact same pattern. For every change you submit, please think about minimizing the review load.

It is easier to review one change once, instead of reviewing it several times, spread over time and several pull requests

HowHsu commented at 3:38 PM on June 18, 2026:

The other target is using the exact same pattern. For every change you submit, please think about minimizing the review load.

It is easier to review one change once, instead of reviewing it several times, spread over time and several pull requests

Got it. I'll take a closer look at the !IBD code path changes in p2p_handshake tomorrow. It seems those changes aren't specifically for exercising the transaction-handling path, so the PR title and description will likely need to be updated accordingly.

HowHsu commented at 2:24 PM on June 23, 2026:

The other target is using the exact same pattern. For every change you submit, please think about minimizing the review load.

It is easier to review one change once, instead of reviewing it several times, spread over time and several pull requests

Updated

in src/test/fuzz/process_message.cpp:54 in 0c8546b343 outdated

  48 | @@ -44,19 +49,28 @@ namespace {
  49 |  TestingSetup* g_setup;
  50 |  std::string_view LIMIT_TO_MESSAGE_TYPE{};
  51 |  
  52 | -void ResetChainman(TestingSetup& setup)
  53 | +void ResetChainmanAndMempool(TestingSetup& setup)
  54 |  {
  55 | -    SetMockTime(setup.m_node.chainman->GetParams().GenesisBlock().Time());
  56 | +    SetMockTime(Params().GenesisBlock().Time());

maflcko commented at 7:30 AM on June 18, 2026:

nit: This seems like the wrong style change for a few reasons:

When there is a params local, the local should be preferred, not the global Params().
This function is duplicated thrice, so maybe a std::vector<std::pair<COutPoint, CAmount>> ResetChainmanAndMempool(TestingSetup& setup) should sit in src/test/util/validation.h. (This refactor can be done in a preparatory commit).
Mixing the SetMockTime global mutator with the FakeNodeClock below seems confusing. Ideally only one is used. I haven't tested this, but my recommendation would be:

auto& GetFakeNodeClock(){
static FakeNodeClock g_fake_clock{};
return g_fake_clock;
}

and then call this helper in the init and in the target code.

HowHsu commented at 3:14 PM on June 18, 2026:

Mixing the SetMockTime global mutator with the FakeNodeClock below seems confusing. Ideally only one is used. I haven't tested this, but my recommendation would be:
auto& GetFakeNodeClock(){
static FakeNodeClock g_fake_clock{};
return g_fake_clock;
}
and then call this helper in the init and in the target code.

This sounds like a separate topic. Could we first replace SetMockTime with FakeNodeClock in this mempool reset PR, and then do the broader FakeNodeClock refactoring in a follow-up PR?

HowHsu commented at 2:26 PM on June 23, 2026:

nit: This seems like the wrong style change for a few reasons:

When there is a params local, the local should be preferred, not the global Params().

This function is duplicated thrice, so maybe a std::vector<std::pair<COutPoint, CAmount>> ResetChainmanAndMempool(TestingSetup& setup) should sit in src/test/util/validation.h. (This refactor can be done in a preparatory commit).

Mixing the SetMockTime global mutator with the FakeNodeClock below seems confusing. Ideally only one is used. I haven't tested this, but my recommendation would be:
auto& GetFakeNodeClock(){
static FakeNodeClock g_fake_clock{};
return g_fake_clock;
}
and then call this helper in the init and in the target code.

I've done this and included it as a commit in this PR, though it still feels like it belongs to a separate topic.

maflcko commented at 2:39 PM on July 16, 2026:

in ef18579f107d8b32b26eeba66d5b43090133370e: Just seeing this commit and I think this is going too far. When I said "Mixing the SetMockTime global mutator with the FakeNodeClock below seems confusing. Ideally only one is used. I haven't tested this, but my recommendation would be:"...

I only meant to apply the suggestion to the fuzz targets that mix SetMockTime and FakeNodeClock: I didn't mean to change this globally. I think the auto& GetFakeNodeClock(){...} should only sit in the fuzz target code that really needs it. All other fuzz targets can just use a local-scope plain FakeNodeClock clock{};, like before.

Crypt-iQ commented at 5:36 PM on July 20, 2026:

c5ab212020980cd206e261d79dd4e6df932743ee: seems like the change to utxo_snapshot can be added back as it fits this pattern

maflcko commented at 7:31 AM on June 18, 2026: member

left two style nits

HowHsu requested review from Crypt-iQ on Jun 18, 2026

HowHsu force-pushed on Jun 18, 2026

DrahtBot added the label CI failed on Jun 18, 2026

HowHsu force-pushed on Jun 18, 2026

in src/test/fuzz/process_messages.cpp:81 in 5e0c1903ec

  77 |      chainman.ResetIbd();
  78 |      chainman.DisableNextWrite();
  79 |  
  80 | +    if (fuzzed_data_provider.ConsumeBool()) {
  81 | +        chainman.JumpOutOfIbd();
  82 | +    }

maflcko commented at 3:08 PM on June 18, 2026:

nit: I wonder if this is the right place. Commit fa0a864b383a794910cdb215bb836dae12357bc6 had this logic in the loop, which would allow a few messages under IBD, and other messages outside IBD.

HowHsu commented at 2:23 PM on June 23, 2026:

Updated

DrahtBot removed the label CI failed on Jun 19, 2026

HowHsu force-pushed on Jun 20, 2026

DrahtBot added the label CI failed on Jun 20, 2026

DrahtBot removed the label CI failed on Jun 20, 2026

HowHsu requested review from maflcko on Jun 22, 2026

DrahtBot added the label Needs rebase on Jun 27, 2026

HowHsu force-pushed on Jun 29, 2026

HowHsu commented at 1:37 PM on June 29, 2026: contributor

Rebased the branch

DrahtBot removed the label Needs rebase on Jun 29, 2026

in src/test/fuzz/process_messages.cpp:109 in ec5c1666b1 outdated

 106 | @@ -107,8 +107,14 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
 107 |          connman.AddTestNode(p2p_node);
 108 |      }
 109 |

Crypt-iQ commented at 2:49 PM on July 14, 2026:

ec5c1666b1812839653e4474c667ef59708a039c:

I'm confused by the commit message. Is it true that IBD is always stuck without this commit? I thought that if a block was mined that extends the tip, the node would exit IBD? Also the "Fixed: ..." makes GH think that this PR is fixing a PR when usually that's reserved for fixing issues?

HowHsu commented at 10:00 AM on July 15, 2026:

ec5c166:

I'm confused by the commit message. Is it true that IBD is always stuck without this commit? I thought that if a block was mined that extends the tip, the node would exit IBD? Also the "Fixed: ..." makes GH think that this PR is fixing a PR when usually that's reserved for fixing issues?

the ResetIbd() call reset it to IBD stage unconditionally, and it is for each input. So if it is a TX type, no new blocks come in and that tx can't be accepted due to the IBD state.

Crypt-iQ commented at 1:51 PM on July 15, 2026:

the ResetIbd() call reset it to IBD stage unconditionally, and it is for each input. So if it is a TX type, no new blocks come in and that tx can't be accepted due to the IBD state.

Yes that makes sense, but the commit message I think is wrong: "never exercises the non-IBD code paths" is false? I think the change itself makes sense.

maflcko commented at 3:16 PM on July 15, 2026:

Yes that makes sense, but the commit message I think is wrong: "never exercises the non-IBD code paths" is false? I think the change itself makes sense.

I think the message is correct, because ResetIbd() forces IBD-mode: assert(IsInitialBlockDownload());. So the fuzz target can never exercise any logic guarded by non-IBD?

Maybe I am missing something? Can you suggest a better wording?

Crypt-iQ commented at 2:28 PM on July 16, 2026:

So the fuzz target can never exercise any logic guarded by non-IBD?

I think this is true of process_message & p2p_handshake, but not of process_messages since a block connecting to the tip will call UpdateIBDStatus:

https://github.com/bitcoin/bitcoin/blob/70d9ec7f3d452789d04dce81dc02db0b3b778bb5/src/validation.cpp#L3097-L3100

I wasn't able to test this behavior since no inputs in qa-assets build on the tip, so I may be off-base.

The message "and never exercises the non-IBD code paths." could be rephrased to "and never exercises the non-IBD transaction-handling code paths for process_message and p2p_handshake. It is possible but unlikely for process_messages to exercise non-IBD transaction-handling code paths if a block is received that builds on the current tip.". Though it's kind of a mouthful.

maflcko commented at 2:43 PM on July 16, 2026:

I wasn't able to test this behavior since no inputs in qa-assets build on the tip, so I may be off-base.

Ah, according to #32822 (comment), there may be some headers building on the tip, but probably no block.

Seems fine to leave the commit message as-is, or change it. No strong opinion.

in src/test/fuzz/process_message.cpp:149 in 9312ba2b73

 145 | @@ -143,7 +146,10 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
 146 |      node.validation_signals->UnregisterValidationInterface(node.peerman.get());
 147 |      node.connman->StopNodes();
 148 |      if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())) {
 149 | -        // Reuse the global chainman, but reset it when it is dirty
 150 | +        // Reuse the global chainman, but reset it when it is dirty. Reset the rng

Crypt-iQ commented at 3:00 PM on July 14, 2026:

9312ba2b7390b456bba2bd930e57d8d7d164b4ca:

The comments in this commit are a bit verbose. I understand this prepares for the following commit, but I don't think this comment in this commit makes sense? If the rng is reset without also creating a fresh mempool, the chainman relies on this iteration's mempool, which is non-deterministic as SaltedTxidHasher / SaltedWtxidHasher are stale. So "rebuild starts from a deterministic state rather than ..." is false.

HowHsu commented at 10:29 AM on July 15, 2026:

Yea, true without the commit next to it. Will update to make the commit more slef-consistent.

in src/test/fuzz/cmpctblock.cpp:9 in dedc8c4bf4

Crypt-iQ commented at 3:04 PM on July 14, 2026:

dedc8c4bf41811d74f76578804aa586c9fe9189a:

iwyu says that several imports can be removed here (coins.h, consensus/consensus.h) as a result of this change. Might be worth running with iwyu and fixing the imports in the touched files?

maflcko commented at 3:21 PM on July 15, 2026:

Might be worth running with iwyu and fixing the imports in the touched files?

For reference, there is an iwyu CI task, which prints the result for all files. This can be used, if needed, because running iwyu locally may produce a different/wrong result.

DrahtBot added the label Needs rebase on Jul 14, 2026

HowHsu force-pushed on Jul 17, 2026

DrahtBot removed the label Needs rebase on Jul 17, 2026

HowHsu force-pushed on Jul 20, 2026

HowHsu requested review from Crypt-iQ on Jul 20, 2026

in src/test/fuzz/process_message.cpp:139 in 80755a0279

 137 | -        // rng first, so ResetChainman() consumes it from a fixed seed.
 138 | +    const auto end_sequence{WITH_LOCK(node.mempool->cs, return node.mempool->GetSequence())};
 139 | +    if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size()) || initial_sequence != end_sequence) {
 140 | +        // Reuse the global chainman and mempool, but reset them when dirty. The
 141 | +        // mempool sequence is compared instead of its size because a tx may be
 142 | +        // added and removed within one iteration, leaving the size unchanged

Crypt-iQ commented at 5:44 PM on July 20, 2026:

80755a0279a4f8293ea3f92c4cb5fa16d28e6b4b: I think the comment here is slightly off because this harness can't remove its own tx so technically comparing sizes is enough (though I slightly prefer the sequence comparison as-is). I think just "Reuse the global chainman and mempool, but reset them when dirty." or no comment is fine, the comment is a bit verbose.

HowHsu commented at 2:36 PM on July 21, 2026:

Good point, I agree the current add-and-remove wording is not proper for process_message. The reason for using the sequence is that it detects mempool change even when the final size is unchanged, e.g. fee eviction, not very sure, but a Tx in-and-evict causes metadata like rollingMinimumFeeRate changes and may affect the code coverage of running the same input the next round. And these tricky cases deserve a comment here for readers IMHO. Maybe change it to because there are tricky cases where some metadata of mempool changes while the mempool size remains 0 and then affect code coverage, e.g. tx eviction

Crypt-iQ commented at 2:53 PM on July 21, 2026:

It's just a personal preference, I do think that in process_message the comment should only mention the size changing (and keep the sequence comparison in code) as I don't think the mempool can be dirtied and 0-size.

HowHsu commented at 3:10 PM on July 21, 2026:

Going over the code again, I think you are right about this, the eviction can't take place in just on inbound message.

in src/test/fuzz/process_message.cpp:60 in 0c2fe89018 outdated

  57 | @@ -57,6 +58,8 @@ void ResetChainman(TestingSetup& setup)
  58 |  }
  59 |  } // namespace
  60 |

Crypt-iQ commented at 5:53 PM on July 20, 2026:

0c2fe890188243ecca9afc914e42aea1a95183f6:

The commit message here seems off because IIRC the chainman in the test setup does not consume the rng, the mempool does. And the commit message implies that without this, the chainman would be non-deterministic which I think is untrue. I think the emphasis should instead be placed on it being a preparatory commit for the mempool change as otherwise this might confuse people who go back and refer to this commit. I might be wrong.

in src/test/fuzz/process_messages.cpp:137 in 80755a0279

 131 | @@ -144,10 +132,14 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
 132 |      node.validation_signals->SyncWithValidationInterfaceQueue();
 133 |      node.validation_signals->UnregisterValidationInterface(node.peerman.get());
 134 |      node.connman->StopNodes();
 135 | -    if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())) {
 136 | -        // Reuse the global chainman, but reset it when it is dirty. Reset the
 137 | -        // rng first, so ResetChainman() consumes it from a fixed seed.
 138 | +    const auto end_sequence{WITH_LOCK(node.mempool->cs, return node.mempool->GetSequence())};
 139 | +    if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size()) || initial_sequence != end_sequence) {
 140 | +        // Reuse the global chainman and mempool, but reset them when dirty. The

Crypt-iQ commented at 5:54 PM on July 20, 2026:

80755a0279a4f8293ea3f92c4cb5fa16d28e6b4b

nit: the comment here is a bit verbose. I think the first sentence is fine, or no comment, and detail in the commit message.

Crypt-iQ commented at 5:55 PM on July 20, 2026: contributor

The code changes seem ok, I think one place can benefit from the time changes (https://github.com/bitcoin/bitcoin/pull/35482#discussion_r3616276175). Left style & accuracy comments.

HowHsu force-pushed on Jul 21, 2026

DrahtBot added the label CI failed on Jul 21, 2026

HowHsu force-pushed on Jul 21, 2026

DrahtBot removed the label CI failed on Jul 21, 2026

fuzz: share a single FakeNodeClock in the chainman-resetting fuzz targets

cmpctblock, process_message, process_messages, and utxo_snapshot rebuild
the chainman from a helper called out of the target body, while the
body itself holds or mutates mocktime through a FakeNodeClock. Since
FakeNodeClock is a LimitOne type, the helper cannot hold one of its
own and reaches for the SetMockTime() global mutator instead, so it is
unclear which of the two owns mocktime.

Add a GetFakeNodeClock() that returns a single process-wide
FakeNodeClock and route these targets through it, replacing their
local FakeNodeClock and genesis-time SetMockTime() calls.

It is initialized with 0s rather than a default-constructed
FakeNodeClock{}, which would read mocktime at whatever point the
static happens to be constructed. Every user sets the time before use
anyway.

2a4ef42d34

test: add helper to reset chainman and mempool

Move the reusable chainman/mempool reset logic from the cmpctblock fuzz target
into test util validation helpers. Return the mature coinbase outputs so
cmpctblock can keep using them while other callers can ignore the result.

2a29cee684

fuzz: let the test input toggle IBD in the p2p fuzz targets

process_message, process_messages and p2p_handshake put the node in
IBD via ResetIbd(), so net_processing returns early at the
IsInitialBlockDownload() check and almost never exercises the non-IBD
transaction-handling code paths behind it.

Only ConnectTip() latches the node back out, through
UpdateIBDStatus(). p2p_handshake connects no block, and in
process_message the single message is the whole iteration, so for
those two the non-IBD paths are out of reach entirely.
process_messages could reach them if one of its messages carried a
block building on the tip, but that is unlikely.

For process_message(s), leaving IBD used to be controllable from the
fuzz input via a jump_out_of_ibd bool that called JumpOutOfIbd().
Commit fa0a864b (#20908) dropped that toggle because mocktime made
it redundant: back then IsInitialBlockDownload() evaluated the tip
timestamp against the current mocked time on every call, so
SetMockTime(ConsumeTime(...)) alone could drive the node in and out
of IBD. That stopped working in #34253, which turned
IsInitialBlockDownload() into a lock-free read of the cached
m_cached_is_ibd flag, latched only by UpdateIBDStatus() on chain
activation; mocktime no longer affects it. p2p_handshake never had
such a toggle.

Restore the toggle. In process_message (a single message), the bool
is consumed last. In process_messages and p2p_handshake, the toggle
lives inside the message loop, as it did before fa0a864b. A latched
bool decides before each message whether to call JumpOutOfIbd(), so
some messages can be handled under IBD and the rest after leaving it.

Fixes: #34253

b11456386b

fuzz: prepare deterministic mempool rebuilds

Reset the global PRNG before the dirty-state reset in process_message
and process_messages.

This prepares the reset path for the following change, which rebuilds
the reused mempool together with the chainman. The mempool construction
path consumes PRNG-backed state, so the rebuild should start from a
fixed seed rather than from the RNG state left by the fuzz input.

d522fd3196

fuzz: reset the reused mempool in process_message(s)

The mempool is reused across iterations of process_message and
process_messages, but it was never reset, so a transaction accepted
into the mempool by one input would leak into the next iteration. A
random payload can produce a transaction that passes
AcceptToMemoryPool.

Rebuild the mempool together with the chainman in
ResetChainmanAndMempool(), called when either the block index grew or
the mempool was modified. The mempool is bound to the chainman at
chainman construction (ChainstateLoadOptions::mempool), so the two
must be reset together: the mempool is rebuilt first, then the
chainman, which re-binds the fresh mempool. This mirrors the
cmpctblock harness.

87b080fe2b

HowHsu force-pushed on Jul 22, 2026

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

Problem

Changes

Corpus note

Code Coverage & Benchmarks

Reviews

Conflicts