validation: default initialize and guard chainman members #24857

pull ajtowns wants to merge 1 commits into bitcoin:master from ajtowns:202204-best_invalid changing 3 files +6 −6

ajtowns commented at 10:12 PM on April 14, 2022: member

#23785 moved pindexBestInvalid from a global to become ChainstateManager.m_best_invalid but dropped the explcit initalization to nullptr. Replace that, and annotate it as guarded by cs_main.
validation: default initialize and guard chainman members fdd0ca541a
ajtowns added the label Needs backport (23.x) on Apr 14, 2022
DrahtBot commented at 9:12 AM on April 15, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24232 (assumeutxo: add init and completion logic by jamesob)
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.
MarcoFalke added this to the milestone 23.1 on Apr 15, 2022
MarcoFalke added the label Refactoring on Apr 15, 2022
MarcoFalke commented at 9:38 AM on April 15, 2022: member

Initialization is done by UnloadBlockIndex for all code paths at least as of current master. Not calling UnloadBlockIndex would be a bug, as it leaves some globals untouched, letting them point to potentially deleted memory.

So it looks like this is refactoring and not a bug fix or something that can be exploited?

ACK on the changes either way, I am just trying to figure out if this is a blocker for 23.0, as you tagged this for backport.
ajtowns removed the label Needs backport (23.x) on Apr 15, 2022
ajtowns commented at 9:55 PM on April 15, 2022: member

@MarcoFalke No, I think you're right and this only becomes a bug when the call to UnloadBlockIndex gets removed in #22564. cc @dongcarl -- maybe you want to just take ths into your PR
MarcoFalke removed this from the milestone 23.1 on Apr 16, 2022
dongcarl commented at 8:22 PM on April 16, 2022: member

@ajtowns Good point! Will do. I will also check in my PR that everything that's reset/initialized by the removed UnloadBlockIndex are explicitly initialized.
dongcarl commented at 5:25 PM on April 18, 2022: member

cherry-picked here! https://github.com/bitcoin/bitcoin/pull/22564/commits/f56f4fe7dbd705d61909c51b9ecde617a6f45dcb
ajtowns commented at 11:21 PM on April 18, 2022: member

See #22564
ajtowns closed this on Apr 18, 2022
DrahtBot locked this on Apr 18, 2023

Contributors

Labels

Linked (view graph)

#23785 refactor: Move stuff to ChainstateManager

validation: default initialize and guard chainman members #24857

Conflicts