Fix block index inconsistency in InvalidateBlock() #16849

1. 
   sdaftuar commented at 6:28 PM on September 10, 2019: member

   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.

2. 
   sdaftuar commented at 6:30 PM on September 10, 2019: member

   See #16444 (comment) for additional background.

3. sdaftuar cross-referenced this on Sep 10, 2019 from issue validation: Run CheckBlockIndex only on success for now by MarcoFalke
4. in src/validation.cpp:2805 in bfd4fde79d outdated

   2800 | +            // multimap, because those candidates will be found and considered
   2801 | +            // as we disconnect.
   2802 | +            // Instead, consider only non-active-chain blocks that have at
   2803 | +            // least as much work as where we expect the new tip to end up.
   2804 | +            if (!m_chain.Contains(candidate) &&
   2805 | +                    !CBlockIndexWorkComparator()(candidate, pindex->pprev) &&
   

   ---

   ---

   

   jamesob commented at 6:41 PM on September 10, 2019:

   Won't this potentially omit candidates with equal work as pindex->pprev (depending on nSequenceId/pointer address)?

   ---

   

   sdaftuar commented at 7:00 PM on September 10, 2019:

   I believe this exactly matches the test in CheckBlockIndex(): https://github.com/bitcoin/bitcoin/blob/1985c4efda56b48f6f9c04f39d69268ee8f0b40a/src/validation.cpp#L4604

5. 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
6. sdaftuar force-pushed on Sep 10, 2019
7. 
   sdaftuar commented at 6:56 PM on September 10, 2019: member

   The only remaining case I've thought of that is unaddressed is if a new block were to arrive while InvalidateBlock() is running that (a) builds off a chain tip that is not being invalidated and (b) has less work than the current tip at the time the block is received, then we might still end up in a state that is temporarily inconsistent. I left in the old code that cleans up before returning as a belt-and-suspenders to ensure that this case is handled the same as before.

   As this is an essentially unobservable bug (outside of travis) to begin with, I'm not sure it would be worth fixing this any further beyond documenting the situation.

8. DrahtBot added the label Validation on Sep 10, 2019
9. 
   TheBlueMatt commented at 9:16 PM on September 11, 2019: contributor

   utACK 2a4e60b48261d3f0ec3d85f97af998ef989134e0. I also discovered another issue in InvalidateBlock while reviewing, see #16856.

10. TheBlueMatt cross-referenced this on Sep 11, 2019 from issue Do not allow descendants of BLOCK_FAILED_VALID blocks to be BLOCK_FAILED_VALID by TheBlueMatt
11. laanwj added this to the milestone 0.19.0 on Sep 26, 2019
12. laanwj added the label Bug on Sep 30, 2019
13. 
    Sjors commented at 1:19 PM on September 30, 2019: member

    ACK 2a4e60b. Tested on top of #16899. Also tested invalidateblock with -checkblockindex=1.

    The changed code is only called by the invalidateblock RPC.

    Nit: maybe mention InvalidateBlock() as well in validation.h comment for CCriticalSection m_cs_chainstate.

14. 
    fjahr commented at 5:06 PM on October 1, 2019: contributor

    ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing invalidateblock.

15. laanwj cross-referenced this on Oct 2, 2019 from issue Release process for 0.19.0 by laanwj
16. laanwj referenced this in commit 30c2b0b1cb on Oct 2, 2019
17. laanwj merged this on Oct 2, 2019
18. laanwj closed this on Oct 2, 2019

    ---

19. sidhujag referenced this in commit a7b114133b on Oct 2, 2019
20. jasonbcox referenced this in commit 85fc9dfe0a on Aug 18, 2020
21. jasonbcox referenced this in commit 67319370e3 on Aug 18, 2020
22. UdjinM6 referenced this in commit fba2a546f6 on Oct 29, 2021
23. pravblockc referenced this in commit 37d1b95013 on Nov 18, 2021
24. bitcoin locked this on Dec 16, 2021

Contributors

sdaftuarjamesobTheBlueMattSjorsfjahr

Milestone
0.19.0