validation: add missing insert to m_dirty_blockindex #27905

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202306_dirty_blockindex changing 1 files +1 −0

mzumsande commented at 9:03 PM on June 16, 2023: contributor

When the status of a block index is changed, we must add it to m_dirty_blockindex or the change might not get persisted to disk. This is missing from one spot in FindMostWorkChain(), where BLOCK_FAILED_CHILD is set. Since we have code that later sets missing BLOCK_FAILED_CHILD during the next startup, I don't think that this can lead to bad block indexes in practice, but I still think it's worth fixing.
DrahtBot commented at 9:04 PM on June 16, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK TheCharlatan, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Validation on Jun 16, 2023

Type	Reviewers
ACK	TheCharlatan, stickies-v

validation: add missing insert to m_dirty_blockindex

...in FindMostWorkChain(). Before this, it was possible that the change
to the block index wouldn't be persisted to disk.

e639364495

mzumsande force-pushed on Jun 16, 2023
TheCharlatan approved
TheCharlatan commented at 10:47 AM on June 20, 2023: contributor

ACK e639364495a26bd67dd08998fc7ec400747f9a15
fanquake requested review from stickies-v on Jun 21, 2023
stickies-v approved
stickies-v commented at 11:14 AM on June 21, 2023: contributor

ACK e639364495a26bd67dd08998fc7ec400747f9a15

Couldn't find any other instances of CBlockIndex::nStatus updates not being added to m_dirty_blockindex. As pointed out by mzumsande, I think indeed this should be corrected nicely upon startup since LoadBlockIndex() iterates from start->end, as opposed to the reverse end->start being done here.
fanquake merged this on Jun 21, 2023
fanquake closed this on Jun 21, 2023
sidhujag referenced this in commit 7eb9eb247f on Jun 21, 2023
mzumsande deleted the branch on Jun 21, 2023
Sjors commented at 2:02 PM on July 29, 2023: member

Should we make nStatus private and only allow writing to it through a helper method that marks the index entry dirty?
luke-jr referenced this in commit a4751de59d on Aug 16, 2023
mzumsande commented at 7:30 PM on August 24, 2023: contributor
Should we make nStatus private and only allow writing to it through a helper method that marks the index entry dirty?

Would be nice if we didn't have to remember, I think there would be a few complications:
- CBlockIndex is an object that can exist and be updateable independent from whether it's part of our block index database - I could imagine there are spots that update its members that don't require marking it dirty.
- Do we only want to mark nStatus private, and leave all other members public? If it's the latter, then it could become quite a big change...
bitcoin locked this on Aug 23, 2024

Contributors

Labels

validation: add missing insert to m_dirty_blockindex #27905

Reviews