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

08f57a0057 Assert that IsComplete() in GetSpendData() (Pieter Wuille)
d8f4b976d5 Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille)
c7048aae95 Simplify SignTransaction precomputation loop (Pieter Wuille)
addb9b5a71 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 08f57a0057 🌴
  Zero-1729:
    crACK 08f57a0
  jonatack:
    Code review ACK 08f57a0057 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

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;
 

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);
 

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()) {

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;

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;

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. */

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;
 }
 

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;
 }