Merge bitcoin/bitcoin#28651: Make miniscript GetWitnessSize accurate for tapscript

b228108 miniscript: make GetWitnessSize accurate for tapscript (Pieter Wuille)
8be9851 test: add tests for miniscript GetWitnessSize (Pieter Wuille)
7ed2b2d test: remove mutable global contexts in miniscript fuzzer/test (Pieter Wuille)

Pull request description:

  So far, the same algorithm is used to compute an (upper bound on) the maximum witness size for both P2WSH and P2TR miniscript. That's unfortunate, because it means fee estimations for P2TR miniscript will miss out on the generic savings brought by P2TR witnesses (smaller signatures and public keys, specifically).

  Fix this by making the algorithm use script context specification calculations, and add tests for it. Also included is a cleanup for the tests to avoid mutable globals, as I found it hard to reason about what exactly was being tested.

ACKs for top commit:
  achow101:
    ACK b228108
  darosior:
    ACK b228108

Tree-SHA512: e4bda7376628f3e91cfc74917cefc554ca16eb5f2a0e1adddc33eb8717c4aaa071e56a40f85a2041ae74ec445a7bd0129bba48994c203e0e6e4d25af65954d9e

Author: achow101

src/script/miniscript.h

@@ -1105,13 +1105,15 @@ struct Node {
     }
 
     internal::WitnessSize CalcWitnessSize() const {
+        const uint32_t sig_size = IsTapscript(m_script_ctx) ? 1 + 65 : 1 + 72;
+        const uint32_t pubkey_size = IsTapscript(m_script_ctx) ? 1 + 32 : 1 + 33;
         switch (fragment) {
             case Fragment::JUST_0: return {{}, 0};
             case Fragment::JUST_1:
             case Fragment::OLDER:
             case Fragment::AFTER: return {0, {}};
-            case Fragment::PK_K: return {1 + 72, 1};
-            case Fragment::PK_H: return {1 + 72 + 1 + 33, 1 + 1 + 33};
+            case Fragment::PK_K: return {sig_size, 1};
+            case Fragment::PK_H: return {sig_size + pubkey_size, 1 + pubkey_size};
             case Fragment::SHA256:
             case Fragment::RIPEMD160:
             case Fragment::HASH256:
@@ -1131,8 +1133,8 @@ struct Node {
             case Fragment::OR_C: return {subs[0]->ws.sat | (subs[0]->ws.dsat + subs[1]->ws.sat), {}};
             case Fragment::OR_D: return {subs[0]->ws.sat | (subs[0]->ws.dsat + subs[1]->ws.sat), subs[0]->ws.dsat + subs[1]->ws.dsat};
             case Fragment::OR_I: return {(subs[0]->ws.sat + 1 + 1) | (subs[1]->ws.sat + 1), (subs[0]->ws.dsat + 1 + 1) | (subs[1]->ws.dsat + 1)};
-            case Fragment::MULTI: return {k * (1 + 72) + 1, k + 1};
-            case Fragment::MULTI_A: return {k * (1 + 65) + static_cast<uint32_t>(keys.size()) - k, static_cast<uint32_t>(keys.size())};
+            case Fragment::MULTI: return {k * sig_size + 1, k + 1};
+            case Fragment::MULTI_A: return {k * sig_size + static_cast<uint32_t>(keys.size()) - k, static_cast<uint32_t>(keys.size())};
             case Fragment::WRAP_A:
             case Fragment::WRAP_N:
             case Fragment::WRAP_S:

src/test/fuzz/miniscript.cpp

@@ -70,6 +70,7 @@ struct TestData {
             sig.push_back(1); // SIGHASH_ALL
             dummy_sigs.insert({pubkey, {sig, i & 1}});
             assert(privkey.SignSchnorr(MESSAGE_HASH, schnorr_sig, nullptr, EMPTY_AUX));
+            schnorr_sig.push_back(1); // Maximally-sized signature has sighash byte
             schnorr_sigs.emplace(XOnlyPubKey{pubkey}, std::make_pair(std::move(schnorr_sig), i & 1));
 
             std::vector<unsigned char> hash;
@@ -113,7 +114,9 @@ struct TestData {
 struct ParserContext {
     typedef CPubKey Key;
 
-    MsCtx script_ctx{MsCtx::P2WSH};
+    const MsCtx script_ctx;
+
+    constexpr ParserContext(MsCtx ctx) noexcept : script_ctx(ctx) {}
 
     bool KeyCompare(const Key& a, const Key& b) const {
         return a < b;
@@ -178,11 +181,13 @@ struct ParserContext {
     MsCtx MsContext() const {
         return script_ctx;
     }
-} PARSER_CTX;
+};
 
 //! Context that implements naive conversion from/to script only, for roundtrip testing.
 struct ScriptParserContext {
-    MsCtx script_ctx{MsCtx::P2WSH};
+    const MsCtx script_ctx;
+
+    constexpr ScriptParserContext(MsCtx ctx) noexcept : script_ctx(ctx) {}
 
     //! For Script roundtrip we never need the key from a key hash.
     struct Key {
@@ -228,10 +233,13 @@ struct ScriptParserContext {
     MsCtx MsContext() const {
         return script_ctx;
     }
-} SCRIPT_PARSER_CONTEXT;
+};
 
 //! Context to produce a satisfaction for a Miniscript node using the pre-computed data.
-struct SatisfierContext: ParserContext {
+struct SatisfierContext : ParserContext {
+
+    constexpr SatisfierContext(MsCtx ctx) noexcept : ParserContext(ctx) {}
+
     // Timelock challenges satisfaction. Make the value (deterministically) vary to explore different
     // paths.
     bool CheckAfter(uint32_t value) const { return value % 2; }
@@ -267,12 +275,10 @@ struct SatisfierContext: ParserContext {
     miniscript::Availability SatHASH160(const std::vector<unsigned char>& hash, std::vector<unsigned char>& preimage) const {
         return LookupHash(hash, preimage, TEST_DATA.hash160_preimages);
     }
-} SATISFIER_CTX;
+};
 
 //! Context to check a satisfaction against the pre-computed data.
-struct CheckerContext: BaseSignatureChecker {
-    TestData *test_data;
-
+const struct CheckerContext: BaseSignatureChecker {
     // Signature checker methods. Checks the right dummy signature is used.
     bool CheckECDSASignature(const std::vector<unsigned char>& sig, const std::vector<unsigned char>& vchPubKey,
                              const CScript& scriptCode, SigVersion sigversion) const override
@@ -294,7 +300,7 @@ struct CheckerContext: BaseSignatureChecker {
 } CHECKER_CTX;
 
 //! Context to check for duplicates when instancing a Node.
-struct KeyComparator {
+const struct KeyComparator {
     bool KeyCompare(const CPubKey& a, const CPubKey& b) const {
         return a < b;
     }
@@ -1027,15 +1033,15 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p
     if (!node) return;
 
     // Check that it roundtrips to text representation
-    PARSER_CTX.script_ctx = script_ctx;
-    std::optional<std::string> str{node->ToString(PARSER_CTX)};
+    const ParserContext parser_ctx{script_ctx};
+    std::optional<std::string> str{node->ToString(parser_ctx)};
     assert(str);
-    auto parsed = miniscript::FromString(*str, PARSER_CTX);
+    auto parsed = miniscript::FromString(*str, parser_ctx);
     assert(parsed);
     assert(*parsed == *node);
 
     // Check consistency between script size estimation and real size.
-    auto script = node->ToScript(PARSER_CTX);
+    auto script = node->ToScript(parser_ctx);
     assert(node->ScriptSize() == script.size());
 
     // Check consistency of "x" property with the script (type K is excluded, because it can end
@@ -1049,12 +1055,12 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p
     if (!node->IsValidTopLevel()) return;
 
     // Check roundtrip to script
-    auto decoded = miniscript::FromScript(script, PARSER_CTX);
+    auto decoded = miniscript::FromScript(script, parser_ctx);
     assert(decoded);
     // Note we can't use *decoded == *node because the miniscript representation may differ, so we check that:
     // - The script corresponding to that decoded form matches exactly
     // - The type matches exactly
-    assert(decoded->ToScript(PARSER_CTX) == script);
+    assert(decoded->ToScript(parser_ctx) == script);
     assert(decoded->GetType() == node->GetType());
 
     // Optionally pad the script or the witness in order to increase the sensitivity of the tests of
@@ -1091,19 +1097,19 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p
         }
     }
 
-    SATISFIER_CTX.script_ctx = script_ctx;
+    const SatisfierContext satisfier_ctx{script_ctx};
 
     // Get the ScriptPubKey for this script, filling spend data if it's Taproot.
     TaprootBuilder builder;
     const CScript script_pubkey{ScriptPubKey(script_ctx, script, builder)};
 
     // Run malleable satisfaction algorithm.
     std::vector<std::vector<unsigned char>> stack_mal;
-    const bool mal_success = node->Satisfy(SATISFIER_CTX, stack_mal, false) == miniscript::Availability::YES;
+    const bool mal_success = node->Satisfy(satisfier_ctx, stack_mal, false) == miniscript::Availability::YES;
 
     // Run non-malleable satisfaction algorithm.
     std::vector<std::vector<unsigned char>> stack_nonmal;
-    const bool nonmal_success = node->Satisfy(SATISFIER_CTX, stack_nonmal, true) == miniscript::Availability::YES;
+    const bool nonmal_success = node->Satisfy(satisfier_ctx, stack_nonmal, true) == miniscript::Availability::YES;
 
     if (nonmal_success) {
         // Non-malleable satisfactions are bounded by the satisfaction size plus:
@@ -1114,6 +1120,9 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p
         // If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it.
         assert(mal_success);
         assert(stack_nonmal == stack_mal);
+        // Compute witness size (excluding script push, control block, and witness count encoding).
+        const size_t wit_size = GetSerializeSize(stack_nonmal, PROTOCOL_VERSION) - GetSizeOfCompactSize(stack_nonmal.size());
+        assert(wit_size <= *node->GetWitnessSize());
 
         // Test non-malleable satisfaction.
         witness_nonmal.stack.insert(witness_nonmal.stack.end(), std::make_move_iterator(stack_nonmal.begin()), std::make_move_iterator(stack_nonmal.end()));
@@ -1229,13 +1238,13 @@ FUZZ_TARGET(miniscript_string, .init = FuzzInit)
     if (buffer.empty()) return;
     FuzzedDataProvider provider(buffer.data(), buffer.size());
     auto str = provider.ConsumeBytesAsString(provider.remaining_bytes() - 1);
-    PARSER_CTX.script_ctx = (MsCtx)provider.ConsumeBool();
-    auto parsed = miniscript::FromString(str, PARSER_CTX);
+    const ParserContext parser_ctx{(MsCtx)provider.ConsumeBool()};
+    auto parsed = miniscript::FromString(str, parser_ctx);
     if (!parsed) return;
 
-    const auto str2 = parsed->ToString(PARSER_CTX);
+    const auto str2 = parsed->ToString(parser_ctx);
     assert(str2);
-    auto parsed2 = miniscript::FromString(*str2, PARSER_CTX);
+    auto parsed2 = miniscript::FromString(*str2, parser_ctx);
     assert(parsed2);
     assert(*parsed == *parsed2);
 }
@@ -1247,9 +1256,9 @@ FUZZ_TARGET(miniscript_script)
     const std::optional<CScript> script = ConsumeDeserializable<CScript>(fuzzed_data_provider);
     if (!script) return;
 
-    SCRIPT_PARSER_CONTEXT.script_ctx = (MsCtx)fuzzed_data_provider.ConsumeBool();
-    const auto ms = miniscript::FromScript(*script, SCRIPT_PARSER_CONTEXT);
+    const ScriptParserContext script_parser_ctx{(MsCtx)fuzzed_data_provider.ConsumeBool()};
+    const auto ms = miniscript::FromScript(*script, script_parser_ctx);
     if (!ms) return;
 
-    assert(ms->ToScript(SCRIPT_PARSER_CONTEXT) == *script);
+    assert(ms->ToScript(script_parser_ctx) == *script);
 }