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?

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<P2PDataStorage.ByteArray, SignedWitness> 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<SignedWitness> 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<SignedWitness> 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<SignedWitness> getSignedWitnessSet(AccountAgeWitness accountAgeWitne
             return getSignedWitnessSetCache.get(key);
         }
 
-        Set<SignedWitness> result = signedWitnessMap.values().stream()
+        Set<SignedWitness> result = getSignedWitnessMapValues().stream()
                 .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), hash))
                 .collect(Collectors.toSet());
 
@@ -373,22 +373,22 @@ private Set<SignedWitness> getSignedWitnessSet(AccountAgeWitness accountAgeWitne
 
     // SignedWitness objects signed by arbitrators
     public Set<SignedWitness> getArbitratorsSignedWitnessSet(AccountAgeWitness accountAgeWitness) {
-        return signedWitnessMap.values().stream()
+        return getSignedWitnessMapValues().stream()
                 .filter(SignedWitness::isSignedByArbitrator)
                 .filter(e -> Arrays.equals(e.getAccountAgeWitnessHash(), accountAgeWitness.getHash()))
                 .collect(Collectors.toSet());
     }
 
     // SignedWitness objects signed by any other peer
     public Set<SignedWitness> 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<SignedWitness> 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<SignedWitness> getUnsignedSignerPubKeys() {
     // witnessOwnerPubKey
     private Set<SignedWitness> getSignedWitnessSetByOwnerPubKey(byte[] ownerPubKey,
                                                                 Stack<P2PDataStorage.ByteArray> 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

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

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<P2PDataStorage.ByteArray> 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<P2PDataStor
     // Log signers per
     public void logSigners() {
         log.info("Signers per AEW");
-        var allSig = signedWitnessService.getSignedWitnessMap();
-        allSig.values().forEach(w -> {
+        Collection<SignedWitness> 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()));
                         }

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