Merge bitcoin#24860: Miniscript integration follow-ups

f3a50c9 miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe516 miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9 miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e42 miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d50 fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064d miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3 miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c66 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot)

Pull request description:

  The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to bitcoin#24148 but i think there are enough of them to be worth a followup PR on its own.

  Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds).
  We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in bitcoin#24149 that will be contained in this file too.

  The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it's reasonable to assume that reusing keys across the Script drops the malleability guarantees.
  It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after bitcoin#24148) with duplicate keys was deemed sane (ie, "safe to use") by Bitcoin Core. We now check for duplicate keys in the constructor.

  This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)

ACKs for top commit:
  sipa:
    utACK f3a50c9 (with the caveat that a lot of it is my own code)
  sanket1729:
    code review ACK f3a50c9. Did not review the fuzz tests.

Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb

Author: fanquake

src/Makefile.test.include

@@ -270,7 +270,7 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/locale.cpp \
  test/fuzz/merkleblock.cpp \
  test/fuzz/message.cpp \
- test/fuzz/miniscript_decode.cpp \
+ test/fuzz/miniscript.cpp \
  test/fuzz/minisketch.cpp \
  test/fuzz/muhash.cpp \
  test/fuzz/multiplication_overflow.cpp \

src/script/miniscript.cpp

@@ -17,69 +17,67 @@ Type SanitizeType(Type e) {
     int num_types = (e << "K"_mst) + (e << "V"_mst) + (e << "B"_mst) + (e << "W"_mst);
     if (num_types == 0) return ""_mst; // No valid type, don't care about the rest
     assert(num_types == 1); // K, V, B, W all conflict with each other
-    bool ok = // Work around a GCC 4.8 bug that breaks user-defined literals in macro calls.
-        (!(e << "z"_mst) || !(e << "o"_mst)) && // z conflicts with o
-        (!(e << "n"_mst) || !(e << "z"_mst)) && // n conflicts with z
-        (!(e << "n"_mst) || !(e << "W"_mst)) && // n conflicts with W
-        (!(e << "V"_mst) || !(e << "d"_mst)) && // V conflicts with d
-        (!(e << "K"_mst) ||  (e << "u"_mst)) && // K implies u
-        (!(e << "V"_mst) || !(e << "u"_mst)) && // V conflicts with u
-        (!(e << "e"_mst) || !(e << "f"_mst)) && // e conflicts with f
-        (!(e << "e"_mst) ||  (e << "d"_mst)) && // e implies d
-        (!(e << "V"_mst) || !(e << "e"_mst)) && // V conflicts with e
-        (!(e << "d"_mst) || !(e << "f"_mst)) && // d conflicts with f
-        (!(e << "V"_mst) ||  (e << "f"_mst)) && // V implies f
-        (!(e << "K"_mst) ||  (e << "s"_mst)) && // K implies s
-        (!(e << "z"_mst) ||  (e << "m"_mst)); // z implies m
-    assert(ok);
+    assert(!(e << "z"_mst) || !(e << "o"_mst)); // z conflicts with o
+    assert(!(e << "n"_mst) || !(e << "z"_mst)); // n conflicts with z
+    assert(!(e << "n"_mst) || !(e << "W"_mst)); // n conflicts with W
+    assert(!(e << "V"_mst) || !(e << "d"_mst)); // V conflicts with d
+    assert(!(e << "K"_mst) ||  (e << "u"_mst)); // K implies u
+    assert(!(e << "V"_mst) || !(e << "u"_mst)); // V conflicts with u
+    assert(!(e << "e"_mst) || !(e << "f"_mst)); // e conflicts with f
+    assert(!(e << "e"_mst) ||  (e << "d"_mst)); // e implies d
+    assert(!(e << "V"_mst) || !(e << "e"_mst)); // V conflicts with e
+    assert(!(e << "d"_mst) || !(e << "f"_mst)); // d conflicts with f
+    assert(!(e << "V"_mst) ||  (e << "f"_mst)); // V implies f
+    assert(!(e << "K"_mst) ||  (e << "s"_mst)); // K implies s
+    assert(!(e << "z"_mst) ||  (e << "m"_mst)); // z implies m
     return e;
 }
 
