From 308f6e8077b096ea5edfdcce0bb43ba1e60b7abd Mon Sep 17 00:00:00 2001 From: woodser Date: Thu, 12 Jan 2023 07:38:09 -0500 Subject: [PATCH] handle errors initializing trade after deposits requested offer remains valid until trade initialized delete maker and taker trades on error after deposits requested schedule trade deletion if unfunded after timeout or startup DepositResponse supports error message to confirm failure show deposit tx ids in trade details window --- .../core/trade/ClosedTradableFormatter.java | 3 -- core/src/main/java/bisq/core/trade/Trade.java | 13 ++++-- .../java/bisq/core/trade/TradeManager.java | 44 +++++++++++++++---- .../core/trade/messages/DepositResponse.java | 15 +++++-- .../core/trade/protocol/TradeProtocol.java | 15 ++++--- .../ArbitratorProcessDepositRequest.java | 34 ++++++++++++-- .../tasks/ProcessDepositResponse.java | 9 ++++ .../resources/i18n/displayStrings.properties | 2 +- .../overlays/windows/TradeDetailsWindow.java | 8 ++-- proto/src/main/proto/pb.proto | 1 + 10 files changed, 111 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/bisq/core/trade/ClosedTradableFormatter.java b/core/src/main/java/bisq/core/trade/ClosedTradableFormatter.java index be2c719010..cf121ee574 100644 --- a/core/src/main/java/bisq/core/trade/ClosedTradableFormatter.java +++ b/core/src/main/java/bisq/core/trade/ClosedTradableFormatter.java @@ -22,14 +22,11 @@ import bisq.core.locale.Res; import bisq.core.monetary.Altcoin; import bisq.core.monetary.Volume; import bisq.core.offer.OpenOffer; -import bisq.core.trade.Tradable; -import bisq.core.trade.Trade; import bisq.core.util.FormattingUtils; import bisq.core.util.coin.CoinFormatter; import org.bitcoinj.core.Coin; import org.bitcoinj.core.Monetary; -import org.bitcoinj.core.TransactionConfidence; import org.bitcoinj.utils.Fiat; import javax.inject.Inject; diff --git a/core/src/main/java/bisq/core/trade/Trade.java b/core/src/main/java/bisq/core/trade/Trade.java index 9fe6c467bf..6d6a05e8ef 100644 --- a/core/src/main/java/bisq/core/trade/Trade.java +++ b/core/src/main/java/bisq/core/trade/Trade.java @@ -635,7 +635,7 @@ public abstract class Trade implements Tradable, Model { isInitialized = true; // start listening to trade wallet - if (isDepositPublished()) { + if (isDepositRequested()) { updateSyncing(); // allow state notifications to process before returning @@ -1462,9 +1462,14 @@ public abstract class Trade implements Tradable, Model { if (isDepositUnlocked()) getWallet().rescanSpent(); // get txs with outputs - List txs = getWallet().getTxs(new MoneroTxQuery() - .setHashes(Arrays.asList(processModel.getMaker().getDepositTxHash(), processModel.getTaker().getDepositTxHash())) - .setIncludeOutputs(true)); + List txs; + try { + txs = getWallet().getTxs(new MoneroTxQuery() + .setHashes(Arrays.asList(processModel.getMaker().getDepositTxHash(), processModel.getTaker().getDepositTxHash())) + .setIncludeOutputs(true)); + } catch (Exception e) { + return; + } // check deposit txs if (!isDepositUnlocked()) { diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index 872e5afd11..6400c1bb30 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -63,6 +63,7 @@ import bisq.network.p2p.P2PService; import bisq.network.p2p.network.TorNetworkNode; import com.google.common.collect.ImmutableList; import bisq.common.ClockWatcher; +import bisq.common.UserThread; import bisq.common.crypto.KeyRing; import bisq.common.handlers.ErrorMessageHandler; import bisq.common.handlers.FaultHandler; @@ -382,6 +383,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi if (isShutDown) return; initTradeAndProtocol(trade, getTradeProtocol(trade)); requestPersistence(); + scheduleDeletionIfUnfunded(trade); } private void initTradeAndProtocol(Trade trade, TradeProtocol tradeProtocol) { @@ -1063,24 +1065,50 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi synchronized(tradableList) { if (!tradableList.contains(trade)) return; - // unreserve key images + // unreserve taker key images if (trade instanceof TakerTrade && trade.getSelf().getReserveTxKeyImages() != null) { xmrWalletService.thawOutputs(trade.getSelf().getReserveTxKeyImages()); xmrWalletService.saveMainWallet(); trade.getSelf().setReserveTxKeyImages(null); } - // stop if trade wallet possibly funded - if (xmrWalletService.multisigWalletExists(trade.getId()) && trade.isDepositRequested()) { - log.warn("Refusing to delete {} {} because trade wallet could be funded", trade.getClass().getSimpleName(), trade.getId()); + // remove trade if wallet deleted + if (!xmrWalletService.multisigWalletExists(trade.getId())) { + removeTrade(trade); return; } - // delete trade wallet if exists - if (xmrWalletService.multisigWalletExists(trade.getId())) trade.deleteWallet(); + // remove trade and wallet unless timeout after deposit requested + boolean isTimeoutError = TradeProtocol.isTimeoutError(trade.getErrorMessage()); + if (!trade.isDepositRequested() || !isTimeoutError) { + removeTrade(trade); + if (xmrWalletService.multisigWalletExists(trade.getId())) trade.deleteWallet(); + } else { + scheduleDeletionIfUnfunded(trade); + } + } + } - // remove trade - removeTrade(trade); + private void scheduleDeletionIfUnfunded(Trade trade) { + if (trade.isDepositRequested() && !trade.isDepositPublished()) { + log.warn("Scheduling to delete trade if unfunded for {} {}", trade.getClass().getSimpleName(), trade.getId()); + UserThread.runAfter(() -> { + if (isShutDown) return; + + // get trade's deposit txs from daemon + MoneroTx makerDepositTx = xmrWalletService.getDaemon().getTx(trade.getMaker().getDepositTxHash()); + MoneroTx takerDepositTx = xmrWalletService.getDaemon().getTx(trade.getTaker().getDepositTxHash()); + + // delete multisig trade wallet if neither deposit tx published + if ((makerDepositTx != null && makerDepositTx.isRelayed()) || (takerDepositTx != null && takerDepositTx.isRelayed())) { + log.warn("Refusing to delete {} {} after protocol timeout because its wallet might be funded", trade.getClass().getSimpleName(), trade.getId()); + } else { + log.warn("Deleting {} {} after protocol timeout", trade.getClass().getSimpleName(), trade.getId()); + removeTrade(trade); + failedTradesManager.removeTrade(trade); + if (xmrWalletService.multisigWalletExists(trade.getId())) trade.deleteWallet(); + } + }, 60); } } diff --git a/core/src/main/java/bisq/core/trade/messages/DepositResponse.java b/core/src/main/java/bisq/core/trade/messages/DepositResponse.java index 9bc3784e77..129d6de5b4 100644 --- a/core/src/main/java/bisq/core/trade/messages/DepositResponse.java +++ b/core/src/main/java/bisq/core/trade/messages/DepositResponse.java @@ -21,8 +21,11 @@ import bisq.core.proto.CoreProtoResolver; import bisq.network.p2p.DirectMessage; import bisq.network.p2p.NodeAddress; -import bisq.common.crypto.PubKeyRing; +import java.util.Optional; + +import bisq.common.crypto.PubKeyRing; +import bisq.common.proto.ProtoUtil; import lombok.EqualsAndHashCode; import lombok.Value; @@ -32,17 +35,20 @@ public final class DepositResponse extends TradeMessage implements DirectMessage private final NodeAddress senderNodeAddress; private final PubKeyRing pubKeyRing; private final long currentDate; + private final String errorMessage; public DepositResponse(String tradeId, NodeAddress senderNodeAddress, PubKeyRing pubKeyRing, String uid, String messageVersion, - long currentDate) { + long currentDate, + String errorMessage) { super(messageVersion, tradeId, uid); this.senderNodeAddress = senderNodeAddress; this.pubKeyRing = pubKeyRing; this.currentDate = currentDate; + this.errorMessage = errorMessage; } @@ -58,6 +64,7 @@ public final class DepositResponse extends TradeMessage implements DirectMessage .setPubKeyRing(pubKeyRing.toProtoMessage()) .setUid(uid); builder.setCurrentDate(currentDate); + Optional.ofNullable(errorMessage).ifPresent(e -> builder.setErrorMessage(errorMessage)); return getNetworkEnvelopeBuilder().setDepositResponse(builder).build(); } @@ -70,7 +77,8 @@ public final class DepositResponse extends TradeMessage implements DirectMessage PubKeyRing.fromProto(proto.getPubKeyRing()), proto.getUid(), messageVersion, - proto.getCurrentDate()); + proto.getCurrentDate(), + ProtoUtil.stringOrNullFromProto(proto.getErrorMessage())); } @Override @@ -79,6 +87,7 @@ public final class DepositResponse extends TradeMessage implements DirectMessage "\n senderNodeAddress=" + senderNodeAddress + ",\n pubKeyRing=" + pubKeyRing + ",\n currentDate=" + currentDate + + ",\n errorMessage=" + errorMessage + "\n} " + super.toString(); } } diff --git a/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java b/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java index 236f7209c0..e9dafc3fb2 100644 --- a/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java +++ b/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java @@ -80,6 +80,7 @@ import javax.annotation.Nullable; public abstract class TradeProtocol implements DecryptedDirectMessageListener, DecryptedMailboxListener { public static final int TRADE_TIMEOUT = 60; + private static final String TIMEOUT_REACHED = "Timeout reached."; protected final ProcessModel processModel; protected final Trade trade; @@ -331,8 +332,7 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D .from(sender)) .setup(tasks( // TODO (woodser): validate request - ProcessSignContractResponse.class, - RemoveOffer.class) + ProcessSignContractResponse.class) .using(new TradeTaskRunner(trade, () -> { startTimeout(TRADE_TIMEOUT); @@ -364,7 +364,8 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D .from(sender)) // TODO (woodser): ensure this asserts sender == response.getSenderNodeAddress() .setup(tasks( // TODO (woodser): validate request - ProcessDepositResponse.class) + ProcessDepositResponse.class, + RemoveOffer.class) .using(new TradeTaskRunner(trade, () -> { stopTimeout(); @@ -548,8 +549,7 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D String err = "Received AckMessage with error state for " + ackMessage.getSourceMsgClassName() + " from "+ peer + " with tradeId " + trade.getId() + " and errorMessage=" + ackMessage.getErrorMessage(); log.warn(err); - stopTimeout(); - if (errorMessageHandler != null) errorMessageHandler.handleErrorMessage(err); + handleError(err); } } @@ -608,7 +608,7 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D synchronized (timeoutTimerLock) { stopTimeout(); timeoutTimer = UserThread.runAfter(() -> { - handleError("Timeout reached. Protocol did not complete in " + timeoutSec + " sec. TradeID=" + trade.getId() + ", state=" + trade.stateProperty().get()); + handleError(TIMEOUT_REACHED + " Protocol did not complete in " + timeoutSec + " sec. TradeID=" + trade.getId() + ", state=" + trade.stateProperty().get()); }, timeoutSec); } } @@ -622,6 +622,9 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D } } + public static boolean isTimeoutError(String errorMessage) { + return errorMessage != null && errorMessage.contains(TIMEOUT_REACHED); + } /////////////////////////////////////////////////////////////////////////////////////////// // Task runner diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java b/core/src/main/java/bisq/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java index 2e2eb07330..02639095cf 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java @@ -23,7 +23,6 @@ import bisq.common.crypto.PubKeyRing; import bisq.common.crypto.Sig; import bisq.common.taskrunner.TaskRunner; import bisq.core.offer.Offer; -import bisq.core.offer.OfferDirection; import bisq.core.trade.HavenoUtils; import bisq.core.trade.Trade; import bisq.core.trade.messages.DepositRequest; @@ -47,6 +46,8 @@ import monero.daemon.model.MoneroSubmitTxResult; @Slf4j public class ArbitratorProcessDepositRequest extends TradeTask { + private boolean depositTxsRelayed = false; + @SuppressWarnings({"unused"}) public ArbitratorProcessDepositRequest(TaskRunner taskHandler, Trade trade) { super(taskHandler, trade); @@ -54,6 +55,7 @@ public class ArbitratorProcessDepositRequest extends TradeTask { @Override protected void run() { + MoneroDaemon daemon = trade.getXmrWalletService().getDaemon(); try { runInterceptHook(); @@ -108,12 +110,12 @@ public class ArbitratorProcessDepositRequest extends TradeTask { if (processModel.getMaker().getDepositTxHex() != null && processModel.getTaker().getDepositTxHex() != null) { // relay txs - MoneroDaemon daemon = trade.getXmrWalletService().getDaemon(); MoneroSubmitTxResult makerResult = daemon.submitTxHex(processModel.getMaker().getDepositTxHex(), true); MoneroSubmitTxResult takerResult = daemon.submitTxHex(processModel.getTaker().getDepositTxHex(), true); if (!makerResult.isGood()) throw new RuntimeException("Error submitting maker deposit tx: " + JsonUtils.serialize(makerResult)); if (!takerResult.isGood()) throw new RuntimeException("Error submitting taker deposit tx: " + JsonUtils.serialize(takerResult)); daemon.relayTxsByHash(Arrays.asList(processModel.getMaker().getDepositTxHash(), processModel.getTaker().getDepositTxHash())); + depositTxsRelayed = true; // update trade state log.info("Arbitrator submitted deposit txs for trade " + trade.getId()); @@ -126,7 +128,8 @@ public class ArbitratorProcessDepositRequest extends TradeTask { processModel.getPubKeyRing(), UUID.randomUUID().toString(), Version.getP2PMessageVersion(), - new Date().getTime()); + new Date().getTime(), + null); // send deposit response to maker and taker sendDepositResponse(trade.getMaker().getNodeAddress(), trade.getMaker().getPubKeyRing(), response); @@ -139,7 +142,30 @@ public class ArbitratorProcessDepositRequest extends TradeTask { // TODO (woodser): request persistence? complete(); } catch (Throwable t) { - failed(t); + + // handle error before deposits relayed + if (!depositTxsRelayed) { + try { + daemon.flushTxPool(processModel.getMaker().getDepositTxHash(), processModel.getTaker().getDepositTxHash()); + } catch (Exception e) { + e.printStackTrace(); + } + + // create deposit response with error + DepositResponse response = new DepositResponse( + trade.getOffer().getId(), + processModel.getMyNodeAddress(), + processModel.getPubKeyRing(), + UUID.randomUUID().toString(), + Version.getP2PMessageVersion(), + new Date().getTime(), + t.getMessage()); + + // send deposit response to maker and taker + sendDepositResponse(trade.getMaker().getNodeAddress(), trade.getMaker().getPubKeyRing(), response); + sendDepositResponse(trade.getTaker().getNodeAddress(), trade.getTaker().getPubKeyRing(), response); + } + failed(t); } } diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessDepositResponse.java b/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessDepositResponse.java index 212a9f7bdf..12d0c4a956 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessDepositResponse.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessDepositResponse.java @@ -20,6 +20,7 @@ package bisq.core.trade.protocol.tasks; import bisq.common.taskrunner.TaskRunner; import bisq.core.trade.Trade; +import bisq.core.trade.messages.DepositResponse; import lombok.extern.slf4j.Slf4j; @Slf4j @@ -34,6 +35,14 @@ public class ProcessDepositResponse extends TradeTask { protected void run() { try { runInterceptHook(); + + // throw if error + DepositResponse message = (DepositResponse) processModel.getTradeMessage(); + if (message.getErrorMessage() != null) { + throw new RuntimeException(message.getErrorMessage()); + } + + // set success state trade.setStateIfValidTransitionTo(Trade.State.ARBITRATOR_PUBLISHED_DEPOSIT_TXS); processModel.getTradeManager().requestPersistence(); complete(); diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index 64b4f3d3e9..339b52fb39 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -2036,7 +2036,7 @@ The above error message will be copied to the clipboard when you click either of It will make debugging easier if you include the haveno.log file by pressing "Open log file", saving a copy, and attaching it to your bug report. popup.error.tryRestart=Please try to restart your application and check your network connection to see if you can resolve the issue. -popup.error.takeOfferRequestFailed=An error occurred when someone tried to take one of your offers:\n{0} +popup.error.takeOfferRequestFailed=An error occurred while taking an offer:\n{0} error.spvFileCorrupted=An error occurred when reading the SPV chain file.\nIt might be that the SPV chain file is corrupted.\n\nError message: {0}\n\nDo you want to delete it and start a resync? error.deleteAddressEntryListFailed=Could not delete AddressEntryList file.\nError: {0} diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/windows/TradeDetailsWindow.java b/desktop/src/main/java/bisq/desktop/main/overlays/windows/TradeDetailsWindow.java index 452da7f719..e07aa9bed5 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/windows/TradeDetailsWindow.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/windows/TradeDetailsWindow.java @@ -269,12 +269,12 @@ public class TradeDetailsWindow extends Overlay { Res.get(contract.getPaymentMethodId())); } - if (trade.getMakerDepositTx() != null) + if (trade.getMaker().getDepositTxHash() != null) addLabelTxIdTextField(gridPane, ++rowIndex, Res.get("shared.makerDepositTransactionId"), - trade.getMakerDepositTx().getHash()); - if (trade.getTakerDepositTx() != null) + trade.getMaker().getDepositTxHash()); + if (trade.getTaker().getDepositTxHash() != null) addLabelTxIdTextField(gridPane, ++rowIndex, Res.get("shared.takerDepositTransactionId"), - trade.getTakerDepositTx().getHash()); + trade.getTaker().getDepositTxHash()); if (trade.getPayoutTxId() != null && !trade.getPayoutTxId().isBlank()) addLabelTxIdTextField(gridPane, ++rowIndex, Res.get("shared.payoutTxId"), diff --git a/proto/src/main/proto/pb.proto b/proto/src/main/proto/pb.proto index ff1d5744ee..13eb6b972f 100644 --- a/proto/src/main/proto/pb.proto +++ b/proto/src/main/proto/pb.proto @@ -344,6 +344,7 @@ message DepositResponse { PubKeyRing pub_key_ring = 3; string uid = 4; int64 current_date = 5; + string error_message = 6; } message DepositsConfirmedMessage {