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

Improve handling of spv resync edge case #3821

Merged
merged 4 commits into from Jan 2, 2020

Conversation

@chimp1984
Copy link
Contributor

chimp1984 commented Dec 21, 2019

This PR handles 2 edge cases:

1. Handling unconfirmed deposit txs at SPV resync

If a trader has a pending trade with an unconfirmed tx and is doing a
spv resync, the deposit tx is not found. They get displayed a popup
asking to restart and if the problem continues to move the trade to the failed
trades list.

In regtest testing the missing deposit tx was always received
from the network at another restart. So this commit adds another button to the popup and use that as the default button to shut down Bisq so that the user do first this restart activity
instead of moving the trade to failed trades.

It is not guaranteed that the trader will receive the missing deposit tx
at restart, but at least it is better as the state before. We should
find a way how to distinguish a valid unconfirmed tx from an invalid one
but it is not clear yet how we can do that with BitcoinJ only. It might require an external service to
look up the tx if it is in the mem pool, but even that will never give a 100% certainty.

Testing:

Testing was done in regtest/localhost mode by creating a trade and do a spv resync when the deposit tx was still unconfirmed. In all 4 trade roles (maker as buyer, maker as seller, taker as buyer, taker as seller) the behaviour was the same that after the spv resync the popup showed up and the deposit tx was not found in the wallet (not received from network), but after another restart the deposit tx was there.

Also tested with an invalid tx (set miner fee to 0) and behaviour was the same. At each startup the popup shows up and even after a restart the deposit tx is not found. So moving the trade to failed trades is the right thing in that case.

EDIT: I will add another commit with text in the popup telling the user to upen the block explorer with the trade fee txs and see if both are valid. This should help to detect invalid txs.

2. Handle unconfirmed BSQ change outputs at SPV resync

In case we had an unconfirmed BSQ change output we reset the
unconfirmedBsqChangeOutputList so that after a SPV resync we do not
have any dangling BSQ utxos in that list which would cause an incorrect
BSQ balance state after the SPV resync.

A case with an invalid tx where BSQ was used for the trade fee causes also that the BSQ tx was never confirming. It contained a BSQ change output to ourself and that utxo is managed inside the BSQ domain. A SPV resyc removed the invalid tx (including a BSQ input and output) but the unconfirmed BSQ change output was not updated to the different state caused by the SPV resync. We need to clear the BSQ utxo list to ensure that there are no dangling utxos which could show a incorrect BSQ state/balance after a SPV resync.

Clearing the BSQ utxo list is non-critical as it is only for improving usability to be able to spend unconfirmed BSQ change outputs. If that list gets cleared the only effect is that the user need to wait for the next confirmation to make the change output spendable again.

Testing:

Normal case (valid tx):

Tested on regtest/localhost with sending BSQ to myself which creates an unconfirmed BSQ tx output. Doing a SPV resync and restart 2 times. After that the unconfirmed BSQ change output is not available for spending. After a confirmation all is spendable again.

Sending 11 BSQ to own wallet. This tx creates a change output of 1499989 BSQ which is shown as balance as we allow to use own change outputs. We do not show the funds sent to ourself in the balance as that use-case is not typical (to send BSQ to one self) and we do not support that this utxo is spendable.
Screen Shot 2019-12-21 at 17 03 33

After SPV resync that utxo is not recognized anymore and we only see the confirmed txs, which result in a 0 BSQ balance (both cahnge and sent BSQ are unconfrimed and therefore not verified by BSQ parser).
Screen Shot 2019-12-21 at 17 04 06

After confirmation the change and the sent BSQ are validated and spendable again:
Screen Shot 2019-12-21 at 17 04 13

Testing with an invalid tx:

To test an invalid tx requires a bit of a hack. I did it by setting the miner fee to 0 and used BSQ for trade fee in the trade. A tx without miner fee is invalid and a SPV resync clears that out, but the BSQ change output still was there and caused an incorrect BQ balance display. This fix solves that as it clears the BSQ utxo list and only shows balance from confirmed txs.

