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

4. 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
5. sdaftuar force-pushed on Sep 10, 2019
6. 
   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.

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

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

9. laanwj added this to the milestone 0.19.0 on Sep 26, 2019
10. laanwj added the label Bug on Sep 30, 2019
11. 
    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.

12. 
    fjahr commented at 5:06 PM on October 1, 2019: member

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

13. laanwj referenced this in commit 30c2b0b1cb on Oct 2, 2019
14. laanwj merged this on Oct 2, 2019
15. laanwj closed this on Oct 2, 2019

    ---

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

Contributors

sdaftuarjamesobTheBlueMattSjorsfjahr

Milestone
0.19.0