phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-04-29 14:59:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

validation: Add SpendBlock function

Browse source 
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.
This commit is contained in:
TheCharlatan 2025-04-19 22:55:59 +02:00
parent 1eb575214d
commit 008748c3ae
No known key found for this signature in database
GPG key ID: 9B79B45691DB4173
4 changed files with 148 additions and 90 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/bench/connectblock.cpp
View file

@ -100,6 +100,8 @@ void BenchmarkConnectBlock(benchmark::Bench& bench, std::vector<CKey>& 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));
});
}

2
src/test/coinstatsindex_tests.cpp
View file

@ -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

231
src/validation.cpp
View file

@ -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<SecondsDouble>(m_chainman.time_check),
Ticks<MillisecondsDouble>(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<MillisecondsDouble>(time_1 - time_start),
block.vtx.size() == 0 ? 0 : Ticks<MillisecondsDouble>(time_1 - time_start) / block.vtx.size(),
Ticks<SecondsDouble>(m_chainman.time_connect),
m_chainman.num_blocks_total == 0 ? 0 : Ticks<MillisecondsDouble>(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<MillisecondsDouble>(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;

3
src/validation.h
View file

@ -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);