From 057e2c5e7cbb7c13cfe3b36df2583338cbf2982c Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Tue, 24 Mar 2020 09:23:58 -0500 Subject: [PATCH 1/4] Remove dust outputs created during withdraw from wallet 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 --- .../main/funds/withdrawal/WithdrawalView.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java index 9f1a59172e0..c98d4e63f17 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java @@ -56,6 +56,7 @@ import org.bitcoinj.core.Coin; import org.bitcoinj.core.InsufficientMoneyException; import org.bitcoinj.core.Transaction; +import org.bitcoinj.core.TransactionOutput; import org.bitcoinj.wallet.Wallet; import javax.inject.Inject; @@ -330,8 +331,9 @@ private void onWithdraw() { feeEstimationTransaction = walletService.getFeeEstimationTransactionForMultipleAddresses(fromAddresses, sendersAmount); } checkNotNull(feeEstimationTransaction, "feeEstimationTransaction must not be null"); - Coin fee = feeEstimationTransaction.getFee(); - sendersAmount = feeExcluded ? amountAsCoin.add(fee) : amountAsCoin; + Coin dust = getDust(feeEstimationTransaction); + Coin fee = feeEstimationTransaction.getFee().add(dust); + sendersAmount = feeExcluded ? amountAsCoin.add(fee) : amountAsCoin.add(dust); Coin receiverAmount = feeExcluded ? amountAsCoin : amountAsCoin.subtract(fee); if (areInputsValid()) { int txSize = feeEstimationTransaction.bitcoinSerialize().length; @@ -629,6 +631,17 @@ public void updateItem(final WithdrawalListItem item, boolean empty) { } }); } + + private Coin getDust(Transaction tx) { + Coin dust = Coin.ZERO; + for (TransactionOutput txo: tx.getOutputs()) { + if (txo.getValue().value < 546) { + dust = dust.add(txo.getValue()); + log.info("dust TXO = {}", txo.getValue().toFriendlyString()); + } + } + return dust; + } } From 378c57492b5653bb0ce576d3fa3329718fd094c8 Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Sat, 28 Mar 2020 21:22:40 -0500 Subject: [PATCH 2/4] Address issues raised in review by sqrrm on 2020-03-26 - 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 --- .../main/funds/withdrawal/WithdrawalView.java | 64 ++++++++++++++----- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java index c98d4e63f17..a84645c64a3 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java @@ -98,6 +98,8 @@ import org.spongycastle.crypto.params.KeyParameter; +import java.text.MessageFormat; + import java.util.ArrayList; import java.util.Comparator; import java.util.HashSet; @@ -331,10 +333,28 @@ private void onWithdraw() { feeEstimationTransaction = walletService.getFeeEstimationTransactionForMultipleAddresses(fromAddresses, sendersAmount); } checkNotNull(feeEstimationTransaction, "feeEstimationTransaction must not be null"); + Coin dust = getDust(feeEstimationTransaction); Coin fee = feeEstimationTransaction.getFee().add(dust); - sendersAmount = feeExcluded ? amountAsCoin.add(fee) : amountAsCoin.add(dust); - Coin receiverAmount = feeExcluded ? amountAsCoin : amountAsCoin.subtract(fee); + Coin receiverAmount = Coin.ZERO; + // amountAsCoin is what the user typed into the withdrawal field. + // this can be interpreted as either the senders amount or receivers amount depending + // on a radio button "fee excluded / fee included". + // therefore we calculate the actual sendersAmount and receiverAmount as follows: + if (feeExcluded) { + receiverAmount = amountAsCoin; + sendersAmount = receiverAmount.add(fee); + } else { + sendersAmount = amountAsCoin.add(dust); // sendersAmount bumped up to UTXO size when dust is in play + receiverAmount = sendersAmount.subtract(fee); + } + if (dust.isPositive()) { + log.info("Dust output ({} satoshi) was detected, the dust amount has been added to the fee (was {}, now {})", + dust.value, + feeEstimationTransaction.getFee(), + fee.value); + } + if (areInputsValid()) { int txSize = feeEstimationTransaction.bitcoinSerialize().length; log.info("Fee for tx with size {}: {} " + Res.getBaseCurrencyCode() + "", txSize, fee.toPlainString()); @@ -342,15 +362,25 @@ private void onWithdraw() { if (receiverAmount.isPositive()) { double feePerByte = CoinUtil.getFeePerByte(fee, txSize); double kb = txSize / 1000d; + + String messageText = Res.get("shared.sendFundsDetailsWithFee", + formatter.formatCoinWithCode(sendersAmount), + withdrawFromTextField.getText(), + withdrawToTextField.getText(), + formatter.formatCoinWithCode(fee), + feePerByte, + kb, + formatter.formatCoinWithCode(receiverAmount)); + if (dust.isPositive()) { + messageText = MessageFormat.format( + "Bisq detected that this transaction would create a change output which is below the minimum dust threshold (and not allowed by Bitcoin consensus rules). Instead, this dust ({0} satoshi) will be added to the transaction fee.\n\n\n", + dust.value) + + messageText; + } + // jmacxx TODO: get review on the above message text, & clarification on proper way to add i18n resource strings + new Popup().headLine(Res.get("funds.withdrawal.confirmWithdrawalRequest")) - .confirmation(Res.get("shared.sendFundsDetailsWithFee", - formatter.formatCoinWithCode(sendersAmount), - withdrawFromTextField.getText(), - withdrawToTextField.getText(), - formatter.formatCoinWithCode(fee), - feePerByte, - kb, - formatter.formatCoinWithCode(receiverAmount))) + .confirmation(messageText) .actionButtonText(Res.get("shared.yes")) .onAction(() -> doWithdraw(sendersAmount, fee, new FutureCallback<>() { @Override @@ -632,12 +662,16 @@ public void updateItem(final WithdrawalListItem item, boolean empty) { }); } - private Coin getDust(Transaction tx) { + // BISQ issue #4039: prevent dust outputs from being created. + // check the outputs of a proposed transaction, if any are below the dust threshold + // add up the dust, noting the details in the log. + // returns the 'dust amount' to indicate if any dust was detected. + private Coin getDust(Transaction transaction) { Coin dust = Coin.ZERO; - for (TransactionOutput txo: tx.getOutputs()) { - if (txo.getValue().value < 546) { - dust = dust.add(txo.getValue()); - log.info("dust TXO = {}", txo.getValue().toFriendlyString()); + for (TransactionOutput transactionOutput: transaction.getOutputs()) { + if (transactionOutput.getValue().value < preferences.getIgnoreDustThreshold()) { + dust = dust.add(transactionOutput.getValue()); + log.info("dust TXO = {}", transactionOutput.toString()); } } return dust; From ea8b327f7be087299420db2011ddc5ee4cc9d1fc Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Sun, 29 Mar 2020 18:23:48 -0500 Subject: [PATCH 3/4] Change the constant used to determine what amount is considered dust Now using `Restrictions.getMinNonDustOutput()` which equates to 546 sats --- .../java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java index a84645c64a3..9e9381e3b2b 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java @@ -669,7 +669,7 @@ public void updateItem(final WithdrawalListItem item, boolean empty) { private Coin getDust(Transaction transaction) { Coin dust = Coin.ZERO; for (TransactionOutput transactionOutput: transaction.getOutputs()) { - if (transactionOutput.getValue().value < preferences.getIgnoreDustThreshold()) { + if (transactionOutput.getValue().isLessThan(Restrictions.getMinNonDustOutput())) { dust = dust.add(transactionOutput.getValue()); log.info("dust TXO = {}", transactionOutput.toString()); } From f1de0799013ada943cb2f498d3d402977d25b4a7 Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Mon, 30 Mar 2020 21:09:19 -0500 Subject: [PATCH 4/4] Change wording of the dust informative message per review by m52go Moved message text into displaystrings.properties --- core/src/main/resources/i18n/displayStrings.properties | 3 ++- .../bisq/desktop/main/funds/withdrawal/WithdrawalView.java | 6 ++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index 6b5d131672f..65ddaca4d51 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -122,7 +122,8 @@ shared.noDateAvailable=No date available shared.noDetailsAvailable=No details available shared.notUsedYet=Not used yet shared.date=Date -shared.sendFundsDetailsWithFee=Sending: {0}\nFrom address: {1}\nTo receiving address: {2}.\nRequired transaction fee is: {3} ({4} satoshis/byte)\nTransaction size: {5} Kb\n\nThe recipient will receive: {6}\n\nAre you sure you want to withdraw this amount? +shared.sendFundsDetailsWithFee=Sending: {0}\nFrom address: {1}\nTo receiving address: {2}.\nRequired mining fee is: {3} ({4} satoshis/byte)\nTransaction size: {5} Kb\n\nThe recipient will receive: {6}\n\nAre you sure you want to withdraw this amount? +shared.sendFundsDetailsDust=Bisq detected that this transaction would create a change output which is below the minimum dust threshold (and not allowed by Bitcoin consensus rules). Instead, this dust ({0} satoshi{1}) will be added to the mining fee.\n\n\n shared.copyToClipboard=Copy to clipboard shared.language=Language shared.country=Country diff --git a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java index 9e9381e3b2b..2539049b252 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java @@ -372,12 +372,10 @@ private void onWithdraw() { kb, formatter.formatCoinWithCode(receiverAmount)); if (dust.isPositive()) { - messageText = MessageFormat.format( - "Bisq detected that this transaction would create a change output which is below the minimum dust threshold (and not allowed by Bitcoin consensus rules). Instead, this dust ({0} satoshi) will be added to the transaction fee.\n\n\n", - dust.value) + messageText = Res.get("shared.sendFundsDetailsDust", + dust.value, dust.value > 1 ? "s" : "") + messageText; } - // jmacxx TODO: get review on the above message text, & clarification on proper way to add i18n resource strings new Popup().headLine(Res.get("funds.withdrawal.confirmWithdrawalRequest")) .confirmation(messageText)