Merge bitcoin/bitcoin#22275: A few follow-ups for taproot signing

08f57a0057c58f19cd8ae3de89548d014980478a Assert that IsComplete() in GetSpendData() (Pieter Wuille)
d8f4b976d5ae9e6eee741dfdda53b8bc8573221b Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille)
c7048aae9545afd8d522e200ecadcf69f22399a0 Simplify SignTransaction precomputation loop (Pieter Wuille)
addb9b5a71ff96bdb1a4c15bc9345de0d7f2c98c Improve comments in taproot signing logic (Pieter Wuille)

Pull request description:

  This addresses a few review comments from #21365 that were left at the time of merge (as well as some from #22166 applying to the commit it shared with #21365).

  I do not think any are blockers for a 22.0 release.

ACKs for top commit:
  theStack:
    re-ACK 08f57a0057c58f19cd8ae3de89548d014980478a 🌴
  Zero-1729:
    crACK 08f57a0
  jonatack:
    Code review ACK 08f57a0057c58f19cd8ae3de89548d014980478a per `git range-diff e9d6eb1 9336670 08f57a0` followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check

Tree-SHA512: 968750109ba8d6faf3016035a38f81c6aefb724c632a3cab0bbf43cf31b9d187b6b0fddd8772768f57338df11eb07ab9c4c6dacdf3cf35b71f29699c67a301ea

8 files changed, 34 insertions, 21 deletions

diff --git a/src/key.h b/src/key.h
index d47e54800c..92cbc1e899 100644
--- a/src/key.h
+++ b/src/key.h
@@ -133,10 +133,15 @@ public:
      * optionally tweaked by *merkle_root. Additional nonce entropy can be provided through
      * aux.
      *
-     * When merkle_root is not nullptr, this results in a signature with a modified key as
-     * specified in BIP341:
-     * - If merkle_root->IsNull(): key + H_TapTweak(pubkey)*G
-     * - Otherwise:                key + H_TapTweak(pubkey || *merkle_root)
+     * merkle_root is used to optionally perform tweaking of the private key, as specified
+     * in BIP341:
+     * - If merkle_root == nullptr: no tweaking is done, sign with key directly (this is
+     *                              used for signatures in BIP342 script).
+     * - If merkle_root->IsNull():  sign with key + H_TapTweak(pubkey) (this is used for
+     *                              key path spending when no scripts are present).
+     * - Otherwise:                 sign with key + H_TapTweak(pubkey || *merkle_root)
+     *                              (this is used for key path spending, with specific
+     *                              Merkle root of the script tree).
      */
     bool SignSchnorr(const uint256& hash, Span<unsigned char> sig, const uint256* merkle_root = nullptr, const uint256* aux = nullptr) const;
 
diff --git a/src/script/interpreter.h b/src/script/interpreter.h
index 93136a0b79..ab49e84577 100644
--- a/src/script/interpreter.h
+++ b/src/script/interpreter.h
@@ -170,6 +170,13 @@ struct PrecomputedTransactionData
 
     PrecomputedTransactionData() = default;
 
