Merge bitcoin#30335: Mining interface followups, reduce cs_main locki…

…ng, test rpc bug fix

a74b0f9 Have testBlockValidity hold cs_main instead of caller (Sjors Provoost)
f6dc6db refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
5fb2b70 Drop unneeded lock from createNewBlock (Sjors Provoost)
75ce763 refactor: testBlockValidity make out argument last (Sjors Provoost)
83a9bef Add missing include for mining interface (Sjors Provoost)

Pull request description:

  Followups from bitcoin#30200

  Fixes:
  - `std::unique_ptr` needs `#include <memory>` (noticed while working on bitcoin#30332, which has fewer includes than its parent PR that I originally tested with)
  - Drop lock from createNewBlock that was spuriously added
  - Have testBlockValidity hold cs_main instead of caller (also fixes a race condition in test-only code)

  Refactor:
  - Use CHECK_NONFATAL to avoid single-use symbol (refactor)
  - move output argument `state` to the end of `testBlockValidity`, see bitcoin#30200 (comment)

ACKs for top commit:
  AngusP:
    Code Review ACK a74b0f9
  itornaza:
    Tested ACK a74b0f9
  ryanofsky:
    Code review ACK a74b0f9. Just new error string is added since last review, and a commit message was updated

Tree-SHA512: 805e133bb59303fcee107d6f02b3e2761396c290efb731a85e6a29ae56b4b1b9cd28ada9629e979704dcfd98cf35034e7e6b618e29923049eb1eca2f65630e41

src/interfaces/mining.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_INTERFACES_MINING_H
 #define BITCOIN_INTERFACES_MINING_H
 
+#include <memory>
 #include <optional>
 #include <uint256.h>
 
@@ -44,6 +45,7 @@ class Mining
  * @returns a block template
  */
  virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
+
  /**
  * Processes new block. A valid new block is automatically relayed to peers.
  *
@@ -62,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()
- * @returns false if any of the checks fail
+ * @param[out] state details of why a block failed to validate
+ * @returns false if it does not build on the current tip, or 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.

src/node/interfaces.cpp

@@ -870,18 +870,24 @@ 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);
+ 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
  {
  BlockAssembler::Options options;
  ApplyArgsManOptions(gArgs, options);
 
- LOCK(::cs_main);
  return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key);
  }
 

src/rpc/mining.cpp

@@ -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");
@@ -387,10 +385,8 @@ static RPCHelpMan generateblock()
  RegenerateCommitments(block, chainman);
 
  {
- 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()));
  }
  }
@@ -667,9 +663,7 @@ static RPCHelpMan getblocktemplate()
  ChainstateManager& chainman = EnsureChainman(node);
  Mining& miner = EnsureMining(node);
  LOCK(cs_main);
- std::optional<uint256> 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;
@@ -713,7 +707,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);
  }