In commit “validation: have LoadBlockIndex account for snapshot use” (51e614fdbc6a23ade324e9bd01228bf97df9bc5d)
There can be multiple blocks at the same height in vSortedByHeight, right? If so, this behavior seems nondeterministic, because it’s sorted by (height, pointer address), so blocks at the assume valid height with pointer addresses less than the assume valid height will be added to setBlockIndexCandidates and blocks at the same height with greater pointer addresses will not by added to setBlockIndexCandidates.
Having this mutable bool variable in the for loop makes this loop more stateful and complicated anyway. I think maybe ideally the GetAssumeValid method should return a (hash, height, nchaintx) tuple instead of a (hash, nchaintx) pair. So the height would be known in advance and the code could look something like:
0const auto last_assumed_valid_block = chainman.GetLastAssumedValid();
1for (const auto& [height, pindex] : vSortedByHeight) {
2 // Update nChainTx the last assumed valid block
3 if (last_assumed_valid_block) {
4 const auto& [av_hash, av_height, av_chaintx] = *last_assumed_valid_block;
5 if (pindex->GetBlockHash() == av_hash) {
6 pindex->nChainTx = av_chaintx;
7 }
8 }
9 ....
10 // Add block to chain candidates lists, unless the block is after the assumed valid
11 // height and the chain is only supposed to contain assumed valid blocks (if it's a
12 // background chain supposed to download and validate them).
13 const int av_height = last_assumed_valid_block ? std::get<1>(*last_assumed_valid_block) : std::numeric_limits<int>::max();
14 if (!chainstate->OnlyContainsAssumedValidBlocks() || pindex->nHeight <= av_height) {
15 chainstate->setBlockIndexCandidates.insert(pindex);
16 }
17}
