This is an alternative version of #5267
It works by having DisconnectTip() lookup the block at height current-COINBASE_MATURITY, and then removing any transactions spending that coinbase (recursively) from the memory pool.
Since re-orgs happen by repeated calls to DisconnectTip(), this will keep the memory pool always consistent, even in the middle of a re-org.
Builds on #5316
Includes a new regression test.
I worry about the increased block access, and the resulting 100 extra blocks that can’t be pruned.
Isn’t it easier to just allow immature spends in the mempool? Typically in a reorganization, after the disconnect, a reconnect follows that will make most of the removed transaction in the mempool valid again anyway.
Yes, there are several ways to fix this, and this one was discussed a bit. However, I’d highly prefer to hit coinsdb for this as inputs to mempool txn are likely to be cached anyway, which is preferable to hitting disk for a second block. (this also has potentially significant implications for pruning, as you now need to keep 100 extra blocks for any given reorg depth).
Also, could you include “Make CTxMemPool::check more thourough by using CheckInputs 3215e5a”? This should make the code much more robust in the future, though currently this branch fails those checks.
@sipa : a re-org happens on the main network on average once or twice a day. Extra block access is really NOT an issue.
RE: not being able to prune to less than 100+plausible-re-org-depth: I’m OK with 120 being the minimum number of blocks a pruning node needs to keep. If we REALLY want to support pruning down lower, then storing the hash of the coinbase transactions in the block indices would be sufficient.
RE: easier to allow immature in mempool: I really don’t like the idea of relaxing the invariant of “every transaction in the mempool is ready to be mined.” Certainly this code is simpler than that alternative. I also think we’ll have to optimize CreateBlock fairly soon, which will probably mean a mempool rewrite to maintain a priority queue of transactions– so creating a block is just “pop off queue until block full.” @TheBlueMatt : including more thorough CheckInputs is a good idea, but I don’t think I’ll have time to do that until Monday (out for Thanksgiving).
RE: run check() after reorgs are complete comment: theoretically, you can reorg onto a more-work chain that has a lower height. That will never happen on the main network, but I hate to say never and I think we should be as robust as possible at all the edge cases we can think up.
Idea for if we decide to care about resurrecting coinbase spends that are invalidated during a re-org but become valid when it is finished:
Move the coinbase spends to the orphan transaction list instead of dropping them. And add code when extending the tip to look for any “now mature” coinbase spends in mapOrphans, and move them to the mempool.
(but I still don’t think we need to care about this case)
Well, I definitely prefer having a fix than not having one, and pruning isn’t exactly a blocker right now.
Given that this also adds a test to detect the incorrect behaviour, I think we should merge it, and work on better solutions later.
re: minimum block count for pruning: you really need to have enough blocks to make human-timescale-fixes possible (ie I’d say a week of blocks, at least, ie 1008 blocks), and while an extra 100 blocks isnt a huge deal to me either, going from 1000 to 1100 is not a trivial difference, though I’m not sure who it’d effect practically.
Anyway, I’ll try to find some time this week to figure out why my pull fails your tests and your pull fails my tests :)
RE: requiring 100 MORE blocks when pruned:
I must be missing something, I don’t understand why this would be number pruned PLUS 100 more blocks, rather than just “at least 100 most-recent blocks plus plausible re-org depth must be maintained”).
Can @sipa or @TheBlueMatt explain why you’re thinking this means pruning needs 100 more blocks than it would without this?
Fixed off-by-one error found by @TheBlueMatt, and now includes his “Make CTxMemPool::check more thorough” commit.
And rebased.
This still leaves transactions in mempool that are potentially
invalid if the maturity period has been reorged out of, but at
least they're not missing inputs entirely.
utACK. I believe the code and solution here are correct, assuming the block 100 back can be correctly read.
I would still prefer a solution that does not rely on that assumption, but that can be done later.
1879+ int immatureHeight = pindexDelete->nHeight-COINBASE_MATURITY+1;
1880+ if (immatureHeight > 0)
1881+ {
1882+ const CBlockIndex* immatureIndex = pindexDelete->GetAncestor(immatureHeight);
1883+ CBlock immatureBlock;
1884+ if (ReadBlockFromDisk(immatureBlock, immatureIndex))
Very good question, the answer isn’t obvious to me.
return state.Abort() if block-100 couldn’t be read would be trivial, but it seems a shame to get stuck on a bad fork if you can’t read block-100 because your disk went bad. Although I suppose if your disk is going bad you want to know sooner rather than later.
A safe alternative would be to mempool.clear() if this read fails.
449+ uint256 hash = txToRemove.front();
450+ txToRemove.pop_front();
451+ if (!mapTx.count(hash))
452+ continue;
453+ const CTransaction& tx = mapTx[hash].GetTx();
454+ removed.push_front(tx);
The logic for removing/resurrecting transactions from the memory
pool was broken for spends of coinbase transactions; they
were not being removed from the mempool when they should.
Fixed by having DisconnectTip() remove any transactions from the memory
pool that are descendants of the was-mature-but-is-now-immature coinbase
transaction.
A future pull request could keep track of all those removed transactions
and try to resurrect them after re-organizing onto the new chain, but
I'm not sure it is worth writing that code for a case that never happens,
and, if it does happen, should correct itself (wallets should re-broadcast
transactions that don't end up in the chain).
ReadFromDisk(block-100) failure will now state.abort().
And picked the push_front versus back nit pointed out by @TheBlueMatt .