Improve handling of failed trades and offers (#3566)

* Improve handling of failed trades and offers - Check if deposit tx is null - Check if trade fee tx is rejected - Listen to reject tx errors - Cleanup addressEntryList - Prevent opening disputes with if deposit tx is null - Add null checks - Improve logs - Cleanups * Add move to failed trade button to popup In case the deposit tx is null we show a popup telling the user to move the trade to failed trades after a restart if the problem persist. * Change log level
bisq-network · Nov 6, 2019 · fee7850 · fee7850
1 parent 70abd27
commit fee7850
Show file tree

Hide file tree

Showing 19 changed files with 382 additions and 96 deletions.
diff --git a/core/src/main/java/bisq/core/app/BisqHeadlessApp.java b/core/src/main/java/bisq/core/app/BisqHeadlessApp.java
@@ -93,7 +93,8 @@ protected void setupHandlers() {
         bisqSetup.setDisplaySecurityRecommendationHandler(key -> log.info("onDisplaySecurityRecommendationHandler"));
         bisqSetup.setDisplayLocalhostHandler(key -> log.info("onDisplayLocalhostHandler"));
         bisqSetup.setWrongOSArchitectureHandler(msg -> log.error("onWrongOSArchitectureHandler. msg={}", msg));
-        bisqSetup.setVoteResultExceptionHandler(voteResultException -> log.warn("voteResultException={}", voteResultException));
+        bisqSetup.setVoteResultExceptionHandler(voteResultException -> log.warn("voteResultException={}", voteResultException.toString()));
+        bisqSetup.setRejectedTxErrorMessageHandler(errorMessage -> log.warn("setRejectedTxErrorMessageHandler. errorMessage={}", errorMessage));
 
         //TODO move to bisqSetup
         corruptedDatabaseFilesHandler.getCorruptedDatabaseFiles().ifPresent(files -> log.warn("getCorruptedDatabaseFiles. files={}", files));

diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java
@@ -55,6 +55,7 @@
 import bisq.core.support.dispute.refund.refundagent.RefundAgentManager;
 import bisq.core.support.traderchat.TraderChatManager;
 import bisq.core.trade.TradeManager;
+import bisq.core.trade.TradeTxException;
 import bisq.core.trade.statistics.AssetTradeActivityCheck;
 import bisq.core.trade.statistics.TradeStatisticsManager;
 import bisq.core.user.Preferences;
@@ -107,6 +108,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
@@ -192,7 +194,8 @@ default void onRequestWalletPassword() {
             spvFileCorruptedHandler, lockedUpFundsHandler, daoErrorMessageHandler, daoWarnMessageHandler,
             filterWarningHandler, displaySecurityRecommendationHandler, displayLocalhostHandler,
             wrongOSArchitectureHandler, displaySignedByArbitratorHandler,
-            displaySignedByPeerHandler, displayPeerLimitLiftedHandler, displayPeerSignerHandler;
+            displaySignedByPeerHandler, displayPeerLimitLiftedHandler, displayPeerSignerHandler,
+            rejectedTxErrorMessageHandler;
     @Setter
     @Nullable
     private Consumer<Boolean> displayTorNetworkSettingsHandler;
@@ -601,28 +604,56 @@ private void initWallet() {
                 showFirstPopupIfResyncSPVRequestedHandler,
                 walletPasswordHandler,
                 () -> {
-                    if (allBasicServicesInitialized)
+                    if (allBasicServicesInitialized) {
                         checkForLockedUpFunds();
+                        checkForInvalidMakerFeeTxs();
+                    }
                 },
                 () -> walletInitialized.set(true));
     }
 
 
     private void checkForLockedUpFunds() {
-        btcWalletService.getAddressEntriesForTrade().stream()
-                .filter(e -> tradeManager.getSetOfAllTradeIds().contains(e.getOfferId()) &&
-                        e.getContext() == AddressEntry.Context.MULTI_SIG)
-                .forEach(e -> {
-                    Coin balance = e.getCoinLockedInMultiSig();
-                    if (balance.isPositive()) {
-                        String message = Res.get("popup.warning.lockedUpFunds",
-                                formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
-                        log.warn(message);
-                        if (lockedUpFundsHandler != null) {
-                            lockedUpFundsHandler.accept(message);
+        // We check if there are locked up funds in failed or closed trades
+        try {
+            Set<String> setOfAllTradeIds = tradeManager.getSetOfFailedOrClosedTradeIdsFromLockedInFunds();
+            btcWalletService.getAddressEntriesForTrade().stream()
+                    .filter(e -> setOfAllTradeIds.contains(e.getOfferId()) &&
+                            e.getContext() == AddressEntry.Context.MULTI_SIG)
+                    .forEach(e -> {
+                        Coin balance = e.getCoinLockedInMultiSig();
+                        if (balance.isPositive()) {
+                            String message = Res.get("popup.warning.lockedUpFunds",
+                                    formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
+                            log.warn(message);
+                            if (lockedUpFundsHandler != null) {
+                                lockedUpFundsHandler.accept(message);
+                            }
                         }
-                    }
-                });
+                    });
+        } catch (TradeTxException e) {
+            log.warn(e.getMessage());
+            if (lockedUpFundsHandler != null) {
+                lockedUpFundsHandler.accept(e.getMessage());
+            }
+        }
+    }
+
+    private void checkForInvalidMakerFeeTxs() {
+        // We check if we have open offers with no confidence object at the maker fee tx. That can happen if the
+        // miner fee was too low and the transaction got removed from mempool and got out from our wallet after a
+        // resync.
+        openOfferManager.getObservableList().forEach(e -> {
+            String offerFeePaymentTxId = e.getOffer().getOfferFeePaymentTxId();
+            if (btcWalletService.getConfidenceForTxId(offerFeePaymentTxId) == null) {
+                String message = Res.get("popup.warning.openOfferWithInvalidMakerFeeTx",
+                        e.getOffer().getShortId(), offerFeePaymentTxId);
+                log.warn(message);
+                if (lockedUpFundsHandler != null) {
+                    lockedUpFundsHandler.accept(message);
+                }
+            }
+        });
     }
 
     private void checkForCorrectOSArchitecture() {
@@ -651,13 +682,60 @@ private void initDomainServices() {
 
         tradeManager.onAllServicesInitialized();
 
-        if (walletsSetup.downloadPercentageProperty().get() == 1)
+        if (walletsSetup.downloadPercentageProperty().get() == 1) {
             checkForLockedUpFunds();
+            checkForInvalidMakerFeeTxs();
+        }
 
         openOfferManager.onAllServicesInitialized();
 
         balances.onAllServicesInitialized();
 
+        walletAppSetup.getRejectedTxException().addListener((observable, oldValue, newValue) -> {
+            // We delay as we might get the rejected tx error before we have completed the create offer protocol
+            UserThread.runAfter(() -> {
+                if (rejectedTxErrorMessageHandler != null && newValue != null && newValue.getTxId() != null) {
+                    String txId = newValue.getTxId();
+                    openOfferManager.getObservableList().stream()
+                            .filter(openOffer -> txId.equals(openOffer.getOffer().getOfferFeePaymentTxId()))
+                            .forEach(openOffer -> {
+                                // We delay to avoid concurrent modification exceptions
+                                UserThread.runAfter(() -> {
+                                    openOffer.getOffer().setErrorMessage(newValue.getMessage());
+                                    rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.openOffer.makerFeeTxRejected", openOffer.getId(), txId));
+                                    openOfferManager.removeOpenOffer(openOffer, () -> {
+                                        log.warn("We removed an open offer because the maker fee was rejected by the Bitcoin " +
+                                                "network. OfferId={}, txId={}", openOffer.getShortId(), txId);
+                                    }, log::warn);
+                                }, 1);
+                            });
+
+                    tradeManager.getTradableList().stream()
+                            .filter(trade -> trade.getOffer() != null)
+                            .forEach(trade -> {
+                                String details = null;
+                                if (txId.equals(trade.getDepositTxId())) {
+                                    details = Res.get("popup.warning.trade.txRejected.deposit");
+                                }
+                                if (txId.equals(trade.getOffer().getOfferFeePaymentTxId()) || txId.equals(trade.getTakerFeeTxId())) {
+                                    details = Res.get("popup.warning.trade.txRejected.tradeFee");
+                                }
+
+                                if (details != null) {
+                                    // We delay to avoid concurrent modification exceptions
+                                    String finalDetails = details;
+                                    UserThread.runAfter(() -> {
+                                        trade.setErrorMessage(newValue.getMessage());
+                                        rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.trade.txRejected",
+                                                finalDetails, trade.getShortId(), txId));
+                                        tradeManager.addTradeToFailedTrades(trade);
+                                    }, 1);
+                                }
+                            });
+                }
+            }, 3);
+        });
+
         arbitratorManager.onAllServicesInitialized();
         mediatorManager.onAllServicesInitialized();
         refundAgentManager.onAllServicesInitialized();

diff --git a/core/src/main/java/bisq/core/app/WalletAppSetup.java b/core/src/main/java/bisq/core/app/WalletAppSetup.java
@@ -17,6 +17,7 @@
 
 package bisq.core.app;
 
+import bisq.core.btc.exceptions.RejectedTxException;
 import bisq.core.btc.setup.WalletsSetup;
 import bisq.core.btc.wallet.WalletsManager;
 import bisq.core.locale.Res;
@@ -71,6 +72,8 @@ public class WalletAppSetup {
     @Getter
     private final StringProperty btcInfo = new SimpleStringProperty(Res.get("mainView.footer.btcInfo.initializing"));
     @Getter
+    private final ObjectProperty<RejectedTxException> rejectedTxException = new SimpleObjectProperty<>();
+    @Getter
     private int numBtcPeers = 0;
     @Getter
     private final BooleanProperty useTorForBTC = new SimpleBooleanProperty();
@@ -132,7 +135,7 @@ void init(@Nullable Consumer<String> chainFileLockedExceptionHandler,
                                 getNumBtcPeers(),
                                 Res.get("mainView.footer.btcInfo.connectionFailed"),
                                 getBtcNetworkAsString());
-                        log.error(exception.getMessage());
+                        log.error(exception.toString());
                         if (exception instanceof TimeoutException) {
                             getWalletServiceErrorMsg().set(Res.get("mainView.walletServiceErrorMsg.timeout"));
                         } else if (exception.getCause() instanceof BlockStoreException) {
@@ -141,6 +144,9 @@ void init(@Nullable Consumer<String> chainFileLockedExceptionHandler,
                             } else if (spvFileCorruptedHandler != null) {
                                 spvFileCorruptedHandler.accept(Res.get("error.spvFileCorrupted", exception.getMessage()));
                             }
+                        } else if (exception instanceof RejectedTxException) {
+                            rejectedTxException.set((RejectedTxException) exception);
+                            getWalletServiceErrorMsg().set(Res.get("mainView.walletServiceErrorMsg.rejectedTxException", exception.getMessage()));
                         } else {
                             getWalletServiceErrorMsg().set(Res.get("mainView.walletServiceErrorMsg.connectionError", exception.toString()));
                         }

diff --git a/core/src/main/java/bisq/core/btc/Balances.java b/core/src/main/java/bisq/core/btc/Balances.java
@@ -119,8 +119,8 @@ private void updateReservedBalance() {
     }
 
     private void updateLockedBalance() {
-        Stream<Trade> lockedTrades = Stream.concat(closedTradableManager.getLockedTradesStream(), failedTradesManager.getLockedTradesStream());
-        lockedTrades = Stream.concat(lockedTrades, tradeManager.getLockedTradesStream());
+        Stream<Trade> lockedTrades = Stream.concat(closedTradableManager.getTradesStreamWithFundsLockedIn(), failedTradesManager.getTradesStreamWithFundsLockedIn());
+        lockedTrades = Stream.concat(lockedTrades, tradeManager.getTradesStreamWithFundsLockedIn());
         long sum = lockedTrades.map(trade -> btcWalletService.getAddressEntry(trade.getId(), AddressEntry.Context.MULTI_SIG)
                 .orElse(null))
                 .filter(Objects::nonNull)

diff --git a/core/src/main/java/bisq/core/btc/exceptions/RejectedTxException.java b/core/src/main/java/bisq/core/btc/exceptions/RejectedTxException.java
@@ -0,0 +1,46 @@
+/*
+ * This file is part of Bisq.
+ *
+ * Bisq is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.core.btc.exceptions;
+
+import org.bitcoinj.core.RejectMessage;
+
+import lombok.Getter;
+
+import javax.annotation.Nullable;
+
+public class RejectedTxException extends RuntimeException {
+    @Getter
+    private final RejectMessage rejectMessage;
+    @Getter
+    @Nullable
+    private final String txId;
+
+    public RejectedTxException(String message, RejectMessage rejectMessage) {
+        super(message);
+        this.rejectMessage = rejectMessage;
+        txId = rejectMessage.getRejectedObjectHash() != null ? rejectMessage.getRejectedObjectHash().toString() : null;
+    }
+
+    @Override
+    public String toString() {
+        return "RejectedTxException{" +
+                "\n     rejectMessage=" + rejectMessage +
+                ",\n     txId='" + txId + '\'' +
+                "\n} " + super.toString();
+    }
+}
diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java
@@ -19,6 +19,7 @@
 
 import bisq.core.app.BisqEnvironment;
 import bisq.core.btc.BtcOptionKeys;
+import bisq.core.btc.exceptions.RejectedTxException;
 import bisq.core.btc.model.AddressEntry;
 import bisq.core.btc.model.AddressEntryList;
 import bisq.core.btc.nodes.BtcNetworkConfig;
@@ -44,6 +45,7 @@
 import org.bitcoinj.core.Peer;
 import org.bitcoinj.core.PeerAddress;
 import org.bitcoinj.core.PeerGroup;
+import org.bitcoinj.core.RejectMessage;
 import org.bitcoinj.core.listeners.DownloadProgressTracker;
 import org.bitcoinj.params.RegTestParams;
 import org.bitcoinj.utils.Threading;
@@ -170,7 +172,9 @@ public WalletsSetup(RegTestHost regTestHost,
     // Lifecycle
     ///////////////////////////////////////////////////////////////////////////////////////////
 
-    public void initialize(@Nullable DeterministicSeed seed, ResultHandler resultHandler, ExceptionHandler exceptionHandler) {
+    public void initialize(@Nullable DeterministicSeed seed,
+                           ResultHandler resultHandler,
+                           ExceptionHandler exceptionHandler) {
         // Tell bitcoinj to execute event handlers on the JavaFX UI thread. This keeps things simple and means
         // we cannot forget to switch threads when adding event handlers. Unfortunately, the DownloadListener
         // we give to the app kit is currently an exception and runs on a library thread. It'll get fixed in
@@ -221,6 +225,20 @@ protected void onSetupCompleted() {
                 peerGroup.addBlocksDownloadedEventListener((peer, block, filteredBlock, blocksLeft) -> {
                     blocksDownloadedFromPeer.set(peer);
                 });
+
+                // Need to be Threading.SAME_THREAD executor otherwise BitcoinJ will skip that listener
+                peerGroup.addPreMessageReceivedEventListener(Threading.SAME_THREAD, (peer, message) -> {
+                    if (message instanceof RejectMessage) {
+                        UserThread.execute(() -> {
+                            RejectMessage rejectMessage = (RejectMessage) message;
+                            String msg = rejectMessage.toString();
+                            log.warn(msg);
+                            exceptionHandler.handleException(new RejectedTxException(msg, rejectMessage));
+                        });
+                    }
+                    return message;
+                });
+
                 chain.addNewBestBlockListener(block -> {
                     connectedPeers.set(peerGroup.getConnectedPeers());
                     chainHeight.set(block.getHeight());
@@ -372,7 +390,9 @@ public void clearBackups() {
     // Restore
     ///////////////////////////////////////////////////////////////////////////////////////////
 
-    public void restoreSeedWords(@Nullable DeterministicSeed seed, ResultHandler resultHandler, ExceptionHandler exceptionHandler) {
+    public void restoreSeedWords(@Nullable DeterministicSeed seed,
+                                 ResultHandler resultHandler,
+                                 ExceptionHandler exceptionHandler) {
         checkNotNull(seed, "Seed must be not be null.");
 
         backupWallets();

diff --git a/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java
@@ -168,7 +168,7 @@ public void onTransactionConfidenceChanged(Wallet wallet, Transaction tx) {
                         // We are only interested in updates from unconfirmed txs and confirmed txs at the
                         // time when it gets into a block. Otherwise we would get called
                         // updateBsqWalletTransactions for each tx as the block depth changes for all.
-                        if (tx.getConfidence().getDepthInBlocks() <= 1 &&
+                        if (tx != null && tx.getConfidence() != null && tx.getConfidence().getDepthInBlocks() <= 1 &&
                                 daoStateService.isParseBlockChainComplete()) {
                             updateBsqWalletTransactions();
                         }

diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java
@@ -672,7 +672,13 @@ public void resetAddressEntriesForOpenOffer(String offerId) {
 
     public void resetAddressEntriesForPendingTrade(String offerId) {
         swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.MULTI_SIG);
-        // Don't swap TRADE_PAYOUT as it might be still open in the last trade step to be used for external transfer
+        // We swap also TRADE_PAYOUT to be sure all is cleaned  up. There might be cases where a user cannot send the funds
+        // to an external wallet directly in the last step of the trade, but the funds are in the Bisq wallet anyway and
+        // the dealing with the external wallet is pure UI thing. The user can move the funds to the wallet and then
+        // send out the funds to the external wallet. As this cleanup is a rare situation and most users do not use
+        // the feature to send out the funds we prefer that strategy (if we keep the address entry it might cause
+        // complications in some edge cases after a SPV resync).
+        swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.TRADE_PAYOUT);
     }
 
     public void swapAnyTradeEntryContextToAvailableEntry(String offerId) {

diff --git a/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java b/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java
@@ -115,9 +115,8 @@ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingExceptio
                         genesisTotalSupply)
                         .ifPresent(txList::add));
 
-        if (System.currentTimeMillis() - startTs > 0)
-            log.info("Parsing {} transactions at block height {} took {} ms", rawBlock.getRawTxs().size(),
-                    blockHeight, System.currentTimeMillis() - startTs);
+        log.info("Parsing {} transactions at block height {} took {} ms", rawBlock.getRawTxs().size(),
+                blockHeight, System.currentTimeMillis() - startTs);
 
         daoStateService.onParseBlockComplete(block);
         return block;