From db26a1fe2d71374a9da8f32169724ae06dc22e0d Mon Sep 17 00:00:00 2001 From: Steven Barclay Date: Tue, 26 Jan 2021 20:10:47 +0000 Subject: [PATCH] Optimise TransactionAwareTrade.isRelatedToTransaction Attempt to remove a bottleneck during the transactions view load, as revealed by JProfiler, by optimising the code to determine if any given transaction and trade are related. Since both these sets will tend to grow linearly with time, causing quadratic slowdown of TransactionsView, try to alleviate (without completely fixing) the problem. To do this, add a cached set of disputed trade IDs to DisputeListService so that TransactionAwareTradable.is(Dispute|RefundPayout)Tx can be made O(1) in the common case that the given trade is not involved in any dispute. Also avoid calling Sha256Hash::toString by passing tx IDs as Sha256Hash objects directly to is(Deposit|Payout)Tx, and short circuit an expensive call BtcWalletService.getTransaction in isDelayedPayoutTx, in the common case, by pre-checking the transaction locktime. This also fixes a bug in isRefundPayoutTx whereby it incorrectly returns false if there are >1 disputes in the list returned by RefundManager but the given trade is not involved in the last one. --- .../support/dispute/DisputeListService.java | 6 ++ .../core/support/dispute/DisputeManager.java | 4 ++ .../transactions/TransactionAwareTrade.java | 70 +++++++++---------- .../TransactionAwareTradeTest.java | 9 +-- 4 files changed, 47 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/bisq/core/support/dispute/DisputeListService.java b/core/src/main/java/bisq/core/support/dispute/DisputeListService.java index d54b5e8b85..f722f2dbda 100644 --- a/core/src/main/java/bisq/core/support/dispute/DisputeListService.java +++ b/core/src/main/java/bisq/core/support/dispute/DisputeListService.java @@ -34,8 +34,10 @@ import javafx.beans.property.SimpleIntegerProperty; import javafx.collections.ObservableList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -53,6 +55,8 @@ public abstract class DisputeListService> impleme private final Map disputeIsClosedSubscriptionsMap = new HashMap<>(); @Getter private final IntegerProperty numOpenDisputes = new SimpleIntegerProperty(); + @Getter + private final Set disputedTradeIds = new HashSet<>(); /////////////////////////////////////////////////////////////////////////////////////////// @@ -154,6 +158,7 @@ public abstract class DisputeListService> impleme disputeIsClosedSubscriptionsMap.get(id).unsubscribe(); disputeIsClosedSubscriptionsMap.remove(id); } + disputedTradeIds.remove(dispute.getTradeId()); }); } addedList.forEach(dispute -> { @@ -168,6 +173,7 @@ public abstract class DisputeListService> impleme }); }); disputeIsClosedSubscriptionsMap.put(id, disputeStateSubscription); + disputedTradeIds.add(dispute.getTradeId()); }); } diff --git a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java index baa11de096..bf78a565c0 100644 --- a/core/src/main/java/bisq/core/support/dispute/DisputeManager.java +++ b/core/src/main/java/bisq/core/support/dispute/DisputeManager.java @@ -69,6 +69,7 @@ import java.security.KeyPair; import java.util.Date; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -231,6 +232,9 @@ public abstract class DisputeManager> extends Sup return disputeListService.getDisputeList(); } + public Set getDisputedTradeIds() { + return disputeListService.getDisputedTradeIds(); + } /////////////////////////////////////////////////////////////////////////////////////////// // API diff --git a/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTrade.java b/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTrade.java index c357d2630b..9d0ab09ef2 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTrade.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTrade.java @@ -36,13 +36,11 @@ import org.bitcoinj.core.TransactionOutput; import javafx.collections.ObservableList; import java.util.Optional; -import java.util.concurrent.atomic.AtomicBoolean; import lombok.extern.slf4j.Slf4j; import static com.google.common.base.Preconditions.checkNotNull; - @Slf4j class TransactionAwareTrade implements TransactionAwareTradable { private final Trade trade; @@ -65,32 +63,31 @@ class TransactionAwareTrade implements TransactionAwareTradable { @Override public boolean isRelatedToTransaction(Transaction transaction) { - String txId = transaction.getTxId().toString(); + Sha256Hash hash = transaction.getTxId(); + String txId = hash.toString(); boolean isTakerOfferFeeTx = txId.equals(trade.getTakerFeeTxId()); boolean isOfferFeeTx = isOfferFeeTx(txId); - boolean isDepositTx = isDepositTx(txId); - boolean isPayoutTx = isPayoutTx(txId); + boolean isDepositTx = isDepositTx(hash); + boolean isPayoutTx = isPayoutTx(hash); boolean isDisputedPayoutTx = isDisputedPayoutTx(txId); - boolean isDelayedPayoutTx = isDelayedPayoutTx(txId); + boolean isDelayedPayoutTx = transaction.getLockTime() != 0 && isDelayedPayoutTx(txId); boolean isRefundPayoutTx = isRefundPayoutTx(txId); return isTakerOfferFeeTx || isOfferFeeTx || isDepositTx || isPayoutTx || isDisputedPayoutTx || isDelayedPayoutTx || isRefundPayoutTx; } - private boolean isPayoutTx(String txId) { + private boolean isPayoutTx(Sha256Hash txId) { return Optional.ofNullable(trade.getPayoutTx()) .map(Transaction::getTxId) - .map(Sha256Hash::toString) .map(hash -> hash.equals(txId)) .orElse(false); } - private boolean isDepositTx(String txId) { + private boolean isDepositTx(Sha256Hash txId) { return Optional.ofNullable(trade.getDepositTx()) .map(Transaction::getTxId) - .map(Sha256Hash::toString) .map(hash -> hash.equals(txId)) .orElse(false); } @@ -104,9 +101,11 @@ class TransactionAwareTrade implements TransactionAwareTradable { private boolean isDisputedPayoutTx(String txId) { String delegateId = trade.getId(); - ObservableList disputes = arbitrationManager.getDisputesAsObservableList(); - return disputes.stream() + + boolean isAnyDisputeRelatedToThis = arbitrationManager.getDisputedTradeIds().contains(trade.getId()); + + return isAnyDisputeRelatedToThis && disputes.stream() .anyMatch(dispute -> { String disputePayoutTxId = dispute.getDisputePayoutTxId(); boolean isDisputePayoutTx = txId.equals(disputePayoutTxId); @@ -139,42 +138,37 @@ class TransactionAwareTrade implements TransactionAwareTradable { if (parentTransaction == null) { return false; } - return isDepositTx(parentTransaction.getTxId().toString()); + return isDepositTx(parentTransaction.getTxId()); }); } private boolean isRefundPayoutTx(String txId) { String tradeId = trade.getId(); ObservableList disputes = refundManager.getDisputesAsObservableList(); - AtomicBoolean isRefundTx = new AtomicBoolean(false); - AtomicBoolean isDisputeRelatedToThis = new AtomicBoolean(false); - disputes.forEach(dispute -> { - String disputeTradeId = dispute.getTradeId(); - isDisputeRelatedToThis.set(tradeId.equals(disputeTradeId)); - if (isDisputeRelatedToThis.get()) { - Transaction tx = btcWalletService.getTransaction(txId); - if (tx != null) { - tx.getOutputs().forEach(txo -> { - if (btcWalletService.isTransactionOutputMine(txo)) { - try { - Address receiverAddress = txo.getScriptPubKey().getToAddress(btcWalletService.getParams()); - Contract contract = checkNotNull(trade.getContract()); - String myPayoutAddressString = contract.isMyRoleBuyer(pubKeyRing) ? - contract.getBuyerPayoutAddressString() : - contract.getSellerPayoutAddressString(); - if (receiverAddress != null && myPayoutAddressString.equals(receiverAddress.toString())) { - isRefundTx.set(true); - } - } catch (Throwable ignore) { - } + boolean isAnyDisputeRelatedToThis = refundManager.getDisputedTradeIds().contains(tradeId); + + if (isAnyDisputeRelatedToThis) { + Transaction tx = btcWalletService.getTransaction(txId); + if (tx != null) { + for (TransactionOutput txo : tx.getOutputs()) { + if (btcWalletService.isTransactionOutputMine(txo)) { + try { + Address receiverAddress = txo.getScriptPubKey().getToAddress(btcWalletService.getParams()); + Contract contract = checkNotNull(trade.getContract()); + String myPayoutAddressString = contract.isMyRoleBuyer(pubKeyRing) ? + contract.getBuyerPayoutAddressString() : + contract.getSellerPayoutAddressString(); + if (receiverAddress != null && myPayoutAddressString.equals(receiverAddress.toString())) { + return true; + } + } catch (RuntimeException ignore) { } - }); + } } } - }); - - return isRefundTx.get() && isDisputeRelatedToThis.get(); + } + return false; } @Override diff --git a/desktop/src/test/java/bisq/desktop/main/funds/transactions/TransactionAwareTradeTest.java b/desktop/src/test/java/bisq/desktop/main/funds/transactions/TransactionAwareTradeTest.java index 24f7cdece5..4552c9bc1c 100644 --- a/desktop/src/test/java/bisq/desktop/main/funds/transactions/TransactionAwareTradeTest.java +++ b/desktop/src/test/java/bisq/desktop/main/funds/transactions/TransactionAwareTradeTest.java @@ -28,7 +28,7 @@ import org.bitcoinj.core.Transaction; import javafx.collections.FXCollections; -import java.util.Collections; +import java.util.Set; import org.junit.Before; import org.junit.Test; @@ -41,8 +41,6 @@ import static org.mockito.Mockito.when; public class TransactionAwareTradeTest { private static final Sha256Hash XID = Sha256Hash.wrap("0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"); - - private Transaction transaction; private ArbitrationManager arbitrationManager; private Trade delegate; @@ -95,7 +93,10 @@ public class TransactionAwareTradeTest { when(dispute.getTradeId()).thenReturn(tradeId); when(arbitrationManager.getDisputesAsObservableList()) - .thenReturn(FXCollections.observableArrayList(Collections.singleton(dispute))); + .thenReturn(FXCollections.observableArrayList(Set.of(dispute))); + + when(arbitrationManager.getDisputedTradeIds()) + .thenReturn(Set.of(tradeId)); when(delegate.getId()).thenReturn(tradeId);