Do not allow descendants of BLOCK_FAILED_VALID blocks to be BLOCK_FAILED_VALID #16856

1. 
   TheBlueMatt commented at 9:15 pm on September 11, 2019: member

   Based on #16849, this was intended primarily to fix n invalid-direction comparison (see the first commit) but discovered there were further issues preventing the addition of a new rule in CheckBlockIndex. We can either go this way, or just delete the code that attempts (incorrectly) to mark blocks BLOCK_FAILED_CHILD in InvalidateBlock and not require this as an invariant (I don’t have a strong opinion here, just PR’ing this as I wrote it during testing and figured I’d demonstrate).
2. Fix block index inconsistency in InvalidateBlock()

   Previously, we could release cs_main while leaving the block index in a state
   that would fail CheckBlockIndex, because setBlockIndexCandidates was not being
   fully populated before releasing cs_main.

   2a4e60b482
3. Fix invalid_walk_tip comparison in InvalidateBlock 2075bd7774
4. Clarify the final |= BLOCK_FAILED_VALID in InvalidateBlock

   This has no functional affect, as the any CBlockIndex*s which
   to_mark_failed is set to will already have been marked failed.

   f043ca0100
5. Add a checkfor the previous invalid comparison of invalid_walk_tip

   This adds a new test in CheckBlockIndex which tests that any
   descendants of a block marked BLOCK_FAILED_VALID will not be marked
   BLOCK_FAILED_VALID as they should never have been connected in the
   first place.
   
   Note that this check requires further changes in InvalidateBlock as
   if you iteratively call InvalidateBlock to walk the chain backwards
   (instead of calling InvalidateBlock on an old block and allowing it
   to do the walking), the later-in-the-chain blocks will violate this.

   0ae728197c
