fuzz: Extend `spend` coverage #34264

Chand-ra commented at 3:12 PM on January 12, 2026: none

Make the wallet/spend fuzz target stateful and extend coverage for several missing methods.

DrahtBot added the label Fuzzing on Jan 12, 2026

DrahtBot commented at 3:12 PM on January 12, 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/34264.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	frankomosh

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35720 (ipc: Update libmultiprocess subtree and drop fuzz test workaround by maflcko)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)

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.

DrahtBot added the label CI failed on Jan 12, 2026

DrahtBot commented at 5:24 PM on January 12, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20924396327/job/60120935082</sub> <sub>LLM reason (✨ experimental): CI failure due to clang-tidy errors in spend.cpp (unnecessary temporary object in emplace_back and unused-return-value).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

brunoerg commented at 6:30 PM on January 12, 2026: contributor

From CI:

[515/729][5.2s] clang-tidy-21 -p=/home/admin/actions-runner/_work/_temp/build -quiet -load=/tidy-build/libbitcoin-tidy.so /home/admin/actions-runner/_work/_temp/src/test/fuzz/block_index_tree.cpp
1126 warnings generated.
+ echo '^^^ ⚠️ Failure generated from clang-tidy'
+ false

[516/729][8.1s] clang-tidy-21 -p=/home/admin/actions-runner/_work/_temp/build -quiet -load=/tidy-build/libbitcoin-tidy.so /home/admin/actions-runner/_work/_temp/src/wallet/test/fuzz/spend.cpp
/home/admin/actions-runner/_work/_temp/src/wallet/test/fuzz/spend.cpp:125:41: error: unnecessary temporary object created while calling emplace_back [modernize-use-emplace,-warnings-as-errors]
  125 |                     tx.vin.emplace_back(CTxIn(out));
      |                                         ^~~~~~   ~
/home/admin/actions-runner/_work/_temp/src/wallet/test/fuzz/spend.cpp:152:23: error: the value returned by this function should not be disregarded; neglecting it may lead to errors [bugprone-unused-return-value,-warnings-as-errors]
  152 |                 (void)FundTransaction(*fuzzed_wallet.wallet, tx, recipients, change_pos, lockUnspents, coin_control);
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2392 warnings generated.

[517/729][3.6s] clang-tidy-21 -p=/home/admin/actions-runner/_work/_temp/build -quiet -load=/tidy-build/libbitcoin-tidy.so /home/admin/actions-runner/_work/_temp/src/qt/bitcoinaddressvalidator.cpp
^^^ ⚠️ Failure generated from clang-tidy
Command '['docker', 'exec', '88e904189f27a153218451cb9cc2cf6a3600de9821f8b631509e657c4e1d73df', '/home/admin/actions-runner/_work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.

in src/wallet/test/fuzz/spend.cpp:125 in e3619a36d8

 127 | +                CMutableTransaction tx;
 128 | +                tx.nLockTime = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
 129 | +                int num_inputs = fuzzed_data_provider.ConsumeIntegralInRange<int>(0, available_outpoints.size());
 130 | +                for (int i = 0; i < num_inputs; i++) {
 131 | +                    auto out = PickValue(fuzzed_data_provider, available_outpoints);
 132 | +                    tx.vin.emplace_back(CTxIn(out));

frankomosh commented at 7:32 AM on January 13, 2026:

Could simplify to tx.vin.emplace_back(out)

in src/wallet/test/fuzz/spend.cpp:152 in e3619a36d8

 154 | +                }
 155 | +                std::optional<unsigned int> change_pos;
 156 | +                if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
 157 | +                bool lockUnspents = fuzzed_data_provider.ConsumeBool();
 158 | +
 159 | +                (void)FundTransaction(*fuzzed_wallet.wallet, tx, recipients, change_pos, lockUnspents, coin_control);

frankomosh commented at 7:48 AM on January 13, 2026:

Also, should FundTransaction have wallet lock protection i.e wrap it in LOCK(fuzzed_wallet.wallet -> cs_wallet), as has been done in ListCoins ?

Chand-ra commented at 3:43 PM on January 13, 2026:

Not really, FundTransaction() doesn't require an exclusive lock unlike ListCoins() does.

