blockstorage: Make m_block_index own CBlockIndex's
ClosedPublic
Actions

Authored by PiRK on Jan 18 2023, 11:18.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCf3788211baac: blockstorage: Make m_block_index own CBlockIndex's

Summary

Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.

A change to BlockManager::LookupBlockIndex:

Previously, it was a const member function returning a non-const CBlockIndex*
Now, there's are const and non-const versions of BlockManager::LookupBlockIndex returning a CBlockIndex with the same const-ness as the member function: (e.g. const CBlockIndex* LookupBlockIndex(...) const)

See next commit for some weirdness that this eliminates.

The range based for-loops are modernize (using auto + destructuring) in
a future commit.

This is a partial backport of core#24050
https://github.com/bitcoin/bitcoin/pull/24050/commits/bec86ae32683ac56b4e6ba9c9b7d21cfbdf4ac03
https://github.com/bitcoin/bitcoin/pull/24050/commits/c2a1655799c5d5dab9b14bd2a6b2d2296efd6964

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Jan 18 2023, 11:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 18 2023, 11:18

PiRK requested review of this revision.Jan 18 2023, 11:18

PiRK edited the summary of this revision. (Show Details)Jan 18 2023, 11:18

PiRK added a child revision: D12992: tests: Remove now-unnecessary manual Unload's.Jan 18 2023, 11:19

PiRK added inline comments.Jan 18 2023, 11:23

src/validation.cpp
3555 ↗	(On Diff #37565)	Removing the const here is necessary because inserting into `setBlockIndexCandidates` removes constness.

Harbormaster completed remote builds in B21768: Diff 37565.Jan 18 2023, 11:32

Fabien added a subscriber: Fabien.Jan 18 2023, 16:38

Fabien added inline comments.

src/node/blockstorage.h
165 ↗	(On Diff #37565)	I don't get why this has to be removed

Fabien requested changes to this revision.Jan 18 2023, 16:40

Fabien added inline comments.

src/validation.cpp
3555 ↗	(On Diff #37565)	I don't understand this explanation also. You should really use structured binding and remove this horrible `it` variable that isn't an iterator and the even worse `i` which is not a loop index. This code is completely breaking all the programmer habits and makes it almost impossible to read.

This revision now requires changes to proceed.Jan 18 2023, 16:40

I think the blockstorage unit deserves to be totally updated with structured bindings, this will make the code and especially this change much easier to review