Currently we use an unordered_map<uint256, CBlockIndex*> for our BlockMap. This is a bit of a peculiar choice because we then store a pointer to the uint256 from the pair inside of the CBlockIndex. It would be conceptually simpler if we used a unordered_map<CBlockIndex> and modified CBlockIndex to directly own the uint256 for the phashBlock.
That’s what this patch attempts doing. The only additional complexity is where we do want to query the map by key, the key equivalence stuff is only in C++20, so we add a helper that lets us construct a mock element easily for using with find.
The result is nicer since we get rid of an additional indirection for accessing the phashblock and we get rid of a lot of std::pair de-structuring. We also save a bit of memory per index (I haven’t computed it precisely, but my guess is we save something like 24 bytes total per index by eliminating 1 pointer, which saves 8 bytes, and then could save us something like 16 bytes of padding). We also no longer can be in an inconsistent state where phashblock does not point to the entries own hash (although modifying phashblock once it is inserted into the map would be invalid – it must be removed, modified, and reinserted).
Future work can likely further improve on this by making CBlockIndex owned* by the unordered_set directly, which will eliminate even more indirection/pointer chasing and make a lot of the code using BlockMap simpler (I don’t think we ever rely on CBlockIndex being non-owned anywhere, nor should we have reason to in the future).
- see https://en.cppreference.com/w/cpp/container/unordered_set, it will still be safe to use pointers for e.g., pprev.
0 References and pointers to data stored in the container are only invalidated by erasing that element, even when the corresponding iterator is invalidated.