in src/wallet/test/fuzz/spend.cpp:128 in e3619a36d8

 130 | +                for (int i = 0; i < num_inputs; i++) {
 131 | +                    auto out = PickValue(fuzzed_data_provider, available_outpoints);
 132 | +                    tx.vin.emplace_back(CTxIn(out));
 133 | +                }
 134 | +                std::vector<CRecipient> recipients;
 135 | +                LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 50) {

frankomosh commented at 8:08 AM on January 13, 2026:

I notice that this logic appears twice. Would it make sense to extract it into a lambda helper at the top of the function?

frankomosh commented at 8:11 AM on January 13, 2026: contributor

I think that the variables int next_locktime and CAmount all_values become dead code after you’ve refactored? Also some inline nits below.

Chand-ra commented at 3:48 PM on January 13, 2026: none

I think that the variables int next_locktime and CAmount all_values become dead code after you’ve refactored? Also some inline nits below.

I believe next_locktime and all_values are still necessary in the setup loop. next_locktime ensures that we generate unique TXIDs and all_values is used to enforce the MAX_MONEY cap so we don't overflow the wallet balance during setup.

Chand-ra force-pushed on Jan 13, 2026

DrahtBot removed the label CI failed on Jan 13, 2026

DrahtBot added the label Needs rebase on Jan 21, 2026

fuzz: Refactor `wallet_create_transaction` to support multiple actions

Update the `wallet_create_transaction` fuzz target to move the
transaction creation logic inside a `CallOneOf` block.

This refactoring is a structural preparation for stateful fuzzing,
allowing commits to easily add and interleave other wallet
operations within the same loop.

51c7cedea3

fuzz: lock inputs to verify correct handling of user-mandated inputs

Move the `CCoinControl` parameter configuration inside the fuzzing
loop to allow mutation of settings between operations. Additionally,
introduce logic to randomly `Select` or `UnSelect` specific available
outpoints within `coin_control`.

This allows the fuzzer to simulate scenarios where a user manually
mandates specific inputs.

2f702af9c9

fuzz: Add coverage for `FundTransaction()`

Extend the target to cover `FundTransaction()`.

37d99b6f9e

fuzz: Add coverage for `ListCoins()`

Extend the fuzz target to cover `ListCoins()`.

1876e77f7e

Chand-ra force-pushed on Jan 22, 2026

DrahtBot removed the label Needs rebase on Jan 22, 2026

sedited requested review from marcofleon on Mar 19, 2026

sedited requested review from frankomosh on Jul 10, 2026

DrahtBot added the label Needs rebase on Jul 14, 2026

DrahtBot commented at 7:00 PM on July 14, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/wallet/test/fuzz/spend.cpp:71 in 1876e77f7e

  65 | @@ -72,33 +66,81 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
  66 |          auto txid{tx.GetHash()};
  67 |          auto ret{fuzzed_wallet.wallet->mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateConfirmed{chainstate.m_chain.Tip()->GetBlockHash(), chainstate.m_chain.Height(), /*index=*/0}))};
  68 |          assert(ret.second);
  69 | +        available_outpoints.emplace_back(txid, 0);
  70 |      }
  71 |

frankomosh commented at 11:00 AM on August 7, 2026:


{
    LOCK(fuzzed_wallet.wallet->cs_wallet);
    assert(fuzzed_wallet.wallet->GetTXOs().size() == static_cast<size_t>(next_locktime));
}

You can add Bruno's suggestion from #35790#pullrequestreview-4782030244 to always catch a possible future regression where the TXO index falls out of sync.

in src/wallet/test/fuzz/spend.cpp:103 in 1876e77f7e

 101 |              [&] {
 102 | -                destination = fuzzed_wallet.GetDestination(fuzzed_data_provider);
 103 | +                auto recipients = create_recipients();
 104 | +                std::optional<unsigned int> change_pos;
 105 | +                if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
 106 | +                [[maybe_unused]] auto _{CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control)};

frankomosh commented at 11:04 AM on August 7, 2026:

              auto res_ct{CreateTransaction(*fuzzed_wallet.wallet, recipients, change_pos, coin_control)};
              if (res_ct) { assert(res_ct->fee >= 0); }

After rebase, the CreateTransaction can reach success paths, and therefore we can do an assert check on the result.

in src/wallet/test/fuzz/spend.cpp:136 in 1876e77f7e

 137 | +                auto recipients = create_recipients();
 138 | +                std::optional<unsigned int> change_pos;
 139 | +                if (fuzzed_data_provider.ConsumeBool()) change_pos = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
 140 | +                bool lockUnspents = fuzzed_data_provider.ConsumeBool();
 141 | +
 142 | +                [[maybe_unused]] auto _{FundTransaction(*fuzzed_wallet.wallet, tx, recipients, change_pos, lockUnspents, coin_control)};

frankomosh commented at 11:07 AM on August 7, 2026:

                auto res_ft{FundTransaction(*fuzzed_wallet.wallet, tx, recipients, change_pos, lockUnspents, coin_control)};
                if (res_ft) { assert(res_ft->fee >= 0); }

Same case here,

in src/wallet/test/fuzz/spend.cpp:140 in 1876e77f7e

 142 | +                [[maybe_unused]] auto _{FundTransaction(*fuzzed_wallet.wallet, tx, recipients, change_pos, lockUnspents, coin_control)};
 143 |              },
 144 |              [&] {
 145 | -                destination = ConsumeTxDestination(fuzzed_data_provider);
 146 | +                LOCK(fuzzed_wallet.wallet->cs_wallet);
 147 | +                (void)ListCoins(*fuzzed_wallet.wallet);

frankomosh commented at 11:20 AM on August 7, 2026:

                auto coins_by_dest = ListCoins(*fuzzed_wallet.wallet);
                for (const auto& [dest, outputs] : coins_by_dest) {
                     for (const auto& output : outputs) {
                          assert(MoneyRange(output.txout.nValue));
                     }
                 }

ListCoins requires cs_wallet and returned map should be non-empty now. Asserting on the result can help check behavior. For example we can catch mutations that corrupt output values during coin enumeration.

frankomosh commented at 11:32 AM on August 7, 2026: contributor

Concept ACK.

Good coverage on FundTransaction and ListCoins. Please do a rebase to pick #35790, which makes coin selection becomes reachable.