From ca7ee7c27811a0f6bb75efe03ff4d8137aa3af60 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 2 Jan 2020 11:08:52 -0500 Subject: [PATCH] Merge #16658: validation: Rename CheckInputs to CheckInputScripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3bd8db80d8d335ab63ece4f110b0fadd562e80b7 [validation] fix comments in CheckInputScripts() (John Newbery) 6f6465cefcd599c89c00f7b51f42a4b87a5ffb0b scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery) Pull request description: CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e074, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). Also fix incorrect comments. ACKs for top commit: MarcoFalke: re-ACK 3bd8db80d8d335ab63ece4f110b0fadd562e80b7, did the rebase myself, checked the scripted diff 👡 promag: ACK 3bd8db80d8d335ab63ece4f110b0fadd562e80b7 :trollface: Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c --- src/script/standard.h | 2 +- src/test/txvalidationcache_tests.cpp | 26 +++++++++---------- src/validation.cpp | 38 +++++++++++++++------------- test/functional/feature_cltv.py | 2 +- test/functional/feature_dersig.py | 2 +- 5 files changed, 36 insertions(+), 34 deletions(-) diff --git a/src/script/standard.h b/src/script/standard.h index 1fe7f7b0f8..ed6ddabe29 100644 --- a/src/script/standard.h +++ b/src/script/standard.h @@ -108,7 +108,7 @@ extern unsigned nMaxDatacarrierBytes; * them to be valid. (but old blocks may not comply with) Currently just P2SH, * but in the future other flags may be added. * - * Failing one of these tests may trigger a DoS ban - see CheckInputs() for + * Failing one of these tests may trigger a DoS ban - see CheckInputScripts() for * details. */ static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH; diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index f794180876..2d80cf76d0 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -12,7 +12,7 @@ #include -bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks); +bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks); BOOST_AUTO_TEST_SUITE(txvalidationcache_tests) @@ -94,8 +94,8 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U); } -// Run CheckInputs (using CoinsTip()) on the given transaction, for all script -// flags. Test that CheckInputs passes for all flags that don't overlap with +// Run CheckInputScripts (using CoinsTip()) on the given transaction, for all script +// flags. Test that CheckInputScripts passes for all flags that don't overlap with // the failing_flags argument, but otherwise fails. // CHECKLOCKTIMEVERIFY and CHECKSEQUENCEVERIFY (and future NOP codes that may // get reassigned) have an interaction with DISCOURAGE_UPGRADABLE_NOPS: if @@ -118,8 +118,8 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail // script/interpreter.cpp test_flags |= SCRIPT_VERIFY_P2SH; } - 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 + bool ret = CheckInputScripts(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, nullptr); + // CheckInputScripts should succeed iff test_flags doesn't intersect with // failing_flags bool expected_return_value = !(test_flags & failing_flags); BOOST_CHECK_EQUAL(ret, expected_return_value); @@ -128,13 +128,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 scriptchecks; - BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(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 scriptchecks; - BOOST_CHECK(CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, &scriptchecks)); BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size()); } } @@ -142,7 +142,7 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) { - // Test that passing CheckInputs with one set of script flags doesn't imply + // Test that passing CheckInputScripts with one set of script flags doesn't imply // that we would pass again with a different set of flags. { LOCK(cs_main); @@ -196,16 +196,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) TxValidationState state; PrecomputedTransactionData ptd_spend_tx; - BOOST_CHECK(!CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr)); + BOOST_CHECK(!CheckInputScripts(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 scriptchecks; - BOOST_CHECK(CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(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 + // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are // not present. Don't add these checks to the cache, so that we can // test later that block validation works fine in the absence of cached // successes. @@ -264,7 +264,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; TxValidationState state; PrecomputedTransactionData txdata; - BOOST_CHECK(CheckInputs(CTransaction(invalid_with_cltv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr)); + BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr)); } // TEST CHECKSEQUENCEVERIFY @@ -292,7 +292,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup) invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; TxValidationState state; PrecomputedTransactionData txdata; - BOOST_CHECK(CheckInputs(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr)); + BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr)); } // TODO: add tests for remaining script flags diff --git a/src/validation.cpp b/src/validation.cpp index 711656ed4d..5c2bcc3d24 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -200,7 +200,7 @@ CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlo std::unique_ptr pblocktree; -bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); +bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); @@ -490,19 +490,19 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS // pool.cs should be locked already, but go ahead and re-take the lock here // to enforce that mempool doesn't change between when we check the view - // and when we actually call through to CheckInputs + // and when we actually call through to CheckInputScripts LOCK(pool.cs); assert(!tx.IsCoinBase()); for (const CTxIn& txin : tx.vin) { const Coin& coin = view.AccessCoin(txin.prevout); - // At this point we haven't actually checked if the coins are all - // available (or shouldn't assume we have, since CheckInputs does). - // So we just return failure if the inputs are not available here, - // and then only have to check equivalence for available inputs. + // AcceptToMemoryPoolWorker has already checked that the coins are + // available, so this shouldn't fail. If the inputs are not available + // here then return false. if (coin.IsSpent()) return false; + // Check equivalence for available inputs. const CTransactionRef& txFrom = pool.get(txin.prevout.hash); if (txFrom) { assert(txFrom->GetHash() == txin.prevout.hash); @@ -516,8 +516,8 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS } } - // Call CheckInputs() to cache signature and script validity against current tip consensus rules. - return CheckInputs(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); + // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules. + return CheckInputScripts(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); } namespace { @@ -838,10 +838,10 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS; - // Check against previous transactions + // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputs(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { - return false; // state filled in by CheckInputs + if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { + return false; // state filled in by CheckInputScripts } return true; @@ -874,7 +874,7 @@ bool MemPoolAccept::ConsensusScriptChecks(ATMPArgs& args, Workspace& ws, Precomp unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus()); assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(m_active_chainstate.CoinsTip())); if (!CheckInputsFromMempoolAndCache(tx, state, m_view, m_pool, currentBlockScriptVerifyFlags, txdata, m_active_chainstate.CoinsTip())) { - return error("%s: BUG! PLEASE REPORT THIS! CheckInputs failed against latest-block but not STANDARD flags %s, %s", + return error("%s: BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not STANDARD flags %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); } @@ -1508,8 +1508,10 @@ void InitScriptExecutionCache() { } /** - * Check whether all inputs of this transaction are valid (no double spends, scripts & sigs, amounts) - * This does not modify the UTXO set. + * Check whether all of this transaction's input scripts succeed. + * + * This involves ECDSA signature checks so can be computationally intensive. This function should + * only be called after the cheap sanity checks in CheckTxInputs passed. * * If pvChecks is not nullptr, script checks are pushed onto it instead of being performed inline. Any * script checks which are not necessary (eg due to script execution cache hits) are, obviously, @@ -1524,7 +1526,7 @@ void InitScriptExecutionCache() { * * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { boost::posix_time::ptime start = boost::posix_time::microsec_clock::local_time(); if (tx.IsCoinBase()) return true; @@ -1605,7 +1607,7 @@ bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsV boost::posix_time::ptime finish = boost::posix_time::microsec_clock::local_time(); boost::posix_time::time_duration diff = finish - start; - statsClient.timing("CheckInputs_ms", diff.total_milliseconds(), 1.0f); + statsClient.timing("CheckInputScripts_ms", diff.total_milliseconds(), 1.0f); return true; } @@ -2357,11 +2359,11 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ TxValidationState tx_state; - if (fScriptChecks && !CheckInputs(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], g_parallel_script_checks ? &vChecks : nullptr)) { + if (fScriptChecks && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], g_parallel_script_checks ? &vChecks : nullptr)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); - LogPrintf("ERROR: ConnectBlock(): CheckInputs on %s failed with %s\n", + LogPrintf("ERROR: ConnectBlock(): CheckInputScripts on %s failed with %s\n", tx.GetHash().ToString(), FormatStateMessage(state)); return false; } diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index 28ec088df3..971cd6ecf5 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -137,7 +137,7 @@ class BIP65Test(BitcoinTestFramework): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputs on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]): + with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]): self.nodes[0].p2p.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) self.nodes[0].p2p.sync_with_ping() diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index ab3d6635ae..5ab9dfbba7 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -120,7 +120,7 @@ class BIP66Test(BitcoinTestFramework): block.rehash() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputs on {} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)'.format(block.vtx[-1].hash)]): + with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)'.format(block.vtx[-1].hash)]): self.nodes[0].p2p.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) self.nodes[0].p2p.sync_with_ping()