re: #31981 (review)
Making this return a BlockValidationState means we have to pass it over the interface, which means we can't drop the special handling
Agree it's not good to pass BlockValidationState over the IPC interface (and the earlier change I posted didn't do that). I just think it it's best if validation.h/cpp uses BlockValidationState to be internally consistent and return the most information to callers.
None of the Assume checks seem problematic in production.
Thanks, I should have been clear my issue was just with unchecked Assume statements, not Assume statements in general. If unexpected states occur in releases I think it's best to report them or return them to callers, not ignore them.
re: #31981 (comment)
Probably also better for it not to LOCK(cs_main) to avoid unnecessary recursive mutex locking.
Can you elaborate on this?
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.
I do think current code as of c1939c43c3addb17c4316d49580762a1e0ec4504 is ok but that it would be nice to keep using BlockValidationState in validation code and return more complete error information. Here are the changes I'd suggest:
<details><summary>diff</summary>
<p>
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -1112,7 +1112,11 @@ public:
bool checkBlock(const CBlock& block, const node::BlockCheckOptions& options, std::string& reason, std::string& debug) override
{
- return TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root, reason, debug);
+ LOCK(chainman().GetMutex());
+ BlockValidationState state{TestBlockValidity(chainman().ActiveChainstate(), block, /*check_pow=*/options.check_pow, /*=check_merkle_root=*/options.check_merkle_root)};
+ reason = state.GetRejectReason();
+ debug = state.GetDebugMessage();
+ return state.IsValid();
}
NodeContext* context() override { return &m_node; }
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -173,10 +173,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
pblock->nNonce = 0;
- std::string reason;
- std::string debug;
- if (m_options.test_block_validity && !TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false, reason, debug)) {
- throw std::runtime_error(strprintf("TestBlockValidity failed: %s - %s", reason, debug));
+ if (m_options.test_block_validity) {
+ if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
+ throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString()));
+ }
}
const auto time_2{SteadyClock::now()};
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -387,10 +387,8 @@ static RPCHelpMan generateblock()
block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
RegenerateCommitments(block, chainman);
- std::string reason;
- std::string debug;
- if (!miner.checkBlock(block, {.check_merkle_root = false, .check_pow = false}, reason, debug)) {
- throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s - %s", reason, debug));
+ if (BlockValidationState state{TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) {
+ throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
}
}
@@ -742,12 +740,7 @@ static RPCHelpMan getblocktemplate()
return "duplicate-inconclusive";
}
- std::string reason;
- std::string debug;
- bool res{miner.checkBlock(block, {.check_pow = false}, reason, debug)};
- if (res) return UniValue::VNULL;
- LogDebug(BCLog::RPC, "Invalid block: %s", debug);
- return UniValue{reason};
+ return BIP22ValidationResult(TestBlockValidity(chainman.ActiveChainstate(), block, /*check_pow=*/false, /*check_merkle_root=*/true));
}
const UniValue& aClientRules = oparam.find_value("rules");
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4648,31 +4648,30 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
return result;
}
-bool TestBlockValidity(Chainstate& chainstate,
- const CBlock& block,
- const bool check_pow,
- const bool check_merkle_root,
- std::string& reason,
- std::string& debug)
+
+BlockValidationState TestBlockValidity(
+ Chainstate& chainstate,
+ const CBlock& block,
+ const bool check_pow,
+ const bool check_merkle_root)
{
// Lock must be held throughout this function for two reasons:
// 1. We don't want the tip to change during several of the validation steps
// 2. To prevent a CheckBlock() race condition for fChecked, see ProcessNewBlock()
- LOCK(chainstate.m_chainman.GetMutex());
+ AssertLockHeld(chainstate.m_chainman.GetMutex());
BlockValidationState state;
CBlockIndex* tip{Assert(chainstate.m_chain.Tip())};
if (block.hashPrevBlock != *Assert(tip->phashBlock)) {
- reason = "inconclusive-not-best-prevblk";
- return false;
+ state.Invalid({}, "inconclusive-not-best-prevblk");
+ return state;
}
// For signets CheckBlock() verifies the challenge iff fCheckPow is set.
if (!CheckBlock(block, state, chainstate.m_chainman.GetConsensus(), /*fCheckPow=*/check_pow, /*fCheckMerkleRoot=*/check_merkle_root)) {
- reason = state.GetRejectReason();
- debug = state.GetDebugMessage();
- return false;
+ if (state.IsValid()) state.Error("CheckBlock failed");
+ return state;
}
/**
@@ -4691,15 +4690,13 @@ bool TestBlockValidity(Chainstate& chainstate,
*/
if (!ContextualCheckBlockHeader(block, state, chainstate.m_blockman, chainstate.m_chainman, tip)) {
- reason = state.GetRejectReason();
- debug = state.GetDebugMessage();
- return false;
+ if (state.IsValid()) state.Error("ContextualCheckBlockHeader failed");
+ return state;
}
if (!ContextualCheckBlock(block, state, chainstate.m_chainman, tip)) {
- reason = state.GetRejectReason();
- debug = state.GetDebugMessage();
- return false;
+ if (state.IsValid()) state.Error("ContextualCheckBlock failed");
+ return state;
}
// We don't want ConnectBlock to update the actual chainstate, so create
@@ -4716,16 +4713,17 @@ bool TestBlockValidity(Chainstate& chainstate,
// Set fJustCheck to true in order to update, and not clear, validation caches.
if(!chainstate.ConnectBlock(block, state, &index_dummy, view, /*fJustCheck=*/true)) {
- reason = state.GetRejectReason();
- debug = state.GetDebugMessage();
- return false;
- }
- if (!Assume(state.IsValid())) {
- LogError("Unexpected invalid validation state");
- return false;
+ if (state.IsValid()) state.Error("ConnectBlock failed");
+ return state;
}
- return true;
+ // Ensure no check returned successfully while also setting an invalid state.
+ if (!Assume(state.IsValid())) {
+ LogError("Unexpected invalid state in TestBlockValidity: %s", state.ToString());
+ state.Error("TestBlockValidity failed");
+ }
+
+ return state;
}
/* This function is called from the RPC code for pruneblockchain */
--- a/src/validation.h
+++ b/src/validation.h
@@ -388,21 +388,23 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
*
* [@param](/bitcoin-bitcoin/contributor/param/)[in] block The block we want to process. Must connect to the
* current tip.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] chainstate The chainstate to connect to.
- * [@param](/bitcoin-bitcoin/contributor/param/)[out] reason rejection reason (BIP22)
- * [@param](/bitcoin-bitcoin/contributor/param/)[out] debug more detailed rejection reason
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] chainstate The chainstate to connect to.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] check_pow perform proof-of-work check, nBits in the header
* is always checked
* [@param](/bitcoin-bitcoin/contributor/param/)[in] check_merkle_root check the merkle root
*
+ * [@return](/bitcoin-bitcoin/contributor/return/) Valid or Invalid state. This doesn't currently return an Error state,
+ * and shouldn't unless there is something wrong with the existing
+ * chainstate. (This is different from functions like AcceptBlock which
+ * can fail trying to save new data.)
+ *
* For signets the challenge verification is skipped when check_pow is false.
*/
-bool TestBlockValidity(Chainstate& chainstate,
- const CBlock& block,
- const bool check_pow,
- const bool check_merkle_root,
- std::string& reason,
- std::string& debug);
+BlockValidationState TestBlockValidity(
+ Chainstate& chainstate,
+ const CBlock& block,
+ bool check_pow,
+ bool check_merkle_root) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
/** Check with the proof of work on each blockheader matches the value in nBits */
bool HasValidProofOfWork(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams);
</p>
</details>
