bisq-network / bisq

A decentralized bitcoin exchange network
https://bisq.network
GNU Affero General Public License v3.0
4.73k stars 1.27k forks source link

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

Closed pazza83 closed 4 months ago

pazza83 commented 11 months ago

Description

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

This seems to happen in the following circumstances:

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.

  1. Trade peer opening mediation
  2. Mediator not able to contact the user who put the trade in failed
  3. Trade peer not able to contact the user who put the trade in failed
  4. Mediator makes proposal (unresponsive peer)
  5. Trade goes to arbitration
  6. Arbitration ticket not opened for user who put the trade in failed
  7. 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:

Actual behaviour

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

ghost commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

(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.
github-actions[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 4 months ago

This issue has been automatically closed because of inactivity. Feel free to reopen it if you think it is still relevant.