From b77b0cc507bdc716e5236b1d9880e648147e0af9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 1 Mar 2021 16:49:59 -0800 Subject: Add MissingDataBehavior and make TransactionSignatureChecker handle it This allows specifying how *TransactionSignatureChecker will behave when presented with missing transaction data such as amounts spent, BIP341 data, or spent outputs. As all call sites still (implicitly) use MissingDataBehavior::ASSERT_FAIL, this commit introduces no change in behavior. --- src/script/interpreter.cpp | 20 +++++++++++++++++--- src/script/interpreter.h | 14 ++++++++++++-- 2 files changed, 29 insertions(+), 5 deletions(-) (limited to 'src/script') diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 20a4ce48b0..2b10f5b0df 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1488,8 +1488,20 @@ static const CHashWriter HASHER_TAPLEAF = TaggedHash("TapLeaf"); static const CHashWriter HASHER_TAPBRANCH = TaggedHash("TapBranch"); static const CHashWriter HASHER_TAPTWEAK = TaggedHash("TapTweak"); +static bool HandleMissingData(MissingDataBehavior mdb) +{ + switch (mdb) { + case MissingDataBehavior::ASSERT_FAIL: + assert(!"Missing data"); + break; + case MissingDataBehavior::FAIL: + return false; + } + assert(!"Unknown MissingDataBehavior value"); +} + template -bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata, const T& tx_to, uint32_t in_pos, uint8_t hash_type, SigVersion sigversion, const PrecomputedTransactionData& cache) +bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata, const T& tx_to, uint32_t in_pos, uint8_t hash_type, SigVersion sigversion, const PrecomputedTransactionData& cache, MissingDataBehavior mdb) { uint8_t ext_flag, key_version; switch (sigversion) { @@ -1509,7 +1521,9 @@ bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata assert(false); } assert(in_pos < tx_to.vin.size()); - assert(cache.m_bip341_taproot_ready && cache.m_spent_outputs_ready); + if (!(cache.m_bip341_taproot_ready && cache.m_spent_outputs_ready)) { + return HandleMissingData(mdb); + } CHashWriter ss = HASHER_TAPSIGHASH; @@ -1696,7 +1710,7 @@ bool GenericTransactionSignatureChecker::CheckSchnorrSignature(Spantxdata); - if (!SignatureHashSchnorr(sighash, execdata, *txTo, nIn, hashtype, sigversion, *this->txdata)) { + if (!SignatureHashSchnorr(sighash, execdata, *txTo, nIn, hashtype, sigversion, *this->txdata, m_mdb)) { return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_HASHTYPE); } if (!VerifySchnorrSignature(sig, pubkey, sighash)) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG); diff --git a/src/script/interpreter.h b/src/script/interpreter.h index b4c163c841..490b337108 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -247,11 +247,21 @@ public: virtual ~BaseSignatureChecker() {} }; +/** Enum to specify what *TransactionSignatureChecker's behavior should be + * when dealing with missing transaction data. + */ +enum class MissingDataBehavior +{ + ASSERT_FAIL, //!< Abort execution through assertion failure (for consensus code) + FAIL, //!< Just act as if the signature was invalid +}; + template class GenericTransactionSignatureChecker : public BaseSignatureChecker { private: const T* txTo; + const MissingDataBehavior m_mdb; unsigned int nIn; const CAmount amount; const PrecomputedTransactionData* txdata; @@ -261,8 +271,8 @@ protected: virtual bool VerifySchnorrSignature(Span sig, const XOnlyPubKey& pubkey, const uint256& sighash) const; public: - GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(nullptr) {} - GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {} + GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, MissingDataBehavior mdb = MissingDataBehavior::ASSERT_FAIL) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(nullptr) {} + GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn, MissingDataBehavior mdb = MissingDataBehavior::ASSERT_FAIL) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {} bool CheckECDSASignature(const std::vector& scriptSig, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override; bool CheckSchnorrSignature(Span sig, Span pubkey, SigVersion sigversion, const ScriptExecutionData& execdata, ScriptError* serror = nullptr) const override; bool CheckLockTime(const CScriptNum& nLockTime) const override; -- cgit v1.2.3 From 3820090bd619ac85ab35eff376c03136fe4a9f04 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 1 Mar 2021 18:07:14 -0800 Subject: Make all SignatureChecker explicit about missing data Remove the implicit MissingDataBehavior::ASSERT_FAIL in the *TransationSignatureChecker constructors, and instead specify it explicit in all call sites: * Test code uses ASSERT_FAIL * Validation uses ASSERT_FAIL (through CachingTransactionSignatureChecker) (including signet) * libconsensus uses FAIL, matching the existing behavior of the non-amount API (and the extended required data for taproot validation is not available yet) * Signing code uses FAIL --- src/script/bitcoinconsensus.cpp | 2 +- src/script/interpreter.h | 4 ++-- src/script/sigcache.h | 2 +- src/script/sign.cpp | 6 +++--- 4 files changed, 7 insertions(+), 7 deletions(-) (limited to 'src/script') diff --git a/src/script/bitcoinconsensus.cpp b/src/script/bitcoinconsensus.cpp index 76609f01a7..a9aa6a0060 100644 --- a/src/script/bitcoinconsensus.cpp +++ b/src/script/bitcoinconsensus.cpp @@ -92,7 +92,7 @@ static int verify_script(const unsigned char *scriptPubKey, unsigned int scriptP set_error(err, bitcoinconsensus_ERR_OK); PrecomputedTransactionData txdata(tx); - return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), &tx.vin[nIn].scriptWitness, flags, TransactionSignatureChecker(&tx, nIn, amount, txdata), nullptr); + return VerifyScript(tx.vin[nIn].scriptSig, CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen), &tx.vin[nIn].scriptWitness, flags, TransactionSignatureChecker(&tx, nIn, amount, txdata, MissingDataBehavior::FAIL), nullptr); } catch (const std::exception&) { return set_error(err, bitcoinconsensus_ERR_TX_DESERIALIZE); // Error deserializing } diff --git a/src/script/interpreter.h b/src/script/interpreter.h index 490b337108..970543ce38 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -271,8 +271,8 @@ protected: virtual bool VerifySchnorrSignature(Span sig, const XOnlyPubKey& pubkey, const uint256& sighash) const; public: - GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, MissingDataBehavior mdb = MissingDataBehavior::ASSERT_FAIL) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(nullptr) {} - GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn, MissingDataBehavior mdb = MissingDataBehavior::ASSERT_FAIL) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {} + GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, MissingDataBehavior mdb) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(nullptr) {} + GenericTransactionSignatureChecker(const T* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn, MissingDataBehavior mdb) : txTo(txToIn), m_mdb(mdb), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {} bool CheckECDSASignature(const std::vector& scriptSig, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override; bool CheckSchnorrSignature(Span sig, Span pubkey, SigVersion sigversion, const ScriptExecutionData& execdata, ScriptError* serror = nullptr) const override; bool CheckLockTime(const CScriptNum& nLockTime) const override; diff --git a/src/script/sigcache.h b/src/script/sigcache.h index bf0ba38c2d..7b6b91c963 100644 --- a/src/script/sigcache.h +++ b/src/script/sigcache.h @@ -27,7 +27,7 @@ private: bool store; public: - CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, bool storeIn, PrecomputedTransactionData& txdataIn) : TransactionSignatureChecker(txToIn, nInIn, amountIn, txdataIn), store(storeIn) {} + CachingTransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, bool storeIn, PrecomputedTransactionData& txdataIn) : TransactionSignatureChecker(txToIn, nInIn, amountIn, txdataIn, MissingDataBehavior::ASSERT_FAIL), store(storeIn) {} bool VerifyECDSASignature(const std::vector& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const override; bool VerifySchnorrSignature(Span sig, const XOnlyPubKey& pubkey, const uint256& sighash) const override; diff --git a/src/script/sign.cpp b/src/script/sign.cpp index dba5ce621a..fb3466f320 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -14,7 +14,7 @@ typedef std::vector valtype; -MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn) {} +MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn, MissingDataBehavior::FAIL) {} bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provider, std::vector& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const { @@ -292,7 +292,7 @@ SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nI Stacks stack(data); // Get signatures - MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue); + MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue, MissingDataBehavior::FAIL); SignatureExtractorChecker extractor_checker(data, tx_checker); if (VerifyScript(data.scriptSig, txout.scriptPubKey, &data.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, extractor_checker)) { data.complete = true; @@ -499,7 +499,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore, } ScriptError serror = SCRIPT_ERR_OK; - if (!VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount), &serror)) { + if (!VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, MissingDataBehavior::FAIL), &serror)) { if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) { // Unable to sign input and verification failed (possible attempt to partially sign). input_errors[i] = "Unable to sign input, invalid stack size (possibly missing key)"; -- cgit v1.2.3 From 497718b467330b2c6bb0d44786020c55f1aa75f9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 1 Mar 2021 18:10:13 -0800 Subject: Treat amount<0 also as missing data for P2WPKH/P2WSH Historically lack of amount data has been treated as amount==-1. Change this and treat it as missing data, as introduced in the previous commits. To be minimally invasive, do this at SignatureHash() call sites rather than inside SignatureHash() (which currently has no means or returning a failure code). --- src/script/interpreter.cpp | 3 +++ src/script/sign.cpp | 3 +++ 2 files changed, 6 insertions(+) (limited to 'src/script') diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 2b10f5b0df..abc0625bb1 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1681,6 +1681,9 @@ bool GenericTransactionSignatureChecker::CheckECDSASignature(const std::vecto int nHashType = vchSig.back(); vchSig.pop_back(); + // Witness sighashes need the amount. + if (sigversion == SigVersion::WITNESS_V0 && amount < 0) return HandleMissingData(m_mdb); + uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata); if (!VerifyECDSASignature(vchSig, pubkey, sighash)) diff --git a/src/script/sign.cpp b/src/script/sign.cpp index fb3466f320..6426c8de59 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -26,6 +26,9 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid if (sigversion == SigVersion::WITNESS_V0 && !key.IsCompressed()) return false; + // Signing for witness scripts needs the amount. + if (sigversion == SigVersion::WITNESS_V0 && amount < 0) return false; + uint256 hash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion); if (!key.Sign(hash, vchSig)) return false; -- cgit v1.2.3