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