Skip to content
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.

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

False deposit confirmation messages gives option for user to move the trade to 'Failed Trades' increasing likelihood of trade ending in arbitration. #6948

Open
pazza83 opened this issue Nov 25, 2023 · 5 comments

Comments

@pazza83
Copy link

pazza83 commented Nov 25, 2023

Description

On occasion Bisq client does not recognize the deposit confirmation ID.

This seems to happen in the following circumstances:

  • Deposit confirmation takes over 4 hours to confirm, I think Bisq stops checking after 4 hours. Might be another number. I think this is in the code. As a result it recommends an SPV resync.
  • User does an SPV resync at some point during the trade process that gives a false error. Perhaps related to above. Eg user gets error message due to deposit not confirming in 4 hours, recommends them to resync. They do an SPV resync and this shows them to trade is failed.

User is promoted to move the trade to failed trade despite their being a confirmed deposit confirmation.

Example with screenshots:

image
Trade showing as failed when it has not in open offers.

image2
Trade information screen showing trade has failed when it has not.

Deposit Transaction ID for the trade was: 7e7189d09c3e11803650be541ef079ff8744b971c3d02e50aa1a852aa6385e46

This causes some users to move the trade to failed. This results in:

1a. Trader peer if seller not receiving payment.
1b. Trade peer if buyer sending payment and not getting their bitcoin released.
2. Trade peer opening mediation
3. Mediator not able to contact the user who put the trade in failed
4. Trade peer not able to contact the user who put the trade in failed
5. Mediator makes proposal (unresponsive peer)
6. Trade goes to arbitration
7. Arbitration ticket not opened for user who put the trade in failed
8. User with false error message often oblivious to the fact they lost funds. They might receive a partial payout from the arbitrator into their Bisq wallet but either not notice or be oblivious to where the bitcoin has come from. Either way they have lost their security deposit.

Version

1.9.4

Steps to reproduce

One of the issues with the above is it is more likely to happen when the mempool is busy, as trade confirmation can often take over 4 hours. Bisq prompts the user to do an SP resync and as a result the trade may show as failed.

In this case the user can resolve the issue by trying another SPV when the deposit has confirmed and/or opening mediation with 'Ctrl' and 'O' but if they put the trade in failed then the process above starts.

Expected behaviour

Ideally Bisq not to give false errors about the deposit transaction IDs.

Alternatively some mitigations would be:

  • Trader chat messages still delivered to user with trade in failed.
  • Mediation chat messages still delivered to user with trade in failed.
  • When Mediator opens ticket then user with failed trade also has a mediation ticket open in their instance.
  • Initial prompt to do an SPV resync to be changed to open mediation by selecting the trade and pressing 'Ctrl' and 'O'. Let the mediator advise the trader if they have locked funds or not rather than leaving it to the trader to know.

Actual behaviour

Trade falsely shows as failed. User at risk of losing their deposit.

@ghost
Copy link

ghost commented Jan 3, 2024

I think this issue was written in kind of a confusing way, at least I found it hard to understand. Communication needs to improve within the Bisq team.

I think Bisq stops checking after 4 hours. Might be another number. I think this is in the code.

If a trade deposit tx shows as pending, bisq has a transaction confidence listener on it until confirmed.

The SPV resync popup is an independent UI feature requested here, in other words Bisq does not give up waiting for an open trade to confirm.

If an SPV resync is done via deleting wallet files with a pending trade open, that pending trade would think its transactions have been nuked and prompt to be moved to failed. The solution there, recently discovered, is not to SPV resync by deleting files but do it from the Settings/Network Settings screen. Trades are protected that way.

@pazza83
Copy link
Author

pazza83 commented Jan 3, 2024

I think this issue was written in kind of a confusing way, at least I found it hard to understand. Communication needs to improve within the Bisq team.

Hi apologies for this. I think I was trying to guess at reasons that might be causing this issue.

There are likely multiple reasons causing trades to be incorrectly moved to failed trades.

A few examples of when this is happening is:

If an SPV resync is done via deleting wallet files with a pending trade open, that pending trade would think its transactions have been nuked and prompt to be moved to failed. The solution there, recently discovered, is not to SPV resync by deleting files but do it from the Settings/Network Settings screen. Trades are protected that way.

I wonder if that could be one of the causes, users are prompted to do an SPV resync and some users do it by deleting the SPV file causing Bisq to think the trade is failed (has a non valid SPV resync).

From my perspective mediating these trades it does seem that unresponsive traders become more prevalent in mediation when fees increase. Where I am able to reach out to the non-responsive users they often report they were unaware of the trade being in progress as it was in their failed trades or trade history.

@suddenwhipvapor
Copy link

While discussing with pazza about this, he suggested Bisq could do a "background check" on an external explorer (even better than just one, use more mirrors for redundancy) and then present the user with an appropriate message. It may happen that Bisq thinks the deposit is unconfirmed, while it was actually confirmed in a block already, so SPV is appropriate and it should be suggested in the popup, or that indeed, the deposit is not confirmed and the user doesn't need to do anything special other than waiting. There would be a timeout after which Bisq will check again and reassess what (and if) to suggest the user does.

@suddenwhipvapor
Copy link

Extending this concept, since BitcoinJ is the Achille's heel of the application, checking an external explorer could be implemented before a trade, to verify that every utxo Bisq is about to spend, is actually still spendable, or in general before anything Bisq tries to do with its existing managed utxos. Probably "ugly", but workable, no?

@schildbach
Copy link

schildbach commented Jan 7, 2024

(came here via discussion in the bitcoinj-users Matrix room: https://matrix.to/#/#bitcoinj-users:matrix.org)

I'm quite sure this is caused by a shortcoming of the segwit specifications, which did not take BIP37 (bloom filters) into account when moving the pubkey+signature out of the scriptSig into the witness. The problem has become more and more serious with Bitcoin Core changes that limit how pending transactions are broadcasted (e.g. not re-broadcasted). What (I think) happens is this:

  1. SPV client receives incoming P2WPK transaction.
  2. Only then it becomes clear what element needs to be added to the filter to also catch spends from the new UTXO (the second txn). We can't use the pubkey similar to P2PK(H), because with segwit the spend will not have a scriptPubKey that could be matched. So we need to add the new outpoint instead.
  3. By then, Bitcoin Core assumes it has already sent the second transaction, or it assumes no peer is interested, so it will never send, even after a new filter has been set. And in later Bitcoin Core versions it will never be re-broadcasted. This is what you're experiencing. I'm pretty sure any SPV client will have this problem, as it's rooted in the specs.
  4. A reconnect will (maybe) re-broadcast and fix the situation. Also a confirmation will fix the situation, as we'll get a partial block and the txn attached to that.

I see the following remediation options:

  1. Preferred: We need a simpler, more private and more future proof version of BIP37 that simply matches against the scriptPubKey of outputs and the scriptPubKey of the UTXO that is spent by inputs. No other matches needed. (Also, almost all operations could be dropped from the spec.)
  2. Drink from the firehose, don't use BIP37. Bitcoin Wallet has this option, and I think it works well nowadays. At least in developed countries costs for mobile data is low enough to support this.
  3. In the second txn, send change back to yourself. This will cause the pubkey hash to match, and that has been added to the filter already when the address was generated.
  4. Avoid chaining pending txns. Only spend confirmed outputs.
  5. Kludge: Connect only against older Bitcoin Core versions that re-broadcast from time to time.
  6. Kludge: Drop connections upon receiving the first transaction.
  7. Kludge: Use P2PKH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@schildbach @pazza83 @suddenwhipvapor and others