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?

@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?

@qmma70 You can use the invalidateblock RPC for that.

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