From 74690f4ed82b1584abb07c0387db0d924c4c0cab Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 25 Apr 2025 15:25:45 +0200 Subject: [PATCH] validation: refactor TestBlockValidity Comments are expanded. Return BlockValidationState instead of passing a reference. Lock Chainman mutex instead of cs_main. Remove redundant chainparams and pindexPrev arguments. Drop defaults for checking proof-of-work and merkle root. The ContextualCheckBlockHeader check is moved to after CheckBlock, which is more similar to normal validation where context-free checks are done first. Validation failure reasons are no longer printed through LogError(), since it depends on the caller whether this implies an actual bug in the node, or an externally sourced block that happens to be invalid. When called from getblocktemplate, via BlockAssembler::CreateNewBlock(), this method already throws an std::runtime_error if validation fails. Additionally it moves the inconclusive-not-best-prevblk check from RPC code to TestBlockValidity. There is no behavior change when callling getblocktemplate with proposal. Previously this would return a BIP22ValidationResult which can throw for state.IsError(). But CheckBlock() and the functions it calls only use state.IsValid(). The final assert is changed into Assume, with a LogError. Co-authored-by: --- src/node/miner.cpp | 8 ++-- src/rpc/mining.cpp | 11 +----- src/validation.cpp | 98 +++++++++++++++++++++++++++++++--------------- src/validation.h | 30 ++++++++++---- 4 files changed, 95 insertions(+), 52 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 7ffa7fb60c9..02a7684178c 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -179,10 +179,10 @@ std::unique_ptr BlockAssembler::CreateNewBlock() pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus()); pblock->nNonce = 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())); + if (m_options.test_block_validity) { + if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) { + throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString())); + } } const auto time_2{SteadyClock::now()}; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 4fa62447af6..f11305f16b8 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -388,8 +388,7 @@ static RPCHelpMan generateblock() block.vtx.insert(block.vtx.end(), txs.begin(), txs.end()); RegenerateCommitments(block, chainman); - BlockValidationState state; - if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) { + if (BlockValidationState state{TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) { throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString())); } } @@ -745,13 +744,7 @@ 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); + return BIP22ValidationResult(TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/true)); } const UniValue& aClientRules = oparam.find_value("rules"); diff --git a/src/validation.cpp b/src/validation.cpp index d5e92600d4f..b440c2752b7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4600,42 +4600,78 @@ 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) + +BlockValidationState TestBlockValidity( + Chainstate& chainstate, + const CBlock& block, + const bool check_pow, + const bool check_merkle_root) { - AssertLockHeld(cs_main); - assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip()); - CCoinsViewCache viewNew(&chainstate.CoinsTip()); + // Lock must be held throughout this function for two reasons: + // 1. We don't want the tip to change during several of the validation steps + // 2. To prevent a CheckBlock() race condition for fChecked, see ProcessNewBlock() + AssertLockHeld(chainstate.m_chainman.GetMutex()); + + BlockValidationState state; + CBlockIndex* tip{Assert(chainstate.m_chain.Tip())}; + + if (block.hashPrevBlock != *Assert(tip->phashBlock)) { + state.Invalid({}, "inconclusive-not-best-prevblk"); + return state; + } + + // For signets CheckBlock() verifies the challenge iff fCheckPow is set. + if (!CheckBlock(block, state, chainstate.m_chainman.GetConsensus(), /*fCheckPow=*/check_pow, /*fCheckMerkleRoot=*/check_merkle_root)) { + // This should never happen, but belt-and-suspenders don't approve the + // block if it does. + if (state.IsValid()) NONFATAL_UNREACHABLE(); + return state; + } + + /** + * At this point ProcessNewBlock would call AcceptBlock(), but we + * don't want to store the block or its header. Run individual checks + * instead: + * - skip AcceptBlockHeader() because: + * - we don't want to update the block index + * - we do not care about duplicates + * - we already ran CheckBlockHeader() via CheckBlock() + * - we already checked for prev-blk-not-found + * - we know the tip is valid, so no need to check bad-prevblk + * - we already ran CheckBlock() + * - do run ContextualCheckBlockHeader() + * - do run ContextualCheckBlock() + */ + + if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, tip)) { + if (state.IsValid()) NONFATAL_UNREACHABLE(); + return state; + } + + if (!ContextualCheckBlock(block, state, chainstate.m_chainman, tip)) { + if (state.IsValid()) NONFATAL_UNREACHABLE(); + return state; + } + + // We don't want ConnectBlock to update the actual chainstate, so create + // a cache on top of it, along with a dummy block index. + CBlockIndex index_dummy{block}; uint256 block_hash(block.GetHash()); - CBlockIndex indexDummy(block); - indexDummy.pprev = pindexPrev; - indexDummy.nHeight = pindexPrev->nHeight + 1; - indexDummy.phashBlock = &block_hash; + index_dummy.pprev = tip; + index_dummy.nHeight = tip->nHeight + 1; + index_dummy.phashBlock = &block_hash; + CCoinsViewCache view_dummy(&chainstate.CoinsTip()); - // 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; + // Set fJustCheck to true in order to update, and not clear, validation caches. + if(!chainstate.ConnectBlock(block, state, &index_dummy, view_dummy, /*fJustCheck=*/true)) { + if (state.IsValid()) NONFATAL_UNREACHABLE(); + return state; } - 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; + // Ensure no check returned successfully while also setting an invalid state. + if (!state.IsValid()) NONFATAL_UNREACHABLE(); + + return state; } /* This function is called from the RPC code for pruneblockchain */ diff --git a/src/validation.h b/src/validation.h index c7e70e3adbc..9b3d1f00f2e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -384,14 +384,28 @@ public: /** 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); +/** + * Verify a block, including transactions. + * + * @param[in] block The block we want to process. Must connect to the + * current tip. + * @param[in] chainstate The chainstate to connect to. + * @param[in] check_pow perform proof-of-work check, nBits in the header + * is always checked + * @param[in] check_merkle_root check the merkle root + * + * @return Valid or Invalid state. This doesn't currently return an Error state, + * and shouldn't unless there is something wrong with the existing + * chainstate. (This is different from functions like AcceptBlock which + * can fail trying to save new data.) + * + * For signets the challenge verification is skipped when check_pow is false. + */ +BlockValidationState TestBlockValidity( + Chainstate& chainstate, + const CBlock& block, + bool check_pow, + bool check_merkle_root) 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);