diff --git a/src/script/script_metrics.h b/src/script/script_metrics.h --- a/src/script/script_metrics.h +++ b/src/script/script_metrics.h @@ -18,6 +18,10 @@ bool operator==(const ScriptExecutionMetrics &rhs) { return nSigChecks == rhs.nSigChecks; } + ScriptExecutionMetrics &operator+=(const ScriptExecutionMetrics &rhs) { + nSigChecks += rhs.nSigChecks; + return *this; + } }; #endif // BITCOIN_SCRIPT_SCRIPT_METRICS_H diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -106,7 +106,8 @@ // evaluating any script flag that is implemented as an upgraded NOP code. static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t failing_flags, - uint32_t required_flags, bool add_to_cache) + uint32_t required_flags, bool add_to_cache, + ScriptExecutionMetrics expected_metrics) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { PrecomputedTransactionData txdata(tx); @@ -122,29 +123,43 @@ test_flags |= SCRIPT_VERIFY_P2SH; } - bool ret = CheckInputs(tx, state, pcoinsTip.get(), true, test_flags, - true, add_to_cache, txdata, nullptr); + ScriptExecutionMetrics metricsDirect(0xf00d); + bool ret = + CheckInputs(tx, state, pcoinsTip.get(), true, test_flags, true, + add_to_cache, txdata, metricsDirect, nullptr); // CheckInputs 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); + if (ret) { + ScriptExecutionMetrics metricsExpectedForFlags = expected_metrics; + if (!(test_flags & SCRIPT_REPORT_SIGCHECKS)) { + metricsExpectedForFlags.nSigChecks = 0; + } + BOOST_CHECK(metricsExpectedForFlags == metricsDirect); + } + // Test the caching if (ret && add_to_cache) { // Check that we get a cache hit if the tx was valid std::vector scriptchecks; + ScriptExecutionMetrics metricsCached(0xbeef); BOOST_CHECK(CheckInputs(tx, state, pcoinsTip.get(), true, test_flags, true, add_to_cache, txdata, - &scriptchecks)); + metricsCached, &scriptchecks)); + BOOST_CHECK(metricsCached == metricsDirect); 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; + ScriptExecutionMetrics metricsUncached(0xbabe); BOOST_CHECK(CheckInputs(tx, state, pcoinsTip.get(), true, test_flags, true, add_to_cache, txdata, - &scriptchecks)); + metricsUncached, &scriptchecks)); + BOOST_CHECK(!ret || metricsUncached == ScriptExecutionMetrics()); BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size()); } } @@ -240,10 +255,11 @@ CValidationState state; PrecomputedTransactionData ptd_spend_tx(tx); + ScriptExecutionMetrics metricsDummy; BOOST_CHECK(!CheckInputs(tx, state, pcoinsTip.get(), true, STANDARD_SCRIPT_VERIFY_FLAGS, true, true, - ptd_spend_tx, nullptr)); + ptd_spend_tx, metricsDummy, nullptr)); // If we call again asking for scriptchecks (as happens in // ConnectBlock), we should add a script check object for this -- we're @@ -251,15 +267,16 @@ std::vector scriptchecks; BOOST_CHECK(CheckInputs(tx, state, pcoinsTip.get(), true, STANDARD_SCRIPT_VERIFY_FLAGS, true, true, - ptd_spend_tx, &scriptchecks)); + ptd_spend_tx, metricsDummy, &scriptchecks)); BOOST_CHECK_EQUAL(scriptchecks.size(), 1U); // Test that CheckInputs returns true iff cleanstack-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. - ValidateCheckInputsForAllFlags( - tx, SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS, 0, false); + ValidateCheckInputsForAllFlags(tx, + SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS, + 0, false, ScriptExecutionMetrics(0)); } // And if we produce a block with this tx, it should be valid, even though @@ -286,7 +303,8 @@ invalid_under_p2sh_tx.vin[0].scriptSig << vchSig2; ValidateCheckInputsForAllFlags(CTransaction(invalid_under_p2sh_tx), - SCRIPT_VERIFY_P2SH, 0, true); + SCRIPT_VERIFY_P2SH, 0, true, + ScriptExecutionMetrics(0)); } // Test CHECKLOCKTIMEVERIFY @@ -310,10 +328,10 @@ vchSig.push_back(uint8_t(SIGHASH_ALL | SIGHASH_FORKID)); invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 101; - ValidateCheckInputsForAllFlags(CTransaction(invalid_with_cltv_tx), - SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | - SCRIPT_ENABLE_REPLAY_PROTECTION, - SCRIPT_ENABLE_SIGHASH_FORKID, true); + ValidateCheckInputsForAllFlags( + CTransaction(invalid_with_cltv_tx), + SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY | SCRIPT_ENABLE_REPLAY_PROTECTION, + SCRIPT_ENABLE_SIGHASH_FORKID, true, ScriptExecutionMetrics(1)); // Make it valid, and check again invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; @@ -322,9 +340,12 @@ CTransaction transaction(invalid_with_cltv_tx); PrecomputedTransactionData txdata(transaction); - BOOST_CHECK(CheckInputs(transaction, state, pcoinsTip.get(), true, - STANDARD_SCRIPT_VERIFY_FLAGS, true, true, - txdata, nullptr)); + ScriptExecutionMetrics metricsRet; + BOOST_CHECK( + CheckInputs(transaction, state, pcoinsTip.get(), true, + STANDARD_SCRIPT_VERIFY_FLAGS | SCRIPT_REPORT_SIGCHECKS, + true, true, txdata, metricsRet, nullptr)); + BOOST_CHECK(metricsRet == ScriptExecutionMetrics(1)); } // TEST CHECKSEQUENCEVERIFY @@ -347,10 +368,10 @@ vchSig.push_back(uint8_t(SIGHASH_ALL | SIGHASH_FORKID)); invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 101; - ValidateCheckInputsForAllFlags(CTransaction(invalid_with_csv_tx), - SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | - SCRIPT_ENABLE_REPLAY_PROTECTION, - SCRIPT_ENABLE_SIGHASH_FORKID, true); + ValidateCheckInputsForAllFlags( + CTransaction(invalid_with_csv_tx), + SCRIPT_VERIFY_CHECKSEQUENCEVERIFY | SCRIPT_ENABLE_REPLAY_PROTECTION, + SCRIPT_ENABLE_SIGHASH_FORKID, true, ScriptExecutionMetrics(1)); // Make it valid, and check again invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; @@ -359,9 +380,12 @@ CTransaction transaction(invalid_with_csv_tx); PrecomputedTransactionData txdata(transaction); - BOOST_CHECK(CheckInputs(transaction, state, pcoinsTip.get(), true, - STANDARD_SCRIPT_VERIFY_FLAGS, true, true, - txdata, nullptr)); + ScriptExecutionMetrics metricsRet; + BOOST_CHECK( + CheckInputs(transaction, state, pcoinsTip.get(), true, + STANDARD_SCRIPT_VERIFY_FLAGS | SCRIPT_REPORT_SIGCHECKS, + true, true, txdata, metricsRet, nullptr)); + BOOST_CHECK(metricsRet == ScriptExecutionMetrics(1)); } // TODO: add tests for remaining script flags @@ -402,7 +426,8 @@ // convention. ValidateCheckInputsForAllFlags( CTransaction(tx), SCRIPT_ENABLE_REPLAY_PROTECTION, - SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_VERIFY_P2SH, true); + SCRIPT_ENABLE_SIGHASH_FORKID | SCRIPT_VERIFY_P2SH, true, + ScriptExecutionMetrics(2)); // Check that if the second input is invalid, but the first input is // valid, the transaction is not cached. @@ -415,10 +440,11 @@ // This transaction is now invalid because the second signature is // missing. + ScriptExecutionMetrics metricsDummy; BOOST_CHECK( !CheckInputs(transaction, state, pcoinsTip.get(), true, STANDARD_SCRIPT_VERIFY_FLAGS | SCRIPT_REPORT_SIGCHECKS, - true, true, txdata, nullptr)); + true, true, txdata, metricsDummy, nullptr)); // Make sure this transaction was not cached (ie becausethe first input // was valid) @@ -426,7 +452,7 @@ BOOST_CHECK( CheckInputs(transaction, state, pcoinsTip.get(), true, STANDARD_SCRIPT_VERIFY_FLAGS | SCRIPT_REPORT_SIGCHECKS, - true, true, txdata, &scriptchecks)); + true, true, txdata, metricsDummy, &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.h b/src/validation.h --- a/src/validation.h +++ b/src/validation.h @@ -473,6 +473,12 @@ * performed inline. Any script checks which are not necessary (eg due to script * execution cache hits) are, obviously, not pushed onto pvChecks/run. * + * Upon success metricsOut will be filled in with either: + * - correct metrics total for all inputs, or, + * - empty metrics, in the case when checks were pushed onto pvChecks (i.e., + * a cache miss with pvChecks non-null), in which case the metrics can be + * found by executing the pushed pvChecks. + * * Setting sigCacheStore/scriptCacheStore to false will remove elements from the * corresponding cache which are matched. This is useful for checking blocks * where we will likely never need the cache entry again. @@ -482,6 +488,7 @@ const uint32_t flags, bool sigCacheStore, bool scriptCacheStore, const PrecomputedTransactionData &txdata, + ScriptExecutionMetrics &metricsOut, std::vector *pvChecks = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -491,8 +491,8 @@ static bool CheckInputsFromMempoolAndCache( const CTransaction &tx, CValidationState &state, const CCoinsViewCache &view, const CTxMemPool &pool, const uint32_t flags, - bool cacheSigStore, PrecomputedTransactionData &txdata) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + bool cacheSigStore, PrecomputedTransactionData &txdata, + ScriptExecutionMetrics &metricsOut) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); // pool.cs should be locked already, but go ahead and re-take the lock here @@ -525,7 +525,7 @@ } return CheckInputs(tx, state, view, true, flags, cacheSigStore, true, - txdata); + txdata, metricsOut); } static bool @@ -758,8 +758,9 @@ // Check against previous transactions. This is done last to help // prevent CPU exhaustion denial-of-service attacks. PrecomputedTransactionData txdata(tx); + ScriptExecutionMetrics metricsStandard; if (!CheckInputs(tx, state, view, true, scriptVerifyFlags, true, false, - txdata)) { + txdata, metricsStandard)) { // State filled in by CheckInputs. return false; } @@ -778,9 +779,10 @@ uint32_t nextBlockScriptVerifyFlags = GetNextBlockScriptFlags(consensusParams, chainActive.Tip()); + ScriptExecutionMetrics metricsConsensus; if (!CheckInputsFromMempoolAndCache(tx, state, view, pool, nextBlockScriptVerifyFlags, true, - txdata)) { + txdata, metricsConsensus)) { // This can occur under some circumstances, if the node receives an // unrequested tx which is invalid due to new consensus rules not // being activated yet (during IBD). @@ -789,6 +791,24 @@ __func__, txid.ToString(), FormatStateMessage(state)); } + if (!(metricsStandard == metricsConsensus)) { + // A poorly-designed standard flag might be introduced that allows + // scripts to branch depending on standard/consensus rules, and + // thereby succeed in different ways with different metrics. + // + // This is annoying, but it's safe to keep going, as we will anyway + // save the consensus metrics to the mempool so that block template + // creation is accurate. + // + // This may also happen if a new uncached field in + // ScriptExecutionMetrics is introduced and so we get different + // metrics cached/uncached. + LogPrintf("%s: BUG! PLEASE REPORT THIS! CheckInputs detected " + "different script metrics for evaluation under " + "next-block and standard flags %s\n", + __func__, txid.ToString()); + } + if (test_accept) { // Tx was accepted, but not added return true; @@ -1211,6 +1231,7 @@ const uint32_t flags, bool sigCacheStore, bool scriptCacheStore, const PrecomputedTransactionData &txdata, + ScriptExecutionMetrics &metricsOut, std::vector *pvChecks) { AssertLockHeld(cs_main); assert(!tx.IsCoinBase()); @@ -1233,11 +1254,22 @@ // transaction hash which is in tx's prevouts properly commits to the // scriptPubKey in the inputs view of that transaction). ScriptCacheKey hashCacheEntry(tx, flags); - ScriptExecutionMetrics metricsDummy; - if (IsKeyInScriptCache(hashCacheEntry, !scriptCacheStore, metricsDummy)) { + if (IsKeyInScriptCache(hashCacheEntry, !scriptCacheStore, metricsOut)) { return true; } + ScriptExecutionMetrics metricsTotal = {}; + + // Sanity check: + // An input is at least 41 bytes large, and can contain up to + // MAX_OPS_PER_SCRIPT operations. Make sure the worst case can't overflow. + constexpr auto maxinputs_metrics = + std::numeric_limits::max() / + MAX_OPS_PER_SCRIPT; + constexpr auto maxinputs_tx = MAX_TX_SIZE / 41; + static_assert(maxinputs_metrics > maxinputs_tx, + "metrics accumulator sigchecks mustn't overflow"); + for (size_t i = 0; i < tx.vin.size(); i++) { const COutPoint &prevout = tx.vin[i].prevout; const Coin &coin = inputs.AccessCoin(prevout); @@ -1292,13 +1324,16 @@ strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(scriptError))); } + + metricsTotal += check.GetScriptExecutionMetrics(); } + metricsOut = metricsTotal; + if (scriptCacheStore && !pvChecks) { // We executed all of the provided scripts, and were told to cache the // result. Do so now. - if (!TryAddKeyInScriptCache(hashCacheEntry, - ScriptExecutionMetrics(0))) { + if (!TryAddKeyInScriptCache(hashCacheEntry, metricsTotal)) { // This shouldn't happen, but it's not a disaster since it just // means we have to re-execute the scripts next time we see this // transaction (and some of its signatures ought to be cached, so it @@ -1903,9 +1938,12 @@ bool fCacheResults = fJustCheck; std::vector vChecks; + // metricsRet may be accurate (found in cache) or 0 (checks were + // deferred into vChecks). + ScriptExecutionMetrics metricsRet; if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, PrecomputedTransactionData(tx), - &vChecks)) { + metricsRet, &vChecks)) { return error("ConnectBlock(): CheckInputs on %s failed with %s", tx.GetId().ToString(), FormatStateMessage(state)); }