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