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

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)

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.

Type	Reviewers
Concept ACK	darosior

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32080 (OP_CHECKCONTRACTVERIFY by bigspider)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
#31868 ([IBD] specialize block serialization by l0rinc)
#31519 (refactor: Use std::span over Span by maflcko)

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.

DrahtBot added the label Tests on Dec 10, 2024

[interpreter] Move taproot commitment and witness program check to BaseSignatureChecker 70ec9f2034

dergoegge force-pushed on Dec 10, 2024

[fuzz] Introduce script_flags_mocked to cover segwit v{0,1} script f9bc6ba48d

dergoegge force-pushed on Dec 10, 2024

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).

bitcoin deleted a comment on Jan 7, 2025

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.

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?

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.

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.

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

in src/test/fuzz/script_flags.cpp:40 in f9bc6ba48d

36+class FuzzedSignatureChecker : public BaseSignatureChecker
37+{
38+public:
39+    FuzzedSignatureChecker(const CTransaction* tx, unsigned int in,
40+                           const CAmount& amount, const PrecomputedTransactionData& tx_data,
41+                           MissingDataBehavior mdb) {}

darosior commented at 9:30 pm on March 12, 2025:

This seems unnecessary, none of that is used.

in src/test/fuzz/script_flags.cpp:51 in f9bc6ba48d

47+    }
48+    bool CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pub_key,
49+                               SigVersion sig_version, ScriptExecutionData& exec_data,
50+                               ScriptError* script_error = nullptr) const override
51+    {
52+        uint64_t fuzz_hash{HashSig(sig)};

darosior commented at 9:32 pm on March 12, 2025:

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)?

in src/test/fuzz/script_flags.cpp:65 in f9bc6ba48d

61+
62+        return sig_ok;
63+    }
64+    bool CheckLockTime(const CScriptNum& lock_time) const override
65+    {
66+        return HashScriptNum(lock_time);

darosior commented at 9:33 pm on March 12, 2025:

Same here, you could just variate based on the value itself?

darosior commented at 9:40 pm on March 12, 2025: member

Concept ACK.

DrahtBot added the label Needs rebase on Mar 20, 2025

DrahtBot commented at 10:14 am on March 20, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Code Coverage & Benchmarks

Reviews

Conflicts