refactor: Add ChainstateManager::ActivateBestChains() method
What changed, and why it matters
This is a routine code cleanup (refactor) in Bitcoin Core. It pulls three nearly identical loops that call ActivateBestChain() into a single new helper method called ActivateBestChains(). There is no change to what the program actually does, no new user-facing behavior, and no indication of a security fix.
No security action needed. Treat as normal maintenance/refactoring commit.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit introduces ChainstateManager::ActivateBestChains(), which iterates over all chainstates and calls ActivateBestChain() on each, returning a util::Result
Changed components
src/validation.cppsrc/validation.hsrc/kernel/bitcoinkernel.cppsrc/node/blockstorage.cppInspect captured patch +26 / −27
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 3a36e2c8..5bb1a06b 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -982,13 +982,9 @@ btck_ChainstateManager* btck_chainstate_manager_create(
LogError("Failed to verify loaded chain state from your datadir: %s", chainstate_err.original);
return nullptr;
}
-
- for (Chainstate* chainstate : WITH_LOCK(chainman->GetMutex(), return chainman->GetAll())) {
- BlockValidationState state;
- if (!chainstate->ActivateBestChain(state, nullptr)) {
- LogError("Failed to connect best block: %s", state.ToString());
- return nullptr;
- }
+ if (auto result = chainman->ActivateBestChains(); !result) {
+ LogError("%s", util::ErrorString(result).original);
+ return nullptr;
}
} catch (const std::exception& e) {
LogError("Failed to load chainstate: %s", e.what());
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index c5d26e5c..0bb22d90 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -1270,16 +1270,8 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
}
// scan for better chains in the block chain database, that are not yet connected in the active best chain
-
- // We can't hold cs_main during ActivateBestChain even though we're accessing
- // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
- // the relevant pointers before the ABC call.
- for (Chainstate* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
- BlockValidationState state;
- if (!chainstate->ActivateBestChain(state, nullptr)) {
- chainman.GetNotifications().fatalError(strprintf(_("Failed to connect best block (%s)."), state.ToString()));
- return;
- }
+ if (auto result = chainman.ActivateBestChains(); !result) {
+ chainman.GetNotifications().fatalError(util::ErrorString(result));
}
// End scope of ImportingNow
}
diff --git a/src/validation.cpp b/src/validation.cpp
index 0d16dc20..cc080d01 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5137,16 +5137,8 @@ void ChainstateManager::LoadExternalBlockFile(
// until after all of the block files are loaded. ActivateBestChain can be
// called by concurrent network message processing. but, that is not
// reliable for the purpose of pruning while importing.
- bool activation_failure = false;
- for (auto c : GetAll()) {
- BlockValidationState state;
- if (!c->ActivateBestChain(state, pblock)) {
- LogDebug(BCLog::REINDEX, "failed to activate chain (%s)\n", state.ToString());
- activation_failure = true;
- break;
- }
- }
- if (activation_failure) {
+ if (auto result{ActivateBestChains()}; !result) {
+ LogDebug(BCLog::REINDEX, "%s\n", util::ErrorString(result).original);
break;
}
}
@@ -6446,3 +6438,19 @@ std::optional<std::pair<const CBlockIndex*, const CBlockIndex*>> ChainstateManag
if (!chainstate) return {};
return std::make_pair(chainstate->m_chain.Tip(), chainstate->TargetBlock());
}
+
+util::Result<void> ChainstateManager::ActivateBestChains()
+{
+ // We can't hold cs_main during ActivateBestChain even though we're accessing
+ // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
+ // the relevant pointers before the ABC call.
+ AssertLockNotHeld(cs_main);
+ for (Chainstate* chainstate : GetAll()) {
+ BlockValidationState state;
+ if (!chainstate->ActivateBestChain(state, nullptr)) {
+ LOCK(GetMutex());
+ return util::Error{Untranslated(strprintf("%s Failed to connect best block (%s)", chainstate->ToString(), state.ToString()))};
+ }
+ }
+ return {};
+}
diff --git a/src/validation.h b/src/validation.h
index f330d90a..82fbf04d 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -1337,6 +1337,9 @@ public:
//! Get range of historical blocks to download.
std::optional<std::pair<const CBlockIndex*, const CBlockIndex*>> GetHistoricalBlockRange() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+ //! Call ActivateBestChain() on every chainstate.
+ util::Result<void> ActivateBestChains() LOCKS_EXCLUDED(::cs_main);
+
//! If, due to invalidation / reconsideration of blocks, the previous
//! best header is no longer valid / guaranteed to be the most-work
//! header in our block-index not known to be invalid, recalculate it.
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.