validation: add missing insert to m_dirty_blockindex #27905

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

3. DrahtBot added the label Validation on Jun 16, 2023
4. 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
5. mzumsande force-pushed on Jun 16, 2023
6. TheCharlatan approved
7. 
   TheCharlatan commented at 10:47 am on June 20, 2023: contributor

   ACK e639364495a26bd67dd08998fc7ec400747f9a15
8. fanquake requested review from stickies-v on Jun 21, 2023
9. stickies-v approved
10. 
    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.

11. fanquake merged this on Jun 21, 2023
12. fanquake closed this on Jun 21, 2023

    ---

13. sidhujag referenced this in commit 7eb9eb247f on Jun 21, 2023
14. mzumsande deleted the branch on Jun 21, 2023
15. 
    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?
16. luke-jr referenced this in commit a4751de59d on Aug 16, 2023
17. 
    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…

Contributors
mzumsande DrahtBot TheCharlatan stickies-v Sjors

Labels
Validation