From 5ae0c470d98a057892ce9b0e22d5cbc2f7961f3b Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 30 Dec 2024 17:42:22 +0100 Subject: [PATCH] validation: replace TestBlockValidity Delete the original TestBlockValidity and rename CheckNewBlock. Have the generateblock and getblocktemplate RPC calls, as well as BlockAssembler::CreateNewBlock use the new method. --- src/node/interfaces.cpp | 2 +- src/node/miner.cpp | 99 ++++++++++++++++++++++------------------- src/rpc/mining.cpp | 16 +++---- src/validation.cpp | 40 +---------------- src/validation.h | 11 +---- 5 files changed, 61 insertions(+), 107 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 6f28acfd4c2408..4bbf1e80ebd3e2 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -990,7 +990,7 @@ class MinerImpl : public Mining bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason) override { - return chainman().CheckNewBlock(block, reason, options.check_merkle_root, options.check_pow, options.target); + return chainman().TestBlockValidity(block, reason, options.check_merkle_root, options.check_pow, options.target); } NodeContext* context() override { return &m_node; } diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 5d7304b597ea34..25df620c0a3e0c 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -120,58 +120,63 @@ std::unique_ptr BlockAssembler::CreateNewBlock() pblocktemplate->vTxFees.push_back(-1); // updated at end pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end - LOCK(::cs_main); - CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip(); - assert(pindexPrev != nullptr); - nHeight = pindexPrev->nHeight + 1; - - pblock->nVersion = m_chainstate.m_chainman.m_versionbitscache.ComputeBlockVersion(pindexPrev, chainparams.GetConsensus()); - // -regtest only: allow overriding block.nVersion with - // -blockversion=N to test forking scenarios - if (chainparams.MineBlocksOnDemand()) { - pblock->nVersion = gArgs.GetIntArg("-blockversion", pblock->nVersion); - } - - pblock->nTime = TicksSinceEpoch(NodeClock::now()); - m_lock_time_cutoff = pindexPrev->GetMedianTimePast(); - + std::chrono::steady_clock::time_point time_1; int nPackagesSelected = 0; int nDescendantsUpdated = 0; - if (m_mempool) { - addPackageTxs(nPackagesSelected, nDescendantsUpdated); + + { + LOCK(::cs_main); + CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip(); + assert(pindexPrev != nullptr); + nHeight = pindexPrev->nHeight + 1; + + pblock->nVersion = m_chainstate.m_chainman.m_versionbitscache.ComputeBlockVersion(pindexPrev, chainparams.GetConsensus()); + // -regtest only: allow overriding block.nVersion with + // -blockversion=N to test forking scenarios + if (chainparams.MineBlocksOnDemand()) { + pblock->nVersion = gArgs.GetIntArg("-blockversion", pblock->nVersion); + } + + pblock->nTime = TicksSinceEpoch(NodeClock::now()); + m_lock_time_cutoff = pindexPrev->GetMedianTimePast(); + + + if (m_mempool) { + addPackageTxs(nPackagesSelected, nDescendantsUpdated); + } + + time_1 = SteadyClock::now(); + + m_last_block_num_txs = nBlockTx; + m_last_block_weight = nBlockWeight; + + // Create coinbase transaction. + CMutableTransaction coinbaseTx; + coinbaseTx.vin.resize(1); + coinbaseTx.vin[0].prevout.SetNull(); + coinbaseTx.vout.resize(1); + coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script; + coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus()); + coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0; + pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx)); + pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev); + pblocktemplate->vTxFees[0] = -nFees; + + LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost); + + // Fill in header + pblock->hashPrevBlock = pindexPrev->GetBlockHash(); + UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev); + pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus()); + pblock->nNonce = 0; + pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]); } - const auto time_1{SteadyClock::now()}; - - m_last_block_num_txs = nBlockTx; - m_last_block_weight = nBlockWeight; - - // Create coinbase transaction. - CMutableTransaction coinbaseTx; - coinbaseTx.vin.resize(1); - coinbaseTx.vin[0].prevout.SetNull(); - coinbaseTx.vout.resize(1); - coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script; - coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus()); - coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0; - pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx)); - pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev); - pblocktemplate->vTxFees[0] = -nFees; - - LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost); - - // Fill in header - pblock->hashPrevBlock = pindexPrev->GetBlockHash(); - UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev); - pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus()); - pblock->nNonce = 0; - pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]); - - BlockValidationState state; - if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, - /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) { - throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString())); + std::string reason; + if (m_options.test_block_validity && !m_chainstate.m_chainman.TestBlockValidity(*pblock, reason, /*check_merkle_root=*/false, /*check_pow=*/false)) { + throw std::runtime_error(strprintf("TestBlockValidity failed: %s", reason)); } + const auto time_2{SteadyClock::now()}; LogDebug(BCLog::BENCH, "CreateNewBlock() packages: %.2fms (%d packages, %d updated descendants), validity: %.2fms (total %.2fms)\n", diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 35510677633fe3..c68231dd6ebb83 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -385,9 +385,9 @@ static RPCHelpMan generateblock() { LOCK(::cs_main); - BlockValidationState state; - if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) { - throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString())); + std::string reason; + if (!miner.checkBlock(block, {.check_merkle_root = false, .check_pow = false}, reason)) { + throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", reason)); } } @@ -712,13 +712,9 @@ static RPCHelpMan getblocktemplate() return "duplicate-inconclusive"; } - // TestBlockValidity only supports blocks built on the current Tip - if (block.hashPrevBlock != tip) { - return "inconclusive-not-best-prevblk"; - } - BlockValidationState state; - TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/true); - return BIP22ValidationResult(state); + std::string reason; + bool res{miner.checkBlock(block, {.check_pow = false}, reason)}; + return res ? UniValue::VNULL : UniValue{reason}; } const UniValue& aClientRules = oparam.find_value("rules"); diff --git a/src/validation.cpp b/src/validation.cpp index 36898527145704..67cd9c54070890 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4585,7 +4585,7 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr& pblock, return true; } -bool ChainstateManager::CheckNewBlock(const CBlock& block, std::string& reason, const bool check_merkle_root, const bool check_pow, const uint256 target) +bool ChainstateManager::TestBlockValidity(const CBlock& block, std::string& reason, const bool check_merkle_root, const bool check_pow, const uint256 target) { AssertLockNotHeld(cs_main); // See ProcessNewBlock() for rationale @@ -4754,44 +4754,6 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& return result; } -bool TestBlockValidity(BlockValidationState& state, - const CChainParams& chainparams, - Chainstate& chainstate, - const CBlock& block, - CBlockIndex* pindexPrev, - bool fCheckPOW, - bool fCheckMerkleRoot) -{ - AssertLockHeld(cs_main); - assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip()); - CCoinsViewCache viewNew(&chainstate.CoinsTip()); - uint256 block_hash(block.GetHash()); - CBlockIndex indexDummy(block); - indexDummy.pprev = pindexPrev; - indexDummy.nHeight = pindexPrev->nHeight + 1; - indexDummy.phashBlock = &block_hash; - - // NOTE: CheckBlockHeader is called by CheckBlock - if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, pindexPrev)) { - LogError("%s: Consensus::ContextualCheckBlockHeader: %s\n", __func__, state.ToString()); - return false; - } - if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) { - LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString()); - return false; - } - if (!ContextualCheckBlock(block, state, chainstate.m_chainman, pindexPrev)) { - LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString()); - return false; - } - if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) { - return false; - } - assert(state.IsValid()); - - return true; -} - /* This function is called from the RPC code for pruneblockchain */ void PruneBlockFilesManual(Chainstate& active_chainstate, int nManualPruneHeight) { diff --git a/src/validation.h b/src/validation.h index c25d7ccbcdcd37..143493202c60fd 100644 --- a/src/validation.h +++ b/src/validation.h @@ -383,15 +383,6 @@ class ValidationCache /** Context-independent validity checks */ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true); -/** Check a block is completely valid from start to finish (only works on top of our current best block) */ -bool TestBlockValidity(BlockValidationState& state, - const CChainParams& chainparams, - Chainstate& chainstate, - const CBlock& block, - CBlockIndex* pindexPrev, - bool fCheckPOW = true, - bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** Check with the proof of work on each blockheader matches the value in nBits */ bool HasValidProofOfWork(const std::vector& headers, const Consensus::Params& consensusParams); @@ -1197,7 +1188,7 @@ class ChainstateManager * For signets the challenge verification is skipped when check_pow is false or * a higher target is provided. */ - bool CheckNewBlock(const CBlock& block, std::string& reason, const bool check_merkle_root = true, const bool check_pow = true, const uint256 target = uint256::ZERO) LOCKS_EXCLUDED(cs_main); + bool TestBlockValidity(const CBlock& block, std::string& reason, const bool check_merkle_root = true, const bool check_pow = true, const uint256 target = uint256::ZERO) LOCKS_EXCLUDED(cs_main); /** * Process an incoming block. This only returns after the best known valid