Break when active block tip is higher than nStopAtHeight #13490

1. 
   qmma70 commented at 0:35 am on June 18, 2018: contributor

   This addresses issue #13477

   In ActivateBestChain, when the loop ends with the active chain’s tip higher than nStopAtHeight, keep disconnecting the tip until the height is exactly the same as nStopAtHeight.

2. rewind when active block tip is higher than stopatheight 58c583da3a
3. fanquake added the label Validation on Jun 18, 2018
4. fanquake requested review from sipa on Jun 18, 2018
5. fanquake requested review from sdaftuar on Jun 18, 2018
6. 
   sipa commented at 0:53 am on June 18, 2018: member

   This seems like a very roundabout way of fixing that bug. Can’t you just break the inner loop whenever the requested height is exceeded?
7. break inner loop if height exceeds nStopAtHeight 6a6a80c753
8. 
   qmma70 commented at 1:42 am on June 18, 2018: contributor

   @sipa Thanks for pointing that out. Fixed and tested.

   Also, would it be worth it to implement a “rewind” functionality so the block can be rewinded to the desired height, like my original commit did?

9. qmma70 renamed this:
   Rewind when active block tip is higher than nStopAtHeight
   Break when active block tip is higher than nStopAtHeight
   on Jun 18, 2018
10. 
    sipa commented at 1:45 am on June 18, 2018: member

    @qmma70 You can use the invalidateblock RPC for that.
11. 
    sdaftuar commented at 2:16 pm on June 18, 2018: member

    I’ll start by saying that I don’t really like the way -stopatheight works in the first place – it’s a useful feature but I don’t like that we clutter up consensus code with it.

    The issue reported in #13477 arises because when ABC invokes StartShutdown(), it can take a while before bitcoind exits, and in the meantime ProcessNewBlock() can continue to fire, resulting in one more block at a time being connected in ABC (since before this patch ABC only checks for the stopheight after connecting a block). However, there is no guarantee that this code would work, because it is possible that ABCStep() could connect multiple blocks, taking us past the stop-height.

    In practice, this is not currently an issue during IBD, because ABCStep will just connect one block at a time except in the case of a reorg. But we only do this to avoid holding cs_main too long; however it’s possible we could change this behavior in the future and therefore break the -stopatheight behavior again.

    So the most correct fix, I think, would involve embedding this into ABCStep… But I like spreading out the knowledge of this feature in multiple consensus functions even less than I like the potential bug with this… So I’d propose that we instead just (a) return at the top of ABCStep ABC() if our tip is at or past our stop-height (so not inside the inner loop at all), and (b) also add a comment explaining that this is not-guaranteed behavior, because ABCStep is allowed to take us past our stop height, even if it’s unlikely. That would help mitigate developer confusion at least at wondering how this code could be correct (since it technically isn’t).

12. qmma70 closed this on Jul 15, 2018

    ---

13. qmma70 deleted the branch on Jul 15, 2018
14. DrahtBot locked this on Sep 8, 2021

Contributors
qmma70 sipa sdaftuar

Review Requested
sipa sdaftuar

Labels
Validation