Fix issue with conflicted mempool tx in listsinceblock #8757

1. 
   jonasschnelli commented at 1:02 pm on September 19, 2016: contributor

   Attempt to fix #8752

   The second parameter (target confirmation) is quite strange and should probably be better documented in the RPC help of listsinceblock (check original comment: #199 (comment))

   At the moment, conflicted mempool transactions (double spends) are listed. This PR does hide all wtxs that are conflicts (GetDepthInMainChain <= -1).

2. Fix issue with conflicted mempool tx in listsinceblock c12f4279fb
3. jonasschnelli added the label RPC/REST/ZMQ on Sep 19, 2016
4. MarcoFalke added the label Needs backport on Sep 19, 2016
5. MarcoFalke added this to the milestone 0.13.1 on Sep 19, 2016
6. 
   FrozenPrincess commented at 1:37 pm on September 19, 2016: none

   Just to clarify:

   This PR does hide all wtxs that are conflicts (GetDepthInMainChain <= -1).

   Does this hide all wtxs that are conflicts, or just wtxs that conflict before block x (when you’re filtering for transactions since block x)?

   (Because I am, and know other people currently relying on listsinceblock to return conflicted transactions to detect when we’ve been double-spent against)

7. 
   MarcoFalke commented at 5:17 pm on September 19, 2016: member

   I think it would help to add some tests for this.
8. 
   jonasschnelli commented at 7:05 am on September 20, 2016: contributor

   @FrozenPrincess: this PR just hides all wallet transactions that are unconfirmed and not in your mempool (very likely conflicted). I guess this would be the expected behavior according to the RPC help description:

   0Get all transactions in blocks since block [blockhash][...]
   

9. 
   FrozenPrincess commented at 5:36 pm on September 20, 2016: none

   @FrozenPrincess: this PR just hides all wallet transactions that are unconfirmed and not in your mempool (very likely conflicted).

   In that case, I’m strongly against this change of behavior. Currently if you have a service that processes bitcoin payments, you just need to run a loop (pseudo code):

    0lastHash = null
    1while (true ) {
    2   transactions, lastHash = bitcoin-cli listsinceblock $lastHash 6
    3   for transaction in transactions {
    4        if transaction.category == "receive" {
    5          if transaction.confirmations == 0 {
    6               // new mempool entry, notify the user or something
    7          }
    8          if transaction.confirmations > 0 {
    9             //  new confirmed transaction, add it
   10          }
   11          if transaction.confirmations < 0 {
   12            // shit, we've been double-spent. make sure to handle this!
   13         }
   14     }
   15  }
   16}
   

   But if you change the behavior to filter out the conflicted transactions – you will silently break peoples (including my) logic for knowing when they’ve been double-spent against. This is especially important now with bip125

10. jonasschnelli removed this from the milestone 0.13.1 on Sep 22, 2016
11. MarcoFalke removed the label Needs backport on Sep 24, 2016
12. 
    RHavar commented at 10:42 pm on June 22, 2017: contributor

    I think this should be closed in favor of #10470, which I believe fixes it properly =)
13. 
    jonasschnelli commented at 8:20 pm on September 29, 2017: contributor

    Closing in favor of #10470
14. jonasschnelli closed this on Sep 29, 2017

    ---

15. meshcollider referenced this in commit bfc4c896d6 on Nov 5, 2019
16. sidhujag referenced this in commit c06d5017eb on Nov 7, 2019
17. sidhujag referenced this in commit cc82e86c7f on Nov 10, 2020
18. DrahtBot locked this on Dec 16, 2021

Contributors
jonasschnelli FrozenPrincess MarcoFalke RHavar

Labels
RPC/REST/ZMQ