-Type ComputeType(Fragment nodetype, Type x, Type y, Type z, const std::vector<Type>& sub_types, uint32_t k, size_t data_size, size_t n_subs, size_t n_keys) {
+Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector<Type>& sub_types, uint32_t k, size_t data_size, size_t n_subs, size_t n_keys) {
     // Sanity check on data
-    if (nodetype == Fragment::SHA256 || nodetype == Fragment::HASH256) {
+    if (fragment == Fragment::SHA256 || fragment == Fragment::HASH256) {
         assert(data_size == 32);
-    } else if (nodetype == Fragment::RIPEMD160 || nodetype == Fragment::HASH160) {
+    } else if (fragment == Fragment::RIPEMD160 || fragment == Fragment::HASH160) {
         assert(data_size == 20);
     } else {
         assert(data_size == 0);
     }
     // Sanity check on k
-    if (nodetype == Fragment::OLDER || nodetype == Fragment::AFTER) {
+    if (fragment == Fragment::OLDER || fragment == Fragment::AFTER) {
         assert(k >= 1 && k < 0x80000000UL);
-    } else if (nodetype == Fragment::MULTI) {
+    } else if (fragment == Fragment::MULTI) {
         assert(k >= 1 && k <= n_keys);
-    } else if (nodetype == Fragment::THRESH) {
+    } else if (fragment == Fragment::THRESH) {
         assert(k >= 1 && k <= n_subs);
     } else {
         assert(k == 0);
     }
     // Sanity check on subs
-    if (nodetype == Fragment::AND_V || nodetype == Fragment::AND_B || nodetype == Fragment::OR_B ||
-        nodetype == Fragment::OR_C || nodetype == Fragment::OR_I || nodetype == Fragment::OR_D) {
+    if (fragment == Fragment::AND_V || fragment == Fragment::AND_B || fragment == Fragment::OR_B ||
+        fragment == Fragment::OR_C || fragment == Fragment::OR_I || fragment == Fragment::OR_D) {
         assert(n_subs == 2);
-    } else if (nodetype == Fragment::ANDOR) {
+    } else if (fragment == Fragment::ANDOR) {
         assert(n_subs == 3);
-    } else if (nodetype == Fragment::WRAP_A || nodetype == Fragment::WRAP_S || nodetype == Fragment::WRAP_C ||
-               nodetype == Fragment::WRAP_D || nodetype == Fragment::WRAP_V || nodetype == Fragment::WRAP_J ||
-               nodetype == Fragment::WRAP_N) {
+    } else if (fragment == Fragment::WRAP_A || fragment == Fragment::WRAP_S || fragment == Fragment::WRAP_C ||
+               fragment == Fragment::WRAP_D || fragment == Fragment::WRAP_V || fragment == Fragment::WRAP_J ||
+               fragment == Fragment::WRAP_N) {
         assert(n_subs == 1);
-    } else if (nodetype != Fragment::THRESH) {
+    } else if (fragment != Fragment::THRESH) {
         assert(n_subs == 0);
     }
     // Sanity check on keys
-    if (nodetype == Fragment::PK_K || nodetype == Fragment::PK_H) {
+    if (fragment == Fragment::PK_K || fragment == Fragment::PK_H) {
         assert(n_keys == 1);
-    } else if (nodetype == Fragment::MULTI) {
+    } else if (fragment == Fragment::MULTI) {
         assert(n_keys >= 1 && n_keys <= 20);
     } else {
         assert(n_keys == 0);
     }
 
-    // Below is the per-nodetype logic for computing the expression types.
+    // Below is the per-fragment logic for computing the expression types.
     // It heavily relies on Type's << operator (where "X << a_mst" means
     // "X has all properties listed in a").
-    switch (nodetype) {
+    switch (fragment) {
         case Fragment::PK_K: return "Konudemsxk"_mst;
         case Fragment::PK_H: return "Knudemsxk"_mst;
         case Fragment::OLDER: return
@@ -247,11 +245,10 @@ Type ComputeType(Fragment nodetype, Type x, Type y, Type z, const std::vector<Ty
             }
     }
     assert(false);
-    return ""_mst;
 }
 
-size_t ComputeScriptLen(Fragment nodetype, Type sub0typ, size_t subsize, uint32_t k, size_t n_subs, size_t n_keys) {
-    switch (nodetype) {
+size_t ComputeScriptLen(Fragment fragment, Type sub0typ, size_t subsize, uint32_t k, size_t n_subs, size_t n_keys) {
+    switch (fragment) {
         case Fragment::JUST_1:
         case Fragment::JUST_0: return 1;
         case Fragment::PK_K: return 34;
@@ -262,7 +259,7 @@ size_t ComputeScriptLen(Fragment nodetype, Type sub0typ, size_t subsize, uint32_
         case Fragment::SHA256: return 4 + 2 + 33;
         case Fragment::HASH160:
         case Fragment::RIPEMD160: return 4 + 2 + 21;
-        case Fragment::MULTI: return 3 + (n_keys > 16) + (k > 16) + 34 * n_keys;
+        case Fragment::MULTI: return 1 + BuildScript(n_keys).size() + BuildScript(k).size() + 34 * n_keys;
         case Fragment::AND_V: return subsize;
         case Fragment::WRAP_V: return subsize + (sub0typ << "x"_mst);
         case Fragment::WRAP_S:
@@ -280,19 +277,17 @@ size_t ComputeScriptLen(Fragment nodetype, Type sub0typ, size_t subsize, uint32_
         case Fragment::THRESH: return subsize + n_subs + BuildScript(k).size();
     }
     assert(false);
-    return 0;
 }
 
-bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, std::vector<unsigned char>>>& out)
+std::optional<std::vector<Opcode>> DecomposeScript(const CScript& script)
 {
-    out.clear();
+    std::vector<Opcode> out;
     CScript::const_iterator it = script.begin(), itend = script.end();
     while (it != itend) {
         std::vector<unsigned char> push_data;
         opcodetype opcode;
         if (!script.GetOp(it, opcode, push_data)) {
-            out.clear();
-            return false;
+            return {};
         } else if (opcode >= OP_1 && opcode <= OP_16) {
             // Deal with OP_n (GetOp does not turn them into pushes).
             push_data.assign(1, CScript::DecodeOP_N(opcode));
@@ -309,30 +304,28 @@ bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, st
             out.emplace_back(OP_EQUAL, std::vector<unsigned char>());
             opcode = OP_VERIFY;
         } else if (IsPushdataOp(opcode)) {
-            if (!CheckMinimalPush(push_data, opcode)) return false;
+            if (!CheckMinimalPush(push_data, opcode)) return {};
         } else if (it != itend && (opcode == OP_CHECKSIG || opcode == OP_CHECKMULTISIG || opcode == OP_EQUAL) && (*it == OP_VERIFY)) {
             // Rule out non minimal VERIFY sequences
-            return false;
+            return {};
         }
         out.emplace_back(opcode, std::move(push_data));
     }
     std::reverse(out.begin(), out.end());
-    return true;
+    return out;
 }
 
-bool ParseScriptNumber(const std::pair<opcodetype, std::vector<unsigned char>>& in, int64_t& k) {
+std::optional<int64_t> ParseScriptNumber(const Opcode& in) {
     if (in.first == OP_0) {
-        k = 0;
-        return true;
+        return 0;
     }
     if (!in.second.empty()) {
-        if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return false;
+        if (IsPushdataOp(in.first) && !CheckMinimalPush(in.second, in.first)) return {};
         try {
-            k = CScriptNum(in.second, true).GetInt64();
-            return true;
+            return CScriptNum(in.second, true).GetInt64();
         } catch(const scriptnum_error&) {}
     }
-    return false;
+    return {};
 }
 
 int FindNextChar(Span<const char> sp, const char m)