Merge pull request #5682 from ghubstan/cache-the-offerpayload-hash

Reduce # of hash calculations in UI OfferBook view

core/src/main/java/bisq/core/notifications/alerts/market/MarketAlerts.java

@@ -32,8 +32,6 @@
 import bisq.core.user.User;
 import bisq.core.util.FormattingUtils;
 
-import bisq.network.p2p.storage.P2PDataStorage;
-
 import bisq.common.crypto.KeyRing;
 import bisq.common.util.MathUtils;
 
@@ -74,12 +72,12 @@ private MarketAlerts(OfferBookService offerBookService, MobileNotificationServic
     public void onAllServicesInitialized() {
         offerBookService.addOfferBookChangedListener(new OfferBookService.OfferBookChangedListener() {
             @Override
-            public void onAdded(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+            public void onAdded(Offer offer) {
                 onOfferAdded(offer);
             }
 
             @Override
-            public void onRemoved(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+            public void onRemoved(Offer offer) {
             }
         });
         applyFilterOnAllOffers();

core/src/main/java/bisq/core/offer/OfferBookService.java

@@ -24,7 +24,6 @@
 import bisq.network.p2p.BootstrapListener;
 import bisq.network.p2p.P2PService;
 import bisq.network.p2p.storage.HashMapChangedListener;
-import bisq.network.p2p.storage.P2PDataStorage;
 import bisq.network.p2p.storage.payload.ProtectedStorageEntry;
 
 import bisq.common.UserThread;
@@ -49,8 +48,6 @@
 
 import javax.annotation.Nullable;
 
-import static bisq.network.p2p.storage.P2PDataStorage.get32ByteHashAsByteArray;
-
 /**
  * Handles storage and retrieval of offers.
  * Uses an invalidation flag to only request the full offer map in case there was a change (anyone has added or removed an offer).
@@ -59,9 +56,9 @@
 public class OfferBookService {
 
     public interface OfferBookChangedListener {
-        void onAdded(Offer offer, P2PDataStorage.ByteArray hashOfPayload);
+        void onAdded(Offer offer);
 
-        void onRemoved(Offer offer, P2PDataStorage.ByteArray hashOfPayload);
+        void onRemoved(Offer offer);
     }
 
     private final P2PService p2PService;
@@ -94,8 +91,7 @@ public void onAdded(Collection<ProtectedStorageEntry> protectedStorageEntries) {
                         OfferPayload offerPayload = (OfferPayload) protectedStorageEntry.getProtectedStoragePayload();
                         Offer offer = new Offer(offerPayload);
                         offer.setPriceFeedService(priceFeedService);
-                        P2PDataStorage.ByteArray hashOfPayload = get32ByteHashAsByteArray(offerPayload);
-                        listener.onAdded(offer, hashOfPayload);
+                        listener.onAdded(offer);
                     }
                 }));
             }
@@ -107,8 +103,7 @@ public void onRemoved(Collection<ProtectedStorageEntry> protectedStorageEntries)
                         OfferPayload offerPayload = (OfferPayload) protectedStorageEntry.getProtectedStoragePayload();
                         Offer offer = new Offer(offerPayload);
                         offer.setPriceFeedService(priceFeedService);
-                        P2PDataStorage.ByteArray hashOfPayload = get32ByteHashAsByteArray(offerPayload);
-                        listener.onRemoved(offer, hashOfPayload);
+                        listener.onRemoved(offer);
                     }
                 }));
             }
@@ -120,12 +115,12 @@ public void onRemoved(Collection<ProtectedStorageEntry> protectedStorageEntries)
                 public void onUpdatedDataReceived() {
                     addOfferBookChangedListener(new OfferBookChangedListener() {
                         @Override
-                        public void onAdded(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+                        public void onAdded(Offer offer) {
                             doDumpStatistics();
                         }
 
                         @Override
-                        public void onRemoved(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+                        public void onRemoved(Offer offer) {
                             doDumpStatistics();
                         }
                     });

core/src/main/java/bisq/core/offer/OfferPayload.java

@@ -22,10 +22,12 @@
 import bisq.network.p2p.storage.payload.ProtectedStoragePayload;
 import bisq.network.p2p.storage.payload.RequiresOwnerIsOnlinePayload;
 
+import bisq.common.crypto.Hash;
 import bisq.common.crypto.PubKeyRing;
 import bisq.common.proto.ProtoUtil;
 import bisq.common.util.CollectionUtils;
 import bisq.common.util.ExtraDataMapValidator;
+import bisq.common.util.Hex;
 import bisq.common.util.JsonExclude;
 
 import java.security.PublicKey;
@@ -121,16 +123,16 @@ public static protobuf.OfferPayload.Direction toProtoMessage(Direction direction
     private final String counterCurrencyCode;
 
     @Deprecated
-    // Not used anymore but we cannot set it Nullable or remove it to not break backward compatibility (diff. hash)
+    // Not used anymore, but we cannot set it Nullable or remove it to not break backward compatibility (diff. hash)
     private final List<NodeAddress> arbitratorNodeAddresses;
     @Deprecated
-    // Not used anymore but we cannot set it Nullable or remove it to not break backward compatibility (diff. hash)
+    // Not used anymore, but we cannot set it Nullable or remove it to not break backward compatibility (diff. hash)
     private final List<NodeAddress> mediatorNodeAddresses;
     private final String paymentMethodId;
     private final String makerPaymentAccountId;
-    // Mutable property. Has to be set before offer is save in P2P network as it changes the objects hash!
-    @Nullable
+    // Mutable property. Has to be set before offer is saved in P2P network as it changes the payload hash!
     @Setter
+    @Nullable
     private String offerFeePaymentTxId;
     @Nullable
     private final String countryCode;
@@ -175,6 +177,10 @@ public static protobuf.OfferPayload.Direction toProtoMessage(Direction direction
     private final Map<String, String> extraDataMap;
     private final int protocolVersion;
 
+    // Cache the payload hash to avoid some repeated calculations.
+    // Take care to calculate it only after the offerFeePaymentTxId is set.
+    private transient byte[] hash;
+
 
     ///////////////////////////////////////////////////////////////////////////////////////////
     // Constructor
@@ -256,8 +262,19 @@ public OfferPayload(String id,
         this.hashOfChallenge = hashOfChallenge;
         this.extraDataMap = ExtraDataMapValidator.getValidatedExtraDataMap(extraDataMap);
         this.protocolVersion = protocolVersion;
+        this.hash = null; // Do not calculate hash before offerFeePaymentTxId is set.
     }
 
+    public byte[] getHash() {
+        if (this.hash == null && this.offerFeePaymentTxId != null) {
+            // A proto message can be created only after the offerFeePaymentTxId is
+            // set to a non-null value;  now is the time to cache the payload hash.
+            this.hash = Hash.getSha256Hash(this.toProtoMessage().toByteArray());
+        }
+        return this.hash;
+    }
+
+
     ///////////////////////////////////////////////////////////////////////////////////////////
     // PROTO BUFFER
     ///////////////////////////////////////////////////////////////////////////////////////////
@@ -431,6 +448,7 @@ public String toString() {
                 ",\n     hashOfChallenge='" + hashOfChallenge + '\'' +
                 ",\n     extraDataMap=" + extraDataMap +
                 ",\n     protocolVersion=" + protocolVersion +
+                ",\n     hash=" + (getHash() == null ? "null" : Hex.encode(getHash())) +
                 "\n}";
     }
 }

desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBook.java

@@ -68,7 +68,7 @@ public class OfferBook {
 
         offerBookService.addOfferBookChangedListener(new OfferBookService.OfferBookChangedListener() {
             @Override
-            public void onAdded(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+            public void onAdded(Offer offer) {
                 printOfferBookListItems("Before onAdded");
                 // We get onAdded called every time a new ProtectedStorageEntry is received.
                 // Mostly it is the same OfferPayload but the ProtectedStorageEntry is different.
@@ -90,14 +90,14 @@ public void onAdded(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
                 // and errorMessage.
                 boolean hasSameOffer = offerBookListItems.stream().anyMatch(item -> item.getOffer().equals(offer));
                 if (!hasSameOffer) {
-                    OfferBookListItem newOfferBookListItem = new OfferBookListItem(offer, hashOfPayload);
+                    OfferBookListItem newOfferBookListItem = new OfferBookListItem(offer);
                     removeDuplicateItem(newOfferBookListItem);
                     offerBookListItems.add(newOfferBookListItem);  // Add replacement.
                     if (log.isDebugEnabled()) {  // TODO delete debug stmt in future PR.
                         log.debug("onAdded: Added new offer {}\n"
                                         + "\twith newItem.payloadHash: {}",
                                 offer.getId(),
-                                newOfferBookListItem.hashOfPayload == null ? "null" : newOfferBookListItem.hashOfPayload.getHex());
+                                newOfferBookListItem.hashOfPayload.getHex());
                     }
                 } else {
                     log.debug("We have the exact same offer already in our list and ignore the onAdded call. ID={}", offer.getId());
@@ -106,9 +106,9 @@ public void onAdded(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
             }
 
             @Override
-            public void onRemoved(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+            public void onRemoved(Offer offer) {
                 printOfferBookListItems("Before onRemoved");
-                removeOffer(offer, hashOfPayload);
+                removeOffer(offer);
                 printOfferBookListItems("After onRemoved");
             }
         });
@@ -128,66 +128,65 @@ private void removeDuplicateItem(OfferBookListItem newOfferBookListItem) {
                                 + "\twith payload hash {} from list.\n"
                                 + "\tThis may make a subsequent onRemoved( {} ) call redundant.",
                         offerId,
-                        oldOfferItem.getHashOfPayload() == null ? "null" : oldOfferItem.getHashOfPayload().getHex(),
+                        oldOfferItem.getHashOfPayload().getHex(),
                         oldOfferItem.getOffer().getId());
             }
         });
     }
 
-    public void removeOffer(Offer offer, P2PDataStorage.ByteArray hashOfPayload) {
+    public void removeOffer(Offer offer) {
         // Update state in case that that offer is used in the take offer screen, so it gets updated correctly
         offer.setState(Offer.State.REMOVED);
         offer.cancelAvailabilityRequest();
 
+        P2PDataStorage.ByteArray hashOfPayload = new P2PDataStorage.ByteArray(offer.getOfferPayload().getHash());
+
         if (log.isDebugEnabled()) {  // TODO delete debug stmt in future PR.
             log.debug("onRemoved: id = {}\n"
                             + "\twith payload-hash = {}",
                     offer.getId(),
-                    hashOfPayload == null ? "null" : hashOfPayload.getHex());
+                    hashOfPayload.getHex());
         }
 
         // Find the removal candidate in the OfferBook list with matching offerId and payload-hash.
         Optional<OfferBookListItem> candidateWithMatchingPayloadHash = offerBookListItems.stream()
-                .filter(item -> item.getOffer().getId().equals(offer.getId()) && (
-                        item.hashOfPayload == null
-                                || item.hashOfPayload.equals(hashOfPayload))
-                )
+                .filter(item -> item.getOffer().getId().equals(offer.getId())
+                        && item.hashOfPayload.equals(hashOfPayload))
                 .findAny();
 
         if (!candidateWithMatchingPayloadHash.isPresent()) {
             if (log.isDebugEnabled()) {  // TODO delete debug stmt in future PR.
                 log.debug("UI view list does not contain offer with id {} and payload-hash {}",
                         offer.getId(),
-                        hashOfPayload == null ? "null" : hashOfPayload.getHex());
+                        hashOfPayload.getHex());
             }
             return;
         }
 
         OfferBookListItem candidate = candidateWithMatchingPayloadHash.get();
 
-        // Remove the candidate only if the candidate's offer payload is null (after list
-        // is populated by 'fillOfferBookListItems()'), or the hash matches the onRemoved
-        // hashOfPayload parameter.  We may receive add/remove messages out of order
-        // (from api's 'editoffer'), and use the offer payload hash to ensure we do not
-        // remove an edited offer immediately after it was added.
-        if ((candidate.getHashOfPayload() == null || candidate.getHashOfPayload().equals(hashOfPayload))) {
+        // Remove the candidate only if the candidate's offer payload the hash matches the
+        // onRemoved hashOfPayload parameter.  We may receive add/remove messages out of
+        // order from the API's 'editoffer' method, and use the offer payload hash to
+        // ensure we do not remove an edited offer immediately after it was added.
+        if (candidate.getHashOfPayload().equals(hashOfPayload)) {
             // The payload-hash test passed, remove the candidate and print reason.
             offerBookListItems.remove(candidate);
 
             if (log.isDebugEnabled()) {  // TODO delete debug stmt in future PR.
-                log.debug("Candidate.payload-hash: {} is null or == onRemoved.payload-hash: {} ?"
+                log.debug("Candidate.payload-hash: {} == onRemoved.payload-hash: {} ?"
                                 + " Yes, removed old offer",
-                        candidate.hashOfPayload == null ? "null" : candidate.hashOfPayload.getHex(),
-                        hashOfPayload == null ? "null" : hashOfPayload.getHex());
+                        candidate.hashOfPayload.getHex(),
+                        hashOfPayload.getHex());
             }
         } else {
             if (log.isDebugEnabled()) {  // TODO delete debug stmt in future PR.
                 // Candidate's payload-hash test failed:  payload-hash != onRemoved.payload-hash.
                 // Print reason for not removing candidate.
                 log.debug("Candidate.payload-hash: {} == onRemoved.payload-hash: {} ?"
                                 + " No, old offer not removed",
-                        candidate.hashOfPayload == null ? "null" : candidate.hashOfPayload.getHex(),
-                        hashOfPayload == null ? "null" : hashOfPayload.getHex());
+                        candidate.hashOfPayload.getHex(),
+                        hashOfPayload.getHex());
             }
         }
     }

desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBookListItem.java

@@ -42,8 +42,6 @@
 
 import org.jetbrains.annotations.NotNull;
 
-import javax.annotation.Nullable;
-
 @Slf4j
 public class OfferBookListItem {
     @Getter
@@ -58,11 +56,7 @@ public class OfferBookListItem {
      * envelope bundles.  It can happen when the API is used to edit an offer because
      * the remove/add msgs are received in the same envelope bundle, then processed in
      * unpredictable order.
-     *
-     * A null value indicates the item's payload hash has not been set by onAdded or
-     * onRemoved since the most recent OfferBook view refresh.
      */
-    @Nullable
     @Getter
     P2PDataStorage.ByteArray hashOfPayload;
 
@@ -71,12 +65,8 @@ public class OfferBookListItem {
     private WitnessAgeData witnessAgeData;
 
     public OfferBookListItem(Offer offer) {
-        this(offer, null);
-    }
-
-    public OfferBookListItem(Offer offer, @Nullable P2PDataStorage.ByteArray hashOfPayload) {
         this.offer = offer;
-        this.hashOfPayload = hashOfPayload;
+        this.hashOfPayload = new P2PDataStorage.ByteArray(offer.getOfferPayload().getHash());
     }
 
     public WitnessAgeData getWitnessAgeData(AccountAgeWitnessService accountAgeWitnessService,
@@ -124,7 +114,7 @@ public WitnessAgeData getWitnessAgeData(AccountAgeWitnessService accountAgeWitne
     public String toString() {
         return "OfferBookListItem{" +
                 "offerId=" + offer.getId() +
-                ", hashOfPayload=" + (hashOfPayload == null ? "null" : hashOfPayload.getHex()) +
+                ", hashOfPayload=" + hashOfPayload.getHex() +
                 ", witnessAgeData=" + (witnessAgeData == null ? "null" : witnessAgeData.displayString) +
                 '}';
     }

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java

@@ -298,7 +298,7 @@ public void onClose(boolean removeOffer) {
         // only local effect. Other trader might see the offer for a few seconds
         // still (but cannot take it).
         if (removeOffer) {
-            offerBook.removeOffer(checkNotNull(offer), null);
+            offerBook.removeOffer(checkNotNull(offer));
         }
 
         btcWalletService.resetAddressEntriesForOpenOffer(offer.getId());

desktop/src/test/java/bisq/desktop/main/offer/offerbook/OfferBookViewModelTest.java

@@ -77,6 +77,7 @@
 import static com.natpryce.makeiteasy.MakeItEasy.with;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
@@ -391,12 +392,19 @@ public void testMaxCharactersForPriceDistance() {
         when(priceFeedService.updateCounterProperty()).thenReturn(new SimpleIntegerProperty());
 
         final OfferBookListItem item1 = make(item);
+        assertNotNull(item1.getHashOfPayload());
         item1.getOffer().setPriceFeedService(priceFeedService);
+
         final OfferBookListItem item2 = make(item.but(with(marketPriceMargin, 0.0197)));
+        assertNotNull(item2.getHashOfPayload());
         item2.getOffer().setPriceFeedService(priceFeedService);
+
         final OfferBookListItem item3 = make(item.but(with(marketPriceMargin, 0.1)));
+        assertNotNull(item3.getHashOfPayload());
         item3.getOffer().setPriceFeedService(priceFeedService);
+
         final OfferBookListItem item4 = make(item.but(with(marketPriceMargin, -0.1)));
+        assertNotNull(item4.getHashOfPayload());
         item4.getOffer().setPriceFeedService(priceFeedService);
         offerBookListItems.addAll(item1, item2);
 
@@ -427,12 +435,15 @@ public void testGetPrice() {
         final OfferBookListItem item = make(btcBuyItem.but(
                 with(useMarketBasedPrice, true),
                 with(marketPriceMargin, -0.12)));
+        assertNotNull(item.getHashOfPayload());
 
         final OfferBookListItem lowItem = make(btcBuyItem.but(
                 with(useMarketBasedPrice, true),
                 with(marketPriceMargin, 0.01)));
+        assertNotNull(lowItem.getHashOfPayload());
 
         final OfferBookListItem fixedItem = make(btcBuyItem);
+        assertNotNull(fixedItem.getHashOfPayload());
 
         item.getOffer().setPriceFeedService(priceFeedService);
         lowItem.getOffer().setPriceFeedService(priceFeedService);