From a39f2e8fcb1867eeafe155eaed10033c98157fe8 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Thu, 3 Dec 2020 09:54:43 -0500 Subject: [PATCH 1/2] Apply project format rules (no code change) --- .../core/btc/wallet/BtcWalletService.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java index 3d45c491878..d5726f10f0f 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -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; @@ -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; @@ -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; @@ -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; } @@ -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; @@ -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; } @@ -583,7 +583,7 @@ private Tuple2 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++; From 29d757b74196c803e967fe4181d764bb11945cf0 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Thu, 3 Dec 2020 10:01:01 -0500 Subject: [PATCH 2/2] - revent that a wrong tx is set as deposit tx We check if the txIds of the inputs matches our maker fee tx and taker fee tx and if the depositTxAddress we use for the confidence lookup is use as an output address. This prevents that past txs which have the our depositTxAddress as input or output (deposit or payout txs) could be interpreted as our deposit tx. This happened because if a bug which caused re-use of the Multisig address entries and if both traders use the same key for multiple trades the depositTxAddress would be the same. We fix that bug as well but we also need to avoid that past already used addresses might be taken again (the Multisig flag got reverted to available in the address entry). - Add check to swapTradeEntryToAvailableEntry to not swap MULTI_SIG entries. - Remove swap for MULTI_SIG entries at resetAddressEntriesForPendingTrade - Add check to swapToAvailable to not swap MULTI_SIG entries. - Remove swaps for MULTI_SIG entries - Add setCoinLockedInMultiSigAddressEntry method - Make coinLockedInMultiSig final and remove setter but use it in constructor. - Rename getCoinLockedInMultiSig to getCoinLockedInMultiSigAsCoin We use an immutable list when operating on AddressEntry so changes on the object would not be reflected in the list. The only mutable field (beside non critical cache fields) is the keyPair. Might be good to refactor that as well at some point. - Add setCoinLockedInMultiSigAddressEntrymethods - Apply API changes: -- resetCoinLockedInMultiSigAddressEntry -- setCoinLockedInMultiSigAddressEntry -- renamed methods --- .../main/java/bisq/core/app/BisqSetup.java | 2 +- .../src/main/java/bisq/core/btc/Balances.java | 2 +- .../bisq/core/btc/model/AddressEntry.java | 50 ++++++++------ .../bisq/core/btc/model/AddressEntryList.java | 27 ++++++++ .../core/btc/wallet/BtcWalletService.java | 32 ++++++++- .../protocol/tasks/SetupPayoutTxListener.java | 8 +-- .../arbitration/PublishedDelayedPayoutTx.java | 3 +- .../BuyerProcessPayoutTxPublishedMessage.java | 3 +- .../buyer/BuyerSetupDepositTxListener.java | 67 ++++++++++++++++++- .../BuyerAsMakerCreatesAndSignsDepositTx.java | 2 +- .../BuyerAsTakerSignsDepositTx.java | 3 +- .../mediation/FinalizeMediatedPayoutTx.java | 2 +- ...ocessMediatedPayoutTxPublishedMessage.java | 3 +- .../seller/SellerSignAndFinalizePayoutTx.java | 2 +- ...SellerAsMakerCreatesUnsignedDepositTx.java | 3 +- .../SellerAsTakerSignsDepositTx.java | 3 +- .../main/funds/locked/LockedListItem.java | 2 +- 17 files changed, 170 insertions(+), 44 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index 925b5e793a1..f6ef065f2f6 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -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()); diff --git a/core/src/main/java/bisq/core/btc/Balances.java b/core/src/main/java/bisq/core/btc/Balances.java index e162d547075..5d3ddd4e437 100644 --- a/core/src/main/java/bisq/core/btc/Balances.java +++ b/core/src/main/java/bisq/core/btc/Balances.java @@ -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)); } diff --git a/core/src/main/java/bisq/core/btc/model/AddressEntry.java b/core/src/main/java/bisq/core/btc/model/AddressEntry.java index 5348950b391..c8804fa29a1 100644 --- a/core/src/main/java/bisq/core/btc/model/AddressEntry.java +++ b/core/src/main/java/bisq/core/btc/model/AddressEntry.java @@ -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; @@ -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; } @@ -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; } @@ -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()); } @@ -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() { @@ -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 + diff --git a/core/src/main/java/bisq/core/btc/model/AddressEntryList.java b/core/src/main/java/bisq/core/btc/model/AddressEntryList.java index ea25be37756..4b41e1c275b 100644 --- a/core/src/main/java/bisq/core/btc/model/AddressEntryList.java +++ b/core/src/main/java/bisq/core/btc/model/AddressEntryList.java @@ -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, @@ -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(); } diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java index d5726f10f0f..c139c0f642e 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -738,6 +738,14 @@ public List 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()) @@ -748,6 +756,23 @@ 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); @@ -755,8 +780,11 @@ public void resetAddressEntriesForOpenOffer(String offerId) { } 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 diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/SetupPayoutTxListener.java b/core/src/main/java/bisq/core/trade/protocol/tasks/SetupPayoutTxListener.java index 3c264335535..5d925ae41c2 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/SetupPayoutTxListener.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/SetupPayoutTxListener.java @@ -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); @@ -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) || diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/arbitration/PublishedDelayedPayoutTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/arbitration/PublishedDelayedPayoutTx.java index 005dd886115..b35e1f53a62 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/arbitration/PublishedDelayedPayoutTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/arbitration/PublishedDelayedPayoutTx.java @@ -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; @@ -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()); diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerProcessPayoutTxPublishedMessage.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerProcessPayoutTxPublishedMessage.java index 71c250b7b91..5b546201afb 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerProcessPayoutTxPublishedMessage.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerProcessPayoutTxPublishedMessage.java @@ -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; @@ -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()); } diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerSetupDepositTxListener.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerSetupDepositTxListener.java index f78156ef5e0..42b8ff4baa9 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerSetupDepositTxListener.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerSetupDepositTxListener.java @@ -28,14 +28,21 @@ import org.bitcoinj.core.Address; import org.bitcoinj.core.NetworkParameters; +import org.bitcoinj.core.Sha256Hash; import org.bitcoinj.core.Transaction; import org.bitcoinj.core.TransactionConfidence; +import org.bitcoinj.core.TransactionInput; +import org.bitcoinj.core.TransactionOutPoint; import org.fxmisc.easybind.EasyBind; import org.fxmisc.easybind.Subscription; +import java.util.Objects; + import lombok.extern.slf4j.Slf4j; +import javax.annotation.Nullable; + import static com.google.common.base.Preconditions.checkArgument; @Slf4j @@ -59,14 +66,20 @@ protected void run() { Transaction preparedDepositTx = new Transaction(params, processModel.getPreparedDepositTx()); checkArgument(!preparedDepositTx.getOutputs().isEmpty(), "preparedDepositTx.getOutputs() must not be empty"); Address depositTxAddress = preparedDepositTx.getOutput(0).getScriptPubKey().getToAddress(params); + + // For buyer as maker takerFeeTxId is null + @Nullable String takerFeeTxId = trade.getTakerFeeTxId(); + String makerFeeTxId = trade.getOffer().getOfferFeePaymentTxId(); TransactionConfidence confidence = walletService.getConfidenceForAddress(depositTxAddress); - if (isVisibleInNetwork(confidence)) { + if (isConfTxDepositTx(confidence, params, depositTxAddress, takerFeeTxId, makerFeeTxId) && + isVisibleInNetwork(confidence)) { applyConfidence(confidence); } else { confidenceListener = new AddressConfidenceListener(depositTxAddress) { @Override public void onTransactionConfidenceChanged(TransactionConfidence confidence) { - if (isVisibleInNetwork(confidence)) { + if (isConfTxDepositTx(confidence, params, depositTxAddress, + takerFeeTxId, makerFeeTxId) && isVisibleInNetwork(confidence)) { applyConfidence(confidence); } } @@ -91,6 +104,56 @@ public void onTransactionConfidenceChanged(TransactionConfidence confidence) { } } + // We check if the txIds of the inputs matches our maker fee tx and taker fee tx and if the depositTxAddress we + // use for the confidence lookup is use as an output address. + // This prevents that past txs which have the our depositTxAddress as input or output (deposit or payout txs) could + // be interpreted as our deposit tx. This happened because if a bug which caused re-use of the Multisig address + // entries and if both traders use the same key for multiple trades the depositTxAddress would be the same. + // We fix that bug as well but we also need to avoid that past already used addresses might be taken again + // (the Multisig flag got reverted to available in the address entry). + private boolean isConfTxDepositTx(@Nullable TransactionConfidence confidence, + NetworkParameters params, + Address depositTxAddress, + @Nullable String takerFeeTxId, + String makerFeeTxId) { + if (confidence == null) { + return false; + } + + Transaction walletTx = processModel.getTradeWalletService().getWalletTx(confidence.getTransactionHash()); + long numInputMatches = walletTx.getInputs().stream() + .map(TransactionInput::getOutpoint) + .filter(Objects::nonNull) + .map(TransactionOutPoint::getHash) + .map(Sha256Hash::toString) + .filter(txId -> txId.equals(takerFeeTxId) || txId.equals(makerFeeTxId)) + .count(); + if (takerFeeTxId == null && numInputMatches != 1) { + log.warn("We got a transactionConfidenceTx which does not match our inputs. " + + "takerFeeTxId is null (valid if role is buyer as maker) and numInputMatches " + + "is not 1 as expected (for makerFeeTxId). " + + "numInputMatches={}, transactionConfidenceTx={}", + numInputMatches, walletTx); + return false; + } else if (takerFeeTxId != null && numInputMatches != 2) { + log.warn("We got a transactionConfidenceTx which does not match our inputs. " + + "numInputMatches is not 2 as expected (for makerFeeTxId and takerFeeTxId). " + + "numInputMatches={}, transactionConfidenceTx={}", + numInputMatches, walletTx); + return false; + } + + boolean isOutputMatching = walletTx.getOutputs().stream() + .map(transactionOutput -> transactionOutput.getScriptPubKey().getToAddress(params)) + .anyMatch(address -> address.equals(depositTxAddress)); + if (!isOutputMatching) { + log.warn("We got a transactionConfidenceTx which does not has the depositTxAddress " + + "as output (but as input). depositTxAddress={}, transactionConfidenceTx={}", + depositTxAddress, walletTx); + } + return isOutputMatching; + } + private void applyConfidence(TransactionConfidence confidence) { if (trade.getDepositTx() == null) { Transaction walletTx = processModel.getTradeWalletService().getWalletTx(confidence.getTransactionHash()); diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_maker/BuyerAsMakerCreatesAndSignsDepositTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_maker/BuyerAsMakerCreatesAndSignsDepositTx.java index 4fc92bda031..5932f735d0f 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_maker/BuyerAsMakerCreatesAndSignsDepositTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_maker/BuyerAsMakerCreatesAndSignsDepositTx.java @@ -63,7 +63,7 @@ protected void run() { Optional addressEntryOptional = walletService.getAddressEntry(id, AddressEntry.Context.MULTI_SIG); checkArgument(addressEntryOptional.isPresent(), "addressEntryOptional must be present"); AddressEntry makerMultiSigAddressEntry = addressEntryOptional.get(); - makerMultiSigAddressEntry.setCoinLockedInMultiSig(makerInputAmount); + processModel.getBtcWalletService().setCoinLockedInMultiSigAddressEntry(makerMultiSigAddressEntry, makerInputAmount.value); walletService.saveAddressEntryList(); Coin msOutputAmount = makerInputAmount diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java index 24f988120da..6833f627314 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignsDepositTx.java @@ -68,7 +68,8 @@ protected void run() { AddressEntry buyerMultiSigAddressEntry = addressEntryOptional.get(); Coin buyerInput = Coin.valueOf(buyerInputs.stream().mapToLong(input -> input.value).sum()); - buyerMultiSigAddressEntry.setCoinLockedInMultiSig(buyerInput.subtract(trade.getTxFee().multiply(2))); + Coin multiSigValue = buyerInput.subtract(trade.getTxFee().multiply(2)); + processModel.getBtcWalletService().setCoinLockedInMultiSigAddressEntry(buyerMultiSigAddressEntry, multiSigValue.value); walletService.saveAddressEntryList(); TradingPeer tradingPeer = processModel.getTradingPeer(); diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/FinalizeMediatedPayoutTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/FinalizeMediatedPayoutTx.java index 87a8bdf32e4..875b78f7af4 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/FinalizeMediatedPayoutTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/FinalizeMediatedPayoutTx.java @@ -110,7 +110,7 @@ protected void run() { processModel.getTradeManager().requestPersistence(); - walletService.swapTradeEntryToAvailableEntry(tradeId, AddressEntry.Context.MULTI_SIG); + walletService.resetCoinLockedInMultiSigAddressEntry(tradeId); complete(); } catch (Throwable t) { diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/ProcessMediatedPayoutTxPublishedMessage.java b/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/ProcessMediatedPayoutTxPublishedMessage.java index 8e5b08a068e..39435ab6ec7 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/ProcessMediatedPayoutTxPublishedMessage.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/mediation/ProcessMediatedPayoutTxPublishedMessage.java @@ -17,7 +17,6 @@ package bisq.core.trade.protocol.tasks.mediation; -import bisq.core.btc.model.AddressEntry; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.WalletService; import bisq.core.support.dispute.mediation.MediationResultState; @@ -70,7 +69,7 @@ protected void run() { .closeDisputedTrade(trade.getId(), Trade.DisputeState.MEDIATION_CLOSED)); } - 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()); } diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSignAndFinalizePayoutTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSignAndFinalizePayoutTx.java index 34f645896b2..d6b0babc655 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSignAndFinalizePayoutTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/seller/SellerSignAndFinalizePayoutTx.java @@ -102,7 +102,7 @@ protected void run() { processModel.getTradeManager().requestPersistence(); - walletService.swapTradeEntryToAvailableEntry(id, AddressEntry.Context.MULTI_SIG); + walletService.resetCoinLockedInMultiSigAddressEntry(id); complete(); } catch (Throwable t) { diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_maker/SellerAsMakerCreatesUnsignedDepositTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_maker/SellerAsMakerCreatesUnsignedDepositTx.java index 160074e9913..b484324d3f3 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_maker/SellerAsMakerCreatesUnsignedDepositTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_maker/SellerAsMakerCreatesUnsignedDepositTx.java @@ -70,7 +70,8 @@ protected void run() { Optional addressEntryOptional = walletService.getAddressEntry(id, AddressEntry.Context.MULTI_SIG); checkArgument(addressEntryOptional.isPresent(), "addressEntryOptional must be present"); AddressEntry makerMultiSigAddressEntry = addressEntryOptional.get(); - makerMultiSigAddressEntry.setCoinLockedInMultiSig(makerInputAmount); + processModel.getBtcWalletService().setCoinLockedInMultiSigAddressEntry(makerMultiSigAddressEntry, makerInputAmount.value); + walletService.saveAddressEntryList(); Coin msOutputAmount = makerInputAmount diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java index 887cbcf7636..ead81f5c20f 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignsDepositTx.java @@ -66,7 +66,8 @@ protected void run() { Coin sellerInput = Coin.valueOf(sellerInputs.stream().mapToLong(input -> input.value).sum()); Coin totalFee = trade.getTxFee().multiply(2); // Fee for deposit and payout tx - sellerMultiSigAddressEntry.setCoinLockedInMultiSig(sellerInput.subtract(totalFee)); + Coin multiSigValue = sellerInput.subtract(totalFee); + processModel.getBtcWalletService().setCoinLockedInMultiSigAddressEntry(sellerMultiSigAddressEntry, multiSigValue.value); walletService.saveAddressEntryList(); TradingPeer tradingPeer = processModel.getTradingPeer(); diff --git a/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedListItem.java b/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedListItem.java index b9701b45fe9..fcafade8020 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedListItem.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/locked/LockedListItem.java @@ -78,7 +78,7 @@ public void cleanup() { } private void updateBalance() { - balance = addressEntry.getCoinLockedInMultiSig(); + balance = addressEntry.getCoinLockedInMultiSigAsCoin(); balanceLabel.setText(formatter.formatCoin(this.balance)); }