From 66f71e59f8c5a0003798e811f5a07bda3309951d Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 18:23:16 -0800 Subject: [PATCH 1/4] [TESTS] Introduce MapStoreServiceFake Now that we want to make changes to the MapStoreService, it isn't sufficient to have a Fake of the ProtectedDataStoreService. Tests now use a REAL ProtectedDataStoreService and a FAKE MapStoreService to exercise more of the production code and allow future testing of changes to MapStoreService. --- .../bisq/network/p2p/storage/TestState.java | 6 ++- ...viceFake.java => MapStoreServiceFake.java} | 43 +++++++++++++------ 2 files changed, 35 insertions(+), 14 deletions(-) rename p2p/src/test/java/bisq/network/p2p/storage/mocks/{ProtectedDataStoreServiceFake.java => MapStoreServiceFake.java} (55%) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java index 645a2b22457..4e4e2a6a491 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -29,12 +29,13 @@ import bisq.network.p2p.storage.messages.RemoveMailboxDataMessage; import bisq.network.p2p.storage.mocks.AppendOnlyDataStoreServiceFake; import bisq.network.p2p.storage.mocks.ClockFake; -import bisq.network.p2p.storage.mocks.ProtectedDataStoreServiceFake; +import bisq.network.p2p.storage.mocks.MapStoreServiceFake; import bisq.network.p2p.storage.payload.MailboxStoragePayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedMailboxStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; +import bisq.network.p2p.storage.persistence.MapStoreService; import bisq.network.p2p.storage.persistence.ProtectedDataStoreListener; import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; import bisq.network.p2p.storage.persistence.ResourceDataStoreService; @@ -80,7 +81,7 @@ public class TestState { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); this.clockFake = new ClockFake(); - this.protectedDataStoreService = new ProtectedDataStoreServiceFake(); + this.protectedDataStoreService = new ProtectedDataStoreService(); this.mockedStorage = new P2PDataStorage(mock(NetworkNode.class), this.mockBroadcaster, @@ -91,6 +92,7 @@ this.protectedDataStoreService, mock(ResourceDataStoreService.class), this.appendOnlyDataStoreListener = mock(AppendOnlyDataStoreListener.class); this.protectedDataStoreListener = mock(ProtectedDataStoreListener.class); this.hashMapChangedListener = mock(HashMapChangedListener.class); + this.protectedDataStoreService.addService(new MapStoreServiceFake()); this.mockedStorage = createP2PDataStorageForTest( this.mockBroadcaster, diff --git a/p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedDataStoreServiceFake.java b/p2p/src/test/java/bisq/network/p2p/storage/mocks/MapStoreServiceFake.java similarity index 55% rename from p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedDataStoreServiceFake.java rename to p2p/src/test/java/bisq/network/p2p/storage/mocks/MapStoreServiceFake.java index 717a3b806b4..9f282497d8a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedDataStoreServiceFake.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/mocks/MapStoreServiceFake.java @@ -19,33 +19,52 @@ import bisq.network.p2p.storage.P2PDataStorage; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; -import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; +import bisq.network.p2p.storage.persistence.MapStoreService; + +import bisq.common.proto.persistable.PersistableEnvelope; +import bisq.common.proto.persistable.PersistablePayload; +import bisq.common.storage.Storage; + +import java.io.File; import java.util.HashMap; import java.util.Map; +import lombok.Getter; + +import static org.mockito.Mockito.mock; + /** - * Implementation of an in-memory ProtectedDataStoreService that can be used in tests. Removes overhead + * Implementation of an in-memory MapStoreService that can be used in tests. Removes overhead * involving files, resources, and services for tests that don't need it. * * @see Reference */ -public class ProtectedDataStoreServiceFake extends ProtectedDataStoreService { +public class MapStoreServiceFake extends MapStoreService { + @Getter private final Map map; - public ProtectedDataStoreServiceFake() { - super(); - map = new HashMap<>(); + public MapStoreServiceFake() { + super(mock(File.class), mock(Storage.class)); + this.map = new HashMap<>(); } - public Map getMap() { - return map; + @Override + public String getFileName() { + return null; } - public void put(P2PDataStorage.ByteArray hashAsByteArray, ProtectedStorageEntry entry) { - map.put(hashAsByteArray, entry); + @Override + protected PersistableEnvelope createStore() { + return null; } - public ProtectedStorageEntry remove(P2PDataStorage.ByteArray hash, ProtectedStorageEntry protectedStorageEntry) { - return map.remove(hash); + + @Override + public boolean canHandle(PersistablePayload payload) { + return true; + } + + protected void readFromResources(String postFix) { + // do nothing. This Fake only supports in-memory storage. } } From f3faf4bb6356090c103e03abac7954efde28e247 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 18:27:08 -0800 Subject: [PATCH 2/4] Persist changes to ProtectedStorageEntrys With the addition of ProtectedStorageEntrys, there are now persistable maps that have different payloads and the same keys. In the ProtectedDataStoreService case, the value is the ProtectedStorageEntry which has a createdTimeStamp, sequenceNumber, and signature that can all change, but still contain an identical payload. Previously, the service was only updating the on-disk representation on the first object and never again. So, when it was recreated from disk it would not have any of the updated metadata. This was just copied from the append-only implementation where the value was the Payload which was immutable. This hasn't caused any issues to this point, but it causes strange behavior such as always receiving seqNr==1 items from seednodes on startup. It is good practice to keep the in-memory objects and on-disk objects in sync and removes an unexpected failure in future dev work that expects the same behavior as the append-only on-disk objects. --- .../java/bisq/network/p2p/storage/P2PDataStorage.java | 5 ++--- .../network/p2p/storage/persistence/MapStoreService.java | 5 +++++ .../storage/persistence/ProtectedDataStoreService.java | 2 +- .../test/java/bisq/network/p2p/storage/TestState.java | 9 +-------- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 9e2f296dfb6..54f23c0993d 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -428,9 +428,8 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn // Persist ProtectedStorageEntrys carrying PersistablePayload payloads and signal listeners on changes if (protectedStoragePayload instanceof PersistablePayload) { - ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(hashOfPayload, protectedStorageEntry); - if (previous == null) - protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); + protectedDataStoreService.put(hashOfPayload, protectedStorageEntry); + protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); } return true; diff --git a/p2p/src/main/java/bisq/network/p2p/storage/persistence/MapStoreService.java b/p2p/src/main/java/bisq/network/p2p/storage/persistence/MapStoreService.java index 5449a3f3a97..67088799c4a 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/persistence/MapStoreService.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/persistence/MapStoreService.java @@ -56,6 +56,11 @@ public MapStoreService(File storageDir, Storage storage) { public abstract boolean canHandle(R payload); + void put(P2PDataStorage.ByteArray hash, R payload) { + getMap().put(hash, payload); + persist(); + } + R putIfAbsent(P2PDataStorage.ByteArray hash, R payload) { R previous = getMap().putIfAbsent(hash, payload); persist(); diff --git a/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java b/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java index 9c75e567368..1fb9f27567b 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java @@ -65,7 +65,7 @@ public void put(P2PDataStorage.ByteArray hash, ProtectedStorageEntry entry) { services.stream() .filter(service -> service.canHandle(entry)) .forEach(service -> { - service.putIfAbsent(hash, entry); + service.put(hash, entry); }); } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java index 4e4e2a6a491..fdda13a66f4 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -221,16 +221,9 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, if (expectedStateChange) { Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getMap().get(hashMapHash)); - // PersistablePayload payloads need to be written to disk and listeners signaled... unless the hash already exists in the protectedDataStore. - // Note: this behavior is different from the HashMap listeners that are signaled on an increase in seq #, even if the hash already exists. - // TODO: Should the behavior be identical between this and the HashMap listeners? - // TODO: Do we want ot overwrite stale values in order to persist updated sequence numbers and timestamps? - if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload && beforeState.protectedStorageEntryBeforeOpDataStoreMap == null) { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { Assert.assertEquals(protectedStorageEntry, this.protectedDataStoreService.getMap().get(hashMapHash)); verify(this.protectedDataStoreListener).onAdded(protectedStorageEntry); - } else { - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.protectedDataStoreService.getMap().get(hashMapHash)); - verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); } verify(this.hashMapChangedListener).onAdded(Collections.singletonList(protectedStorageEntry)); From 3503fe3b10a2fcb15af48f2f1c9989edaa73632a Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 18:35:08 -0800 Subject: [PATCH 3/4] [DEADCODE] Remove protectedDataStoreListener There were no users. --- .../network/p2p/storage/P2PDataStorage.java | 26 +++---------------- .../ProtectedDataStoreListener.java | 26 ------------------- .../bisq/network/p2p/storage/TestState.java | 25 ++++-------------- 3 files changed, 9 insertions(+), 68 deletions(-) delete mode 100644 p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreListener.java diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 54f23c0993d..c0a22c691b2 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -42,7 +42,6 @@ import bisq.network.p2p.storage.payload.RequiresOwnerIsOnlinePayload; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreService; -import bisq.network.p2p.storage.persistence.ProtectedDataStoreListener; import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; import bisq.network.p2p.storage.persistence.ResourceDataStoreService; import bisq.network.p2p.storage.persistence.SequenceNumberMap; @@ -126,7 +125,6 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers final SequenceNumberMap sequenceNumberMap = new SequenceNumberMap(); private final Set appendOnlyDataStoreListeners = new CopyOnWriteArraySet<>(); - private final Set protectedDataStoreListeners = new CopyOnWriteArraySet<>(); private final Clock clock; /// The maximum number of items that must exist in the SequenceNumberMap before it is scheduled for a purge @@ -426,11 +424,9 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn if (allowBroadcast) broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); - // Persist ProtectedStorageEntrys carrying PersistablePayload payloads and signal listeners on changes - if (protectedStoragePayload instanceof PersistablePayload) { + // Persist ProtectedStorageEntrys carrying PersistablePayload payloads + if (protectedStoragePayload instanceof PersistablePayload) protectedDataStoreService.put(hashOfPayload, protectedStorageEntry); - protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - } return true; } @@ -631,17 +627,6 @@ public void removeAppendOnlyDataStoreListener(AppendOnlyDataStoreListener listen appendOnlyDataStoreListeners.remove(listener); } - @SuppressWarnings("unused") - public void addProtectedDataStoreListener(ProtectedDataStoreListener listener) { - protectedDataStoreListeners.add(listener); - } - - @SuppressWarnings("unused") - public void removeProtectedDataStoreListener(ProtectedDataStoreListener listener) { - protectedDataStoreListeners.remove(listener); - } - - /////////////////////////////////////////////////////////////////////////////////////////// // Private /////////////////////////////////////////////////////////////////////////////////////////// @@ -667,11 +652,8 @@ private void removeFromMapAndDataStore( ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); if (protectedStoragePayload instanceof PersistablePayload) { ProtectedStorageEntry previous = protectedDataStoreService.remove(hashOfPayload, protectedStorageEntry); - if (previous != null) { - protectedDataStoreListeners.forEach(e -> e.onRemoved(protectedStorageEntry)); - } else { - log.info("We cannot remove the protectedStorageEntry from the persistedEntryMap as it does not exist."); - } + if (previous == null) + log.error("We cannot remove the protectedStorageEntry from the persistedEntryMap as it does not exist."); } }); diff --git a/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreListener.java b/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreListener.java deleted file mode 100644 index 6eb1a12cbf2..00000000000 --- a/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreListener.java +++ /dev/null @@ -1,26 +0,0 @@ -/* - * This file is part of Bisq. - * - * Bisq is free software: you can redistribute it and/or modify it - * under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at - * your option) any later version. - * - * Bisq is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public - * License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with Bisq. If not, see . - */ - -package bisq.network.p2p.storage.persistence; - -import bisq.network.p2p.storage.payload.ProtectedStorageEntry; - -public interface ProtectedDataStoreListener { - void onAdded(ProtectedStorageEntry protectedStorageEntry); - - void onRemoved(ProtectedStorageEntry protectedStorageEntry); -} diff --git a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java index fdda13a66f4..c4ceab5ca2a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -35,8 +35,6 @@ import bisq.network.p2p.storage.payload.ProtectedMailboxStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; -import bisq.network.p2p.storage.persistence.MapStoreService; -import bisq.network.p2p.storage.persistence.ProtectedDataStoreListener; import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; import bisq.network.p2p.storage.persistence.ResourceDataStoreService; import bisq.network.p2p.storage.persistence.SequenceNumberMap; @@ -71,7 +69,6 @@ public class TestState { final Broadcaster mockBroadcaster; final AppendOnlyDataStoreListener appendOnlyDataStoreListener; - private final ProtectedDataStoreListener protectedDataStoreListener; private final HashMapChangedListener hashMapChangedListener; private final Storage mockSeqNrStorage; private final ProtectedDataStoreService protectedDataStoreService; @@ -90,7 +87,6 @@ this.protectedDataStoreService, mock(ResourceDataStoreService.class), this.mockSeqNrStorage, this.clockFake, MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE); this.appendOnlyDataStoreListener = mock(AppendOnlyDataStoreListener.class); - this.protectedDataStoreListener = mock(ProtectedDataStoreListener.class); this.hashMapChangedListener = mock(HashMapChangedListener.class); this.protectedDataStoreService.addService(new MapStoreServiceFake()); @@ -100,8 +96,7 @@ this.protectedDataStoreService, mock(ResourceDataStoreService.class), this.mockSeqNrStorage, this.clockFake, this.hashMapChangedListener, - this.appendOnlyDataStoreListener, - this.protectedDataStoreListener); + this.appendOnlyDataStoreListener); } @@ -120,8 +115,7 @@ void simulateRestart() { this.mockSeqNrStorage, this.clockFake, this.hashMapChangedListener, - this.appendOnlyDataStoreListener, - this.protectedDataStoreListener); + this.appendOnlyDataStoreListener); } private static P2PDataStorage createP2PDataStorageForTest( @@ -130,8 +124,7 @@ private static P2PDataStorage createP2PDataStorageForTest( Storage sequenceNrMapStorage, ClockFake clock, HashMapChangedListener hashMapChangedListener, - AppendOnlyDataStoreListener appendOnlyDataStoreListener, - ProtectedDataStoreListener protectedDataStoreListener) { + AppendOnlyDataStoreListener appendOnlyDataStoreListener) { P2PDataStorage p2PDataStorage = new P2PDataStorage(mock(NetworkNode.class), broadcaster, @@ -145,7 +138,6 @@ protectedDataStoreService, mock(ResourceDataStoreService.class), p2PDataStorage.addHashMapChangedListener(hashMapChangedListener); p2PDataStorage.addAppendOnlyDataStoreListener(appendOnlyDataStoreListener); - p2PDataStorage.addProtectedDataStoreListener(protectedDataStoreListener); return p2PDataStorage; } @@ -153,7 +145,6 @@ protectedDataStoreService, mock(ResourceDataStoreService.class), private void resetState() { reset(this.mockBroadcaster); reset(this.appendOnlyDataStoreListener); - reset(this.protectedDataStoreListener); reset(this.hashMapChangedListener); reset(this.mockSeqNrStorage); } @@ -221,10 +212,8 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, if (expectedStateChange) { Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getMap().get(hashMapHash)); - if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) Assert.assertEquals(protectedStorageEntry, this.protectedDataStoreService.getMap().get(hashMapHash)); - verify(this.protectedDataStoreListener).onAdded(protectedStorageEntry); - } verify(this.hashMapChangedListener).onAdded(Collections.singletonList(protectedStorageEntry)); @@ -245,7 +234,6 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, // Internal state didn't change... nothing should be notified verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); - verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); } } @@ -286,7 +274,6 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, Assert.assertEquals(expected, actual); } else { verify(this.hashMapChangedListener, never()).onRemoved(any()); - verify(this.protectedDataStoreListener, never()).onAdded(any()); } if (!expectedSeqNrWrite) @@ -314,11 +301,9 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, if (expectedHashMapAndDataStoreUpdated) { Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); - if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) Assert.assertNull(this.protectedDataStoreService.getMap().get(hashMapHash)); - verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); - } } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); } From 44a11a0619ef37b069d379f108be2ce6c28ce263 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 18:35:52 -0800 Subject: [PATCH 4/4] [DEADCODE] Remove unused methods in ProtectedDataStoreService --- .../persistence/ProtectedDataStoreService.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java b/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java index 1fb9f27567b..c671ad28658 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/persistence/ProtectedDataStoreService.java @@ -69,20 +69,6 @@ public void put(P2PDataStorage.ByteArray hash, ProtectedStorageEntry entry) { }); } - public ProtectedStorageEntry putIfAbsent(P2PDataStorage.ByteArray hash, ProtectedStorageEntry entry) { - Map map = getMap(); - if (!map.containsKey(hash)) { - put(hash, entry); - return null; - } else { - return map.get(hash); - } - } - - public boolean containsKey(P2PDataStorage.ByteArray hash) { - return getMap().containsKey(hash); - } - public ProtectedStorageEntry remove(P2PDataStorage.ByteArray hash, ProtectedStorageEntry protectedStorageEntry) { final ProtectedStorageEntry[] result = new ProtectedStorageEntry[1]; services.stream()