Skip to content

Commit

Permalink
Ignore utxo below a user defined threshold to avoid dust attacks
Browse files Browse the repository at this point in the history 
Implements bisq-network#2604
  • Loading branch information
@ManfredKarrer
ManfredKarrer committed Apr 17, 2019
1 parent 21c5eec commit fcbc3b6
Show file tree
Hide file tree
Showing 16 changed files with 204 additions and 73 deletions.
1 change: 1 addition & 0 deletions common/src/main/proto/pb.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1351,6 +1351,7 @@ message PreferencesPayload {
string rpc_pw = 48;
string take_offer_selected_payment_account_id = 49;
double buyer_security_deposit_as_percent = 50;
int32 ignore_dust_threshold = 51;
}

///////////////////////////////////////////////////////////////////////////////////////////
Expand Down
26 changes: 15 additions & 11 deletions core/src/main/java/bisq/core/btc/wallet/BisqDefaultCoinSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,24 +75,28 @@ public CoinSelection select(Coin target, List<TransactionOutput> candidates) {
long total = 0;
long targetValue = target.value;
for (TransactionOutput output : sortedOutputs) {
if (total >= targetValue) {
long change = total - targetValue;
if (change == 0 || change >= Restrictions.getMinNonDustOutput().value)
break;
}

if (output.getParentTransaction() != null &&
isTxSpendable(output.getParentTransaction()) &&
isTxOutputSpendable(output)) {
selected.add(output);
total += output.getValue().value;
if (!isDustAttackUtxo(output)) {
if (total >= targetValue) {
long change = total - targetValue;
if (change == 0 || change >= Restrictions.getMinNonDustOutput().value)
break;
}

if (output.getParentTransaction() != null &&
isTxSpendable(output.getParentTransaction()) &&
isTxOutputSpendable(output)) {
selected.add(output);
total += output.getValue().value;
}
}
}
// Total may be lower than target here, if the given candidates were insufficient to create to requested
// transaction.
return new CoinSelection(Coin.valueOf(total), selected);
}

protected abstract boolean isDustAttackUtxo(TransactionOutput output);

public Coin getChange(Coin target, CoinSelection coinSelection) throws InsufficientMoneyException {
long value = target.value;
long available = coinSelection.valueGathered.value;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/bisq/core/btc/wallet/BsqCoinSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,11 @@ protected boolean isTxOutputSpendable(TransactionOutput output) {
// check if it is an own change output.
return unconfirmedBsqChangeOutputListService.hasTransactionOutput(output);
}

// For BSQ we do not check for dust attack utxos as they are 5.46 BSQ and a considerable value.
// The default 546 sat dust limit is handled in the BitcoinJ side anyway.
@Override
protected boolean isDustAttackUtxo(TransactionOutput output) {
return false;
}
}
7 changes: 7 additions & 0 deletions core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,4 +669,11 @@ public Address getUnusedAddress() {
public String getUnusedBsqAddressAsString() {
return "B" + getUnusedAddress().toBase58();
}

// For BSQ we do not check for dust attack utxos as they are 5.46 BSQ and a considerable value.
// The default 546 sat dust limit is handled in the BitcoinJ side anyway.
@Override
protected boolean isDustAttackUtxo(TransactionOutput output) {
return false;
}
}
30 changes: 19 additions & 11 deletions core/src/main/java/bisq/core/btc/wallet/BtcCoinSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,38 +24,38 @@

import java.util.Set;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import lombok.extern.slf4j.Slf4j;

/**
* We use a specialized version of the CoinSelector based on the DefaultCoinSelector implementation.
* We lookup for spendable outputs which matches any of our addresses.
*/
@Slf4j
class BtcCoinSelector extends BisqDefaultCoinSelector {
private static final Logger log = LoggerFactory.getLogger(BtcCoinSelector.class);

private final Set<Address> addresses;
private final long ignoreDustThreshold;


///////////////////////////////////////////////////////////////////////////////////////////
// Constructor
///////////////////////////////////////////////////////////////////////////////////////////

BtcCoinSelector(Set<Address> addresses, boolean permitForeignPendingTx) {
private BtcCoinSelector(Set<Address> addresses, long ignoreDustThreshold, boolean permitForeignPendingTx) {
super(permitForeignPendingTx);
this.addresses = addresses;
this.ignoreDustThreshold = ignoreDustThreshold;
}

BtcCoinSelector(Set<Address> addresses) {
this(addresses, true);
BtcCoinSelector(Set<Address> addresses, long ignoreDustThreshold) {
this(addresses, ignoreDustThreshold, true);
}

BtcCoinSelector(Address address, @SuppressWarnings("SameParameterValue") boolean permitForeignPendingTx) {
this(Sets.newHashSet(address), permitForeignPendingTx);
BtcCoinSelector(Address address, long ignoreDustThreshold, @SuppressWarnings("SameParameterValue") boolean permitForeignPendingTx) {
this(Sets.newHashSet(address), ignoreDustThreshold, permitForeignPendingTx);
}

BtcCoinSelector(Address address) {
this(Sets.newHashSet(address), true);
BtcCoinSelector(Address address, long ignoreDustThreshold) {
this(Sets.newHashSet(address), ignoreDustThreshold, true);
}

@Override
Expand All @@ -68,4 +68,12 @@ protected boolean isTxOutputSpendable(TransactionOutput output) {
return false;
}
}

// We ignore utxos which are considered dust attacks for spying on users wallets.
// The ignoreDustThreshold value is set in the preferences. If not set we use default non dust
// value of 546 sat.
@Override
protected boolean isDustAttackUtxo(TransactionOutput output) {
return output.getValue().value < ignoreDustThreshold;
}
}
56 changes: 35 additions & 21 deletions core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,17 +202,18 @@ private Transaction completePreparedProposalTx(Transaction feeTx, byte[] opRetur
// safety check counter to avoid endless loops
int counter = 0;
// estimated size of input sig
final int sigSizePerInput = 106;
int sigSizePerInput = 106;
// typical size for a tx with 3 inputs
int txSizeWithUnsignedInputs = 300;
final Coin txFeePerByte = feeService.getTxFeePerByte();
Coin txFeePerByte = feeService.getTxFeePerByte();

Address changeAddress = getFreshAddressEntry().getAddress();
checkNotNull(changeAddress, "changeAddress must not be null");

final BtcCoinSelector coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE));
final List<TransactionInput> preparedBsqTxInputs = preparedTx.getInputs();
final List<TransactionOutput> preparedBsqTxOutputs = preparedTx.getOutputs();
BtcCoinSelector coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE),
preferences.getIgnoreDustThreshold());
List<TransactionInput> preparedBsqTxInputs = preparedTx.getInputs();
List<TransactionOutput> preparedBsqTxOutputs = preparedTx.getOutputs();
int numInputs = preparedBsqTxInputs.size();
Transaction resultTx = null;
boolean isFeeOutsideTolerance;
Expand Down Expand Up @@ -309,17 +310,18 @@ private Transaction addInputsForMinerFee(Transaction preparedTx, byte[] opReturn
// safety check counter to avoid endless loops
int counter = 0;
// estimated size of input sig
final int sigSizePerInput = 106;
int sigSizePerInput = 106;
// typical size for a tx with 3 inputs
int txSizeWithUnsignedInputs = 300;
final Coin txFeePerByte = feeService.getTxFeePerByte();
Coin txFeePerByte = feeService.getTxFeePerByte();

Address changeAddress = getFreshAddressEntry().getAddress();
checkNotNull(changeAddress, "changeAddress must not be null");

final BtcCoinSelector coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE));
final List<TransactionInput> preparedBsqTxInputs = preparedTx.getInputs();
final List<TransactionOutput> preparedBsqTxOutputs = preparedTx.getOutputs();
BtcCoinSelector coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE),
preferences.getIgnoreDustThreshold());
List<TransactionInput> preparedBsqTxInputs = preparedTx.getInputs();
List<TransactionOutput> preparedBsqTxOutputs = preparedTx.getOutputs();
int numInputs = preparedBsqTxInputs.size();
Transaction resultTx = null;
boolean isFeeOutsideTolerance;
Expand Down Expand Up @@ -444,20 +446,21 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx, boolean useC
// safety check counter to avoid endless loops
int counter = 0;
// estimated size of input sig
final int sigSizePerInput = 106;
int sigSizePerInput = 106;
// typical size for a tx with 2 inputs
int txSizeWithUnsignedInputs = 203;
// If useCustomTxFee we allow overriding the estimated fee from preferences
final Coin txFeePerByte = useCustomTxFee ? getTxFeeForWithdrawalPerByte() : feeService.getTxFeePerByte();
Coin txFeePerByte = useCustomTxFee ? getTxFeeForWithdrawalPerByte() : feeService.getTxFeePerByte();
// In case there are no change outputs we force a change by adding min dust to the BTC input
Coin forcedChangeValue = Coin.ZERO;

