fuzz: Expand script verification flag testing to segwit v0 and tapscript #31460
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31460. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
…seSignatureChecker
54b6ec7 to
ddec30a
Compare
ddec30a to
f9bc6ba
Compare
|
@reardencode @0xBEEFCAF3 @EthanHeilman Perhaps this is interesting for you all to review as it is relevant to testing your soft-fork proposals (i.e. #29247, #29269). |
| Span<const unsigned char> program, | ||
| const CScript& exec_script) const | ||
| { | ||
| assert(program.size() >= uint256::size()); |
There was a problem hiding this comment.
In 70ec9f2: Instead of asserting that the program size is greater or equal to uint256 size, could we assert that the program size is exactly the WITNESS_V0_SCRIPTHASH_SIZE?
There was a problem hiding this comment.
I mostly added that assertion to prevent a buffer overflow in the memcmp below, which won't happen as long as the program span is 32 bytes in size or larger.
I can see the value in asserting that program is an actually a v0 script hash (i.e. 32 bytes exactly). I'll consider changing it if I retouch.
The same applies to |
|
An alternative to touching consensus code to test this is to fuzz |
Yes that would be an alternative to some extend but I want to keep this test to
That is possible but I would consider that out of scope for this PR. |
|
@dergoegge This PR encouraged me to break the unit testing improvements I made to the Tapscript unit tests out the OP_CAT PR and into their own PR here: #31640 |
| FuzzedSignatureChecker(const CTransaction* tx, unsigned int in, | ||
| const CAmount& amount, const PrecomputedTransactionData& tx_data, | ||
| MissingDataBehavior mdb) {} |
There was a problem hiding this comment.
This seems unnecessary, none of that is used.
| SigVersion sig_version, ScriptExecutionData& exec_data, | ||
| ScriptError* script_error = nullptr) const override | ||
| { | ||
| uint64_t fuzz_hash{HashSig(sig)}; |
There was a problem hiding this comment.
What does hashing adds here? If you need more than one bit why not take it out of sig directly (and have a default value if it's too small)?
| } | ||
| bool CheckLockTime(const CScriptNum& lock_time) const override | ||
| { | ||
| return HashScriptNum(lock_time); |
There was a problem hiding this comment.
Same here, you could just variate based on the value itself?
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Can be marked up for grabs. Happy to review! |
The
script_flagsharness aims to test that our script verification flags are soft-forks (i.e. applying flags can only tighten the verification rules and not widen them).SigVersion::WITNESS_V0andSigVersion::TAPSCRIPTscripts are currently not covered by this test, as fuzzers are blocked from e.g. creating a valid taproot script path spend commitment.This PR:
BaseSignatureChecker(real impl inGenericTransactionSignatureChecker)script_flags_mockedwhich uses aBaseSignatureCheckermock, unblocking fuzzers from reaching segwit v{0,1} interpreter code (as well as paths relating to valid signatures)