6. DrahtBot added the label Validation on Sep 11, 2019
7. in src/validation.cpp:2851 in 0ae728197c

   2847@@ -2813,33 +2848,62 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
   2848         setDirtyBlockIndex.insert(invalid_walk_tip);
   2849         setBlockIndexCandidates.erase(invalid_walk_tip);
   2850         setBlockIndexCandidates.insert(invalid_walk_tip->pprev);
   2851-        if (invalid_walk_tip->pprev == to_mark_failed && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) {
   2852+        if (invalid_walk_tip == to_mark_failed->pprev && (to_mark_failed->nStatus & BLOCK_FAILED_VALID)) {
   

   ---

   ---

   

   Sjors commented at 11:35 am on September 30, 2019:

   I’m confused by this fix. Let’s say we called invalidateblock on block 10. In that case to_mark_failed is block 10, to_mark_failed->pprev is block 9. Let’s say we’re almost done and we just disconnected the tip at height 11. Then invalid_walk_tip is block 11. So this comparison always returns false?

   ---

   

   TheBlueMatt commented at 2:57 am on October 3, 2019:

   If we’re almost done, to_mark_failed got set to block 12 in the last iteration of the loop (yes, these variable names make no sense).

   ---

   

   Sjors commented at 4:24 pm on October 3, 2019:

   Ah, it’s actually pindex that stays at block 10 throughout this function. to_mark_failed is set to invalid_walk_tip. By the time we reach this line we chipped one block off of the tip.

   Looking at the first iteration: let’s say the original tip is block 20. to_mark_failed starts at block 10. We disconnect block 20 and mark it BLOCK_FAILED_VALID. Theif statement here returns false, so block 20 temporarily remains BLOCK_FAILED_VALID. The second iteration to_mark_failed points to block 20, so then we set it to BLOCK_FAILED_CHILD.

   Do I understand correctly that the reason for this “we fix it the next iteration” is because we might get shut down? So we need to wait with BLOCK_FAILED_CHILD until we’re sure the parent is BLOCK_FAILED_VALID .

   ---

   

   TheBlueMatt commented at 4:42 pm on October 3, 2019:

   Right, I mean its not strictly because we want to be safe in case of shutdown (though that’s nice), and more that this hunk previously never did anything at all (though its intent was “descendants get marked FAILED_CHILD”).
8. laanwj added the label Bug on Sep 30, 2019
9. 
   Sjors commented at 12:08 pm on September 30, 2019: member

   Looking at the pindexFirstInvalid check in isolation (0ae7281), that makes sense to me. If I drop everything except that check, then a whole bunch of functional tests throw an assert: ((pindex->nStatus & BLOCK_FAILED_VALID) == 0), function CheckBlockIndex, file validation.cpp, line 4669.. But I don’t understand the fix itself, at least not the bit above my inline comment.
10. laanwj referenced this in commit 30c2b0b1cb on Oct 2, 2019
11. sidhujag referenced this in commit a7b114133b on Oct 2, 2019
12. 
    jnewbery commented at 4:25 pm on December 13, 2019: member

    Can you give a bit of motivation for why we want to mark descendants of BLOCK_FAILED_VALID as BLOCK_FAILED_CHILD? I can’t see any functional difference in the way that a CBlockIndex is treated when its nStatus is BLOCK_FAILED_CHILD. The only comparisons seem to be against BLOCK_FAILED_MASK.

    If that’s the case, then I think we should just mark all descendants of the block to invalidate as BLOCK_FAILED_VALID. If reconsiderblock is later called, the nStatus of the block and all its descendants are reset anyway. It seems to me that InvalidateBlock() would be much simpler if we did that.

13. 
    TheBlueMatt commented at 10:14 pm on December 13, 2019: member

    Right, I don’t think we really use it anywhere, I just figured as long as we make the distinction, make the distinction. Maybe @sipa recalls why it was there in the first place?
14. 
    sipa commented at 10:38 pm on December 13, 2019: member

    I don’t recall exactly why they’re separate. One point is that BLOCK_FAILED_CHILD doesn’t actually need to be stored on disk, as it is recomputed at db load time. So an advantage of keeping them separate is that if only FAILED_CHILD gets set on a blockindex object, it doesn’t need to be made dirty and written to disk. I’m not sure that optimization is ever taken advantage of (and it probably can be achieved in other ways too).
15. 
    jnewbery commented at 11:05 pm on December 13, 2019: member

    I’m not sure that optimization is ever taken advantage of

    Looks to me like it isn’t used. I’d vote to just use BLOCK_FAILED_VALID and keep InvalidateBlock() simple.

16. 
    jnewbery commented at 8:16 pm on December 20, 2019: member

    Alternative here https://github.com/jnewbery/bitcoin/tree/2019-12-block-failed-valid-descendants just removes tracking of BLOCK_FAILED_CHILD. Let me know what you think, @TheBlueMatt .
17. fanquake deleted a comment on Dec 21, 2019
18. 
    BrannonKing commented at 3:35 am on September 6, 2020: none

    @jnewbery , that branch doesn’t remove all references to BLOCK_FAILED_CHILD. Was the intent to remove them all?
19. 
    jnewbery commented at 8:27 am on September 6, 2020: member

    @BrannonKing - The intent was to demonstrate how removing BLOCK_FAILED_CHILD from InvalidateBlock() makes it simpler (and therefore less likely to hide bugs). I agree that we should remove BLOCK_FAILED_CHILD everywhere.
20. 
    jnewbery commented at 10:06 am on October 1, 2020: member

    @BrannonKing - I’ve rebased my branch on master and removed all the remaining instances of BLOCK_FAILED_CHILD. @TheBlueMatt - This PR hasn’t had any activity for about a year. Are you happy to close it?
21. TheBlueMatt closed this on Oct 28, 2020

    ---

22. UdjinM6 referenced this in commit fba2a546f6 on Oct 29, 2021
23. pravblockc referenced this in commit 37d1b95013 on Nov 18, 2021
24. DrahtBot locked this on Feb 15, 2022

Contributors
TheBlueMatt Sjors jnewbery sipa BrannonKing

Labels
Bug Validation