Move the mempool lock into CCoinsViewMemPool, since the existing code always manually locks the mempool (with the lock having same lifetime as the view!).
Move a call to CMempool::pruneSpent into CCoinsViewMemPool::GetCoins,
since CCoinsViewMemPool::GetCoins is called only once in the codebase,
followed by a call to pruneSpent with a TODO suggesting it be moved.
Move the mempool lock into CCoinsViewMemPool, since the existing code
always manually locks the mempool (with the lock having same lifetime
as the view!).
Move a call to CMempool::pruneSpent into CCoinsViewMemPool::GetCoins,
since CCoinsViewMemPool::GetCoins is called only once in the codebase,
followed by a call to pruneSpent with a `TODO` suggesting it be moved.
632 | @@ -633,7 +633,10 @@ void CTxMemPool::ClearPrioritisation(const uint256 hash) 633 | } 634 | 635 | 636 | -CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView *baseIn, CTxMemPool &mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { } 637 | +CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView *baseIn, CTxMemPool &mempoolIn) 638 | + : CCoinsViewBacked(baseIn), 639 | + mempool(mempoolIn), 640 | + criticalBlock(mempoolIn.cs, "CCoinsViewMemPool_lock", __FILE__, __LINE__) {}
By initializing this last gcc complains about initialization out of order (yes, that is an actual warning...), I'd prefer we not add new warning messages.
Aside from gcc warnings, tested ACK only commithash 377f674491230b5bce8308fde6189f7439866921: http://bitcoin.ninja/TheBlueMatt-5206.txt
utACK
Some further optimizations may be possible later, as AcceptToMemoryPool may need less code now to detect missing inputs; unsure.
You can at least remove the block at https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L925
Alternatively, have a LOCK_INIT(cs) macro that does initialize the cs variable, but does not declare it, and use that in the mempool constructor.
CCoinsViewMemPool::GetCoins is actually called multiple places in the code base, and this pull causes changes in behavior. For instance, using the rpc signrawtransaction, it used to be possible to sign a double spend using a zero-conf transaction as input when the first spend was in the mempool, but now that is not possible. For reference, it is still possible to sign a double spend with a confirmed transaction as input.
Not tested, but I think the pruning change breaks spending unconfirmed outputs because pruned CCoins wind up in the CCoinsViewCache in AcceptToMemoryPool.
When accepting the child, the call to view.HaveCoins(txin.prevout.hash)) results in a call to CCoinsViewMemPool::GetCoins via CCoinsViewCache::FetchCoins, and a pruned version of the unconfirmed parent is cached.
Then, view.HaveInputs(tx) will see the coins as being already spent.
@dgenr8 Are you sure about that? A pruned entry should still result in a true for HaveInputs as the unspent entry itself shouldnt be removed, and thats the only one HaveInputs cares about.
Ok, no pruning happens in view.HaveCoins(txin.prevout.hash)) because the child is not yet accepted (duh). If a conflicting child arrives later, it should correctly fail HaveInputs . So it seems this may work as a way to detect intra-mempool conflicts (not all conflicts though). This also explains @morcos finding above.
What about @sipa comment at line 649? For some reason he thought it was important not to return a pruned entry but the change explicitly does that.
You can at least remove the block at https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L925 @TheBlueMatt Removing the check at line 954 instead should be considered. It literally says "if tx is spendable, don't let it into the mempool!" and only works as a duplicate check because 'spendable' has been defined locally to include the mempool. The earlier check is more efficient (and more expressive IMHO).
@dgenr8 You misread the comment at line 649. The problem is that a pruned transaction may exist in pcoinsTip (perhaps because it used to be there and reorganized away), so if a transaction exists in the mempool, we have to return that one, even if a HaveCoins on the underlying cache may be true. The mempool consistency guarantees that in that case the underlying coin is fully spent anyway.
The check on line 954 (as @dgenr8 mentions) is actually worrysome. It is possible that HaveCoins returns true while there is actually no entry there (because it was reorganized away). This may possibly currently result in rejecting completely valid transactions.
Just to add one more question about how this is all supposed to work. It seems to me that if you see a double spend of a transaction that is already confirmed it will fail the HaveCoins check on its inputs in line 961 and therefore be treated as an orphan transaction. Is that the desired behavior? It doesn't seem like the HaveInputs check in line 969 gets used until we remove the explicit conflict check in #5347 (@dgenr8, this is what he meant by the block at line 925).
@morcos The risk currently already exists that a double-spending transaction is treated as an orphan. The correct solution for that is keeping track of recently-spent transactions, rather than relying on mempool/UTXOset logic to detect it. Unspent outputs are pretty much by definition a bad way to keep tracking of spent coins...
: ), oh yeah, guess that makes sense.
@apoelstra Can you move the pruneSpent call to also apply to transactions fetched from the underlying cache?
@sipa, @apoelstra already did in #5347.
