From 1314fd61a5e76eb73da054d018c0ac57c590279d Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Tue, 15 Dec 2020 01:10:10 -0500 Subject: [PATCH 1/9] Add accountAgeWitnessCache The accountAgeWitnessMap is very large (70k items) and access is a bit expensive. We usually only access less than 100 items, those who have offers online. So we use a cache for a fast lookup and only if not found there we use the accountAgeWitnessMap and put then the new item into our cache. --- .../witness/AccountAgeWitnessService.java | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java index b7678c7fa0c..9b8a0daab8c 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java @@ -138,9 +138,13 @@ public String getDisplayString() { @Getter private final AccountAgeWitnessUtils accountAgeWitnessUtils; - @Getter private final Map accountAgeWitnessMap = new HashMap<>(); + // The accountAgeWitnessMap is very large (70k items) and access is a bit expensive. We usually only access less + // than 100 items, those who have offers online. So we use a cache for a fast lookup and only if + // not found there we use the accountAgeWitnessMap and put then the new item into our cache. + private final Map accountAgeWitnessCache = new HashMap<>(); + /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -233,8 +237,17 @@ public void addToMap(AccountAgeWitness accountAgeWitness) { public void publishMyAccountAgeWitness(PaymentAccountPayload paymentAccountPayload) { AccountAgeWitness accountAgeWitness = getMyWitness(paymentAccountPayload); - if (!accountAgeWitnessMap.containsKey(accountAgeWitness.getHashAsByteArray())) + P2PDataStorage.ByteArray hash = accountAgeWitness.getHashAsByteArray(); + + // We use first our fast lookup cache. If its in accountAgeWitnessCache it is also in accountAgeWitnessMap + // and we do not publish. + if (accountAgeWitnessCache.containsKey(hash)) { + return; + } + + if (!accountAgeWitnessMap.containsKey(hash)) { p2PService.addPersistableNetworkPayload(accountAgeWitness, false); + } } public byte[] getPeerAccountAgeWitnessHash(Trade trade) { @@ -284,12 +297,21 @@ private Optional findTradePeerWitness(Trade trade) { private Optional getWitnessByHash(byte[] hash) { P2PDataStorage.ByteArray hashAsByteArray = new P2PDataStorage.ByteArray(hash); - final boolean containsKey = accountAgeWitnessMap.containsKey(hashAsByteArray); - if (!containsKey) - log.debug("hash not found in accountAgeWitnessMap"); + // First we look up in our fast lookup cache + if (accountAgeWitnessCache.containsKey(hashAsByteArray)) { + return Optional.of(accountAgeWitnessCache.get(hashAsByteArray)); + } + + if (accountAgeWitnessMap.containsKey(hashAsByteArray)) { + AccountAgeWitness accountAgeWitness = accountAgeWitnessMap.get(hashAsByteArray); + + // We add it to our fast lookup cache + accountAgeWitnessCache.put(hashAsByteArray, accountAgeWitness); - return accountAgeWitnessMap.containsKey(hashAsByteArray) ? - Optional.of(accountAgeWitnessMap.get(hashAsByteArray)) : Optional.empty(); + return Optional.of(accountAgeWitness); + } + + return Optional.empty(); } private Optional getWitnessByHashAsHex(String hashAsHex) { From 359dc3759dae1ea16642da0a40f8412bd660edb4 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Tue, 15 Dec 2020 01:15:35 -0500 Subject: [PATCH 2/9] Add getSignedWitnessSetCache The getSignedWitnessSet is called very often and is a bit expensive. We cache the result in that map but we remove the cache entry if we get a matching SignedWitness added to the signedWitnessMap. --- .../account/sign/SignedWitnessService.java | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 788c369eed6..8bcf2749a87 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -75,10 +75,19 @@ public class SignedWitnessService { private final ArbitratorManager arbitratorManager; private final User user; + // TODO signedWitnessMap could be a HashSet as clients access the map values only. + // Only one test uses the key for removing an entry but seems that use case does not exist otherwise. + // As there are only about 5000 objects it will probably not have a significatn impact to change that... @Getter private final Map signedWitnessMap = new HashMap<>(); + + // The getSignedWitnessSet is called very often and is a bit expensive. We cache the result in that map but we + // remove the cache entry if we get a matching SignedWitness added to the signedWitnessMap. + private final Map> getSignedWitnessSetCache = new HashMap<>(); + private final FilterManager filterManager; + /////////////////////////////////////////////////////////////////////////////////////////// // Constructor /////////////////////////////////////////////////////////////////////////////////////////// @@ -345,9 +354,21 @@ private boolean verifySignatureWithDSAKey(SignedWitness signedWitness) { } private Set getSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - return signedWitnessMap.values().stream() - .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) + byte[] hash = accountAgeWitness.getHash(); + P2PDataStorage.ByteArray key = new P2PDataStorage.ByteArray(hash); + // In case we get a new entry added to signedWitnessMap with our hash we remove the entry from the cache so + // that we use the updated signedWitnessMap to fill the cache new. + if (getSignedWitnessSetCache.containsKey(key)) { + return getSignedWitnessSetCache.get(key); + } + + Set result = signedWitnessMap.values().stream() + .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), hash)) .collect(Collectors.toSet()); + + getSignedWitnessSetCache.put(key, result); + + return result; } // SignedWitness objects signed by arbitrators @@ -489,6 +510,10 @@ private boolean verifyDate(SignedWitness signedWitness, long childSignedWitnessD @VisibleForTesting void addToMap(SignedWitness signedWitness) { signedWitnessMap.putIfAbsent(signedWitness.getHashAsByteArray(), signedWitness); + + // We remove the entry with that hash in case we have cached it, so at the next getSignedWitnessSet + // call we use the updated signedWitnessMap to re-fill our cache. + getSignedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); } private void publishSignedWitness(SignedWitness signedWitness) { From 425bfa3bf72a58bdbbc695391cf54cf6fb5cfcd8 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Tue, 15 Dec 2020 10:20:49 -0500 Subject: [PATCH 3/9] Fix failing test (remove did operate on map directly and we did not remove the item from the cache) - Add getSignedWitnessMapValues method for access to signedWitnessMap values - Remove Getter for signedWitnessMap - Add removeSignedWitness method (used in test only) - Use addToMap in test instead of direct access to map - Remove getSignedWitnessSetCache entry matching signedWitness.getHashAsByteArray() as well. See comment. @sqrrm: Can you cross check? --- .../account/sign/SignedWitnessService.java | 49 +++++++++++++------ .../witness/AccountAgeWitnessService.java | 2 +- .../witness/AccountAgeWitnessUtils.java | 10 ++-- .../witness/AccountAgeWitnessServiceTest.java | 8 +-- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 8bcf2749a87..17b33d075d7 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -51,6 +51,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Date; import java.util.HashMap; import java.util.HashSet; @@ -61,7 +62,6 @@ import java.util.Stack; import java.util.stream.Collectors; -import lombok.Getter; import lombok.extern.slf4j.Slf4j; @Slf4j @@ -75,10 +75,6 @@ public class SignedWitnessService { private final ArbitratorManager arbitratorManager; private final User user; - // TODO signedWitnessMap could be a HashSet as clients access the map values only. - // Only one test uses the key for removing an entry but seems that use case does not exist otherwise. - // As there are only about 5000 objects it will probably not have a significatn impact to change that... - @Getter private final Map signedWitnessMap = new HashMap<>(); // The getSignedWitnessSet is called very often and is a bit expensive. We cache the result in that map but we @@ -151,6 +147,10 @@ private void onBootstrapComplete() { // API /////////////////////////////////////////////////////////////////////////////////////////// + public Collection getSignedWitnessMapValues() { + return signedWitnessMap.values(); + } + /** * List of dates as long when accountAgeWitness was signed * @@ -208,7 +208,7 @@ public String ownerPubKeyAsString(AccountAgeWitness accountAgeWitness) { @VisibleForTesting public Set getSignedWitnessSetByOwnerPubKey(byte[] ownerPubKey) { - return signedWitnessMap.values().stream() + return getSignedWitnessMapValues().stream() .filter(e -> Arrays.equals(e.getWitnessOwnerPubKey(), ownerPubKey)) .collect(Collectors.toSet()); } @@ -362,7 +362,7 @@ private Set getSignedWitnessSet(AccountAgeWitness accountAgeWitne return getSignedWitnessSetCache.get(key); } - Set result = signedWitnessMap.values().stream() + Set result = getSignedWitnessMapValues().stream() .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), hash)) .collect(Collectors.toSet()); @@ -373,7 +373,7 @@ private Set getSignedWitnessSet(AccountAgeWitness accountAgeWitne // SignedWitness objects signed by arbitrators public Set getArbitratorsSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - return signedWitnessMap.values().stream() + return getSignedWitnessMapValues().stream() .filter(SignedWitness::isSignedByArbitrator) .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) .collect(Collectors.toSet()); @@ -381,14 +381,14 @@ public Set getArbitratorsSignedWitnessSet(AccountAgeWitness accou // SignedWitness objects signed by any other peer public Set getTrustedPeerSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - return signedWitnessMap.values().stream() + return getSignedWitnessMapValues().stream() .filter(e -> !e.isSignedByArbitrator()) .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) .collect(Collectors.toSet()); } public Set getRootSignedWitnessSet(boolean includeSignedByArbitrator) { - return signedWitnessMap.values().stream() + return getSignedWitnessMapValues().stream() .filter(witness -> getSignedWitnessSetByOwnerPubKey(witness.getSignerPubKey(), new Stack<>()).isEmpty()) .filter(witness -> includeSignedByArbitrator || witness.getVerificationMethod() != SignedWitness.VerificationMethod.ARBITRATOR) @@ -409,7 +409,7 @@ public Set getUnsignedSignerPubKeys() { // witnessOwnerPubKey private Set getSignedWitnessSetByOwnerPubKey(byte[] ownerPubKey, Stack excluded) { - return signedWitnessMap.values().stream() + return getSignedWitnessMapValues().stream() .filter(e -> Arrays.equals(e.getWitnessOwnerPubKey(), ownerPubKey)) .filter(e -> !excluded.contains(new P2PDataStorage.ByteArray(e.getSignerPubKey()))) .collect(Collectors.toSet()); @@ -508,12 +508,18 @@ private boolean verifyDate(SignedWitness signedWitness, long childSignedWitnessD /////////////////////////////////////////////////////////////////////////////////////////// @VisibleForTesting - void addToMap(SignedWitness signedWitness) { - signedWitnessMap.putIfAbsent(signedWitness.getHashAsByteArray(), signedWitness); + public void addToMap(SignedWitness signedWitness) { + P2PDataStorage.ByteArray hash = signedWitness.getHashAsByteArray(); + signedWitnessMap.putIfAbsent(hash, signedWitness); // We remove the entry with that hash in case we have cached it, so at the next getSignedWitnessSet // call we use the updated signedWitnessMap to re-fill our cache. getSignedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); + + // Not sure if that is needed as well, tests did succeed in both cases, but seems to be more safe to remove + // potential entries with hash as well. A removed item in getSignedWitnessSetCache carries no risk, though a + // remaining item would. + getSignedWitnessSetCache.remove(hash); } private void publishSignedWitness(SignedWitness signedWitness) { @@ -526,7 +532,22 @@ private void publishSignedWitness(SignedWitness signedWitness) { } private void doRepublishAllSignedWitnesses() { - signedWitnessMap.forEach((e, signedWitness) -> p2PService.addPersistableNetworkPayload(signedWitness, true)); + getSignedWitnessMapValues() + .forEach(signedWitness -> p2PService.addPersistableNetworkPayload(signedWitness, true)); + } + + @VisibleForTesting + public void removeSignedWitness(SignedWitness signedWitness) { + P2PDataStorage.ByteArray hash = signedWitness.getHashAsByteArray(); + signedWitnessMap.remove(hash); + + // Need to remove the entry matching signedWitness.getAccountAgeWitnessHash() (test would fail otherwise) + getSignedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); + + // Not sure if that is needed as well, tests did succeed in both cases, but seems to be more safe to remove + // potential entries with hash as well. A removed item in getSignedWitnessSetCache carries no risk, though a + // remaining item would. + getSignedWitnessSetCache.remove(hash); } // Remove SignedWitnesses that are signed by TRADE that also have an ARBITRATOR signature diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java index 9b8a0daab8c..c3f4f196c1a 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java @@ -681,7 +681,7 @@ public String arbitratorSignOrphanWitness(AccountAgeWitness accountAgeWitness, ECKey key, long time) { // Find AccountAgeWitness as signedwitness - var signedWitness = signedWitnessService.getSignedWitnessMap().values().stream() + var signedWitness = signedWitnessService.getSignedWitnessMapValues().stream() .filter(sw -> Arrays.equals(sw.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) .findAny() .orElse(null); diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessUtils.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessUtils.java index 6bc4b27218b..628f0a81862 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessUtils.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessUtils.java @@ -30,6 +30,7 @@ import bisq.common.util.Utilities; import java.util.Arrays; +import java.util.Collection; import java.util.Optional; import java.util.Stack; @@ -67,12 +68,11 @@ public void logSignedWitnesses() { } private void logChild(SignedWitness sigWit, String initString, Stack excluded) { - var allSig = signedWitnessService.getSignedWitnessMap(); log.info("{}AEW: {} PKH: {} time: {}", initString, Utilities.bytesAsHexString(sigWit.getAccountAgeWitnessHash()).substring(0, 7), Utilities.bytesAsHexString(Hash.getRipemd160hash(sigWit.getWitnessOwnerPubKey())).substring(0, 7), sigWit.getDate()); - allSig.values().forEach(w -> { + signedWitnessService.getSignedWitnessMapValues().forEach(w -> { if (!excluded.contains(new P2PDataStorage.ByteArray(w.getWitnessOwnerPubKey())) && Arrays.equals(w.getSignerPubKey(), sigWit.getWitnessOwnerPubKey())) { excluded.push(new P2PDataStorage.ByteArray(w.getWitnessOwnerPubKey())); @@ -85,10 +85,10 @@ private void logChild(SignedWitness sigWit, String initString, Stack { + Collection signedWitnessMapValues = signedWitnessService.getSignedWitnessMapValues(); + signedWitnessMapValues.forEach(w -> { log.info("AEW {}", Utilities.bytesAsHexString(w.getAccountAgeWitnessHash())); - allSig.values().forEach(ww -> { + signedWitnessMapValues.forEach(ww -> { if (Arrays.equals(w.getSignerPubKey(), ww.getWitnessOwnerPubKey())) { log.info(" {}", Utilities.bytesAsHexString(ww.getAccountAgeWitnessHash())); } diff --git a/core/src/test/java/bisq/core/account/witness/AccountAgeWitnessServiceTest.java b/core/src/test/java/bisq/core/account/witness/AccountAgeWitnessServiceTest.java index 06ffd4470c1..946e4db64d4 100644 --- a/core/src/test/java/bisq/core/account/witness/AccountAgeWitnessServiceTest.java +++ b/core/src/test/java/bisq/core/account/witness/AccountAgeWitnessServiceTest.java @@ -310,12 +310,12 @@ public void testArbitratorSignDummyWitness() throws CryptoException { // Remove SignedWitness signed by arbitrator @SuppressWarnings("OptionalGetWithoutIsPresent") - var signedWitnessArb = signedWitnessService.getSignedWitnessMap().values().stream() + var signedWitnessArb = signedWitnessService.getSignedWitnessMapValues().stream() .filter(sw -> sw.getVerificationMethod() == SignedWitness.VerificationMethod.ARBITRATOR) .findAny() .get(); - signedWitnessService.getSignedWitnessMap().remove(signedWitnessArb.getHashAsByteArray()); - assertEquals(signedWitnessService.getSignedWitnessMap().size(), 2); + signedWitnessService.removeSignedWitness(signedWitnessArb); + assertEquals(signedWitnessService.getSignedWitnessMapValues().size(), 2); // Check that no account age witness is a signer assertFalse(service.accountIsSigner(aew1)); @@ -354,7 +354,7 @@ private void signAccountAgeWitness(AccountAgeWitness accountAgeWitness, witnessOwnerPubKey.getEncoded(), time, SignedWitnessService.MINIMUM_TRADE_AMOUNT_FOR_SIGNING.value); - signedWitnessService.getSignedWitnessMap().putIfAbsent(signedWitness.getHashAsByteArray(), signedWitness); + signedWitnessService.addToMap(signedWitness); } } From f2273e663db4712651e2022b6a73607e6cd78975 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Tue, 15 Dec 2020 14:44:30 -0500 Subject: [PATCH 4/9] Use ConcurrentHashMap for cache. As the normal maps are not using a ConcurrentHashMap it is likely unnecessary as well that I am not aware of a multi-threaded access. But as it does not show any difference in performance it is likely the bit more safe option. --- .../main/java/bisq/core/account/sign/SignedWitnessService.java | 3 ++- .../bisq/core/account/witness/AccountAgeWitnessService.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 17b33d075d7..e2ff6e152d4 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -60,6 +60,7 @@ import java.util.Optional; import java.util.Set; import java.util.Stack; +import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -79,7 +80,7 @@ public class SignedWitnessService { // The getSignedWitnessSet is called very often and is a bit expensive. We cache the result in that map but we // remove the cache entry if we get a matching SignedWitness added to the signedWitnessMap. - private final Map> getSignedWitnessSetCache = new HashMap<>(); + private final Map> getSignedWitnessSetCache = new ConcurrentHashMap<>(); private final FilterManager filterManager; diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java index c3f4f196c1a..62ff315d02d 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java @@ -73,6 +73,7 @@ import java.util.Optional; import java.util.Random; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -143,7 +144,7 @@ public String getDisplayString() { // The accountAgeWitnessMap is very large (70k items) and access is a bit expensive. We usually only access less // than 100 items, those who have offers online. So we use a cache for a fast lookup and only if // not found there we use the accountAgeWitnessMap and put then the new item into our cache. - private final Map accountAgeWitnessCache = new HashMap<>(); + private final Map accountAgeWitnessCache = new ConcurrentHashMap<>(); /////////////////////////////////////////////////////////////////////////////////////////// From 35e2bf63831f4c9a5592fdf9ad15da78eb6bc1ca Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Sun, 20 Dec 2020 09:34:00 -0500 Subject: [PATCH 5/9] Rename getSignedWitnessSetCache to signedWitnessSetCache --- .../core/account/sign/SignedWitnessService.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index e2ff6e152d4..7caf96d62ea 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -80,7 +80,7 @@ public class SignedWitnessService { // The getSignedWitnessSet is called very often and is a bit expensive. We cache the result in that map but we // remove the cache entry if we get a matching SignedWitness added to the signedWitnessMap. - private final Map> getSignedWitnessSetCache = new ConcurrentHashMap<>(); + private final Map> signedWitnessSetCache = new ConcurrentHashMap<>(); private final FilterManager filterManager; @@ -359,15 +359,15 @@ private Set getSignedWitnessSet(AccountAgeWitness accountAgeWitne P2PDataStorage.ByteArray key = new P2PDataStorage.ByteArray(hash); // In case we get a new entry added to signedWitnessMap with our hash we remove the entry from the cache so // that we use the updated signedWitnessMap to fill the cache new. - if (getSignedWitnessSetCache.containsKey(key)) { - return getSignedWitnessSetCache.get(key); + if (signedWitnessSetCache.containsKey(key)) { + return signedWitnessSetCache.get(key); } Set result = getSignedWitnessMapValues().stream() .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), hash)) .collect(Collectors.toSet()); - getSignedWitnessSetCache.put(key, result); + signedWitnessSetCache.put(key, result); return result; } @@ -515,12 +515,12 @@ public void addToMap(SignedWitness signedWitness) { // We remove the entry with that hash in case we have cached it, so at the next getSignedWitnessSet // call we use the updated signedWitnessMap to re-fill our cache. - getSignedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); + signedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); // Not sure if that is needed as well, tests did succeed in both cases, but seems to be more safe to remove // potential entries with hash as well. A removed item in getSignedWitnessSetCache carries no risk, though a // remaining item would. - getSignedWitnessSetCache.remove(hash); + signedWitnessSetCache.remove(hash); } private void publishSignedWitness(SignedWitness signedWitness) { @@ -543,12 +543,12 @@ public void removeSignedWitness(SignedWitness signedWitness) { signedWitnessMap.remove(hash); // Need to remove the entry matching signedWitness.getAccountAgeWitnessHash() (test would fail otherwise) - getSignedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); + signedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); // Not sure if that is needed as well, tests did succeed in both cases, but seems to be more safe to remove // potential entries with hash as well. A removed item in getSignedWitnessSetCache carries no risk, though a // remaining item would. - getSignedWitnessSetCache.remove(hash); + signedWitnessSetCache.remove(hash); } // Remove SignedWitnesses that are signed by TRADE that also have an ARBITRATOR signature From b6a01d0ac7ae189b9272bc76b15bba8a9f5d1efb Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Sun, 20 Dec 2020 10:00:28 -0500 Subject: [PATCH 6/9] Rename signedWitnessSetCache to signedWitnessSetByAccountAgeWitnessHash Change comments (after understanding the domain usage better) Maintain the set at add and remove methods Use HashMap instead of ConcurrentHashMap as that class is only used from UserThread and other fields are not threadsafe either. --- .../account/sign/SignedWitnessService.java | 53 ++++++++----------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 7caf96d62ea..73ced302456 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -60,7 +60,6 @@ import java.util.Optional; import java.util.Set; import java.util.Stack; -import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -78,9 +77,9 @@ public class SignedWitnessService { private final Map signedWitnessMap = new HashMap<>(); - // The getSignedWitnessSet is called very often and is a bit expensive. We cache the result in that map but we - // remove the cache entry if we get a matching SignedWitness added to the signedWitnessMap. - private final Map> signedWitnessSetCache = new ConcurrentHashMap<>(); + // This map keeps all SignedWitnesses with the same AccountAgeWitnessHash in a Set. + // This avoids iterations over the signedWitnessMap for getting the set of such SignedWitnesses. + private final Map> signedWitnessSetByAccountAgeWitnessHash = new HashMap<>(); private final FilterManager filterManager; @@ -355,19 +354,19 @@ private boolean verifySignatureWithDSAKey(SignedWitness signedWitness) { } private Set getSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - byte[] hash = accountAgeWitness.getHash(); - P2PDataStorage.ByteArray key = new P2PDataStorage.ByteArray(hash); + byte[] accountAgeWitnessHash = accountAgeWitness.getHash(); + P2PDataStorage.ByteArray key = new P2PDataStorage.ByteArray(accountAgeWitnessHash); // In case we get a new entry added to signedWitnessMap with our hash we remove the entry from the cache so // that we use the updated signedWitnessMap to fill the cache new. - if (signedWitnessSetCache.containsKey(key)) { - return signedWitnessSetCache.get(key); + if (signedWitnessSetByAccountAgeWitnessHash.containsKey(key)) { + return signedWitnessSetByAccountAgeWitnessHash.get(key); } Set result = getSignedWitnessMapValues().stream() - .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), hash)) + .filter(signedWitness -> Arrays.equals(signedWitness.getAccountAgeWitnessHash(), accountAgeWitnessHash)) .collect(Collectors.toSet()); - signedWitnessSetCache.put(key, result); + signedWitnessSetByAccountAgeWitnessHash.put(key, result); return result; } @@ -510,17 +509,11 @@ private boolean verifyDate(SignedWitness signedWitness, long childSignedWitnessD @VisibleForTesting public void addToMap(SignedWitness signedWitness) { - P2PDataStorage.ByteArray hash = signedWitness.getHashAsByteArray(); - signedWitnessMap.putIfAbsent(hash, signedWitness); + signedWitnessMap.putIfAbsent(signedWitness.getHashAsByteArray(), signedWitness); - // We remove the entry with that hash in case we have cached it, so at the next getSignedWitnessSet - // call we use the updated signedWitnessMap to re-fill our cache. - signedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); - - // Not sure if that is needed as well, tests did succeed in both cases, but seems to be more safe to remove - // potential entries with hash as well. A removed item in getSignedWitnessSetCache carries no risk, though a - // remaining item would. - signedWitnessSetCache.remove(hash); + P2PDataStorage.ByteArray accountAgeWitnessHash = new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash()); + signedWitnessSetByAccountAgeWitnessHash.putIfAbsent(accountAgeWitnessHash, new HashSet<>()); + signedWitnessSetByAccountAgeWitnessHash.get(accountAgeWitnessHash).add(signedWitness); } private void publishSignedWitness(SignedWitness signedWitness) { @@ -539,16 +532,16 @@ private void doRepublishAllSignedWitnesses() { @VisibleForTesting public void removeSignedWitness(SignedWitness signedWitness) { - P2PDataStorage.ByteArray hash = signedWitness.getHashAsByteArray(); - signedWitnessMap.remove(hash); - - // Need to remove the entry matching signedWitness.getAccountAgeWitnessHash() (test would fail otherwise) - signedWitnessSetCache.remove(new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash())); - - // Not sure if that is needed as well, tests did succeed in both cases, but seems to be more safe to remove - // potential entries with hash as well. A removed item in getSignedWitnessSetCache carries no risk, though a - // remaining item would. - signedWitnessSetCache.remove(hash); + signedWitnessMap.remove(signedWitness.getHashAsByteArray()); + + P2PDataStorage.ByteArray accountAgeWitnessHash = new P2PDataStorage.ByteArray(signedWitness.getAccountAgeWitnessHash()); + if (signedWitnessSetByAccountAgeWitnessHash.containsKey(accountAgeWitnessHash)) { + Set set = signedWitnessSetByAccountAgeWitnessHash.get(accountAgeWitnessHash); + set.remove(signedWitness); + if (set.isEmpty()) { + signedWitnessSetByAccountAgeWitnessHash.remove(accountAgeWitnessHash); + } + } } // Remove SignedWitnesses that are signed by TRADE that also have an ARBITRATOR signature From aea904d24f032358afa6d000fced4fd5d7287380 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Sun, 20 Dec 2020 10:28:28 -0500 Subject: [PATCH 7/9] We do not need to lookup in getSignedWitnessMapValues anymore as we maintain the signedWitnessSetByAccountAgeWitnessHash at each add/remove operation. --- .../account/sign/SignedWitnessService.java | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 73ced302456..6bbdb793eea 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -353,22 +353,9 @@ private boolean verifySignatureWithDSAKey(SignedWitness signedWitness) { } } - private Set getSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - byte[] accountAgeWitnessHash = accountAgeWitness.getHash(); - P2PDataStorage.ByteArray key = new P2PDataStorage.ByteArray(accountAgeWitnessHash); - // In case we get a new entry added to signedWitnessMap with our hash we remove the entry from the cache so - // that we use the updated signedWitnessMap to fill the cache new. - if (signedWitnessSetByAccountAgeWitnessHash.containsKey(key)) { - return signedWitnessSetByAccountAgeWitnessHash.get(key); - } - - Set result = getSignedWitnessMapValues().stream() - .filter(signedWitness -> Arrays.equals(signedWitness.getAccountAgeWitnessHash(), accountAgeWitnessHash)) - .collect(Collectors.toSet()); - - signedWitnessSetByAccountAgeWitnessHash.put(key, result); - - return result; + public Set getSignedWitnessSet(AccountAgeWitness accountAgeWitness) { + P2PDataStorage.ByteArray key = new P2PDataStorage.ByteArray(accountAgeWitness.getHash()); + return signedWitnessSetByAccountAgeWitnessHash.getOrDefault(key, new HashSet<>()); } // SignedWitness objects signed by arbitrators From 1462673e57dd8887117ac793084cd2cfd39d4438 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Sun, 20 Dec 2020 10:29:46 -0500 Subject: [PATCH 8/9] Use getSignedWitnessSet instead of iterations over getSignedWitnessMapValues --- .../bisq/core/account/sign/SignedWitnessService.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 6bbdb793eea..69a7d49feb0 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -74,6 +74,7 @@ public class SignedWitnessService { private final P2PService p2PService; private final ArbitratorManager arbitratorManager; private final User user; + private final FilterManager filterManager; private final Map signedWitnessMap = new HashMap<>(); @@ -81,8 +82,6 @@ public class SignedWitnessService { // This avoids iterations over the signedWitnessMap for getting the set of such SignedWitnesses. private final Map> signedWitnessSetByAccountAgeWitnessHash = new HashMap<>(); - private final FilterManager filterManager; - /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -360,17 +359,15 @@ public Set getSignedWitnessSet(AccountAgeWitness accountAgeWitnes // SignedWitness objects signed by arbitrators public Set getArbitratorsSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - return getSignedWitnessMapValues().stream() + return getSignedWitnessSet(accountAgeWitness).stream() .filter(SignedWitness::isSignedByArbitrator) - .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) .collect(Collectors.toSet()); } // SignedWitness objects signed by any other peer public Set getTrustedPeerSignedWitnessSet(AccountAgeWitness accountAgeWitness) { - return getSignedWitnessMapValues().stream() + return getSignedWitnessSet(accountAgeWitness).stream() .filter(e -> !e.isSignedByArbitrator()) - .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) .collect(Collectors.toSet()); } From f5bb702d6b4d6aae1e0cbea81f12bf1ef92ad43f Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Sun, 20 Dec 2020 10:32:50 -0500 Subject: [PATCH 9/9] Improve arbitratorSignOrphanWitness method --- .../witness/AccountAgeWitnessService.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java index 62ff315d02d..cf113513c34 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitnessService.java @@ -679,16 +679,20 @@ public void arbitratorSignAccountAgeWitness(Coin tradeAmount, } public String arbitratorSignOrphanWitness(AccountAgeWitness accountAgeWitness, - ECKey key, + ECKey ecKey, long time) { - // Find AccountAgeWitness as signedwitness - var signedWitness = signedWitnessService.getSignedWitnessMapValues().stream() - .filter(sw -> Arrays.equals(sw.getAccountAgeWitnessHash(), accountAgeWitness.getHash())) + // TODO Is not found signedWitness considered an error case? + // Previous code version was throwing an exception in case no signedWitness was found... + + // signAndPublishAccountAgeWitness returns an empty string in success case and error otherwise + return signedWitnessService.getSignedWitnessSet(accountAgeWitness).stream() .findAny() - .orElse(null); - checkNotNull(signedWitness); - return signedWitnessService.signAndPublishAccountAgeWitness(accountAgeWitness, key, - signedWitness.getWitnessOwnerPubKey(), time); + .map(SignedWitness::getWitnessOwnerPubKey) + .map(witnessOwnerPubKey -> + signedWitnessService.signAndPublishAccountAgeWitness(accountAgeWitness, ecKey, + witnessOwnerPubKey, time) + ) + .orElse("No signedWitness found"); } public String arbitratorSignOrphanPubKey(ECKey key,