From addb9b5a71ff96bdb1a4c15bc9345de0d7f2c98c Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 17 Jun 2021 16:54:56 -0700 Subject: [PATCH 1/4] Improve comments in taproot signing logic --- src/key.h | 13 +++++++++---- src/script/interpreter.h | 7 +++++++ src/script/sign.cpp | 2 +- src/script/standard.h | 9 ++++++--- 4 files changed, 23 insertions(+), 8 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 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 void Init(const T& tx, std::vector&& spent_outputs, bool force = false); diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 2faf7e5048..2a1c99e387 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}; 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::set, 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 leaves; }; /** Whether the builder is in a valid state so far. */ From c7048aae9545afd8d522e200ecadcf69f22399a0 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 17 Jun 2021 17:25:07 -0700 Subject: [PATCH 2/4] Simplify SignTransaction precomputation loop --- src/script/sign.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 2a1c99e387..4714d0ef11 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -640,25 +640,22 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, PrecomputedTransactionData txdata; std::vector 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()) { From d8f4b976d5ae9e6eee741dfdda53b8bc8573221b Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 17 Jun 2021 17:36:48 -0700 Subject: [PATCH 3/4] Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator These were unused except in tests, and were also overlooked when changing SIGHASH_ALL -> SIGHASH_DEFAULT. --- src/script/sign.h | 4 ++-- src/test/script_tests.cpp | 2 +- src/test/transaction_tests.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) 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& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override; bool CreateSchnorrSig(const SigningProvider& provider, std::vector& sig, const XOnlyPubKey& pubkey, const uint256* leaf_hash, const uint256* merkle_root, SigVersion sigversion) const override; 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; } From 08f57a0057c58f19cd8ae3de89548d014980478a Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 17 Jun 2021 18:14:53 -0700 Subject: [PATCH 4/4] Assert that IsComplete() in GetSpendData() --- src/script/standard.cpp | 1 + 1 file changed, 1 insertion(+) 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;