Replace the random unit tests with a fuzz target #94
Conversation
|
I've been running it overnight (with |
|
It's pretty similar to #78, but i don't think it's redundant |
|
Initial corpus from continuously fuzzing for the past day(s): https://github.com/darosior/qa-assets/tree/miniscript_random |
As noted by MarcoFalke on the Bitcoin Core repo, this may be potentially dangerous. Use a generalistic named function for constructing CScripts instead. Co-Authored-By: Pieter Wuille <[email protected]>
|
If you're done with changes here, I'd like to take a shot at making this non-recursive. |
|
I won't have time for this before tomorrow, so I'll let you go first. |
darosior
force-pushed
the
random_tests_to_fuzz
branch
from
8fdbcac to
c4f5df0
Compare
|
Applied your comments, thanks. All yours now :) |
|
Here is my branch (yours, with a commit on top): https://github.com/sipa/bitcoin/commits/random_fuzz It has some logic to propagate type requirements down, in the hope that this causes faster failure on uninteresting paths. The downside is that it adds complexity, and in case it is wrong, may mean potentially interesting cases are never explored. I'm open to discussing dropping some or all of that. The current commit keeps track of (nodetype, type) tuples and prints out the generated miniscripts for each new case in those. That's mostly to observe whether it actually reaches interesting things. |
|
Hmm, it hits assertion failures! Will investigate tomorrow. |
|
This miniscript is considered IsSaneTopLevel, but signing for it fails with ops limit exceeded: The decoded script is |
| case NodeType::PK_H: return subsize + 3 + 21; | ||
| case NodeType::OLDER: return subsize + 1 + (CScript() << k).size(); | ||
| case NodeType::AFTER: return subsize + 1 + (CScript() << k).size(); | ||
| case NodeType::OLDER: return subsize + 1 + BuildScript(k).size(); |
There was a problem hiding this comment.
Unrelated to this PR but i wonder why we add subsize here. It must always be 0 so that's unnecessarily confusing. Same for all fragments without subs in this function.
Removing them on the PR following this comment: bitcoin/bitcoin#24147 (comment)
|
So i'm going to close this since it's not up to date and the work around the fuzz targets has been scattered all around the place. I'll open an issue to centralize what's left to be done. |
2 participants
Based on #93, since the bugfix is needed.
This replicates the tests of
random_tests, except forTestSatisfy: instead of iterating to find the challenges, and set making them available one by one to compare the satisfactions with the previous iteration's ones, just assert the result againstVerifyScriptand some invariants regarding mal / nonmal.This is does not try to be (expensively) smart by trying to always create a valid node, instead carry around the fuzz data provider and return as soon as we generate an invalid node at any depth. tThanks to Pieter for guidance.)