From 3d4017d30da5719fac1bbca9a103b568552dcd54 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 31 May 2018 10:39:09 +0200 Subject: [PATCH] Merge #13309: Directly operate with CMutableTransaction in SignSignature 6b8b63af1461dc11ffd813401e2c36fa44656715 Generic TransactionSignatureCreator works with both CTransaction and CMutableTransaction (Martin Ankerl) Pull request description: Refactored `TransactionSignatureCreator` into a templated `GenericTransactionSignatureCreator` that works with both `CMutableTransaction` and `CTransaction`. The advantage is that now in `SignSignature`, the `MutableTransactionSignatureCreator` can now operate directly with the `CMutableTransaction` without the need to copy the data into a `CTransaction`. Running all unit tests brings a very noticable speedup on my machine: 48.4 sec before this change 36.4 sec with this change -------- 12.0 seconds saved running only `--run_test=transaction_tests/test_big_witness_transaction`: 16.7 sec before this change 5.9 sec with this change -------- 10.8 seconds saved This relates to my first attempt with the const_cast hack #13202, and to the slow unit test issue #10026. Also see #13050 which modifies the tests but not the production code (like this PR) to get a speedup. Tree-SHA512: 2cff0e9699f484f26120a40e431a24c8bc8f9e780fd89cb0ecf20c5be3eab6c43f9c359cde244abd9f3620d06c7c354e3b9dd3da41fa2ca1ac1e09386fea25fb --- src/script/interpreter.cpp | 50 +++++++++++++++++++++++++++----------- src/script/interpreter.h | 38 +++++++++++++---------------- src/script/sign.cpp | 7 +++--- src/script/sign.h | 15 +++--------- src/wallet/wallet.cpp | 3 +-- 5 files changed, 61 insertions(+), 52 deletions(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 0f7b9e3303..5e8a5ca9f8 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1032,7 +1032,7 @@ bool EvalScript(std::vector >& stack, const CScript& CSHA256() .Write(vchMessage.data(), vchMessage.size()) .Finalize(vchHash.data()); - fSuccess = checker.VerifySignature(vchSig, CPubKey(vchPubKey), uint256(vchHash)); + fSuccess = CPubKey(vchPubKey).Verify(uint256(vchHash), vchSig); } if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && vchSig.size()) { @@ -1308,9 +1308,11 @@ namespace { * Wrapper that serializes like CTransaction, but with the modifications * required for the signature hash done in-place */ -class CTransactionSignatureSerializer { +template +class CTransactionSignatureSerializer +{ private: - const CTransaction& txTo; //!< reference to the spending transaction (the one being serialized) + const T& txTo; //!< reference to the spending transaction (the one being serialized) const CScript& scriptCode; //!< output script being consumed const unsigned int nIn; //!< input index of txTo being signed const bool fAnyoneCanPay; //!< whether the hashtype has the SIGHASH_ANYONECANPAY flag set @@ -1318,7 +1320,7 @@ private: const bool fHashNone; //!< whether the hashtype is SIGHASH_NONE public: - CTransactionSignatureSerializer(const CTransaction &txToIn, const CScript &scriptCodeIn, unsigned int nInIn, int nHashTypeIn) : + CTransactionSignatureSerializer(const T& txToIn, const CScript& scriptCodeIn, unsigned int nInIn, int nHashTypeIn) : txTo(txToIn), scriptCode(scriptCodeIn), nIn(nInIn), fAnyoneCanPay(!!(nHashTypeIn & SIGHASH_ANYONECANPAY)), fHashSingle((nHashTypeIn & 0x1f) == SIGHASH_SINGLE), @@ -1402,7 +1404,9 @@ public: } }; -uint256 GetPrevoutHash(const CTransaction& txTo) { +template +uint256 GetPrevoutHash(const T& txTo) +{ CHashWriter ss(SER_GETHASH, 0); for (const auto& txin : txTo.vin) { ss << txin.prevout; @@ -1410,7 +1414,9 @@ uint256 GetPrevoutHash(const CTransaction& txTo) { return ss.GetHash(); } -uint256 GetSequenceHash(const CTransaction& txTo) { +template +uint256 GetSequenceHash(const T& txTo) +{ CHashWriter ss(SER_GETHASH, 0); for (const auto& txin : txTo.vin) { ss << txin.nSequence; @@ -1418,7 +1424,9 @@ uint256 GetSequenceHash(const CTransaction& txTo) { return ss.GetHash(); } -uint256 GetOutputsHash(const CTransaction& txTo) { +template +uint256 GetOutputsHash(const T& txTo) +{ CHashWriter ss(SER_GETHASH, 0); for (const auto& txout : txTo.vout) { ss << txout; @@ -1428,14 +1436,20 @@ uint256 GetOutputsHash(const CTransaction& txTo) { } // namespace -PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction& txTo) +template +PrecomputedTransactionData::PrecomputedTransactionData(const T& txTo) { hashPrevouts = GetPrevoutHash(txTo); hashSequence = GetSequenceHash(txTo); hashOutputs = GetOutputsHash(txTo); } -uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache) +// explicit instantiation +template PrecomputedTransactionData::PrecomputedTransactionData(const CTransaction& txTo); +template PrecomputedTransactionData::PrecomputedTransactionData(const CMutableTransaction& txTo); + +template +uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache) { assert(nIn < txTo.vin.size()); @@ -1450,7 +1464,7 @@ uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsig } // Wrapper to serialize only the necessary parts of the transaction being signed - CTransactionSignatureSerializer txTmp(txTo, scriptCode, nIn, nHashType); + CTransactionSignatureSerializer txTmp(txTo, scriptCode, nIn, nHashType); // Serialize and hash CHashWriter ss(SER_GETHASH, 0); @@ -1458,12 +1472,14 @@ uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsig return ss.GetHash(); } -bool BaseSignatureChecker::VerifySignature(const std::vector& vchSig, const CPubKey& pubkey, const uint256& sighash) const +template +bool GenericTransactionSignatureChecker::VerifySignature(const std::vector& vchSig, const CPubKey& pubkey, const uint256& sighash) const { return pubkey.Verify(sighash, vchSig); } -bool TransactionSignatureChecker::CheckSig(const std::vector& vchSigIn, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const +template +bool GenericTransactionSignatureChecker::CheckSig(const std::vector& vchSigIn, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const { CPubKey pubkey(vchPubKey); if (!pubkey.IsValid()) @@ -1484,7 +1500,8 @@ bool TransactionSignatureChecker::CheckSig(const std::vector& vch return true; } -bool TransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime) const +template +bool GenericTransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime) const { // There are two kinds of nLockTime: lock-by-blockheight // and lock-by-blocktime, distinguished by whether @@ -1520,7 +1537,8 @@ bool TransactionSignatureChecker::CheckLockTime(const CScriptNum& nLockTime) con return true; } -bool TransactionSignatureChecker::CheckSequence(const CScriptNum& nSequence) const +template +bool GenericTransactionSignatureChecker::CheckSequence(const CScriptNum& nSequence) const { // Relative lock times are supported by comparing the passed // in operand to the sequence number of the input. @@ -1566,6 +1584,10 @@ bool TransactionSignatureChecker::CheckSequence(const CScriptNum& nSequence) con return true; } +// explicit instantiation +template class GenericTransactionSignatureChecker; +template class GenericTransactionSignatureChecker; + bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* serror) { set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR); diff --git a/src/script/interpreter.h b/src/script/interpreter.h index 631ffc8340..dcd98b9a56 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -110,7 +110,8 @@ struct PrecomputedTransactionData { uint256 hashPrevouts, hashSequence, hashOutputs; - explicit PrecomputedTransactionData(const CTransaction& tx); + template + explicit PrecomputedTransactionData(const T& tx); }; enum class SigVersion @@ -118,13 +119,12 @@ enum class SigVersion BASE = 0, }; -uint256 SignatureHash(const CScript &scriptCode, const CTransaction& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr); +template +uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr); class BaseSignatureChecker { public: - virtual bool VerifySignature(const std::vector& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const; - virtual bool CheckSig(const std::vector& scriptSig, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const { return false; @@ -143,33 +143,29 @@ public: virtual ~BaseSignatureChecker() {} }; -class TransactionSignatureChecker : public BaseSignatureChecker +template +class GenericTransactionSignatureChecker : public BaseSignatureChecker { private: - const CTransaction* txTo; + const T* txTo; unsigned int nIn; const CAmount amount; const PrecomputedTransactionData* txdata; -public: - TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(nullptr) {} - TransactionSignatureChecker(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, const PrecomputedTransactionData& txdataIn) : txTo(txToIn), nIn(nInIn), amount(amountIn), txdata(&txdataIn) {} - - // The overriden functions are now final. - bool CheckSig(const std::vector& scriptSig, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const final; - bool CheckLockTime(const CScriptNum& nLockTime) const final; - bool CheckSequence(const CScriptNum& nSequence) const final; -}; - -class MutableTransactionSignatureChecker : public TransactionSignatureChecker -{ -private: - const CTransaction txTo; +protected: + virtual bool VerifySignature(const std::vector& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const; public: - MutableTransactionSignatureChecker(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amount) : TransactionSignatureChecker(&txTo, nInIn, amount), txTo(*txToIn) {} + 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) {} + bool CheckSig(const std::vector& scriptSig, const std::vector& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override; + bool CheckLockTime(const CScriptNum& nLockTime) const override; + bool CheckSequence(const CScriptNum& nSequence) const override; }; +using TransactionSignatureChecker = GenericTransactionSignatureChecker; +using MutableTransactionSignatureChecker = GenericTransactionSignatureChecker; + bool EvalScript(std::vector >& stack, const CScript& script, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* error = nullptr); bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, unsigned int flags, const BaseSignatureChecker& checker, ScriptError* error = nullptr); diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 3125c67d38..3098013535 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -14,9 +14,9 @@ typedef std::vector valtype; -TransactionSignatureCreator::TransactionSignatureCreator(const CTransaction* 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) {} -bool TransactionSignatureCreator::CreateSig(const SigningProvider& provider, std::vector& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const +bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provider, std::vector& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const { CKey key; if (!provider.GetKey(address, key)) @@ -170,8 +170,7 @@ bool SignSignature(const SigningProvider &provider, const CScript& fromPubKey, C { assert(nIn < txTo.vin.size()); - CTransaction txToConst(txTo); - TransactionSignatureCreator creator(&txToConst, nIn, amount, nHashType); + MutableTransactionSignatureCreator creator(&txTo, nIn, amount, nHashType); SignatureData sigdata; bool ret = ProduceSignature(provider, creator, fromPubKey, sigdata); diff --git a/src/script/sign.h b/src/script/sign.h index f8b5e98d2c..3e016a33de 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -37,26 +37,19 @@ public: }; /** A signature creator for transactions. */ -class TransactionSignatureCreator : public BaseSignatureCreator { - const CTransaction* txTo; +class MutableTransactionSignatureCreator : public BaseSignatureCreator { + const CMutableTransaction* txTo; unsigned int nIn; int nHashType; CAmount amount; - const TransactionSignatureChecker checker; + const MutableTransactionSignatureChecker checker; public: - TransactionSignatureCreator(const CTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn=SIGHASH_ALL); + MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn = SIGHASH_ALL); 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; }; -class MutableTransactionSignatureCreator : public TransactionSignatureCreator { - CTransaction tx; - -public: - MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amount, int nHashTypeIn) : TransactionSignatureCreator(&tx, nInIn, amount, nHashTypeIn), tx(*txToIn) {} -}; - /** A signature creator that just produces 72-byte empty signatures. */ extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2c7f078ea0..12a4df97ed 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3891,14 +3891,13 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac if (sign) { - CTransaction txNewConst(txNew); int nIn = 0; for(const auto& coin : vecCoins) { const CScript& scriptPubKey = coin.txout.scriptPubKey; SignatureData sigdata; - if (!ProduceSignature(*this, TransactionSignatureCreator(&txNewConst, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) + if (!ProduceSignature(*this, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata)) { strFailReason = _("Signing transaction failed"); return false;