Skip to content

Fix issue with conflicted mempool tx in listsinceblock #8757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jonasschnelli wants to merge 1 commit into from
Closed

Fix issue with conflicted mempool tx in listsinceblock #8757

jonasschnelli wants to merge 1 commit into from

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Sep 19, 2016

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).

@jonasschnelli jonasschnelli mentioned this pull request Sep 19, 2016
Closed
@maflcko maflcko added this to the 0.13.1 milestone Sep 19, 2016
@FrozenPrincess
Copy link

FrozenPrincess commented Sep 19, 2016

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)

@maflcko
Copy link
Member

maflcko commented Sep 19, 2016

I think it would help to add some tests for this.

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Sep 20, 2016

@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:

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

@FrozenPrincess
Copy link

FrozenPrincess commented Sep 20, 2016

@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):

lastHash = null
while (true ) {
   transactions, lastHash = bitcoin-cli listsinceblock $lastHash 6
   for transaction in transactions {
        if transaction.category == "receive" {
          if transaction.confirmations == 0 {
               // new mempool entry, notify the user or something
          }
          if transaction.confirmations > 0 {
             //  new confirmed transaction, add it
          }
          if transaction.confirmations < 0 {
            // shit, we've been double-spent. make sure to handle this!
         }
     }
  }
}

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

@jonasschnelli jonasschnelli removed this from the 0.13.1 milestone Sep 22, 2016
@mchrostowski mchrostowski mentioned this pull request May 27, 2017
Closed
@RHavar
Copy link
Contributor

RHavar commented Jun 22, 2017

I think this should be closed in favor of #10470, which I believe fixes it properly =)

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Sep 29, 2017

Closing in favor of #10470

@adamjonas adamjonas mentioned this pull request Oct 25, 2019
Merged
meshcollider added a commit that referenced this pull request Nov 5, 2019
@meshcollider
Merge #17258: Fix issue with conflicted mempool tx in listsinceblock
bfc4c89 
436ad43 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)

Pull request description:

  Closes #8752 by bringing back abandoned #10470.

  This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.

  For more context, #8757 was closed in favor of #10470.

ACKs for top commit:
  instagibbs:
    utACK 436ad43
  kallewoof:
    utACK 436ad43
  jonatack:
    I'm not qualifed to give an ACK here but 436ad43 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:

Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 7, 2019
@meshcollider
Merge bitcoin#17258: Fix issue with conflicted mempool tx in listsinc…
c06d501 
…eblock

436ad43 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)

Pull request description:

  Closes bitcoin#8752 by bringing back abandoned bitcoin#10470.

  This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.

  For more context, bitcoin#8757 was closed in favor of bitcoin#10470.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@436ad43
  kallewoof:
    utACK 436ad43
  jonatack:
    I'm not qualifed to give an ACK here but 436ad43 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:

Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@meshcollider
Merge bitcoin#17258: Fix issue with conflicted mempool tx in listsinc…
cc82e86 
…eblock

436ad43 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)

Pull request description:

  Closes bitcoin#8752 by bringing back abandoned bitcoin#10470.

  This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.

  For more context, bitcoin#8757 was closed in favor of bitcoin#10470.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@436ad43
  kallewoof:
    utACK 436ad43
  jonatack:
    I'm not qualifed to give an ACK here but 436ad43 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:

Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rpc listsinceblock hash is not correctly filtering double-spends

4 participants

@jonasschnelli @FrozenPrincess @maflcko @RHavar