Address changeAddress = getFreshAddressEntry().getAddress();
checkNotNull(changeAddress, "changeAddress must not be null");

final BtcCoinSelector coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE));
final List<TransactionInput> preparedBsqTxInputs = preparedBsqTx.getInputs();
final List<TransactionOutput> preparedBsqTxOutputs = preparedBsqTx.getOutputs();
BtcCoinSelector coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE),
preferences.getIgnoreDustThreshold());
List<TransactionInput> preparedBsqTxInputs = preparedBsqTx.getInputs();
List<TransactionOutput> preparedBsqTxOutputs = preparedBsqTx.getOutputs();
int numInputs = preparedBsqTxInputs.size() + 1; // We add 1 for the BTC fee input
Transaction resultTx = null;
boolean isFeeOutsideTolerance;
Expand Down Expand Up @@ -781,7 +784,7 @@ public void doubleSpendTransaction(String txId, Runnable resultHandler, ErrorMes
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;
sendRequest.aesKey = aesKey;
sendRequest.coinSelector = new BtcCoinSelector(toAddress);
sendRequest.coinSelector = new BtcCoinSelector(toAddress, preferences.getIgnoreDustThreshold());
sendRequest.changeAddress = toAddress;
wallet.completeTx(sendRequest);
tx = sendRequest.tx;
Expand All @@ -802,7 +805,7 @@ public void doubleSpendTransaction(String txId, Runnable resultHandler, ErrorMes
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;
sendRequest.aesKey = aesKey;
sendRequest.coinSelector = new BtcCoinSelector(toAddress);
sendRequest.coinSelector = new BtcCoinSelector(toAddress, preferences.getIgnoreDustThreshold());
sendRequest.changeAddress = toAddress;
sendResult = wallet.sendCoins(sendRequest);
} catch (InsufficientMoneyException e) {
Expand All @@ -818,7 +821,8 @@ public void doubleSpendTransaction(String txId, Runnable resultHandler, ErrorMes
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;
sendRequest.aesKey = aesKey;
sendRequest.coinSelector = new BtcCoinSelector(toAddress, false);
sendRequest.coinSelector = new BtcCoinSelector(toAddress,
preferences.getIgnoreDustThreshold(), false);
sendRequest.changeAddress = toAddress;

try {
Expand Down Expand Up @@ -972,7 +976,8 @@ public int getEstimatedFeeTxSize(List<Coin> outputValues, Coin txFee)
SendRequest sendRequest = SendRequest.forTx(transaction);
sendRequest.shuffleOutputs = false;
sendRequest.aesKey = aesKey;
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE));
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE),
preferences.getIgnoreDustThreshold());
sendRequest.fee = txFee;
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;
Expand Down Expand Up @@ -1044,7 +1049,7 @@ private SendRequest getSendRequest(String fromAddress,

checkNotNull(addressEntry.get(), "addressEntry.get() must not be null");
checkNotNull(addressEntry.get().getAddress(), "addressEntry.get().getAddress() must not be null");
sendRequest.coinSelector = new BtcCoinSelector(addressEntry.get().getAddress());
sendRequest.coinSelector = new BtcCoinSelector(addressEntry.get().getAddress(), preferences.getIgnoreDustThreshold());
sendRequest.changeAddress = addressEntry.get().getAddress();
return sendRequest;
}
Expand Down Expand Up @@ -1089,7 +1094,8 @@ private SendRequest getSendRequestForMultipleAddresses(Set<String> fromAddresses
if (addressEntries.isEmpty())
throw new AddressEntryException("No Addresses for withdraw found in our wallet");

sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesFromAddressEntries(addressEntries));
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesFromAddressEntries(addressEntries),
preferences.getIgnoreDustThreshold());
Optional<AddressEntry> addressEntryOptional = Optional.<AddressEntry>empty();
AddressEntry changeAddressAddressEntry = null;
if (changeAddress != null)
Expand All @@ -1100,4 +1106,12 @@ private SendRequest getSendRequestForMultipleAddresses(Set<String> fromAddresses
sendRequest.changeAddress = changeAddressAddressEntry.getAddress();
return sendRequest;
}

// We ignore utxos which are considered dust attacks for spying on users wallets.
// The ignoreDustThreshold value is set in the preferences. If not set we use default non dust
// value of 546 sat.
@Override
protected boolean isDustAttackUtxo(TransactionOutput output) {
return output.getValue().value < preferences.getIgnoreDustThreshold();
}
}
6 changes: 6 additions & 0 deletions core/src/main/java/bisq/core/btc/wallet/NonBsqCoinSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,10 @@ protected boolean isTxOutputSpendable(TransactionOutput output) {
// So we consider any txOutput which is not in the state as BTC output.
return !daoStateService.existsTxOutput(key) || daoStateService.isRejectedIssuanceOutput(key);
}

// BTC utxo in the BSQ wallet are usually from rejected comp request so we don't expect dust attack utxos here.
@Override
protected boolean isDustAttackUtxo(TransactionOutput output) {
return false;
}
}
27 changes: 17 additions & 10 deletions core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import bisq.core.btc.setup.WalletConfig;
import bisq.core.btc.setup.WalletsSetup;
import bisq.core.locale.Res;
import bisq.core.user.Preferences;

import org.bitcoinj.core.Address;
import org.bitcoinj.core.AddressFormatException;
Expand Down Expand Up @@ -115,6 +116,7 @@ public class TradeWalletService {
private static final Logger log = LoggerFactory.getLogger(TradeWalletService.class);

private final WalletsSetup walletsSetup;
private final Preferences preferences;
private final NetworkParameters params;

@Nullable
Expand All @@ -130,8 +132,9 @@ public class TradeWalletService {
///////////////////////////////////////////////////////////////////////////////////////////

@Inject
public TradeWalletService(WalletsSetup walletsSetup) {
public TradeWalletService(WalletsSetup walletsSetup, Preferences preferences) {
this.walletsSetup = walletsSetup;
this.preferences = preferences;
this.params = BisqEnvironment.getParameters();
walletsSetup.addSetupCompletedHandler(() -> {
walletConfig = walletsSetup.getWalletConfig();
Expand Down Expand Up @@ -198,10 +201,12 @@ public Transaction createBtcTradingFeeTx(Address fundingAddress,
sendRequest = SendRequest.forTx(tradingFeeTx);
sendRequest.shuffleOutputs = false;
sendRequest.aesKey = aesKey;
if (useSavingsWallet)
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE));
else
sendRequest.coinSelector = new BtcCoinSelector(fundingAddress);
if (useSavingsWallet) {
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE),
preferences.getIgnoreDustThreshold());
} else {
sendRequest.coinSelector = new BtcCoinSelector(fundingAddress, preferences.getIgnoreDustThreshold());
}
// We use a fixed fee

sendRequest.fee = txFee;
Expand Down Expand Up @@ -278,10 +283,12 @@ public Transaction completeBsqTradingFeeTx(Transaction preparedBsqTx,
SendRequest sendRequest = SendRequest.forTx(preparedBsqTx);
sendRequest.shuffleOutputs = false;
sendRequest.aesKey = aesKey;
if (useSavingsWallet)
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE));
else
sendRequest.coinSelector = new BtcCoinSelector(fundingAddress);
if (useSavingsWallet) {
sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE),
preferences.getIgnoreDustThreshold());
} else {
sendRequest.coinSelector = new BtcCoinSelector(fundingAddress, preferences.getIgnoreDustThreshold());
}
// We use a fixed fee
sendRequest.fee = txFee;
sendRequest.feePerKb = Coin.ZERO;
Expand Down Expand Up @@ -1196,7 +1203,7 @@ private void addAvailableInputsAndChangeOutputs(Transaction transaction, Address
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;
// we allow spending of unconfirmed tx (double spend risk is low and usability would suffer if we need to wait for 1 confirmation)
sendRequest.coinSelector = new BtcCoinSelector(address);
sendRequest.coinSelector = new BtcCoinSelector(address, preferences.getIgnoreDustThreshold());
// We use always the same address in a trade for all transactions
sendRequest.changeAddress = changeAddress;
// With the usage of completeTx() we get all the work done with fee calculation, validation and coin selection.
Expand Down
12 changes: 8 additions & 4 deletions core/src/main/java/bisq/core/btc/wallet/WalletService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,14 +423,18 @@ public Coin getBalanceForAddress(Address address) {
protected Coin getBalance(List<TransactionOutput> transactionOutputs, Address address) {
Coin balance = Coin.ZERO;
for (TransactionOutput output : transactionOutputs) {
if (isOutputScriptConvertibleToAddress(output) &&
address != null &&
address.equals(getAddressFromOutput(output)))
balance = balance.add(output.getValue());
if (!isDustAttackUtxo(output)) {
if (isOutputScriptConvertibleToAddress(output) &&
address != null &&
address.equals(getAddressFromOutput(output)))
balance = balance.add(output.getValue());
}
}
return balance;
}

protected abstract boolean isDustAttackUtxo(TransactionOutput output);

public Coin getBalance(TransactionOutput output) {
return getBalanceForAddress(getAddressFromOutput(output));
}
Expand Down
Loading

0 comments on commit fcbc3b6

Please sign in to comment.