fuzz: Limit wallet_notifications iterations (take 2)

maflcko commented at 1:37 pm on December 11, 2024: member

This is a follow-up to #31238#issue-2639196806, limiting the scope of the fuzz target even further, because it is still by far the most expensive one, see https://github.com/bitcoin-core/qa-assets/pull/214#issuecomment-2536009266.

Also, remove the FundTx check, as it is one of the heavier calls.

In the future, the fuzz target can fully be reworked, because it also has poor stability, see #31238 (comment)

fuzz: Limit wallet_notifications iterations (take 2) fae1833a64

DrahtBot commented at 1:37 pm on December 11, 2024: 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/31467.

Reviews

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

DrahtBot added the label Tests on Dec 11, 2024

in src/wallet/test/fuzz/notifications.cpp:121 in fae1833a64

116@@ -117,9 +117,6 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup)
117                     tx.vout.emplace_back(in, wallet.GetScriptPubKey(fuzzed_data_provider));
118                     // Add tx to block
119                     block.vtx.emplace_back(MakeTransactionRef(tx));
120-                    // Check that funding the tx doesn't crash the wallet
121-                    a.FundTx(fuzzed_data_provider, tx);

brunoerg commented at 6:01 pm on December 11, 2024:

FundTx is part of FuzzedWallet and is only used in this target. Since we’re removing this call here, maybe we could delete the entire function in FuzzedWallet.

maflcko commented at 9:20 am on December 12, 2024:

Good point. This should either be kept (as I added it as a regression fuzz test), or the whole target can be removed.

IIUC the overhead comes from the knapsack solver running 1000 times over all inputs, which seems heavy. I wonder if there is an easy way to disable knapsack here?

I guess a middle ground could be to execute this call only ever once per fuzz input.

brunoerg commented at 9:44 am on December 12, 2024:

IIUC the overhead comes from the knapsack solver running 1000 times over all inputs, which seems heavy. I wonder if there is an easy way to disable knapsack here?

This is the 1000 iterations from ApproximateBestSubset?

brunoerg commented at 1:03 pm on December 12, 2024:

Just fyi - When I was working on this paper (https://repositorio.usp.br/directbitstream/ceb3347f-d97b-4995-8c0d-17ec195dd128/3218430.pdf), I did some experiments by reducing the number of iterations in ApproximateBestSubset, I think I mention it in the paper, but I remember that reducing the number of iterations by the half had no impact on its behavior.

marcofleon commented at 1:20 pm on December 23, 2024:

Keeping the two FundTx calls doesn’t make the target significantly slower for me (-runs=1 takes 8 sec vs 6 sec without them). But I agree that if they are removed then the function should be removed from FuzzedWallet.

sipa commented at 3:14 pm on December 23, 2024:

I don’t see too much time difference in a corpus made using both with/without this FundTx call, so it seems better to keep the coverage it could provide.

If knapsack is causing slowness, that seems mostly an argument to improve it, reduce its iteration counts, or get rid of it. Paging @murchandamus .

murchandamus commented at 7:54 pm on December 30, 2024:

Removing Knapsack is still on my to-do list!

fanquake requested review from marcofleon on Dec 17, 2024

fuzz: Limit wallet_notifications iterations (take 2) #31467

Code Coverage & Benchmarks

Reviews