Skip to content

Commit

Permalink
Have testBlockValidity hold cs_main instead of caller
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Sjors committed Jun 27, 2024
1 parent f6dc6db commit a74b0f9
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 7 deletions.
2 changes: 1 addition & 1 deletion src/interfaces/mining.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
11 changes: 9 additions & 2 deletions src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool) override
Expand Down
4 changes: 0 additions & 4 deletions src/rpc/mining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,6 @@ static RPCHelpMan generateblock()

ChainstateManager& chainman = EnsureChainman(node);
{
LOCK(cs_main);

std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)};
if (!blocktemplate) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
Expand All @@ -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()));
Expand Down

0 comments on commit a74b0f9

Please sign in to comment.