refactor: Move stuff to ChainstateManager #23785

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:2112-moveCMan changing 5 files +77 −74

MarcoFalke commented at 4:48 pm on December 15, 2021: member

Move globals or members of the wrong class to the right class.

Move FindForkInGlobalIndex from BlockManager to CChainState

The helper was moved in commit b026e318c39f59a06e29f1b25c7f577e01b25ccb,
which also mentioned that it could be moved to CChainState. So do that,
as the functionality is not block-storage related.

This also allows to drop one function argument.

fa3d62cf7b

Move AcceptBlockHeader to ChainstateManager
This is needed for the next commit.
fa47b5c100

Move m_failed_blocks to ChainstateManager

The member is unrelated to block storage (BlockManager). It is related
to validation.

Fix the confusion by moving it.

Can be reviewed with
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

facd2137ec

Move pindexBestInvalid to ChainstateManager
A private member is better than a global.
fab6d6b2d1
MarcoFalke commented at 4:48 pm on December 15, 2021: member

This is required for and split off of https://github.com/bitcoin/bitcoin/pull/23581
MarcoFalke renamed this:
~~2112 move c man~~
refactor: Move stuff to ChainstateManager
on Dec 15, 2021
MarcoFalke added the label Refactoring on Dec 15, 2021
in src/validation.h:911 in facd2137ec outdated
906+ * 907+ * Because we already walk m_block_index in height-order at startup, we go 908+ * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, 909+ * instead of putting things in this set. 910+ */ 911+ std::set<CBlockIndex*> m_failed_blocks;
naumenkogs commented at 9:20 am on December 16, 2021:

probably irrelevant, but would it make sense to make it unordered_set?

MarcoFalke commented at 1:26 pm on December 16, 2021:

Maybe as a separate pull? I want to keep this move-only refactoring and not change the behaviour/performance.
naumenkogs commented at 9:25 am on December 16, 2021: member

ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
Sjors commented at 10:44 am on December 16, 2021: member

ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
in src/validation.h:869 in fab6d6b2d1
865@@ -896,18 +866,53 @@ class ChainstateManager 866 //! by the background validation chainstate. 867 bool m_snapshot_validated{false}; 868 869+ CBlockIndex* m_best_invalid;
shaavan commented at 11:15 am on December 16, 2021:

I was just curious, though. How about naming this variable to:

0 CBlockIndex* m_best_invalid_index;
or to:

0 CBlockIndex* m_index_best_invalid;
Because:

This would help express the purpose of this variable more clearly.

Since this replaces pindexBestInvalid, it makes it more logical to name it this way.
Sjors commented at 12:38 pm on December 16, 2021:

Alternatively m_best_invalid_block_index, though that’s a bit long

MarcoFalke commented at 1:29 pm on December 16, 2021:

Personally I am not a fan of putting the type of the symbol in the name of the symbol.

For example, I think we prefer std::string name over std::string strName or std::string name_string.

Also, leaving as-is for now to not invalidate reviews.
shaavan approved
shaavan commented at 11:20 am on December 16, 2021: contributor

ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4

The refactoring looks clean, and it was easy to rationalize each change since you provided reasoning in the commit message itself. Thanks for that!
MarcoFalke merged this on Dec 16, 2021
MarcoFalke closed this on Dec 16, 2021
MarcoFalke deleted the branch on Dec 16, 2021
sidhujag referenced this in commit b0cd4fc5e3 on Dec 16, 2021
Fabcien referenced this in commit 8818fa59f8 on Nov 16, 2022
Fabcien referenced this in commit efb457b0ee on Nov 16, 2022
Fabcien referenced this in commit dedb536fd1 on Nov 16, 2022
Fabcien referenced this in commit cf55baaecd on Nov 16, 2022
DrahtBot locked this on Dec 16, 2022

Contributors
MarcoFalke naumenkogs Sjors shaavan

Labels
Refactoring