refactor trade protocol error handling and wallet deletion

core/src/main/java/haveno/core/trade/Trade.java

@@ -50,6 +50,7 @@ import haveno.core.monetary.Volume;
 import haveno.core.network.MessageState;
 import haveno.core.offer.Offer;
 import haveno.core.offer.OfferDirection;
+import haveno.core.offer.OpenOffer;
 import haveno.core.payment.payload.PaymentAccountPayload;
 import haveno.core.proto.CoreProtoResolver;
 import haveno.core.proto.network.CoreNetworkProtoResolver;
@@ -73,12 +74,14 @@ import haveno.network.p2p.NodeAddress;
 import haveno.network.p2p.P2PService;
 import javafx.beans.property.DoubleProperty;
 import javafx.beans.property.IntegerProperty;
+import javafx.beans.property.LongProperty;
 import javafx.beans.property.ObjectProperty;
 import javafx.beans.property.ReadOnlyDoubleProperty;
 import javafx.beans.property.ReadOnlyObjectProperty;
 import javafx.beans.property.ReadOnlyStringProperty;
 import javafx.beans.property.SimpleDoubleProperty;
 import javafx.beans.property.SimpleIntegerProperty;
+import javafx.beans.property.SimpleLongProperty;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.beans.property.SimpleStringProperty;
 import javafx.beans.property.StringProperty;
