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

1. 
   TheBlueMatt commented at 11:35 pm on September 9, 2015: member

   Based on #6595
2. Add failing test checking timelocked-txn removal during reorg 85871dd5a1
3. Fix removal of time-locked transactions during reorg 2276af1501
4. Fix comment in removeForReorg b394d266de
5. Make indentation in ActivateBestChainStep readable 49a09add82
6. removeForReorg calls once-per-disconnect-> once-per-reorg 66aea039a6
7. 
   dcousens commented at 2:45 am on September 10, 2015: contributor

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

   utACK.

8. 
   btcdrak commented at 9:18 am on September 10, 2015: contributor

   utACK
9. 
   sdaftuar commented at 1:42 pm on September 10, 2015: member

   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.
10. 
    TheBlueMatt commented at 11:24 pm on September 10, 2015: member

    @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?
11. 
    sipa commented at 11:28 pm on September 10, 2015: member

    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.
12. 
    TheBlueMatt commented at 11:31 pm on September 10, 2015: member

    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.
13. 
    sipa commented at 11:31 pm on September 10, 2015: member

    Ah, no opinion.
14. 
    sdaftuar commented at 0:43 am on September 11, 2015: member

    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).
15. 
    sdaftuar commented at 2:48 pm on September 11, 2015: member

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

16. 
    petertodd commented at 5:42 pm on September 21, 2015: contributor

    Looks like this needs a rebase now that #6654 is merged.
17. laanwj added the label Consensus on Sep 23, 2015
18. 
    TheBlueMatt commented at 0:26 am on September 26, 2015: member

    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.
19. TheBlueMatt closed this on Sep 26, 2015

    ---

20. MarcoFalke locked this on Sep 8, 2021

Contributors
TheBlueMatt dcousens btcdrak sdaftuar sipa petertodd

Labels
Consensus