From 4f1cbbd00ec77ee6d8dc18bd07ea3552cb359037 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Fri, 11 Sep 2020 15:35:47 -0500 Subject: [PATCH 1/6] Add check for refund agent if donation address is valid --- .../main/java/bisq/core/dao/DaoFacade.java | 9 ++ .../bisq/core/support/dispute/Dispute.java | 12 +++ .../core/support/dispute/DisputeManager.java | 45 +++++++++- .../arbitration/ArbitrationManager.java | 6 +- .../dispute/mediation/MediationManager.java | 6 +- .../support/dispute/refund/RefundManager.java | 6 +- .../core/trade/DelayedPayoutTxValidation.java | 88 ++++++++++++++++++- .../resources/i18n/displayStrings.properties | 9 +- .../main/offer/MutableOfferViewModel.java | 2 +- .../main/overlays/windows/ContractWindow.java | 7 ++ .../windows/DisputeSummaryWindow.java | 36 +++++++- .../pendingtrades/PendingTradesDataModel.java | 41 +++++++-- .../dispute/agent/DisputeAgentView.java | 31 +++++++ .../agent/arbitration/ArbitratorView.java | 3 + .../dispute/agent/mediation/MediatorView.java | 3 + .../dispute/agent/refund/RefundAgentView.java | 6 +- proto/src/main/proto/pb.proto | 1 + 17 files changed, 285 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/DaoFacade.java b/core/src/main/java/bisq/core/dao/DaoFacade.java index 1485ae25465..50b6402c5a7 100644 --- a/core/src/main/java/bisq/core/dao/DaoFacade.java +++ b/core/src/main/java/bisq/core/dao/DaoFacade.java @@ -95,6 +95,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; @@ -750,4 +751,12 @@ public long getRequiredBond(BondedRoleType bondedRoleType) { long baseFactor = daoStateService.getParamValueAsCoin(Param.BONDED_ROLE_FACTOR, height).value; return requiredBondUnit * baseFactor; } + + public Set getAllPastParamValues(Param param) { + Set set = new HashSet<>(); + periodService.getCycles().forEach(cycle -> { + set.add(getParamValue(param, cycle.getHeightOfFirstBlock())); + }); + return set; + } } diff --git a/core/src/main/java/bisq/core/support/dispute/Dispute.java b/core/src/main/java/bisq/core/support/dispute/Dispute.java index d5f45a7a185..6a79a9f740d 100644 --- a/core/src/main/java/bisq/core/support/dispute/Dispute.java +++ b/core/src/main/java/bisq/core/support/dispute/Dispute.java @@ -103,6 +103,11 @@ public final class Dispute implements NetworkPayload { @Nullable private String delayedPayoutTxId; + // Added at v1.3.9 + @Setter + @Nullable + private String donationAddressOfDelayedPayoutTx; + /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -228,6 +233,7 @@ public protobuf.Dispute toProtoMessage() { Optional.ofNullable(supportType).ifPresent(result -> builder.setSupportType(SupportType.toProtoMessage(supportType))); Optional.ofNullable(mediatorsDisputeResult).ifPresent(result -> builder.setMediatorsDisputeResult(mediatorsDisputeResult)); Optional.ofNullable(delayedPayoutTxId).ifPresent(result -> builder.setDelayedPayoutTxId(delayedPayoutTxId)); + Optional.ofNullable(donationAddressOfDelayedPayoutTx).ifPresent(result -> builder.setDonationAddressOfDelayedPayoutTx(donationAddressOfDelayedPayoutTx)); return builder.build(); } @@ -271,6 +277,11 @@ public static Dispute fromProto(protobuf.Dispute proto, CoreProtoResolver corePr dispute.setDelayedPayoutTxId(delayedPayoutTxId); } + String donationAddressOfDelayedPayoutTx = proto.getDonationAddressOfDelayedPayoutTx(); + if (!donationAddressOfDelayedPayoutTx.isEmpty()) { + dispute.setDonationAddressOfDelayedPayoutTx(donationAddressOfDelayedPayoutTx); + } + return dispute; } @@ -382,6 +393,7 @@ public String toString() { ",\n supportType=" + supportType + ",\n mediatorsDisputeResult='" + mediatorsDisputeResult + '\'' + ",\n delayedPayoutTxId='" + delayedPayoutTxId + '\'' + + ",\n donationAddressOfDelayedPayoutTx='" + donationAddressOfDelayedPayoutTx + '\'' + "\n}"; } } diff --git a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java index 9998e1f2eba..5a01fdba56e 100644 --- a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java +++ b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java @@ -21,6 +21,7 @@ import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.Restrictions; import bisq.core.btc.wallet.TradeWalletService; +import bisq.core.dao.DaoFacade; import bisq.core.locale.CurrencyUtil; import bisq.core.locale.Res; import bisq.core.monetary.Altcoin; @@ -35,6 +36,7 @@ import bisq.core.support.dispute.messages.PeerOpenedDisputeMessage; import bisq.core.support.messages.ChatMessage; import bisq.core.trade.Contract; +import bisq.core.trade.DelayedPayoutTxValidation; import bisq.core.trade.Trade; import bisq.core.trade.TradeManager; import bisq.core.trade.closed.ClosedTradableManager; @@ -58,6 +60,7 @@ import javafx.beans.property.IntegerProperty; +import javafx.collections.FXCollections; import javafx.collections.ObservableList; import java.util.List; @@ -66,10 +69,12 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; +import lombok.Getter; import lombok.extern.slf4j.Slf4j; import javax.annotation.Nullable; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; @Slf4j @@ -82,6 +87,10 @@ public abstract class DisputeManager disputeListService; private final PriceFeedService priceFeedService; + private final DaoFacade daoFacade; + + @Getter + protected final ObservableList disputesWithInvalidDonationAddress = FXCollections.observableArrayList(); /////////////////////////////////////////////////////////////////////////////////////////// @@ -95,6 +104,7 @@ public DisputeManager(P2PService p2PService, TradeManager tradeManager, ClosedTradableManager closedTradableManager, OpenOfferManager openOfferManager, + DaoFacade daoFacade, PubKeyRing pubKeyRing, DisputeListService disputeListService, PriceFeedService priceFeedService) { @@ -105,6 +115,7 @@ public DisputeManager(P2PService p2PService, this.tradeManager = tradeManager; this.closedTradableManager = closedTradableManager; this.openOfferManager = openOfferManager; + this.daoFacade = daoFacade; this.pubKeyRing = pubKeyRing; this.disputeListService = disputeListService; this.priceFeedService = priceFeedService; @@ -178,7 +189,7 @@ public void addAndPersistChatMessage(ChatMessage message) { @Nullable public abstract NodeAddress getAgentNodeAddress(Dispute dispute); - protected abstract Trade.DisputeState getDisputeState_StartedByPeer(); + protected abstract Trade.DisputeState getDisputeStateStartedByPeer(); public abstract void cleanupDisputes(); @@ -257,6 +268,7 @@ public Optional findOwnDispute(String tradeId) { return disputeList.stream().filter(e -> e.getTradeId().equals(tradeId)).findAny(); } + /////////////////////////////////////////////////////////////////////////////////////////// // Message handler /////////////////////////////////////////////////////////////////////////////////////////// @@ -278,6 +290,10 @@ protected void onOpenNewDisputeMessage(OpenNewDisputeMessage openNewDisputeMessa Contract contract = dispute.getContract(); addPriceInfoMessage(dispute, 0); + if (!DelayedPayoutTxValidation.isValidDonationAddress(dispute, daoFacade)) { + disputesWithInvalidDonationAddress.add(dispute); + } + PubKeyRing peersPubKeyRing = dispute.isDisputeOpenerIsBuyer() ? contract.getSellerPubKeyRing() : contract.getBuyerPubKeyRing(); if (isAgent(dispute)) { if (!disputeList.contains(dispute)) { @@ -310,7 +326,7 @@ protected void onOpenNewDisputeMessage(OpenNewDisputeMessage openNewDisputeMessa addMediationResultMessage(dispute); } - // not dispute requester receives that from dispute agent + // Not-dispute-requester receives that msg from dispute agent protected void onPeerOpenedDisputeMessage(PeerOpenedDisputeMessage peerOpenedDisputeMessage) { T disputeList = getDisputeList(); if (disputeList == null) { @@ -320,14 +336,34 @@ protected void onPeerOpenedDisputeMessage(PeerOpenedDisputeMessage peerOpenedDis String errorMessage = null; Dispute dispute = peerOpenedDisputeMessage.getDispute(); + + Optional optionalTrade = tradeManager.getTradeById(dispute.getTradeId()); + checkArgument(optionalTrade.isPresent()); + Trade trade = optionalTrade.get(); + try { + DelayedPayoutTxValidation.validatePayoutTx(trade, + trade.getDelayedPayoutTx(), + dispute, + daoFacade, + btcWalletService); + } catch (DelayedPayoutTxValidation.DonationAddressException | + DelayedPayoutTxValidation.InvalidTxException | + DelayedPayoutTxValidation.InvalidLockTimeException | + DelayedPayoutTxValidation.MissingDelayedPayoutTxException | + DelayedPayoutTxValidation.AmountMismatchException e) { + // The peer sent us an invalid donation address. We do not return here as we don't want to break + // mediation/arbitration and log only the issue. The dispute agent will run validation as well and will get + // a popup displayed to react. + log.error("Donation address invalid. {}", e.toString()); + } + if (!isAgent(dispute)) { if (!disputeList.contains(dispute)) { Optional storedDisputeOptional = findDispute(dispute); if (!storedDisputeOptional.isPresent()) { dispute.setStorage(disputeListService.getStorage()); disputeList.add(dispute); - Optional tradeOptional = tradeManager.getTradeById(dispute.getTradeId()); - tradeOptional.ifPresent(trade -> trade.setDisputeState(getDisputeState_StartedByPeer())); + trade.setDisputeState(getDisputeStateStartedByPeer()); errorMessage = null; } else { // valid case if both have opened a dispute and agent was not online. @@ -516,6 +552,7 @@ private void doSendPeerOpenedDisputeMessage(Dispute disputeFromOpener, disputeFromOpener.isSupportTicket(), disputeFromOpener.getSupportType()); dispute.setDelayedPayoutTxId(disputeFromOpener.getDelayedPayoutTxId()); + dispute.setDonationAddressOfDelayedPayoutTx(disputeFromOpener.getDonationAddressOfDelayedPayoutTx()); Optional storedDisputeOptional = findDispute(dispute); diff --git a/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java b/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java index e77daf2b048..2ddacf350f5 100644 --- a/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java +++ b/core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java @@ -25,6 +25,7 @@ import bisq.core.btc.wallet.TradeWalletService; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.btc.wallet.WalletService; +import bisq.core.dao.DaoFacade; import bisq.core.locale.Res; import bisq.core.offer.OpenOffer; import bisq.core.offer.OpenOfferManager; @@ -88,11 +89,12 @@ public ArbitrationManager(P2PService p2PService, TradeManager tradeManager, ClosedTradableManager closedTradableManager, OpenOfferManager openOfferManager, + DaoFacade daoFacade, PubKeyRing pubKeyRing, ArbitrationDisputeListService arbitrationDisputeListService, PriceFeedService priceFeedService) { super(p2PService, tradeWalletService, walletService, walletsSetup, tradeManager, closedTradableManager, - openOfferManager, pubKeyRing, arbitrationDisputeListService, priceFeedService); + openOfferManager, daoFacade, pubKeyRing, arbitrationDisputeListService, priceFeedService); } @@ -134,7 +136,7 @@ public NodeAddress getAgentNodeAddress(Dispute dispute) { } @Override - protected Trade.DisputeState getDisputeState_StartedByPeer() { + protected Trade.DisputeState getDisputeStateStartedByPeer() { return Trade.DisputeState.DISPUTE_STARTED_BY_PEER; } diff --git a/core/src/main/java/bisq/core/support/dispute/mediation/MediationManager.java b/core/src/main/java/bisq/core/support/dispute/mediation/MediationManager.java index 42f9b37f5cf..088af6f8680 100644 --- a/core/src/main/java/bisq/core/support/dispute/mediation/MediationManager.java +++ b/core/src/main/java/bisq/core/support/dispute/mediation/MediationManager.java @@ -20,6 +20,7 @@ import bisq.core.btc.setup.WalletsSetup; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.TradeWalletService; +import bisq.core.dao.DaoFacade; import bisq.core.locale.Res; import bisq.core.offer.OpenOffer; import bisq.core.offer.OpenOfferManager; @@ -80,11 +81,12 @@ public MediationManager(P2PService p2PService, TradeManager tradeManager, ClosedTradableManager closedTradableManager, OpenOfferManager openOfferManager, + DaoFacade daoFacade, PubKeyRing pubKeyRing, MediationDisputeListService mediationDisputeListService, PriceFeedService priceFeedService) { super(p2PService, tradeWalletService, walletService, walletsSetup, tradeManager, closedTradableManager, - openOfferManager, pubKeyRing, mediationDisputeListService, priceFeedService); + openOfferManager, daoFacade, pubKeyRing, mediationDisputeListService, priceFeedService); } /////////////////////////////////////////////////////////////////////////////////////////// @@ -117,7 +119,7 @@ public void dispatchMessage(SupportMessage message) { } @Override - protected Trade.DisputeState getDisputeState_StartedByPeer() { + protected Trade.DisputeState getDisputeStateStartedByPeer() { return Trade.DisputeState.MEDIATION_STARTED_BY_PEER; } diff --git a/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java b/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java index f28208f050a..f00aca5fbde 100644 --- a/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java +++ b/core/src/main/java/bisq/core/support/dispute/refund/RefundManager.java @@ -20,6 +20,7 @@ import bisq.core.btc.setup.WalletsSetup; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.TradeWalletService; +import bisq.core.dao.DaoFacade; import bisq.core.locale.Res; import bisq.core.offer.OpenOffer; import bisq.core.offer.OpenOfferManager; @@ -74,11 +75,12 @@ public RefundManager(P2PService p2PService, TradeManager tradeManager, ClosedTradableManager closedTradableManager, OpenOfferManager openOfferManager, + DaoFacade daoFacade, PubKeyRing pubKeyRing, RefundDisputeListService refundDisputeListService, PriceFeedService priceFeedService) { super(p2PService, tradeWalletService, walletService, walletsSetup, tradeManager, closedTradableManager, - openOfferManager, pubKeyRing, refundDisputeListService, priceFeedService); + openOfferManager, daoFacade, pubKeyRing, refundDisputeListService, priceFeedService); } /////////////////////////////////////////////////////////////////////////////////////////// @@ -111,7 +113,7 @@ public void dispatchMessage(SupportMessage message) { } @Override - protected Trade.DisputeState getDisputeState_StartedByPeer() { + protected Trade.DisputeState getDisputeStateStartedByPeer() { return Trade.DisputeState.REFUND_REQUEST_STARTED_BY_PEER; } diff --git a/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java b/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java index 6c835d2408b..7b0997d57ae 100644 --- a/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java +++ b/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java @@ -21,6 +21,7 @@ import bisq.core.dao.DaoFacade; import bisq.core.dao.governance.param.Param; import bisq.core.offer.Offer; +import bisq.core.support.dispute.Dispute; import bisq.common.config.Config; @@ -33,9 +34,14 @@ import org.bitcoinj.core.TransactionOutput; import java.util.List; +import java.util.Set; +import java.util.function.Consumer; import lombok.extern.slf4j.Slf4j; +import javax.annotation.Nullable; + +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; @Slf4j @@ -77,12 +83,80 @@ public static class InvalidInputException extends Exception { } } + public static boolean isValidDonationAddress(Dispute dispute, DaoFacade daoFacade) { + String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); + + // Old clients don't have it set yet. Can be removed after a forced update + if (addressAsString == null) { + return true; + } + + // We use all past addresses from DAO param changes as the dispute case might have been opened later and the + // DAO param changed in the meantime. + Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); + + if (allPastParamValues.contains(addressAsString)) { + return true; + } + + log.warn("Donation address is not a valid DAO donation address." + + "\nAddress used in the dispute: " + addressAsString + + "\nAll DAO param donation addresses:" + allPastParamValues); + return false; + } + public static void validatePayoutTx(Trade trade, Transaction delayedPayoutTx, DaoFacade daoFacade, BtcWalletService btcWalletService) throws DonationAddressException, MissingDelayedPayoutTxException, InvalidTxException, InvalidLockTimeException, AmountMismatchException { + validatePayoutTx(trade, + delayedPayoutTx, + null, + daoFacade, + btcWalletService, + null); + } + + public static void validatePayoutTx(Trade trade, + Transaction delayedPayoutTx, + @Nullable Dispute dispute, + DaoFacade daoFacade, + BtcWalletService btcWalletService) + throws DonationAddressException, MissingDelayedPayoutTxException, + InvalidTxException, InvalidLockTimeException, AmountMismatchException { + validatePayoutTx(trade, + delayedPayoutTx, + dispute, + daoFacade, + btcWalletService, + null); + } + + public static void validatePayoutTx(Trade trade, + Transaction delayedPayoutTx, + DaoFacade daoFacade, + BtcWalletService btcWalletService, + @Nullable Consumer addressConsumer) + throws DonationAddressException, MissingDelayedPayoutTxException, + InvalidTxException, InvalidLockTimeException, AmountMismatchException { + validatePayoutTx(trade, + delayedPayoutTx, + null, + daoFacade, + btcWalletService, + addressConsumer); + } + + public static void validatePayoutTx(Trade trade, + Transaction delayedPayoutTx, + @Nullable Dispute dispute, + DaoFacade daoFacade, + BtcWalletService btcWalletService, + @Nullable Consumer addressConsumer) + throws DonationAddressException, MissingDelayedPayoutTxException, + InvalidTxException, InvalidLockTimeException, AmountMismatchException { String errorMsg; if (delayedPayoutTx == null) { errorMsg = "DelayedPayoutTx must not be null"; @@ -144,7 +218,6 @@ public static void validatePayoutTx(Trade trade, // We do not support past DAO param addresses to avoid that those receive funds (no bond set up anymore). // Users who have not synced the DAO cannot trade. - NetworkParameters params = btcWalletService.getParams(); Address address = output.getAddressFromP2PKHScript(params); if (address == null) { @@ -159,6 +232,9 @@ public static void validatePayoutTx(Trade trade, } String addressAsString = address.toString(); + if (addressConsumer != null) { + addressConsumer.accept(addressAsString); + } // In case the seller has deactivated the DAO the default address will be used. String defaultDonationAddressString = Param.RECIPIENT_BTC_ADDRESS.getDefaultValue(); @@ -190,6 +266,16 @@ public static void validatePayoutTx(Trade trade, log.error(delayedPayoutTx.toString()); throw new DonationAddressException(errorMsg); } + + if (dispute != null) { + // Verify that address in the dispute matches the one in the trade. + String donationAddressOfDelayedPayoutTx = dispute.getDonationAddressOfDelayedPayoutTx(); + // Old clients don't have it set yet. Can be removed after a forced update + if (donationAddressOfDelayedPayoutTx != null) { + checkArgument(addressAsString.equals(donationAddressOfDelayedPayoutTx), + "donationAddressOfDelayedPayoutTx from dispute does not match address from delayed payout tx"); + } + } } public static void validatePayoutTxInput(Transaction depositTx, diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index 753d8bc3394..4156441334f 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -214,7 +214,8 @@ shared.mediator=Mediator shared.arbitrator=Arbitrator shared.refundAgent=Arbitrator shared.refundAgentForSupportStaff=Refund agent -shared.delayedPayoutTxId=Refund collateral transaction ID +shared.delayedPayoutTxId=Delayed payout transaction ID +shared.delayedPayoutTxReceiverAddress=Delayed payout transaction sent to shared.unconfirmedTransactionsLimitReached=You have too many unconfirmed transactions at the moment. Please try again later. @@ -1080,6 +1081,12 @@ support.peerOpenedDispute=Your trading peer has requested a dispute.\n\n{0}\n\nB support.peerOpenedDisputeForMediation=Your trading peer has requested mediation.\n\n{0}\n\nBisq version: {1} support.mediatorsDisputeSummary=System message: Mediator''s dispute summary:\n{0} support.mediatorsAddress=Mediator''s node address: {0} +support.warning.disputesWithInvalidDonationAddress=The delayed payout transaction has used an invalid receiver address. \ + It does not match any of the DAO parameter values for the valid donation addresses.\n\nThis might be a scam attempt. \ + Please inform the developers about that incident and do not close that case before the situation is resolved!\n\n\ + Address used in the dispute: {0}\n\n\ + All DAO param donation addresses: {1}\n\n\ + Trade ID: {2} #################################################################### diff --git a/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferViewModel.java b/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferViewModel.java index 989d063b25e..588c06cb564 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferViewModel.java @@ -231,7 +231,7 @@ public void activate() { if (DevEnv.isDevMode()) { UserThread.runAfter(() -> { amount.set("0.001"); - price.set("0.008"); + price.set("70000"); minAmount.set(amount.get()); onFocusOutPriceAsPercentageTextField(true, false); applyMakerFee(); diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/ContractWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/ContractWindow.java index ff053d90338..ec8e10ede0b 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/ContractWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/ContractWindow.java @@ -141,6 +141,8 @@ private void addContent() { rows++; if (dispute.getDelayedPayoutTxId() != null) rows++; + if (dispute.getDonationAddressOfDelayedPayoutTx() != null) + rows++; if (showAcceptedCountryCodes) rows++; if (showAcceptedBanks) @@ -248,6 +250,11 @@ private void addContent() { if (dispute.getDelayedPayoutTxId() != null) addLabelTxIdTextField(gridPane, ++rowIndex, Res.get("shared.delayedPayoutTxId"), dispute.getDelayedPayoutTxId()); + if (dispute.getDonationAddressOfDelayedPayoutTx() != null) { + addLabelTxIdTextField(gridPane, ++rowIndex, Res.get("shared.delayedPayoutTxReceiverAddress"), + dispute.getDonationAddressOfDelayedPayoutTx()); + } + if (dispute.getPayoutTxSerialized() != null) addLabelTxIdTextField(gridPane, ++rowIndex, Res.get("shared.payoutTxId"), dispute.getPayoutTxId()); diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java index 6202b6b2042..d78723ec538 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java @@ -34,6 +34,8 @@ import bisq.core.btc.wallet.Restrictions; import bisq.core.btc.wallet.TradeWalletService; import bisq.core.btc.wallet.TxBroadcaster; +import bisq.core.dao.DaoFacade; +import bisq.core.dao.governance.param.Param; import bisq.core.locale.Res; import bisq.core.offer.Offer; import bisq.core.provider.fee.FeeService; @@ -45,6 +47,7 @@ import bisq.core.support.dispute.mediation.MediationManager; import bisq.core.support.dispute.refund.RefundManager; import bisq.core.trade.Contract; +import bisq.core.trade.DelayedPayoutTxValidation; import bisq.core.util.FormattingUtils; import bisq.core.util.ParsingUtils; import bisq.core.util.coin.CoinFormatter; @@ -84,6 +87,7 @@ import java.util.Date; import java.util.Optional; +import java.util.Set; import java.util.concurrent.TimeUnit; import org.slf4j.Logger; @@ -105,6 +109,7 @@ public class DisputeSummaryWindow extends Overlay { private final BtcWalletService btcWalletService; private final TxFeeEstimationService txFeeEstimationService; private final FeeService feeService; + private final DaoFacade daoFacade; private Dispute dispute; private Optional finalizeDisputeHandlerOptional = Optional.empty(); private ToggleGroup tradeAmountToggleGroup, reasonToggleGroup; @@ -141,7 +146,8 @@ public DisputeSummaryWindow(@Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormat TradeWalletService tradeWalletService, BtcWalletService btcWalletService, TxFeeEstimationService txFeeEstimationService, - FeeService feeService) { + FeeService feeService, + DaoFacade daoFacade) { this.formatter = formatter; this.mediationManager = mediationManager; @@ -150,6 +156,7 @@ public DisputeSummaryWindow(@Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormat this.btcWalletService = btcWalletService; this.txFeeEstimationService = txFeeEstimationService; this.feeService = feeService; + this.daoFacade = daoFacade; type = Type.Confirmation; } @@ -642,7 +649,10 @@ private void addButtons(Contract contract) { log.warn("dispute.getDepositTxSerialized is null"); return; } - if (dispute.getSupportType() == SupportType.REFUND && + + if (!DelayedPayoutTxValidation.isValidDonationAddress(dispute, daoFacade)) { + showInValidDonationAddressPopup(); + } else if (dispute.getSupportType() == SupportType.REFUND && peersDisputeOptional.isPresent() && !peersDisputeOptional.get().isClosed()) { showPayoutTxConfirmation(contract, disputeResult, @@ -741,6 +751,17 @@ public void onFailure(TxBroadcastException exception) { } private void doClose(Button closeTicketButton) { + var disputeManager = checkNotNull(getDisputeManager(dispute)); + if (!DelayedPayoutTxValidation.isValidDonationAddress(dispute, daoFacade)) { + showInValidDonationAddressPopup(); + + // For mediators we do not enforce that the case cannot be closed to stay flexible, + // but for refund agents we do. + if (disputeManager instanceof RefundManager) { + return; + } + } + disputeResult.setLoserPublisher(isLoserPublisherCheckBox.isSelected()); disputeResult.setCloseDate(new Date()); dispute.setDisputeResult(disputeResult); @@ -765,7 +786,7 @@ private void doClose(Button closeTicketButton) { text += Res.get("disputeSummaryWindow.close.nextStepsForRefundAgentArbitration"); } - checkNotNull(getDisputeManager(dispute)).sendDisputeResultMessage(disputeResult, dispute, text); + disputeManager.sendDisputeResultMessage(disputeResult, dispute, text); if (peersDisputeOptional.isPresent() && !peersDisputeOptional.get().isClosed() && !DevEnv.isDevMode()) { UserThread.runAfter(() -> new Popup() @@ -781,6 +802,15 @@ private void doClose(Button closeTicketButton) { hide(); } + private void showInValidDonationAddressPopup() { + String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); + Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); + String tradeId = dispute.getTradeId(); + new Popup().warning(Res.get("support.warning.disputesWithInvalidDonationAddress", + addressAsString, allPastParamValues, tradeId)) + .show(); + } + private DisputeManager> getDisputeManager(Dispute dispute) { if (dispute.getSupportType() != null) { switch (dispute.getSupportType()) { diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java index 6d1a288f118..e1f2b10d9bf 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java @@ -47,6 +47,7 @@ import bisq.core.support.messages.ChatMessage; import bisq.core.support.traderchat.TraderChatManager; import bisq.core.trade.BuyerTrade; +import bisq.core.trade.DelayedPayoutTxValidation; import bisq.core.trade.SellerTrade; import bisq.core.trade.Trade; import bisq.core.trade.TradeManager; @@ -82,6 +83,7 @@ import org.spongycastle.crypto.params.KeyParameter; import java.util.UUID; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import lombok.Getter; @@ -536,6 +538,25 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { // In case we re-open a dispute we allow Trade.DisputeState.REFUND_REQUESTED useRefundAgent = disputeState == Trade.DisputeState.MEDIATION_CLOSED || disputeState == Trade.DisputeState.REFUND_REQUESTED; + AtomicReference donationAddressString = new AtomicReference<>(""); + Transaction delayedPayoutTx = trade.getDelayedPayoutTx(); + try { + DelayedPayoutTxValidation.validatePayoutTx(trade, + delayedPayoutTx, + daoFacade, + btcWalletService, + donationAddressString::set); + } catch (DelayedPayoutTxValidation.DonationAddressException | + DelayedPayoutTxValidation.InvalidTxException | + DelayedPayoutTxValidation.InvalidLockTimeException | + DelayedPayoutTxValidation.MissingDelayedPayoutTxException | + DelayedPayoutTxValidation.AmountMismatchException e) { + // The peer sent us an invalid donation address. We do not return here as we don't want to break + // mediation/arbitration and log only the issue. The dispute agent will run validation as well and will get + // a popup displayed to react. + log.error("Donation address invalid. {}", e.toString()); + } + ResultHandler resultHandler; if (useMediation) { // If no dispute state set we start with mediation @@ -564,6 +585,11 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { isSupportTicket, SupportType.MEDIATION); + dispute.setDonationAddressOfDelayedPayoutTx(donationAddressString.get()); + if (delayedPayoutTx != null) { + dispute.setDelayedPayoutTxId(delayedPayoutTx.getHashAsString()); + } + trade.setDisputeState(Trade.DisputeState.MEDIATION_REQUESTED); disputeManager.sendOpenNewDisputeMessage(dispute, false, @@ -588,7 +614,7 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { } else if (useRefundAgent) { resultHandler = () -> navigation.navigateTo(MainView.class, SupportView.class, RefundClientView.class); - if (trade.getDelayedPayoutTx() == null) { + if (delayedPayoutTx == null) { log.error("Delayed payout tx is missing"); return; } @@ -603,13 +629,12 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { return; } - long lockTime = trade.getDelayedPayoutTx().getLockTime(); + long lockTime = delayedPayoutTx.getLockTime(); int bestChainHeight = btcWalletService.getBestChainHeight(); long remaining = lockTime - bestChainHeight; if (remaining > 0) { - new Popup() - .instruction(Res.get("portfolio.pending.timeLockNotOver", - FormattingUtils.getDateFromBlockHeight(remaining), remaining)) + new Popup().instruction(Res.get("portfolio.pending.timeLockNotOver", + FormattingUtils.getDateFromBlockHeight(remaining), remaining)) .show(); return; } @@ -639,6 +664,9 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { isSupportTicket, SupportType.REFUND); + dispute.setDonationAddressOfDelayedPayoutTx(donationAddressString.get()); + dispute.setDelayedPayoutTxId(delayedPayoutTx.getHashAsString()); + String tradeId = dispute.getTradeId(); mediationManager.findDispute(tradeId) .ifPresent(mediatorsDispute -> { @@ -651,9 +679,6 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { dispute.setMediatorsDisputeResult(message); } }); - - dispute.setDelayedPayoutTxId(trade.getDelayedPayoutTx().getHashAsString()); - trade.setDisputeState(Trade.DisputeState.REFUND_REQUESTED); //todo add UI spinner as it can take a bit if peer is offline diff --git a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java index 13e52608678..6f6efdd2064 100644 --- a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java +++ b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java @@ -27,6 +27,8 @@ import bisq.core.account.witness.AccountAgeWitnessService; import bisq.core.alert.PrivateNotificationManager; +import bisq.core.dao.DaoFacade; +import bisq.core.dao.governance.param.Param; import bisq.core.locale.Res; import bisq.core.support.dispute.Dispute; import bisq.core.support.dispute.DisputeList; @@ -54,13 +56,18 @@ import javafx.beans.property.ReadOnlyObjectWrapper; +import javafx.collections.ListChangeListener; + import java.util.List; +import java.util.Set; import static bisq.desktop.util.FormBuilder.getIconForLabel; public abstract class DisputeAgentView extends DisputeView implements MultipleHolderNameDetection.Listener { private final MultipleHolderNameDetection multipleHolderNameDetection; + private final DaoFacade daoFacade; + private ListChangeListener disputesWithInvalidDonationAddressListener; public DisputeAgentView(DisputeManager> disputeManager, KeyRing keyRing, @@ -71,6 +78,7 @@ public DisputeAgentView(DisputeManager { + c.next(); + if (c.wasAdded()) { + showWarningForInvalidDonationAddress(c.getAddedSubList()); + } + }; + } + + protected void showWarningForInvalidDonationAddress(List disputes) { + disputes.forEach(dispute -> { + String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); + Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); + new Popup().warning(Res.get("support.warning.disputesWithInvalidDonationAddress", + addressAsString, allPastParamValues, dispute.getTradeId())) + .show(); + }); } @Override @@ -117,6 +143,9 @@ protected void activate() { if (multipleHolderNameDetection.hasSuspiciousDisputesDetected()) { suspiciousDisputeDetected(); } + + disputeManager.getDisputesWithInvalidDonationAddress().addListener(disputesWithInvalidDonationAddressListener); + showWarningForInvalidDonationAddress(disputeManager.getDisputesWithInvalidDonationAddress()); } @Override @@ -124,6 +153,8 @@ protected void deactivate() { super.deactivate(); multipleHolderNameDetection.removeListener(this); + + disputeManager.getDisputesWithInvalidDonationAddress().removeListener(disputesWithInvalidDonationAddressListener); } diff --git a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/arbitration/ArbitratorView.java b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/arbitration/ArbitratorView.java index dcb33c61124..7c2278de28c 100644 --- a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/arbitration/ArbitratorView.java +++ b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/arbitration/ArbitratorView.java @@ -25,6 +25,7 @@ import bisq.core.account.witness.AccountAgeWitnessService; import bisq.core.alert.PrivateNotificationManager; +import bisq.core.dao.DaoFacade; import bisq.core.support.SupportType; import bisq.core.support.dispute.Dispute; import bisq.core.support.dispute.DisputeSession; @@ -53,6 +54,7 @@ public ArbitratorView(ArbitrationManager arbitrationManager, ContractWindow contractWindow, TradeDetailsWindow tradeDetailsWindow, AccountAgeWitnessService accountAgeWitnessService, + DaoFacade daoFacade, @Named(Config.USE_DEV_PRIVILEGE_KEYS) boolean useDevPrivilegeKeys) { super(arbitrationManager, keyRing, @@ -63,6 +65,7 @@ public ArbitratorView(ArbitrationManager arbitrationManager, contractWindow, tradeDetailsWindow, accountAgeWitnessService, + daoFacade, useDevPrivilegeKeys); } diff --git a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/mediation/MediatorView.java b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/mediation/MediatorView.java index ba939c34d23..d96fb9e8f2a 100644 --- a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/mediation/MediatorView.java +++ b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/mediation/MediatorView.java @@ -25,6 +25,7 @@ import bisq.core.account.witness.AccountAgeWitnessService; import bisq.core.alert.PrivateNotificationManager; +import bisq.core.dao.DaoFacade; import bisq.core.support.SupportType; import bisq.core.support.dispute.Dispute; import bisq.core.support.dispute.DisputeSession; @@ -53,6 +54,7 @@ public MediatorView(MediationManager mediationManager, ContractWindow contractWindow, TradeDetailsWindow tradeDetailsWindow, AccountAgeWitnessService accountAgeWitnessService, + DaoFacade daoFacade, @Named(Config.USE_DEV_PRIVILEGE_KEYS) boolean useDevPrivilegeKeys) { super(mediationManager, keyRing, @@ -63,6 +65,7 @@ public MediatorView(MediationManager mediationManager, contractWindow, tradeDetailsWindow, accountAgeWitnessService, + daoFacade, useDevPrivilegeKeys); } diff --git a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/refund/RefundAgentView.java b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/refund/RefundAgentView.java index 5aae5f53f81..71f27f8954e 100644 --- a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/refund/RefundAgentView.java +++ b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/refund/RefundAgentView.java @@ -25,6 +25,7 @@ import bisq.core.account.witness.AccountAgeWitnessService; import bisq.core.alert.PrivateNotificationManager; +import bisq.core.dao.DaoFacade; import bisq.core.support.SupportType; import bisq.core.support.dispute.Dispute; import bisq.core.support.dispute.DisputeSession; @@ -37,9 +38,8 @@ import bisq.common.config.Config; import bisq.common.crypto.KeyRing; -import javax.inject.Named; - import javax.inject.Inject; +import javax.inject.Named; @FxmlView public class RefundAgentView extends DisputeAgentView { @@ -54,6 +54,7 @@ public RefundAgentView(RefundManager refundManager, ContractWindow contractWindow, TradeDetailsWindow tradeDetailsWindow, AccountAgeWitnessService accountAgeWitnessService, + DaoFacade daoFacade, @Named(Config.USE_DEV_PRIVILEGE_KEYS) boolean useDevPrivilegeKeys) { super(refundManager, keyRing, @@ -64,6 +65,7 @@ public RefundAgentView(RefundManager refundManager, contractWindow, tradeDetailsWindow, accountAgeWitnessService, + daoFacade, useDevPrivilegeKeys); } diff --git a/proto/src/main/proto/pb.proto b/proto/src/main/proto/pb.proto index 2f334e06e03..b10a4a02daf 100644 --- a/proto/src/main/proto/pb.proto +++ b/proto/src/main/proto/pb.proto @@ -792,6 +792,7 @@ message Dispute { SupportType support_type = 24; string mediators_dispute_result = 25; string delayed_payout_tx_id = 26; + string donation_address_of_delayed_payout_tx = 27; } message Attachment { From 2b0433870cb859239f6b8462838103b00533b990 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Fri, 11 Sep 2020 17:00:24 -0500 Subject: [PATCH 2/6] Dont allow opening refudn agent dispute if delayed payout tx is invalid. --- .../portfolio/pendingtrades/PendingTradesDataModel.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java index e1f2b10d9bf..07be33a3c45 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java @@ -554,7 +554,14 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { // The peer sent us an invalid donation address. We do not return here as we don't want to break // mediation/arbitration and log only the issue. The dispute agent will run validation as well and will get // a popup displayed to react. - log.error("Donation address invalid. {}", e.toString()); + log.error("DelayedPayoutTxValidation failed. {}", e.toString()); + + if (useRefundAgent) { + // We don't allow to continue and publish payout tx and open refund agent case. + // In case it was caused by some bug we want to prevent a wrong payout. In case its a scam attempt we + // want to protect the refund agent. + return; + } } ResultHandler resultHandler; From c48abbf575531f9e26fe00072ce48a1dd3c3bd8b Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Fri, 11 Sep 2020 17:37:03 -0500 Subject: [PATCH 3/6] Improve address validation code --- .../core/support/dispute/DisputeManager.java | 4 +- .../core/trade/DelayedPayoutTxValidation.java | 75 ++++++------------- .../windows/DisputeSummaryWindow.java | 25 ++++--- 3 files changed, 39 insertions(+), 65 deletions(-) diff --git a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java index 5a01fdba56e..d327eaad72c 100644 --- a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java +++ b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java @@ -290,7 +290,9 @@ protected void onOpenNewDisputeMessage(OpenNewDisputeMessage openNewDisputeMessa Contract contract = dispute.getContract(); addPriceInfoMessage(dispute, 0); - if (!DelayedPayoutTxValidation.isValidDonationAddress(dispute, daoFacade)) { + try { + DelayedPayoutTxValidation.validateDonationAddress(dispute.getDonationAddressOfDelayedPayoutTx(), daoFacade); + } catch (DelayedPayoutTxValidation.DonationAddressException e) { disputesWithInvalidDonationAddress.add(dispute); } diff --git a/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java b/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java index 7b0997d57ae..42717e17460 100644 --- a/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java +++ b/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java @@ -23,8 +23,6 @@ import bisq.core.offer.Offer; import bisq.core.support.dispute.Dispute; -import bisq.common.config.Config; - import org.bitcoinj.core.Address; import org.bitcoinj.core.Coin; import org.bitcoinj.core.NetworkParameters; @@ -33,7 +31,6 @@ import org.bitcoinj.core.TransactionOutPoint; import org.bitcoinj.core.TransactionOutput; -import java.util.List; import java.util.Set; import java.util.function.Consumer; @@ -83,26 +80,33 @@ public static class InvalidInputException extends Exception { } } - public static boolean isValidDonationAddress(Dispute dispute, DaoFacade daoFacade) { - String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); + public static void validateDonationAddress(String addressAsString, DaoFacade daoFacade) + throws DonationAddressException { - // Old clients don't have it set yet. Can be removed after a forced update if (addressAsString == null) { - return true; + return; } - // We use all past addresses from DAO param changes as the dispute case might have been opened later and the - // DAO param changed in the meantime. + // We support any of the past addresses as well as in case the peer has not enabled the DAO or is out of sync we + // do not want to break validation. Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); - if (allPastParamValues.contains(addressAsString)) { - return true; - } + // If Dao is deactivated we need to add the default address as getAllPastParamValues will not return us any. + allPastParamValues.add(Param.RECIPIENT_BTC_ADDRESS.getDefaultValue()); + + // If Dao is deactivated we need to add the past addresses used as well. + // This list need to be updated once a new address gets defined. + allPastParamValues.add("3EtUWqsGThPtjwUczw27YCo6EWvQdaPUyp"); // burning man 2019 + allPastParamValues.add("3A8Zc1XioE2HRzYfbb5P8iemCS72M6vRJV"); // burningman2 + - log.warn("Donation address is not a valid DAO donation address." + - "\nAddress used in the dispute: " + addressAsString + - "\nAll DAO param donation addresses:" + allPastParamValues); - return false; + if (!allPastParamValues.contains(addressAsString)) { + String errorMsg = "Donation address is not a valid DAO donation address." + + "\nAddress used in the dispute: " + addressAsString + + "\nAll DAO param donation addresses:" + allPastParamValues; + log.error(errorMsg); + throw new DonationAddressException(errorMsg); + } } public static void validatePayoutTx(Trade trade, @@ -212,16 +216,10 @@ public static void validatePayoutTx(Trade trade, throw new AmountMismatchException(errorMsg); } - - // Validate donation address - // Get most recent donation address. - // We do not support past DAO param addresses to avoid that those receive funds (no bond set up anymore). - // Users who have not synced the DAO cannot trade. - NetworkParameters params = btcWalletService.getParams(); Address address = output.getAddressFromP2PKHScript(params); if (address == null) { - // The donation address can be as well be a multisig address. + // The donation address can be a multisig address as well. address = output.getAddressFromP2SH(params); if (address == null) { errorMsg = "Donation address cannot be resolved (not of type P2PKHScript or P2SH). Output: " + output; @@ -236,36 +234,7 @@ public static void validatePayoutTx(Trade trade, addressConsumer.accept(addressAsString); } - // In case the seller has deactivated the DAO the default address will be used. - String defaultDonationAddressString = Param.RECIPIENT_BTC_ADDRESS.getDefaultValue(); - boolean defaultNotMatching = !defaultDonationAddressString.equals(addressAsString); - String recentDonationAddressString = daoFacade.getParamValue(Param.RECIPIENT_BTC_ADDRESS); - boolean recentFromDaoNotMatching = !recentDonationAddressString.equals(addressAsString); - - // If buyer has DAO deactivated or not synced he will not be able to see recent address used by the seller, so - // we add it hard coded here. We need to support also the default one as - // FIXME This is a quick fix and should be improved in future. - // We use the default addresses for non mainnet networks. For dev testing it need to be changed here. - // We use a list to gain more flexibility at updates of DAO param, but still might fail if buyer has not updated - // software. Needs a better solution.... - List hardCodedAddresses = Config.baseCurrencyNetwork().isMainnet() ? - List.of("3EtUWqsGThPtjwUczw27YCo6EWvQdaPUyp", "3A8Zc1XioE2HRzYfbb5P8iemCS72M6vRJV") : // mainnet - Config.baseCurrencyNetwork().isDaoBetaNet() ? List.of("1BVxNn3T12veSK6DgqwU4Hdn7QHcDDRag7") : // daoBetaNet - Config.baseCurrencyNetwork().isTestnet() ? List.of("2N4mVTpUZAnhm9phnxB7VrHB4aBhnWrcUrV") : // testnet - List.of("2MzBNTJDjjXgViKBGnatDU3yWkJ8pJkEg9w"); // regtest or DAO testnet (regtest) - - boolean noneOfHardCodedMatching = hardCodedAddresses.stream().noneMatch(e -> e.equals(addressAsString)); - - // If seller has DAO deactivated as well we get default address - if (recentFromDaoNotMatching && defaultNotMatching && noneOfHardCodedMatching) { - errorMsg = "Donation address is invalid." + - "\nAddress used by BTC seller: " + addressAsString + - "\nRecent donation address:" + recentDonationAddressString + - "\nDefault donation address: " + defaultDonationAddressString; - log.error(errorMsg); - log.error(delayedPayoutTx.toString()); - throw new DonationAddressException(errorMsg); - } + validateDonationAddress(addressAsString, daoFacade); if (dispute != null) { // Verify that address in the dispute matches the one in the trade. diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java index d78723ec538..d7ce950ea13 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java @@ -650,17 +650,18 @@ private void addButtons(Contract contract) { return; } - if (!DelayedPayoutTxValidation.isValidDonationAddress(dispute, daoFacade)) { + try { + DelayedPayoutTxValidation.validateDonationAddress(dispute.getDonationAddressOfDelayedPayoutTx(), daoFacade); + + if (dispute.getSupportType() == SupportType.REFUND && + peersDisputeOptional.isPresent() && + !peersDisputeOptional.get().isClosed()) { + showPayoutTxConfirmation(contract, disputeResult, () -> doClose(closeTicketButton)); + } else { + doClose(closeTicketButton); + } + } catch (DelayedPayoutTxValidation.DonationAddressException exception) { showInValidDonationAddressPopup(); - } else if (dispute.getSupportType() == SupportType.REFUND && - peersDisputeOptional.isPresent() && - !peersDisputeOptional.get().isClosed()) { - showPayoutTxConfirmation(contract, disputeResult, - () -> { - doClose(closeTicketButton); - }); - } else { - doClose(closeTicketButton); } }); @@ -752,7 +753,9 @@ public void onFailure(TxBroadcastException exception) { private void doClose(Button closeTicketButton) { var disputeManager = checkNotNull(getDisputeManager(dispute)); - if (!DelayedPayoutTxValidation.isValidDonationAddress(dispute, daoFacade)) { + try { + DelayedPayoutTxValidation.validateDonationAddress(dispute.getDonationAddressOfDelayedPayoutTx(), daoFacade); + } catch (DelayedPayoutTxValidation.DonationAddressException exception) { showInValidDonationAddressPopup(); // For mediators we do not enforce that the case cannot be closed to stay flexible, From d82631f52ce71bb9b1ff1718ee6cbcc30445a85e Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Fri, 11 Sep 2020 19:41:08 -0500 Subject: [PATCH 4/6] Fix some issues found during testing Refactorings --- .../main/java/bisq/core/dao/DaoFacade.java | 16 +++ .../core/support/dispute/DisputeManager.java | 14 +- .../core/trade/DelayedPayoutTxValidation.java | 125 +++++++++--------- .../java/bisq/core/trade/TradeManager.java | 6 +- .../BuyerVerifiesFinalDelayedPayoutTx.java | 7 +- .../BuyerVerifiesPreparedDelayedPayoutTx.java | 6 +- .../resources/i18n/displayStrings.properties | 5 +- .../windows/DisputeSummaryWindow.java | 65 ++++----- .../pendingtrades/PendingTradesDataModel.java | 6 +- .../steps/buyer/BuyerStep1View.java | 11 +- .../steps/buyer/BuyerStep2View.java | 11 +- .../dispute/agent/DisputeAgentView.java | 19 +-- 12 files changed, 145 insertions(+), 146 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/DaoFacade.java b/core/src/main/java/bisq/core/dao/DaoFacade.java index 50b6402c5a7..bfc542a8621 100644 --- a/core/src/main/java/bisq/core/dao/DaoFacade.java +++ b/core/src/main/java/bisq/core/dao/DaoFacade.java @@ -759,4 +759,20 @@ public Set getAllPastParamValues(Param param) { }); return set; } + + public Set getAllDonationAddresses() { + // We support any of the past addresses as well as in case the peer has not enabled the DAO or is out of sync we + // do not want to break validation. + Set allPastParamValues = getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); + + // If Dao is deactivated we need to add the default address as getAllPastParamValues will not return us any. + allPastParamValues.add(Param.RECIPIENT_BTC_ADDRESS.getDefaultValue()); + + // If Dao is deactivated we need to add the past addresses used as well. + // This list need to be updated once a new address gets defined. + allPastParamValues.add("3EtUWqsGThPtjwUczw27YCo6EWvQdaPUyp"); // burning man 2019 + allPastParamValues.add("3A8Zc1XioE2HRzYfbb5P8iemCS72M6vRJV"); // burningman2 + + return allPastParamValues; + } } diff --git a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java index d327eaad72c..3fc8538e157 100644 --- a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java +++ b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java @@ -74,7 +74,6 @@ import javax.annotation.Nullable; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; @Slf4j @@ -292,7 +291,7 @@ protected void onOpenNewDisputeMessage(OpenNewDisputeMessage openNewDisputeMessa try { DelayedPayoutTxValidation.validateDonationAddress(dispute.getDonationAddressOfDelayedPayoutTx(), daoFacade); - } catch (DelayedPayoutTxValidation.DonationAddressException e) { + } catch (DelayedPayoutTxValidation.AddressException e) { disputesWithInvalidDonationAddress.add(dispute); } @@ -340,7 +339,10 @@ protected void onPeerOpenedDisputeMessage(PeerOpenedDisputeMessage peerOpenedDis Dispute dispute = peerOpenedDisputeMessage.getDispute(); Optional optionalTrade = tradeManager.getTradeById(dispute.getTradeId()); - checkArgument(optionalTrade.isPresent()); + if (!optionalTrade.isPresent()) { + return; + } + Trade trade = optionalTrade.get(); try { DelayedPayoutTxValidation.validatePayoutTx(trade, @@ -348,11 +350,7 @@ protected void onPeerOpenedDisputeMessage(PeerOpenedDisputeMessage peerOpenedDis dispute, daoFacade, btcWalletService); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.InvalidLockTimeException | - DelayedPayoutTxValidation.MissingDelayedPayoutTxException | - DelayedPayoutTxValidation.AmountMismatchException e) { + } catch (DelayedPayoutTxValidation.ValidationException e) { // The peer sent us an invalid donation address. We do not return here as we don't want to break // mediation/arbitration and log only the issue. The dispute agent will run validation as well and will get // a popup displayed to react. diff --git a/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java b/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java index 42717e17460..023f9d608db 100644 --- a/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java +++ b/core/src/main/java/bisq/core/trade/DelayedPayoutTxValidation.java @@ -19,7 +19,6 @@ import bisq.core.btc.wallet.BtcWalletService; import bisq.core.dao.DaoFacade; -import bisq.core.dao.governance.param.Param; import bisq.core.offer.Offer; import bisq.core.support.dispute.Dispute; @@ -44,68 +43,21 @@ @Slf4j public class DelayedPayoutTxValidation { - public static class DonationAddressException extends Exception { - DonationAddressException(String msg) { - super(msg); - } - } - - public static class MissingDelayedPayoutTxException extends Exception { - MissingDelayedPayoutTxException(String msg) { - super(msg); - } - } - - public static class InvalidTxException extends Exception { - InvalidTxException(String msg) { - super(msg); - } - } - - public static class AmountMismatchException extends Exception { - AmountMismatchException(String msg) { - super(msg); - } - } - - public static class InvalidLockTimeException extends Exception { - InvalidLockTimeException(String msg) { - super(msg); - } - } - - public static class InvalidInputException extends Exception { - InvalidInputException(String msg) { - super(msg); - } - } - public static void validateDonationAddress(String addressAsString, DaoFacade daoFacade) - throws DonationAddressException { + throws AddressException { if (addressAsString == null) { + log.warn("address is null at validateDonationAddress. This is expected in case of an not updated trader."); return; } - // We support any of the past addresses as well as in case the peer has not enabled the DAO or is out of sync we - // do not want to break validation. - Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); - - // If Dao is deactivated we need to add the default address as getAllPastParamValues will not return us any. - allPastParamValues.add(Param.RECIPIENT_BTC_ADDRESS.getDefaultValue()); - - // If Dao is deactivated we need to add the past addresses used as well. - // This list need to be updated once a new address gets defined. - allPastParamValues.add("3EtUWqsGThPtjwUczw27YCo6EWvQdaPUyp"); // burning man 2019 - allPastParamValues.add("3A8Zc1XioE2HRzYfbb5P8iemCS72M6vRJV"); // burningman2 - - + Set allPastParamValues = daoFacade.getAllDonationAddresses(); if (!allPastParamValues.contains(addressAsString)) { String errorMsg = "Donation address is not a valid DAO donation address." + "\nAddress used in the dispute: " + addressAsString + "\nAll DAO param donation addresses:" + allPastParamValues; log.error(errorMsg); - throw new DonationAddressException(errorMsg); + throw new AddressException(errorMsg); } } @@ -113,8 +65,8 @@ public static void validatePayoutTx(Trade trade, Transaction delayedPayoutTx, DaoFacade daoFacade, BtcWalletService btcWalletService) - throws DonationAddressException, MissingDelayedPayoutTxException, - InvalidTxException, InvalidLockTimeException, AmountMismatchException { + throws AddressException, MissingTxException, + InvalidTxException, InvalidLockTimeException, InvalidAmountException { validatePayoutTx(trade, delayedPayoutTx, null, @@ -128,8 +80,8 @@ public static void validatePayoutTx(Trade trade, @Nullable Dispute dispute, DaoFacade daoFacade, BtcWalletService btcWalletService) - throws DonationAddressException, MissingDelayedPayoutTxException, - InvalidTxException, InvalidLockTimeException, AmountMismatchException { + throws AddressException, MissingTxException, + InvalidTxException, InvalidLockTimeException, InvalidAmountException { validatePayoutTx(trade, delayedPayoutTx, dispute, @@ -143,8 +95,8 @@ public static void validatePayoutTx(Trade trade, DaoFacade daoFacade, BtcWalletService btcWalletService, @Nullable Consumer addressConsumer) - throws DonationAddressException, MissingDelayedPayoutTxException, - InvalidTxException, InvalidLockTimeException, AmountMismatchException { + throws AddressException, MissingTxException, + InvalidTxException, InvalidLockTimeException, InvalidAmountException { validatePayoutTx(trade, delayedPayoutTx, null, @@ -159,13 +111,13 @@ public static void validatePayoutTx(Trade trade, DaoFacade daoFacade, BtcWalletService btcWalletService, @Nullable Consumer addressConsumer) - throws DonationAddressException, MissingDelayedPayoutTxException, - InvalidTxException, InvalidLockTimeException, AmountMismatchException { + throws AddressException, MissingTxException, + InvalidTxException, InvalidLockTimeException, InvalidAmountException { String errorMsg; if (delayedPayoutTx == null) { errorMsg = "DelayedPayoutTx must not be null"; log.error(errorMsg); - throw new MissingDelayedPayoutTxException("DelayedPayoutTx must not be null"); + throw new MissingTxException("DelayedPayoutTx must not be null"); } // Validate tx structure @@ -213,7 +165,7 @@ public static void validatePayoutTx(Trade trade, errorMsg = "Output value of deposit tx and delayed payout tx is not matching. Output: " + output + " / msOutputAmount: " + msOutputAmount; log.error(errorMsg); log.error(delayedPayoutTx.toString()); - throw new AmountMismatchException(errorMsg); + throw new InvalidAmountException(errorMsg); } NetworkParameters params = btcWalletService.getParams(); @@ -225,7 +177,7 @@ public static void validatePayoutTx(Trade trade, errorMsg = "Donation address cannot be resolved (not of type P2PKHScript or P2SH). Output: " + output; log.error(errorMsg); log.error(delayedPayoutTx.toString()); - throw new DonationAddressException(errorMsg); + throw new AddressException(errorMsg); } } @@ -261,4 +213,51 @@ public static void validatePayoutTxInput(Transaction depositTx, "Deposit tx=" + depositTx); } } + + + /////////////////////////////////////////////////////////////////////////////////////////// + // Exceptions + /////////////////////////////////////////////////////////////////////////////////////////// + + public static class ValidationException extends Exception { + ValidationException(String msg) { + super(msg); + } + } + + public static class AddressException extends ValidationException { + AddressException(String msg) { + super(msg); + } + } + + public static class MissingTxException extends ValidationException { + MissingTxException(String msg) { + super(msg); + } + } + + public static class InvalidTxException extends ValidationException { + InvalidTxException(String msg) { + super(msg); + } + } + + public static class InvalidAmountException extends ValidationException { + InvalidAmountException(String msg) { + super(msg); + } + } + + public static class InvalidLockTimeException extends ValidationException { + InvalidLockTimeException(String msg) { + super(msg); + } + } + + public static class InvalidInputException extends ValidationException { + InvalidInputException(String msg) { + super(msg); + } + } } diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index 66962f10d68..6c0f3d79a3b 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -309,11 +309,7 @@ private void initPendingTrades() { trade.getDelayedPayoutTx(), daoFacade, btcWalletService); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.InvalidLockTimeException | - DelayedPayoutTxValidation.MissingDelayedPayoutTxException | - DelayedPayoutTxValidation.AmountMismatchException e) { + } catch (DelayedPayoutTxValidation.ValidationException e) { log.warn("Delayed payout tx exception, trade {}, exception {}", trade.getId(), e.getMessage()); if (!allowFaultyDelayedTxs) { // We move it to failed trades so it cannot be continued. diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesFinalDelayedPayoutTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesFinalDelayedPayoutTx.java index fa1aeacdbe6..3aaf36a39d9 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesFinalDelayedPayoutTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesFinalDelayedPayoutTx.java @@ -55,12 +55,7 @@ protected void run() { DelayedPayoutTxValidation.validatePayoutTxInput(depositTx, delayedPayoutTx); complete(); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.MissingDelayedPayoutTxException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.InvalidLockTimeException | - DelayedPayoutTxValidation.AmountMismatchException | - DelayedPayoutTxValidation.InvalidInputException e) { + } catch (DelayedPayoutTxValidation.ValidationException e) { failed(e.getMessage()); } catch (Throwable t) { failed(t); diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesPreparedDelayedPayoutTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesPreparedDelayedPayoutTx.java index 3242ba8cf6a..7853767d276 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesPreparedDelayedPayoutTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/buyer/BuyerVerifiesPreparedDelayedPayoutTx.java @@ -43,11 +43,7 @@ protected void run() { processModel.getBtcWalletService()); complete(); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.MissingDelayedPayoutTxException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.InvalidLockTimeException | - DelayedPayoutTxValidation.AmountMismatchException e) { + } catch (DelayedPayoutTxValidation.ValidationException e) { failed(e.getMessage()); } catch (Throwable t) { failed(t); diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index 4156441334f..2f41ef75b0c 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -1086,7 +1086,10 @@ support.warning.disputesWithInvalidDonationAddress=The delayed payout transactio Please inform the developers about that incident and do not close that case before the situation is resolved!\n\n\ Address used in the dispute: {0}\n\n\ All DAO param donation addresses: {1}\n\n\ - Trade ID: {2} + Trade ID: {2}\ + {3} +support.warning.disputesWithInvalidDonationAddress.mediator=\n\nDo you still want to close the dispute? +support.warning.disputesWithInvalidDonationAddress.refundAgent=\n\nYou must not do the payout. #################################################################### diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java index d7ce950ea13..3e349efc4db 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java @@ -35,7 +35,6 @@ import bisq.core.btc.wallet.TradeWalletService; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.dao.DaoFacade; -import bisq.core.dao.governance.param.Param; import bisq.core.locale.Res; import bisq.core.offer.Offer; import bisq.core.provider.fee.FeeService; @@ -87,7 +86,6 @@ import java.util.Date; import java.util.Optional; -import java.util.Set; import java.util.concurrent.TimeUnit; import org.slf4j.Logger; @@ -650,18 +648,12 @@ private void addButtons(Contract contract) { return; } - try { - DelayedPayoutTxValidation.validateDonationAddress(dispute.getDonationAddressOfDelayedPayoutTx(), daoFacade); - - if (dispute.getSupportType() == SupportType.REFUND && - peersDisputeOptional.isPresent() && - !peersDisputeOptional.get().isClosed()) { - showPayoutTxConfirmation(contract, disputeResult, () -> doClose(closeTicketButton)); - } else { - doClose(closeTicketButton); - } - } catch (DelayedPayoutTxValidation.DonationAddressException exception) { - showInValidDonationAddressPopup(); + if (dispute.getSupportType() == SupportType.REFUND && + peersDisputeOptional.isPresent() && + !peersDisputeOptional.get().isClosed()) { + showPayoutTxConfirmation(contract, disputeResult, () -> doCloseIfValid(closeTicketButton)); + } else { + doCloseIfValid(closeTicketButton); } }); @@ -742,7 +734,6 @@ public void onSuccess(Transaction transaction) { public void onFailure(TxBroadcastException exception) { log.error("TxBroadcastException at doPayout", exception); new Popup().error(exception.toString()).show(); - ; } }); } catch (InsufficientMoneyException | WalletException | TransactionVerificationException e) { @@ -751,20 +742,43 @@ public void onFailure(TxBroadcastException exception) { } } - private void doClose(Button closeTicketButton) { + private void doCloseIfValid(Button closeTicketButton) { var disputeManager = checkNotNull(getDisputeManager(dispute)); try { DelayedPayoutTxValidation.validateDonationAddress(dispute.getDonationAddressOfDelayedPayoutTx(), daoFacade); - } catch (DelayedPayoutTxValidation.DonationAddressException exception) { - showInValidDonationAddressPopup(); + doClose(closeTicketButton); + } catch (DelayedPayoutTxValidation.AddressException exception) { + String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); + String tradeId = dispute.getTradeId(); // For mediators we do not enforce that the case cannot be closed to stay flexible, // but for refund agents we do. - if (disputeManager instanceof RefundManager) { - return; + if (disputeManager instanceof MediationManager) { + new Popup().width(900) + .warning(Res.get("support.warning.disputesWithInvalidDonationAddress", + addressAsString, + daoFacade.getAllDonationAddresses(), + tradeId, + Res.get("support.warning.disputesWithInvalidDonationAddress.mediator"))) + .onAction(() -> { + doClose(closeTicketButton); + }) + .actionButtonText(Res.get("shared.yes")) + .closeButtonText(Res.get("shared.no")) + .show(); + } else { + new Popup().width(900) + .warning(Res.get("support.warning.disputesWithInvalidDonationAddress", + addressAsString, + daoFacade.getAllDonationAddresses(), + tradeId, + Res.get("support.warning.disputesWithInvalidDonationAddress.refundAgent"))) + .show(); } } + } + private void doClose(Button closeTicketButton) { disputeResult.setLoserPublisher(isLoserPublisherCheckBox.isSelected()); disputeResult.setCloseDate(new Date()); dispute.setDisputeResult(disputeResult); @@ -789,7 +803,7 @@ private void doClose(Button closeTicketButton) { text += Res.get("disputeSummaryWindow.close.nextStepsForRefundAgentArbitration"); } - disputeManager.sendDisputeResultMessage(disputeResult, dispute, text); + checkNotNull(getDisputeManager(dispute)).sendDisputeResultMessage(disputeResult, dispute, text); if (peersDisputeOptional.isPresent() && !peersDisputeOptional.get().isClosed() && !DevEnv.isDevMode()) { UserThread.runAfter(() -> new Popup() @@ -805,15 +819,6 @@ private void doClose(Button closeTicketButton) { hide(); } - private void showInValidDonationAddressPopup() { - String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); - Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); - String tradeId = dispute.getTradeId(); - new Popup().warning(Res.get("support.warning.disputesWithInvalidDonationAddress", - addressAsString, allPastParamValues, tradeId)) - .show(); - } - private DisputeManager> getDisputeManager(Dispute dispute) { if (dispute.getSupportType() != null) { switch (dispute.getSupportType()) { diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java index 07be33a3c45..3da51c70c78 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesDataModel.java @@ -546,11 +546,7 @@ private void doOpenDispute(boolean isSupportTicket, Transaction depositTx) { daoFacade, btcWalletService, donationAddressString::set); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.InvalidLockTimeException | - DelayedPayoutTxValidation.MissingDelayedPayoutTxException | - DelayedPayoutTxValidation.AmountMismatchException e) { + } catch (DelayedPayoutTxValidation.ValidationException e) { // The peer sent us an invalid donation address. We do not return here as we don't want to break // mediation/arbitration and log only the issue. The dispute agent will run validation as well and will get // a popup displayed to react. diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java index 184dcd038f9..75dfb517c45 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java @@ -43,16 +43,13 @@ public void activate() { trade.getDelayedPayoutTx(), model.dataModel.daoFacade, model.dataModel.btcWalletService); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.AmountMismatchException | - DelayedPayoutTxValidation.InvalidLockTimeException e) { + } catch (DelayedPayoutTxValidation.MissingTxException ignore) { + // We don't react on those errors as a failed trade might get listed initially but getting removed from the + // trade manager after initPendingTrades which happens after activate might be called. + } catch (DelayedPayoutTxValidation.ValidationException e) { if (!model.dataModel.tradeManager.isAllowFaultyDelayedTxs()) { new Popup().warning(Res.get("portfolio.pending.invalidDelayedPayoutTx", e.getMessage())).show(); } - } catch (DelayedPayoutTxValidation.MissingDelayedPayoutTxException ignore) { - // We don't react on those errors as a failed trade might get listed initially but getting removed from the - // trade manager after initPendingTrades which happens after activate might be called. } } diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java index d6d2aeabe33..9011390f599 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java @@ -122,16 +122,13 @@ public void activate() { trade.getDelayedPayoutTx(), model.dataModel.daoFacade, model.dataModel.btcWalletService); - } catch (DelayedPayoutTxValidation.DonationAddressException | - DelayedPayoutTxValidation.InvalidTxException | - DelayedPayoutTxValidation.AmountMismatchException | - DelayedPayoutTxValidation.InvalidLockTimeException e) { + } catch (DelayedPayoutTxValidation.MissingTxException ignore) { + // We don't react on those errors as a failed trade might get listed initially but getting removed from the + // trade manager after initPendingTrades which happens after activate might be called. + } catch (DelayedPayoutTxValidation.ValidationException e) { if (!model.dataModel.tradeManager.isAllowFaultyDelayedTxs()) { new Popup().warning(Res.get("portfolio.pending.invalidDelayedPayoutTx", e.getMessage())).show(); } - } catch (DelayedPayoutTxValidation.MissingDelayedPayoutTxException ignore) { - // We don't react on those errors as a failed trade might get listed initially but getting removed from the - // trade manager after initPendingTrades which happens after activate might be called. } if (timeoutTimer != null) diff --git a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java index 6f6efdd2064..f7d059b1105 100644 --- a/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java +++ b/desktop/src/main/java/bisq/desktop/main/support/dispute/agent/DisputeAgentView.java @@ -28,7 +28,6 @@ import bisq.core.account.witness.AccountAgeWitnessService; import bisq.core.alert.PrivateNotificationManager; import bisq.core.dao.DaoFacade; -import bisq.core.dao.governance.param.Param; import bisq.core.locale.Res; import bisq.core.support.dispute.Dispute; import bisq.core.support.dispute.DisputeList; @@ -59,7 +58,6 @@ import javafx.collections.ListChangeListener; import java.util.List; -import java.util.Set; import static bisq.desktop.util.FormBuilder.getIconForLabel; @@ -126,13 +124,16 @@ public void initialize() { } protected void showWarningForInvalidDonationAddress(List disputes) { - disputes.forEach(dispute -> { - String addressAsString = dispute.getDonationAddressOfDelayedPayoutTx(); - Set allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS); - new Popup().warning(Res.get("support.warning.disputesWithInvalidDonationAddress", - addressAsString, allPastParamValues, dispute.getTradeId())) - .show(); - }); + disputes.stream() + .filter(dispute -> !dispute.isClosed()) + .forEach(dispute -> { + new Popup().warning(Res.get("support.warning.disputesWithInvalidDonationAddress", + dispute.getDonationAddressOfDelayedPayoutTx(), + daoFacade.getAllDonationAddresses(), + dispute.getTradeId(), + "")) + .show(); + }); } @Override From 677211badf15297b1ef6c419ced865679dcbdb0d Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Fri, 11 Sep 2020 20:24:31 -0500 Subject: [PATCH 5/6] Allow close dispute for refund agent without payout --- .../resources/i18n/displayStrings.properties | 3 + .../windows/DisputeSummaryWindow.java | 69 +++++++++++-------- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index 2f41ef75b0c..b72799295ce 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -2466,6 +2466,9 @@ disputeSummaryWindow.close.txDetails=Spending: {0}\n\ Transaction size: {5} Kb\n\n\ Are you sure you want to publish this transaction? +disputeSummaryWindow.close.noPayout.headline=Close without any payout +disputeSummaryWindow.close.noPayout.text=Do you want to close without doing any payout? + emptyWalletWindow.headline={0} emergency wallet tool emptyWalletWindow.info=Please use that only in emergency case if you cannot access your fund from the UI.\n\n\ Please note that all open offers will be closed automatically when using this tool.\n\n\ diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java index 3e349efc4db..b374f457b25 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java @@ -387,14 +387,15 @@ private boolean isPayoutAmountValid() { .add(offer.getSellerSecurityDeposit()); Coin totalAmount = buyerAmount.add(sellerAmount); - if (!totalAmount.isPositive()) { - return false; - } - - if (getDisputeManager(dispute) instanceof RefundManager) { - // We allow to spend less in case of RefundAgent + boolean isRefundAgent = getDisputeManager(dispute) instanceof RefundManager; + if (isRefundAgent) { + // We allow to spend less in case of RefundAgent or even zero to both, so in that case no payout tx will + // be made return totalAmount.compareTo(available) <= 0; } else { + if (!totalAmount.isPositive()) { + return false; + } return totalAmount.compareTo(available) == 0; } } @@ -651,7 +652,9 @@ private void addButtons(Contract contract) { if (dispute.getSupportType() == SupportType.REFUND && peersDisputeOptional.isPresent() && !peersDisputeOptional.get().isClosed()) { - showPayoutTxConfirmation(contract, disputeResult, () -> doCloseIfValid(closeTicketButton)); + showPayoutTxConfirmation(contract, + disputeResult, + () -> doCloseIfValid(closeTicketButton)); } else { doCloseIfValid(closeTicketButton); } @@ -687,28 +690,36 @@ private void showPayoutTxConfirmation(Contract contract, DisputeResult disputeRe formatter.formatCoinWithCode(sellerPayoutAmount), sellerPayoutAddressString); } - new Popup().width(900) - .headLine(Res.get("disputeSummaryWindow.close.txDetails.headline")) - .confirmation(Res.get("disputeSummaryWindow.close.txDetails", - formatter.formatCoinWithCode(inputAmount), - buyerDetails, - sellerDetails, - formatter.formatCoinWithCode(fee), - feePerByte, - kb)) - .actionButtonText(Res.get("shared.yes")) - .onAction(() -> { - doPayout(buyerPayoutAmount, - sellerPayoutAmount, - fee, - buyerPayoutAddressString, - sellerPayoutAddressString, - resultHandler); - }) - .closeButtonText(Res.get("shared.cancel")) - .onClose(() -> { - }) - .show(); + if (outputAmount.isPositive()) { + new Popup().width(900) + .headLine(Res.get("disputeSummaryWindow.close.txDetails.headline")) + .confirmation(Res.get("disputeSummaryWindow.close.txDetails", + formatter.formatCoinWithCode(inputAmount), + buyerDetails, + sellerDetails, + formatter.formatCoinWithCode(fee), + feePerByte, + kb)) + .actionButtonText(Res.get("shared.yes")) + .onAction(() -> { + doPayout(buyerPayoutAmount, + sellerPayoutAmount, + fee, + buyerPayoutAddressString, + sellerPayoutAddressString, + resultHandler); + }) + .closeButtonText(Res.get("shared.cancel")) + .show(); + } else { + // No payout will be made + new Popup().headLine(Res.get("disputeSummaryWindow.close.noPayout.headline")) + .confirmation(Res.get("disputeSummaryWindow.close.noPayout.text")) + .actionButtonText(Res.get("shared.yes")) + .onAction(resultHandler::handleResult) + .closeButtonText(Res.get("shared.cancel")) + .show(); + } } private void doPayout(Coin buyerPayoutAmount, From 08fb5966296c013cce5c5384a10e747260379d56 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Fri, 11 Sep 2020 20:25:48 -0500 Subject: [PATCH 6/6] Call validatePayoutTx only after trades are initialized --- core/src/main/java/bisq/core/trade/Trade.java | 16 ++++-- .../java/bisq/core/trade/TradeManager.java | 1 + .../steps/buyer/BuyerStep1View.java | 53 ++++++++++++++----- .../steps/buyer/BuyerStep2View.java | 45 +++++++++++----- 4 files changed, 88 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/bisq/core/trade/Trade.java b/core/src/main/java/bisq/core/trade/Trade.java index a00fe2dca89..3242bd535ed 100644 --- a/core/src/main/java/bisq/core/trade/Trade.java +++ b/core/src/main/java/bisq/core/trade/Trade.java @@ -725,9 +725,19 @@ public void applyDelayedPayoutTxBytes(byte[] delayedPayoutTxBytes) { @Nullable public Transaction getDelayedPayoutTx() { if (delayedPayoutTx == null) { - delayedPayoutTx = delayedPayoutTxBytes != null && processModel.getBtcWalletService() != null ? - processModel.getBtcWalletService().getTxFromSerializedTx(delayedPayoutTxBytes) : - null; + BtcWalletService btcWalletService = processModel.getBtcWalletService(); + if (btcWalletService == null) { + log.warn("btcWalletService is null. You might call that method before the tradeManager has " + + "initialized all trades"); + return null; + } + + if (delayedPayoutTxBytes == null) { + log.warn("delayedPayoutTxBytes are null"); + return null; + } + + delayedPayoutTx = btcWalletService.getTxFromSerializedTx(delayedPayoutTxBytes); } return delayedPayoutTx; } diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index 6c0f3d79a3b..ea9bcdf2937 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -135,6 +135,7 @@ public class TradeManager implements PersistedDataHost { private final Storage> tradableListStorage; private TradableList tradableList; + @Getter private final BooleanProperty pendingTradesInitialized = new SimpleBooleanProperty(); private List tradesForStatistics; @Setter diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java index 75dfb517c45..716d6bae04b 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep1View.java @@ -24,7 +24,13 @@ import bisq.core.locale.Res; import bisq.core.trade.DelayedPayoutTxValidation; +import bisq.common.UserThread; + +import javafx.beans.property.BooleanProperty; +import javafx.beans.value.ChangeListener; + public class BuyerStep1View extends TradeStepView { + private ChangeListener pendingTradesInitializedListener; /////////////////////////////////////////////////////////////////////////////////////////// // Constructor, Initialisation @@ -38,18 +44,18 @@ public BuyerStep1View(PendingTradesViewModel model) { public void activate() { super.activate(); - try { - DelayedPayoutTxValidation.validatePayoutTx(trade, - trade.getDelayedPayoutTx(), - model.dataModel.daoFacade, - model.dataModel.btcWalletService); - } catch (DelayedPayoutTxValidation.MissingTxException ignore) { - // We don't react on those errors as a failed trade might get listed initially but getting removed from the - // trade manager after initPendingTrades which happens after activate might be called. - } catch (DelayedPayoutTxValidation.ValidationException e) { - if (!model.dataModel.tradeManager.isAllowFaultyDelayedTxs()) { - new Popup().warning(Res.get("portfolio.pending.invalidDelayedPayoutTx", e.getMessage())).show(); - } + // We need to have the trades initialized before we can call validatePayoutTx. + BooleanProperty pendingTradesInitialized = model.dataModel.tradeManager.getPendingTradesInitialized(); + if (pendingTradesInitialized.get()) { + validatePayoutTx(); + } else { + pendingTradesInitializedListener = (observable, oldValue, newValue) -> { + if (newValue) { + validatePayoutTx(); + UserThread.execute(() -> pendingTradesInitialized.removeListener(pendingTradesInitializedListener)); + } + }; + pendingTradesInitialized.addListener(pendingTradesInitializedListener); } } @@ -85,6 +91,29 @@ protected String getFirstHalfOverWarnText() { protected String getPeriodOverWarnText() { return Res.get("portfolio.pending.step1.openForDispute"); } + + + /////////////////////////////////////////////////////////////////////////////////////////// + // Private + /////////////////////////////////////////////////////////////////////////////////////////// + + private void validatePayoutTx() { + try { + DelayedPayoutTxValidation.validatePayoutTx(trade, + trade.getDelayedPayoutTx(), + model.dataModel.daoFacade, + model.dataModel.btcWalletService); + } catch (DelayedPayoutTxValidation.MissingTxException ignore) { + // We don't react on those errors as a failed trade might get listed initially but getting removed from the + // trade manager after initPendingTrades which happens after activate might be called. + log.error(""); + } catch (DelayedPayoutTxValidation.ValidationException e) { + if (!model.dataModel.tradeManager.isAllowFaultyDelayedTxs()) { + new Popup().warning(Res.get("portfolio.pending.invalidDelayedPayoutTx", e.getMessage())).show(); + } + } + } + } diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java index 9011390f599..6096e0cf3d2 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep2View.java @@ -88,6 +88,9 @@ import org.fxmisc.easybind.EasyBind; import org.fxmisc.easybind.Subscription; +import javafx.beans.property.BooleanProperty; +import javafx.beans.value.ChangeListener; + import java.util.List; import java.util.concurrent.TimeUnit; @@ -104,6 +107,7 @@ public class BuyerStep2View extends TradeStepView { private BusyAnimation busyAnimation; private Subscription tradeStatePropertySubscription; private Timer timeoutTimer; + private ChangeListener pendingTradesInitializedListener; /////////////////////////////////////////////////////////////////////////////////////////// // Constructor, Initialisation @@ -117,18 +121,18 @@ public BuyerStep2View(PendingTradesViewModel model) { public void activate() { super.activate(); - try { - DelayedPayoutTxValidation.validatePayoutTx(trade, - trade.getDelayedPayoutTx(), - model.dataModel.daoFacade, - model.dataModel.btcWalletService); - } catch (DelayedPayoutTxValidation.MissingTxException ignore) { - // We don't react on those errors as a failed trade might get listed initially but getting removed from the - // trade manager after initPendingTrades which happens after activate might be called. - } catch (DelayedPayoutTxValidation.ValidationException e) { - if (!model.dataModel.tradeManager.isAllowFaultyDelayedTxs()) { - new Popup().warning(Res.get("portfolio.pending.invalidDelayedPayoutTx", e.getMessage())).show(); - } + // We need to have the trades initialized before we can call validatePayoutTx. + BooleanProperty pendingTradesInitialized = model.dataModel.tradeManager.getPendingTradesInitialized(); + if (pendingTradesInitialized.get()) { + validatePayoutTx(); + } else { + pendingTradesInitializedListener = (observable, oldValue, newValue) -> { + if (newValue) { + validatePayoutTx(); + UserThread.execute(() -> pendingTradesInitialized.removeListener(pendingTradesInitializedListener)); + } + }; + pendingTradesInitialized.addListener(pendingTradesInitializedListener); } if (timeoutTimer != null) @@ -629,6 +633,23 @@ private void showPopup() { } } + private void validatePayoutTx() { + try { + DelayedPayoutTxValidation.validatePayoutTx(trade, + trade.getDelayedPayoutTx(), + model.dataModel.daoFacade, + model.dataModel.btcWalletService); + } catch (DelayedPayoutTxValidation.MissingTxException ignore) { + // We don't react on those errors as a failed trade might get listed initially but getting removed from the + // trade manager after initPendingTrades which happens after activate might be called. + log.error(""); + } catch (DelayedPayoutTxValidation.ValidationException e) { + if (!model.dataModel.tradeManager.isAllowFaultyDelayedTxs()) { + new Popup().warning(Res.get("portfolio.pending.invalidDelayedPayoutTx", e.getMessage())).show(); + } + } + } + @Override protected void updateConfirmButtonDisableState(boolean isDisabled) { confirmButton.setDisable(isDisabled);