IIUC, we only care about state when something goes wrong, and we should only care about ppindex when all headers are processed. So I think a util::Expected<CBlockIndex*, BlockValidationState> return type fits really well here. It cleans up both out-parameters, prevents accessing ppindex when not all headers have been processed, and hides state when we don't care about it.
Example diff (didn't review carefully):
<details>
<summary>git diff on cb5b22214e</summary>
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f2768efe19..3f0f328840 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2960,8 +2960,6 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
return;
}
- const CBlockIndex *pindexLast = nullptr;
-
// We'll set already_validated_work to true if these headers are
// successfully processed as part of a low-work headers sync in progress
// (either in PRESYNC or REDOWNLOAD phase).
@@ -3046,11 +3044,9 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
// something new (if these headers are valid).
bool received_new_header{last_received_header == nullptr};
- // Now process all the headers and return the block validation state.
- BlockValidationState state{m_chainman.ProcessNewBlockHeaders(headers,
- /*min_pow_checked=*/true,
- &pindexLast)};
- if (state.IsInvalid()) {
+ auto headers_processed{m_chainman.ProcessNewBlockHeaders(headers, /*min_pow_checked=*/true)};
+ if (!headers_processed) {
+ const auto& state{headers_processed.error()};
if (!pfrom.IsInboundConn() && state.GetResult() == BlockValidationResult::BLOCK_CACHED_INVALID) {
// Warn user if outgoing peers send us headers of blocks that we previously marked as invalid.
LogWarning("%s (received from peer=%i). "
@@ -3061,9 +3057,10 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
return;
}
+ const CBlockIndex* pindexLast{headers_processed.value()};
assert(pindexLast);
- if (state.IsValid() && received_new_header) {
+ if (received_new_header) {
LogBlockHeader(*pindexLast, pfrom, /*via_compact_block=*/false);
}
@@ -4554,15 +4551,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
}
}
- const CBlockIndex *pindex = nullptr;
- BlockValidationState state{m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true, &pindex)};
- if (state.IsInvalid()) {
- MaybePunishNodeForBlock(pfrom.GetId(), state, /*via_compact_block=*/true, "invalid header via cmpctblock");
+ auto headers_processed{m_chainman.ProcessNewBlockHeaders({{cmpctblock.header}}, /*min_pow_checked=*/true)};
+ if (!headers_processed) {
+ MaybePunishNodeForBlock(pfrom.GetId(), headers_processed.error(), /*via_compact_block=*/true, "invalid header via cmpctblock");
return;
}
// If AcceptBlockHeader returned true, it set pindex
- Assert(pindex);
+ const CBlockIndex* pindex{Assert(headers_processed.value())};
if (received_new_header) {
LogBlockHeader(*pindex, pfrom, /*via_compact_block=*/true);
}
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index e979ccc9a4..fe3c827d15 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -1123,12 +1123,15 @@ static RPCHelpMan submitheader()
}
}
- BlockValidationState state{chainman.ProcessNewBlockHeaders({{h}}, /*min_pow_checked=*/true)};
- if (state.IsValid()) return UniValue::VNULL;
- if (state.IsError()) {
- throw JSONRPCError(RPC_VERIFY_ERROR, state.ToString());
+ if (auto headers_processed{chainman.ProcessNewBlockHeaders({{h}}, /*min_pow_checked=*/true)}) {
+ return UniValue::VNULL;
+ } else {
+ const auto& state{headers_processed.error()};
+ if (state.IsError()) {
+ throw JSONRPCError(RPC_VERIFY_ERROR, state.ToString());
+ }
+ throw JSONRPCError(RPC_VERIFY_ERROR, state.GetRejectReason());
}
- throw JSONRPCError(RPC_VERIFY_ERROR, state.GetRejectReason());
},
};
}
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index 0fef42eb88..3ec5f35258 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -103,7 +103,9 @@ bool BuildChainTestingSetup::BuildChain(const CBlockIndex* pindex,
for (auto& block : chain) {
block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
- if (!Assert(m_node.chainman)->ProcessNewBlockHeaders({{*block}}, true, &pindex).IsValid()) {
+ if (auto headers_processed{Assert(m_node.chainman)->ProcessNewBlockHeaders({{*block}}, true)}) {
+ pindex = *headers_processed;
+ } else {
return false;
}
}
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 197ae9c871..db4b0165f2 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -88,8 +88,7 @@ void initialize_chain()
if constexpr (INVALID) {
auto& chainman{*setup->m_node.chainman};
for (const auto& block : chain) {
- auto result{chainman.ProcessNewBlockHeaders({{*block}}, true)};
- Assert(result.IsValid());
+ Assert(chainman.ProcessNewBlockHeaders({{*block}}, true));
const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
Assert(index);
}
@@ -169,8 +168,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
// Consume the bool, but skip the code for the INVALID fuzz target
if constexpr (!INVALID) {
for (const auto& block : *g_chain) {
- auto result{chainman.ProcessNewBlockHeaders({{*block}}, true)};
- Assert(result.IsValid());
+ Assert(chainman.ProcessNewBlockHeaders({{*block}}, true));
const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockIndex(block->GetHash()))};
Assert(index);
}
diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 85ed77a9ab..ca2624859f 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -104,7 +104,7 @@ std::shared_ptr<CBlock> MinerTestingSetup::FinalizeBlock(std::shared_ptr<CBlock>
// submit block header, so that miner can get the block height from the
// global state and the node has the topology of the chain
- BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{*pblock}}, true).IsValid());
+ BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlockHeaders({{*pblock}}, true));
return pblock;
}
diff --git a/src/validation.cpp b/src/validation.cpp
index b323b5c06e..4c123c1cc7 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4300,31 +4300,27 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida
}
// Exposed wrapper for AcceptBlockHeader
-BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex)
+util::Expected<CBlockIndex*, BlockValidationState> ChainstateManager::ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked)
{
AssertLockNotHeld(cs_main);
Assume(!headers.empty());
BlockValidationState state;
+ CBlockIndex* pindex{nullptr};
{
LOCK(cs_main);
for (const CBlockHeader& header : headers) {
- CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
bool accepted{AcceptBlockHeader(header, state, &pindex, min_pow_checked)};
CheckBlockIndex();
if (!accepted) {
if (state.IsValid()) NONFATAL_UNREACHABLE();
- return state;
- }
-
- if (ppindex) {
- *ppindex = pindex;
+ return util::Unexpected{state};
}
}
}
if (NotifyHeaderTip()) {
- if (IsInitialBlockDownload() && ppindex && *ppindex) {
- const CBlockIndex& last_accepted{**ppindex};
+ if (IsInitialBlockDownload() && pindex) {
+ const CBlockIndex& last_accepted{*pindex};
int64_t blocks_left{(NodeClock::now() - last_accepted.Time()) / GetConsensus().PowTargetSpacing()};
blocks_left = std::max<int64_t>(0, blocks_left);
const double progress{100.0 * last_accepted.nHeight / (last_accepted.nHeight + blocks_left)};
@@ -4333,7 +4329,7 @@ BlockValidationState ChainstateManager::ProcessNewBlockHeaders(std::span<const C
}
if (!state.IsValid()) NONFATAL_UNREACHABLE();
- return state;
+ return pindex;
}
void ChainstateManager::ReportHeadersPresync(int64_t height, int64_t timestamp)
diff --git a/src/validation.h b/src/validation.h
index 85f252f6ab..0d755a40d6 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -1239,12 +1239,10 @@ public:
*
* [@param](/bitcoin-bitcoin/contributor/param/)[in] headers The block headers themselves
* [@param](/bitcoin-bitcoin/contributor/param/)[in] min_pow_checked True if proof-of-work anti-DoS checks have been done by caller for headers chain
- * [@param](/bitcoin-bitcoin/contributor/param/)[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers
- * [@returns](/bitcoin-bitcoin/contributor/returns/) BlockValidationState indicating the result. IsValid() returns true if all headers
- * were accepted. On failure, IsInvalid() is true and the state contains the specific
- * validation failure reason.
+ * [@returns](/bitcoin-bitcoin/contributor/returns/) The last new block index object for the given headers if all headers were accepted.
+ * Otherwise, BlockValidationState containing the validation failure reason.
*/
- BlockValidationState ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
+ util::Expected<CBlockIndex*, BlockValidationState> ProcessNewBlockHeaders(std::span<const CBlockHeader> headers, bool min_pow_checked) LOCKS_EXCLUDED(cs_main);
/**
* Sufficiently validate a block for disk storage (and store on disk).
</details>
