fuzz: Limit wallet_notifications iterations (take 2)#31467
fuzz: Limit wallet_notifications iterations (take 2)#31467maflcko wants to merge 1 commit intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31467. ReviewsSee the guideline for information on the review process. |
| // Add tx to block | ||
| block.vtx.emplace_back(MakeTransactionRef(tx)); | ||
| // Check that funding the tx doesn't crash the wallet | ||
| a.FundTx(fuzzed_data_provider, tx); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
Removing Knapsack is still on my to-do list!
|
@dergoegge you likely also have some thoughts here? |
|
Follow-ups could be:
Though, I don't have time to work on this right now, so closing for now. |
fad2faf fuzz: Delete wallet_notifications (MarcoFalke) Pull request description: The fuzz target has many issues: * It has never found a meaningful issue. * It is still slow, despite #28933 and #31238. * It is unmaintained, see #28882 (comment) (missing meaningful coverage) or #31238 (comment) (unstable) or #31467 (comment) (fix slowness), etc ... So remove it for now. It can be added back once one or all of the issues have been addressed. ACKs for top commit: fanquake: ACK fad2faf brunoerg: ACK fad2faf Tree-SHA512: e48c08352688c0eead5793ee1c7513ddd37459bc665e914a770a3f69772674ed0e14c05e5d07b333ca0ab03bb35d7d9d32561311af569958e19dc4607c11fade
7 participants
This is a follow-up to #31238 (comment), limiting the scope of the fuzz target even further, because it is still by far the most expensive one, see bitcoin-core/qa-assets#214 (comment).
Also, remove the
FundTxcheck, 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)