The only important commit is “Only load BlockMan in BlockMan member functions”, everything else is all just small style changes.
Here’s the commit message, reproduced:
0This commit effectively splits the "load block index itself" logic from
1"derive Chainstate variables from loaded block index" logic.
2
3This means that BlockManager::LoadBlockIndex{,DB} will only load what's
4relevant to the BlockManager.
226 for (auto& [_, block_index] : m_block_index) {
227- vSortedByHeight.push_back(std::make_pair(block_index.nHeight, &block_index));
228+ vSortedByHeight.push_back(&block_index);
229 }
230- sort(vSortedByHeight.begin(), vSortedByHeight.end());
231+ sort(vSortedByHeight.begin(), vSortedByHeight.end(),
std::sort? If yes, maybe switch to that while touching
236 // Find start of assumed-valid region.
237 int first_assumed_valid_height = std::numeric_limits<int>::max();
238
239- for (const auto& [height, block] : vSortedByHeight) {
240- if (block->IsAssumedValid()) {
241+ for (CBlockIndex* pindex : vSortedByHeight) {
const right here in this commit to avoid a separate commit for a single keyword? Also, could keep the symbol name block to reduce the diff by one line :sweat_smile:
In commit “style-only: No need for std::pair for vSortedByHeight” (6741a6957a240e9a0cdbd73df95f247b449e7b72)
This is fine but I don’t really get it. Previous commit is renaming a pindex to block_index, this one is renaming a block variable to pindex. If these cleanups were consolidated into one commit, it’d be easier to see if the changes were consistent. (Also of the names I like simple block the best).
4090+ std::vector<CBlockIndex*> vSortedByHeight;
4091+ vSortedByHeight.reserve(m_blockman.m_block_index.size());
4092+ for (auto& [_, block_index] : m_blockman.m_block_index) {
4093+ vSortedByHeight.push_back(&block_index);
4094+ }
4095+ sort(vSortedByHeight.begin(), vSortedByHeight.end(),
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
::UnloadBlockIndex to BlockManager by dongcarl)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.
chainstate->setBlockIndexCandidates filling code out of block-manager to chainstate-manager. This seems like it makes sense because you would expect block manager just to deal with low level blocks and not know anything about chain states or validation. Would be good to have @jamesob concept ACK since he added most of the code that’s moving
4093+ vSortedByHeight.push_back(&block_index);
4094+ }
4095+ sort(vSortedByHeight.begin(), vSortedByHeight.end(),
4096+ [](const CBlockIndex* pa, const CBlockIndex* pb) {
4097+ return pa->nHeight < pb->nHeight;
4098+ });
In commit “Only load BlockMan in BlockMan member functions” (f5a20039a4a140679981943fa2e645ded687d14e)
IMO, it would be better to add a std::vector<CBlockIndex*> GetSortedByHeight(BlockManager&) or similar helper function instead of duplicating this code.
...since the height information in already in CBlockIndex* and we can
use an easy custom sorter.
This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
I strongly recommend reviewing with the following git-diff flags:
--color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change
...it's declared in blockstorage.h
...also use std::sort for clarity
Pushed af44e85b920055be4c3f724000a17f9d8d31d5ae to f865cf8ded2b2fbc82a6fbc41226d991909a6880
review ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 🗂
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK f865cf8ded2b2fbc82a6fbc41226d991909a6880 🗂
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUg0DAv/VZj7G6Rp0qriCmTSDHrk21I938vrPa39hBPwQ9GU3DpaKPoIth5mj2Vl
8jMREh59ZzqKKorCPa0RdcVaRyiVPTRup0EOSi0b9qwIQREr1pumfJb1Mir9HJtCY
9WbLA/sj1YGuXpIUvWAEUkKE56vpLVahWnSx5jd9QFO0DBWbfQ3iHWj/Soc5EBxEV
105pquK8ceeWyZxN7NKHWwKZegfjMmuPvmrr1HVfFB9qwjzYIsJk7Hv83PXbTHNzMa
11OBBxlGy6o/anN1w+iS6tflGCzmTgxXXWKROlm2pCDAgOJZi4FyM2vj/V/9XM4E4F
12RghIzqLltFmVJwvRV7x/o0Ahf3QHsez5TIl8JvFxVjBAwPP6WW2cbUQnXJRGrpho
13oIUVnBf/r5iZCImOGzCkslm5oBDmWHCGi2jscPcdIQAcQNrusIN2jy4OrWxVAYFw
14fjORBHiWF1TnQElKrfBurviwt+LO8SZFcjtTm7ZN4BGFFknCGgUBD9hqP9GB6mzt
15Tu78aGDd
16=BBgB
17-----END PGP SIGNATURE-----