From a7e41326234d3a381fdde0924af74c6561b10798 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 18 Apr 2025 11:09:34 +0200 Subject: [PATCH 1/8] consensus: Use Coin span in GetP2SHSigOpCount Move the responsibility of retrieving coins from GetP2SHSigOpCount to its caller. This is a part of a series of commits for removing access to the CCoinsViewCache in consensus verification functions. The goal is to allow calling verification functions with pre-fetched, or a user-defined set of coins. Define two explicit template specializations for both a span of references to coins and a span of coins. This allows using it for both Coin entries referenced from the CCoinsViewCache, and from contiguous memory, like the vector in CBlockUndo. --- src/coins.cpp | 10 ++++++++++ src/coins.h | 9 +++++++++ src/consensus/tx_verify.cpp | 31 +++++++++++++++++++++++++------ src/consensus/tx_verify.h | 6 ++++-- src/test/fuzz/coins_view.cpp | 3 ++- src/test/script_p2sh_tests.cpp | 9 ++++++--- 6 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 24a102b0bc1..6ee253ad40d 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -160,6 +160,16 @@ const Coin& CCoinsViewCache::AccessCoin(const COutPoint &outpoint) const { } } +std::vector> CCoinsViewCache::AccessCoins(const CTransaction& tx) const +{ + std::vector> coins; + coins.reserve(tx.vin.size()); + for (const CTxIn& input : tx.vin) { + coins.emplace_back(AccessCoin(input.prevout)); + } + return coins; +} + bool CCoinsViewCache::HaveCoin(const COutPoint &outpoint) const { CCoinsMap::const_iterator it = FetchCoin(outpoint); return (it != cacheCoins.end() && !it->second.coin.IsSpent()); diff --git a/src/coins.h b/src/coins.h index 61fb4af6420..a1a2848f740 100644 --- a/src/coins.h +++ b/src/coins.h @@ -415,6 +415,15 @@ public: */ const Coin& AccessCoin(const COutPoint &output) const; + /** + * Return a vector of references to Coins in the cache, or coinEmpty if + * the Coin is not found. + * + * Generally, this should only be held for a short scope. The coins should + * generally not be held through any other calls to this cache. + */ + std::vector> AccessCoins(const CTransaction& tx) const; + /** * Add a coin. Set possible_overwrite to true if an unspent version may * already exist in the cache. diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 95466b759cb..90e99ff0ff4 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -14,6 +14,16 @@ #include #include +template +static constexpr const Coin& GetCoin(const T& item) +{ + if constexpr (std::is_same_v>) { + return item.get(); + } else { + return item; + } +} + bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) { if (tx.nLockTime == 0) @@ -123,23 +133,31 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx) return nSigOps; } -unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& inputs) +template +unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins) { if (tx.IsCoinBase()) return 0; unsigned int nSigOps = 0; - for (unsigned int i = 0; i < tx.vin.size(); i++) - { - const Coin& coin = inputs.AccessCoin(tx.vin[i].prevout); + Assert(coins.size() == tx.vin.size()); + auto input_it = tx.vin.begin(); + for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) { + const Coin& coin{GetCoin(*it)}; assert(!coin.IsSpent()); const CTxOut &prevout = coin.out; if (prevout.scriptPubKey.IsPayToScriptHash()) - nSigOps += prevout.scriptPubKey.GetSigOpCount(tx.vin[i].scriptSig); + nSigOps += prevout.scriptPubKey.GetSigOpCount(input_it->scriptSig); } return nSigOps; } +template unsigned int GetP2SHSigOpCount>( + const CTransaction& tx, const std::span); + +template unsigned int GetP2SHSigOpCount>>( + const CTransaction& tx, const std::span>); + int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, uint32_t flags) { int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR; @@ -148,7 +166,8 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i return nSigOps; if (flags & SCRIPT_VERIFY_P2SH) { - nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR; + auto coins{inputs.AccessCoins(tx)}; + nSigOps += GetP2SHSigOpCount(tx, std::span{coins}) * WITNESS_SCALE_FACTOR; } for (unsigned int i = 0; i < tx.vin.size(); i++) diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index d2cf792cf3f..c226394bcb8 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_CONSENSUS_TX_VERIFY_H #define BITCOIN_CONSENSUS_TX_VERIFY_H +#include #include #include @@ -39,11 +40,12 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx); /** * Count ECDSA signature operations in pay-to-script-hash inputs. * - * @param[in] mapInputs Map of previous transactions that have outputs we're spending + * @param[in] coins Sorted span of Coins containing previous transaction outputs we're spending * @return maximum number of sigops required to validate this transaction's inputs * @see CTransaction::FetchInputs */ -unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& mapInputs); +template +unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins); /** * Compute total signature operation cost of a transaction. diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 9c6aa6e7a1e..f36704f64a3 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -266,7 +266,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) // consensus/tx_verify.cpp:130: unsigned int GetP2SHSigOpCount(const CTransaction &, const CCoinsViewCache &): Assertion `!coin.IsSpent()' failed. return; } - (void)GetP2SHSigOpCount(transaction, coins_view_cache); + auto coins{coins_view_cache.AccessCoins(transaction)}; + (void)GetP2SHSigOpCount(transaction, std::span{coins}); }, [&] { const CTransaction transaction{random_mutable_transaction}; diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp index bb408b7b0f5..e831bad64f9 100644 --- a/src/test/script_p2sh_tests.cpp +++ b/src/test/script_p2sh_tests.cpp @@ -362,7 +362,8 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard) BOOST_CHECK(::AreInputsStandard(CTransaction(txTo), coins)); // 22 P2SH sigops for all inputs (1 for vin[0], 6 for vin[3], 15 for vin[4] - BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txTo), coins), 22U); + auto coinsTxTo{coins.AccessCoins(CTransaction(txTo))}; + BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txTo), std::span{coinsTxTo}), 22U); CMutableTransaction txToNonStd1; txToNonStd1.vout.resize(1); @@ -374,7 +375,8 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard) txToNonStd1.vin[0].scriptSig << std::vector(sixteenSigops.begin(), sixteenSigops.end()); BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd1), coins)); - BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd1), coins), 16U); + auto coinsTxToNonStd1{coins.AccessCoins(CTransaction(txToNonStd1))}; + BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd1), std::span{coinsTxToNonStd1}), 16U); CMutableTransaction txToNonStd2; txToNonStd2.vout.resize(1); @@ -386,7 +388,8 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard) txToNonStd2.vin[0].scriptSig << std::vector(twentySigops.begin(), twentySigops.end()); BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd2), coins)); - BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U); + auto coinsTxToNonStd2{coins.AccessCoins(CTransaction(txToNonStd2))}; + BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), std::span{coinsTxToNonStd2}), 20U); } BOOST_AUTO_TEST_SUITE_END() From 781668b6e8078ad76aa303f89f169bc9ed040e65 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 18 Apr 2025 12:51:31 +0200 Subject: [PATCH 2/8] consensus: Use Coin span in GetTransactionSigOpCost Move the responsibility of retrieving coins from GetTransactionSigOpCost to its caller. This is a part of a series of commits for removing access to the CCoinsViewCache in consensus verification functions. The goal is to allow calling verification functions with pre-fetched, or a user-defined set of coins. Define two explicit template specializations for both a span of references to coins and a span of coins. This allows using it for both Coin entries referenced from the CCoinsViewCache, and from contiguous memory, like the vector in CBlockUndo. --- src/consensus/tx_verify.cpp | 20 +++++++++++++------- src/consensus/tx_verify.h | 7 ++++--- src/node/psbt.cpp | 3 ++- src/test/fuzz/coins_view.cpp | 3 ++- src/test/sigopcount_tests.cpp | 31 ++++++++++++++++++++----------- src/validation.cpp | 14 +++++++++----- 6 files changed, 50 insertions(+), 28 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 90e99ff0ff4..3919a11b8b7 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -158,7 +158,8 @@ template unsigned int GetP2SHSigOpCount>( template unsigned int GetP2SHSigOpCount>>( const CTransaction& tx, const std::span>); -int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, uint32_t flags) +template +int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags) { int64_t nSigOps = GetLegacySigOpCount(tx) * WITNESS_SCALE_FACTOR; @@ -166,19 +167,24 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i return nSigOps; if (flags & SCRIPT_VERIFY_P2SH) { - auto coins{inputs.AccessCoins(tx)}; - nSigOps += GetP2SHSigOpCount(tx, std::span{coins}) * WITNESS_SCALE_FACTOR; + nSigOps += GetP2SHSigOpCount(tx, coins) * WITNESS_SCALE_FACTOR; } - for (unsigned int i = 0; i < tx.vin.size(); i++) - { - const Coin& coin = inputs.AccessCoin(tx.vin[i].prevout); + Assert(coins.size() == tx.vin.size()); + auto input_it = tx.vin.begin(); + for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) { + const Coin& coin{GetCoin(*it)}; assert(!coin.IsSpent()); const CTxOut &prevout = coin.out; - nSigOps += CountWitnessSigOps(tx.vin[i].scriptSig, prevout.scriptPubKey, &tx.vin[i].scriptWitness, flags); + nSigOps += CountWitnessSigOps(input_it->scriptSig, prevout.scriptPubKey, &input_it->scriptWitness, flags); } return nSigOps; } +template int64_t GetTransactionSigOpCost>( + const CTransaction& tx, std::span coins, uint32_t flags); + +template int64_t GetTransactionSigOpCost>>( + const CTransaction& tx, const std::span> coins, uint32_t flags); bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee) { diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index c226394bcb8..5c9dae5a425 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -49,12 +49,13 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const T coins); /** * Compute total signature operation cost of a transaction. - * @param[in] tx Transaction for which we are computing the cost - * @param[in] inputs Map of previous transactions that have outputs we're spending + * @param[in] tx Transaction for which we are computing the cost + * @param[in] coins Sorted span of Coins containing previous transaction outputs we're spending * @param[in] flags Script verification flags * @return Total signature operation cost of tx */ -int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& inputs, uint32_t flags); +template +int64_t GetTransactionSigOpCost(const CTransaction& tx, const T coins, uint32_t flags); /** * Check if transaction is final and can be included in a block with the diff --git a/src/node/psbt.cpp b/src/node/psbt.cpp index 51e252bffca..3b203594e30 100644 --- a/src/node/psbt.cpp +++ b/src/node/psbt.cpp @@ -137,7 +137,8 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx) if (success) { CTransaction ctx = CTransaction(mtx); - size_t size(GetVirtualTransactionSize(ctx, GetTransactionSigOpCost(ctx, view, STANDARD_SCRIPT_VERIFY_FLAGS), ::nBytesPerSigOp)); + auto coins{view.AccessCoins(ctx)}; + size_t size(GetVirtualTransactionSize(ctx, GetTransactionSigOpCost(ctx, std::span{coins}, STANDARD_SCRIPT_VERIFY_FLAGS), ::nBytesPerSigOp)); result.estimated_vsize = size; // Estimate fee rate CFeeRate feerate(fee, size); diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index f36704f64a3..ef5fbddba5d 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -282,7 +282,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) // script/interpreter.cpp:1705: size_t CountWitnessSigOps(const CScript &, const CScript &, const CScriptWitness *, unsigned int): Assertion `(flags & SCRIPT_VERIFY_P2SH) != 0' failed. return; } - (void)GetTransactionSigOpCost(transaction, coins_view_cache, flags); + auto coins{coins_view_cache.AccessCoins(transaction)}; + (void)GetTransactionSigOpCost(transaction, std::span{coins}, flags); }, [&] { (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache); diff --git a/src/test/sigopcount_tests.cpp b/src/test/sigopcount_tests.cpp index aed67d5f3cf..886b78127e1 100644 --- a/src/test/sigopcount_tests.cpp +++ b/src/test/sigopcount_tests.cpp @@ -131,13 +131,15 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) CScript scriptSig = CScript() << OP_0 << OP_0; BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, CScriptWitness()); + auto spending_coins{coins.AccessCoins(CTransaction(spendingTx))}; // Legacy counting only includes signature operations in scriptSigs and scriptPubKeys // of a transaction and does not take the actual executed sig operations into account. // spendingTx in itself does not contain a signature operation. - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 0); + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 0); // creationTx contains two signature operations in its scriptPubKey, but legacy counting // is not accurate. - assert(GetTransactionSigOpCost(CTransaction(creationTx), coins, flags) == MAX_PUBKEYS_PER_MULTISIG * WITNESS_SCALE_FACTOR); + auto creation_coins{coins.AccessCoins(CTransaction(creationTx))}; + assert(GetTransactionSigOpCost(CTransaction(creationTx), std::span{creation_coins}, flags) == MAX_PUBKEYS_PER_MULTISIG * WITNESS_SCALE_FACTOR); // Sanity check: script verification fails because of an invalid signature. assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY); } @@ -149,7 +151,8 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) CScript scriptSig = CScript() << OP_0 << OP_0 << ToByteVector(redeemScript); BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, CScriptWitness()); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 2 * WITNESS_SCALE_FACTOR); + auto spending_coins{coins.AccessCoins(CTransaction(spendingTx))}; + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 2 * WITNESS_SCALE_FACTOR); assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY); } @@ -163,22 +166,25 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 1); + auto spending_coins{coins.AccessCoins(CTransaction(spendingTx))}; + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 1); // No signature operations if we don't verify the witness. - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags & ~SCRIPT_VERIFY_WITNESS) == 0); + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags & ~SCRIPT_VERIFY_WITNESS) == 0); assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_EQUALVERIFY); // The sig op cost for witness version != 0 is zero. assert(scriptPubKey[0] == 0x00); scriptPubKey[0] = 0x51; BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 0); + spending_coins = coins.AccessCoins(CTransaction(spendingTx)); + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 0); scriptPubKey[0] = 0x00; BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness); // The witness of a coinbase transaction is not taken into account. spendingTx.vin[0].prevout.SetNull(); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 0); + spending_coins = coins.AccessCoins(CTransaction(spendingTx)); + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 0); } // P2WPKH nested in P2SH @@ -191,7 +197,8 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) scriptWitness.stack.emplace_back(0); BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 1); + auto spending_coins{coins.AccessCoins(CTransaction(spendingTx))}; + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 1); assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_EQUALVERIFY); } @@ -206,8 +213,9 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) scriptWitness.stack.emplace_back(witnessScript.begin(), witnessScript.end()); BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 2); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags & ~SCRIPT_VERIFY_WITNESS) == 0); + auto spending_coins{coins.AccessCoins(CTransaction(spendingTx))}; + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 2); + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags & ~SCRIPT_VERIFY_WITNESS) == 0); assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY); } @@ -223,7 +231,8 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost) scriptWitness.stack.emplace_back(witnessScript.begin(), witnessScript.end()); BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness); - assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 2); + auto spending_coins{coins.AccessCoins(CTransaction(spendingTx))}; + assert(GetTransactionSigOpCost(CTransaction(spendingTx), std::span{spending_coins}, flags) == 2); assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY); } } diff --git a/src/validation.cpp b/src/validation.cpp index aa1effb736f..1e6e9d90363 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -887,7 +887,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard"); } - int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS); + auto coins{m_view.AccessCoins(tx)}; + int64_t nSigOpsCost = GetTransactionSigOpCost(tx, std::span{coins}, STANDARD_SCRIPT_VERIFY_FLAGS); // Keep track of transactions that spend a coinbase, which we re-scan // during reorgs to ensure COINBASE_MATURITY is still met. @@ -2687,10 +2688,13 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // * legacy (always) // * p2sh (when P2SH enabled in flags and excludes coinbase) // * witness (when witness enabled in flags and excludes coinbase) - nSigOpsCost += GetTransactionSigOpCost(tx, view, flags); - if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) { - state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops"); - break; + { + auto coins{view.AccessCoins(tx)}; + nSigOpsCost += GetTransactionSigOpCost(tx, std::span{coins}, flags); + if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) { + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-blk-sigops", "too many sigops"); + break; + } } if (!tx.IsCoinBase()) From c1285a81dedf754d3d6d2047684bb6940e22efde Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 18 Apr 2025 14:59:25 +0200 Subject: [PATCH 3/8] consensus: Use Coin span in CheckTxInputs Move the responsibility of retrieving coins from CheckTxInputs to its caller. The missing inputs check will be moved in an upcoming commit. This is a part of a series of commits for removing access to the CCoinsViewCache in consensus verification functions. The goal is to allow calling verification functions with pre-fetched, or a user-defined set of coins. Define two explicit template specializations for both a span of references to coins and a span of coins. This allows using it for both Coin entries referenced from the CCoinsViewCache, and from contiguous memory, like the vector in CBlockUndo. --- src/consensus/tx_verify.cpp | 18 +++++++++++++----- src/consensus/tx_verify.h | 3 ++- src/test/fuzz/coins_view.cpp | 5 ++++- src/txmempool.cpp | 3 ++- src/validation.cpp | 9 +++++---- 5 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 3919a11b8b7..fbc7479683a 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -186,7 +186,8 @@ template int64_t GetTransactionSigOpCost>( template int64_t GetTransactionSigOpCost>>( const CTransaction& tx, const std::span> coins, uint32_t flags); -bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee) +template +bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const T coins, int nSpendHeight, CAmount& txfee) { // are the actual inputs available? if (!inputs.HaveInputs(tx)) { @@ -195,15 +196,16 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, } CAmount nValueIn = 0; - for (unsigned int i = 0; i < tx.vin.size(); ++i) { - const COutPoint &prevout = tx.vin[i].prevout; - const Coin& coin = inputs.AccessCoin(prevout); + Assert(coins.size() == tx.vin.size()); + auto input_it = tx.vin.begin(); + for (auto it = coins.begin(); it != coins.end(); ++it, ++input_it) { + const Coin& coin{GetCoin(*it)}; assert(!coin.IsSpent()); // If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "bad-txns-premature-spend-of-coinbase", - strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); + strprintf("tried to spend coinbase at depth %d", static_cast(nSpendHeight - coin.nHeight))); } // Check for negative or overflow input values @@ -228,3 +230,9 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, txfee = txfee_aux; return true; } + +template bool Consensus::CheckTxInputs>( + const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const std::span coins, int nSpendHeight, CAmount& txfee); + +template bool Consensus::CheckTxInputs>>( + const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const std::span> coins, int nSpendHeight, CAmount& txfee); diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index 5c9dae5a425..0333045aefa 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -25,7 +25,8 @@ namespace Consensus { * @param[out] txfee Set to the transaction fee if successful. * Preconditions: tx.IsCoinBase() is false. */ -[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee); +template +[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, const T coins, int nSpendHeight, CAmount& txfee); } // namespace Consensus /** Auxiliary functions for transaction validation (ideally should not be exposed) */ diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index ef5fbddba5d..e5e5fb45369 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -255,7 +255,10 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) // It is not allowed to call CheckTxInputs if CheckTransaction failed return; } - if (Consensus::CheckTxInputs(transaction, state, coins_view_cache, fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()), tx_fee_out)) { + // are the actual inputs available? + if (!coins_view_cache.HaveInputs(transaction)) return; + auto coins{coins_view_cache.AccessCoins(transaction)}; + if (Consensus::CheckTxInputs(transaction, state, coins_view_cache, std::span{coins}, fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()), tx_fee_out)) { assert(MoneyRange(tx_fee_out)); } }, diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 3a5a3fb306d..f100621fd19 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -777,7 +777,8 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass CAmount txfee = 0; assert(!tx.IsCoinBase()); - assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, txfee)); + auto coins{mempoolDuplicate.AccessCoins(tx)}; + assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, std::span{coins}, spendheight, txfee)); for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout); AddCoins(mempoolDuplicate, tx, std::numeric_limits::max()); } diff --git a/src/validation.cpp b/src/validation.cpp index 1e6e9d90363..532c445f042 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -874,7 +874,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // The mempool holds txs for the next block, so pass height+1 to CheckTxInputs - if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) { + auto coins{m_view.AccessCoins(tx)}; + if (!Consensus::CheckTxInputs(tx, state, m_view, std::span{coins}, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) { return false; // state filled in by CheckTxInputs } @@ -887,7 +888,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard"); } - auto coins{m_view.AccessCoins(tx)}; int64_t nSigOpsCost = GetTransactionSigOpCost(tx, std::span{coins}, STANDARD_SCRIPT_VERIFY_FLAGS); // Keep track of transactions that spend a coinbase, which we re-scan @@ -2648,14 +2648,15 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, { if (!state.IsValid()) break; const CTransaction &tx = *(block.vtx[i]); - nInputs += tx.vin.size(); if (!tx.IsCoinBase()) { CAmount txfee = 0; TxValidationState tx_state; - if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { + + auto coins{view.AccessCoins(tx)}; + if (!Consensus::CheckTxInputs(tx, tx_state, view, std::span{coins}, pindex->nHeight, txfee)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), From 1eb575214dbcf7ad034565227d8a5eadf064b775 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 18 Apr 2025 22:24:03 +0200 Subject: [PATCH 4/8] validation: Use vector of outputs instead of CCoinsViewCache in CheckInputScripts Move the responsibility of retrieving coins from the CCoinsViewCache in CheckInputScripts to its caller. Add a helper method in CCoinsViewCache to collect all Coin's outputs spent by a transaction's inputs. Callers of CCoinsViewCache are updated to either pre-fetch the spent outputs, or pass in an empty vector if the precomputed transaction data has already been initialized with the required outputs. This is a part of a series of commits for removing access to the CCoinsViewCache in consensus verification functions. The goal is to allow calling verification functions with pre-fetched, or a user-defined set of coins. --- src/coins.cpp | 13 +++++++++++ src/coins.h | 7 ++++++ src/test/txvalidationcache_tests.cpp | 35 ++++++++++++++++++++-------- src/validation.cpp | 32 ++++++++++++------------- 4 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 6ee253ad40d..6f7982c258c 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -170,6 +170,19 @@ std::vector> CCoinsViewCache::AccessCoins(con return coins; } +std::vector CCoinsViewCache::GetUnspentOutputs(const CTransaction& tx) const +{ + std::vector spent_outputs; + spent_outputs.reserve(tx.vin.size()); + for (const auto& txin : tx.vin) { + const COutPoint& prevout = txin.prevout; + const Coin& coin = AccessCoin(prevout); + assert(!coin.IsSpent()); + spent_outputs.emplace_back(coin.out); + } + return spent_outputs; +} + bool CCoinsViewCache::HaveCoin(const COutPoint &outpoint) const { CCoinsMap::const_iterator it = FetchCoin(outpoint); return (it != cacheCoins.end() && !it->second.coin.IsSpent()); diff --git a/src/coins.h b/src/coins.h index a1a2848f740..907ab4cdf2d 100644 --- a/src/coins.h +++ b/src/coins.h @@ -424,6 +424,13 @@ public: */ std::vector> AccessCoins(const CTransaction& tx) const; + /** + * Return a vector of unspent outputs of coins in the cache that are spent + * by the provided transaction. The coins they belong to must be unspent in + * the cache. + */ + std::vector GetUnspentOutputs(const CTransaction& tx) const; + /** * Add a coin. Set possible_overwrite to true if an unspent version may * already exist in the cache. diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index af36a956931..6268b0177b4 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -21,7 +21,7 @@ struct Dersig100Setup : public TestChain100Setup { }; bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + std::vector&& spent_outputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, std::vector* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -124,6 +124,9 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail { PrecomputedTransactionData txdata; + std::vector spent_outputs{active_coins_tip.GetUnspentOutputs(tx)}; + txdata.Init(tx, std::move(spent_outputs)); + FastRandomContext insecure_rand(true); for (int count = 0; count < 10000; ++count) { @@ -142,7 +145,7 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail // WITNESS requires P2SH test_flags |= SCRIPT_VERIFY_P2SH; } - bool ret = CheckInputScripts(tx, state, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, nullptr); + bool ret = CheckInputScripts(tx, state, {}, test_flags, true, add_to_cache, txdata, validation_cache, nullptr); // CheckInputScripts should succeed iff test_flags doesn't intersect with // failing_flags bool expected_return_value = !(test_flags & failing_flags); @@ -152,13 +155,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(CheckInputScripts(tx, state, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(tx, state, {}, test_flags, true, add_to_cache, txdata, validation_cache, &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(CheckInputScripts(tx, state, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(tx, state, {}, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks)); BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size()); } } @@ -216,13 +219,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) TxValidationState state; PrecomputedTransactionData ptd_spend_tx; - BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, nullptr)); + std::vector spent_outputs{m_node.chainman->ActiveChainstate().CoinsTip().GetUnspentOutputs(CTransaction(spend_tx))}; + ptd_spend_tx.Init(spend_tx, std::move(spent_outputs)); + + BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, 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(CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, &scriptchecks)); BOOST_CHECK_EQUAL(scriptchecks.size(), 1U); // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are @@ -284,7 +290,10 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; TxValidationState state; PrecomputedTransactionData txdata; - BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)); + std::vector spent_outputs{m_node.chainman->ActiveChainstate().CoinsTip().GetUnspentOutputs(CTransaction(invalid_with_cltv_tx))}; + txdata.Init(invalid_with_cltv_tx, std::move(spent_outputs)); + + BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, {}, SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)); } // TEST CHECKSEQUENCEVERIFY @@ -312,7 +321,10 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; TxValidationState state; PrecomputedTransactionData txdata; - BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)); + std::vector spent_outputs{m_node.chainman->ActiveChainstate().CoinsTip().GetUnspentOutputs(CTransaction(invalid_with_csv_tx))}; + txdata.Init(invalid_with_csv_tx, std::move(spent_outputs)); + + BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, {}, SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)); } // TODO: add tests for remaining script flags @@ -373,13 +385,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) TxValidationState state; PrecomputedTransactionData txdata; + std::vector spent_outputs{m_node.chainman->ActiveChainstate().CoinsTip().GetUnspentOutputs(CTransaction(tx))}; + txdata.Init(tx, std::move(spent_outputs)); + // This transaction is now invalid under segwit, because of the second input. - BOOST_CHECK(!CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)); + BOOST_CHECK(!CheckInputScripts(CTransaction(tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)); std::vector scriptchecks; // Make sure this transaction was not cached (ie because the first // input was valid) - BOOST_CHECK(CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(CTransaction(tx), state, {}, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, &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 532c445f042..f35acb7ee21 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -137,11 +137,11 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato } bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + std::vector&& spent_outputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, std::vector* pvChecks = nullptr) - EXCLUSIVE_LOCKS_REQUIRED(cs_main); + EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction& tx) { @@ -428,8 +428,10 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS } } + std::vector spent_outputs{view.GetUnspentOutputs(tx)}; + // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules. - return CheckInputScripts(tx, state, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache); + return CheckInputScripts(tx, state, std::move(spent_outputs), flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache); } namespace { @@ -1234,15 +1236,17 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS; + std::vector spent_outputs{m_view.GetUnspentOutputs(tx)}; + // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache())) { + if (!CheckInputScripts(tx, state, std::move(spent_outputs), scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache())) { // 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. TxValidationState state_dummy; // Want reported failures to be from first CheckInputScripts - if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata, GetValidationCache()) && - !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata, GetValidationCache())) { + if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, {}, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata, GetValidationCache()) && + !CheckInputScripts(tx, state_dummy, {}, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata, GetValidationCache())) { // Only the witness is missing, so the transaction itself may be fine. state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, state.GetRejectReason(), state.GetDebugMessage()); @@ -2162,7 +2166,7 @@ ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, cons * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp */ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + std::vector&& spent_outputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, std::vector* pvChecks) @@ -2187,15 +2191,6 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, } if (!txdata.m_spent_outputs_ready) { - std::vector spent_outputs; - spent_outputs.reserve(tx.vin.size()); - - for (const auto& txin : tx.vin) { - const COutPoint& prevout = txin.prevout; - const Coin& coin = inputs.AccessCoin(prevout); - assert(!coin.IsSpent()); - spent_outputs.emplace_back(coin.out); - } txdata.Init(tx, std::move(spent_outputs)); } assert(txdata.m_spent_outputs.size() == tx.vin.size()); @@ -2703,7 +2698,10 @@ bool Chainstate::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 && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, parallel_script_checks ? &vChecks : nullptr)) { + + std::vector spent_outputs{view.GetUnspentOutputs(tx)}; + + if (fScriptChecks && !CheckInputScripts(tx, tx_state, std::move(spent_outputs), flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, 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()); From 008748c3ae5d60dc5c3e1e0212481e2f03a84545 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Sat, 19 Apr 2025 22:55:59 +0200 Subject: [PATCH 5/8] validation: Add SpendBlock function Move the BIP30 checks from ConnectBlock to a new SpendBlock method. This is a move-only change, more content to SpendBlock is added in the next commits. The goal is to move logic requiring access to the CCoinsViewCache out of ConnectBlock and to the new SpendBlock method. SpendBlock will in future handle all UTXO set interactions that previously took place in ConnectBlock. By returning early in case of a BIP30 validation failure, an additional failure check for an invalid block reward in case of its failure is left out. Callers of ConnectBlock now also need to call SpendBlock before. This is enforced in a future commit by adding a CBlockUndo argument that needs to be passed to both. This is a part of a series of commits for removing access to the CCoinsViewCache in consensus verification functions. The goal is to allow calling verification functions with pre-fetched, or a user-defined set of coins. --- src/bench/connectblock.cpp | 2 + src/test/coinstatsindex_tests.cpp | 2 + src/validation.cpp | 231 ++++++++++++++++++------------ src/validation.h | 3 + 4 files changed, 148 insertions(+), 90 deletions(-) diff --git a/src/bench/connectblock.cpp b/src/bench/connectblock.cpp index 3746ea29374..e51333d0d27 100644 --- a/src/bench/connectblock.cpp +++ b/src/bench/connectblock.cpp @@ -100,6 +100,8 @@ void BenchmarkConnectBlock(benchmark::Bench& bench, std::vector& keys, std auto* pindex{chainman->m_blockman.AddToBlockIndex(test_block, chainman->m_best_header)}; // Doing this here doesn't impact the benchmark CCoinsViewCache viewNew{&chainstate.CoinsTip()}; + const auto block_hash{test_block.GetHash()}; + assert(chainstate.SpendBlock(test_block, pindex, block_hash, viewNew, test_block_state)); assert(chainstate.ConnectBlock(test_block, test_block_state, pindex, viewNew)); }); } diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index e09aad05e94..9ed1939c1c0 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -100,6 +100,8 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_unclean_shutdown, TestChain100Setup) BOOST_CHECK(CheckBlock(block, state, params.GetConsensus())); BOOST_CHECK(m_node.chainman->AcceptBlock(new_block, state, &new_block_index, true, nullptr, nullptr, true)); CCoinsViewCache view(&chainstate.CoinsTip()); + const auto block_hash{block.GetHash()}; + BOOST_CHECK(chainstate.SpendBlock(block, new_block_index, block_hash, view, state)); BOOST_CHECK(chainstate.ConnectBlock(block, state, new_block_index, view)); } // Send block connected notification, then stop the index without diff --git a/src/validation.cpp b/src/validation.cpp index f35acb7ee21..ea638703b89 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2470,10 +2470,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, return false; } - // verify that the view's current state corresponds to the previous block - uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash(); - assert(hashPrevBlock == view.GetBestBlock()); - m_chainman.num_blocks_total++; // Special case for the genesis block, skipping connection of its transactions @@ -2522,92 +2518,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, Ticks(m_chainman.time_check), Ticks(m_chainman.time_check) / m_chainman.num_blocks_total); - // Do not allow blocks that contain transactions which 'overwrite' older transactions, - // unless those are already completely spent. - // If such overwrites are allowed, coinbases and transactions depending upon those - // can be duplicated to remove the ability to spend the first instance -- even after - // being sent to another address. - // See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information. - // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC. - // Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the - // two in the chain that violate it. This prevents exploiting the issue against nodes during their - // initial block download. - bool fEnforceBIP30 = !IsBIP30Repeat(*pindex); - - // Once BIP34 activated it was not possible to create new duplicate coinbases and thus other than starting - // with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs. But by the - // time BIP34 activated, in each of the existing pairs the duplicate coinbase had overwritten the first - // before the first had been spent. Since those coinbases are sufficiently buried it's no longer possible to create further - // duplicate transactions descending from the known pairs either. - // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check. - - // BIP34 requires that a block at height X (block X) has its coinbase - // scriptSig start with a CScriptNum of X (indicated height X). The above - // logic of no longer requiring BIP30 once BIP34 activates is flawed in the - // case that there is a block X before the BIP34 height of 227,931 which has - // an indicated height Y where Y is greater than X. The coinbase for block - // X would also be a valid coinbase for block Y, which could be a BIP30 - // violation. An exhaustive search of all mainnet coinbases before the - // BIP34 height which have an indicated height greater than the block height - // reveals many occurrences. The 3 lowest indicated heights found are - // 209,921, 490,897, and 1,983,702 and thus coinbases for blocks at these 3 - // heights would be the first opportunity for BIP30 to be violated. - - // The search reveals a great many blocks which have an indicated height - // greater than 1,983,702, so we simply remove the optimization to skip - // BIP30 checking for blocks at height 1,983,702 or higher. Before we reach - // that block in another 25 years or so, we should take advantage of a - // future consensus change to do a new and improved version of BIP34 that - // will actually prevent ever creating any duplicate coinbases in the - // future. - static constexpr int BIP34_IMPLIES_BIP30_LIMIT = 1983702; - - // There is no potential to create a duplicate coinbase at block 209,921 - // because this is still before the BIP34 height and so explicit BIP30 - // checking is still active. - - // The final case is block 176,684 which has an indicated height of - // 490,897. Unfortunately, this issue was not discovered until about 2 weeks - // before block 490,897 so there was not much opportunity to address this - // case other than to carefully analyze it and determine it would not be a - // problem. Block 490,897 was, in fact, mined with a different coinbase than - // block 176,684, but it is important to note that even if it hadn't been or - // is remined on an alternate fork with a duplicate coinbase, we would still - // not run into a BIP30 violation. This is because the coinbase for 176,684 - // is spent in block 185,956 in transaction - // d4f7fbbf92f4a3014a230b2dc70b8058d02eb36ac06b4a0736d9d60eaa9e8781. This - // spending transaction can't be duplicated because it also spends coinbase - // 0328dd85c331237f18e781d692c92de57649529bd5edf1d01036daea32ffde29. This - // coinbase has an indicated height of over 4.2 billion, and wouldn't be - // duplicatable until that height, and it's currently impossible to create a - // chain that long. Nevertheless we may wish to consider a future soft fork - // which retroactively prevents block 490,897 from creating a duplicate - // coinbase. The two historical BIP30 violations often provide a confusing - // edge case when manipulating the UTXO and it would be simpler not to have - // another edge case to deal with. - - // testnet3 has no blocks before the BIP34 height with indicated heights - // post BIP34 before approximately height 486,000,000. After block - // 1,983,702 testnet3 starts doing unnecessary BIP30 checking again. - assert(pindex->pprev); - CBlockIndex* pindexBIP34height = pindex->pprev->GetAncestor(params.GetConsensus().BIP34Height); - //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond. - fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == params.GetConsensus().BIP34Hash)); - - // TODO: Remove BIP30 checking from block height 1,983,702 on, once we have a - // consensus change that ensures coinbases at those heights cannot - // duplicate earlier coinbases. - if (fEnforceBIP30 || pindex->nHeight >= BIP34_IMPLIES_BIP30_LIMIT) { - for (const auto& tx : block.vtx) { - for (size_t o = 0; o < tx->vout.size(); o++) { - if (view.HaveCoin(COutPoint(tx->GetHash(), o))) { - state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30", - "tried to overwrite transaction"); - } - } - } - } - // Enforce BIP68 (sequence locks) int nLockTimeFlags = 0; if (DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_CSV)) { @@ -3009,6 +2919,128 @@ static void UpdateTipLog( !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages) : ""); } +bool Chainstate::SpendBlock( + const CBlock& block, + const CBlockIndex* pindex, + const uint256& block_hash, + CCoinsViewCache& view, + BlockValidationState& state) +{ + AssertLockHeld(cs_main); + assert(pindex); + Assume(block.GetHash() == block_hash); + assert(*pindex->phashBlock == block_hash); + + const CChainParams& params{m_chainman.GetParams()}; + // Special case for the genesis block, skipping connection of its transactions + // (its coinbase is unspendable) + if (block_hash == params.GetConsensus().hashGenesisBlock) { + return true; + } + + // verify that the view's current state corresponds to the previous block + uint256 hashPrevBlock = pindex->pprev == nullptr ? uint256() : pindex->pprev->GetBlockHash(); + assert(hashPrevBlock == view.GetBestBlock()); + + const auto time_start{SteadyClock::now()}; + + // Do not allow blocks that contain transactions which 'overwrite' older transactions, + // unless those are already completely spent. + // If such overwrites are allowed, coinbases and transactions depending upon those + // can be duplicated to remove the ability to spend the first instance -- even after + // being sent to another address. + // See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information. + // This rule was originally applied to all blocks with a timestamp after March 15, 2012, 0:00 UTC. + // Now that the whole chain is irreversibly beyond that time it is applied to all blocks except the + // two in the chain that violate it. This prevents exploiting the issue against nodes during their + // initial block download. + bool fEnforceBIP30 = !IsBIP30Repeat(*pindex); + + // Once BIP34 activated it was not possible to create new duplicate coinbases and thus other than starting + // with the 2 existing duplicate coinbase pairs, not possible to create overwriting txs. But by the + // time BIP34 activated, in each of the existing pairs the duplicate coinbase had overwritten the first + // before the first had been spent. Since those coinbases are sufficiently buried it's no longer possible to create further + // duplicate transactions descending from the known pairs either. + // If we're on the known chain at height greater than where BIP34 activated, we can save the db accesses needed for the BIP30 check. + + // BIP34 requires that a block at height X (block X) has its coinbase + // scriptSig start with a CScriptNum of X (indicated height X). The above + // logic of no longer requiring BIP30 once BIP34 activates is flawed in the + // case that there is a block X before the BIP34 height of 227,931 which has + // an indicated height Y where Y is greater than X. The coinbase for block + // X would also be a valid coinbase for block Y, which could be a BIP30 + // violation. An exhaustive search of all mainnet coinbases before the + // BIP34 height which have an indicated height greater than the block height + // reveals many occurrences. The 3 lowest indicated heights found are + // 209,921, 490,897, and 1,983,702 and thus coinbases for blocks at these 3 + // heights would be the first opportunity for BIP30 to be violated. + + // The search reveals a great many blocks which have an indicated height + // greater than 1,983,702, so we simply remove the optimization to skip + // BIP30 checking for blocks at height 1,983,702 or higher. Before we reach + // that block in another 25 years or so, we should take advantage of a + // future consensus change to do a new and improved version of BIP34 that + // will actually prevent ever creating any duplicate coinbases in the + // future. + static constexpr int BIP34_IMPLIES_BIP30_LIMIT = 1983702; + + // There is no potential to create a duplicate coinbase at block 209,921 + // because this is still before the BIP34 height and so explicit BIP30 + // checking is still active. + + // The final case is block 176,684 which has an indicated height of + // 490,897. Unfortunately, this issue was not discovered until about 2 weeks + // before block 490,897 so there was not much opportunity to address this + // case other than to carefully analyze it and determine it would not be a + // problem. Block 490,897 was, in fact, mined with a different coinbase than + // block 176,684, but it is important to note that even if it hadn't been or + // is remined on an alternate fork with a duplicate coinbase, we would still + // not run into a BIP30 violation. This is because the coinbase for 176,684 + // is spent in block 185,956 in transaction + // d4f7fbbf92f4a3014a230b2dc70b8058d02eb36ac06b4a0736d9d60eaa9e8781. This + // spending transaction can't be duplicated because it also spends coinbase + // 0328dd85c331237f18e781d692c92de57649529bd5edf1d01036daea32ffde29. This + // coinbase has an indicated height of over 4.2 billion, and wouldn't be + // duplicatable until that height, and it's currently impossible to create a + // chain that long. Nevertheless we may wish to consider a future soft fork + // which retroactively prevents block 490,897 from creating a duplicate + // coinbase. The two historical BIP30 violations often provide a confusing + // edge case when manipulating the UTXO and it would be simpler not to have + // another edge case to deal with. + + // testnet3 has no blocks before the BIP34 height with indicated heights + // post BIP34 before approximately height 486,000,000. After block + // 1,983,702 testnet3 starts doing unnecessary BIP30 checking again. + assert(pindex->pprev); + CBlockIndex* pindexBIP34height = pindex->pprev->GetAncestor(params.GetConsensus().BIP34Height); + //Only continue to enforce if we're below BIP34 activation height or the block hash at that height doesn't correspond. + fEnforceBIP30 = fEnforceBIP30 && (!pindexBIP34height || !(pindexBIP34height->GetBlockHash() == params.GetConsensus().BIP34Hash)); + + // TODO: Remove BIP30 checking from block height 1,983,702 on, once we have a + // consensus change that ensures coinbases at those heights cannot + // duplicate earlier coinbases. + if (fEnforceBIP30 || pindex->nHeight >= BIP34_IMPLIES_BIP30_LIMIT) { + for (const auto& tx : block.vtx) { + for (size_t o = 0; o < tx->vout.size(); o++) { + if (view.HaveCoin(COutPoint(tx->GetHash(), o))) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-BIP30", + "tried to overwrite transaction"); + } + } + } + } + + const auto time_1{SteadyClock::now()}; + m_chainman.time_check += time_1 - time_start; + LogDebug(BCLog::BENCH, " - Spend Block processed %u transactions: %.2fms (%.3fms/tx) [%.2fs (%.2fms/blk)]\n", (unsigned)block.vtx.size(), + Ticks(time_1 - time_start), + block.vtx.size() == 0 ? 0 : Ticks(time_1 - time_start) / block.vtx.size(), + Ticks(m_chainman.time_connect), + m_chainman.num_blocks_total == 0 ? 0 : Ticks(m_chainman.time_connect) / m_chainman.num_blocks_total); + + return true; +} + void Chainstate::UpdateTip(const CBlockIndex* pindexNew) { AssertLockHeld(::cs_main); @@ -3200,6 +3232,17 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, Ticks(time_2 - time_1)); { CCoinsViewCache view(&CoinsTip()); + + const auto block_hash{blockConnecting.GetHash()}; + if (!SpendBlock(blockConnecting, pindexNew, block_hash, view, state)) { + assert(state.IsInvalid()); + if (m_chainman.m_options.signals) { + m_chainman.m_options.signals->BlockChecked(blockConnecting, state); + } + InvalidBlockFound(pindexNew, state); + LogError("%s: SpendBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString()); + return false; + } bool rv = ConnectBlock(blockConnecting, state, pindexNew, view); if (m_chainman.m_options.signals) { m_chainman.m_options.signals->BlockChecked(blockConnecting, state); @@ -4678,6 +4721,9 @@ bool TestBlockValidity(BlockValidationState& state, LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString()); return false; } + if (!chainstate.SpendBlock(block, &indexDummy, block_hash, viewNew, state)) { + return false; + } if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) { return false; } @@ -4862,6 +4908,11 @@ VerifyDBResult CVerifyDB::VerifyDB( LogPrintf("Verification error: ReadBlock failed at %d, hash=%s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); return VerifyDBResult::CORRUPTED_BLOCK_DB; } + const auto block_hash{block.GetHash()}; + if (!chainstate.SpendBlock(block, pindex, block_hash, coins, state)) { + LogError("SpendBlock failed %s\n", state.ToString()); + return VerifyDBResult::CORRUPTED_BLOCK_DB; + } if (!chainstate.ConnectBlock(block, state, pindex, coins)) { LogPrintf("Verification error: found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); return VerifyDBResult::CORRUPTED_BLOCK_DB; diff --git a/src/validation.h b/src/validation.h index e361c7af101..93be54c6122 100644 --- a/src/validation.h +++ b/src/validation.h @@ -709,6 +709,9 @@ public: bool ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool SpendBlock(const CBlock& block, const CBlockIndex* pindex, const uint256& block_hash, + CCoinsViewCache& view, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + // Apply the effects of a block disconnection on the UTXO set. bool DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); From e1f88913b7b416ae2cedf449a9fef55c4abc512a Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Sun, 20 Apr 2025 12:48:43 +0200 Subject: [PATCH 6/8] validation: Move SetBestBlock out of ConnectBlock This is a part of a series of commits for removing access to the CCoinsViewCache in consensus verification functions. The goal is to allow calling verification functions with pre-fetched, or a user-defined set of coins. --- src/validation.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index ea638703b89..608d1b14103 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2475,8 +2475,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // Special case for the genesis block, skipping connection of its transactions // (its coinbase is unspendable) if (block_hash == params.GetConsensus().hashGenesisBlock) { - if (!fJustCheck) - view.SetBestBlock(pindex->GetBlockHash()); return true; } @@ -2676,9 +2674,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, m_blockman.m_dirty_blockindex.insert(pindex); } - // add this block to the view's block chain - view.SetBestBlock(pindex->GetBlockHash()); - const auto time_6{SteadyClock::now()}; m_chainman.time_index += time_6 - time_5; LogDebug(BCLog::BENCH, " - Index writing: %.2fms [%.2fs (%.2fms/blk)]\n", @@ -3253,6 +3248,8 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, LogError("%s: ConnectBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString()); return false; } + // add this block to the view's block chain + view.SetBestBlock(block_hash); time_3 = SteadyClock::now(); m_chainman.time_connect_total += time_3 - time_2; assert(m_chainman.num_blocks_total > 0); @@ -4917,6 +4914,7 @@ VerifyDBResult CVerifyDB::VerifyDB( LogPrintf("Verification error: found unconnectable block at %d, hash=%s (%s)\n", pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString()); return VerifyDBResult::CORRUPTED_BLOCK_DB; } + coins.SetBestBlock(block_hash); if (chainstate.m_chainman.m_interrupt) return VerifyDBResult::INTERRUPTED; } } From 8221e89ac40349bfb322f9ca573c7fd9237e46a2 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Fri, 18 Apr 2025 13:59:56 +0200 Subject: [PATCH 7/8] validation: Move coin existence and spend check to SpendBlock Move the remaining UTXO-related operations from ConnectBlock to SpendBlock. This includes moving the existence check, the UpdateCoins call, and CBlockUndo generation. ConnectBlock now takes a pre-populated CBlockUndo as an argument and no longer accesses or manipulates the UTXO set. --- src/bench/connectblock.cpp | 6 +- src/consensus/tx_verify.cpp | 12 +--- src/consensus/tx_verify.h | 2 +- src/test/coinstatsindex_tests.cpp | 6 +- src/test/fuzz/coins_view.cpp | 2 +- src/txmempool.cpp | 3 +- src/validation.cpp | 102 ++++++++++++++++++++---------- src/validation.h | 6 +- 8 files changed, 86 insertions(+), 53 deletions(-) diff --git a/src/bench/connectblock.cpp b/src/bench/connectblock.cpp index e51333d0d27..1451b88c9d0 100644 --- a/src/bench/connectblock.cpp +++ b/src/bench/connectblock.cpp @@ -9,6 +9,7 @@ #include