Skip to content

fuzz: wallet, add target for CoinControl - #27902

Merged
achow101 merged 1 commit into
bitcoin:masterfrom
Ayush170-Future:fuzz-coverage-coincontrol
Jun 19, 2023
Merged

achow101 merged 1 commit into
bitcoin:masterfrom
Ayush170-Future:fuzz-coverage-coincontrol

Conversation

@Ayush170-Future

Copy link
Copy Markdown
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

DrahtBot commented Jun 16, 2023 •

Copy link
Copy Markdown
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 DrahtBot added the Tests label Jun 16, 2023
Comment thread src/wallet/test/fuzz/coincontrol.cpp Outdated

@brunoerg brunoerg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Ayush170-Future
Ayush170-Future force-pushed the fuzz-coverage-coincontrol branch from 74be54a to 241ca23 Compare June 16, 2023 18:11
@Ayush170-Future

Copy link
Copy Markdown
Contributor Author

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.

@kevkevinpal kevkevinpal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/Makefile.test.include Outdated
Comment thread src/wallet/test/fuzz/coincontrol.cpp Outdated
@Ayush170-Future
Ayush170-Future force-pushed the fuzz-coverage-coincontrol branch from 241ca23 to 40b333e Compare June 17, 2023 18:26
@Ayush170-Future

Copy link
Copy Markdown
Contributor Author

Force pushed addressing @kevkevinpal review.

  • Changed the position of wallet/test/fuzz/coincontrol.cpp in the Makefile.test.include to organize it alphabetically.

@kevkevinpal

Copy link
Copy Markdown
Contributor

reACK 40b333e

@maflcko

maflcko commented Jun 19, 2023

Copy link
Copy Markdown
Member

lgtm ACK 40b333e

@DrahtBot
DrahtBot removed the request for review from maflcko June 19, 2023 08:47

@brunoerg brunoerg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK 40b333e

@maflcko

maflcko commented Jun 19, 2023

Copy link
Copy Markdown
Member

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

@dergoegge dergoegge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 40b333e

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

Copy link
Copy Markdown
Member

ACK 40b333e

@achow101
achow101 merged commit 8f40271 into bitcoin:master Jun 19, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Jun 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants