From 83a9bef0e2acad7655e23d30e1c52412f380d93d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 25 Jun 2024 13:28:36 +0200 Subject: [PATCH 1/5] Add missing include for mining interface Needed for std::unique_ptr --- src/interfaces/mining.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index b96881f67c487..1f603399bfee3 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_INTERFACES_MINING_H #define BITCOIN_INTERFACES_MINING_H +#include #include #include From 75ce7637ad75af890581660c0bb3565c3c03bd6c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 25 Jun 2024 13:33:35 +0200 Subject: [PATCH 2/5] refactor: testBlockValidity make out argument last --- src/interfaces/mining.h | 5 +++-- src/node/interfaces.cpp | 2 +- src/rpc/mining.cpp | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 1f603399bfee3..bc98448833010 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -45,6 +45,7 @@ class Mining * @returns a block template */ virtual std::unique_ptr createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0; + /** * Processes new block. A valid new block is automatically relayed to peers. * @@ -63,12 +64,12 @@ class Mining * Only works on top of our current best block. * Does not check proof-of-work. * - * @param[out] state details of why a block failed to validate * @param[in] block the block to validate * @param[in] check_merkle_root call CheckMerkleRoot() + * @param[out] state details of why a block failed to validate * @returns false if any of the checks fail */ - virtual bool testBlockValidity(BlockValidationState& state, const CBlock& block, bool check_merkle_root = true) = 0; + virtual bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) = 0; //! Get internal node context. Useful for RPC and testing, //! but not accessible across processes. diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index e0bab6e22e80d..931b98e59c643 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -870,7 +870,7 @@ class MinerImpl : public Mining return context()->mempool->GetTransactionsUpdated(); } - bool testBlockValidity(BlockValidationState& state, const CBlock& block, bool check_merkle_root) override + bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) override { LOCK(::cs_main); return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root); diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 2b93c189657d6..fe91d1ed89dc1 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -390,7 +390,7 @@ static RPCHelpMan generateblock() LOCK(cs_main); BlockValidationState state; - if (!miner.testBlockValidity(state, block, /*check_merkle_root=*/false)) { + if (!miner.testBlockValidity(block, /*check_merkle_root=*/false, state)) { throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("testBlockValidity failed: %s", state.ToString())); } } @@ -713,7 +713,7 @@ static RPCHelpMan getblocktemplate() return "inconclusive-not-best-prevblk"; } BlockValidationState state; - miner.testBlockValidity(state, block); + miner.testBlockValidity(block, /*check_merkle_root=*/true, state); return BIP22ValidationResult(state); } From 5fb2b704897fe10b5bd5ed754a5afd2ddc4a9e1d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 26 Jun 2024 12:27:06 +0200 Subject: [PATCH 3/5] Drop unneeded lock from createNewBlock This was added in 4bf2e361da1964f7c278b4939967a0e5afde20b0, but BlockAssembler::CreateNewBlock already locks cs_main internally. --- src/node/interfaces.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 931b98e59c643..5e87bf234a72f 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -881,7 +881,6 @@ class MinerImpl : public Mining BlockAssembler::Options options; ApplyArgsManOptions(gArgs, options); - LOCK(::cs_main); return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key); } From f6dc6db44ddc22ade96a69a02908f14cfb279a37 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 26 Jun 2024 12:29:33 +0200 Subject: [PATCH 4/5] refactor: use CHECK_NONFATAL to avoid single-use symbol --- src/rpc/mining.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index fe91d1ed89dc1..d9fd82d708286 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -667,9 +667,7 @@ static RPCHelpMan getblocktemplate() ChainstateManager& chainman = EnsureChainman(node); Mining& miner = EnsureMining(node); LOCK(cs_main); - std::optional maybe_tip{miner.getTipHash()}; - CHECK_NONFATAL(maybe_tip); - uint256 tip{maybe_tip.value()}; + uint256 tip{CHECK_NONFATAL(miner.getTipHash()).value()}; std::string strMode = "template"; UniValue lpval = NullUniValue; From a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 26 Jun 2024 13:42:55 +0200 Subject: [PATCH 5/5] Have testBlockValidity hold cs_main instead of caller The goal of interfaces is to eventually run in their own process, so we can't use EXCLUSIVE_LOCKS_REQUIRED in their declaration. However TestBlockValidaty will crash (in its call to ConnectBlock) if the tip changes from under the proposed block. Have the testBlockValidity implementation hold the lock instead, and non-fatally check for this condition. --- src/interfaces/mining.h | 2 +- src/node/interfaces.cpp | 11 +++++++++-- src/rpc/mining.cpp | 4 ---- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index bc98448833010..7d71a01450e25 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -67,7 +67,7 @@ class Mining * @param[in] block the block to validate * @param[in] check_merkle_root call CheckMerkleRoot() * @param[out] state details of why a block failed to validate - * @returns false if any of the checks fail + * @returns false if it does not build on the current tip, or any of the checks fail */ virtual bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 5e87bf234a72f..fa151407fa0d5 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -872,8 +872,15 @@ class MinerImpl : public Mining bool testBlockValidity(const CBlock& block, bool check_merkle_root, BlockValidationState& state) override { - LOCK(::cs_main); - return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root); + LOCK(cs_main); + CBlockIndex* tip{chainman().ActiveChain().Tip()}; + // Fail if the tip updated before the lock was taken + if (block.hashPrevBlock != tip->GetBlockHash()) { + state.Error("Block does not connect to current chain tip."); + return false; + } + + return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root); } std::unique_ptr createNewBlock(const CScript& script_pub_key, bool use_mempool) override diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index d9fd82d708286..7e420dcd9b509 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -371,8 +371,6 @@ static RPCHelpMan generateblock() ChainstateManager& chainman = EnsureChainman(node); { - LOCK(cs_main); - std::unique_ptr blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)}; if (!blocktemplate) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); @@ -387,8 +385,6 @@ static RPCHelpMan generateblock() RegenerateCommitments(block, chainman); { - LOCK(cs_main); - BlockValidationState state; if (!miner.testBlockValidity(block, /*check_merkle_root=*/false, state)) { throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("testBlockValidity failed: %s", state.ToString()));