Fix the two CI failures in the tapscript commits

src/script/script.cpp

@@ -375,12 +375,46 @@ static int MultisigCount(const opcodetype opcode, const std::vector<unsigned cha
     return -1;
 }
 
+/**
+ * The k of a tapscript k-of-n, which allows far more keys than OP_CHECKMULTISIG does.
+ *
+ * A k from 1 to 16 is an OP_N as usual. Beyond the range OP_N reaches, k is instead a minimal
+ * push of the number: one byte up to 127, two beyond that, since a third would only ever hold
+ * the sign of a count that cannot be negative.
+ */
+static int MultiACount(const opcodetype opcode, const std::vector<unsigned char>& push_data)
+{
+    if (const int small{SmallIntValue(opcode)}; small > 0) return small;
+    if (opcode != 1 && opcode != 2) return -1;
+    if (push_data.size() != static_cast<size_t>(opcode)) return -1;
+    // The sign bit is never set here, and a zero high byte is minimal only where it keeps the
+    // byte below it from reading as the sign, as in 128 written 80 00.
+    if (push_data.back() & 0x80) return -1;
+    if (push_data.back() == 0 && (push_data.size() == 1 || !(push_data[0] & 0x80))) return -1;
+    int value{push_data[0]};
+    if (push_data.size() == 2) value |= int{push_data[1]} << 8;
+    if (value <= 16 || value > int{MAX_PUBKEYS_PER_MULTI_A}) return -1;
+    return value;
+}
+
 static bool IsPubkeyPush(const opcodetype opcode, const std::vector<unsigned char>& push_data)
 {
     if (opcode > OP_PUSHDATA4) return false;
     return push_data.size() == CPubKey::COMPRESSED_SIZE || push_data.size() == CPubKey::SIZE;
 }
 
