Fix removal of timelocked-txn from mempool during reorg

TheBlueMatt commented at 11:43 pm on August 27, 2015: member

Add failing test checking timelocked-txn removal during reorg 85871dd5a1

Fix removal of time-locked transactions during reorg 2276af1501

laanwj added the label Bug on Aug 31, 2015

laanwj added the label Mempool on Aug 31, 2015

Fix comment in removeForReorg b394d266de

in src/txmempool.cpp: in 2276af1501 outdated

156@@ -156,22 +157,26 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list<CTransaction>& rem
157     }
158 }
159 
160-void CTxMemPool::removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight)
161+void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight)
162 {
163     // Remove transactions spending a coinbase which are now immature

sdaftuar commented at 8:00 pm on September 3, 2015:

nit: update this comment?

TheBlueMatt commented at 4:40 am on September 6, 2015:

Done

dcousens commented at 1:05 pm on September 7, 2015: contributor

utACK, concept ACK.

sipa commented at 3:46 pm on September 7, 2015: member

Untested ACK.

sdaftuar commented at 2:40 pm on September 8, 2015: member

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.

sipa commented at 2:51 pm on September 8, 2015: member

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.

TheBlueMatt commented at 0:53 am on September 9, 2015: member

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.

in src/txmempool.cpp: in b394d266de

182+                std::map<uint256, CTxMemPoolEntry>::const_iterator it2 = mapTx.find(txin.prevout.hash);
183+                if (it2 != mapTx.end())
184+                    continue;
185+                const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash);
186+                if (fSanityCheck) assert(coins);
187+                if (!coins || (coins->IsCoinBase() && ((signed long)nMemPoolHeight) - coins->nHeight < COINBASE_MATURITY)) {

petertodd commented at 8:43 pm on September 9, 2015:

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.

petertodd commented at 8:46 pm on September 9, 2015:

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.

TheBlueMatt commented at 8:54 pm on September 9, 2015:

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

petertodd commented at 9:08 pm on September 9, 2015:

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.

TheBlueMatt commented at 9:18 pm on September 9, 2015:

No, it would be my fault, I think I wrote that code, too :p.

TheBlueMatt commented at 9:18 pm on September 9, 2015:

Why are redundant checks to make code more reviewable and more clearly not segfault-y bad?

petertodd commented at 9:26 pm on September 9, 2015:

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)

petertodd commented at 8:46 pm on September 9, 2015: contributor

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

TheBlueMatt commented at 7:00 pm on November 5, 2015: member

Rebased at #6915

TheBlueMatt closed this on Nov 5, 2015

luke-jr referenced this in commit 5ed69848b9 on Jan 9, 2016

luke-jr referenced this in commit cd59f95831 on Jan 9, 2016

luke-jr referenced this in commit 7f445a3695 on Jan 9, 2016

luke-jr referenced this in commit 2e052f4be8 on Jan 10, 2016

luke-jr referenced this in commit 45f5d23185 on Jan 10, 2016

MarcoFalke locked this on Sep 8, 2021

Fix removal of timelocked-txn from mempool during reorg #6595