fuzz: wallet, add target for `CoinControl` #27902

pull Ayush170-Future wants to merge 1 commits into bitcoin:master from Ayush170-Future:fuzz-coverage-coincontrol changing 2 files +88 −0

Ayush170-Future commented at 4:35 PM on June 16, 2023: contributor

This PR adds fuzz coverage for wallet/coincontrol.

Motivation: Issue #27272

The idea is to create different/unique instances of COutPoint by placing it inside the CallOneOf function, which may or may not be consumed by all of the CoinControl file's methods.

This is my first PR on Bitcoin Core, and I will try my best to address any reviews/changes ASAP. I'm also working on fuzz harness files for other files in the wallet and plan to open PR for them soon.
DrahtBot commented at 4:35 PM on June 16, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK kevkevinpal, MarcoFalke, brunoerg, dergoegge, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Jun 16, 2023

Type	Reviewers
ACK	kevkevinpal, MarcoFalke, brunoerg, dergoegge, achow101

in src/wallet/test/fuzz/coincontrol.cpp:32 in 74be54abdb outdated

  27 | +    ArgsManager& args = *node.args;
  28 | +
  29 | +    // for GetBoolArg to return true sometimes
  30 | +    if (fuzzed_data_provider.ConsumeBool()) {
  31 | +        args.ForceSetArg("-avoidpartialspends", "1");
  32 | +    }

maflcko commented at 4:56 PM on June 16, 2023:

     
        args.ForceSetArg("-avoidpartialspends", fuzzed_data_provider.ConsumeBool()?"1":"0");

nit, to avoid relying on the default value at all

Ayush170-Future commented at 6:12 PM on June 16, 2023:

Done! Thank you (I'm a big fan of your work).

brunoerg commented at 5:32 PM on June 16, 2023: contributor

lf you remove the trailing whitespace in new lines, lint will be happy:

diff --git a/src/wallet/test/fuzz/coincontrol.cpp b/src/wallet/test/fuzz/coincontrol.cpp
@@ -0,0 +1,89 @@
+    
+FUZZ_TARGET_INIT(coincontrol, initialize_coincontrol) 
+    
+                // Condition to avoid the assertion in GetInputWeight

DrahtBot added the label CI failed on Jun 16, 2023
Ayush170-Future force-pushed on Jun 16, 2023
Ayush170-Future commented at 6:21 PM on June 16, 2023: contributor
Force pushed addressing @MarcoFalke and @brunoerg reviews.
- clang-format the code to remove all the trailing white spaces.
- Avoided relying on the default values, by addressing changes in this review.
DrahtBot removed the label CI failed on Jun 16, 2023
in src/Makefile.test.include:199 in 241ca232dc outdated
```
 195 | @@ -196,6 +196,7 @@ endif
 196 |  FUZZ_WALLET_SRC = \
 197 |   wallet/test/fuzz/coinselection.cpp \
 198 |   wallet/test/fuzz/fees.cpp \
 199 | + wallet/test/fuzz/coincontrol.cpp \
```
kevkevinpal commented at 8:51 PM on June 16, 2023:

nit: move the path up to be organized alphabetically (feel free to ignore)

Ayush170-Future commented at 9:44 AM on June 17, 2023:

Yes, sure. Updating this.
in src/wallet/test/fuzz/coincontrol.cpp:32 in 241ca232dc outdated
```
  27 | +    ArgsManager& args = *node.args;
  28 | +
  29 | +    // for GetBoolArg to return true sometimes
  30 | +    args.ForceSetArg("-avoidpartialspends", fuzzed_data_provider.ConsumeBool()?"1":"0");
  31 | +
  32 | +    CCoinControl coin_control;
```
kevkevinpal commented at 9:07 PM on June 16, 2023:

nit: are we not able to use coincontrol? does it conflict with line 23?

Ayush170-Future commented at 9:50 AM on June 17, 2023:

I don't think this will conflict, but I'll have to double-check. But I set it to coin_control to keep it consistent with the other places where CCoinControl is defined.

Take a look at this: wallet/spend.cpp
kevkevinpal approved
kevkevinpal commented at 9:17 PM on June 16, 2023: contributor

crACK 241ca232dcd0c38169423360de367caa9ee3f49b

checked that we're covering all the functions in ./src/wallet/coincontrol.h and ./src/wallet/coincontrol.cpp

have not yet run the fuzz tests on my local yet
fuzz: wallet, add target for CoinControl 40b333e21f
Ayush170-Future force-pushed on Jun 17, 2023
Ayush170-Future commented at 6:30 PM on June 17, 2023: contributor
Force pushed addressing @kevkevinpal review.
- Changed the position of wallet/test/fuzz/coincontrol.cpp in the Makefile.test.include to organize it alphabetically.
kevkevinpal commented at 9:21 PM on June 18, 2023: contributor

reACK 40b333e
Ayush170-Future requested review from maflcko on Jun 18, 2023
fanquake requested review from dergoegge on Jun 19, 2023
fanquake requested review from murchandamus on Jun 19, 2023
maflcko commented at 8:47 AM on June 19, 2023: member

lgtm ACK 40b333e21f8741e2f553df6b5dcff7277c00a982
DrahtBot removed review request from maflcko on Jun 19, 2023
brunoerg approved
brunoerg commented at 1:15 PM on June 19, 2023: contributor

crACK 40b333e21f8741e2f553df6b5dcff7277c00a982
maflcko commented at 1:34 PM on June 19, 2023: member

Did the author or someone else run this with the ubsan+integer sanitizer to check for any issues before merge?
dergoegge approved
dergoegge commented at 2:55 PM on June 19, 2023: member

ACK 40b333e21f8741e2f553df6b5dcff7277c00a982

Did the author or someone else run this with the ubsan+integer sanitizer to check for any issues before merge?

Yes I did for ~15min on 64 cores. Should be enough given that there isn't much code under test (it plateaued pretty quickly).
achow101 commented at 5:07 PM on June 19, 2023: member

ACK 40b333e21f8741e2f553df6b5dcff7277c00a982
achow101 merged this on Jun 19, 2023
achow101 closed this on Jun 19, 2023
sidhujag referenced this in commit d270bf7c2d on Jun 21, 2023
bitcoin locked this on Jun 18, 2024

Contributors

Review Requested
murchandamus

Labels

Tests

Linked (view graph)

#27272 Increase fuzz coverage in the wallet

fuzz: wallet, add target for `CoinControl` #27902

Reviews