+/** A 32 byte push, which is an x-only key where a checksig-family opcode claims it as one. */
+static bool IsXOnlyPush(const opcodetype opcode, const std::vector<unsigned char>& push_data)
+{
+    return opcode <= OP_PUSHDATA4 && push_data.size() == XOnlyPubKey::size();
+}
+
+/** Opcodes that leave the stack as they found it, so the key below them is still on top. */
+static bool IsStackNeutralNop(const opcodetype opcode)
+{
+    return opcode == OP_NOP || (opcode >= OP_NOP1 && opcode <= OP_NOP10);
+}
+
 /** Mirror of the push run the caller counts when it is balanced by a drop. */
 static bool IsRunPush(const opcodetype opcode)
 {
@@ -423,17 +457,26 @@ static size_t UnprovenPubkeyBytes(const CScript& script, const CScriptWitness* w
     size_t run_keys{0};
     // Keys in the current push run, which the caller counts whole if a drop balances it.
     size_t push_run_keys{0};
-    int run_opened_by{-1}, last_count{-1};
+    // The current run of a tapscript k-of-n, which names its keys one at a time as
+    // <pubkey> OP_CHECKSIG (<pubkey> OP_CHECKSIGADD)* <k> OP_NUMEQUAL. Only that shape credits k.
+    size_t multi_a_keys{0};
+    int run_opened_by{-1}, last_count{-1}, last_a_count{-1};
     unsigned int inside_noop{0};
     bool last_is_push{false};
+    // Whether the item before this one was a key a checksig-family opcode can still claim, and
+    // whether it was the 32 byte kind that is only a key because one does.
+    bool last_is_key{false}, last_is_xonly{false};
     opcodetype opcode{OP_INVALIDOPCODE}, last_opcode{OP_INVALIDOPCODE};
     std::vector<unsigned char> push_data;
 
     for (CScript::const_iterator it{script.begin()}; it < script.end();) {
         // The caller counts an unparsable script whole, and the iterator may not have advanced.
         if (!script.GetOp(it, opcode, push_data)) return 0;
         const bool is_pubkey{IsPubkeyPush(opcode, push_data)};
+        const bool is_xonly{IsXOnlyPush(opcode, push_data)};
         const int count{MultisigCount(opcode, push_data)};
+        const int a_count{MultiACount(opcode, push_data)};
+        bool is_key{false};
 
         if (inside_noop) {
             // A closed OP_FALSE OP_IF envelope is counted whole by the caller, so skip over it.
@@ -449,34 +492,76 @@ static size_t UnprovenPubkeyBytes(const CScript& script, const CScriptWitness* w
             ++run_keys;
             ++push_run_keys;
             ++pubkeys;
+            is_key = true;
+        } else if (is_xonly) {
+            // Nothing tells an x-only key from a hash but the opcode that spends it, so this
+            // one is counted where a checksig-family opcode claims it rather than here. It
+            // stays out of the push run for the same reason: a drop never reaches it.
+            run_keys = 0;
+            is_key = true;
         } else if ((opcode == OP_DROP || opcode == OP_2DROP) && last_is_push) {
             // The caller counts the whole run as dropped data, so do not charge it again.
             pubkeys -= push_run_keys;
             run_keys = 0;
+            multi_a_keys = 0;
         } else if (opcode == OP_CHECKSIG || opcode == OP_CHECKSIGVERIFY) {
+            if (last_is_xonly) ++pubkeys;
             ++provable;
             run_keys = 0;
+            // <pubkey> OP_CHECKSIG opens a tapscript k-of-n; anything else there ends one.
+            multi_a_keys = (last_is_key && opcode == OP_CHECKSIG) ? 1 : 0;
+        } else if (opcode == OP_CHECKSIGADD) {
+            if (last_is_xonly) ++pubkeys;
+            // The run is credited the k it closes on, so a key earns nothing on its own here.
+            multi_a_keys = (multi_a_keys && last_is_key) ? multi_a_keys + 1 : 0;
+            run_keys = 0;
+        } else if ((opcode == OP_NUMEQUAL || opcode == OP_NUMEQUALVERIFY) && multi_a_keys) {
+            // Padding or reordering the keys forfeits the credit rather than earning one, and
+            // so does naming a k the run cannot satisfy. The OP_CHECKSIG that opened the run
+            // has credited one of the k already.
+            if (last_a_count > 0 && size_t(last_a_count) <= multi_a_keys) {
+                provable += size_t(last_a_count) - 1;
+            }
+            multi_a_keys = 0;
+            run_keys = 0;
         } else if (opcode == OP_CHECKMULTISIG || opcode == OP_CHECKMULTISIGVERIFY) {
             // Padding or reordering the keys forfeits the credit rather than earning one.
             if (run_opened_by > 0 && last_count > 0 && size_t(last_count) == run_keys) {
                 provable += size_t(run_opened_by);
             }
             run_keys = 0;
+            multi_a_keys = 0;
         } else if (count < 0) {
             // A key count sits on both ends of a multisig, so it does not break the run.
             run_keys = 0;
+            if (a_count < 0) multi_a_keys = 0;
         }
 
         const bool is_push{IsRunPush(opcode)};
         if (!is_push) push_run_keys = 0;
+        // An opcode that leaves the stack as it found it leaves the key below it on top, so it
+        // cannot hide that key from the opcode which spends it. It still breaks the run above:
+        // padding a k-of-n is what forfeits its credit.
+        if (!IsStackNeutralNop(opcode)) {
+            last_is_key = is_key;
+            last_is_xonly = is_xonly && !inside_noop;
+        }
         last_opcode = opcode;
         last_count = count;
+        last_a_count = a_count;
         last_is_push = is_push;
     }
 
     // A script can name more signatures than the spender supplies, including in branches that
     // never run. Where the witness carries the spend it is the honest count; a P2SH spend leaves
     // its signatures in the scriptSig, which is not visible here.
+    //
+    // A taproot script-path spend trails the script with a control block and, only when one is
+    // present, an annex, but CountWitnessSignatures excludes only the true last item. A control
+    // block can fall in the 64-73 byte range at a shallow merkle depth, so when an annex pushes
+    // it off the end, it may be miscounted as a signature. That can only inflate the count this
+    // rule caps against, never shrink it, so it can only undercharge, never overcharge: a
+    // documented imprecision, not a bug.
     if (witness && !witness->stack.empty()) {
         provable = std::min(provable, CountWitnessSignatures(*witness));
     }

src/test/script_tests.cpp

@@ -1532,6 +1532,34 @@ static CScriptWitness MultisigWitness(const int sigs, const CScript& script)
     return witness;
 }
 
+/** <pubkey> OP_CHECKSIG (<pubkey> OP_CHECKSIGADD)*(n-1) <k> OP_NUMEQUAL, tapscript's k-of-n. */
+static CScript MultiAScript(const int k, const int n, const size_t key_size = 32,
+                            const opcodetype terminator = OP_NUMEQUAL)
+{
+    CScript script{CScript() << std::vector<unsigned char>(key_size, 1) << OP_CHECKSIG};
+    for (int i{1}; i < n; ++i) {
+        // the byte only has to vary between keys; past 255 it wraps
+        script << std::vector<unsigned char>(key_size, static_cast<unsigned char>(i + 1)) << OP_CHECKSIGADD;
+    }
+    return script << k << terminator;
+}
+
+/** A tapscript witness offering sigs signatures, as a spend of the given k-of-n would. */
+static CScriptWitness TapscriptWitness(const int sigs, const int n, const CScript& script)
+{
+    CScriptWitness witness;
+    // A key the spend does not sign for leaves an empty stack item in place of its signature.
+    for (int i{0}; i < n - sigs; ++i) {
+        witness.stack.emplace_back();
+    }
+    for (int i{0}; i < sigs; ++i) {
+        witness.stack.emplace_back(64, i + 1); // a BIP340 signature, which carries no sighash byte
+    }
+    witness.stack.emplace_back(script.begin(), script.end());
+    witness.stack.emplace_back(33, 0xc0); // the control block of a depth zero taproot leaf
+    return witness;
+}
+
 BOOST_AUTO_TEST_CASE(script_DataCarrierBytes)
 {
     using zeros = std::vector<unsigned char>;
@@ -1641,6 +1669,110 @@ BOOST_AUTO_TEST_CASE(script_DataCarrierBytes_unproven_pubkeys)
                                                              << OP_2DROP << OP_2DROP << OP_DROP));
 }
 
