Skip to content

fuzz: wallet, add target for fees #27647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 15, 2023
Merged

fuzz: wallet, add target for fees #27647

merged 1 commit into from
Jun 15, 2023

Conversation

brunoerg
Copy link
Contributor

@brunoerg brunoerg commented May 12, 2023 •
edited
Loading

This PR adds fuzz coverage for wallet/fees. Some functions may use or not (non default) values from wallet, CCoinControl or FeeCalculation. So the logic is to make the test sometimes fill up some attributes and others no.

Obs: As soon as this PR gets some reviews, I can open the proper PR to qa-assets as well.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 12, 2023 •
edited
Loading

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 Xekyo, MarcoFalke, dergoegge

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 May 12, 2023
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 6372a73 to aaa3a5a Compare May 12, 2023 22:02
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch 2 times, most recently from 6b8ebe8 to b00b17d Compare May 12, 2023 23:09
@brunoerg brunoerg mentioned this pull request May 12, 2023
Closed
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from b00b17d to 583ccc7 Compare May 12, 2023 23:37
maflcko
maflcko reviewed May 13, 2023
View reviewed changes
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 583ccc7 to 604f869 Compare May 13, 2023 11:57
@brunoerg
Copy link
Contributor Author

Force-pushed addressing @MarcoFalke's review.

  • Changed wallet to be static
  • Changed format according to clang-format

maflcko
maflcko approved these changes May 14, 2023
View reviewed changes
@fanquake fanquake requested review from murchandamus and furszy May 15, 2023 11:08
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 604f869 to 93c0db7 Compare May 15, 2023 13:28
@brunoerg
Copy link
Contributor Author

Force-pushed addressing @MarcoFalke's review.

@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch 2 times, most recently from 11545e9 to aebce01 Compare May 15, 2023 14:56
@brunoerg
Copy link
Contributor Author

Force-pushed

Thanks @MarcoFalke for the review

@brunoerg
Copy link
Contributor Author

CI failure seems unrelated

@brunoerg brunoerg mentioned this pull request May 15, 2023
Merged
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 7cccbec to 4018eb2 Compare May 15, 2023 22:36
@brunoerg
Copy link
Contributor Author

Force-pushed to replace CreateMockWalletDatabasefor CreateMockableWalletDatabase.

Ayush170-Future
Ayush170-Future reviewed May 31, 2023
View reviewed changes
@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 4018eb2 to 76bfccb Compare May 31, 2023 16:43
@brunoerg
Copy link
Contributor Author

Force-pushed removing the unused variable random_mutable_transaction

@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 76bfccb to 4da8d6b Compare May 31, 2023 16:50
@brunoerg brunoerg requested a review from maflcko June 7, 2023 17:30
maflcko
maflcko reviewed Jun 8, 2023
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm ACK 4da8d6b

left some nits, didn't test

@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 4da8d6b to 171502b Compare June 13, 2023 18:15
@brunoerg
Copy link
Contributor Author

brunoerg commented Jun 13, 2023 •
edited
Loading

Force-pushed addressing #27647 (comment) and #27647 (comment)

CI error is unrelated.

@brunoerg brunoerg force-pushed the 2023-05-fuzz-wallet-fees branch from 171502b to 162602b Compare June 14, 2023 14:21
@murchandamus
Copy link
Contributor

ACK 162602b

@DrahtBot DrahtBot requested review from maflcko and removed request for murchandamus June 14, 2023 14:54
@maflcko
Copy link
Member

maflcko commented Jun 15, 2023

lgtm ACK 162602b

@DrahtBot DrahtBot removed the request for review from maflcko June 15, 2023 07:07
@fanquake fanquake requested a review from dergoegge June 15, 2023 08:37
dergoegge
dergoegge approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

Code review ACK 162602b

@fanquake fanquake merged commit 7a59865 into bitcoin:master Jun 15, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2023
@fanquake
Merge bitcoin#27647: fuzz: wallet, add target for fees
a8c5a3d 
162602b fuzz: wallet, add target for `fees` (brunoerg)

Pull request description:

  This PR adds fuzz coverage for `wallet/fees`. Some functions may use or not (non default) values from `wallet`, `CCoinControl` or `FeeCalculation`. So the logic is to make the test sometimes fill up some attributes and others no.

  Obs: As soon as this PR gets some reviews, I can open the proper PR to `qa-assets` as well.

ACKs for top commit:
  Xekyo:
    ACK 162602b
  MarcoFalke:
    lgtm ACK 162602b
  dergoegge:
    Code review ACK 162602b

Tree-SHA512: 6545802f27aafb60bf5a119af514e9425b643780dea6650bba766bb5be813f2aaddb7afc7f0efa2943ceb26f5ea08b42c95a3c0df897493c71f2d2f99e9e4236
@maflcko
Copy link
Member

maflcko commented Jun 19, 2023

When adding a fuzz target it makes sense to run it with the sanitizers enabled, so that bugs are caught and fixed either before or with adding the target.

echo 'OiAAAPr//wAAAAAAAAA=' | base64  --decode > /tmp/a
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=wallet_fees ./src/test/fuzz/fuzz  /tmp/a

wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed)
    #0 0x5625ef46a094 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) src/wallet/fees.cpp:58:58
    #1 0x5625eedd467f in wallet::(anonymous namespace)::wallet_fees_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/fees.cpp:64:11
...

SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change wallet/fees.cpp:58:58 in

@maflcko
Copy link
Member

maflcko commented Jun 20, 2023

#27917

@dergoegge dergoegge mentioned this pull request Jun 20, 2023
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Jun 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@furszy furszy Awaiting requested review from furszy

4 more reviewers

@maflcko maflcko maflcko approved these changes

@Ayush170-Future Ayush170-Future Ayush170-Future approved these changes

@dergoegge dergoegge dergoegge approved these changes

@Aminkavoos Aminkavoos Aminkavoos approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@brunoerg @DrahtBot @murchandamus @maflcko @dergoegge @Ayush170-Future @Aminkavoos @fanquake