@@ -133,14 +136,18 @@ public abstract class Trade implements Tradable, Model {
 
     private static final String MONERO_TRADE_WALLET_PREFIX = "xmr_trade_";
     private static final long SHUTDOWN_TIMEOUT_MS = 60000;
-    private static final long DELETE_BACKUPS_AFTER_NUM_BLOCKS = 3600; // ~5 days
     private static final long SYNC_EVERY_NUM_BLOCKS = 360; // ~1/2 day
+    private static final long DELETE_AFTER_NUM_BLOCKS = 1; // if deposit requested but not published
+    private static final long DELETE_AFTER_MS = TradeProtocol.TRADE_STEP_TIMEOUT_SECONDS;
     private final Object walletLock = new Object();
     private final Object pollLock = new Object();
+    private final LongProperty walletHeight = new SimpleLongProperty(0);
     private MoneroWallet wallet;
     boolean wasWalletSynced;
     boolean pollInProgress;
     boolean restartInProgress;
+    private Subscription protocolErrorStateSubscription;
+    private Subscription protocolErrorHeightSubscription;
 
     ///////////////////////////////////////////////////////////////////////////////////////////
     // Enums
@@ -643,6 +650,7 @@ public abstract class Trade implements Tradable, Model {
             if (!isInitialized || isShutDownStarted) return;
             ThreadUtils.submitToPool(() -> {
                 if (newValue == Trade.Phase.DEPOSIT_REQUESTED) startPolling();
+                if (newValue == Trade.Phase.DEPOSITS_PUBLISHED) xmrWalletService.freezeOutputs(getSelf().getReserveTxKeyImages());
                 if (isDepositsPublished() && !isPayoutUnlocked()) updatePollPeriod();
                 if (isPaymentReceived()) {
                     UserThread.execute(() -> {
@@ -785,6 +793,10 @@ public abstract class Trade implements Tradable, Model {
         }
     }
 
+    public long getHeight() {
+        return walletHeight.get();
+    }
+
     private String getWalletName() {
         return MONERO_TRADE_WALLET_PREFIX + getShortId() + "_" + getShortUid();
     }
@@ -814,7 +826,7 @@ public abstract class Trade implements Tradable, Model {
             if (!xmrConnectionService.isSyncedWithinTolerance()) return false;
             Long targetHeight = xmrConnectionService.getTargetHeight();
             if (targetHeight == null) return false;
-            if (targetHeight - wallet.getHeight() <= 3) return true; // synced if within 3 blocks of target height
+            if (targetHeight - walletHeight.get() <= 3) return true; // synced if within 3 blocks of target height
             return false;
         }
     }
@@ -941,7 +953,7 @@ public abstract class Trade implements Tradable, Model {
                     }
 
                     // wallet must be synced
-                    if (isDepositRequested() && !isSyncedWithinTolerance()) {
+                    if (isDepositRequested() && isWalletBehind()) {
                         log.warn("Wallet is not synced for {} {}, syncing", getClass().getSimpleName(), getId());
                         syncWallet(true);
                     }
@@ -966,19 +978,9 @@ public abstract class Trade implements Tradable, Model {
                     forceCloseWallet();
 
                     // delete wallet
-                    log.info("Deleting wallet for {} {}", getClass().getSimpleName(), getId());
+                    log.info("Deleting wallet and backups for {} {}", getClass().getSimpleName(), getId());
                     xmrWalletService.deleteWallet(getWalletName());
-
+                    xmrWalletService.deleteWalletBackups(getWalletName());
-                    // delete trade wallet backups if empty and payout unlocked, else schedule
-                    if (isPayoutUnlocked() || !isDepositRequested() || isDepositFailed()) {
-                        xmrWalletService.deleteWalletBackups(getWalletName());
-                    } else {
-
-                        // schedule backup deletion by recording delete height
-                        log.warn("Scheduling to delete backup wallet for " + getClass().getSimpleName() + " " + getId() + " in the small chance it becomes funded");
-                        processModel.setDeleteBackupsHeight(xmrConnectionService.getLastInfo().getHeight() + DELETE_BACKUPS_AFTER_NUM_BLOCKS);
-                        maybeScheduleDeleteBackups();
-                    }
                 } catch (Exception e) {
                     log.warn(e.getMessage());
                     e.printStackTrace();
@@ -1290,9 +1292,6 @@ public abstract class Trade implements Tradable, Model {
 
     private void clearProcessData() {
 
-        // delete backup wallets after main wallet + blocks
-        maybeScheduleDeleteBackups();
-
         // delete trade wallet
         synchronized (walletLock) {
             if (!walletExists()) return; // done if already cleared
@@ -1310,27 +1309,6 @@ public abstract class Trade implements Tradable, Model {
         }
     }
 
-    private void maybeScheduleDeleteBackups() {
-        if (processModel.getDeleteBackupsHeight() == 0) return;
-        if (xmrConnectionService.getLastInfo().getHeight() >= processModel.getDeleteBackupsHeight()) {
-            xmrWalletService.deleteWalletBackups(getWalletName());
-            processModel.setDeleteBackupsHeight(0); // reset delete height
-        } else {
-            MoneroWalletListener deleteBackupsListener = new MoneroWalletListener() {
-                @Override
-                public synchronized void onNewBlock(long height) { // prevent concurrent deletion
-                    if (processModel.getDeleteBackupsHeight() == 0) return;
-                    if (xmrConnectionService.getLastInfo().getHeight() >= processModel.getDeleteBackupsHeight()) {
-                        xmrWalletService.deleteWalletBackups(getWalletName());
-                        processModel.setDeleteBackupsHeight(0); // reset delete height
-                        xmrWalletService.removeWalletListener(this);
-                    }
-                }
-            };
-            xmrWalletService.addWalletListener(deleteBackupsListener);
-        }
-    }
-
     public void maybeClearSensitiveData() {
         String change = "";
         if (removeAllChatMessages()) {
@@ -1409,6 +1387,127 @@ public abstract class Trade implements Tradable, Model {
         });
     }
 
+    ///////////////////////////////////////////////////////////////////////////////////////////
+    // Trade error cleanup
+    ///////////////////////////////////////////////////////////////////////////////////////////
+
+    public void onProtocolError() {
+
+        // check if deposit published
+        if (isDepositsPublished()) {
+            restorePublishedTrade();
+            return;
+        }
+
+        // unreserve taker key images
+        if (this instanceof TakerTrade) {
+            ThreadUtils.submitToPool(() -> {
+                xmrWalletService.thawOutputs(getSelf().getReserveTxKeyImages());
+            });
+        }
+
+        // unreserve open offer
+        Optional<OpenOffer> openOffer = processModel.getOpenOfferManager().getOpenOfferById(this.getId());
+        if (this instanceof MakerTrade && openOffer.isPresent()) {
+            processModel.getOpenOfferManager().unreserveOpenOffer(openOffer.get());
+        }
+
+        // remove if deposit not requested or is failed
+        if (!isDepositRequested() || isDepositFailed()) {
+            removeTradeOnError();
+            return;
+        }
+
+        // done if wallet already deleted
+        if (!walletExists()) return;
+
+        // move to failed trades
+        processModel.getTradeManager().onMoveInvalidTradeToFailedTrades(this);
+
+        // set error height
+        if (processModel.getTradeProtocolErrorHeight() == 0) {
+            log.warn("Scheduling to remove trade if unfunded for {} {} from height {}", getClass().getSimpleName(), getId(), xmrConnectionService.getLastInfo().getHeight());
+            processModel.setTradeProtocolErrorHeight(xmrConnectionService.getLastInfo().getHeight());
+        }
+
+        // listen for deposits published to restore trade
+        protocolErrorStateSubscription = EasyBind.subscribe(stateProperty(), state -> {
+            if (isDepositsPublished()) {
+                restorePublishedTrade();
+                if (protocolErrorStateSubscription != null) {    // unsubscribe
+                    protocolErrorStateSubscription.unsubscribe();
+                    protocolErrorStateSubscription = null;
+                }
+            }
+        });
+
+        // listen for block confirmations to remove trade
+        long startTime = System.currentTimeMillis();
+        protocolErrorHeightSubscription = EasyBind.subscribe(walletHeight, lastWalletHeight -> {
+            if (isShutDown || isDepositsPublished()) return;
+            if (lastWalletHeight.longValue() < processModel.getTradeProtocolErrorHeight() + DELETE_AFTER_NUM_BLOCKS) return;
+            if (System.currentTimeMillis() - startTime < DELETE_AFTER_MS) return;
+
+            // remove trade off thread
+            ThreadUtils.submitToPool(() -> {
+
+                // get trade's deposit txs from daemon
+                MoneroTx makerDepositTx = getMaker().getDepositTxHash() == null ? null : xmrWalletService.getDaemon().getTx(getMaker().getDepositTxHash());
+                MoneroTx takerDepositTx = getTaker().getDepositTxHash() == null ? null : xmrWalletService.getDaemon().getTx(getTaker().getDepositTxHash());
+
+                // remove trade and wallet if neither deposit tx published
+                if (makerDepositTx == null && takerDepositTx == null) {
+                    log.warn("Deleting {} {} after protocol error", getClass().getSimpleName(), getId());
+                    if (this instanceof ArbitratorTrade && (getMaker().getReserveTxHash() != null || getTaker().getReserveTxHash() != null)) {
+                        processModel.getTradeManager().onMoveInvalidTradeToFailedTrades(this); // arbitrator retains trades with reserved funds for analysis and penalty
+                        deleteWallet();
+                        onShutDownStarted();
+                        ThreadUtils.submitToPool(() -> shutDown()); // run off thread
+                    } else {
+                        removeTradeOnError();
+                    }
+                } else if (!isPayoutPublished()) {
+
+                    // set error if wallet may be partially funded
+                    String errorMessage = "Refusing to delete " + getClass().getSimpleName() + " " + getId() + " after protocol error because its wallet might be funded";
+                    prependErrorMessage(errorMessage);
+                    log.warn(errorMessage);
+                }
+
+                // unsubscribe
+                if (protocolErrorHeightSubscription != null) {
+                    protocolErrorHeightSubscription.unsubscribe();
+                    protocolErrorHeightSubscription = null;
+                }
+            });
+        });
+    }
+
+    private void restorePublishedTrade() {
+
+        // close open offer
+        if (this instanceof MakerTrade && processModel.getOpenOfferManager().getOpenOfferById(getId()).isPresent()) {
+            log.info("Closing open offer because {} {} was restored after protocol error", getClass().getSimpleName(), getShortId());
+            processModel.getOpenOfferManager().closeOpenOffer(checkNotNull(getOffer()));
+        }
+
+        // re-freeze outputs
+        xmrWalletService.freezeOutputs(getSelf().getReserveTxKeyImages());
+
+        // restore trade from failed trades
+        processModel.getTradeManager().onMoveFailedTradeToPendingTrades(this);
+    }
+
+    private void removeTradeOnError() {
+        log.warn("removeTradeOnError() trade={}, tradeId={}, state={}", getClass().getSimpleName(), getShortId(), getState());
+
+        // clear and shut down trade
+        clearAndShutDown();
+
+        // unregister trade
+        processModel.getTradeManager().unregisterTrade(this);
+    }
+
     ///////////////////////////////////////////////////////////////////////////////////////////
     // Model implementation
     ///////////////////////////////////////////////////////////////////////////////////////////
@@ -1815,6 +1914,7 @@ public abstract class Trade implements Tradable, Model {
     }
 
     public boolean isDepositsPublished() {
+        if (isDepositFailed()) return false;
         return getState().getPhase().ordinal() >= Phase.DEPOSITS_PUBLISHED.ordinal() && getMaker().getDepositTxHash() != null && getTaker().getDepositTxHash() != null;
     }
 
@@ -1935,9 +2035,11 @@ public abstract class Trade implements Tradable, Model {
 
     public BigInteger getFrozenAmount() {
         BigInteger sum = BigInteger.ZERO;
-        for (String keyImage : getSelf().getReserveTxKeyImages()) {
+        if (getSelf().getReserveTxKeyImages() != null) {
-            List<MoneroOutputWallet> outputs = xmrWalletService.getOutputs(new MoneroOutputQuery().setIsFrozen(true).setIsSpent(false).setKeyImage(new MoneroKeyImage(keyImage)));
+            for (String keyImage : getSelf().getReserveTxKeyImages()) {
-            if (!outputs.isEmpty()) sum = sum.add(outputs.get(0).getAmount());
+                List<MoneroOutputWallet> outputs = xmrWalletService.getOutputs(new MoneroOutputQuery().setIsFrozen(true).setIsSpent(false).setKeyImage(new MoneroKeyImage(keyImage)));
+                if (!outputs.isEmpty()) sum = sum.add(outputs.get(0).getAmount());
+            }
         }
         return sum;
     }
@@ -2169,12 +2271,12 @@ public abstract class Trade implements Tradable, Model {
                 // skip if payout unlocked
                 if (isPayoutUnlocked()) return;
 
-                // skip if either deposit tx id is unknown
+                // skip if deposit txs unknown or not requested
-                if (processModel.getMaker().getDepositTxHash() == null || processModel.getTaker().getDepositTxHash() == null) return;
+                if (processModel.getMaker().getDepositTxHash() == null || processModel.getTaker().getDepositTxHash() == null || !isDepositRequested()) return;
 
                 // sync if wallet too far behind daemon
                 if (xmrConnectionService.getTargetHeight() == null) return;
-                if (wallet.getHeight() < xmrConnectionService.getTargetHeight() - SYNC_EVERY_NUM_BLOCKS) syncWallet(false);
+                if (walletHeight.get() < xmrConnectionService.getTargetHeight() - SYNC_EVERY_NUM_BLOCKS) syncWallet(false);
 
                 // update deposit txs
                 if (!isDepositsUnlocked()) {
@@ -2286,12 +2388,13 @@ public abstract class Trade implements Tradable, Model {
         if (isWalletBehind()) {
             synchronized (walletLock) {
                 xmrWalletService.syncWallet(wallet);
+                walletHeight.set(wallet.getHeight());
             }
         }
     }
 
     private boolean isWalletBehind() {
-        return wallet.getHeight() < xmrConnectionService.getTargetHeight();
+        return walletHeight.get() < xmrConnectionService.getTargetHeight();
     }
 
     private void setDepositTxs(List<? extends MoneroTx> txs) {

core/src/main/java/haveno/core/trade/TradeManager.java

@@ -133,7 +133,6 @@ import monero.daemon.model.MoneroTx;
 import org.bitcoinj.core.Coin;
 import org.bouncycastle.crypto.params.KeyParameter;
 import org.fxmisc.easybind.EasyBind;
-import org.fxmisc.easybind.Subscription;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -434,7 +433,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
             Set<Runnable> tasks = new HashSet<Runnable>();
             Set<String> uids = new HashSet<String>();
             Set<Trade> tradesToSkip = new HashSet<Trade>();
-            Set<Trade> tradesToMaybeRemoveOnError = new HashSet<Trade>();
+            Set<Trade> uninitializedTrades = new HashSet<Trade>();
             for (Trade trade : trades) {
                 tasks.add(() -> {
                     try {
@@ -451,7 +450,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
 
                         // remove trade if protocol didn't initialize
                         if (getOpenTradeByUid(trade.getUid()).isPresent() && !trade.isDepositsPublished()) {
-                            tradesToMaybeRemoveOnError.add(trade);
+                            uninitializedTrades.add(trade);
                         }
                     } catch (Exception e) {
                         if (!isShutDownStarted) {
@@ -477,9 +476,9 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
             // process after all wallets initialized
             if (!HavenoUtils.isSeedNode()) {
 
-                // maybe remove trades on error
+                // handle uninitialized trades
-                for (Trade trade : tradesToMaybeRemoveOnError) {
+                for (Trade trade : uninitializedTrades) {
-                    maybeRemoveTradeOnError(trade);
+                    trade.onProtocolError();
                 }
 
                 // freeze or thaw outputs
@@ -623,7 +622,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
           // process with protocol
           ((ArbitratorProtocol) getTradeProtocol(trade)).handleInitTradeRequest(request, sender, errorMessage -> {
               log.warn("Arbitrator error during trade initialization for trade {}: {}", trade.getId(), errorMessage);
-              maybeRemoveTradeOnError(trade);
+              trade.onProtocolError();
           });
 
           requestPersistence();
@@ -704,7 +703,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
           // process with protocol
           ((MakerProtocol) getTradeProtocol(trade)).handleInitTradeRequest(request, sender, errorMessage -> {
               log.warn("Maker error during trade initialization: " + errorMessage);
-              maybeRemoveTradeOnError(trade);
+              trade.onProtocolError();
           });
       }
     }
@@ -797,8 +796,11 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
 
         Optional<Trade> tradeOptional = getOpenTrade(response.getOfferId());
         if (!tradeOptional.isPresent()) {
-            log.warn("No trade with id " + response.getOfferId());
+            tradeOptional = getFailedTrade(response.getOfferId());
-            return;
+            if (!tradeOptional.isPresent()) {
+                log.warn("No trade with id " + response.getOfferId());
+                return;
+            }
         }
         Trade trade = tradeOptional.get();
         ((TraderProtocol) getTradeProtocol(trade)).handleDepositResponse(response, peer);
@@ -885,8 +887,8 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
                             requestPersistence();
                         }, errorMessage -> {
                             log.warn("Taker error during trade initialization: " + errorMessage);
-                            xmrWalletService.resetAddressEntriesForOpenOffer(trade.getId());
+                            xmrWalletService.resetAddressEntriesForOpenOffer(trade.getId()); // TODO: move to maybe remove on error
-                            maybeRemoveTradeOnError(trade);
+                            trade.onProtocolError();
                             errorMessageHandler.handleErrorMessage(errorMessage);
                         });
                         requestPersistence();
@@ -945,13 +947,32 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
         if (trade.isCompleted()) throw new RuntimeException("Trade " + trade.getId() + " was already completed");
         closedTradableManager.add(trade);
         trade.setCompleted(true);
-        removeTrade(trade);
+        removeTrade(trade, true);
 
         // TODO The address entry should have been removed already. Check and if its the case remove that.
         xmrWalletService.resetAddressEntriesForTrade(trade.getId());
         requestPersistence();
     }
 
+    public void unregisterTrade(Trade trade) {
+        removeTrade(trade, true);
+        removeFailedTrade(trade);
+        requestPersistence();
+    }
+
+    public void removeTrade(Trade trade, boolean removeDirectMessageListener) {
+        log.info("TradeManager.removeTrade() " + trade.getId());
+        
+        // remove trade
+        synchronized (tradableList) {
+            if (!tradableList.remove(trade)) return;
+        }
+
+        // unregister message listener and persist
+        if (removeDirectMessageListener) p2PService.removeDecryptedDirectMessageListener(getTradeProtocol(trade));
+        requestPersistence();
+    }
+
 
     ///////////////////////////////////////////////////////////////////////////////////////////
     // Dispute
@@ -1014,8 +1035,17 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
     // If trade is in already in critical state (if taker role: taker fee; both roles: after deposit published)
     // we move the trade to FailedTradesManager
     public void onMoveInvalidTradeToFailedTrades(Trade trade) {
-        removeTrade(trade);
         failedTradesManager.add(trade);
+        removeTrade(trade, false);
+    }
+
+    public void onMoveFailedTradeToPendingTrades(Trade trade) {
+        addFailedTradeToPendingTrades(trade);
+        failedTradesManager.removeTrade(trade);
+    }
+
+    public void removeFailedTrade(Trade trade) {
+        failedTradesManager.removeTrade(trade);
     }
 
     public void addFailedTradeToPendingTrades(Trade trade) {
@@ -1255,132 +1285,6 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
         }
     }
 
-    private void removeTrade(Trade trade) {
-        log.info("TradeManager.removeTrade() " + trade.getId());
-        
-        // remove trade
-        synchronized (tradableList) {
-            if (!tradableList.remove(trade)) return;
-        }
-
-        // unregister and persist
-        p2PService.removeDecryptedDirectMessageListener(getTradeProtocol(trade));
-        requestPersistence();
-    }
-
-    private void maybeRemoveTradeOnError(Trade trade) {
-        if (trade.isDepositRequested() && !trade.isDepositFailed()) {
-            listenForCleanup(trade);
-        } else {
-            removeTradeOnError(trade);
-        }
-    }
-
-    private void removeTradeOnError(Trade trade) {
-        log.warn("TradeManager.removeTradeOnError() trade={}, tradeId={}, state={}", trade.getClass().getSimpleName(), trade.getShortId(), trade.getState());
-
-        // unreserve taker key images
-        if (trade instanceof TakerTrade) {
-            xmrWalletService.thawOutputs(trade.getSelf().getReserveTxKeyImages());
-            trade.getSelf().setReserveTxKeyImages(null);
-        }
-
-        // unreserve open offer
-        Optional<OpenOffer> openOffer = openOfferManager.getOpenOfferById(trade.getId());
-        if (trade instanceof MakerTrade && openOffer.isPresent()) {
-            openOfferManager.unreserveOpenOffer(openOffer.get());
-        }
-
-        // clear and shut down trade
-        trade.clearAndShutDown();
-
-        // remove trade from list
-        removeTrade(trade);
-    }
-
-    private void listenForCleanup(Trade trade) {
-        if (getOpenTrade(trade.getId()).isPresent() && trade.isDepositRequested()) {
-            if (trade.isDepositsPublished()) {
-                cleanupPublishedTrade(trade);
-            } else {
-                log.warn("Scheduling to delete open trade if unfunded for {} {}", trade.getClass().getSimpleName(), trade.getId());
-                new TradeCleanupListener(trade); // TODO: better way than creating listener?
-            }
-        }
-    }
-
-    private void cleanupPublishedTrade(Trade trade) {
-        if (trade instanceof MakerTrade && openOfferManager.getOpenOfferById(trade.getId()).isPresent()) {
-            log.warn("Closing open offer as cleanup step");
-            openOfferManager.closeOpenOffer(checkNotNull(trade.getOffer()));
-        }
-    }
-
-    private class TradeCleanupListener {
-
-        private static final long REMOVE_AFTER_MS = 60000;
-        private static final int REMOVE_AFTER_NUM_CONFIRMATIONS = 1;
-        private Long startHeight;
-        private Subscription stateSubscription;
-        private Subscription heightSubscription;
-
-        public TradeCleanupListener(Trade trade) {
-
-            // listen for deposits published to close open offer
-            stateSubscription = EasyBind.subscribe(trade.stateProperty(), state -> {
-                if (trade.isDepositsPublished()) {
-                    cleanupPublishedTrade(trade);
-                    if (stateSubscription != null) {
-                        stateSubscription.unsubscribe();
-                        stateSubscription = null;
-                    }
-                }
-            });
-
-            // listen for block confirmation to remove trade
-            long startTime = System.currentTimeMillis();
-            heightSubscription = EasyBind.subscribe(xmrWalletService.getConnectionService().chainHeightProperty(), lastBlockHeight -> {
-                if (isShutDown) return;
-                if (startHeight == null) startHeight = lastBlockHeight.longValue();
-                if (lastBlockHeight.longValue() >= startHeight + REMOVE_AFTER_NUM_CONFIRMATIONS) {
-                    new Thread(() -> {
-
-                        // wait minimum time
-                        HavenoUtils.waitFor(Math.max(0, REMOVE_AFTER_MS - (System.currentTimeMillis() - startTime)));
-
-                        // get trade's deposit txs from daemon
-                        MoneroTx makerDepositTx = trade.getMaker().getDepositTxHash() == null ? null : xmrWalletService.getDaemon().getTx(trade.getMaker().getDepositTxHash());
-                        MoneroTx takerDepositTx = trade.getTaker().getDepositTxHash() == null ? null : xmrWalletService.getDaemon().getTx(trade.getTaker().getDepositTxHash());
-
-                        // remove trade and wallet if neither deposit tx published
-                        if (makerDepositTx == null && takerDepositTx == null) {
-                            log.warn("Deleting {} {} after protocol error", trade.getClass().getSimpleName(), trade.getId());
-                            if (trade instanceof ArbitratorTrade && (trade.getMaker().getReserveTxHash() != null || trade.getTaker().getReserveTxHash() != null)) {
-                                onMoveInvalidTradeToFailedTrades(trade); // arbitrator retains trades with reserved funds for analysis and penalty
-                            } else {
-                                removeTradeOnError(trade);
-                                failedTradesManager.removeTrade(trade);
-                            }
-                        } else if (!trade.isPayoutPublished()) {
-
-                            // set error that wallet may be partially funded
-                            String errorMessage = "Refusing to delete " + trade.getClass().getSimpleName() + " " + trade.getId() + " after protocol timeout because its wallet might be funded";
-                            trade.prependErrorMessage(errorMessage);
-                            log.warn(errorMessage);
-                        }
-
-                        // unsubscribe
-                        if (heightSubscription != null) {
-                            heightSubscription.unsubscribe();
-                            heightSubscription = null;
-                        }
-
-                    }).start();
-                }
-            });
-        }
-    }
-
     // TODO Remove once tradableList is refactored to a final field
     //  (part of the persistence refactor PR)
     private void onTradesChanged() {

core/src/main/java/haveno/core/trade/protocol/ProcessModel.java

@@ -157,7 +157,7 @@ public class ProcessModel implements Model, PersistablePayload {
     private String multisigAddress;
     @Getter
     @Setter
-    private long deleteBackupsHeight;
+    private long tradeProtocolErrorHeight;
 
     // We want to indicate the user the state of the message delivery of the
     // PaymentSentMessage. As well we do an automatic re-send in case it was not ACKed yet.
@@ -207,7 +207,7 @@ public class ProcessModel implements Model, PersistablePayload {
                 .setPaymentSentMessageStateArbitrator(paymentSentMessageStatePropertyArbitrator.get().name())
                 .setBuyerPayoutAmountFromMediation(buyerPayoutAmountFromMediation)
                 .setSellerPayoutAmountFromMediation(sellerPayoutAmountFromMediation)
-                .setDeleteBackupsHeight(deleteBackupsHeight);
+                .setTradeProtocolErrorHeight(tradeProtocolErrorHeight);
         Optional.ofNullable(maker).ifPresent(e -> builder.setMaker((protobuf.TradePeer) maker.toProtoMessage()));
         Optional.ofNullable(taker).ifPresent(e -> builder.setTaker((protobuf.TradePeer) taker.toProtoMessage()));
         Optional.ofNullable(arbitrator).ifPresent(e -> builder.setArbitrator((protobuf.TradePeer) arbitrator.toProtoMessage()));
@@ -230,7 +230,7 @@ public class ProcessModel implements Model, PersistablePayload {
         processModel.setFundsNeededForTrade(proto.getFundsNeededForTrade());
         processModel.setBuyerPayoutAmountFromMediation(proto.getBuyerPayoutAmountFromMediation());
         processModel.setSellerPayoutAmountFromMediation(proto.getSellerPayoutAmountFromMediation());
-        processModel.setDeleteBackupsHeight(proto.getDeleteBackupsHeight());
+        processModel.setTradeProtocolErrorHeight(proto.getTradeProtocolErrorHeight());
 
         // nullable
         processModel.setPayoutTxSignature(ProtoUtil.byteArrayOrNullFromProto(proto.getPayoutTxSignature()));

core/src/main/java/haveno/core/trade/protocol/TradeProtocol.java

@@ -427,15 +427,7 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D
         System.out.println(getClass().getSimpleName() + ".handleDepositResponse()");
         ThreadUtils.execute(() -> {
             synchronized (trade) {
-
-                // check trade
-                if (trade.hasFailed()) {
-                    log.warn("{} {} ignoring {} from {} because trade failed with previous error: {}", trade.getClass().getSimpleName(), trade.getId(), response.getClass().getSimpleName(), sender, trade.getErrorMessage());
-                    return;
-                }
                 Validator.checkTradeId(processModel.getOfferId(), response);
-
-                // process message
                 latchTrade();
                 processModel.setTradeMessage(response);
                 expect(anyState(Trade.State.SENT_PUBLISH_DEPOSIT_TX_REQUEST, Trade.State.SAW_ARRIVED_PUBLISH_DEPOSIT_TX_REQUEST, Trade.State.ARBITRATOR_PUBLISHED_DEPOSIT_TXS, Trade.State.DEPOSIT_TXS_SEEN_IN_NETWORK)

core/src/main/java/haveno/core/trade/protocol/tasks/ArbitratorProcessDepositRequest.java

@@ -28,6 +28,7 @@ import haveno.core.trade.Trade;
 import haveno.core.trade.messages.DepositRequest;
 import haveno.core.trade.messages.DepositResponse;
 import haveno.core.trade.protocol.TradePeer;
+import haveno.core.trade.protocol.TradeProtocol;
 import haveno.network.p2p.NodeAddress;
 import haveno.network.p2p.SendDirectMessageListener;
 import lombok.extern.slf4j.Slf4j;
@@ -101,6 +102,10 @@ public class ArbitratorProcessDepositRequest extends TradeTask {
                 throw new RuntimeException("Error processing deposit tx from " + (isFromTaker ? "taker " : "maker ") + trader.getNodeAddress() + ", offerId=" + offer.getId() + ": " + e.getMessage());
             }
 
+            // extend timeout
+            if (isTimedOut()) throw new RuntimeException("Trade protocol has timed out while verifying deposit tx for {} {}" + trade.getClass().getSimpleName() + " " + trade.getShortId());
+            trade.getProtocol().startTimeout(TradeProtocol.TRADE_STEP_TIMEOUT_SECONDS);
+
             // set deposit info
             trader.setSecurityDeposit(securityDeposit.subtract(verifiedTx.getFee())); // subtract mining fee from security deposit
             trader.setDepositTxFee(verifiedTx.getFee());
@@ -109,7 +114,6 @@ public class ArbitratorProcessDepositRequest extends TradeTask {
             if (request.getPaymentAccountKey() != null) trader.setPaymentAccountKey(request.getPaymentAccountKey());
 
             // relay deposit txs when both available
-            // TODO (woodser): add small delay so tx has head start against double spend attempts?
             if (processModel.getMaker().getDepositTxHex() != null && processModel.getTaker().getDepositTxHex() != null) {
 
                 // update trade state
@@ -147,8 +151,8 @@ public class ArbitratorProcessDepositRequest extends TradeTask {
                 if (processModel.getTaker().getDepositTxHex() == null) log.info("Arbitrator waiting for deposit request from taker for trade " + trade.getId());
             }
 
-            complete();
             processModel.getTradeManager().requestPersistence();
+            complete();
         } catch (Throwable t) {
 
             // handle error before deposits relayed
@@ -192,4 +196,8 @@ public class ArbitratorProcessDepositRequest extends TradeTask {
             }
         });
     }
+
+    private boolean isTimedOut() {
+        return !processModel.getTradeManager().hasOpenTrade(trade);
+    }
 }

core/src/main/java/haveno/core/trade/protocol/tasks/MaybeSendSignContractRequest.java

@@ -124,7 +124,6 @@ public class MaybeSendSignContractRequest extends TradeTask {
 
                     throw e;
                 }
-                
 
                 // reset protocol timeout
                 trade.getProtocol().startTimeout(TradeProtocol.TRADE_STEP_TIMEOUT_SECONDS);

core/src/main/java/haveno/core/trade/protocol/tasks/ProcessDepositResponse.java

@@ -23,6 +23,7 @@ import java.math.BigInteger;
 import haveno.common.taskrunner.TaskRunner;
 import haveno.core.trade.Trade;
 import haveno.core.trade.messages.DepositResponse;
+import haveno.core.trade.protocol.TradeProtocol;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
@@ -42,9 +43,13 @@ public class ProcessDepositResponse extends TradeTask {
           DepositResponse message = (DepositResponse) processModel.getTradeMessage();
           if (message.getErrorMessage() != null) {
             trade.setStateIfValidTransitionTo(Trade.State.PUBLISH_DEPOSIT_TX_REQUEST_FAILED);
+            processModel.getTradeManager().unregisterTrade(trade);
             throw new RuntimeException(message.getErrorMessage());
           }
 
+          // reset protocol timeout
+          trade.getProtocol().startTimeout(TradeProtocol.TRADE_STEP_TIMEOUT_SECONDS);
+
           // record security deposits
           trade.getBuyer().setSecurityDeposit(BigInteger.valueOf(message.getBuyerSecurityDeposit()));
           trade.getSeller().setSecurityDeposit(BigInteger.valueOf(message.getSellerSecurityDeposit()));

core/src/main/java/haveno/core/xmr/wallet/XmrWalletService.java

@@ -549,11 +549,20 @@ public class XmrWalletService {
     /**
      * Freeze the given outputs with a lock on the wallet.
      *
-     * @param keyImages the key images to freeze
+     * @param keyImages the key images to freeze (ignored if null or empty)
      */
     public void freezeOutputs(Collection<String> keyImages) {
+        if (keyImages == null || keyImages.isEmpty()) return;
         synchronized (WALLET_LOCK) {
-            for (String keyImage : keyImages) wallet.freezeOutput(keyImage);
+
+            // collect outputs to freeze
+            List<String> unfrozenKeyImages = getOutputs(new MoneroOutputQuery().setIsFrozen(false).setIsSpent(false)).stream()
+                    .map(output -> output.getKeyImage().getHex())
+                    .collect(Collectors.toList());
+            unfrozenKeyImages.retainAll(keyImages);
+
+            // freeze outputs
+            for (String keyImage : unfrozenKeyImages) wallet.freezeOutput(keyImage);
             cacheWalletInfo();
             requestSaveMainWallet();
         }
@@ -567,7 +576,15 @@ public class XmrWalletService {
     public void thawOutputs(Collection<String> keyImages) {
         if (keyImages == null || keyImages.isEmpty()) return;
         synchronized (WALLET_LOCK) {
-            for (String keyImage : keyImages) wallet.thawOutput(keyImage);
+
+            // collect outputs to thaw
+            List<String> frozenKeyImages = getOutputs(new MoneroOutputQuery().setIsFrozen(true).setIsSpent(false)).stream()
+                    .map(output -> output.getKeyImage().getHex())
+                    .collect(Collectors.toList());
+            frozenKeyImages.retainAll(keyImages);
+
+            // thaw outputs
+            for (String keyImage : frozenKeyImages) wallet.thawOutput(keyImage);
             cacheWalletInfo();
             requestSaveMainWallet();
         }

desktop/src/main/java/haveno/desktop/main/portfolio/failedtrades/FailedTradesView.java

@@ -556,7 +556,7 @@ public class FailedTradesView extends ActivatableViewAndModel<VBox, FailedTrades
                             @Override
                             public void updateItem(FailedTradesListItem newItem, boolean empty) {
                                 super.updateItem(newItem, empty);
-                                if (!empty && newItem != null) {
+                                if (!empty && newItem != null && newItem.getTrade().isDepositsPublished()) {
                                     Label icon = FormBuilder.getIcon(AwesomeIcon.UNDO);
                                     JFXButton iconButton = new JFXButton("", icon);
                                     iconButton.setStyle("-fx-cursor: hand;");

proto/src/main/proto/pb.proto

@@ -1558,7 +1558,7 @@ message ProcessModel {
     bytes mediated_payout_tx_signature = 15; // placeholder if mediation used in future
     int64 buyer_payout_amount_from_mediation = 16;
     int64 seller_payout_amount_from_mediation = 17;
-    int64 delete_backups_height = 18;
+    int64 trade_protocol_error_height = 18;
     string trade_fee_address = 19;
 }