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
This commit is contained in:
woodser 2023-01-14 10:38:37 -05:00
parent 266d129462
commit cd7f176e2b
7 changed files with 72 additions and 60 deletions

View File

@ -225,7 +225,7 @@ class CoreTradesService {
}
private Optional<Trade> getClosedTrade(String tradeId) {
Optional<Tradable> tradable = closedTradableManager.getTradableById(tradeId);
Optional<Tradable> tradable = closedTradableManager.getTradeById(tradeId);
return tradable.filter((t) -> t instanceof Trade).map(value -> (Trade) value);
}

View File

@ -196,7 +196,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe
@Override
public void onAdded(Offer offer) {
Optional<OpenOffer> 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<OpenOffer> 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();

View File

@ -151,6 +151,10 @@ public class ClosedTradableManager implements PersistedDataHost {
return closedTradables.stream().filter(e -> e.getId().equals(id)).findFirst();
}
public Optional<Tradable> 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()

View File

@ -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;

View File

@ -895,58 +895,62 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi
}
public Stream<Trade> getTradesStreamWithFundsLockedIn() {
return getObservableList().stream().filter(Trade::isFundsLockedIn);
synchronized (tradableList) {
return getObservableList().stream().filter(Trade::isFundsLockedIn);
}
}
public Set<String> getSetOfFailedOrClosedTradeIdsFromLockedInFunds() throws TradeTxException {
AtomicReference<TradeTxException> tradeTxException = new AtomicReference<>();
Set<String> 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<String> 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<Trade> 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<Trade> getClosedTrade(String tradeId) {

View File

@ -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);

View File

@ -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;