Don't we want to skip fetching a spent coin, e.g. in net_processing.cpp:AlreadyHave ?

@gmaxwell I think the logic is we don't know if it has been spent or disconnected

Even if its been disconnected do we really want to fetch it again?

I don't think it really matters much either way, we're talking about real edge cases:

• a tx being announced to us which recently appeared in a block and has already had its output spent. And I think we must be talking about a case where a flush happened in between the tx and it's spend otherwise the coin would have been fresh and erased and HaveCoinInCache would never have returned true anyway.
• a disconnected tx which wasn't accepted to the mempool during reorg but is now being reannounced to us.

My opinion is that there are pros and cons either way, but the simplicity of having the semantics be the same is the deciding factor.

I think the semantics change should happen. My doubt is only if AlreadyHave should be using this call; or perhaps one that will also potentially return spent entries.

Do you feel like changing CCoinsViewMemPool::HaveCoin to also check for non-spentness? I believe that function is in fact not called anywhere, so perhaps it can assert instead. The point is that having a consistent definition everywhere would be nice.

@sipa I agree it is not currently used. I'm on the fence about changing it, but happy to if that's what everyone else wants. I feel like the definition is a bit different anyway if it's in the mempool, since you could for instance still create a new transaction that spends that coin which would be accepted (as a replacement).

@morcos Ok, let's discuss that independently perhaps - it has much less real effect anyway.

utACK

utACK 5257698

utACK 5257698