Merge bitcoin/bitcoin#30335: Mining interface followups, reduce cs_main locking, test rpc bug fix

a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed Have testBlockValidity hold cs_main instead of caller (Sjors Provoost)
f6dc6db44ddc22ade96a69a02908f14cfb279a37 refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
5fb2b704897fe10b5bd5ed754a5afd2ddc4a9e1d Drop unneeded lock from createNewBlock (Sjors Provoost)
75ce7637ad75af890581660c0bb3565c3c03bd6c refactor: testBlockValidity make out argument last (Sjors Provoost)
83a9bef0e2acad7655e23d30e1c52412f380d93d Add missing include for mining interface (Sjors Provoost)

Pull request description:

  Followups from #30200

  Fixes:
  - `std::unique_ptr` needs `#include <memory>` (noticed while working on #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 https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176

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

Tree-SHA512: 805e133bb59303fcee107d6f02b3e2761396c290efb731a85e6a29ae56b4b1b9cd28ada9629e979704dcfd98cf35034e7e6b618e29923049eb1eca2f65630e41

1 files changed, 10 insertions, 4 deletions

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index e0bab6e22e..fa151407fa 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -870,10 +870,17 @@ public:
         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
@@ -881,7 +888,6 @@ public:
         BlockAssembler::Options options;
         ApplyArgsManOptions(gArgs, options);
 
-        LOCK(::cs_main);
         return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key);
     }