fuzz: Expand script verification flag testing to segwit v0 and tapscript #31460

1. 
   dergoegge commented at 2:45 pm on December 10, 2024: member

   The script_flags harness 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_V0 and SigVersion::TAPSCRIPT scripts are currently not covered by this test, as fuzzers are blocked from e.g. creating a valid taproot script path spend commitment.

   This PR:

   • Moves the taproot commitment and witness script hash checks to BaseSignatureChecker (real impl in GenericTransactionSignatureChecker)
   • Introduces a second script flags harness script_flags_mocked which uses a BaseSignatureChecker mock, unblocking fuzzers from reaching segwit v{0,1} interpreter code (as well as paths relating to valid signatures)
2. 
   DrahtBot commented at 2:45 pm on December 10, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31460.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31519 (refactor: Use std::span over Span by maflcko)
   • #29269 (Add OP_INTERNALKEY for Tapscript by reardencode)

   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.

3. DrahtBot added the label Tests on Dec 10, 2024
4. [interpreter] Move taproot commitment and witness program check to BaseSignatureChecker 70ec9f2034
5. dergoegge force-pushed on Dec 10, 2024
6. [fuzz] Introduce script_flags_mocked to cover segwit v{0,1} script f9bc6ba48d
7. dergoegge force-pushed on Dec 10, 2024
8. 
   dergoegge commented at 9:41 am on January 3, 2025: member

   @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).
9. bitcoin deleted a comment on Jan 7, 2025
10. in src/script/interpreter.cpp:1807 in 70ec9f2034 outdated

    1802+template <class T>
    1803+bool GenericTransactionSignatureChecker<T>::CheckWitnessScriptHash(
    1804+    Span<const unsigned char> program,
    1805+    const CScript& exec_script) const
    1806+{
    1807+    assert(program.size() >= uint256::size());
    

    ---

    ---

    

    brunoerg commented at 5:24 pm on January 13, 2025:

    In 70ec9f20341a5337372597ed481b048e40982b55: 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?

    ---

    

    dergoegge commented at 10:34 am on January 16, 2025:

    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.

11. 
    brunoerg commented at 1:37 pm on January 15, 2025: contributor

    SigVersion::WITNESS_V0 and SigVersion::TAPSCRIPT scripts are currently not covered by this test, as fuzzers are blocked from e.g. creating a valid taproot script path spend commitment.

    The same applies to signature_checker and eval_script harnesses?

12. 
    darosior commented at 2:44 pm on January 15, 2025: member

    An alternative to touching consensus code to test this is to fuzz EvalScript directly. We already have a target for it, eval_script. It could be adapted to also be ran under a Tapscript context by filling dummy ScriptExecutionData, and the soft fork check could be added to this target. The downside of this approach is that it would not cover the VerifyScript logic, but in the context of making sure proposed opcodes are indeed soft forks what we really care about it EvalScript.
13. 
    dergoegge commented at 10:23 am on January 16, 2025: member

    We already have a target for it, eval_script. It could be adapted…

    Yes that would be an alternative to some extend but I want to keep this test to VerifyScript as that function uses the flags as well.

    The same applies to signature_checker and eval_script harnesses?

    That is possible but I would consider that out of scope for this PR.

14. 
    EthanHeilman commented at 2:06 am on January 21, 2025: contributor

    @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: https://github.com/bitcoin/bitcoin/pull/31640

Contributors
dergoegge DrahtBot brunoerg darosior EthanHeilman

Labels
Tests