Prevent dust outputs from being created during withdraw from wallet #4093

ghost · 2020-03-24T14:50:52Z

This change fixes an issue whereby dust change outputs are inadvertently created when users make withdrawals from their wallets. (Funds -> Send Funds)

The solution taken here is to detect a dust TXO during the withdrawal fee estimation process and add that amount to the fee thus eliminating the dust output.

For example if the user has 1 BTC and goes to withdraw 0.99999900 BTC it will detect a change TXO of 100 sats which is below the dust limit, increase the fee by 100 sats and therefore withdraw 1 BTC.

This fix only applies to user withdrawals from their wallet. Other use cases such as P2P trading, deposits and fees will be handled separately.

Related to #4039

This change fixes an issue whereby dust change outputs are inadvertently created when users make withdrawals from their wallets. (Funds -> Send Funds) The solution taken here is to detect a dust TXO during the withdrawal fee estimation process and add that amount to the fee thus eliminating the dust output. For example if the user has 1 BTC and goes to withdraw 0.99999900 BTC it will detect a change TXO of 100 sats which is below the dust limit, increase the fee by 100 sats and therefore withdraw 1 BTC. This fix only applies to user withdrawals from their wallet. Other use cases such as P2P trading, deposits and fees will be handled separately. Related to #4039

sqrrm

Thanks for improving the dust handling.

Please see inline comments on the code.

desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java

- added a comment describing the `removeDust` method and its effects. - use more descriptive variable names. - made the logging more verbose to help log readers. - use a constant for the dust limit - add a notice to the user when dust is padded to the fee

ghost · 2020-03-29T02:48:09Z

Here are some tests I found useful:

TEST 1 - sending full wallet balance minus 1 satoshi

Setup - you have funds in your wallet, potentially split into more than 1 UTXO
Send your complete wallet balance minus 1 satoshi, option "Amount includes fee".
Verify that the confirmation dialog displays: Warning about dust (1 sats) added to fees;
Sending = the full amount of your wallet; Recipient will receive full amount of your wallet - fee
Click yes to make the transaction
Verify that the receiver gets full amount of your wallet - fee
Verify that the transaction gets confirmed on the blockchain.

TEST 2 - avoid dusty change output (amount excludes fee)

Setup - Set custom fee to 18 sats per byte
Setup - you have 1 UTXO in wallet = 1.0 BTC
Send 0.99995501, option "Amount excludes fee"
Verify that the confirmation dialog displays: Warning about dust (431 sats) added to fees; Sending 1.0 BTC; Recipient will receive 0.99995501 BTC; Fee is 4499 sats
Click yes to make the transaction
Verify that the receiver 0.99995501 BTC
Verify that the transaction gets confirmed on the blockchain.

TEST 3 - avoid dusty change output (amount includes fee)

Setup - Set custom fee to 18 sats per byte
Setup - you have 1 UTXO in wallet = 1.0 BTC
Send 0.99999999, option "Amount includes fee"
Verify that the confirmation dialog displays:Warning about dust (1 sats) added to fees; Sending 1.0 BTC; Recipient will receive 0.99995930 BTC; Fee is 4069 sats
Click yes to make the transaction
Verify that the receiver gets 0.99995931 BTC
Verify that the transaction gets confirmed on the blockchain.

TEST 4 - normal transaction with change output (amount excludes fee)

Setup - Set custom fee to 18 sats per byte
Setup - you have 1 UTXO in wallet = 1.0 BTC
Send 0.99990000, option "Amount excludes fee"
Verify that the confirmation dialog displays: Sending 0.99994050 BTC; Recipient will receive 0.9999 BTC; Fee is 4050 sats
Click yes to make the transaction
Verify that the receiver gets 0.9999 BTC
Verify that the transaction gets confirmed on the blockchain.

TEST 5 - normal transaction with change output (amount includes fee)

Setup - Set custom fee to 18 sats per byte
Setup - you have 1 UTXO in wallet = 1.0 BTC
Send 0.99990000, option "Amount includes fee"
Verify that the confirmation dialog displays: Sending 0.9999 BTC; Recipient will receive 0.99985950 BTC; Fee is 4050 sats
Click yes to make the transaction
Verify that the receiver gets 0.99985950 BTC
Verify that the transaction gets confirmed on the blockchain.

Now using `Restrictions.getMinNonDustOutput()` which equates to 546 sats

Moved message text into displaystrings.properties

This commit fixes bisq-network#4093, where it was demonstrated that a bisq.properties file containing the following entries would cause Bisq to fail at startup: baseCurrencyNetwork=BTC_MAINNET bannedSeedNodes= bannedBtcNodes= bannedPriceRelayNodes=5bmpx76qllutpcyp The source of the problem was that the jOptSimple argument parsing library converts the empty value of bannedSeedNodes to a List<String> of size 1 where the 0th element of the list is an empty string. This empty string was then attempted to be converted into a new NodeAddress, causing a validation error. This conversion happened during Guice wiring, and manifested as a blank white screen appearing as wiring errors often do in Bisq. The fix is simple and surgical. We now filter out any empty string elements before attempting to convert the banned seed node value to a new node address. I have reviewed the other related options, such as bannedPriceRelayNodes and bannedBtcNodes, and they do not cause the problem described above, so no filtering or other changes have been made to the way they work.

sqrrm

ACK

This is a lot easier to read than current master, good work. Tested and works as expected, good guide to testing as well.

ripcurlx requested a review from sqrrm March 25, 2020 08:58

sqrrm requested changes Mar 26, 2020

View reviewed changes

ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Mar 27, 2020

ghost requested a review from sqrrm March 29, 2020 03:00

jmacxx added 2 commits March 29, 2020 18:23

Change the constant used to determine what amount is considered dust

ea8b327

Now using `Restrictions.getMinNonDustOutput()` which equates to 546 sats

Change wording of the dust informative message per review by m52go

f1de079

Moved message text into displaystrings.properties

cbeams mentioned this pull request Mar 31, 2020

Avoid startup failure when bannedSeedNodes arg is empty #4104

Merged

ripcurlx added this to the v1.3.0 milestone Apr 1, 2020

sqrrm approved these changes Apr 3, 2020

View reviewed changes

sqrrm merged commit d0a8890 into bisq-network:master Apr 3, 2020

ghost mentioned this pull request Apr 7, 2020

For Cycle 12 bisq-network/compensation#523

Closed

sqrrm mentioned this pull request Apr 14, 2020

For Cycle 12 bisq-network/compensation#536

Closed

ghost deleted the fix_dust_wallet branch June 29, 2020 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent dust outputs from being created during withdraw from wallet #4093

Prevent dust outputs from being created during withdraw from wallet #4093

ghost commented Mar 24, 2020

sqrrm left a comment

ghost commented Mar 29, 2020 •

edited by ghost

Loading

sqrrm left a comment

Prevent dust outputs from being created during withdraw from wallet #4093

Prevent dust outputs from being created during withdraw from wallet #4093

Conversation

ghost commented Mar 24, 2020

sqrrm left a comment

Choose a reason for hiding this comment

ghost commented Mar 29, 2020 • edited by ghost Loading

TEST 1 - sending full wallet balance minus 1 satoshi

TEST 2 - avoid dusty change output (amount excludes fee)

TEST 3 - avoid dusty change output (amount includes fee)

TEST 4 - normal transaction with change output (amount excludes fee)

TEST 5 - normal transaction with change output (amount includes fee)

sqrrm left a comment

Choose a reason for hiding this comment

ghost commented Mar 29, 2020 •

edited by ghost

Loading