Merge #20207: Follow-up extra comments on taproot code and tests

2d8099c Mention units of MAX_STANDARD_ policy constants (Pieter Wuille)
84e29c7 Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille)
f867cbc Clean up assets test minimizer LDFLAGS (Pieter Wuille)
ea0e786 Document additional IsWitnessStandard behavior (Pieter Wuille)
6040de9 Add comments on CPubKey::IsValid (Pieter Wuille)
8dbb7de Add comments to VerifyTaprootCommitment (Pieter Wuille)
cdf900c Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille)
18246ed Fix and improve taproot_construct comments (Pieter Wuille)

Pull request description:

  Addressing some review comments raised here: bitcoin/bitcoin#19953 (review) and bitcoin/bitcoin#19953 (review)

ACKs for top commit:
  jonatack:
    ACK 2d8099c per `git range-diff 5009159 4f10965 2d8099c`
  ariard:
    ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard.

Tree-SHA512: c4881546c379ea8efc7ef99a43cbf3b9cd3f9dde5fd97a07ee66f2b593c78aef0bd8784853c5c9c737b66c269241a1048bbbdd6c964a3d872efd8ba0ec410b68

Author: MarcoFalke

src/Makefile.test.include

@@ -1105,7 +1105,7 @@ test_fuzz_script_interpreter_SOURCES = test/fuzz/script_interpreter.cpp
 test_fuzz_script_assets_test_minimizer_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
 test_fuzz_script_assets_test_minimizer_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 test_fuzz_script_assets_test_minimizer_LDADD = $(FUZZ_SUITE_LD_COMMON)
-test_fuzz_script_assets_test_minimizer_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+test_fuzz_script_assets_test_minimizer_LDFLAGS = $(FUZZ_SUITE_LDFLAGS_COMMON)
 test_fuzz_script_assets_test_minimizer_SOURCES = test/fuzz/script_assets_test_minimizer.cpp
 
 test_fuzz_script_ops_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)

src/policy/policy.h

@@ -38,11 +38,11 @@ static const unsigned int DEFAULT_BYTES_PER_SIGOP = 20;
 static const bool DEFAULT_PERMIT_BAREMULTISIG = true;
 /** The maximum number of witness stack items in a standard P2WSH script */
 static const unsigned int MAX_STANDARD_P2WSH_STACK_ITEMS = 100;
-/** The maximum size of each witness stack item in a standard P2WSH script */
+/** The maximum size in bytes of each witness stack item in a standard P2WSH script */
 static const unsigned int MAX_STANDARD_P2WSH_STACK_ITEM_SIZE = 80;
-/** The maximum size of each witness stack item in a standard BIP 342 script (Taproot, leaf version 0xc0) */
+/** The maximum size in bytes of each witness stack item in a standard BIP 342 script (Taproot, leaf version 0xc0) */
 static const unsigned int MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE = 80;
-/** The maximum size of a standard witnessScript */
+/** The maximum size in bytes of a standard witnessScript */
 static const unsigned int MAX_STANDARD_P2WSH_SCRIPT_SIZE = 3600;
 /** The maximum size of a standard ScriptSig */
 static const unsigned int MAX_STANDARD_SCRIPTSIG_SIZE = 1650;
@@ -105,7 +105,9 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs,
     /**
      * Check if the transaction is over standard P2WSH resources limit:
      * 3600bytes witnessScript size, 80bytes per witness stack element, 100 witness stack elements
-     * These limits are adequate for multi-signature up to n-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL,
+     * These limits are adequate for multisignatures up to n-of-100 using OP_CHECKSIG, OP_ADD, and OP_EQUAL.
+     *
+     * Also enforce a maximum stack item size limit and no annexes for tapscript spends.
      */
 bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
 

src/pubkey.h

@@ -170,6 +170,15 @@ class CPubKey
     /*
      * Check syntactic correctness.
      *
+     * When setting a pubkey (Set()) or deserializing fails (its header bytes
+     * don't match the length of the data), the size is set to 0. Thus,
+     * by checking size, one can observe whether Set() or deserialization has
+     * failed.
+     *
+     * This does not check for more than that. In particular, it does not verify
+     * that the coordinates correspond to a point on the curve (see IsFullyValid()
+     * for that instead).
+     *
      * Note that this is consensus critical as CheckECDSASignature() calls it!
      */
     bool IsValid() const

src/script/interpreter.cpp

