From cd7f176e2b72e2531068009674ff0c8b49125b1d Mon Sep 17 00:00:00 2001 From: woodser Date: Sat, 14 Jan 2023 10:38:37 -0500 Subject: [PATCH] fix 'trade not found' bug caused by open offer being spent do not remove open offer with spent funds if reserved for trade fix concurrent modification exception --- .../java/bisq/core/api/CoreTradesService.java | 2 +- .../bisq/core/offer/OpenOfferManager.java | 19 ++-- .../core/trade/ClosedTradableManager.java | 4 + .../main/java/bisq/core/trade/Tradable.java | 1 - .../java/bisq/core/trade/TradeManager.java | 104 +++++++++--------- .../bisq/daemon/grpc/GrpcTradesService.java | 1 + .../closedtrades/ClosedTradesViewModel.java | 1 - 7 files changed, 72 insertions(+), 60 deletions(-) diff --git a/core/src/main/java/bisq/core/api/CoreTradesService.java b/core/src/main/java/bisq/core/api/CoreTradesService.java index ec5f7086ca..3b96f59b9c 100644 --- a/core/src/main/java/bisq/core/api/CoreTradesService.java +++ b/core/src/main/java/bisq/core/api/CoreTradesService.java @@ -225,7 +225,7 @@ class CoreTradesService { } private Optional getClosedTrade(String tradeId) { - Optional tradable = closedTradableManager.getTradableById(tradeId); + Optional tradable = closedTradableManager.getTradeById(tradeId); return tradable.filter((t) -> t instanceof Trade).map(value -> (Trade) value); } diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index cee11ebd57..1d9d2bec7e 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -196,7 +196,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe @Override public void onAdded(Offer offer) { Optional openOfferOptional = getOpenOfferById(offer.getId()); - if (openOfferOptional.isPresent() && offer.isReservedFundsSpent()) { + if (openOfferOptional.isPresent() && openOfferOptional.get().getState() != OpenOffer.State.RESERVED && offer.isReservedFundsSpent()) { removeOpenOffer(openOfferOptional.get(), null); } } @@ -247,8 +247,8 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe // .ifPresent(errorMsg -> invalidOffers.add(new Tuple2<>(openOffer, errorMsg)))); // process unposted offers - processUnpostedOffers((transaction) -> {}, (errMessage) -> { - log.warn("Error processing unposted offers on new unlocked balance: " + errMessage); + processUnpostedOffers((transaction) -> {}, (errorMessage) -> { + log.warn("Error processing unposted offers on new unlocked balance: " + errorMessage); }); // register to process unposted offers when unlocked balance increases @@ -257,8 +257,8 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe @Override public void onBalancesChanged(BigInteger newBalance, BigInteger newUnlockedBalance) { if (lastUnlockedBalance == null || lastUnlockedBalance.compareTo(newUnlockedBalance) < 0) { - processUnpostedOffers((transaction) -> {}, (errMessage) -> { - log.warn("Error processing unposted offers on new unlocked balance: " + errMessage); + processUnpostedOffers((transaction) -> {}, (errorMessage) -> { + log.warn("Error processing unposted offers on new unlocked balance: " + errorMessage); }); } lastUnlockedBalance = newUnlockedBalance; @@ -327,6 +327,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe List openOffersList = new ArrayList<>(openOffers); openOffersList.forEach(openOffer -> removeOpenOffer(openOffer, () -> { }, errorMessage -> { + log.warn("Error removing open offer: " + errorMessage); })); if (completeHandler != null) UserThread.runAfter(completeHandler, size * 200 + 500, TimeUnit.MILLISECONDS); @@ -448,10 +449,11 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe addOpenOffer(openOffer); requestPersistence(); resultHandler.handleResult(transaction); - }, (errMessage) -> { + }, (errorMessage) -> { + log.warn("Error processing unposted offer {}: {}", openOffer.getId(), errorMessage); onRemoved(openOffer); - offer.setErrorMessage(errMessage); - errorMessageHandler.handleErrorMessage(errMessage); + offer.setErrorMessage(errorMessage); + errorMessageHandler.handleErrorMessage(errorMessage); }); } @@ -691,6 +693,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe processUnpostedOffer(openOffers, scheduledOffer, (transaction) -> { latch.countDown(); }, errorMessage -> { + log.warn("Error processing unposted offer {}: {}", scheduledOffer.getId(), errorMessage); onRemoved(scheduledOffer); errorMessages.add(errorMessage); latch.countDown(); diff --git a/core/src/main/java/bisq/core/trade/ClosedTradableManager.java b/core/src/main/java/bisq/core/trade/ClosedTradableManager.java index b2b32f9458..0470fcf71d 100644 --- a/core/src/main/java/bisq/core/trade/ClosedTradableManager.java +++ b/core/src/main/java/bisq/core/trade/ClosedTradableManager.java @@ -151,6 +151,10 @@ public class ClosedTradableManager implements PersistedDataHost { return closedTradables.stream().filter(e -> e.getId().equals(id)).findFirst(); } + public Optional getTradeById(String id) { + return closedTradables.stream().filter(e -> e instanceof Trade && e.getId().equals(id)).findFirst(); + } + public void maybeClearSensitiveData() { log.info("checking closed trades eligibility for having sensitive data cleared"); closedTradables.stream() diff --git a/core/src/main/java/bisq/core/trade/Tradable.java b/core/src/main/java/bisq/core/trade/Tradable.java index 32186be469..01001805bf 100644 --- a/core/src/main/java/bisq/core/trade/Tradable.java +++ b/core/src/main/java/bisq/core/trade/Tradable.java @@ -20,7 +20,6 @@ package bisq.core.trade; import bisq.core.monetary.Price; import bisq.core.monetary.Volume; import bisq.core.offer.Offer; -import bisq.core.trade.protocol.TradingPeer; import bisq.network.p2p.NodeAddress; import bisq.common.proto.persistable.PersistablePayload; diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index 892e186f22..4e11b2ccc7 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -895,58 +895,62 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi } public Stream getTradesStreamWithFundsLockedIn() { - return getObservableList().stream().filter(Trade::isFundsLockedIn); + synchronized (tradableList) { + return getObservableList().stream().filter(Trade::isFundsLockedIn); + } } public Set getSetOfFailedOrClosedTradeIdsFromLockedInFunds() throws TradeTxException { AtomicReference tradeTxException = new AtomicReference<>(); - Set tradesIdSet = getTradesStreamWithFundsLockedIn() - .filter(Trade::hasFailed) - .map(Trade::getId) - .collect(Collectors.toSet()); - tradesIdSet.addAll(failedTradesManager.getTradesStreamWithFundsLockedIn() - .filter(trade -> trade.getMakerDepositTx() != null || trade.getTakerDepositTx() != null) - .map(trade -> { - log.warn("We found a failed trade with locked up funds. " + - "That should never happen. trade ID=" + trade.getId()); + synchronized (tradableList) { + Set tradesIdSet = getTradesStreamWithFundsLockedIn() + .filter(Trade::hasFailed) + .map(Trade::getId) + .collect(Collectors.toSet()); + tradesIdSet.addAll(failedTradesManager.getTradesStreamWithFundsLockedIn() + .filter(trade -> trade.getMakerDepositTx() != null || trade.getTakerDepositTx() != null) + .map(trade -> { + log.warn("We found a failed trade with locked up funds. " + + "That should never happen. trade ID=" + trade.getId()); + return trade.getId(); + }) + .collect(Collectors.toSet())); + tradesIdSet.addAll(closedTradableManager.getTradesStreamWithFundsLockedIn() + .map(trade -> { + MoneroTx makerDepositTx = trade.getMakerDepositTx(); + if (makerDepositTx != null) { + if (!makerDepositTx.isConfirmed()) { + tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithUnconfirmedDepositTx", trade.getShortId()))); // TODO (woodser): rename to closedTradeWithLockedDepositTx + } else { + log.warn("We found a closed trade with locked up funds. " + + "That should never happen. {} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); + } + } else { + log.warn("Closed trade with locked up funds missing maker deposit tx. {} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); + tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithNoDepositTx", trade.getShortId()))); + } + + MoneroTx takerDepositTx = trade.getTakerDepositTx(); + if (takerDepositTx != null) { + if (!takerDepositTx.isConfirmed()) { + tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithUnconfirmedDepositTx", trade.getShortId()))); + } else { + log.warn("We found a closed trade with locked up funds. " + + "That should never happen. trade ID={} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); + } + } else { + log.warn("Closed trade with locked up funds missing taker deposit tx. {} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); + tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithNoDepositTx", trade.getShortId()))); + } return trade.getId(); - }) - .collect(Collectors.toSet())); - tradesIdSet.addAll(closedTradableManager.getTradesStreamWithFundsLockedIn() - .map(trade -> { - MoneroTx makerDepositTx = trade.getMakerDepositTx(); - if (makerDepositTx != null) { - if (!makerDepositTx.isConfirmed()) { - tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithUnconfirmedDepositTx", trade.getShortId()))); // TODO (woodser): rename to closedTradeWithLockedDepositTx - } else { - log.warn("We found a closed trade with locked up funds. " + - "That should never happen. {} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); - } - } else { - log.warn("Closed trade with locked up funds missing maker deposit tx. {} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); - tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithNoDepositTx", trade.getShortId()))); - } + }) + .collect(Collectors.toSet())); - MoneroTx takerDepositTx = trade.getTakerDepositTx(); - if (takerDepositTx != null) { - if (!takerDepositTx.isConfirmed()) { - tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithUnconfirmedDepositTx", trade.getShortId()))); - } else { - log.warn("We found a closed trade with locked up funds. " + - "That should never happen. trade ID={} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); - } - } else { - log.warn("Closed trade with locked up funds missing taker deposit tx. {} ID={}, state={}, payoutState={}, disputeState={}", trade.getId(), trade.getClass().getSimpleName(), trade.getId(), trade.getState(), trade.getPayoutState(), trade.getDisputeState()); - tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithNoDepositTx", trade.getShortId()))); - } - return trade.getId(); - }) - .collect(Collectors.toSet())); + if (tradeTxException.get() != null) + throw tradeTxException.get(); - if (tradeTxException.get() != null) - throw tradeTxException.get(); - - return tradesIdSet; + return tradesIdSet; + } } // If trade still has funds locked up it might come back from failed trades @@ -1028,10 +1032,12 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi } public List getOpenTrades() { - return ImmutableList.copyOf(getObservableList().stream() - .filter(e -> e instanceof Trade) - .map(e -> e) - .collect(Collectors.toList())); + synchronized (tradableList) { + return ImmutableList.copyOf(getObservableList().stream() + .filter(e -> e instanceof Trade) + .map(e -> e) + .collect(Collectors.toList())); + } } public Optional getClosedTrade(String tradeId) { diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java index a2d7182d85..cabd2833b4 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java @@ -87,6 +87,7 @@ class GrpcTradesService extends TradesImplBase { } catch (IllegalArgumentException cause) { // Offer makers may call 'gettrade' many times before a trade exists. // Log a 'trade not found' warning instead of a full stack trace. + cause.printStackTrace(); exceptionHandler.handleExceptionAsWarning(log, "getTrade", cause, responseObserver); } catch (Throwable cause) { exceptionHandler.handleException(log, cause, responseObserver); diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesViewModel.java b/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesViewModel.java index a94be340a7..1696773492 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/closedtrades/ClosedTradesViewModel.java @@ -20,7 +20,6 @@ package bisq.desktop.main.portfolio.closedtrades; import bisq.desktop.common.model.ActivatableWithDataModel; import bisq.desktop.common.model.ViewModel; -import bisq.core.monetary.Volume; import bisq.core.trade.ClosedTradableFormatter; import org.bitcoinj.core.Coin;