Skip to content
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.

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

Add script/string roundtrip fuzz tests #106

Merged
merged 5 commits into from
Apr 20, 2022
Merged

Add script/string roundtrip fuzz tests #106

merged 5 commits into from
Apr 20, 2022

Conversation

sipa
Copy link
Owner

@sipa sipa commented Feb 28, 2022

Builds on top of #105.

This adds script and string roundtrip fuzz tests, all in the same source file (reusing some logic), and unified the names of the tests:

  • miniscript_stable (the old miniscript_random)
  • miniscript_smart (the old miniscript_random_smart)
  • miniscript_script (round-trip from/to script)
  • miniscript_string (round-trip from/to string)

Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Code looks good to me, this is much cleaner and probably much more efficient than my previous targets. I did not test them to compare the coverage though.

I think we should get rid of my miniscript_decode if we have miniscript_script. EDIT: i wrongly assumed #78 had been merged already!

@@ -606,7 +592,7 @@ void TestNode(const NodeRef& node, FuzzedDataProvider& provider)
} // namespace

/** Fuzz target that runs TestNode on nodes generated using ConsumeNodeStable. */
FUZZ_TARGET_INIT(miniscript_random_stable, initialize_miniscript_random)
FUZZ_TARGET(miniscript_random_stable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat.

Comment on lines +680 to +929
/* Fuzz tests that test parsing from a script, and roundtripping via script. */
FUZZ_TARGET(miniscript_script)
Copy link
Contributor

@darosior darosior Mar 17, 2022

Choose a reason for hiding this comment

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

This effectively replaces the miniscript_decode target we already have? The miniscript_decode target isn't in master.

@sipa sipa force-pushed the 202202_decodefuzz branch 2 times, most recently from 3c25782 to 952d025 Compare March 23, 2022 14:58
@sipa
Copy link
Owner Author

sipa commented Mar 23, 2022

Rebased on new #105.

@sipa sipa force-pushed the 202202_decodefuzz branch 2 times, most recently from 4006134 to 6065e95 Compare March 24, 2022 17:20
@sipa sipa force-pushed the 202202_decodefuzz branch from 6065e95 to b431733 Compare April 5, 2022 14:48
Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Apart from the typo in the minscript_string target mentioned above, this looks good to me.

@sipa sipa force-pushed the 202202_decodefuzz branch from b431733 to ef71d2c Compare April 19, 2022 21:21
@sipa
Copy link
Owner Author

sipa commented Apr 19, 2022

Rebased on top of merged #105, and also addressed:

  • Partially reverted to FUZZ_TARGET_INIT again, to make sure initializations aren't executed when not needed.
  • s/str/str2/ in the string test

Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK ef71d2c -- just a nit on FuzzInit not being necessary for one of the targets but no big deal.

@sipa sipa force-pushed the 202202_decodefuzz branch from ef71d2c to c65113d Compare April 20, 2022 15:05
@darosior
Copy link
Contributor

ACK c65113d

@sipa sipa merged commit c68bb14 into master Apr 20, 2022
sipa added a commit that referenced this pull request Apr 21, 2022
033238f Modernize code: use std::optional instead of bool/outarg (Pieter Wuille)

Pull request description:

  Built on top of #106.

ACKs for top commit:
  darosior:
    ACK 033238f

Tree-SHA512: c198e0cc6b6596dcbf98004b50ad7b3d3b51be0fd04f776ff184f4b0f4eafb0e3fedb2b3ffce181014f240106c0e6d30840685417123866c15c7dcb1b150f193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants