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

Fix trade withdraw to external wallet step 4 #4260

Merged
merged 1 commit into from May 25, 2020
Merged

Fix trade withdraw to external wallet step 4 #4260

merged 1 commit into from May 25, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 15, 2020

The routine cleanUpAddressEntries in TradeManager was prematurely releasing funds associated with trades when they reach the last step of the process (Step 4, Withdraw to External Wallet).

Usually, with a single trade active this would not be an issue because cleanUpAddressEntries is called after the withdrawal, but if you have more than one trade at Step 4 then the first withdrawal would go though, but all the others would fail with an error box "Missing x.xxx BTC" (the trade proceeds or deposit amount). Alternatively, if you restart the bisq app with a
trade in Step 4, the same cleanup will occur and the attempt to withdraw to external wallet will fail.

The change here considers any trade held by the TradeManager to be in use and therefore will not have their associated address entries freed up. After Step 4 has passed, the trade is no
longer held by the TradeManager, and so cleanUpAddressEntries will at that point free up the address.

Fixes #4205
Fixes #3592

The routine `cleanUpAddressEntries` in TradeManager was prematurely
releasing funds associated with trades when they reach the last
step of the process (Step 4, Withdraw to External Wallet).
Usually, with a single trade active this would not be an issue
because `cleanUpAddressEntries` is called after the withdrawal,
but if you have more than one trade at Step 4 then the first
withdrawal would go though, but all the others would fail with
an error box "Missing x.xxx BTC" (the trade proceeds or deposit
amount).  Alternatively, if you restart the bisq app with a
trade in Step 4, the same cleanup will occur and the attempt
to withdraw to external wallet will fail.
The change here considers any trade held by the TradeManager to
be in use and therefore will not have their associated address
entries freed up.  After Step 4 has passed, the trade is no
longer held by the TradeManager, and so cleanUpAddressEntries
will at that point free up the address.
@ghost
Copy link
Author

ghost commented May 15, 2020

Test scenario (issue, prior to fix):

  • Create a trade, take it through to Step 4.
  • Shut down bisq and restart it, to trigger cleanUpAddressEntries.
  • Click Withdraw to External Wallet. It fails with an warning box Missing x.xxxx BTC and in the logs Insufficient Funds exception.

Test scenario (with fix applied):

  • Create a trade, take it through to Step 4.
  • Shut down bisq and restart it, to trigger cleanUpAddressEntries.
  • Click Withdraw to External Wallet and verify that it succeeds.
  • Verify that log messages are logged like this, AFTER the withdrawal succeeds.
    We found an outdated addressEntry for trade .. context=TRADE_PAYOUT
    swap addressEntry .. offerId from context TRADE_PAYOUT to available

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

More people testing this during release testing would be good.

@sqrrm sqrrm merged commit 481ae23 into bisq-network:master May 25, 2020
@ripcurlx ripcurlx added this to the v1.3.5 milestone Jun 4, 2020
@ghost ghost mentioned this pull request Jun 11, 2020
@ghost ghost deleted the fix_withdraw_trade branch June 29, 2020 20:43
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

Successfully merging this pull request may close these issues.

Final step of trade (withdraw BTC to external wallet) fails UTXO combinations resulting in dust outputs
2 participants