Not too important but there’s an effort to replace recursive mutexes with nonrecursive ones in #19303. Suggestion is just to replace the lock with a lock annotation.
0--- a/src/node/interfaces.cpp
1+++ b/src/node/interfaces.cpp
2@@ -1112,7 +1112,11 @@ public:
3
4 bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
5 {
6- return TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root, reason, debug);
7+ LOCK(chainman().GetMutex());
8+ BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root)};
9+ reason = state.GetRejectReason();
10+ debug = state.GetDebugMessage();
11+ return state.IsValid();
12 }
13
14 NodeContext* context() override { return &m_node; }
15--- a/src/node/miner.cpp
16+++ b/src/node/miner.cpp
17@@ -173,10 +173,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
18 pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
19 pblock->nNonce = 0;
20
21- std::string reason;
22- std::string debug;
23- if (m_options.test_block_validity && !TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) {
24- throw std::runtime_error(strprintf("TestBlockValidity failed: %s - %s", reason, debug));
25+ if (m_options.test_block_validity) {
26+ if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
27+ throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
28+ }
29 }
30 const auto time_2{SteadyClock::now()};
31
32--- a/src/rpc/mining.cpp
33+++ b/src/rpc/mining.cpp
34@@ -387,10 +387,8 @@ static RPCHelpMan generateblock()
35 block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
36 RegenerateCommitments(block, chainman);
37
38- std::string reason;
39- std::string debug;
40- if (!miner.checkBlock(block, {.check_merkle_root = false, .check_pow = false}, reason, debug)) {
41- throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s - %s", reason, debug));
42+ if (BlockValidationState state{TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
43+ throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
44 }
45 }
46
47@@ -742,12 +740,7 @@ static RPCHelpMan getblocktemplate()
48 return "duplicate-inconclusive";
49 }
50
51- std::string reason;
52- std::string debug;
53- bool res{miner.checkBlock(block, {.check_pow = false}, reason, debug)};
54- if (res) return UniValue::VNULL;
55- LogDebug(BCLog::RPC, "Invalid block: %s", debug);
56- return UniValue{reason};
57+ return BIP22ValidationResult(TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/true));
58 }
59
60 const UniValue& aClientRules = oparam.find_value("rules");
61--- a/src/validation.cpp
62+++ b/src/validation.cpp
63@@ -4648,31 +4648,30 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
64 return result;
65 }
66
67-bool TestBlockValidity(Chainstate& chainstate,
68- const CBlock& block,
69- const bool check_pow,
70- const bool check_merkle_root,
71- std::string& reason,
72- std::string& debug)
73+
74+BlockValidationState TestBlockValidity(
75+ Chainstate& chainstate,
76+ const CBlock& block,
77+ const bool check_pow,
78+ const bool check_merkle_root)
79 {
80 // Lock must be held throughout this function for two reasons:
81 // 1. We don't want the tip to change during several of the validation steps
82 // 2. To prevent a CheckBlock() race condition for fChecked, see ProcessNewBlock()
83- LOCK(chainstate.m_chainman.GetMutex());
84+ AssertLockHeld(chainstate.m_chainman.GetMutex());
85
86 BlockValidationState state;
87 CBlockIndex* tip{Assert(chainstate.m_chain.Tip())};
88
89 if (block.hashPrevBlock != *Assert(tip->phashBlock)) {
90- reason = "inconclusive-not-best-prevblk";
91- return false;
92+ state.Invalid({}, "inconclusive-not-best-prevblk");
93+ return state;
94 }
95
96 // For signets CheckBlock() verifies the challenge iff fCheckPow is set.
97 if (!CheckBlock(block, state, chainstate.m_chainman.GetConsensus(), /*fCheckPow=*/check_pow, /*fCheckMerkleRoot=*/check_merkle_root)) {
98- reason = state.GetRejectReason();
99- debug = state.GetDebugMessage();
100- return false;
101+ if (state.IsValid()) state.Error("CheckBlock failed");
102+ return state;
103 }
104
105 /**
106@@ -4691,15 +4690,13 @@ bool TestBlockValidity(Chainstate& chainstate,
107 */
108
109 if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, tip)) {
110- reason = state.GetRejectReason();
111- debug = state.GetDebugMessage();
112- return false;
113+ if (state.IsValid()) state.Error("ContextualCheckBlockHeader failed");
114+ return state;
115 }
116
117 if (!ContextualCheckBlock(block, state, chainstate.m_chainman, tip)) {
118- reason = state.GetRejectReason();
119- debug = state.GetDebugMessage();
120- return false;
121+ if (state.IsValid()) state.Error("ContextualCheckBlock failed");
122+ return state;
123 }
124
125 // We don't want ConnectBlock to update the actual chainstate, so create
126@@ -4716,16 +4713,17 @@ bool TestBlockValidity(Chainstate& chainstate,
127
128 // Set fJustCheck to true in order to update, and not clear, validation caches.
129 if(!chainstate.ConnectBlock(block, state, &index_dummy, view, /*fJustCheck=*/true)) {
130- reason = state.GetRejectReason();
131- debug = state.GetDebugMessage();
132- return false;
133- }
134- if (!Assume(state.IsValid())) {
135- LogError("Unexpected invalid validation state");
136- return false;
137+ if (state.IsValid()) state.Error("ConnectBlock failed");
138+ return state;
139 }
140
141- return true;
142+ // Ensure no check returned successfully while also setting an invalid state.
143+ if (!Assume(state.IsValid())) {
144+ LogError("Unexpected invalid state in TestBlockValidity: %s", state.ToString());
145+ state.Error("TestBlockValidity failed");
146+ }
147+
148+ return state;
149 }
150
151 /* This function is called from the RPC code for pruneblockchain */
152--- a/src/validation.h
153+++ b/src/validation.h
154@@ -388,21 +388,23 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
155 *
156 * [@param](/bitcoin-bitcoin/contributor/param/)[in] block The block we want to process. Must connect to the
157 * current tip.
158- * [@param](/bitcoin-bitcoin/contributor/param/)[in] chainstate The chainstate to connect to.
159- * [@param](/bitcoin-bitcoin/contributor/param/)[out] reason rejection reason (BIP22)
160- * [@param](/bitcoin-bitcoin/contributor/param/)[out] debug more detailed rejection reason
161+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] chainstate The chainstate to connect to.
162 * [@param](/bitcoin-bitcoin/contributor/param/)[in] check_pow perform proof-of-work check, nBits in the header
163 * is always checked
164 * [@param](/bitcoin-bitcoin/contributor/param/)[in] check_merkle_root check the merkle root
165 *
166+ * [@return](/bitcoin-bitcoin/contributor/return/) Valid or Invalid state. This doesn't currently return an Error state,
167+ * and shouldn't unless there is something wrong with the existing
168+ * chainstate. (This is different from functions like AcceptBlock which
169+ * can fail trying to save new data.)
170+ *
171 * For signets the challenge verification is skipped when check_pow is false.
172 */
173-bool TestBlockValidity(Chainstate& chainstate,
174- const CBlock& block,
175- const bool check_pow,
176- const bool check_merkle_root,
177- std::string& reason,
178- std::string& debug);
179+BlockValidationState TestBlockValidity(
180+ Chainstate& chainstate,
181+ const CBlock& block,
182+ bool check_pow,
183+ bool check_merkle_root) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
184
185 /** Check with the proof of work on each blockheader matches the value in nBits */
186 bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);
