refactor: Move LoadGenesisBlock to ChainstateManager #35673

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2607-LoadGenesisBlock-mv changing 6 files +17 −16

maflcko commented at 8:49 AM on July 7, 2026: member

The function does not need anything from any chainstate, so it should not sit in the Chainstate class.
DrahtBot added the label Refactoring on Jul 7, 2026
DrahtBot commented at 8:49 AM on July 7, 2026: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35673.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK l0rinc, janb84, sedited

Stale ACK ismaelsadeeq, stickies-v, yuvicc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
- #26022 (Add util::ResultPtr class by ryanofsky)
- #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
- #25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Type	Reviewers
ACK	l0rinc, janb84, sedited
Stale ACK	ismaelsadeeq, stickies-v, yuvicc

in src/validation.cpp:4950 in fa202e628f

4947 | +bool ChainstateManager::LoadGenesisBlock()
4948 |  {
4949 |      LOCK(cs_main);
4950 |  
4951 | -    const CChainParams& params{m_chainman.GetParams()};
4952 | +    const CBlock& block{GetParams().GenesisBlock()};

sedited commented at 9:43 AM on July 7, 2026:

Nit: Maybe rename this to genesis_block for clarity?

sedited approved
sedited commented at 9:43 AM on July 7, 2026: contributor

Nice, ACK fa202e628f47771c0ebfa725038c59cfeaf690d2

I have the same commit lying on my chainman/chainstate split branch: https://github.com/sedited/bitcoin/commit/d8a0306ad8b5b99948ce618d58a3f1081e6ddd5d
ismaelsadeeq approved
ismaelsadeeq commented at 9:44 AM on July 7, 2026: member

ACK fa202e628f47771c0ebfa725038c59cfeaf690d2
maflcko force-pushed on Jul 7, 2026
sedited approved
sedited commented at 12:40 PM on July 7, 2026: contributor

ACK fa28a7b7a6ec00312ccc55e8b6cf24a918610ef2
DrahtBot requested review from ismaelsadeeq on Jul 7, 2026
ismaelsadeeq commented at 12:50 PM on July 7, 2026: member

reACK fa28a7b7a6ec00312ccc55e8b6cf24a918610ef2

Minor rename changes in the last push.

in src/validation.cpp:4953 in fa28a7b7a6

4950 |  
4951 | -    const CChainParams& params{m_chainman.GetParams()};
4952 | +    const CBlock& genesis_block{GetParams().GenesisBlock()};
4953 |  
4954 |      // Check whether we're already initialized by checking for genesis in
4955 |      // m_blockman.m_block_index. Note that we can't use m_chain here, since it is

stickies-v commented at 2:23 PM on July 7, 2026:

nit: m_chain is not a member of ChainstateManager, so this comment is stale.

    // m_blockman.m_block_index. Note that we can't use the chain tip here, since it is

in src/validation.h:1087 in fa28a7b7a6
```
1082 | @@ -1085,6 +1083,9 @@ class ChainstateManager
1083 |      //! coins databases. This will be split somehow across chainstates.
1084 |      size_t m_total_coinsdb_cache{0};
1085 |  
1086 | +    /// Ensures a genesis block is in the block tree, possibly writing one to disk.
1087 | +    bool LoadGenesisBlock();
```
stickies-v commented at 2:27 PM on July 7, 2026:

pre-existing: this should probably be [[nodiscard]], and we have one callsite in ImportBlocks that ignores the return value. It's already LogError'd and there's not much more we can do with ImportBlocks currently returning void, but might be worth updating and returning early (or casting to void), so future users don't silently ignore errors?

yuvicc commented at 5:07 PM on July 7, 2026:

Looks like a behavior change, could be done in a follow-up?

maflcko commented at 5:14 PM on July 7, 2026:

Heh, I'd say more important than any logging and bool-return or early-return is probably the calling the fatal error function. ImportBlocks already does that correctly, so ignoring the return value seems correct here.

Also, added the [[nodiscard]] here.

Also, fixed up the __func__ stuff, while pushing.

[Edit: So I'd even be fine with making this function void (assuming that any fatal error results in an interrupt+shutdown anyway), but this may be too spicy for this pull request here and maybe also not worth the review.]
stickies-v approved
stickies-v commented at 2:27 PM on July 7, 2026: contributor

ACK fa28a7b7a6ec00312ccc55e8b6cf24a918610ef2
janb84 commented at 3:28 PM on July 7, 2026: contributor

ACK fa28a7b7a6ec00312ccc55e8b6cf24a918610ef2

Nice cleanup. LoadGenesisBlock never used any Chainstate member, it is pure ChainstateManager state.

Simple grep confirmed no missed callers.
yuvicc commented at 5:06 PM on July 7, 2026: contributor

ACK fa28a7b7a6ec00312ccc55e8b6cf24a918610ef2

refactor: Move LoadGenesisBlock to ChainstateManager

The function does not need anything from any chainstate, so it should
not sit in the Chainstate class.

Also, mark it [[nodiscard]], and the one place that ignores the return
value with (void).

Also, change the error log strings to not include the __func__, which is
redundant with -logsourcelocations. This is not a refactor, but this log
is only for debugging extremely rare errors.

fa615bd163

maflcko force-pushed on Jul 7, 2026
l0rinc commented at 10:00 PM on July 7, 2026: contributor

Tested ACK fa615bd163ac74c11a8e15ed8513a5b81ed9ef3b

I rebased this locally and generated a few throw-away unit/functional checks to sanity-check the move (covering fresh/already-loaded genesis, failure paths, and startup/reindex/loadblock behavior).
DrahtBot requested review from yuvicc on Jul 7, 2026
DrahtBot requested review from sedited on Jul 7, 2026
DrahtBot requested review from janb84 on Jul 7, 2026
DrahtBot requested review from stickies-v on Jul 7, 2026
DrahtBot requested review from ismaelsadeeq on Jul 7, 2026
janb84 commented at 7:10 AM on July 8, 2026: contributor
reACK fa615bd163ac74c11a8e15ed8513a5b81ed9ef3b

changes since last ACK:
- fixed func in logging
- added [[nodiscard]] attribute to func. to increase safety.
sedited approved
sedited commented at 8:00 AM on July 8, 2026: contributor

ACK fa615bd163ac74c11a8e15ed8513a5b81ed9ef3b
sedited merged this on Jul 8, 2026
sedited closed this on Jul 8, 2026
maflcko deleted the branch on Jul 8, 2026
stickies-v commented at 7:18 PM on July 8, 2026: contributor

post merge ACK fa615bd163ac74c11a8e15ed8513a5b81ed9ef3b

Contributors

Review Requested
ismaelsadeeq stickies-v yuvicc

Labels

Refactoring

refactor: Move LoadGenesisBlock to ChainstateManager #35673

Code Coverage & Benchmarks

Reviews

Conflicts