Recently mined TXs with spent outputs are considered orphans when re-received #16977

When a transaction is received, multiple places usually try to figure out if that TX is already known locally (mempool or part of the active chain) and thus should be skipped.

It first happens directly after receiving and deserialization of the TX, right before AcceptToMemoryPool() is called for the TX.

The second check happens inside MemPoolAccept::PreChecks() which is called from AcceptToMemoryPool(). Here it first checks if the inputs are existent and unspent. If HaveCoin() returns false for any input, it then checks if maybe the currently processed TX itself is already existent in the coins view/cache, which indicates that the TX is already locally processed (part of the active chain, aka mined). If this is the case, TX_CONFLICT is returned, otherwise pfMissingInputs is set to true, causing the orphan logic to trigger later.

The problem now is, that HaveCoinInCache (which is used to check if the TX is locally already processed) will only return true for unspent outputs of the currently processed TX. This means, if all outputs of the TX are already spent, it will not realize anymore that the TX was already mined. It will then continue by setting pfMissingInputs to true, which then falsely causes the orphan logic to trigger.

This results in the TX being added to the orphan map and then not being removed again until expiration kicks in.

My question now is: Is this expected behavior? I would assume that changing HaveCoinInCache() to optionally return true even for spent coins would partially solve the issue, at least for everyday cases that happen from time to time. This is because coins completely vanish from the coins DB when the cache is flushed, so the "fix" would only work between mining of the TX and the next cache flush. It would not protect against peers deliberately trying to fill the orphan map with such TXs...but this doesn't really matter because they can already fill it with any TX that has an unknown input.

Before #10559, HaveCoinInCache() would have returned true in this case actually, so the issue with false-positive orphans was not present (or at least not that bad...) at that time. Was this overlooked maybe?

I think this is expected behavior, though I agree this is a type of thing that would be nice to improve. Specifically:

I would assume that changing HaveCoinInCache() to optionally return true even for spent coins would partially solve the issue, at least for everyday cases that happen from time to time.

I imagine that this it is very rare that you'd have a recently mined transaction whose outputs are fully spent that is still in the utxo cache, because our utxo cache can remove entries that were never seen on disk. When new utxos are created, they are FRESH in the CCoinsViewCache, and when they're fully spent, we can delete them from the map without ever flushing to disk (because our on-disk storage never knew about the utxo in the first place). So I would think it would make very little difference whether you included spentness for the purposes of AlreadyHave().

One way we might be able to optimize this situation is to just cache the txids of what was mined in the last block -- we already cache the last connected block in memory to optimize block relay, so I think we could add a cache of recently mined transactions to optimize out orphan requests due to these timing issues. (Or even just iterate the cached block, if that's sufficiently fast.)

Ok I think I understand why changing HaveCoinInCache() would not help. Caching the txids of the last recent block(s) should work. I also figured out that utilizing the txindex might be helpful, at least in my case (forked coin that has txindex enabled by default). Or maybe a rolling bloom filter is the better solution, but then I'm not sure reorgs would be handled well as entries can't be deleted (which is probably ok because the TXs are still considered "already seen").

Has been fixed in #17951