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.

Sign up for GitHub

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 that a wrong tx is set as deposit tx #4883

Merged
merged 2 commits into from
Dec 3, 2020
Merged

Prevent that a wrong tx is set as deposit tx #4883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a39f2e8
Apply project format rules (no code change)
chimp1984 Dec 3, 2020
29d757b
- revent that a wrong tx is set as deposit tx
chimp1984 Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/app/BisqSetup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ private void checkForLockedUpFunds() {
.filter(e -> setOfAllTradeIds.contains(e.getOfferId()) &&
e.getContext() == AddressEntry.Context.MULTI_SIG)
.forEach(e -> {
Coin balance = e.getCoinLockedInMultiSig();
Coin balance = e.getCoinLockedInMultiSigAsCoin();
if (balance.isPositive()) {
String message = Res.get("popup.warning.lockedUpFunds",
formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/btc/Balances.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ private void updateLockedBalance() {
long sum = lockedTrades.map(trade -> btcWalletService.getAddressEntry(trade.getId(), AddressEntry.Context.MULTI_SIG)
.orElse(null))
.filter(Objects::nonNull)
.mapToLong(addressEntry -> addressEntry.getCoinLockedInMultiSig().getValue())
.mapToLong(AddressEntry::getCoinLockedInMultiSig)
.sum();
lockedBalance.set(Coin.valueOf(sum));
}
Expand Down
50 changes: 31 additions & 19 deletions core/src/main/java/bisq/core/btc/model/AddressEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,19 @@ public enum Context {
private final byte[] pubKey;
@Getter
private final byte[] pubKeyHash;

private long coinLockedInMultiSig;

@Getter
private boolean segwit;
private final long coinLockedInMultiSig;
@Getter
private final boolean segwit;

// Not an immutable field. Set at startup once wallet is ready and at encrypting/decrypting wallet.
@Nullable
transient private DeterministicKey keyPair;

// Only used as cache
@Nullable
transient private Address address;
// Only used as cache
@Nullable
transient private String addressString;

Expand All @@ -93,16 +96,29 @@ public AddressEntry(DeterministicKey keyPair, Context context, boolean segwit) {
this(keyPair, context, null, segwit);
}

public AddressEntry(@NotNull DeterministicKey keyPair,
public AddressEntry(DeterministicKey keyPair,
Context context,
@Nullable String offerId,
boolean segwit) {
this(keyPair,
context,
offerId,
0,
segwit);
}

public AddressEntry(DeterministicKey keyPair,
Context context,
@Nullable String offerId,
long coinLockedInMultiSig,
boolean segwit) {
this(keyPair.getPubKey(),
keyPair.getPubKeyHash(),
context,
offerId,
coinLockedInMultiSig,
segwit);
this.keyPair = keyPair;
this.context = context;
this.offerId = offerId;
pubKey = keyPair.getPubKey();
pubKeyHash = keyPair.getPubKeyHash();
this.segwit = segwit;
}


Expand All @@ -114,13 +130,13 @@ private AddressEntry(byte[] pubKey,
byte[] pubKeyHash,
Context context,
@Nullable String offerId,
Coin coinLockedInMultiSig,
long coinLockedInMultiSig,
boolean segwit) {
this.pubKey = pubKey;
this.pubKeyHash = pubKeyHash;
this.context = context;
this.offerId = offerId;
this.coinLockedInMultiSig = coinLockedInMultiSig.value;
this.coinLockedInMultiSig = coinLockedInMultiSig;
this.segwit = segwit;
}

Expand All @@ -129,7 +145,7 @@ public static AddressEntry fromProto(protobuf.AddressEntry proto) {
proto.getPubKeyHash().toByteArray(),
ProtoUtil.enumFromProto(AddressEntry.Context.class, proto.getContext().name()),
ProtoUtil.stringOrNullFromProto(proto.getOfferId()),
Coin.valueOf(proto.getCoinLockedInMultiSig()),
proto.getCoinLockedInMultiSig(),
proto.getSegwit());
}

Expand Down Expand Up @@ -164,10 +180,6 @@ public DeterministicKey getKeyPair() {
return keyPair;
}

public void setCoinLockedInMultiSig(@NotNull Coin coinLockedInMultiSig) {
this.coinLockedInMultiSig = coinLockedInMultiSig.value;
}

// For display we usually only display the first 8 characters.
@Nullable
public String getShortOfferId() {
Expand Down Expand Up @@ -196,14 +208,14 @@ public boolean isTrade() {
return context == Context.MULTI_SIG || context == Context.TRADE_PAYOUT;
}

public Coin getCoinLockedInMultiSig() {
public Coin getCoinLockedInMultiSigAsCoin() {
return Coin.valueOf(coinLockedInMultiSig);
}

@Override
public String toString() {
return "AddressEntry{" +
"address=" + address +
"address=" + getAddress() +
", context=" + context +
", offerId='" + offerId + '\'' +
", coinLockedInMultiSig=" + coinLockedInMultiSig +
Expand Down
27 changes: 27 additions & 0 deletions core/src/main/java/bisq/core/btc/model/AddressEntryList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,15 @@ public void addAddressEntry(AddressEntry addressEntry) {
}

public void swapToAvailable(AddressEntry addressEntry) {
if (addressEntry.getContext() == AddressEntry.Context.MULTI_SIG) {
log.error("swapToAvailable called with an addressEntry with MULTI_SIG context. " +
"This in not permitted as we must not reuse those address entries and there are " +
"no redeemable funds on those addresses. " +
"Only the keys are used for creating the Multisig address. " +
"addressEntry={}", addressEntry);
return;
}

boolean setChangedByRemove = entrySet.remove(addressEntry);
boolean setChangedByAdd = entrySet.add(new AddressEntry(addressEntry.getKeyPair(),
AddressEntry.Context.AVAILABLE,
Expand All @@ -217,6 +226,24 @@ public AddressEntry swapAvailableToAddressEntryWithOfferId(AddressEntry addressE
return newAddressEntry;
}

public void setCoinLockedInMultiSigAddressEntry(AddressEntry addressEntry, long value) {
if (addressEntry.getContext() != AddressEntry.Context.MULTI_SIG) {
log.error("setCoinLockedInMultiSigAddressEntry must be called only on MULTI_SIG entries");
return;
}

boolean setChangedByRemove = entrySet.remove(addressEntry);
AddressEntry entry = new AddressEntry(addressEntry.getKeyPair(),
addressEntry.getContext(),
addressEntry.getOfferId(),
value,
addressEntry.isSegwit());
boolean setChangedByAdd = entrySet.add(entry);
if (setChangedByRemove || setChangedByAdd) {
requestPersistence();
}
}

public void requestPersistence() {
persistenceManager.requestPersistence();
}
Expand Down
58 changes: 43 additions & 15 deletions core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,8 @@ private Transaction completePreparedProposalTx(Transaction feeTx, byte[] opRetur
sendRequest.signInputs = false;

sendRequest.fee = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs +
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4);
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4);

sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;
Expand All @@ -274,8 +274,8 @@ private Transaction completePreparedProposalTx(Transaction feeTx, byte[] opRetur
numSegwitInputs = numInputs.second;
txVsizeWithUnsignedInputs = resultTx.getVsize();
long estimatedFeeAsLong = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs +
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4).value;
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4).value;

// calculated fee must be inside of a tolerance range with tx fee
isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000;
Expand Down Expand Up @@ -374,8 +374,8 @@ private Transaction addInputsForMinerFee(Transaction preparedTx,
sendRequest.signInputs = false;

sendRequest.fee = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs +
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4);
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4);
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;

Expand All @@ -393,8 +393,8 @@ private Transaction addInputsForMinerFee(Transaction preparedTx,
numSegwitInputs = numInputs.second;
txVsizeWithUnsignedInputs = resultTx.getVsize();
final long estimatedFeeAsLong = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs +
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4).value;
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4).value;
// calculated fee must be inside of a tolerance range with tx fee
isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000;
}
Expand Down Expand Up @@ -532,8 +532,8 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx,
sendRequest.signInputs = false;

sendRequest.fee = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs +
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4);
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4);
sendRequest.feePerKb = Coin.ZERO;
sendRequest.ensureMinRequiredFee = false;

Expand All @@ -558,8 +558,8 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx,
numSegwitInputs = numInputs.second;
txVsizeWithUnsignedInputs = resultTx.getVsize();
final long estimatedFeeAsLong = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs +
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4).value;
sigSizePerInput * numLegacyInputs +
sigSizePerInput * numSegwitInputs / 4).value;
// calculated fee must be inside of a tolerance range with tx fee
isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000;
}
Expand All @@ -583,7 +583,7 @@ private Tuple2<Integer, Integer> getNumInputs(Transaction tx) {
for (TransactionInput input : tx.getInputs()) {
TransactionOutput connectedOutput = input.getConnectedOutput();
if (connectedOutput == null || ScriptPattern.isP2PKH(connectedOutput.getScriptPubKey()) ||
ScriptPattern.isP2PK(connectedOutput.getScriptPubKey())) {
ScriptPattern.isP2PK(connectedOutput.getScriptPubKey())) {
// If connectedOutput is null, we don't know here the input type. To avoid underpaying fees,
// we treat it as a legacy input which will result in a higher fee estimation.
numLegacyInputs++;
Expand Down Expand Up @@ -738,6 +738,14 @@ public List<AddressEntry> getAddressEntryListAsImmutableList() {
}

public void swapTradeEntryToAvailableEntry(String offerId, AddressEntry.Context context) {
if (context == AddressEntry.Context.MULTI_SIG) {
log.error("swapTradeEntryToAvailableEntry called with MULTI_SIG context. " +
"This in not permitted as we must not reuse those address entries and there " +
"are no redeemable funds on that addresses. Only the keys are used for creating " +
"the Multisig address. offerId={}, context={}", offerId, context);
return;
}

getAddressEntryListAsImmutableList().stream()
.filter(e -> offerId.equals(e.getOfferId()))
.filter(e -> context == e.getContext())
Expand All @@ -748,15 +756,35 @@ public void swapTradeEntryToAvailableEntry(String offerId, AddressEntry.Context
});
}

// When funds from MultiSig address is spent we reset the coinLockedInMultiSig value to 0.
public void resetCoinLockedInMultiSigAddressEntry(String offerId) {
setCoinLockedInMultiSigAddressEntry(offerId, 0);
}

public void setCoinLockedInMultiSigAddressEntry(String offerId, long value) {
getAddressEntryListAsImmutableList().stream()
.filter(e -> AddressEntry.Context.MULTI_SIG == e.getContext())
.filter(e -> offerId.equals(e.getOfferId()))
.forEach(addressEntry -> setCoinLockedInMultiSigAddressEntry(addressEntry, value));
}

public void setCoinLockedInMultiSigAddressEntry(AddressEntry addressEntry, long value) {
log.info("Set coinLockedInMultiSig for addressEntry {} to value {}", addressEntry, value);
addressEntryList.setCoinLockedInMultiSigAddressEntry(addressEntry, value);
}

public void resetAddressEntriesForOpenOffer(String offerId) {
log.info("resetAddressEntriesForOpenOffer offerId={}", offerId);
swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.OFFER_FUNDING);
swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.RESERVED_FOR_TRADE);
}

public void resetAddressEntriesForPendingTrade(String offerId) {
swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.MULTI_SIG);
// We swap also TRADE_PAYOUT to be sure all is cleaned up. There might be cases where a user cannot send the funds
// We must not swap MULTI_SIG entries as those addresses are not detected in the isAddressUnused
// check at getOrCreateAddressEntry and could lead to a reuse of those keys and result in the same 2of2 MS
// address if same peers trade again.

// We swap TRADE_PAYOUT to be sure all is cleaned up. There might be cases where a user cannot send the funds
// to an external wallet directly in the last step of the trade, but the funds are in the Bisq wallet anyway and
// the dealing with the external wallet is pure UI thing. The user can move the funds to the wallet and then
// send out the funds to the external wallet. As this cleanup is a rare situation and most users do not use
Expand Down
8 changes: 2 additions & 6 deletions core/src/main/java/bisq/core/trade/protocol/tasks/SetupPayoutTxListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public void onTransactionConfidenceChanged(TransactionConfidence confidence) {

tradeStateSubscription = EasyBind.subscribe(trade.stateProperty(), newValue -> {
if (trade.isPayoutPublished()) {
swapMultiSigEntry();
processModel.getBtcWalletService().resetCoinLockedInMultiSigAddressEntry(trade.getId());

// hack to remove tradeStateSubscription at callback
UserThread.execute(this::unSubscribe);
Expand All @@ -98,16 +98,12 @@ private void applyConfidence(TransactionConfidence confidence) {
log.info("We had the payout tx already set. tradeId={}, state={}", trade.getId(), trade.getState());
}

swapMultiSigEntry();
processModel.getBtcWalletService().resetCoinLockedInMultiSigAddressEntry(trade.getId());

// need delay as it can be called inside the handler before the listener and tradeStateSubscription are actually set.
UserThread.execute(this::unSubscribe);
}

private void swapMultiSigEntry() {
processModel.getBtcWalletService().swapTradeEntryToAvailableEntry(trade.getId(), AddressEntry.Context.MULTI_SIG);
}

private boolean isInNetwork(TransactionConfidence confidence) {
return confidence != null &&
(confidence.getConfidenceType().equals(TransactionConfidence.ConfidenceType.BUILDING) ||
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/bisq/core/trade/protocol/tasks/arbitration/PublishedDelayedPayoutTx.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package bisq.core.trade.protocol.tasks.arbitration;

import bisq.core.btc.exceptions.TxBroadcastException;
import bisq.core.btc.model.AddressEntry;
import bisq.core.btc.wallet.BtcWalletService;
import bisq.core.btc.wallet.TxBroadcaster;
import bisq.core.btc.wallet.WalletService;
Expand Down Expand Up @@ -46,7 +45,7 @@ protected void run() {
BtcWalletService btcWalletService = processModel.getBtcWalletService();

// We have spent the funds from the deposit tx with the delayedPayoutTx
btcWalletService.swapTradeEntryToAvailableEntry(trade.getId(), AddressEntry.Context.MULTI_SIG);
btcWalletService.resetCoinLockedInMultiSigAddressEntry(trade.getId());
// We might receive funds on AddressEntry.Context.TRADE_PAYOUT so we don't swap that

Transaction committedDelayedPayoutTx = WalletService.maybeAddSelfTxToWallet(delayedPayoutTx, btcWalletService.getWallet());
Expand Down
3 changes: 1 addition & 2 deletions .../main/java/bisq/core/trade/protocol/tasks/buyer/BuyerProcessPayoutTxPublishedMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package bisq.core.trade.protocol.tasks.buyer;

import bisq.core.account.sign.SignedWitness;
import bisq.core.btc.model.AddressEntry;
import bisq.core.btc.wallet.BtcWalletService;
import bisq.core.btc.wallet.WalletService;
import bisq.core.trade.Trade;
Expand Down Expand Up @@ -60,7 +59,7 @@ protected void run() {
BtcWalletService.printTx("payoutTx received from peer", committedPayoutTx);

trade.setState(Trade.State.BUYER_RECEIVED_PAYOUT_TX_PUBLISHED_MSG);
processModel.getBtcWalletService().swapTradeEntryToAvailableEntry(trade.getId(), AddressEntry.Context.MULTI_SIG);
processModel.getBtcWalletService().resetCoinLockedInMultiSigAddressEntry(trade.getId());
} else {
log.info("We got the payout tx already set from BuyerSetupPayoutTxListener and do nothing here. trade ID={}", trade.getId());
}
Expand Down
Loading