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

Prevent dust outputs from being created during the trade process #4094

Merged
merged 3 commits into from Apr 3, 2020
Merged

Prevent dust outputs from being created during the trade process #4094

merged 3 commits into from Apr 3, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 24, 2020

This change fixes an issue whereby dust change outputs are inadvertently created during the trading process, unbeknownst to the user. The dust outputs cause the Bitcoin node to reject the
transaction and the trade then becomes stuck.

The solution taken here is to detect a dust TXO during the trade process and remove it from the transaction before broadcasting. This applies in both making an offer and/or taking an existing offer. When a dust output is prevented, it will be noted in the log as follows:

image

Related to #4039

This change fixes an issue whereby dust change outputs are
inadvertently created during the trading process, unbenownst to the
user.  The dust outputs cause the Bitcoin node to reject the
transaction and the trade then becomes stuck.

The solution taken here is to detect a dust TXO during the trade
process and remove it from the transaction before broadcasting.

Related to #4039
@ghost
Copy link
Author

ghost commented Mar 24, 2020

I neglected to mention that this initial commit only handles the path taken when BTC is used for the trading fee; when BSQ is used the transaction is built by a different routine so some work will be needed to check for dust in the same way there.

Also, a bit sloppy that I check the dust limit using a hard coded 546 sats value; preferences.getIgnoreDustThreshold() could be used which equates to 600 sats currently.

[edit] both these issues have now been addressed.

@sqrrm sqrrm self-assigned this Mar 26, 2020
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.

Looks functionally correct and easy to follow. Would be helpful with a comment on how this works and what the effects are, perhaps around the removeDust method

Please review inline comments

- added a comment describing the `removeDust` method and its effects.
- applied a fix to the declaration of an ArrayList.
- use more descriptive variable names.
- made the logging more verbose to help log readers.
@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Mar 27, 2020
Also, use `Restrictions.getMinNonDustOutput()` for the dust limit
@@ -257,6 +260,7 @@ public Transaction completeBsqTradingFeeTx(Transaction preparedBsqTx,
checkNotNull(wallet, "Wallet must not be null");
wallet.completeTx(sendRequest);
Transaction resultTx = sendRequest.tx;
removeDust(resultTx);
Copy link
Member

Choose a reason for hiding this comment

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

I think this will cause BSQ change that lower than dust to be used as extra fee. That could be a non trivial amount (5.46 BSQ > USD3). While that's not a huge amount it's still too big to be an acceptable loss for users. If BSQ prices rise it will become even less so.

I think we'll have to fail transactions that try to pay the fee using BSQ if the change is less than dust. Best would be to show something at the time the fee is selected that it's likely to result in a dust change output and thus not be possible to continue. I think that should be doable, the fees are known at the time of setting up the trade so we can calculate the BSQ dust. One thing we need to do is make sure that if there is enough BSQ but split over 2 or more outputs, it's aggregated to avoid dust outputs. Example, counting satoshis:

Fee: 1000
BSQ wallet outputs:

  1. 1200
  2. 900

I think the tx might be constructed using only outputs 1. leaving a change output of 200, but it would be possible to construct the tx to take both 1. and 2. to pay the 1000 in fee and get the change 1100.

Copy link
Author

Choose a reason for hiding this comment

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

@sqrrm From what I've seen there is existing code which checks the BSQ for dust before building the transaction (see #3688). The removeDust insertion here looks at the transaction after it is built, just before broadcasting, to remove any dust change BTC. I will look more into how it works though.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible the check during tx build is not there but rather later, causing it to look like a bug. Still, that's at least better than burning BSQ unwittingly. If you could check that it's handled properly and not as a bug report that would be best of course.

Copy link
Author

Choose a reason for hiding this comment

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

At the moment there is only one way to get messages back to the GUI - via the errorMessage property on the Offer which is used by numerous exceptional events. I looked into reporting warnings back via adding a warningMessage property and will propose that as a new PR to address #3688. (Its a non-trivial change and I don't want to complicate this PR by adding it in here).

@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 1, 2020
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.

ACK

@sqrrm sqrrm merged commit c54a5ac into bisq-network:master Apr 3, 2020
@ghost ghost mentioned this pull request Apr 7, 2020
@ghost ghost deleted the fix_dust_trading branch May 16, 2020 12:57
@ghost ghost restored the fix_dust_trading branch May 16, 2020 12:58
@ghost ghost deleted the fix_dust_trading 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
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants