Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validation: add missing insert to m_dirty_blockindex #27905

Merged
merged 1 commit into from Jun 21, 2023

Conversation

mzumsande
Copy link
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
Copy link
Contributor

DrahtBot commented Jun 16, 2023

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.

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

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e639364

@fanquake fanquake requested a review from stickies-v June 21, 2023 10:29
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e639364

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 fanquake merged commit d23cdf6 into bitcoin:master Jun 21, 2023
15 checks passed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 21, 2023
…ndex

apply to BLOCK_CONFLICT_CHAINLOCK state as well
@mzumsande mzumsande deleted the 202306_dirty_blockindex branch June 21, 2023 18:21
@Sjors
Copy link
Member

Sjors commented Jul 29, 2023

Should we make nStatus private and only allow writing to it through a helper method that marks the index entry dirty?

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
...in FindMostWorkChain(). Before this, it was possible that the change
to the block index wouldn't be persisted to disk.

Github-Pull: bitcoin#27905
Rebased-From: e639364
@mzumsande
Copy link
Contributor Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants