Merge #16658: validation: Rename CheckInputs to CheckInputScripts

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
This commit is contained in:
MarcoFalke 2020-01-02 11:08:52 -05:00 committed by UdjinM6
parent 83a7235c09
commit ca7ee7c278
5 changed files with 36 additions and 34 deletions

View File

@ -108,7 +108,7 @@ extern unsigned nMaxDatacarrierBytes;
* them to be valid. (but old blocks may not comply with) Currently just P2SH, * them to be valid. (but old blocks may not comply with) Currently just P2SH,
* but in the future other flags may be added. * 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. * details.
*/ */
static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH; static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH;

View File

@ -12,7 +12,7 @@
#include <boost/test/unit_test.hpp> #include <boost/test/unit_test.hpp>
bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks); bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks);
BOOST_AUTO_TEST_SUITE(txvalidationcache_tests) 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); BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U);
} }
// Run CheckInputs (using CoinsTip()) on the given transaction, for all script // Run CheckInputScripts (using CoinsTip()) on the given transaction, for all script
// flags. Test that CheckInputs passes for all flags that don't overlap with // flags. Test that CheckInputScripts passes for all flags that don't overlap with
// the failing_flags argument, but otherwise fails. // the failing_flags argument, but otherwise fails.
// CHECKLOCKTIMEVERIFY and CHECKSEQUENCEVERIFY (and future NOP codes that may // CHECKLOCKTIMEVERIFY and CHECKSEQUENCEVERIFY (and future NOP codes that may
// get reassigned) have an interaction with DISCOURAGE_UPGRADABLE_NOPS: if // 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 // script/interpreter.cpp
test_flags |= SCRIPT_VERIFY_P2SH; test_flags |= SCRIPT_VERIFY_P2SH;
} }
bool ret = CheckInputs(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, nullptr); bool ret = CheckInputScripts(tx, state, &::ChainstateActive().CoinsTip(), test_flags, true, add_to_cache, txdata, nullptr);
// CheckInputs should succeed iff test_flags doesn't intersect with // CheckInputScripts should succeed iff test_flags doesn't intersect with
// failing_flags // failing_flags
bool expected_return_value = !(test_flags & failing_flags); bool expected_return_value = !(test_flags & failing_flags);
BOOST_CHECK_EQUAL(ret, expected_return_value); 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) { if (ret && add_to_cache) {
// Check that we get a cache hit if the tx was valid // Check that we get a cache hit if the tx was valid
std::vector<CScriptCheck> scriptchecks; std::vector<CScriptCheck> 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()); BOOST_CHECK(scriptchecks.empty());
} else { } else {
// Check that we get script executions to check, if the transaction // Check that we get script executions to check, if the transaction
// was invalid, or we didn't add to cache. // was invalid, or we didn't add to cache.
std::vector<CScriptCheck> scriptchecks; std::vector<CScriptCheck> 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()); 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) 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. // that we would pass again with a different set of flags.
{ {
LOCK(cs_main); LOCK(cs_main);
@ -196,16 +196,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
TxValidationState state; TxValidationState state;
PrecomputedTransactionData ptd_spend_tx; 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 // If we call again asking for scriptchecks (as happens in
// ConnectBlock), we should add a script check object for this -- we're // ConnectBlock), we should add a script check object for this -- we're
// not caching invalidity (if that changes, delete this test case). // not caching invalidity (if that changes, delete this test case).
std::vector<CScriptCheck> scriptchecks; std::vector<CScriptCheck> 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); 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 // 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 // test later that block validation works fine in the absence of cached
// successes. // successes.
@ -264,7 +264,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
TxValidationState state; TxValidationState state;
PrecomputedTransactionData txdata; 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 // TEST CHECKSEQUENCEVERIFY
@ -292,7 +292,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
TxValidationState state; TxValidationState state;
PrecomputedTransactionData txdata; 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 // TODO: add tests for remaining script flags

View File

@ -200,7 +200,7 @@ CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlo
std::unique_ptr<CBlockTreeDB> pblocktree; std::unique_ptr<CBlockTreeDB> pblocktree;
bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks = nullptr); bool CheckInputScripts(const CTransaction& tx, TxValidationState &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 FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false);
static FlatFileSeq BlockFileSeq(); static FlatFileSeq BlockFileSeq();
static FlatFileSeq UndoFileSeq(); 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 // 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 // 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); LOCK(pool.cs);
assert(!tx.IsCoinBase()); assert(!tx.IsCoinBase());
for (const CTxIn& txin : tx.vin) { for (const CTxIn& txin : tx.vin) {
const Coin& coin = view.AccessCoin(txin.prevout); const Coin& coin = view.AccessCoin(txin.prevout);
// At this point we haven't actually checked if the coins are all // AcceptToMemoryPoolWorker has already checked that the coins are
// available (or shouldn't assume we have, since CheckInputs does). // available, so this shouldn't fail. If the inputs are not available
// So we just return failure if the inputs are not available here, // here then return false.
// and then only have to check equivalence for available inputs.
if (coin.IsSpent()) return false; if (coin.IsSpent()) return false;
// Check equivalence for available inputs.
const CTransactionRef& txFrom = pool.get(txin.prevout.hash); const CTransactionRef& txFrom = pool.get(txin.prevout.hash);
if (txFrom) { if (txFrom) {
assert(txFrom->GetHash() == txin.prevout.hash); 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. // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules.
return CheckInputs(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata); return CheckInputScripts(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata);
} }
namespace { namespace {
@ -838,10 +838,10 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute
constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS; 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. // This is done last to help prevent CPU exhaustion denial-of-service attacks.
if (!CheckInputs(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) { if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, txdata)) {
return false; // state filled in by CheckInputs return false; // state filled in by CheckInputScripts
} }
return true; 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()); unsigned int currentBlockScriptVerifyFlags = GetBlockScriptFlags(m_active_chainstate.m_chain.Tip(), chainparams.GetConsensus());
assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(m_active_chainstate.CoinsTip())); 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())) { 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)); __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) * Check whether all of this transaction's input scripts succeed.
* This does not modify the UTXO set. *
* 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 * 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, * 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 * 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<CScriptCheck> *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<CScriptCheck> *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{ {
boost::posix_time::ptime start = boost::posix_time::microsec_clock::local_time(); boost::posix_time::ptime start = boost::posix_time::microsec_clock::local_time();
if (tx.IsCoinBase()) return true; 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::ptime finish = boost::posix_time::microsec_clock::local_time();
boost::posix_time::time_duration diff = finish - start; 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; return true;
} }
@ -2357,11 +2359,11 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
std::vector<CScriptCheck> vChecks; std::vector<CScriptCheck> vChecks;
bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
TxValidationState tx_state; 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 // Any transaction validation failure in ConnectBlock is a block consensus failure
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
tx_state.GetRejectReason(), tx_state.GetDebugMessage()); 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)); tx.GetHash().ToString(), FormatStateMessage(state));
return false; return false;
} }

View File

@ -137,7 +137,7 @@ class BIP65Test(BitcoinTestFramework):
block.hashMerkleRoot = block.calc_merkle_root() block.hashMerkleRoot = block.calc_merkle_root()
block.solve() 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)) self.nodes[0].p2p.send_and_ping(msg_block(block))
assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
self.nodes[0].p2p.sync_with_ping() self.nodes[0].p2p.sync_with_ping()

View File

@ -120,7 +120,7 @@ class BIP66Test(BitcoinTestFramework):
block.rehash() block.rehash()
block.solve() 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)) self.nodes[0].p2p.send_and_ping(msg_block(block))
assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
self.nodes[0].p2p.sync_with_ping() self.nodes[0].p2p.sync_with_ping()