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

2d8099c713dfd4b546150fd53c2e4f364b9009f4 Mention units of MAX_STANDARD_ policy constants (Pieter Wuille)
84e29c7c0141b52044020ec0c5dfa8a462b7e97f Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille)
f867cbcc268a3bfaeef5510a7e40e6d3c0818b6d Clean up assets test minimizer LDFLAGS (Pieter Wuille)
ea0e78677bdbe3313f594118c500cf7784c56970 Document additional IsWitnessStandard behavior (Pieter Wuille)
6040de9a46725826330cd63cdf76e2121a18e728 Add comments on CPubKey::IsValid (Pieter Wuille)
8dbb7de67ce0a71f5fc54289c0ff048ac8dd0acc Add comments to VerifyTaprootCommitment (Pieter Wuille)
cdf900cbf26db05c7edb398ea645f1d23049d810 Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille)
18246ed5f09dd078fa1410b7ec2ba4379cc5e032 Fix and improve taproot_construct comments (Pieter Wuille)

Pull request description:

  Addressing some review comments raised here: https://github.com/bitcoin/bitcoin/pull/19953#pullrequestreview-512238027 and https://github.com/bitcoin/bitcoin/pull/19953#pullrequestreview-513499921

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

7 files changed, 43 insertions, 12 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 87166ecb79..f4c726b0b2 100644
--- a/src/Makefile.test.include
+++ b/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)
diff --git a/src/policy/policy.h b/src/policy/policy.h
index fdbf45a66c..726a14a27e 100644
--- a/src/policy/policy.h
+++ b/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);
 
diff --git a/src/pubkey.h b/src/pubkey.h
index 0f784b86e4..80d0c18540 100644
--- a/src/pubkey.h
+++ b/src/pubkey.h
@@ -170,6 +170,15 @@ public:
     /*
      * 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
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 5735e7df66..bb5a7158a5 100644
--- a/src/script/interpreter.cpp
+++ b/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);
 }
 
diff --git a/src/validation.cpp b/src/validation.cpp
index 9197daec80..d8f7bfc913 100644
--- a/src/validation.cpp
+++ b/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");
 
diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
index 0f0fe8a34a..116eb7e3d7 100755
--- a/test/functional/feature_taproot.py
+++ b/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()
diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
index 8e5848d493..26ccab3039 100644
--- a/test/functional/test_framework/script.py
+++ b/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 = []