validation: Run CheckBlockIndex only on success for now #16453

1. 
   MarcoFalke commented at 7:51 pm on July 24, 2019: member

   To run CheckBlockIndex also on failure was changed in 613c46fe and uncovered the bug #16444.

   This pull restores the previous behavior, where CheckBlockIndex was only called on success (return true in https://github.com/bitcoin/bitcoin/blob/a6cba19831da9de6c5f968849d07c2a006557fe4/src/validation.cpp#L3440) and never on failure.

2. 
   jamesob commented at 8:01 pm on July 24, 2019: member

   ACK https://github.com/bitcoin/bitcoin/pull/16453/commits/fa219951be716c6e0e74c9fab823346ace3ac18b

   This restores the original behavior before #16194. Wonder why the failures we saw were intermittent, though.

3. 
   ryanofsky commented at 8:10 pm on July 24, 2019: member

   Is there a stack trace, or any clue why CheckBlockIndex is failing?

   utACK fa219951be716c6e0e74c9fab823346ace3ac18b. It does make sense to restore the previous behavior and remove the extra CheckBlockIndex call as a workaround.

4. MarcoFalke renamed this:
   validation: Run CheckBlockIndex only on success (Fixup)
   WIP: validation: Run CheckBlockIndex only on success (Fixup)
   on Jul 24, 2019
5. MarcoFalke added the label Brainstorming on Jul 24, 2019
6. MarcoFalke added the label Validation on Jul 24, 2019
7. MarcoFalke added this to the milestone 0.19.0 on Jul 24, 2019
8. 
   MarcoFalke commented at 8:14 pm on July 24, 2019: member

   Wonder why the failures we saw were intermittent, though.

   Yes, I don’t really like my change. Ideally CheckBlockIndex passes any time (even on failure).

9. 
   MarcoFalke commented at 9:05 pm on July 24, 2019: member

   Closing for now. Hoping someone else comes up with a better fix.
10. MarcoFalke closed this on Jul 24, 2019

    ---

11. MarcoFalke deleted the branch on Jul 24, 2019
12. fanquake added the label Up for grabs on Jul 25, 2019
13. MarcoFalke renamed this:
    WIP: validation: Run CheckBlockIndex only on success (Fixup)
    validation: Run CheckBlockIndex only on success for now
    on Aug 23, 2019
14. MarcoFalke removed the label Brainstorming on Aug 23, 2019
15. MarcoFalke removed the label Up for grabs on Aug 23, 2019
16. MarcoFalke restored the branch on Aug 23, 2019
17. 
    MarcoFalke commented at 10:41 pm on August 23, 2019: member

    A lot of the test runs are failing due to this and we are blindly resetting each failing test run, thus missing actual test failures. We need to either fix the underlying bug or restore the previous behavior.
18. MarcoFalke reopened this on Aug 23, 2019

    ---

19. 
    DrahtBot commented at 2:23 am on August 24, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
    • #16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

20. validation: Run CheckBlockIndex only on success. Fixup to 613c46fe fa5ad3e4aa
21. MarcoFalke force-pushed on Aug 24, 2019
22. in src/validation.cpp:3397 in fa5ad3e4aa

    3393 
    3394             if (!accepted) {
    3395                 if (first_invalid) *first_invalid = header;
    3396                 return false;
    3397             }
    3398+            ::ChainstateActive().CheckBlockIndex(chainparams.GetConsensus());
    

    ---

    ---

    

    ryanofsky commented at 5:53 pm on August 27, 2019:

    Maybe comment that check can fail if header is not accepted. Could quote or link to #16444 (comment)
23. ryanofsky approved
24. 
    ryanofsky commented at 5:55 pm on August 27, 2019: member

    utACK fa5ad3e4aac93535140ee0cf787f1b1ca4cd58d3. Control flow simplified a little since last review, just moving CheckBlockIndex calls after returns.
25. 
    jamesob commented at 7:18 pm on August 27, 2019: member

    ACK https://github.com/bitcoin/bitcoin/pull/16453/commits/fa5ad3e4aac93535140ee0cf787f1b1ca4cd58d3
26. 
    TheBlueMatt commented at 10:31 pm on September 4, 2019: member

    I’d really prefer we at least attempt to fix InvalidateBlock to fix the issue in #16444 (comment) before we jump to removing CheckBlockIndex. Ideally we’d capture this case in CheckBlockIndex directly.
27. 
    sdaftuar commented at 6:36 pm on September 10, 2019: member

    I’d really prefer we at least attempt to fix InvalidateBlock to fix the issue in #16444 (comment) before we jump to removing CheckBlockIndex. Ideally we’d capture this case in CheckBlockIndex directly.

    Agreed – please see #16849.

28. MarcoFalke closed this on Sep 15, 2019

    ---

29. MarcoFalke deleted the branch on Sep 15, 2019
30. laanwj referenced this in commit cd737214ce on Sep 16, 2019
31. sidhujag referenced this in commit f9a3ff0065 on Sep 16, 2019
32. gabriel-bjg referenced this in commit 75dca8b262 on Oct 26, 2021
33. vijaydasmp referenced this in commit c1060026d1 on Dec 6, 2021
34. vijaydasmp referenced this in commit 63b39a1f65 on Dec 10, 2021
35. vijaydasmp referenced this in commit 68a010f4ff on Dec 13, 2021
36. vijaydasmp referenced this in commit 3bc73a2abf on Dec 13, 2021
37. vijaydasmp referenced this in commit bff9273316 on Dec 15, 2021
38. DrahtBot locked this on Dec 16, 2021

Contributors
MarcoFalke jamesob ryanofsky DrahtBot TheBlueMatt sdaftuar

Labels
Validation

Milestone
0.19.0