+BOOST_AUTO_TEST_CASE(script_DataCarrierBytes_tapscript_unproven_pubkeys)
+{
+    using zeros = std::vector<unsigned char>;
+
+    // The same payload costs the same whichever way it is written. A 1-of-15 under P2WSH and
+    // the taproot leaf that says the same thing are charged alike, where once the leaf was free
+    BOOST_CHECK_EQUAL("132+0", DatacarrierBytesStr(MultisigScript(1, 15), MultisigWitness(1, MultisigScript(1, 15))));
+    BOOST_CHECK_EQUAL("132+0", DatacarrierBytesStr(MultiAScript(1, 15), TapscriptWitness(1, 15, MultiAScript(1, 15))));
+
+    // An x-only key a signature is offered for is a spending condition, not data
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(CScript() << zeros(32) << OP_CHECKSIG));
+    // Ordinary tapscript multisig keeps its spare keys, exactly as OP_CHECKMULTISIG does
+    for (const auto& [k, n] : {std::pair{2, 3}, std::pair{3, 5}, std::pair{1, 2}, std::pair{1, 3},
+                               std::pair{3, 3}, std::pair{5, 5}, std::pair{11, 15}, std::pair{20, 20}}) {
+        const CScript script{MultiAScript(k, n)};
+        BOOST_CHECK_EQUAL(DatacarrierBytesStr(MultisigScript(k, n)),
+                          DatacarrierBytesStr(script, TapscriptWitness(k, n, script)));
+    }
+    // Eleven keys behind one signature are free, as under OP_CHECKMULTISIG; past the tolerance
+    // every further key is payload, however honest the spend that offers it
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(MultiAScript(1, 11), TapscriptWitness(1, 11, MultiAScript(1, 11))));
+    BOOST_CHECK_EQUAL("33+0", DatacarrierBytesStr(MultiAScript(1, 12), TapscriptWitness(1, 12, MultiAScript(1, 12))));
+    BOOST_CHECK_EQUAL("66+0", DatacarrierBytesStr(MultiAScript(1, 13), TapscriptWitness(1, 13, MultiAScript(1, 13))));
+    BOOST_CHECK_EQUAL("33+0", DatacarrierBytesStr(MultiAScript(5, 16), TapscriptWitness(5, 16, MultiAScript(5, 16))));
+    for (const auto& [k, n] : {std::pair{1, 11}, std::pair{1, 12}, std::pair{1, 13}, std::pair{5, 16}}) {
+        const CScript script{MultiAScript(k, n)};
+        BOOST_CHECK_EQUAL(DatacarrierBytesStr(MultisigScript(k, n), MultisigWitness(k, MultisigScript(k, n))),
+                          DatacarrierBytesStr(script, TapscriptWitness(k, n, script)));
+    }
+    // Above 16 the key count is a minimal push rather than an OP_N, and still counts. A leaf may
+    // hold far more keys than OP_CHECKMULTISIG, so it is read up to MAX_PUBKEYS_PER_MULTI_A
+    BOOST_CHECK_EQUAL("33+0", DatacarrierBytesStr(MultiAScript(17, 28), TapscriptWitness(17, 28, MultiAScript(17, 28))));
+    // From 128 the minimal push carries a zero high byte to keep the sign clear, as in 80 00
+    for (const int k : {127, 128, 255, 256}) {
+        const CScript script{MultiAScript(k, k + 2)};
+        BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(script, TapscriptWitness(k, k + 2, script)));
+    }
+    // ... while a zero high byte nothing needs is not minimal, and forfeits the credit
+    CScript non_minimal{CScript() << zeros(32) << OP_CHECKSIG};
+    for (int i{1}; i < 28; ++i) {
+        non_minimal << zeros(32) << OP_CHECKSIGADD;
+    }
+    non_minimal << std::vector<unsigned char>{17, 0} << OP_NUMEQUAL;
+    BOOST_CHECK_EQUAL("561+0", DatacarrierBytesStr(non_minimal, TapscriptWitness(17, 28, non_minimal)));
+    // The credit is the shape of the run, not the size of its keys: a leaf may hold either
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(MultiAScript(3, 5, 33), TapscriptWitness(3, 5, MultiAScript(3, 5, 33))));
+    // A run closed by OP_NUMEQUALVERIFY says the same thing as one closed by OP_NUMEQUAL
+    const CScript verify{MultiAScript(3, 5, 32, OP_NUMEQUALVERIFY)};
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(verify, TapscriptWitness(3, 5, verify)));
+
+    // A 32 byte push is only a key where something spends it. The preimage of a hashlock and
+    // the hash it is checked against are neither of them keys, in a leaf or anywhere else
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(CScript() << OP_SHA256 << zeros(32) << OP_EQUALVERIFY
+                                                           << zeros(33) << OP_CHECKSIG));
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(CScript() << zeros(32) << OP_SHA256 << zeros(32)
+                                                           << OP_EQUALVERIFY << zeros(32) << OP_CHECKSIG));
+
+    // Padding the run forfeits the credit rather than earning one, so an 11-of-15 written the
+    // long way round is charged as though it proved nothing but the key it opens on
+    CScript padded{CScript() << zeros(32) << OP_CHECKSIG};
+    for (int i{1}; i < 15; ++i) {
+        padded << zeros(32) << OP_NOP << OP_CHECKSIGADD;
+    }
+    padded << 11 << OP_NUMEQUAL;
+    BOOST_CHECK_EQUAL("132+0", DatacarrierBytesStr(padded, TapscriptWitness(11, 15, padded)));
+    // ... while an OP_NOP does not hide the key beneath it from the opcode that spends it
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(CScript() << zeros(32) << OP_NOP << OP_CHECKSIG));
+
+    // Naming a k the run cannot satisfy forfeits the credit too
+    CScript inflated{CScript() << zeros(32) << OP_CHECKSIG};
+    for (int i{1}; i < 15; ++i) {
+        inflated << zeros(32) << OP_CHECKSIGADD;
+    }
+    inflated << 20 << OP_NUMEQUAL;
+    BOOST_CHECK_EQUAL("132+0", DatacarrierBytesStr(inflated, TapscriptWitness(11, 15, inflated)));
+
+    // The OP_CHECKSIG a run opens on credits one of the k already, so a 1-of-n is credited once
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(MultiAScript(1, 3), TapscriptWitness(1, 3, MultiAScript(1, 3))));
+    // Naming more signatures than the spend offers earns nothing here as it does elsewhere
+    BOOST_CHECK_EQUAL("132+0", DatacarrierBytesStr(MultiAScript(11, 15), TapscriptWitness(1, 15, MultiAScript(11, 15))));
+
+    // A key inside an OP_FALSE OP_IF envelope is counted with the envelope, once
+    BOOST_CHECK_EQUAL("0+37", DatacarrierBytesStr(CScript() << OP_FALSE << OP_IF << zeros(32)
+                                                            << OP_CHECKSIG << OP_ENDIF));
+    // A dropped key is already counted as dropped data; it is not charged twice, and no credit
+    // drives the count below zero
+    BOOST_CHECK_EQUAL("0+35", DatacarrierBytesStr(CScript() << zeros(32) << OP_CHECKSIG << zeros(33) << OP_DROP));
+    CScript over_credited{MultiAScript(3, 3)};
+    over_credited << OP_3 << OP_NUMEQUAL << OP_3 << OP_NUMEQUAL;
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(over_credited));
+
+    // A 32 byte push nothing spends and no drop balances is reached by neither rule. It is
+    // stranded on the stack, which leaves the leaf unspendable, so it buys nothing.
+    CScript stranded;
+    for (int i{0}; i < 15; ++i) {
+        stranded << zeros(32);
+    }
+    BOOST_CHECK_EQUAL("0+0", DatacarrierBytesStr(stranded));
+    // Balance the same pushes with drops and the run rule charges them whole
+    BOOST_CHECK_EQUAL("0+496", DatacarrierBytesStr(CScript{stranded} << OP_2DROP << OP_2DROP << OP_2DROP
+                                                                     << OP_2DROP << OP_2DROP << OP_2DROP
+                                                                     << OP_2DROP << OP_DROP));
+}
+
 BOOST_AUTO_TEST_CASE(script_GetScriptForTransactionInput)
 {
     using zeros = std::vector<unsigned char>;