+    /** Initialize this PrecomputedTransactionData with transaction data.
+     *
+     * @param[in]   tx             The transaction for which data is being precomputed.
+     * @param[in]   spent_outputs  The CTxOuts being spent, one for each tx.vin, in order.
+     * @param[in]   force          Whether to precompute data for all optional features,
+     *                             regardless of what is in the inputs (used at signing
+     *                             time, when the inputs aren't filled in yet). */
     template <class T>
     void Init(const T& tx, std::vector<CTxOut>&& spent_outputs, bool force = false);
 
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 2faf7e5048..4714d0ef11 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -61,7 +61,7 @@ bool MutableTransactionSignatureCreator::CreateSchnorrSig(const SigningProvider&
 
     CKey key;
     {
-        // For now, use the old full pubkey-based key derivation logic. As it indexed by
+        // For now, use the old full pubkey-based key derivation logic. As it is indexed by
         // Hash160(full pubkey), we need to try both a version prefixed with 0x02, and one
         // with 0x03.
         unsigned char b[33] = {0x02};
@@ -640,25 +640,22 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
 
     PrecomputedTransactionData txdata;
     std::vector<CTxOut> spent_outputs;
-    spent_outputs.resize(mtx.vin.size());
-    bool have_all_spent_outputs = true;
-    for (unsigned int i = 0; i < mtx.vin.size(); i++) {
+    for (unsigned int i = 0; i < mtx.vin.size(); ++i) {
         CTxIn& txin = mtx.vin[i];
         auto coin = coins.find(txin.prevout);
         if (coin == coins.end() || coin->second.IsSpent()) {
-            have_all_spent_outputs = false;
+            txdata.Init(txConst, /* spent_outputs */ {}, /* force */ true);
+            break;
         } else {
-            spent_outputs[i] = CTxOut(coin->second.out.nValue, coin->second.out.scriptPubKey);
+            spent_outputs.emplace_back(coin->second.out.nValue, coin->second.out.scriptPubKey);
         }
     }
-    if (have_all_spent_outputs) {
+    if (spent_outputs.size() == mtx.vin.size()) {
         txdata.Init(txConst, std::move(spent_outputs), true);
-    } else {
-        txdata.Init(txConst, {}, true);
     }
 
     // Sign what we can:
-    for (unsigned int i = 0; i < mtx.vin.size(); i++) {
+    for (unsigned int i = 0; i < mtx.vin.size(); ++i) {
         CTxIn& txin = mtx.vin[i];
         auto coin = coins.find(txin.prevout);
         if (coin == coins.end() || coin->second.IsSpent()) {
diff --git a/src/script/sign.h b/src/script/sign.h
index b8fcac2e3c..6d3479c143 100644
--- a/src/script/sign.h
+++ b/src/script/sign.h
@@ -45,8 +45,8 @@ class MutableTransactionSignatureCreator : public BaseSignatureCreator {
     const PrecomputedTransactionData* m_txdata;
 
 public:
-    MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn = SIGHASH_ALL);
-    MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData* txdata, int nHashTypeIn = SIGHASH_ALL);
+    MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn);
+    MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData* txdata, int nHashTypeIn);
     const BaseSignatureChecker& Checker() const override { return checker; }
     bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override;
     bool CreateSchnorrSig(const SigningProvider& provider, std::vector<unsigned char>& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const override;
diff --git a/src/script/standard.cpp b/src/script/standard.cpp
index b8349bb9ab..67a79a157c 100644
--- a/src/script/standard.cpp
+++ b/src/script/standard.cpp
@@ -504,6 +504,7 @@ WitnessV1Taproot TaprootBuilder::GetOutput() { return WitnessV1Taproot{m_output_
 
 TaprootSpendData TaprootBuilder::GetSpendData() const
 {
+    assert(IsComplete());
     TaprootSpendData spd;
     spd.merkle_root = m_branch.size() == 0 ? uint256() : m_branch[0]->hash;
     spd.internal_key = m_internal_key;
diff --git a/src/script/standard.h b/src/script/standard.h
index ac4e2f3276..78492733db 100644
--- a/src/script/standard.h
+++ b/src/script/standard.h
@@ -227,8 +227,11 @@ struct TaprootSpendData
     /** The Merkle root of the script tree (0 if no scripts). */
     uint256 merkle_root;
     /** Map from (script, leaf_version) to (sets of) control blocks.
-     *  The control blocks are sorted by size, so that the signing logic can
-     *  easily prefer the cheapest one. */
+     *  More than one control block for a given script is only possible if it
+     *  appears in multiple branches of the tree. We keep them all so that
+     *  inference can reconstruct the full tree. Within each set, the control
+     *  blocks are sorted by size, so that the signing logic can easily
+     *  prefer the cheapest one. */
     std::map<std::pair<CScript, int>, std::set<std::vector<unsigned char>, ShortestVectorFirstComparator>> scripts;
     /** Merge other TaprootSpendData (for the same scriptPubKey) into this. */
     void Merge(TaprootSpendData other);
@@ -252,7 +255,7 @@ private:
         /** Merkle hash of this node. */
         uint256 hash;
         /** Tracked leaves underneath this node (either from the node itself, or its children).
-         *  The merkle_branch field for each is the partners to get to *this* node. */
+         *  The merkle_branch field of each is the partners to get to *this* node. */
         std::vector<LeafInfo> leaves;
     };
     /** Whether the builder is in a valid state so far. */
diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index 56e2aa63b9..2c39cbffb9 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -1160,7 +1160,7 @@ SignatureData CombineSignatures(const CTxOut& txout, const CMutableTransaction&
     SignatureData data;
     data.MergeSignatureData(scriptSig1);
     data.MergeSignatureData(scriptSig2);
-    ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&tx, 0, txout.nValue), txout.scriptPubKey, data);
+    ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&tx, 0, txout.nValue, SIGHASH_DEFAULT), txout.scriptPubKey, data);
     return data;
 }
 
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index 571f792a53..97fd0600fa 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -561,7 +561,7 @@ SignatureData CombineSignatures(const CMutableTransaction& input1, const CMutabl
     SignatureData sigdata;
     sigdata = DataFromTransaction(input1, 0, tx->vout[0]);
     sigdata.MergeSignatureData(DataFromTransaction(input2, 0, tx->vout[0]));
-    ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&input1, 0, tx->vout[0].nValue), tx->vout[0].scriptPubKey, sigdata);
+    ProduceSignature(DUMMY_SIGNING_PROVIDER, MutableTransactionSignatureCreator(&input1, 0, tx->vout[0].nValue, SIGHASH_ALL), tx->vout[0].scriptPubKey, sigdata);
     return sigdata;
 }