From a0a6dbbe75a535e606b4768e737a80483b36a6ab Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 25 Apr 2025 15:25:45 +0200 Subject: [PATCH 1/4] validation: refactor TestBlockValidity A later commit adds checkBlock() to the Mining interface. In order to avoid passing BlockValidationState over IPC, this commit first refactors TestBlockValidity to return a boolean instead, and pass failure reasons via a string. Comments are expanded. 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. --- src/node/miner.cpp | 8 ++--- src/rpc/mining.cpp | 20 +++++------ src/validation.cpp | 88 ++++++++++++++++++++++++++++++++++------------ src/validation.h | 26 ++++++++++---- 4 files changed, 98 insertions(+), 44 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index c24debd1e69..280736db2da 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -173,10 +173,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())); + std::string reason; + std::string debug; + if (m_options.test_block_validity && !TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) { + throw std::runtime_error(strprintf("TestBlockValidity failed: %s - %s", reason, debug)); } const auto time_2{SteadyClock::now()}; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 95184cafee2..3e6d5e7ebfd 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -387,9 +387,10 @@ 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)) { - throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString())); + std::string reason; + std::string debug; + if (!TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) { + throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s - %s", reason, debug)); } } @@ -741,13 +742,12 @@ 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; + std::string debug; + bool res{TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/true, reason, debug)}; + if (res) return UniValue::VNULL; + LogDebug(BCLog::RPC, "Invalid block: %s", debug); + return UniValue{reason}; } const UniValue& aClientRules = oparam.find_value("rules"); diff --git a/src/validation.cpp b/src/validation.cpp index 1213d8be9f9..bd38f5b83c9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4648,40 +4648,82 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& return result; } -bool TestBlockValidity(BlockValidationState& state, - const CChainParams& chainparams, - Chainstate& chainstate, +bool TestBlockValidity(Chainstate& chainstate, const CBlock& block, - CBlockIndex* pindexPrev, - bool fCheckPOW, - bool fCheckMerkleRoot) + const bool check_pow, + const bool check_merkle_root, + std::string& reason, + std::string& debug) { - 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; + // 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() + LOCK(chainstate.m_chainman.GetMutex()); - // 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()); + BlockValidationState state; + CBlockIndex* tip{Assert(chainstate.m_chain.Tip())}; + + if (block.hashPrevBlock != *Assert(tip->phashBlock)) { + reason = "inconclusive-not-best-prevblk"; return false; } - if (!CheckBlock(block, state, chainparams.GetConsensus(), fCheckPOW, fCheckMerkleRoot)) { - LogError("%s: Consensus::CheckBlock: %s\n", __func__, state.ToString()); + + // 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)) { + reason = state.GetRejectReason(); + debug = state.GetDebugMessage(); return false; } - if (!ContextualCheckBlock(block, state, chainstate.m_chainman, pindexPrev)) { - LogError("%s: Consensus::ContextualCheckBlock: %s\n", __func__, state.ToString()); + + /** + * 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)) { + reason = state.GetRejectReason(); + debug = state.GetDebugMessage(); return false; } - if (!chainstate.ConnectBlock(block, state, &indexDummy, viewNew, true)) { + + if (!ContextualCheckBlock(block, state, chainstate.m_chainman, tip)) { + reason = state.GetRejectReason(); + debug = state.GetDebugMessage(); + return false; + } + + // 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()); + index_dummy.pprev = tip; + index_dummy.nHeight = tip->nHeight + 1; + index_dummy.phashBlock = &block_hash; + CCoinsViewCache tip_view(&chainstate.CoinsTip()); + CCoinsView blockCoins; + CCoinsViewCache view(&blockCoins); + view.SetBackend(tip_view); + + // Set fJustCheck to true in order to update, and not clear, validation caches. + if(!chainstate.ConnectBlock(block, state, &index_dummy, view, /*fJustCheck=*/true)) { + reason = state.GetRejectReason(); + debug = state.GetDebugMessage(); + return false; + } + if (!Assume(state.IsValid())) { + LogError("Unexpected invalid validation state"); return false; } - assert(state.IsValid()); return true; } diff --git a/src/validation.h b/src/validation.h index e361c7af101..5a0f18c09f8 100644 --- a/src/validation.h +++ b/src/validation.h @@ -383,14 +383,26 @@ 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, +/** + * 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[out] reason rejection reason (BIP22) + * @param[out] debug more detailed rejection reason + * @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 + * + * For signets the challenge verification is skipped when check_pow is false. + */ +bool TestBlockValidity(Chainstate& chainstate, const CBlock& block, - CBlockIndex* pindexPrev, - bool fCheckPOW = true, - bool fCheckMerkleRoot = true) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + const bool check_pow, + const bool check_merkle_root, + std::string& reason, + std::string& debug); /** Check with the proof of work on each blockheader matches the value in nBits */ bool HasValidProofOfWork(const std::vector& headers, const Consensus::Params& consensusParams); From 9822bd64d26ca056c0fe44e5e2b3e1f38e6021ef Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 23 Jan 2025 12:59:54 +0100 Subject: [PATCH 2/4] ipc: drop BlockValidationState special handling The Mining interface avoids using BlockValidationState. --- src/ipc/capnp/mining-types.h | 8 +------- src/ipc/capnp/mining.capnp | 10 ---------- src/ipc/capnp/mining.cpp | 36 ------------------------------------ src/test/ipc_test.capnp | 3 +-- src/test/ipc_test.cpp | 19 ------------------- 5 files changed, 2 insertions(+), 74 deletions(-) diff --git a/src/ipc/capnp/mining-types.h b/src/ipc/capnp/mining-types.h index 2e60b43fcf3..62789759490 100644 --- a/src/ipc/capnp/mining-types.h +++ b/src/ipc/capnp/mining-types.h @@ -14,13 +14,7 @@ #include namespace mp { -// Custom serialization for BlockValidationState. -void CustomBuildMessage(InvokeContext& invoke_context, - const BlockValidationState& src, - ipc::capnp::messages::BlockValidationState::Builder&& builder); -void CustomReadMessage(InvokeContext& invoke_context, - const ipc::capnp::messages::BlockValidationState::Reader& reader, - BlockValidationState& dest); +// Custom serializations } // namespace mp #endif // BITCOIN_IPC_CAPNP_MINING_TYPES_H diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index 32048e0ed19..f3327bf2e7b 100644 --- a/src/ipc/capnp/mining.capnp +++ b/src/ipc/capnp/mining.capnp @@ -44,13 +44,3 @@ struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") { timeout @0 : Float64 $Proxy.name("timeout"); feeThreshold @1 : Int64 $Proxy.name("fee_threshold"); } - -# Note: serialization of the BlockValidationState C++ type is somewhat fragile -# and using the struct can be awkward. It would be good if testBlockValidity -# method were changed to return validity information in a simpler format. -struct BlockValidationState { - mode @0 :Int32; - result @1 :Int32; - rejectReason @2 :Text; - debugMessage @3 :Text; -} diff --git a/src/ipc/capnp/mining.cpp b/src/ipc/capnp/mining.cpp index 0f9533c1c73..f598f1b2d8e 100644 --- a/src/ipc/capnp/mining.cpp +++ b/src/ipc/capnp/mining.cpp @@ -8,40 +8,4 @@ #include namespace mp { -void CustomBuildMessage(InvokeContext& invoke_context, - const BlockValidationState& src, - ipc::capnp::messages::BlockValidationState::Builder&& builder) -{ - if (src.IsValid()) { - builder.setMode(0); - } else if (src.IsInvalid()) { - builder.setMode(1); - } else if (src.IsError()) { - builder.setMode(2); - } else { - assert(false); - } - builder.setResult(static_cast(src.GetResult())); - builder.setRejectReason(src.GetRejectReason()); - builder.setDebugMessage(src.GetDebugMessage()); -} - -void CustomReadMessage(InvokeContext& invoke_context, - const ipc::capnp::messages::BlockValidationState::Reader& reader, - BlockValidationState& dest) -{ - if (reader.getMode() == 0) { - assert(reader.getResult() == 0); - assert(reader.getRejectReason().size() == 0); - assert(reader.getDebugMessage().size() == 0); - } else if (reader.getMode() == 1) { - dest.Invalid(static_cast(reader.getResult()), reader.getRejectReason(), reader.getDebugMessage()); - } else if (reader.getMode() == 2) { - assert(reader.getResult() == 0); - dest.Error(reader.getRejectReason()); - assert(reader.getDebugMessage().size() == 0); - } else { - assert(false); - } -} } // namespace mp diff --git a/src/test/ipc_test.capnp b/src/test/ipc_test.capnp index 7fd59cf5882..e33f711bf3c 100644 --- a/src/test/ipc_test.capnp +++ b/src/test/ipc_test.capnp @@ -19,6 +19,5 @@ interface FooInterface $Proxy.wrap("FooImplementation") { passUniValue @2 (arg :Text) -> (result :Text); passTransaction @3 (arg :Data) -> (result :Data); passVectorChar @4 (arg :Data) -> (result :Data); - passBlockState @5 (arg :Mining.BlockValidationState) -> (result :Mining.BlockValidationState); - passScript @6 (arg :Data) -> (result :Data); + passScript @5 (arg :Data) -> (result :Data); } diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp index fb21b3a71d0..7e5157f418e 100644 --- a/src/test/ipc_test.cpp +++ b/src/test/ipc_test.cpp @@ -102,25 +102,6 @@ void IpcPipeTest() std::vector vec2{foo->passVectorChar(vec1)}; BOOST_CHECK_EQUAL(std::string_view(vec1.begin(), vec1.end()), std::string_view(vec2.begin(), vec2.end())); - BlockValidationState bs1; - bs1.Invalid(BlockValidationResult::BLOCK_MUTATED, "reject reason", "debug message"); - BlockValidationState bs2{foo->passBlockState(bs1)}; - BOOST_CHECK_EQUAL(bs1.IsValid(), bs2.IsValid()); - BOOST_CHECK_EQUAL(bs1.IsError(), bs2.IsError()); - BOOST_CHECK_EQUAL(bs1.IsInvalid(), bs2.IsInvalid()); - BOOST_CHECK_EQUAL(static_cast(bs1.GetResult()), static_cast(bs2.GetResult())); - BOOST_CHECK_EQUAL(bs1.GetRejectReason(), bs2.GetRejectReason()); - BOOST_CHECK_EQUAL(bs1.GetDebugMessage(), bs2.GetDebugMessage()); - - BlockValidationState bs3; - BlockValidationState bs4{foo->passBlockState(bs3)}; - BOOST_CHECK_EQUAL(bs3.IsValid(), bs4.IsValid()); - BOOST_CHECK_EQUAL(bs3.IsError(), bs4.IsError()); - BOOST_CHECK_EQUAL(bs3.IsInvalid(), bs4.IsInvalid()); - BOOST_CHECK_EQUAL(static_cast(bs3.GetResult()), static_cast(bs4.GetResult())); - BOOST_CHECK_EQUAL(bs3.GetRejectReason(), bs4.GetRejectReason()); - BOOST_CHECK_EQUAL(bs3.GetDebugMessage(), bs4.GetDebugMessage()); - auto script1{CScript() << OP_11}; auto script2{foo->passScript(script1)}; BOOST_CHECK_EQUAL(HexStr(script1), HexStr(script2)); From 6176f9b4c4092dac4b330e146ec091d0c08223e6 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 23 Jan 2025 11:39:20 +0100 Subject: [PATCH 3/4] Add checkBlock to Mining interface And use it in miner_tests, getblocktemplate and generateblock. --- src/interfaces/mining.h | 17 +++++++++++++++- src/ipc/capnp/mining.capnp | 6 ++++++ src/node/interfaces.cpp | 5 +++++ src/node/types.h | 12 ++++++++++++ src/rpc/mining.cpp | 4 ++-- src/test/miner_tests.cpp | 40 +++++++++++++++++++++++++++++++++++++- 6 files changed, 80 insertions(+), 4 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 95658dd95b2..72eb248aef8 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -7,7 +7,7 @@ #include // for CAmount #include // for BlockRef -#include // for BlockCreateOptions, BlockWaitOptions +#include // for BlockCreateOptions, BlockWaitOptions, BlockCheckOptions #include // for CBlock, CBlockHeader #include // for CTransactionRef #include // for int64_t @@ -114,6 +114,21 @@ public: */ virtual std::unique_ptr createNewBlock(const node::BlockCreateOptions& options = {}) = 0; + /** + * Checks if a given block is valid. + * + * @param[in] block the block to check + * @param[in] options verification options: the proof-of-work check can be + * skipped in order to verify a template generated by + * external software. + * @param[out] reason failure reason (BIP22) + * @param[out] debug more detailed rejection reason + * @returns whether the block is valid + * + * For signets the challenge verification is skipped when check_pow is false. + */ + virtual bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) = 0; + //! Get internal node context. Useful for RPC and testing, //! but not accessible across processes. virtual node::NodeContext* context() { return nullptr; } diff --git a/src/ipc/capnp/mining.capnp b/src/ipc/capnp/mining.capnp index f3327bf2e7b..8ee4745b858 100644 --- a/src/ipc/capnp/mining.capnp +++ b/src/ipc/capnp/mining.capnp @@ -18,6 +18,7 @@ interface Mining $Proxy.wrap("interfaces::Mining") { getTip @2 (context :Proxy.Context) -> (result: Common.BlockRef, hasResult: Bool); waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef); createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate); + checkBlock @5 (block: Data, options: BlockCheckOptions) -> (reason: Text, debug: Text, result: Bool); } interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") { @@ -44,3 +45,8 @@ struct BlockWaitOptions $Proxy.wrap("node::BlockWaitOptions") { timeout @0 : Float64 $Proxy.name("timeout"); feeThreshold @1 : Int64 $Proxy.name("fee_threshold"); } + +struct BlockCheckOptions $Proxy.wrap("node::BlockCheckOptions") { + checkMerkleRoot @0 :Bool $Proxy.name("check_merkle_root"); + checkPow @1 :Bool $Proxy.name("check_pow"); +} diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8aec2758f8b..4b40e4ce8a3 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -1110,6 +1110,11 @@ public: return std::make_unique(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node); } + bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override + { + return TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root, reason, debug); + } + NodeContext* context() override { return &m_node; } ChainstateManager& chainman() { return *Assert(m_node.chainman); } KernelNotifications& notifications() { return *Assert(m_node.notifications); } diff --git a/src/node/types.h b/src/node/types.h index 0f9b871084a..547d644831c 100644 --- a/src/node/types.h +++ b/src/node/types.h @@ -17,6 +17,7 @@ #include #include #include