If a trader has a pending trade with an unconfirmed tx and is doing a
spv resync, the deposit tx is not found. They get displayed a popup
asking to restart and if problem continues to move trade to failed
trades. In regtest testing the missing deposit tx was always received
from the network at a restart. So this commit adds another button as
default button to shut down Bisq so that the user do first that activity
instead of moving the trade to failed trades.
It is not guaranteed that the trader will receive the missing deposit tx
at restart, but at least it is better as the state before. We should
find a way how to distinguish a valid unconfirmed tx from an invalid one
but it is not clear yet how we can do that and if it is feasible with
doing that with BitcoinJ only. It might require a external service to
look up the tx if it is in the mem pool, but even that will never gie a
100% certainty.
@chimp1984 chimp1984 requested review from ripcurlx and sqrrm as code owners Dec 21, 2019
@chimp1984 chimp1984 changed the title Add button for shutdown Improve handling of spv resync edge case Dec 21, 2019
chimp1984 added 2 commits Dec 21, 2019
In case we had an unconfirmed change output we reset the
unconfirmedBsqChangeOutputList so that after a SPV resync we do not
have any dangling BSQ utxos in that list which would cause an incorrect
BSQ balance state after the SPV resync.
@chimp1984

This comment has been minimized.

Copy link
Contributor Author

chimp1984 commented Dec 21, 2019

@m52go Could you have a look to that popup text and improve it if needed?

popup.warning.trade.depositTxNull=The trade with ID ''{0}'' has no deposit transaction set.\n\n\
  Please restart the application to see if the problem still exist.\n\n\
  If so, please open the trade details popup by clicking on the trade ID and open a block explorer for both the \
  maker fee transaction and the taker fee transaction (by clicking on both transaction IDs). If any of those \
  transactions is not found in the block explorer it is likely because it was an invalid transaction.\n\n\
  If this was the case, please report the problem in the Bisq support channel at the Bisq Keybase team. If you are \
  sure the problem is caused by an invalid transaction move the trade to failed trades.\n\n\
  No funds have left your wallet in that case. If your trade fee tx was valid you have lost the trade fee and you can \
  request for reimbursement at the support repository on Github (https://github.com/bisq-network/support/issues).\n\n\
  Make a SPV resync at ''Settings/Network'' to clean up your wallet from any invalid transactions!
@m52go

This comment has been minimized.

Copy link
Member

m52go commented Dec 23, 2019

Suggestion:

[notable changes]

  • added "IMPORTANT" text at beginning (optional, can remove depending on popup headline, if any)
  • added keybase link
popup.warning.trade.depositTxNull=IMPORTANT: the trade with ID ''{0}'' has no deposit transaction set.\n\n\
  Please restart the application to see if the problem still exists.\n\n\
  If it does, please open the trade details popup by clicking on the trade ID. Then click on the transaction IDs for
  the maker fee transaction and the taker fee transaction to view them on a block explorer. A transaction \
  that cannot be found in a block explorer is probably an invalid transaction.\n\n\
  If this happens, please report it in the #support channel on the Bisq Keybase (https://keybase.io/team/bisq). \
  If your trade fee tx is invalid, no funds have left your wallet, you can move the trade to failed trades,\
  and do an SPV resync for your funds to reappear (see how below).\n\n\
  If your trade fee tx is valid, the fee amount is lost, and you can make a \
  request for reimbursement on the support repository on GitHub (https://github.com/bisq-network/support/issues).\n\n\
  In both cases, please do an SPV resync from the ''Settings/Network'' screen to clean your wallet of any lingering issues!
@chimp1984

This comment has been minimized.

Copy link
Contributor Author

chimp1984 commented Dec 23, 2019

@m52go Thanks!

@chimp1984 chimp1984 mentioned this pull request Dec 23, 2019
@sqrrm
sqrrm approved these changes Jan 2, 2020
Copy link
Member

sqrrm left a comment

utACK

This will likely lead to fewer trades moved to failed trades when they shouldn't be.

A point on invalid txs; some txs, such as those with 0 miner fee, are valid and could be mined. It's just very unlikely that they would be. In Bisq's case it's clear they won't be mined, but in the case when fees go up and bitcoin nodes with the default mempool size kick low fee txs out but other nodes with larger mempools keep them it's not quite clear which txs are going to confirm later and which won't.

@sqrrm sqrrm merged commit 5676841 into bisq-network:master Jan 2, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ripcurlx ripcurlx added this to the v1.2.5 milestone Jan 2, 2020
@sqrrm sqrrm mentioned this pull request Jan 13, 2020
@chimp1984 chimp1984 deleted the chimp1984:handle-spv-resync-edge-cases branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.