@@ -1834,9 +1834,13 @@ static bool ExecuteWitnessScript(const Span<const valtype>& stack_span, const CS
 static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, const std::vector<unsigned char>& program, const CScript& script, uint256& tapleaf_hash)
 {
     const int path_len = (control.size() - TAPROOT_CONTROL_BASE_SIZE) / TAPROOT_CONTROL_NODE_SIZE;
+    //! The inner pubkey (x-only, so no Y coordinate parity).
     const XOnlyPubKey p{uint256(std::vector<unsigned char>(control.begin() + 1, control.begin() + TAPROOT_CONTROL_BASE_SIZE))};
+    //! The output pubkey (taken from the scriptPubKey).
     const XOnlyPubKey q{uint256(program)};
+    // Compute the tapleaf hash.
     tapleaf_hash = (CHashWriter(HASHER_TAPLEAF) << uint8_t(control[0] & TAPROOT_LEAF_MASK) << script).GetSHA256();
+    // Compute the Merkle root from the leaf and the provided path.
     uint256 k = tapleaf_hash;
     for (int i = 0; i < path_len; ++i) {
         CHashWriter ss_branch{HASHER_TAPBRANCH};
@@ -1848,7 +1852,9 @@ static bool VerifyTaprootCommitment(const std::vector<unsigned char>& control, c
         }
         k = ss_branch.GetSHA256();
     }
+    // Compute the tweak from the Merkle root and the inner pubkey.
     k = (CHashWriter(HASHER_TAPTWEAK) << MakeSpan(p) << k).GetSHA256();
+    // Verify that the output pubkey matches the tweaked inner pubkey, after correcting for parity.
     return q.CheckPayToContract(p, k, control[0] & 1);
 }
 

src/validation.cpp

@@ -696,7 +696,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
     }
 
-    // Check for non-standard witness in P2WSH
+    // Check for non-standard witnesses.
     if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, m_view))
         return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard");
 

test/functional/feature_taproot.py

@@ -444,6 +444,8 @@ def make_spender(comment, *, tap=None, witv0=False, script=None, pkh=None, p2sh=
     * standard: whether the (valid version of) spending is expected to be standard
     * err_msg: a string with an expected error message for failure (or None, if not cared about)
     * sigops_weight: the pre-taproot sigops weight consumed by a successful spend
+    * need_vin_vout_mismatch: whether this test requires being tested in a transaction input that has no corresponding
+                              transaction output.
     """
 
     conf = dict()

test/functional/test_framework/script.py

@@ -824,21 +824,33 @@ def taproot_tree_helper(scripts):
     h = TaggedHash("TapBranch", left_h + right_h)
     return (left + right, h)
 
+# A TaprootInfo object has the following fields:
+# - scriptPubKey: the scriptPubKey (witness v1 CScript)
+# - inner_pubkey: the inner pubkey (32 bytes)
+# - negflag: whether the pubkey in the scriptPubKey was negated from inner_pubkey+tweak*G (bool).
+# - tweak: the tweak (32 bytes)
+# - leaves: a dict of name -> TaprootLeafInfo objects for all known leaves
 TaprootInfo = namedtuple("TaprootInfo", "scriptPubKey,inner_pubkey,negflag,tweak,leaves")
+
+# A TaprootLeafInfo object has the following fields:
+# - script: the leaf script (CScript or bytes)
+# - version: the leaf version (0xc0 for BIP342 tapscript)
+# - merklebranch: the merkle branch to use for this leaf (32*N bytes)
 TaprootLeafInfo = namedtuple("TaprootLeafInfo", "script,version,merklebranch")
 
 def taproot_construct(pubkey, scripts=None):
     """Construct a tree of Taproot spending conditions
 
-    pubkey: an ECPubKey object for the internal pubkey
+    pubkey: a 32-byte xonly pubkey for the internal pubkey (bytes)
     scripts: a list of items; each item is either:
-             - a (name, CScript) tuple
-             - a (name, CScript, leaf version) tuple
+             - a (name, CScript or bytes, leaf version) tuple
+             - a (name, CScript or bytes) tuple (defaulting to leaf version 0xc0)
              - another list of items (with the same structure)
-             - a function, which specifies how to compute the hashing partner
-               in function of the hash of whatever it is combined with
+             - a list of two items; the first of which is an item itself, and the
+               second is a function. The function takes as input the Merkle root of the
+               first item, and produces a (fictitious) partner to hash with.
 
-    Returns: script (sPK or redeemScript), tweak, {name:(script, leaf version, negation flag, innerkey, merklepath), ...}
+    Returns: a TaprootInfo object
     """
     if scripts is None:
         scripts = []