ResetBlockFailureFlags did not remove the invalidity flag in other chain #21086

1. 
   ghost commented at 12:59 pm on February 5, 2021: none

   I think that I may have found a bug in ResetBlockFailureFlags. The bug happens when there are two chains (say two blocks with blockheight 200) and a block before that fork will be invalidated (say blockheight 100). If you then use ResetBlockFailureFlags to remove the failure flags from one of the two blocks at block height 200. ResetBlockFailureFlags will remove the flags of all descendants and ancestors of the block 200 on one of the chains. But for the block number 200 on the other chain and the following blocks the flags will not be removed. The BLOCK_FAILED_CHILD flags remain for these blocks. But the BLOCK_FAILED_VALID flag of block 100 will be removed. This can then produce a problem in CheckBlockIndex on line

   assert((pindex->nStatus & BLOCK_FAILED_MASK) == 0); // The failed mask cannot be set for blocks without invalid parents.

   My fix searches for a BLOCK_FAILED_VALID flag and remembers the block index. It will then call ResetBlockFailureFlags for this block index again to remove the flags all all descendants.

2. ResetBlockFailureFlags did not remove the invalidity flag in other chain fe55621f49
3. fanquake added the label Validation on Feb 5, 2021
4. in src/validation.cpp:3112 in fe55621f49 outdated

   3107             m_blockman.m_failed_blocks.erase(pindex);
   3108         }
   3109         pindex = pindex->pprev;
   3110     }
   3111+	
   3112+	//Remove the invalidity flag from all descendants of the first invalid ancestor block
   

   ---

   ---

   

   steevo87 commented at 11:31 am on February 6, 2021:

   Changing tab to spaces:

   0   //Remove the invalidity flag from all descendants of the first invalid ancestor block
   

5. in src/validation.cpp:3097 in fe55621f49 outdated

   3092@@ -3093,16 +3093,26 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
   3093         }
   3094         it++;
   3095     }
   3096+	
   3097+	CBlockIndex *pindexFirstInvalid = nullptr;
   

   ---

   ---

   

   steevo87 commented at 11:32 am on February 6, 2021:

   Changing tab to spaces:

   0   CBlockIndex *pindexFirstInvalid = nullptr;
   

6. Update src/validation.cpp

   Co-authored-by: Stephen Vidas <stephen.vidas@gmail.com>

   4ea2b3a054
7. Update src/validation.cpp

   Co-authored-by: Stephen Vidas <stephen.vidas@gmail.com>

   aaf293d740
8. Update src/validation.cpp

   Co-authored-by: Stephen Vidas <stephen.vidas@gmail.com>

   6821683a86
9. in src/validation.cpp:3104 in fe55621f49 outdated

   3099     // Remove the invalidity flag from all ancestors too.
   3100     while (pindex != nullptr) {
   3101         if (pindex->nStatus & BLOCK_FAILED_MASK) {
   3102+			if (pindex->nStatus & BLOCK_FAILED_VALID) {
   3103+				pindexFirstInvalid = pindex;
   3104+			}
   

   ---

   ---

   

   steevo87 commented at 11:34 am on February 6, 2021:

   Changing tabs to spaces:

   0         if (pindex->nStatus & BLOCK_FAILED_VALID) {
   1            pindexFirstInvalid = pindex;
   2         }
   

10. 
    DrahtBot commented at 4:50 pm on March 15, 2021: contributor

    🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

11. 
    MarcoFalke commented at 6:24 pm on March 15, 2021: member

    I haven’t looked at the diff, but it would be nice to have a test that fails before the changes and passes after.
12. fanquake added the label Up for grabs on Oct 2, 2022
13. 
    fanquake commented at 2:44 pm on October 2, 2022: member

    Closing for now (no follow up, dead GH account), and marking up for grabs. As mentioned, a test showing the bug would also be a good place to start.
14. fanquake closed this on Oct 2, 2022

    ---

15. bitcoin locked this on Oct 2, 2023

Contributors
ghost steevo87 DrahtBot MarcoFalke fanquake

Labels
Validation Up for grabs