Only call NotifyBlockTip when chainActive changes #12431

1. 
   jamesob commented at 3:48 am on February 14, 2018: member

   This is a subset of the more controversial #12407, but this also adds a test demonstrating the bug.

   In InvalidateBlock, we’re calling NotifyBlockTip with the now-invalid block’s prev regardless of what chain the ancestor block is on. This could create numerous issues, but it at least screws up waitforblockheight (or anything else relying on rpc/blockchain.cpp:latestblock) when InvalidateBlock is called on a block not in chainActive, which can happen via RPC.

   Only call NotifyBlockTip when the block being marked invalid is on the active chain.

2. jamesob force-pushed on Feb 14, 2018
3. jamesob force-pushed on Feb 14, 2018
4. fanquake added the label Validation on Feb 14, 2018
5. [tests] Add a (failing) test for waitforblockheight

   Demonstrates the presence of a bug in in `validation.cpp:InvalidateBlock`
   which will update `rpc/blockchain.cpp:latestblock` erroneously.

   152b7fb25f
6. Only call NotifyBlockTip when the active chain changes

   Previously, if `invalidateblock` was called on a block in a branch,
   NotifyBlockTip would be called on that block's predecessor, creating an
   incorrect `rpc/blockchain.cpp:latestblock` value.
   
   Only call NotifyBlockTip if the chain being modified is activeChain.

   f98b543522
7. in test/functional/rpc_blockchain.py:284 in 83545659ea outdated

   277+                node.waitforblockheight(height, timeout)['height'],
   278+                current_height)
   279+
   280+        assert_waitforheight(0)
   281+        assert_waitforheight(current_height - 3)
   282+        assert_waitforheight(current_height)
   

   ---

   ---

   

   conscott commented at 7:05 pm on February 15, 2018:

   Might also want to add a timeout case

   ---

   

   jamesob commented at 4:52 pm on February 16, 2018:

   Played around with doing this, but found that the return from RPC is (unfortunately) no different in the timeout case, meaning we’d have to spin up a separate thread in-test, run a timer, and then assert that the RPC thread hadn’t yet stopped execution – probably overkill.

   ---

   

   jamesob commented at 4:53 pm on February 16, 2018:

   I’ve added a testcase for current_height + 1, though.
8. jamesob force-pushed on Feb 16, 2018
9. 
   jamesob commented at 4:53 pm on February 16, 2018: member

   Thanks for the look, @conscott. Rebased.
10. 
    conscott commented at 4:57 pm on February 16, 2018: contributor

    utACK f98b543522687a4ff93ad7d53fa3cc67b5c6d752
11. MarcoFalke requested review from MarcoFalke on Feb 16, 2018
12. 
    ryanofsky commented at 7:14 pm on March 6, 2018: member

    @jamesob, I’m curious how this might interact with #11856 with gets rid of the uiInterface.NotifyBlockTip signal, replacing it with CValidationInterface::UpdatedBlockTip listeners.

    EDIT: Looking into this more, this bugfix makes sense in combination with #11856, and would prevent spurious UpdatedBlockTip notifications in that PR.

13. 
    ryanofsky commented at 8:22 pm on March 6, 2018: member

    utACK f98b543522687a4ff93ad7d53fa3cc67b5c6d752. This fixes a pretty clear waitforblockheight bug and adds a nice test case.
14. 
    sdaftuar commented at 2:02 pm on March 7, 2018: member

    utACK f98b543522687a4ff93ad7d53fa3cc67b5c6d752
15. fanquake requested review from TheBlueMatt on Mar 14, 2018
16. 
    ryanofsky commented at 3:33 pm on March 15, 2018: member

    @TheBlueMatt, this is a simple, one line change fixing the NotifyBlockTip notification in InvalidateBlock. Could you confirm that it looks ok and doesn’t mess up your plans for #11856?
17. 
    TheBlueMatt commented at 3:41 pm on March 15, 2018: member

    Without thinking about it too hard, this seems fine. I’d obviously like to move towards getting rid of uiInterface calls in validation, ala #11856, but this should likely be applied there as well.
18. 
    ryanofsky commented at 3:53 pm on March 15, 2018: member

    @laanwj, I think this looks good to merge
19. laanwj merged this on Mar 15, 2018
20. laanwj closed this on Mar 15, 2018

    ---

21. laanwj referenced this in commit 947c25ead2 on Mar 15, 2018
22. PastaPastaPasta referenced this in commit ace8fb8abc on Jun 10, 2020
23. PastaPastaPasta referenced this in commit 36523d13df on Jun 13, 2020
24. PastaPastaPasta referenced this in commit 62e36f39b3 on Jun 13, 2020
25. PastaPastaPasta referenced this in commit a22d90420c on Jun 13, 2020
26. PastaPastaPasta referenced this in commit c1f34f70cd on Jun 17, 2020
27. MarcoFalke locked this on Sep 8, 2021

Contributors
jamesob conscott ryanofsky sdaftuar TheBlueMatt

Review Requested
TheBlueMatt MarcoFalke

Labels
Validation