From 9b92538adebd30353ffc1a427bc20ab0c3fc75f1 Mon Sep 17 00:00:00 2001
From: Matt Corallo <git@bluematt.me>
Date: Fri, 11 May 2018 13:52:49 -0400
Subject: [PATCH] Remove unused fScriptChecks parameter from CheckInputs
fScriptChecks = false just short-circuits the entire function, so
passing it in is entirely useless.
---
src/test/txvalidationcache_tests.cpp | 20 ++--
src/validation.cpp | 164 +++++++++++++--------------
2 files changed, 90 insertions(+), 94 deletions(-)
diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
index e69ebcc2c..193858cca 100644
--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -13,7 +13,7 @@
#include <boost/test/unit_test.hpp>
-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks);
+bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks);
BOOST_AUTO_TEST_SUITE(tx_validationcache_tests)
@@ -125,7 +125,7 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail
// WITNESS requires P2SH
test_flags |= SCRIPT_VERIFY_P2SH;
}
- bool ret = CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), true, test_flags, true, add_to_cache, txdata, nullptr);
+ bool ret = CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, nullptr);
// CheckInputs should succeed iff test_flags doesn't intersect with
// failing_flags
bool expected_return_value = !(test_flags & failing_flags);
@@ -135,13 +135,13 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail
if (ret && add_to_cache) {
// Check that we get a cache hit if the tx was valid
std::vector<CScriptCheck> scriptchecks;
- BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), true, test_flags, true, add_to_cache, txdata, &scriptchecks));
+ BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks));
BOOST_CHECK(scriptchecks.empty());
} else {
// Check that we get script executions to check, if the transaction
// was invalid, or we didn't add to cache.
std::vector<CScriptCheck> scriptchecks;
- BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), true, test_flags, true, add_to_cache, txdata, &scriptchecks));
+ BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks));
BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size());
}
}
@@ -204,13 +204,13 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
CValidationState state;
PrecomputedTransactionData ptd_spend_tx(spend_tx);
- BOOST_CHECK(!CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), true, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr));
+ BOOST_CHECK(!CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr));
// If we call again asking for scriptchecks (as happens in
// ConnectBlock), we should add a script check object for this -- we're
// not caching invalidity (if that changes, delete this test case).
std::vector<CScriptCheck> scriptchecks;
- BOOST_CHECK(CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), true, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks));
+ BOOST_CHECK(CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks));
BOOST_CHECK_EQUAL(scriptchecks.size(), 1U);
// Test that CheckInputs returns true iff DERSIG-enforcing flags are
@@ -272,7 +272,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
CValidationState state;
PrecomputedTransactionData txdata(invalid_with_cltv_tx);
- BOOST_CHECK(CheckInputs(CTransaction(invalid_with_cltv_tx), state, ::ChainstateActive().CoinsTip(), true, SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr));
+ BOOST_CHECK(CheckInputs(CTransaction(invalid_with_cltv_tx), state, ::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr));
}
// TEST CHECKSEQUENCEVERIFY
@@ -300,7 +300,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
CValidationState state;
PrecomputedTransactionData txdata(invalid_with_csv_tx);
- BOOST_CHECK(CheckInputs(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), true, SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr));
+ BOOST_CHECK(CheckInputs(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr));
}
// TODO: add tests for remaining script flags
@@ -362,12 +362,12 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
CValidationState state;
PrecomputedTransactionData txdata(tx);
// This transaction is now invalid under segwit, because of the second input.
- BOOST_CHECK(!CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), true, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, nullptr));
+ BOOST_CHECK(!CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, nullptr));
std::vector<CScriptCheck> scriptchecks;
// Make sure this transaction was not cached (ie because the first
// input was valid)
- BOOST_CHECK(CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), true, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, &scriptchecks));
+ BOOST_CHECK(CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, &scriptchecks));
// Should get 2 script checks back -- caching is on a whole-transaction basis.
BOOST_CHECK_EQUAL(scriptchecks.size(), 2U);
}
diff --git a/src/validation.cpp b/src/validation.cpp
index 74f68a304..c1646743f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -184,7 +184,7 @@ std::unique_ptr<CBlockTreeDB> pblocktree;
// See definition for documentation
static void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight);
static void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight);
-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks = nullptr);
+bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks = nullptr);
static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false);
static FlatFileSeq BlockFileSeq();
static FlatFileSeq UndoFileSeq();
@@ -425,7 +425,7 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, CValidationSt
}
}
- return CheckInputs(tx, state, view, true, flags, cacheSigStore, true, txdata);
+ return CheckInputs(tx, state, view, flags, cacheSigStore, true, txdata);
}
/**
@@ -773,15 +773,17 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS;
// Check against previous transactions
- // This is done last to help prevent CPU exhaustion denial-of-service attacks.
+ // The first loop above does all the inexpensive checks.
+ // Only if ALL inputs pass do we perform expensive ECDSA signature checks.
+ // Helps prevent CPU exhaustion denial-of-service attacks.
PrecomputedTransactionData txdata(tx);
- if (!CheckInputs(tx, state, view, true, scriptVerifyFlags, true, false, txdata)) {
+ if (!CheckInputs(tx, state, view, scriptVerifyFlags, true, false, txdata)) {
// SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
// need to turn both off, and compare against just turning off CLEANSTACK
// to see if the failure is specifically due to witness validation.
CValidationState stateDummy; // Want reported failures to be from first CheckInputs
- if (!tx.HasWitness() && CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) &&
- !CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) {
+ if (!tx.HasWitness() && CheckInputs(tx, stateDummy, view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) &&
+ !CheckInputs(tx, stateDummy, view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) {
// Only the witness is missing, so the transaction itself may be fine.
state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false,
state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage());
@@ -1298,90 +1300,79 @@ void InitScriptExecutionCache() {
*
* Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp
*/
-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
- if (!tx.IsCoinBase())
- {
- if (pvChecks)
- pvChecks->reserve(tx.vin.size());
+ if (tx.IsCoinBase()) return true;
- // The first loop above does all the inexpensive checks.
- // Only if ALL inputs pass do we perform expensive ECDSA signature checks.
- // Helps prevent CPU exhaustion attacks.
+ if (pvChecks) {
+ pvChecks->reserve(tx.vin.size());
+ }
- // Skip script verification when connecting blocks under the
- // assumevalid block. Assuming the assumevalid block is valid this
- // is safe because block merkle hashes are still computed and checked,
- // Of course, if an assumed valid block is invalid due to false scriptSigs
- // this optimization would allow an invalid chain to be accepted.
- if (fScriptChecks) {
- // First check if script executions have been cached with the same
- // flags. Note that this assumes that the inputs provided are
- // correct (ie that the transaction hash which is in tx's prevouts
- // properly commits to the scriptPubKey in the inputs view of that
- // transaction).
- uint256 hashCacheEntry;
- // We only use the first 19 bytes of nonce to avoid a second SHA
- // round - giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64)
- static_assert(55 - sizeof(flags) - 32 >= 128/8, "Want at least 128 bits of nonce for script execution cache");
- CSHA256().Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
- AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
- if (scriptExecutionCache.contains(hashCacheEntry, !cacheFullScriptStore)) {
- return true;
- }
-
- for (unsigned int i = 0; i < tx.vin.size(); i++) {
- const COutPoint &prevout = tx.vin[i].prevout;
- const Coin& coin = inputs.AccessCoin(prevout);
- assert(!coin.IsSpent());
-
- // We very carefully only pass in things to CScriptCheck which
- // are clearly committed to by tx' witness hash. This provides
- // a sanity check that our caching is not introducing consensus
- // failures through additional data in, eg, the coins being
- // spent being checked as a part of CScriptCheck.
-
- // Verify signature
- CScriptCheck check(coin.out, tx, i, flags, cacheSigStore, &txdata);
- if (pvChecks) {
- pvChecks->push_back(CScriptCheck());
- check.swap(pvChecks->back());
- } else if (!check()) {
- if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
- // Check whether the failure was caused by a
- // non-mandatory script verification check, such as
- // non-standard DER encodings or non-null dummy
- // arguments; if so, ensure we return NOT_STANDARD
- // instead of CONSENSUS to avoid downstream users
- // splitting the network between upgraded and
- // non-upgraded nodes by banning CONSENSUS-failing
- // data providers.
- CScriptCheck check2(coin.out, tx, i,
- flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
- if (check2())
- return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
- }
- // MANDATORY flag failures correspond to
- // ValidationInvalidReason::CONSENSUS. Because CONSENSUS
- // failures are the most serious case of validation
- // failures, we may need to consider using
- // RECENT_CONSENSUS_CHANGE for any script failure that
- // could be due to non-upgraded nodes which we may want to
- // support, to avoid splitting the network (but this
- // depends on the details of how net_processing handles
- // such errors).
- return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError())));
- }
- }
-
- if (cacheFullScriptStore && !pvChecks) {
- // We executed all of the provided scripts, and were told to
- // cache the result. Do so now.
- scriptExecutionCache.insert(hashCacheEntry);
+ // First check if script executions have been cached with the same
+ // flags. Note that this assumes that the inputs provided are
+ // correct (ie that the transaction hash which is in tx's prevouts
+ // properly commits to the scriptPubKey in the inputs view of that
+ // transaction).
+ uint256 hashCacheEntry;
+ // We only use the first 19 bytes of nonce to avoid a second SHA
+ // round - giving us 19 + 32 + 4 = 55 bytes (+ 8 + 1 = 64)
+ static_assert(55 - sizeof(flags) - 32 >= 128/8, "Want at least 128 bits of nonce for script execution cache");
+ CSHA256().Write(scriptExecutionCacheNonce.begin(), 55 - sizeof(flags) - 32).Write(tx.GetWitnessHash().begin(), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
+ AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
+ if (scriptExecutionCache.contains(hashCacheEntry, !cacheFullScriptStore)) {
+ return true;
+ }
+
+ for (unsigned int i = 0; i < tx.vin.size(); i++) {
+ const COutPoint &prevout = tx.vin[i].prevout;
+ const Coin& coin = inputs.AccessCoin(prevout);
+ assert(!coin.IsSpent());
+
+ // We very carefully only pass in things to CScriptCheck which
+ // are clearly committed to by tx' witness hash. This provides
+ // a sanity check that our caching is not introducing consensus
+ // failures through additional data in, eg, the coins being
+ // spent being checked as a part of CScriptCheck.
+
+ // Verify signature
+ CScriptCheck check(coin.out, tx, i, flags, cacheSigStore, &txdata);
+ if (pvChecks) {
+ pvChecks->push_back(CScriptCheck());
+ check.swap(pvChecks->back());
+ } else if (!check()) {
+ if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
+ // Check whether the failure was caused by a
+ // non-mandatory script verification check, such as
+ // non-standard DER encodings or non-null dummy
+ // arguments; if so, ensure we return NOT_STANDARD
+ // instead of CONSENSUS to avoid downstream users
+ // splitting the network between upgraded and
+ // non-upgraded nodes by banning CONSENSUS-failing
+ // data providers.
+ CScriptCheck check2(coin.out, tx, i,
+ flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
+ if (check2())
+ return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
}
+ // MANDATORY flag failures correspond to
+ // ValidationInvalidReason::CONSENSUS. Because CONSENSUS
+ // failures are the most serious case of validation
+ // failures, we may need to consider using
+ // RECENT_CONSENSUS_CHANGE for any script failure that
+ // could be due to non-upgraded nodes which we may want to
+ // support, to avoid splitting the network (but this
+ // depends on the details of how net_processing handles
+ // such errors).
+ return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError())));
}
}
+ if (cacheFullScriptStore && !pvChecks) {
+ // We executed all of the provided scripts, and were told to
+ // cache the result. Do so now.
+ scriptExecutionCache.insert(hashCacheEntry);
+ }
+
return true;
}
@@ -1769,6 +1760,11 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
pindexBestHeader->GetAncestor(pindex->nHeight) == pindex &&
pindexBestHeader->nChainWork >= nMinimumChainWork) {
// This block is a member of the assumed verified chain and an ancestor of the best header.
+ // Script verification is skipped when connecting blocks under the
+ // assumevalid block. Assuming the assumevalid block is valid this
+ // is safe because block merkle hashes are still computed and checked,
+ // Of course, if an assumed valid block is invalid due to false scriptSigs
+ // this optimization would allow an invalid chain to be accepted.
// The equivalent time check discourages hash power from extorting the network via DOS attack
// into accepting an invalid block through telling users they must manually set assumevalid.
// Requiring a software change or burying the invalid block, regardless of the setting, makes
@@ -1952,7 +1948,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
{
std::vector<CScriptCheck> vChecks;
bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
- if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
+ if (fScriptChecks && !CheckInputs(tx, state, view, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) {
if (state.GetReason() == ValidationInvalidReason::TX_NOT_STANDARD) {
// CheckInputs may return NOT_STANDARD for extra flags we passed,
// but we can't return that, as it's not defined for a block, so