Change semantics of HaveCoinInCache to match HaveCoin #10559

1. 
   morcos commented at 9:05 pm on June 8, 2017: member

   Previously it was possible for HaveCoinInCache to return true for a spent coin. It is more clear to keep the semantics the same. HaveCoinInCache is used for two reasons:

   • tracking coins we may want to uncache, in which case it is unlikely there would be spent coins we could uncache (not dirty)
   • optimistically checking whether we have already included a tx in the blockchain, in which case a spent coin is not a reliable indicator that we have.

   This makes the logic in #10557 more obviously correct, although it is ok without this PR

2. Change semantics of HaveCoinInCache to match HaveCoin

   Previously it was possible for HaveCoinInCache to return true for a spent
   coin. It is more clear to keep the semantics the same. HaveCoinInCache is
   used for two reasons:
   - tracking coins we may want to uncache, in which case it is unlikely there
   would be spent coins we could uncache (not dirty)
   - optimistically checking whether we have already included a tx in the
   blockchain, in which case a spent coin is not a reliable indicator that we have.

   525769853e
3. 
   gmaxwell commented at 9:09 pm on June 8, 2017: contributor

   Don’t we want to skip fetching a spent coin, e.g. in net_processing.cpp:AlreadyHave ?
4. 
   morcos commented at 9:17 pm on June 8, 2017: member

   @gmaxwell I think the logic is we don’t know if it has been spent or disconnected
5. 
   gmaxwell commented at 9:19 pm on June 8, 2017: contributor

   Even if its been disconnected do we really want to fetch it again?
6. 
   morcos commented at 9:27 pm on June 8, 2017: member

   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.

7. fanquake added the label UTXO Db and Indexes on Jun 9, 2017
8. 
   gmaxwell commented at 1:52 am on June 9, 2017: contributor

   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.
9. 
   sipa commented at 2:11 am on June 9, 2017: member

   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.
10. 
    morcos commented at 3:51 pm on June 12, 2017: member

    @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).
11. 
    sipa commented at 6:38 pm on June 12, 2017: member

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

    utACK

12. 
    ryanofsky commented at 4:20 pm on June 22, 2017: member

    utACK 525769853efd7796679bf95d069a432df46d8e35. #10581 also includes this change so either or both of these PRs could be merged.
13. 
    TheBlueMatt commented at 2:22 am on June 25, 2017: member

    utACK 525769853efd7796679bf95d069a432df46d8e35
14. 
    laanwj commented at 3:09 pm on June 26, 2017: member

    utACK 5257698
15. laanwj merged this on Jun 26, 2017
16. laanwj closed this on Jun 26, 2017

    ---

17. laanwj referenced this in commit 234ffc677e on Jun 26, 2017
18. codablock referenced this in commit ca3d238b6d on Sep 27, 2017
19. codablock referenced this in commit 76274770aa on Oct 12, 2017
20. codablock referenced this in commit b59661ecd2 on Oct 26, 2017
21. codablock referenced this in commit 5b629d84b7 on Oct 26, 2017
22. codablock referenced this in commit d1d7dcb32d on Oct 26, 2017
23. codablock referenced this in commit f49b39d83d on Oct 30, 2017
24. codablock referenced this in commit 13208abce3 on Oct 31, 2017
25. codablock referenced this in commit a7b03f0686 on Oct 31, 2017
26. codablock referenced this in commit 06aa02ff63 on Oct 31, 2017
27. UdjinM6 referenced this in commit f67497c973 on Nov 8, 2017
28. schancel referenced this in commit a3a9fa29b6 on Jul 18, 2019
29. jonspock referenced this in commit 9c8c7b4aca on Aug 29, 2019
30. proteanx referenced this in commit 31df77233e on Sep 4, 2019
31. DrahtBot locked this on Sep 8, 2021

Contributors
morcos gmaxwell sipa ryanofsky TheBlueMatt laanwj

Labels
UTXO Db and Indexes