removeForReorg calls once-per-disconnect-> once-per-reorg #6656

Based on #6595

For those unaware, you can view the diff without whitespace changes by adding ?w=0 to the url.

utACK.

utACK

What do you think about moving the calls to mempool.removeForReorg() that are inside ActivateBestChainStep() to ActivateBestChain()? I think that would probably make things a bit cleaner.

@sdaftuar I'd feel bad calling removeForReorg and iterating over the mempool after each new block (instead of only if we actually called Disconnect). It would simplify the code some, but not much? I dunno, do you think its worth it?

It should be in Step, I think. In between the calls to Step cs_main is released, so the result is observable. I don't think we want to release cs_main while the mempool is in an inconsistent state.

I believe @sdaftuar meant to put the call before cs_main was released in ActivateBestChain so that you dont have to do it before every return statement.

Ah, no opinion.

Yeah that's what I had in mind, except my thought was to continue to use the blocksDisconnected bool to signal the caller it should make the call to mempool.removeForReorg() (ie pass in a new bool by reference and populate it in ActivateBestChainStep). It just seems less error prone for future changes to the code to not have to guard every place ActivateBestChainStep returns (and since ActivateBestChainStep asserts that the caller holds cs_main, so it doesn't seem inherently strange to do it in the caller).

@TheBlueMatt: this is what I had in mind: https://github.com/sdaftuar/bitcoin/commit/cc4b7b0959c8fe3e12337cb1bfd642ad7f042449

Eh, maybe it's just personal taste which formulation is better, you can take it or leave it. I'll think about additional testing.

Looks like this needs a rebase now that #6654 is merged.

This needs a more involved rebase, because it requires a more thourough checking that it is still correct. Closing for now, someone can come along and fix it again later.