not sure about this test. Seems fine to let reviewers know it exists, but the value of having to maintain this seems not worth it.
Overall, it seems racy (with various sleeps and large magic values), so while it may work today in the current CI, there is no expectation that it will continue to work in the future without additional work.
Going forward, I wonder if a better fix would be to simply require that the first block in the first block file is the genesis block.
The question is, whether there is any version or use-case out there that requires support for a missing genesis block as the first block in the first file during -reindex? I'd say no, because the other code doesn't assume so. At least, I presume it will break the // Activate the genesis block so normal node progress can continue feature: If the genesis block was never present or only present in the end, the node would wait in init for the whole duration of the -reindex.
For reference, one could come up with an alternative fix like this:
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 8726c741fa..2967802b6a 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -1278,4 +1278,6 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
ImportingNow imp{chainman.m_blockman.m_importing};
+ // Note that the genesis block is already written/loaded here.
+
// -reindex
if (!chainman.m_blockman.m_blockfiles_indexed) {
@@ -1305,6 +1307,4 @@ void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_
chainman.m_blockman.m_blockfiles_indexed = true;
LogInfo("Reindexing finished");
- // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
- chainman.ActiveChainstate().LoadGenesisBlock();
}
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 1725fe70bd..e46edf0eb4 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -60,8 +60,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
// At this point blocktree args are consistent with what's on disk.
- // If we're not mid-reindex (based on disk + args), add a genesis block on disk
- // (otherwise we use the one already on disk).
- // This is called again in ImportBlocks after the reindex completes.
- if (chainman.m_blockman.m_blockfiles_indexed && !chainman.ActiveChainstate().LoadGenesisBlock()) {
+ // Even in an ongoing -reindex (based on disk + args), force-write a
+ // genesis block if missing.
+ if (!chainman.ActiveChainstate().LoadGenesisBlock()) {
return {ChainstateLoadStatus::FAILURE, _("Error initializing block database")};
}
However, that breaks feature_reindex_readonly.py. So the fix would probably have to only write the genesis block if it isn't already present.
I mention it, because it seems easier to assume the genesis block always exists and is active as soon as init starts. (See also #34440#issue-3869652089: "Further ideas: Change CChain to always have at least one element (genesis), that way there is always genesis and tip.")
