Fix removal of timelocked-txn from mempool during reorg #6595
Conversation
src/txmempool.cpp
Outdated
|
utACK, concept ACK. |
|
Untested ACK. |
|
ACK. FYI I think that it's worth thinking about this code again if we consider merging #6216, because I believe with the anti-fee sniping use of nlocktime we may end up purging a lot of transactions from the mempool whenever there is a reorg. |
|
Longer term, I think it's better to change the invariant for the mempool
from "Valid for the expected next block" to "will be eventually valid".
Accepting non-final transactions then becomes a policy question, rather
than a consistency requirement. It would also speed up block disconnects,
which is a good thing as reorganizations are already slower than normal
block connects - they shouldn't need this extra cleanup work on top.
|
|
I'm not too worried about changing the mempool invariant...mostly because I think (someone should audit to be sure), that this code would work as-is if it were just called once at the end of a reorg instead of after each bock disconnect. |
There was a problem hiding this comment.
To be clear, the reason why you test both !coins and against COINBASE_MATURITY is because AccessCoins() will return a CCoins result for immature coins, right?
I smell a future refactoring, but for now please add a comment describing those two cases and why there are two tests.
There was a problem hiding this comment.
Oh, and come to think of it your test above doesn't actually test both those cases; you need to do a >COINBASE_MATURITY reorg as well, to a chain that is equally long.
There was a problem hiding this comment.
Hmm? I didnt change this code at all, only its indentation. And, yes, I know the test-case is not complete, but creating a testcase that is complete is kinda a PITA (or, which case are you claiming should be implemented?)
There was a problem hiding this comment.
Sorry, arguably that's not your fault, agreed. :)
I'm really thinking of the case where we change this to be called end-of-reorg; if that isn't the case in theory this !coins check should be redundant.
Might be reasonable to leave this code as-is, and make a separate end-of-reorg function that just removes non-final transactions from the mempool.
There was a problem hiding this comment.
No, it would be my fault, I think I wrote that code, too :p.
There was a problem hiding this comment.
Why are redundant checks to make code more reviewable and more clearly not segfault-y bad?
There was a problem hiding this comment.
They're not bad! I just think it'd help if they were explained with comments. (and testing all ways of invoking them is always good)
|
@sipa I was the one who added that invariant in the first place, and I only meant it as a DoS attack defence, not as a true invariant. In fact, IIRC my original proposal was that we'd allow txs into the mempool so long as they'd be valid in the next two or three blocks. As for calling it at the end of a reorg, that looks fine to me modulo issues raised in my comments. |
|
Rebased at #6915 |
Github-Pull: bitcoin#6595 Rebased-From: 85871dd
Github-Pull: bitcoin#6595 Rebased-From: 2276af1
Github-Pull: bitcoin#6595 Rebased-From: b394d26
Github-Pull: bitcoin#6595 Rebased-From: 2276af1
Github-Pull: bitcoin#6595 Rebased-From: b394d26
6 participants
No description provided.