From 617585d859558a6427ea7908265697b513734bf6 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 15 Nov 2019 09:10:21 -0800 Subject: [PATCH 01/22] [PR COMMENTS] Make maxSequenceNumberBeforePurge final Instead of using a subclass that overwrites a value, utilize Guice to inject the real value of 10000 in the app and let the tests overwrite it with their own. --- .../main/java/bisq/network/p2p/P2PModule.java | 1 + .../network/p2p/storage/P2PDataStorage.java | 11 ++++++--- .../P2PDataStorageRemoveExpiredTest.java | 2 +- .../bisq/network/p2p/storage/TestState.java | 24 ++++--------------- 4 files changed, 14 insertions(+), 24 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/P2PModule.java b/p2p/src/main/java/bisq/network/p2p/P2PModule.java index 6356515f18b..d1f3aeb22c5 100644 --- a/p2p/src/main/java/bisq/network/p2p/P2PModule.java +++ b/p2p/src/main/java/bisq/network/p2p/P2PModule.java @@ -105,5 +105,6 @@ protected void configure() { bindConstant().annotatedWith(named(NetworkOptionKeys.MSG_THROTTLE_PER_10_SEC)).to(environment.getRequiredProperty(NetworkOptionKeys.MSG_THROTTLE_PER_10_SEC)); bindConstant().annotatedWith(named(NetworkOptionKeys.SEND_MSG_THROTTLE_TRIGGER)).to(environment.getRequiredProperty(NetworkOptionKeys.SEND_MSG_THROTTLE_TRIGGER)); bindConstant().annotatedWith(named(NetworkOptionKeys.SEND_MSG_THROTTLE_SLEEP)).to(environment.getRequiredProperty(NetworkOptionKeys.SEND_MSG_THROTTLE_SLEEP)); + bindConstant().annotatedWith(named("MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE")).to(1000); } } 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 04313763d17..7ed3ee77f6f 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -63,6 +63,8 @@ import com.google.protobuf.ByteString; +import com.google.inject.name.Named; + import javax.inject.Inject; import com.google.common.annotations.VisibleForTesting; @@ -121,7 +123,9 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers private final Set protectedDataStoreListeners = new CopyOnWriteArraySet<>(); private final Clock clock; - protected int maxSequenceNumberMapSizeBeforePurge; + /// The maximum number of items that must exist in the SequenceNumberMap before it is scheduled for a purge + /// which removes entries after PURGE_AGE_DAYS. + private final int maxSequenceNumberMapSizeBeforePurge; /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -134,12 +138,14 @@ public P2PDataStorage(NetworkNode networkNode, ProtectedDataStoreService protectedDataStoreService, ResourceDataStoreService resourceDataStoreService, Storage sequenceNumberMapStorage, - Clock clock) { + Clock clock, + @Named("MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE") int maxSequenceNumberBeforePurge) { this.broadcaster = broadcaster; this.appendOnlyDataStoreService = appendOnlyDataStoreService; this.protectedDataStoreService = protectedDataStoreService; this.resourceDataStoreService = resourceDataStoreService; this.clock = clock; + this.maxSequenceNumberMapSizeBeforePurge = maxSequenceNumberBeforePurge; networkNode.addMessageListener(this); @@ -147,7 +153,6 @@ public P2PDataStorage(NetworkNode networkNode, this.sequenceNumberMapStorage = sequenceNumberMapStorage; sequenceNumberMapStorage.setNumMaxBackupFiles(5); - this.maxSequenceNumberMapSizeBeforePurge = 1000; } @Override diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java index 7814004cd2e..aec700c8ab5 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java @@ -156,7 +156,7 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, TestState.getTestNodeAddress(), null, true)); - for (int i = 0; i < 4; ++i) { + for (int i = 0; i < MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE - 1; ++i) { KeyPair ownerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayloadStub(ownerKeys.getPublic(), 0); ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); 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 b13ec2a97a4..942cd668e1c 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -63,6 +63,8 @@ * Used in the P2PDataStorage*Test(s) in order to leverage common test set up and validation. */ public class TestState { + static final int MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE = 5; + final P2PDataStorage mockedStorage; final Broadcaster mockBroadcaster; @@ -72,34 +74,16 @@ public class TestState { private final Storage mockSeqNrStorage; final ClockFake clockFake; - /** - * Subclass of P2PDataStorage that allows for easier testing, but keeps all functionality - */ - static class P2PDataStorageForTest extends P2PDataStorage { - - P2PDataStorageForTest(NetworkNode networkNode, - Broadcaster broadcaster, - AppendOnlyDataStoreService appendOnlyDataStoreService, - ProtectedDataStoreService protectedDataStoreService, - ResourceDataStoreService resourceDataStoreService, - Storage sequenceNumberMapStorage, - Clock clock) { - super(networkNode, broadcaster, appendOnlyDataStoreService, protectedDataStoreService, resourceDataStoreService, sequenceNumberMapStorage, clock); - - this.maxSequenceNumberMapSizeBeforePurge = 5; - } - } - TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); this.clockFake = new ClockFake(); - this.mockedStorage = new P2PDataStorageForTest(mock(NetworkNode.class), + this.mockedStorage = new P2PDataStorage(mock(NetworkNode.class), this.mockBroadcaster, new AppendOnlyDataStoreServiceFake(), new ProtectedDataStoreServiceFake(), mock(ResourceDataStoreService.class), - this.mockSeqNrStorage, this.clockFake); + this.mockSeqNrStorage, this.clockFake, MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE); this.appendOnlyDataStoreListener = mock(AppendOnlyDataStoreListener.class); this.protectedDataStoreListener = mock(ProtectedDataStoreListener.class); From 3bd67bab05781b7763e5d1817fdc80066c2739b6 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Mon, 18 Nov 2019 10:37:42 -0800 Subject: [PATCH 02/22] [TESTS] Clean up 'Analyze Code' warnings Remove unused imports and clean up some access modifiers now that the final test structure is complete --- .../P2PDataStoragePersistableNetworkPayloadTest.java | 1 + .../storage/P2PDataStorageProtectedStorageEntryTest.java | 1 + .../network/p2p/storage/P2PDataStoreDisconnectTest.java | 5 +---- p2p/src/test/java/bisq/network/p2p/storage/TestState.java | 6 +----- .../p2p/storage/mocks/ProtectedStoragePayloadStub.java | 2 +- 5 files changed, 5 insertions(+), 10 deletions(-) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java index c8a3d9c6134..9213bcb0fbe 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java @@ -55,6 +55,7 @@ * 2 & 3 Client API [addPersistableNetworkPayload(reBroadcast=(true && false))] * 4. onMessage() [onMessage(AddPersistableNetworkPayloadMessage)] */ +@SuppressWarnings("unused") public class P2PDataStoragePersistableNetworkPayloadTest { @RunWith(Parameterized.class) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 2aa8bc03603..82e58623ca8 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -65,6 +65,7 @@ * 1. Client API [addProtectedStorageEntry(), refreshTTL(), remove()] * 2. onMessage() [AddDataMessage, RefreshOfferMessage, RemoveDataMessage] */ +@SuppressWarnings("unused") public class P2PDataStorageProtectedStorageEntryTest { @RunWith(Parameterized.class) abstract public static class ProtectedStorageEntryTestBase { diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java index 7a54138a928..6b1ce7db935 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java @@ -39,10 +39,7 @@ import org.junit.Before; import org.junit.Test; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static bisq.network.p2p.storage.TestState.*; @@ -173,7 +170,7 @@ public void connectionClosedReduceTTLAndExpireItemsFromPeerPersistable() class ExpirablePersistentProtectedStoragePayloadStub extends ExpirableProtectedStoragePayloadStub implements PersistablePayload { - public ExpirablePersistentProtectedStoragePayloadStub(PublicKey ownerPubKey) { + private ExpirablePersistentProtectedStoragePayloadStub(PublicKey ownerPubKey) { super(ownerPubKey, 0); } } 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 942cd668e1c..a0fa8a00d8f 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -35,9 +35,7 @@ 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.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; @@ -47,8 +45,6 @@ import java.security.PublicKey; -import java.time.Clock; - import java.util.concurrent.TimeUnit; import org.junit.Assert; @@ -70,7 +66,7 @@ public class TestState { final AppendOnlyDataStoreListener appendOnlyDataStoreListener; private final ProtectedDataStoreListener protectedDataStoreListener; - final HashMapChangedListener hashMapChangedListener; + private final HashMapChangedListener hashMapChangedListener; private final Storage mockSeqNrStorage; final ClockFake clockFake; diff --git a/p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java b/p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java index d6be958138f..5d9a9d3784f 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java @@ -45,7 +45,7 @@ public class ProtectedStoragePayloadStub implements ProtectedStoragePayload { @Getter private PublicKey ownerPubKey; - protected Message messageMock; + protected final Message messageMock; public ProtectedStoragePayloadStub(PublicKey ownerPubKey) { this.ownerPubKey = ownerPubKey; From b281566e1492b5c0a54bf71963f4b70b21b5e172 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 11:15:35 -0800 Subject: [PATCH 03/22] [REFACTOR] HashMapListener::onAdded/onRemoved Previously, this interface was called each time an item was changed. This required listeners to understand performance implications of multiple adds or removes in a short time span. Instead, give each listener the ability to process a list of added or removed entrys which can help them avoid performance issues. This patch is just a refactor. Each listener is called once for each ProtectedStorageEntry. Future patches will change this. --- .../java/bisq/core/alert/AlertManager.java | 32 +++++++++------- .../proposal/ProposalListPresentation.java | 21 +++++++---- .../governance/proposal/ProposalService.java | 13 +++++-- .../java/bisq/core/filter/FilterManager.java | 31 +++++++++------- .../bisq/core/offer/OfferBookService.java | 37 +++++++++++-------- .../dispute/agent/DisputeAgentManager.java | 23 +++++++----- .../java/bisq/network/p2p/P2PService.java | 13 ++++--- .../p2p/storage/HashMapChangedListener.java | 6 ++- .../network/p2p/storage/P2PDataStorage.java | 5 ++- .../bisq/network/p2p/storage/TestState.java | 9 +++-- 10 files changed, 113 insertions(+), 77 deletions(-) diff --git a/core/src/main/java/bisq/core/alert/AlertManager.java b/core/src/main/java/bisq/core/alert/AlertManager.java index 05a3872a7ff..0fba35a283f 100644 --- a/core/src/main/java/bisq/core/alert/AlertManager.java +++ b/core/src/main/java/bisq/core/alert/AlertManager.java @@ -44,6 +44,8 @@ import java.math.BigInteger; +import java.util.Collection; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -79,22 +81,26 @@ public AlertManager(P2PService p2PService, if (!ignoreDevMsg) { p2PService.addHashSetChangedListener(new HashMapChangedListener() { @Override - public void onAdded(ProtectedStorageEntry data) { - final ProtectedStoragePayload protectedStoragePayload = data.getProtectedStoragePayload(); - if (protectedStoragePayload instanceof Alert) { - Alert alert = (Alert) protectedStoragePayload; - if (verifySignature(alert)) - alertMessageProperty.set(alert); - } + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); + if (protectedStoragePayload instanceof Alert) { + Alert alert = (Alert) protectedStoragePayload; + if (verifySignature(alert)) + alertMessageProperty.set(alert); + } + }); } @Override - public void onRemoved(ProtectedStorageEntry data) { - final ProtectedStoragePayload protectedStoragePayload = data.getProtectedStoragePayload(); - if (protectedStoragePayload instanceof Alert) { - if (verifySignature((Alert) protectedStoragePayload)) - alertMessageProperty.set(null); - } + public void onRemoved(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); + if (protectedStoragePayload instanceof Alert) { + if (verifySignature((Alert) protectedStoragePayload)) + alertMessageProperty.set(null); + } + }); } }); } diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java index 9be6942b6c9..1d75c5519fa 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java @@ -41,6 +41,7 @@ import javafx.collections.ObservableList; import javafx.collections.transformation.FilteredList; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -130,17 +131,21 @@ public void onParseBlockCompleteAfterBatchProcessing(Block block) { /////////////////////////////////////////////////////////////////////////////////////////// @Override - public void onAdded(ProtectedStorageEntry entry) { - if (entry.getProtectedStoragePayload() instanceof TempProposalPayload) { - tempProposalsChanged = true; - } + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof TempProposalPayload) { + tempProposalsChanged = true; + } + }); } @Override - public void onRemoved(ProtectedStorageEntry entry) { - if (entry.getProtectedStoragePayload() instanceof TempProposalPayload) { - tempProposalsChanged = true; - } + public void onRemoved(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof TempProposalPayload) { + tempProposalsChanged = true; + } + }); } @Override diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java index 6da0d387516..594ce6c10af 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java @@ -50,6 +50,7 @@ import javafx.collections.FXCollections; import javafx.collections.ObservableList; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -133,13 +134,17 @@ public void start() { /////////////////////////////////////////////////////////////////////////////////////////// @Override - public void onAdded(ProtectedStorageEntry entry) { - onProtectedDataAdded(entry, true); + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + onProtectedDataAdded(protectedStorageEntry, true); + }); } @Override - public void onRemoved(ProtectedStorageEntry entry) { - onProtectedDataRemoved(entry); + public void onRemoved(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + onProtectedDataRemoved(protectedStorageEntry); + }); } diff --git a/core/src/main/java/bisq/core/filter/FilterManager.java b/core/src/main/java/bisq/core/filter/FilterManager.java index 86687d92c37..c32c27b07b7 100644 --- a/core/src/main/java/bisq/core/filter/FilterManager.java +++ b/core/src/main/java/bisq/core/filter/FilterManager.java @@ -52,6 +52,7 @@ import java.math.BigInteger; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.concurrent.CopyOnWriteArrayList; @@ -133,23 +134,27 @@ public void onAllServicesInitialized() { p2PService.addHashSetChangedListener(new HashMapChangedListener() { @Override - public void onAdded(ProtectedStorageEntry data) { - if (data.getProtectedStoragePayload() instanceof Filter) { - Filter filter = (Filter) data.getProtectedStoragePayload(); - boolean wasValid = addFilter(filter); - if (!wasValid) { - UserThread.runAfter(() -> p2PService.getP2PDataStorage().removeInvalidProtectedStorageEntry(data), 1); + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof Filter) { + Filter filter = (Filter) protectedStorageEntry.getProtectedStoragePayload(); + boolean wasValid = addFilter(filter); + if (!wasValid) { + UserThread.runAfter(() -> p2PService.getP2PDataStorage().removeInvalidProtectedStorageEntry(protectedStorageEntry), 1); + } } - } + }); } @Override - public void onRemoved(ProtectedStorageEntry data) { - if (data.getProtectedStoragePayload() instanceof Filter) { - Filter filter = (Filter) data.getProtectedStoragePayload(); - if (verifySignature(filter)) - resetFilters(); - } + public void onRemoved(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof Filter) { + Filter filter = (Filter) protectedStorageEntry.getProtectedStoragePayload(); + if (verifySignature(filter)) + resetFilters(); + } + }); } }); } diff --git a/core/src/main/java/bisq/core/offer/OfferBookService.java b/core/src/main/java/bisq/core/offer/OfferBookService.java index 5ebe152453c..e9b36e89219 100644 --- a/core/src/main/java/bisq/core/offer/OfferBookService.java +++ b/core/src/main/java/bisq/core/offer/OfferBookService.java @@ -40,6 +40,7 @@ import java.io.File; +import java.util.Collection; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -87,26 +88,30 @@ public OfferBookService(P2PService p2PService, p2PService.addHashSetChangedListener(new HashMapChangedListener() { @Override - public void onAdded(ProtectedStorageEntry data) { - offerBookChangedListeners.stream().forEach(listener -> { - if (data.getProtectedStoragePayload() instanceof OfferPayload) { - OfferPayload offerPayload = (OfferPayload) data.getProtectedStoragePayload(); - Offer offer = new Offer(offerPayload); - offer.setPriceFeedService(priceFeedService); - listener.onAdded(offer); - } + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + offerBookChangedListeners.stream().forEach(listener -> { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof OfferPayload) { + OfferPayload offerPayload = (OfferPayload) protectedStorageEntry.getProtectedStoragePayload(); + Offer offer = new Offer(offerPayload); + offer.setPriceFeedService(priceFeedService); + listener.onAdded(offer); + } + }); }); } @Override - public void onRemoved(ProtectedStorageEntry data) { - offerBookChangedListeners.stream().forEach(listener -> { - if (data.getProtectedStoragePayload() instanceof OfferPayload) { - OfferPayload offerPayload = (OfferPayload) data.getProtectedStoragePayload(); - Offer offer = new Offer(offerPayload); - offer.setPriceFeedService(priceFeedService); - listener.onRemoved(offer); - } + public void onRemoved(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + offerBookChangedListeners.stream().forEach(listener -> { + if (protectedStorageEntry.getProtectedStoragePayload() instanceof OfferPayload) { + OfferPayload offerPayload = (OfferPayload) protectedStorageEntry.getProtectedStoragePayload(); + Offer offer = new Offer(offerPayload); + offer.setPriceFeedService(priceFeedService); + listener.onRemoved(offer); + } + }); }); } }); diff --git a/core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java b/core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java index 16cc1b38e87..ac6480bb1b2 100644 --- a/core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java +++ b/core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java @@ -46,6 +46,7 @@ import java.math.BigInteger; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; @@ -131,18 +132,22 @@ public DisputeAgentManager(KeyRing keyRing, public void onAllServicesInitialized() { disputeAgentService.addHashSetChangedListener(new HashMapChangedListener() { @Override - public void onAdded(ProtectedStorageEntry data) { - if (isExpectedInstance(data)) { - updateMap(); - } + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (isExpectedInstance(protectedStorageEntry)) { + updateMap(); + } + }); } @Override - public void onRemoved(ProtectedStorageEntry data) { - if (isExpectedInstance(data)) { - updateMap(); - removeAcceptedDisputeAgentFromUser(data); - } + public void onRemoved(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (isExpectedInstance(protectedStorageEntry)) { + updateMap(); + removeAcceptedDisputeAgentFromUser(protectedStorageEntry); + } + }); } }); diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/p2p/src/main/java/bisq/network/p2p/P2PService.java index d90fdeb2565..733cc385cce 100644 --- a/p2p/src/main/java/bisq/network/p2p/P2PService.java +++ b/p2p/src/main/java/bisq/network/p2p/P2PService.java @@ -75,6 +75,7 @@ import java.security.PublicKey; import java.util.Arrays; +import java.util.Collection; import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -432,15 +433,15 @@ public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) { /////////////////////////////////////////////////////////////////////////////////////////// @Override - public void onAdded(ProtectedStorageEntry protectedStorageEntry) { - if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) - processMailboxEntry((ProtectedMailboxStorageEntry) protectedStorageEntry); + public void onAdded(Collection protectedStorageEntries) { + protectedStorageEntries.forEach(protectedStorageEntry -> { + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) + processMailboxEntry((ProtectedMailboxStorageEntry) protectedStorageEntry); + }); } @Override - public void onRemoved(ProtectedStorageEntry data) { - } - + public void onRemoved(Collection protectedStorageEntries) { } /////////////////////////////////////////////////////////////////////////////////////////// // DirectMessages diff --git a/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java b/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java index a3ac1c20258..8bdcb28715a 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java @@ -19,11 +19,13 @@ import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import java.util.Collection; + public interface HashMapChangedListener { - void onAdded(ProtectedStorageEntry data); + void onAdded(Collection protectedStorageEntries); @SuppressWarnings("UnusedParameters") - void onRemoved(ProtectedStorageEntry data); + void onRemoved(Collection protectedStorageEntries); // We process all expired entries after a delay (60 s) after onBootstrapComplete. // We notify listeners of start and completion so they can optimize to only update after batch processing is done. 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 7ed3ee77f6f..513abca0585 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -75,6 +75,7 @@ import java.time.Clock; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.Date; import java.util.HashMap; @@ -409,7 +410,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn // This is an updated entry. Record it and signal listeners. map.put(hashOfPayload, protectedStorageEntry); - hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); + hashMapChangedListeners.forEach(e -> e.onAdded(Collections.singletonList(protectedStorageEntry))); // Record the updated sequence number and persist it. Higher delay so we can batch more items. sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); @@ -643,7 +644,7 @@ public void removeProtectedDataStoreListener(ProtectedDataStoreListener listener private void removeFromMapAndDataStore(ProtectedStorageEntry protectedStorageEntry, ByteArray hashOfPayload) { map.remove(hashOfPayload); - hashMapChangedListeners.forEach(e -> e.onRemoved(protectedStorageEntry)); + hashMapChangedListeners.forEach(e -> e.onRemoved(Collections.singletonList(protectedStorageEntry))); ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); if (protectedStoragePayload instanceof PersistablePayload) { 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 a0fa8a00d8f..594e00fb5f1 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -45,6 +45,7 @@ import java.security.PublicKey; +import java.util.Collections; import java.util.concurrent.TimeUnit; import org.junit.Assert; @@ -174,7 +175,7 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); } - verify(this.hashMapChangedListener).onAdded(protectedStorageEntry); + verify(this.hashMapChangedListener).onAdded(Collections.singletonList(protectedStorageEntry)); final ArgumentCaptor captor = ArgumentCaptor.forClass(BroadcastMessage.class); verify(this.mockBroadcaster).broadcast(captor.capture(), any(NodeAddress.class), @@ -192,7 +193,7 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); // Internal state didn't change... nothing should be notified - verify(this.hashMapChangedListener, never()).onAdded(protectedStorageEntry); + verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); } @@ -216,7 +217,7 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); } - verify(this.hashMapChangedListener).onRemoved(protectedStorageEntry); + verify(this.hashMapChangedListener).onRemoved(Collections.singletonList(protectedStorageEntry)); if (expectedSeqNrWriteOnStateChange) this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); @@ -232,7 +233,7 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); - verify(this.hashMapChangedListener, never()).onAdded(protectedStorageEntry); + verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); } From 4f08588717cd6fce46dbf31e03404c6af8e05ae3 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 11:29:28 -0800 Subject: [PATCH 04/22] [REFACTOR] removeFromMapAndDataStore can operate on Collections Minor performance overhead for constructing MapEntry and Collections of one element, but keeps the code cleaner and all removes can still use the same logic to remove from map, delete from data store, signal listeners, etc. The MapEntry type is used instead of Pair since it will require less operations when this is eventually used in the removeExpiredEntries path. --- .../network/p2p/storage/P2PDataStorage.java | 35 +++++++++++++------ 1 file changed, 24 insertions(+), 11 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 513abca0585..6c0992c6d73 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -68,6 +68,8 @@ import javax.inject.Inject; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Maps; + import java.security.KeyPair; import java.security.PublicKey; @@ -75,6 +77,7 @@ import java.time.Clock; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.Date; @@ -643,19 +646,29 @@ public void removeProtectedDataStoreListener(ProtectedDataStoreListener listener /////////////////////////////////////////////////////////////////////////////////////////// private void removeFromMapAndDataStore(ProtectedStorageEntry protectedStorageEntry, ByteArray hashOfPayload) { - map.remove(hashOfPayload); - hashMapChangedListeners.forEach(e -> e.onRemoved(Collections.singletonList(protectedStorageEntry))); + removeFromMapAndDataStore(Collections.singletonList(Maps.immutableEntry(hashOfPayload, protectedStorageEntry))); + } - ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); - if (protectedStoragePayload instanceof PersistablePayload) { - ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload); - ProtectedStorageEntry previous = protectedDataStoreService.remove(compactHash, 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."); + private void removeFromMapAndDataStore( + Collection> entriesToRemoveWithPayloadHash) { + entriesToRemoveWithPayloadHash.forEach(entryToRemoveWithPayloadHash -> { + ByteArray hashOfPayload = entryToRemoveWithPayloadHash.getKey(); + ProtectedStorageEntry protectedStorageEntry = entryToRemoveWithPayloadHash.getValue(); + + map.remove(hashOfPayload); + hashMapChangedListeners.forEach(e -> e.onRemoved(Collections.singletonList(protectedStorageEntry))); + + ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); + if (protectedStoragePayload instanceof PersistablePayload) { + ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload); + ProtectedStorageEntry previous = protectedDataStoreService.remove(compactHash, 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."); + } } - } + }); } private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) { From 489b25aa13f62ab71dfb306cbb6dc955e33374bb Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 11:42:33 -0800 Subject: [PATCH 05/22] Change removeFromMapAndDataStore to signal listeners at the end in a batch All current users still call this one-at-a-time. But, it gives the ability for the expire code path to remove in a batch. --- .../java/bisq/network/p2p/storage/P2PDataStorage.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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 6c0992c6d73..c08cde13d1e 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -651,12 +651,17 @@ private void removeFromMapAndDataStore(ProtectedStorageEntry protectedStorageEnt private void removeFromMapAndDataStore( Collection> entriesToRemoveWithPayloadHash) { + + if (entriesToRemoveWithPayloadHash.isEmpty()) + return; + + ArrayList entriesForSignal = new ArrayList<>(entriesToRemoveWithPayloadHash.size()); entriesToRemoveWithPayloadHash.forEach(entryToRemoveWithPayloadHash -> { ByteArray hashOfPayload = entryToRemoveWithPayloadHash.getKey(); ProtectedStorageEntry protectedStorageEntry = entryToRemoveWithPayloadHash.getValue(); map.remove(hashOfPayload); - hashMapChangedListeners.forEach(e -> e.onRemoved(Collections.singletonList(protectedStorageEntry))); + entriesForSignal.add(protectedStorageEntry); ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); if (protectedStoragePayload instanceof PersistablePayload) { @@ -669,6 +674,8 @@ private void removeFromMapAndDataStore( } } }); + + hashMapChangedListeners.forEach(e -> e.onRemoved(entriesForSignal)); } private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) { From eae641ee73f47922d1d02e52413db4f45e3785ea Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 11:50:10 -0800 Subject: [PATCH 06/22] Update removeExpiredEntries to remove all items in a batch This will cause HashMapChangedListeners to receive just one onRemoved() call for the expire work instead of multiple onRemoved() calls for each item. This required a bit of updating for the remove validation in tests so that it correctly compares onRemoved with multiple items. --- .../network/p2p/storage/P2PDataStorage.java | 10 +-- .../P2PDataStorageRemoveExpiredTest.java | 12 ++- .../bisq/network/p2p/storage/TestState.java | 79 +++++++++++++------ 3 files changed, 69 insertions(+), 32 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 c08cde13d1e..c676297519b 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -203,13 +203,11 @@ void removeExpiredEntries() { // Batch processing can cause performance issues, so we give listeners a chance to deal with it by notifying // about start and end of iteration. hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataStarted); - toRemoveList.forEach(mapEntry -> { - ProtectedStorageEntry protectedStorageEntry = mapEntry.getValue(); - ByteArray payloadHash = mapEntry.getKey(); - - log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(protectedStorageEntry)); - removeFromMapAndDataStore(protectedStorageEntry, payloadHash); + toRemoveList.forEach(toRemoveItem -> { + log.debug("We found an expired data entry. We remove the protectedData:\n\t" + + Utilities.toTruncatedString(toRemoveItem.getValue())); }); + removeFromMapAndDataStore(toRemoveList); hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); if (sequenceNumberMap.size() > this.maxSequenceNumberMapSizeBeforePurge) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java index aec700c8ab5..1a30e2cc789 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java @@ -32,6 +32,7 @@ import java.security.KeyPair; import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; import java.util.concurrent.TimeUnit; import org.junit.Assert; @@ -149,10 +150,13 @@ public void removeExpiredEntries_ExpiresExpiredPersistableExpirableEntries() thr public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchAlgorithmException { final int initialClockIncrement = 5; + ArrayList expectedRemoves = new ArrayList<>(); + // Add 4 entries to our sequence number map that will be purged KeyPair purgedOwnerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload purgedProtectedStoragePayload = new PersistableExpirableProtectedStoragePayloadStub(purgedOwnerKeys.getPublic(), 0); ProtectedStorageEntry purgedProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(purgedProtectedStoragePayload, purgedOwnerKeys); + expectedRemoves.add(purgedProtectedStorageEntry); Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, TestState.getTestNodeAddress(), null, true)); @@ -160,6 +164,7 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA KeyPair ownerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayloadStub(ownerKeys.getPublic(), 0); ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + expectedRemoves.add(tmpEntry); Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(tmpEntry, TestState.getTestNodeAddress(), null, true)); } @@ -171,6 +176,7 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA KeyPair keepOwnerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload keepProtectedStoragePayload = new PersistableExpirableProtectedStoragePayloadStub(keepOwnerKeys.getPublic(), 0); ProtectedStorageEntry keepProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(keepProtectedStoragePayload, keepOwnerKeys); + expectedRemoves.add(keepProtectedStorageEntry); Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, TestState.getTestNodeAddress(), null, true)); @@ -178,17 +184,17 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA // Advance time past it so they will be valid purge targets this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(P2PDataStorage.PURGE_AGE_DAYS + 1 - initialClockIncrement)); - // The first entry (11 days old) should be purged + // The first 4 entries (11 days old) should be purged from the SequenceNumberMap SavedTestState beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, purgedProtectedStorageEntry, true, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, expectedRemoves, true, false, false, false); // Which means that an addition of a purged entry should succeed. beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, TestState.getTestNodeAddress(), null, false)); this.testState.verifyProtectedStorageAdd(beforeState, purgedProtectedStorageEntry, true, false); - // The second entry (5 days old) should still exist which means trying to add it again should fail. + // The last entry (5 days old) should still exist in the SequenceNumberMap which means trying to add it again should fail. beforeState = this.testState.saveTestState(keepProtectedStorageEntry); Assert.assertFalse(this.testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, TestState.getTestNodeAddress(), null, false)); this.testState.verifyProtectedStorageAdd(beforeState, keepProtectedStorageEntry, false, false); 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 594e00fb5f1..8e74b5845f8 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -45,7 +45,10 @@ import java.security.PublicKey; +import java.util.Collection; import java.util.Collections; +import java.util.HashSet; +import java.util.Set; import java.util.concurrent.TimeUnit; import org.junit.Assert; @@ -205,38 +208,68 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, boolean expectedBroadcastOnStateChange, boolean expectedSeqNrWriteOnStateChange, boolean expectedIsDataOwner) { - P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); + verifyProtectedStorageRemove(beforeState, Collections.singletonList(protectedStorageEntry), + expectedStateChange, expectedBroadcastOnStateChange, + expectedSeqNrWriteOnStateChange, expectedIsDataOwner); + } + + void verifyProtectedStorageRemove(SavedTestState beforeState, + Collection protectedStorageEntries, + boolean expectedStateChange, + boolean expectedBroadcastOnStateChange, + boolean expectedSeqNrWriteOnStateChange, + boolean expectedIsDataOwner) { + + // The default matcher expects orders to stay the same. So, create a custom matcher function since + // we don't care about the order. if (expectedStateChange) { - Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); + final ArgumentCaptor> argument = ArgumentCaptor.forClass(Collection.class); + verify(this.hashMapChangedListener).onRemoved(argument.capture()); - if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { - Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(storageHash)); + Set actual = new HashSet<>(argument.getValue()); + Set expected = new HashSet<>(protectedStorageEntries); - verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); - } + // Ensure we didn't remove duplicates + Assert.assertEquals(protectedStorageEntries.size(), expected.size()); + Assert.assertEquals(argument.getValue().size(), actual.size()); + Assert.assertEquals(expected, actual); + } else { + verify(this.hashMapChangedListener, never()).onRemoved(any()); + } - verify(this.hashMapChangedListener).onRemoved(Collections.singletonList(protectedStorageEntry)); + protectedStorageEntries.forEach(protectedStorageEntry -> { + P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); + P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - if (expectedSeqNrWriteOnStateChange) - this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); + if (expectedStateChange) { + Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); - if (expectedBroadcastOnStateChange) { - if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) - verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); - else - verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); - } + if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { + Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(storageHash)); - } else { - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); + verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); + } - verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); - verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); - verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); - verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); - } + if (expectedSeqNrWriteOnStateChange) + this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); + + if (expectedBroadcastOnStateChange) { + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) + verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + else + verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + } + + } else { + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); + + verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); + verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); + verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); + verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); + } + }); } void verifyRefreshTTL(SavedTestState beforeState, From a50e59f7ebaf4452aacc5590a1e9acf1d762f397 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 14:30:46 -0800 Subject: [PATCH 07/22] ProposalService::onProtectedDataRemoved signals listeners once on batch removes #3143 identified an issue that tempProposals listeners were being signaled once for each item that was removed during the P2PDataStore operation that expired old TempProposal objects. Some of the listeners are very expensive (ProposalListPresentation::updateLists()) which results in large UI performance issues. Now that the infrastructure is in place to receive updates from the P2PDataStore in a batch, the ProposalService can apply all of the removes received from the P2PDataStore at once. This results in only 1 onChanged() callback for each listener. The end result is that updateLists() is only called once and the performance problems are reduced. This removes the need for #3148 and those interfaces will be removed in the next patch. --- .../governance/proposal/ProposalService.java | 58 ++++---- ...osalServiceP2PDataStorageListenerTest.java | 127 ++++++++++++++++++ 2 files changed, 160 insertions(+), 25 deletions(-) create mode 100644 core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java index 594ce6c10af..7953d403b66 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java @@ -50,6 +50,7 @@ import javafx.collections.FXCollections; import javafx.collections.ObservableList; +import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Optional; @@ -142,9 +143,7 @@ public void onAdded(Collection protectedStorageEntries) { @Override public void onRemoved(Collection protectedStorageEntries) { - protectedStorageEntries.forEach(protectedStorageEntry -> { - onProtectedDataRemoved(protectedStorageEntry); - }); + onProtectedDataRemoved(protectedStorageEntries); } @@ -271,30 +270,39 @@ private void onProtectedDataAdded(ProtectedStorageEntry entry, boolean fromBroad } } - private void onProtectedDataRemoved(ProtectedStorageEntry entry) { - ProtectedStoragePayload protectedStoragePayload = entry.getProtectedStoragePayload(); - if (protectedStoragePayload instanceof TempProposalPayload) { - Proposal proposal = ((TempProposalPayload) protectedStoragePayload).getProposal(); - // We allow removal only if we are in the proposal phase. - boolean inPhase = periodService.isInPhase(daoStateService.getChainHeight(), DaoPhase.Phase.PROPOSAL); - boolean txInPastCycle = periodService.isTxInPastCycle(proposal.getTxId(), daoStateService.getChainHeight()); - Optional tx = daoStateService.getTx(proposal.getTxId()); - boolean unconfirmedOrNonBsqTx = !tx.isPresent(); - // if the tx is unconfirmed we need to be in the PROPOSAL phase, otherwise the tx must be confirmed. - if (inPhase || txInPastCycle || unconfirmedOrNonBsqTx) { - if (tempProposals.contains(proposal)) { - tempProposals.remove(proposal); - log.debug("We received a remove request for a TempProposalPayload and have removed the proposal " + - "from our list. proposal creation date={}, proposalTxId={}, inPhase={}, " + - "txInPastCycle={}, unconfirmedOrNonBsqTx={}", - proposal.getCreationDateAsDate(), proposal.getTxId(), inPhase, txInPastCycle, unconfirmedOrNonBsqTx); + private void onProtectedDataRemoved(Collection protectedStorageEntries) { + + // The listeners of tmpProposals can do large amounts of work that cause performance issues. Apply all of the + // updates at once using retainAll which will cause all listeners to be updated only once. + ArrayList tempProposalsWithUpdates = new ArrayList<>(tempProposals); + + protectedStorageEntries.forEach(protectedStorageEntry -> { + ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); + if (protectedStoragePayload instanceof TempProposalPayload) { + Proposal proposal = ((TempProposalPayload) protectedStoragePayload).getProposal(); + // We allow removal only if we are in the proposal phase. + boolean inPhase = periodService.isInPhase(daoStateService.getChainHeight(), DaoPhase.Phase.PROPOSAL); + boolean txInPastCycle = periodService.isTxInPastCycle(proposal.getTxId(), daoStateService.getChainHeight()); + Optional tx = daoStateService.getTx(proposal.getTxId()); + boolean unconfirmedOrNonBsqTx = !tx.isPresent(); + // if the tx is unconfirmed we need to be in the PROPOSAL phase, otherwise the tx must be confirmed. + if (inPhase || txInPastCycle || unconfirmedOrNonBsqTx) { + if (tempProposalsWithUpdates.contains(proposal)) { + tempProposalsWithUpdates.remove(proposal); + log.debug("We received a remove request for a TempProposalPayload and have removed the proposal " + + "from our list. proposal creation date={}, proposalTxId={}, inPhase={}, " + + "txInPastCycle={}, unconfirmedOrNonBsqTx={}", + proposal.getCreationDateAsDate(), proposal.getTxId(), inPhase, txInPastCycle, unconfirmedOrNonBsqTx); + } + } else { + log.warn("We received a remove request outside the PROPOSAL phase. " + + "Proposal creation date={}, proposal.txId={}, current blockHeight={}", + proposal.getCreationDateAsDate(), proposal.getTxId(), daoStateService.getChainHeight()); } - } else { - log.warn("We received a remove request outside the PROPOSAL phase. " + - "Proposal creation date={}, proposal.txId={}, current blockHeight={}", - proposal.getCreationDateAsDate(), proposal.getTxId(), daoStateService.getChainHeight()); } - } + }); + + tempProposals.retainAll(tempProposalsWithUpdates); } private void onAppendOnlyDataAdded(PersistableNetworkPayload persistableNetworkPayload, boolean fromBroadcastMessage) { diff --git a/core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java b/core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java new file mode 100644 index 00000000000..7945a241b28 --- /dev/null +++ b/core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java @@ -0,0 +1,127 @@ +/* + * 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.core.dao.governance.proposal; + +import bisq.core.dao.governance.period.PeriodService; +import bisq.core.dao.governance.proposal.storage.appendonly.ProposalStorageService; +import bisq.core.dao.governance.proposal.storage.temp.TempProposalPayload; +import bisq.core.dao.governance.proposal.storage.temp.TempProposalStorageService; +import bisq.core.dao.state.DaoStateService; +import bisq.core.dao.state.model.governance.DaoPhase; +import bisq.core.dao.state.model.governance.Proposal; + +import bisq.network.p2p.P2PService; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreService; +import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; + +import javafx.collections.ListChangeListener; + +import java.util.Arrays; +import java.util.Collections; + +import org.junit.Before; +import org.junit.Test; + +import static org.mockito.Mockito.*; + +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + + +/** + * Tests of the P2PDataStorage::onRemoved callback behavior to ensure that the proper number of signal events occur. + */ +public class ProposalServiceP2PDataStorageListenerTest { + private ProposalService proposalService; + + @Mock + private PeriodService periodService; + + @Mock + private DaoStateService daoStateService; + + @Mock + private ListChangeListener tempProposalListener; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + + this.proposalService = new ProposalService( + mock(P2PService.class), + this.periodService, + mock(ProposalStorageService.class), + mock(TempProposalStorageService.class), + mock(AppendOnlyDataStoreService.class), + mock(ProtectedDataStoreService.class), + this.daoStateService, + mock(ProposalValidatorProvider.class), + true); + + // Create a state so that all added/removed Proposals will actually update the tempProposals list. + when(this.periodService.isInPhase(anyInt(), any(DaoPhase.Phase.class))).thenReturn(true); + when(this.daoStateService.isParseBlockChainComplete()).thenReturn(false); + } + + private static ProtectedStorageEntry buildProtectedStorageEntry() { + ProtectedStorageEntry protectedStorageEntry = mock(ProtectedStorageEntry.class); + TempProposalPayload tempProposalPayload = mock(TempProposalPayload.class); + Proposal tempProposal = mock(Proposal.class); + when(protectedStorageEntry.getProtectedStoragePayload()).thenReturn(tempProposalPayload); + when(tempProposalPayload.getProposal()).thenReturn(tempProposal); + + return protectedStorageEntry; + } + + // TESTCASE: If an onRemoved callback is called which does not remove anything the tempProposals listeners + // are not signaled. + @Test + public void onRemoved_noSignalIfNoChange() { + this.proposalService.onRemoved(Collections.singletonList(mock(ProtectedStorageEntry.class))); + + verify(this.tempProposalListener, never()).onChanged(any()); + } + + // TESTCASE: If an onRemoved callback is called with 1 element AND it creates a remove of 1 element, the tempProposal + // listeners are signaled once. + @Test + public void onRemoved_signalOnceOnOneChange() { + ProtectedStorageEntry one = buildProtectedStorageEntry(); + this.proposalService.onAdded(Collections.singletonList(one)); + this.proposalService.getTempProposals().addListener(this.tempProposalListener); + + this.proposalService.onRemoved(Collections.singletonList(one)); + + verify(this.tempProposalListener).onChanged(any()); + } + + // TESTCASE: If an onRemoved callback is called with 2 elements AND it creates a remove of 2 elements, the + // tempProposal listeners are signaled once. + @Test + public void onRemoved_signalOnceOnMultipleChanges() { + ProtectedStorageEntry one = buildProtectedStorageEntry(); + ProtectedStorageEntry two = buildProtectedStorageEntry(); + this.proposalService.onAdded(Arrays.asList(one, two)); + this.proposalService.getTempProposals().addListener(this.tempProposalListener); + + this.proposalService.onRemoved(Arrays.asList(one, two)); + + verify(this.tempProposalListener).onChanged(any()); + } +} From a8139f3a04e72d1a0f009981086be69cbd016f42 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 16 Nov 2019 14:33:42 -0800 Subject: [PATCH 08/22] Remove HashmapChangedListener::onBatch operations Now that the only user of this interface has been removed, go ahead and delete it. This is a partial revert of f5d75c4f6085e17f512a73aff3f6d16f6dad5cc3 that includes the code that was added into ProposalService that subscribed to the P2PDataStore. --- .../proposal/ProposalListPresentation.java | 54 +------------------ .../p2p/storage/HashMapChangedListener.java | 8 --- .../network/p2p/storage/P2PDataStorage.java | 6 +-- 3 files changed, 3 insertions(+), 65 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java index 1d75c5519fa..49aa219281c 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java @@ -20,16 +20,11 @@ import bisq.core.btc.wallet.BsqWalletService; import bisq.core.dao.DaoSetupService; import bisq.core.dao.governance.proposal.storage.appendonly.ProposalPayload; -import bisq.core.dao.governance.proposal.storage.temp.TempProposalPayload; import bisq.core.dao.state.DaoStateListener; import bisq.core.dao.state.DaoStateService; import bisq.core.dao.state.model.blockchain.Block; import bisq.core.dao.state.model.governance.Proposal; -import bisq.network.p2p.storage.HashMapChangedListener; -import bisq.network.p2p.storage.P2PDataStorage; -import bisq.network.p2p.storage.payload.ProtectedStorageEntry; - import bisq.common.UserThread; import org.bitcoinj.core.TransactionConfidence; @@ -41,7 +36,6 @@ import javafx.collections.ObservableList; import javafx.collections.transformation.FilteredList; -import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -56,8 +50,7 @@ * our own proposal that is not critical). Foreign proposals are only shown if confirmed and fully validated. */ @Slf4j -public class ProposalListPresentation implements DaoStateListener, HashMapChangedListener, - MyProposalListService.Listener, DaoSetupService { +public class ProposalListPresentation implements DaoStateListener, MyProposalListService.Listener, DaoSetupService { private final ProposalService proposalService; private final DaoStateService daoStateService; private final MyProposalListService myProposalListService; @@ -67,7 +60,6 @@ public class ProposalListPresentation implements DaoStateListener, HashMapChange @Getter private final FilteredList activeOrMyUnconfirmedProposals = new FilteredList<>(allProposals); private final ListChangeListener proposalListChangeListener; - private boolean tempProposalsChanged; /////////////////////////////////////////////////////////////////////////////////////////// @@ -77,7 +69,6 @@ public class ProposalListPresentation implements DaoStateListener, HashMapChange @Inject public ProposalListPresentation(ProposalService proposalService, DaoStateService daoStateService, - P2PDataStorage p2PDataStorage, MyProposalListService myProposalListService, BsqWalletService bsqWalletService, ProposalValidatorProvider validatorProvider) { @@ -88,7 +79,6 @@ public ProposalListPresentation(ProposalService proposalService, this.validatorProvider = validatorProvider; daoStateService.addDaoStateListener(this); - p2PDataStorage.addHashMapChangedListener(this); myProposalListService.addListener(this); proposalListChangeListener = c -> updateLists(); @@ -125,48 +115,6 @@ public void onParseBlockCompleteAfterBatchProcessing(Block block) { updateLists(); } - - /////////////////////////////////////////////////////////////////////////////////////////// - // HashMapChangedListener - /////////////////////////////////////////////////////////////////////////////////////////// - - @Override - public void onAdded(Collection protectedStorageEntries) { - protectedStorageEntries.forEach(protectedStorageEntry -> { - if (protectedStorageEntry.getProtectedStoragePayload() instanceof TempProposalPayload) { - tempProposalsChanged = true; - } - }); - } - - @Override - public void onRemoved(Collection protectedStorageEntries) { - protectedStorageEntries.forEach(protectedStorageEntry -> { - if (protectedStorageEntry.getProtectedStoragePayload() instanceof TempProposalPayload) { - tempProposalsChanged = true; - } - }); - } - - @Override - public void onBatchRemoveExpiredDataStarted() { - // We temporary remove the listener when batch processing starts to avoid that we rebuild our lists at each - // remove call. After batch processing at onBatchRemoveExpiredDataCompleted we add again our listener and call - // the updateLists method. - proposalService.getTempProposals().removeListener(proposalListChangeListener); - } - - @Override - public void onBatchRemoveExpiredDataCompleted() { - proposalService.getTempProposals().addListener(proposalListChangeListener); - // We only call updateLists if tempProposals have changed. updateLists() is an expensive call and takes 200 ms. - if (tempProposalsChanged) { - updateLists(); - tempProposalsChanged = false; - } - } - - /////////////////////////////////////////////////////////////////////////////////////////// // MyProposalListService.Listener /////////////////////////////////////////////////////////////////////////////////////////// diff --git a/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java b/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java index 8bdcb28715a..ce483889703 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java @@ -26,12 +26,4 @@ public interface HashMapChangedListener { @SuppressWarnings("UnusedParameters") void onRemoved(Collection protectedStorageEntries); - - // We process all expired entries after a delay (60 s) after onBootstrapComplete. - // We notify listeners of start and completion so they can optimize to only update after batch processing is done. - default void onBatchRemoveExpiredDataStarted() { - } - - default void onBatchRemoveExpiredDataCompleted() { - } } 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 c676297519b..3bfaa946727 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -200,15 +200,13 @@ void removeExpiredEntries() { .filter(entry -> entry.getValue().isExpired(this.clock)) .collect(Collectors.toCollection(ArrayList::new)); - // Batch processing can cause performance issues, so we give listeners a chance to deal with it by notifying - // about start and end of iteration. - hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataStarted); + // Batch processing can cause performance issues, so do all of the removes first, then update the listeners + // to let them know about the removes. toRemoveList.forEach(toRemoveItem -> { log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(toRemoveItem.getValue())); }); removeFromMapAndDataStore(toRemoveList); - hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); if (sequenceNumberMap.size() > this.maxSequenceNumberMapSizeBeforePurge) sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap())); From 849155a92af779842c3bda8e5160effe3646a622 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 11:04:44 -0800 Subject: [PATCH 09/22] [TESTS] Regression test for #3629 Write a test that shows the incorrect behavior for #3629, the hashmap is rebuilt from disk using the 20-byte key instead of the 32-byte key. --- .../network/p2p/storage/P2PDataStorage.java | 4 +- ...PDataStorageProtectedStorageEntryTest.java | 18 ++++++ .../bisq/network/p2p/storage/TestState.java | 64 +++++++++++++++++-- 3 files changed, 80 insertions(+), 6 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 3bfaa946727..ce69d64d8e6 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -121,7 +121,9 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers private Timer removeExpiredEntriesTimer; private final Storage sequenceNumberMapStorage; - private final SequenceNumberMap sequenceNumberMap = new SequenceNumberMap(); + + @VisibleForTesting + final SequenceNumberMap sequenceNumberMap = new SequenceNumberMap(); private final Set appendOnlyDataStoreListeners = new CopyOnWriteArraySet<>(); private final Set protectedDataStoreListeners = new CopyOnWriteArraySet<>(); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 82e58623ca8..1f582272cb2 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -38,6 +38,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import org.junit.Assert; @@ -571,6 +572,23 @@ protected Class getEntryClass() { return ProtectedStorageEntry.class; } + + // Tests that just apply to PersistablePayload objects + + // XXXBUG_3629XXX: Persisted ProtectedStorageEntries are saved to disk via their 20-byte hash. This causes + // the internal hash map to be reloaded with the 20-byte key instead of the 32-byte key. + @Test + public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629RegressionTest() { + ProtectedStorageEntry protectedStorageEntry = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(protectedStorageEntry, true, true); + + Map beforeRestart = this.testState.mockedStorage.getMap(); + + this.testState.simulateRestart(); + + // Should be equal + Assert.assertNotEquals(beforeRestart, this.testState.mockedStorage.getMap()); + } } /** 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 8e74b5845f8..7eef681236d 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -36,6 +36,7 @@ import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; 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; @@ -65,33 +66,86 @@ public class TestState { static final int MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE = 5; - final P2PDataStorage mockedStorage; + P2PDataStorage mockedStorage; final Broadcaster mockBroadcaster; final AppendOnlyDataStoreListener appendOnlyDataStoreListener; private final ProtectedDataStoreListener protectedDataStoreListener; private final HashMapChangedListener hashMapChangedListener; private final Storage mockSeqNrStorage; + private final ProtectedDataStoreService protectedDataStoreService; final ClockFake clockFake; TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); this.clockFake = new ClockFake(); + this.protectedDataStoreService = new ProtectedDataStoreServiceFake(); this.mockedStorage = new P2PDataStorage(mock(NetworkNode.class), this.mockBroadcaster, new AppendOnlyDataStoreServiceFake(), - new ProtectedDataStoreServiceFake(), mock(ResourceDataStoreService.class), + 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.mockedStorage.addHashMapChangedListener(this.hashMapChangedListener); - this.mockedStorage.addAppendOnlyDataStoreListener(this.appendOnlyDataStoreListener); - this.mockedStorage.addProtectedDataStoreListener(this.protectedDataStoreListener); + this.mockedStorage = createP2PDataStorageForTest( + this.mockBroadcaster, + this.protectedDataStoreService, + this.mockSeqNrStorage, + this.clockFake, + this.hashMapChangedListener, + this.appendOnlyDataStoreListener, + this.protectedDataStoreListener); + } + + + /** + * Re-initializes the in-memory data structures from the storage objects to simulate a node restarting. Important + * to note that the current TestState uses Test Doubles instead of actual disk storage so this is just "simulating" + * not running the entire storage code paths. + */ + void simulateRestart() { + when(this.mockSeqNrStorage.initAndGetPersisted(any(SequenceNumberMap.class), anyLong())) + .thenReturn(this.mockedStorage.sequenceNumberMap); + + this.mockedStorage = createP2PDataStorageForTest( + this.mockBroadcaster, + this.protectedDataStoreService, + this.mockSeqNrStorage, + this.clockFake, + this.hashMapChangedListener, + this.appendOnlyDataStoreListener, + this.protectedDataStoreListener); + } + + private static P2PDataStorage createP2PDataStorageForTest( + Broadcaster broadcaster, + ProtectedDataStoreService protectedDataStoreService, + Storage sequenceNrMapStorage, + ClockFake clock, + HashMapChangedListener hashMapChangedListener, + AppendOnlyDataStoreListener appendOnlyDataStoreListener, + ProtectedDataStoreListener protectedDataStoreListener) { + + P2PDataStorage p2PDataStorage = new P2PDataStorage(mock(NetworkNode.class), + broadcaster, + new AppendOnlyDataStoreServiceFake(), + protectedDataStoreService, mock(ResourceDataStoreService.class), + sequenceNrMapStorage, clock, MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE); + + // Currently TestState only supports reading ProtectedStorageEntries off disk. + p2PDataStorage.readFromResources("unused"); + p2PDataStorage.readPersisted(); + + p2PDataStorage.addHashMapChangedListener(hashMapChangedListener); + p2PDataStorage.addAppendOnlyDataStoreListener(appendOnlyDataStoreListener); + p2PDataStorage.addProtectedDataStoreListener(protectedDataStoreListener); + + return p2PDataStorage; } private void resetState() { From e212240b88378d69ac7fc52a6f0be31e574f5fbe Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 12:11:24 -0800 Subject: [PATCH 10/22] [BUGFIX] Reconstruct HashMap using 32-byte key Addresses the first half of #3629 by ensuring that the reconstructed HashMap always has the 32-byte key for each payload. It turns out, the TempProposalStore persists the ProtectedStorageEntrys on-disk as a List and doesn't persist the key at all. Then, on reconstruction, it creates the 20-byte key for its internal map. The fix is to update the TempProposalStore to use the 32-byte key instead. This means that all writes, reads, and reconstrution of the TempProposalStore uses the 32-byte key which matches perfectly with the in-memory map of the P2PDataStorage that expects 32-byte keys. Important to note that until all seednodes receive this update, nodes will continue to have both the 20-byte and 32-byte keys in their HashMap. --- .../proposal/storage/temp/TempProposalStore.java | 2 +- .../bisq/network/p2p/storage/P2PDataStorage.java | 14 ++------------ .../P2PDataStorageProtectedStorageEntryTest.java | 8 +++----- .../java/bisq/network/p2p/storage/TestState.java | 15 ++++++--------- 4 files changed, 12 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java b/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java index 81ef3c05908..169ba028a6b 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java @@ -56,7 +56,7 @@ public class TempProposalStore implements PersistableEnvelope { /////////////////////////////////////////////////////////////////////////////////////////// private TempProposalStore(List list) { - list.forEach(entry -> map.put(P2PDataStorage.getCompactHashAsByteArray(entry.getProtectedStoragePayload()), entry)); + list.forEach(entry -> map.put(P2PDataStorage.get32ByteHashAsByteArray(entry.getProtectedStoragePayload()), entry)); } public Message toProtoMessage() { 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 ce69d64d8e6..1d79d6384c2 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -423,8 +423,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn // Persist ProtectedStorageEntrys carrying PersistablePayload payloads and signal listeners on changes if (protectedStoragePayload instanceof PersistablePayload) { - ByteArray compactHash = P2PDataStorage.getCompactHashAsByteArray(protectedStoragePayload); - ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(compactHash, protectedStorageEntry); + ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(hashOfPayload, protectedStorageEntry); if (previous == null) protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); } @@ -663,8 +662,7 @@ private void removeFromMapAndDataStore( ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); if (protectedStoragePayload instanceof PersistablePayload) { - ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload); - ProtectedStorageEntry previous = protectedDataStoreService.remove(compactHash, protectedStorageEntry); + ProtectedStorageEntry previous = protectedDataStoreService.remove(hashOfPayload, protectedStorageEntry); if (previous != null) { protectedDataStoreListeners.forEach(e -> e.onRemoved(protectedStorageEntry)); } else { @@ -714,14 +712,6 @@ public static ByteArray get32ByteHashAsByteArray(NetworkPayload data) { return new ByteArray(P2PDataStorage.get32ByteHash(data)); } - public static ByteArray getCompactHashAsByteArray(ProtectedStoragePayload protectedStoragePayload) { - return new ByteArray(getCompactHash(protectedStoragePayload)); - } - - private static byte[] getCompactHash(ProtectedStoragePayload protectedStoragePayload) { - return Hash.getSha256Ripemd160hash(protectedStoragePayload.toProtoMessage().toByteArray()); - } - // Get a new map with entries older than PURGE_AGE_DAYS purged from the given map. private Map getPurgedSequenceNumberMap(Map persisted) { Map purged = new HashMap<>(); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 1f582272cb2..dda63aa1dfa 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -575,8 +575,7 @@ protected Class getEntryClass() { // Tests that just apply to PersistablePayload objects - // XXXBUG_3629XXX: Persisted ProtectedStorageEntries are saved to disk via their 20-byte hash. This causes - // the internal hash map to be reloaded with the 20-byte key instead of the 32-byte key. + // TESTCASE: Ensure the HashMap is the same before and after a restart @Test public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629RegressionTest() { ProtectedStorageEntry protectedStorageEntry = this.getProtectedStorageEntryForAdd(1); @@ -585,9 +584,8 @@ public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629Reg Map beforeRestart = this.testState.mockedStorage.getMap(); this.testState.simulateRestart(); - - // Should be equal - Assert.assertNotEquals(beforeRestart, this.testState.mockedStorage.getMap()); + + Assert.assertEquals(beforeRestart, this.testState.mockedStorage.getMap()); } } 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 7eef681236d..214f2cceacb 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -215,7 +215,6 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, boolean expectedStateChange, boolean expectedIsDataOwner) { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); if (expectedStateChange) { Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getMap().get(hashMapHash)); @@ -225,10 +224,10 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, // 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) { - Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getProtectedDataStoreMap().get(storageHash)); + Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); verify(this.protectedDataStoreListener).onAdded(protectedStorageEntry); } else { - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(storageHash)); + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); } @@ -245,7 +244,7 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(storageHash)); + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); @@ -294,13 +293,12 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, protectedStorageEntries.forEach(protectedStorageEntry -> { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); if (expectedStateChange) { Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { - Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(storageHash)); + Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); } @@ -420,11 +418,10 @@ private SavedTestState(TestState testState, PersistableNetworkPayload persistabl private SavedTestState(TestState testState, ProtectedStorageEntry protectedStorageEntry) { this(testState); - P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - this.protectedStorageEntryBeforeOpDataStoreMap = testState.mockedStorage.getProtectedDataStoreMap().get(storageHash); - P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); this.protectedStorageEntryBeforeOp = testState.mockedStorage.getMap().get(hashMapHash); + this.protectedStorageEntryBeforeOpDataStoreMap = testState.mockedStorage.getProtectedDataStoreMap().get(hashMapHash); + this.creationTimestampBeforeUpdate = (this.protectedStorageEntryBeforeOp != null) ? this.protectedStorageEntryBeforeOp.getCreationTimeStamp() : 0; } From 455f7d2689f2ef31fbe176c1bedf1743cbd87113 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 12:18:24 -0800 Subject: [PATCH 11/22] [BUGFIX] Use 32-byte key in requestData path Addresses the second half of #3629 by using the HashMap, not the protectedDataStore to generate the known keys in the requestData path. This won't have any bandwidth reduction until all seednodes have the update and only have the 32-byte key in their HashMap. fixes #3629 --- .../java/bisq/network/p2p/peers/getdata/RequestDataHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java index f0c972780e3..a116d171b7d 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java @@ -132,7 +132,7 @@ void requestData(NodeAddress nodeAddress, boolean isPreliminaryDataRequest) { .map(e -> e.bytes) .collect(Collectors.toSet()); - Set excludedKeysFromPersistedEntryMap = dataStorage.getProtectedDataStoreMap().keySet() + Set excludedKeysFromPersistedEntryMap = dataStorage.getMap().keySet() .stream() .map(e -> e.bytes) .collect(Collectors.toSet()); From 793e84d88841e7dd66aa0b8197e7eeca887749b2 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 12:22:11 -0800 Subject: [PATCH 12/22] [DEAD CODE] Remove getProtectedDataStoreMap The only user has been migrated to getMap(). Delete it so future development doesn't have the same 20-byte vs 32-byte key issue. --- .../java/bisq/network/p2p/storage/P2PDataStorage.java | 5 ----- .../test/java/bisq/network/p2p/storage/TestState.java | 10 +++++----- 2 files changed, 5 insertions(+), 10 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 1d79d6384c2..fc5a2b1d197 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -222,11 +222,6 @@ public Map getAppendOnlyDataStoreMap() { return appendOnlyDataStoreService.getMap(); } - public Map getProtectedDataStoreMap() { - return protectedDataStoreService.getMap(); - } - - /////////////////////////////////////////////////////////////////////////////////////////// // MessageListener implementation /////////////////////////////////////////////////////////////////////////////////////////// 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 214f2cceacb..37fe4e4b081 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -224,10 +224,10 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, // 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) { - Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); + Assert.assertEquals(protectedStorageEntry, this.protectedDataStoreService.getMap().get(hashMapHash)); verify(this.protectedDataStoreListener).onAdded(protectedStorageEntry); } else { - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.protectedDataStoreService.getMap().get(hashMapHash)); verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); } @@ -244,7 +244,7 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.protectedDataStoreService.getMap().get(hashMapHash)); verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); @@ -298,7 +298,7 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { - Assert.assertNull(this.mockedStorage.getProtectedDataStoreMap().get(hashMapHash)); + Assert.assertNull(this.protectedDataStoreService.getMap().get(hashMapHash)); verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); } @@ -420,7 +420,7 @@ private SavedTestState(TestState testState, ProtectedStorageEntry protectedStora P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); this.protectedStorageEntryBeforeOp = testState.mockedStorage.getMap().get(hashMapHash); - this.protectedStorageEntryBeforeOpDataStoreMap = testState.mockedStorage.getProtectedDataStoreMap().get(hashMapHash); + this.protectedStorageEntryBeforeOpDataStoreMap = testState.protectedDataStoreService.getMap().get(hashMapHash); this.creationTimestampBeforeUpdate = (this.protectedStorageEntryBeforeOp != null) ? this.protectedStorageEntryBeforeOp.getCreationTimeStamp() : 0; From 526aee5ed4a3989a3ef31d0a63b47da06459f8a7 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 16:06:14 -0800 Subject: [PATCH 13/22] [TESTS] Allow tests to validate SequenceNumberMap write separately In order to implement remove-before-add behavior, we need a way to verify that the SequenceNumberMap was the only item updated. --- .../storage/P2PDataStorageClientAPITest.java | 2 +- ...2PDataStorageProtectedStorageEntryTest.java | 2 +- .../bisq/network/p2p/storage/TestState.java | 18 +++++++++++------- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java index 867cbea229a..4783cb25813 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java @@ -188,7 +188,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry); Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); - this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, true, true); + this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, false, true); } // TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index dda63aa1dfa..34755586107 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -211,7 +211,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, true, this.expectIsDataOwner()); + this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, expectInternalStateChange, this.expectIsDataOwner()); } /// Valid Add Tests (isValidForAdd() and matchesRelevantPubKey() return true) 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 37fe4e4b081..b601fc92c01 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -259,19 +259,19 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, ProtectedStorageEntry protectedStorageEntry, boolean expectedStateChange, boolean expectedBroadcastOnStateChange, - boolean expectedSeqNrWriteOnStateChange, + boolean expectedSeqNrWrite, boolean expectedIsDataOwner) { verifyProtectedStorageRemove(beforeState, Collections.singletonList(protectedStorageEntry), expectedStateChange, expectedBroadcastOnStateChange, - expectedSeqNrWriteOnStateChange, expectedIsDataOwner); + expectedSeqNrWrite, expectedIsDataOwner); } void verifyProtectedStorageRemove(SavedTestState beforeState, Collection protectedStorageEntries, boolean expectedStateChange, boolean expectedBroadcastOnStateChange, - boolean expectedSeqNrWriteOnStateChange, + boolean expectedSeqNrWrite, boolean expectedIsDataOwner) { // The default matcher expects orders to stay the same. So, create a custom matcher function since @@ -294,6 +294,14 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, protectedStorageEntries.forEach(protectedStorageEntry -> { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); + if (expectedSeqNrWrite) { + this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray( + protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); + } else { + verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); + } + + if (expectedStateChange) { Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); @@ -303,9 +311,6 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); } - if (expectedSeqNrWriteOnStateChange) - this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); - if (expectedBroadcastOnStateChange) { if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); @@ -319,7 +324,6 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); - verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); } }); } From 372c26de74e2b45d0fcc921d32a51f37756eac04 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 16:22:53 -0800 Subject: [PATCH 14/22] Implement remove-before-add message sequence behavior It is possible to receive a RemoveData or RemoveMailboxData message before the relevant AddData, but the current code does not handle it. This results in internal state updates and signal handler's being called when an Add is received with a lower sequence number than a previously seen Remove. Minor test validation changes to allow tests to specify that only the SequenceNumberMap should be written during an operation. --- .../network/p2p/storage/P2PDataStorage.java | 24 +++++----- .../storage/P2PDataStorageClientAPITest.java | 2 +- ...PDataStorageProtectedStorageEntryTest.java | 44 +++++++++++++++---- 3 files changed, 48 insertions(+), 22 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 fc5a2b1d197..01023019b7a 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -482,13 +482,6 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - // If we don't know about the target of this remove, ignore it - ProtectedStorageEntry storedEntry = map.get(hashOfPayload); - if (storedEntry == null) { - log.debug("Remove data ignored as we don't have an entry for that data."); - return false; - } - // If we have seen a more recent operation for this payload, ignore this one if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; @@ -498,19 +491,26 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, return false; // If we have already seen an Entry with the same hash, verify the metadata is the same - if (!protectedStorageEntry.matchesRelevantPubKey(storedEntry)) + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + if (storedEntry != null && !protectedStorageEntry.matchesRelevantPubKey(storedEntry)) return false; - // Valid remove entry, do the remove and signal listeners - removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload); - printData("after remove"); - // Record the latest sequence number and persist it sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + // This means the RemoveData or RemoveMailboxData was seen prior to the AddData. We have already updated + // the SequenceNumberMap appropriately so the stale Add will not pass validation, but we don't want to + // signal listeners for state changes since no original state existed. + if (storedEntry == null) + return false; + + // Valid remove entry, do the remove and signal listeners + removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload); + printData("after remove"); + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) { broadcast(new RemoveMailboxDataMessage((ProtectedMailboxStorageEntry) protectedStorageEntry), sender, null, isDataOwner); } else { diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java index 4783cb25813..867cbea229a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java @@ -188,7 +188,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry); Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); - this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, false, true); + this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, true, true); } // TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 34755586107..3d39762e6ba 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -204,6 +204,14 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, boolean expectedReturnValue, boolean expectInternalStateChange) { + doProtectedStorageRemoveAndVerify(entry, expectedReturnValue, expectInternalStateChange, expectInternalStateChange); + } + + void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, + boolean expectedReturnValue, + boolean expectInternalStateChange, + boolean expectSeqNrWrite) { + SavedTestState beforeState = this.testState.saveTestState(entry); boolean addResult = this.doRemove(entry); @@ -211,7 +219,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, expectInternalStateChange, this.expectIsDataOwner()); + this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, expectSeqNrWrite, this.expectIsDataOwner()); } /// Valid Add Tests (isValidForAdd() and matchesRelevantPubKey() return true) @@ -325,12 +333,12 @@ public void remove_seqNrGtAddSeqNr() { doProtectedStorageRemoveAndVerify(entryForRemove, true, true); } - // TESTCASE: Removing an item before it was added + // TESTCASE: Removing an item before it was added. This triggers a SequenceNumberMap write, but nothing else @Test public void remove_notExists() { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, true); } // TESTCASE: Removing an item after successfully adding (remove seq # < add seq #) @@ -415,17 +423,35 @@ public void add_afterRemoveLessSeqNr() { } // TESTCASE: Received remove for nonexistent item that was later received - // XXXBUGXXX: There may be cases where removes are reordered with adds (remove during pending GetDataRequest?). - // The proper behavior may be to not add the late messages, but the current code will successfully add them - // even in the AddOncePayload (mailbox) case. @Test public void remove_lateAdd() { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + this.doRemove(entryForRemove); + + doProtectedStorageAddAndVerify(entryForAdd, false, false); + } + + // TESTCASE: Invalid remove doesn't block a valid add (isValidForRemove == false | matchesRelevantPubKey == false) + @Test + public void remove_entryNotIsValidForRemoveDoesntBlockAdd1() { + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1, false, false); + + this.doRemove(entryForRemove); + + doProtectedStorageAddAndVerify(entryForAdd, true, true); + } + + // TESTCASE: Invalid remove doesn't block a valid add (isValidForRemove == false | matchesRelevantPubKey == true) + @Test + public void remove_entryNotIsValidForRemoveDoesntBlockAdd2() { + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1, false, true); + + this.doRemove(entryForRemove); - // should be (false, false) doProtectedStorageAddAndVerify(entryForAdd, true, true); } } @@ -584,7 +610,7 @@ public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629Reg Map beforeRestart = this.testState.mockedStorage.getMap(); this.testState.simulateRestart(); - + Assert.assertEquals(beforeRestart, this.testState.mockedStorage.getMap()); } } From 931c1f47b4ff40ccf54137d60a1d1dc6574cd822 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 08:16:02 -0800 Subject: [PATCH 15/22] [TESTS] Allow remove() verification to be more flexible Now that we have introduced remove-before-add, we need a way to validate that the SequenceNumberMap was written, but nothing else. Add this feature to the validation path. --- .../storage/P2PDataStorageClientAPITest.java | 6 +-- ...PDataStorageProtectedStorageEntryTest.java | 2 +- .../P2PDataStorageRemoveExpiredTest.java | 12 ++--- .../storage/P2PDataStoreDisconnectTest.java | 2 +- .../bisq/network/p2p/storage/TestState.java | 46 ++++++++++--------- 5 files changed, 35 insertions(+), 33 deletions(-) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java index 867cbea229a..e669038ffc5 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java @@ -188,7 +188,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry); Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); - this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, true, true); + this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, false, false, true, true); } // TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API @@ -210,7 +210,7 @@ public void getMailboxDataWithSignedSeqNr_AddThenRemove() throws NoSuchAlgorithm SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry); Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); - this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, true, true, true,true); + this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, true, true, true, true,true); } // TESTCASE: Removing a mailbox message that was added from the onMessage handler @@ -237,6 +237,6 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry); Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); - this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, true, true, true,true); + this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, true, true, true, true,true); } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 3d39762e6ba..67223aee326 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -219,7 +219,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, expectSeqNrWrite, this.expectIsDataOwner()); + this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, expectInternalStateChange, expectInternalStateChange, expectSeqNrWrite, this.expectIsDataOwner()); } /// Valid Add Tests (isValidForAdd() and matchesRelevantPubKey() return true) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java index 1a30e2cc789..f365abdf7d9 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java @@ -67,7 +67,7 @@ public void removeExpiredEntries_SkipsNonExpirableEntries() throws NoSuchAlgorit SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, false, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, false, false, false, false, false); } // TESTCASE: Correctly skips all PersistableNetworkPayloads since they are not expirable @@ -93,7 +93,7 @@ public void removeExpiredEntries_SkipNonExpiredExpirableEntries() throws CryptoE SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, false, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, false, false, false, false, false); } // TESTCASE: Correctly expires non-persistable entries that are expired @@ -110,7 +110,7 @@ public void removeExpiredEntries_ExpiresExpiredExpirableEntries() throws CryptoE SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, true, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, true, true, false, false, false); } // TESTCASE: Correctly skips persistable entries that are not expired @@ -124,7 +124,7 @@ public void removeExpiredEntries_SkipNonExpiredPersistableExpirableEntries() thr SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, false, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, false, false, false, false, false); } // TESTCASE: Correctly expires persistable entries that are expired @@ -141,7 +141,7 @@ public void removeExpiredEntries_ExpiresExpiredPersistableExpirableEntries() thr SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, true, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, true, true, false, false, false); } // TESTCASE: Ensure we try to purge old entries sequence number map when size exceeds the maximum size @@ -187,7 +187,7 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA // The first 4 entries (11 days old) should be purged from the SequenceNumberMap SavedTestState beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); - this.testState.verifyProtectedStorageRemove(beforeState, expectedRemoves, true, false, false, false); + this.testState.verifyProtectedStorageRemove(beforeState, expectedRemoves, true, true, false, false, false); // Which means that an addition of a purged entry should succeed. beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java index 6b1ce7db935..1f58b815188 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java @@ -69,7 +69,7 @@ private static void verifyStateAfterDisconnect(TestState currentState, ProtectedStorageEntry protectedStorageEntry = beforeState.protectedStorageEntryBeforeOp; currentState.verifyProtectedStorageRemove(beforeState, protectedStorageEntry, - wasRemoved, false, false, false); + wasRemoved, wasRemoved, false, false, false); if (wasTTLReduced) Assert.assertTrue(protectedStorageEntry.getCreationTimeStamp() < beforeState.creationTimestampBeforeUpdate); 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 b601fc92c01..645a2b22457 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -257,26 +257,28 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, void verifyProtectedStorageRemove(SavedTestState beforeState, ProtectedStorageEntry protectedStorageEntry, - boolean expectedStateChange, - boolean expectedBroadcastOnStateChange, + boolean expectedHashMapAndDataStoreUpdated, + boolean expectedListenersSignaled, + boolean expectedBroadcast, boolean expectedSeqNrWrite, boolean expectedIsDataOwner) { verifyProtectedStorageRemove(beforeState, Collections.singletonList(protectedStorageEntry), - expectedStateChange, expectedBroadcastOnStateChange, + expectedHashMapAndDataStoreUpdated, expectedListenersSignaled, expectedBroadcast, expectedSeqNrWrite, expectedIsDataOwner); } void verifyProtectedStorageRemove(SavedTestState beforeState, Collection protectedStorageEntries, - boolean expectedStateChange, - boolean expectedBroadcastOnStateChange, + boolean expectedHashMapAndDataStoreUpdated, + boolean expectedListenersSignaled, + boolean expectedBroadcast, boolean expectedSeqNrWrite, boolean expectedIsDataOwner) { // The default matcher expects orders to stay the same. So, create a custom matcher function since // we don't care about the order. - if (expectedStateChange) { + if (expectedListenersSignaled) { final ArgumentCaptor> argument = ArgumentCaptor.forClass(Collection.class); verify(this.hashMapChangedListener).onRemoved(argument.capture()); @@ -289,20 +291,32 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, Assert.assertEquals(expected, actual); } else { verify(this.hashMapChangedListener, never()).onRemoved(any()); + verify(this.protectedDataStoreListener, never()).onAdded(any()); } + if (!expectedSeqNrWrite) + verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); + + if (!expectedBroadcast) + verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); + + protectedStorageEntries.forEach(protectedStorageEntry -> { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - if (expectedSeqNrWrite) { + if (expectedSeqNrWrite) this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray( protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); - } else { - verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); + + if (expectedBroadcast) { + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) + verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + else + verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); } - if (expectedStateChange) { + if (expectedHashMapAndDataStoreUpdated) { Assert.assertNull(this.mockedStorage.getMap().get(hashMapHash)); if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) { @@ -310,20 +324,8 @@ void verifyProtectedStorageRemove(SavedTestState beforeState, verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); } - - if (expectedBroadcastOnStateChange) { - if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) - verify(this.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); - else - verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); - } - } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); - - verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); - verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); - verify(this.protectedDataStoreListener, never()).onAdded(protectedStorageEntry); } }); } From 0472ffc794072e2c7ba3c7f7ac6b246bafa764d1 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 08:35:57 -0800 Subject: [PATCH 16/22] Broadcast remove-before-add messages to P2P network In order to aid in propagation of remove() messages, broadcast them in the event the remove is seen before the add. --- .../bisq/network/p2p/storage/P2PDataStorage.java | 16 ++++++++-------- .../p2p/storage/P2PDataStorageClientAPITest.java | 2 +- .../P2PDataStorageProtectedStorageEntryTest.java | 4 ++-- 3 files changed, 11 insertions(+), 11 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 01023019b7a..0aae51e3e84 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -501,14 +501,14 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); - // This means the RemoveData or RemoveMailboxData was seen prior to the AddData. We have already updated - // the SequenceNumberMap appropriately so the stale Add will not pass validation, but we don't want to - // signal listeners for state changes since no original state existed. - if (storedEntry == null) - return false; - - // Valid remove entry, do the remove and signal listeners - removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload); + if (storedEntry != null) { + // Valid remove entry, do the remove and signal listeners + removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload); + } /* else { + // This means the RemoveData or RemoveMailboxData was seen prior to the AddData. We have already updated + // the SequenceNumberMap appropriately so the stale Add will not pass validation, but we still want to + // broadcast the remove to peers so they can update their state appropriately + } */ printData("after remove"); if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) { diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java index e669038ffc5..4e0cb6c895f 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java @@ -186,7 +186,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry); - Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); + Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true)); this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, false, false, true, true); } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 67223aee326..37876872276 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -219,7 +219,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, expectInternalStateChange, expectInternalStateChange, expectSeqNrWrite, this.expectIsDataOwner()); + this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, expectInternalStateChange, expectSeqNrWrite, expectSeqNrWrite, this.expectIsDataOwner()); } /// Valid Add Tests (isValidForAdd() and matchesRelevantPubKey() return true) @@ -338,7 +338,7 @@ public void remove_seqNrGtAddSeqNr() { public void remove_notExists() { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, false, true); } // TESTCASE: Removing an item after successfully adding (remove seq # < add seq #) From 6e2ea6e3edd139f9a8c60d60c81279981e5090f3 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 08:42:58 -0800 Subject: [PATCH 17/22] [TESTS] Clean up remove verification helpers Now that there are cases where the SequenceNumberMap and Broadcast are called, but no other internal state is updated, the existing helper functions conflate too many decisions. Remove them in favor of explicitly defining each state change expected. --- ...PDataStorageProtectedStorageEntryTest.java | 43 ++++++++----------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 37876872276..e457fd6c84f 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -202,15 +202,10 @@ void doProtectedStorageAddAndVerify(ProtectedStorageEntry protectedStorageEntry, void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, boolean expectedReturnValue, - boolean expectInternalStateChange) { - - doProtectedStorageRemoveAndVerify(entry, expectedReturnValue, expectInternalStateChange, expectInternalStateChange); - } - - void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, - boolean expectedReturnValue, - boolean expectInternalStateChange, - boolean expectSeqNrWrite) { + boolean expectedHashMapAndDataStoreUpdated, + boolean expectedListenersSignaled, + boolean expectedBroadcast, + boolean expectedSeqNrWrite) { SavedTestState beforeState = this.testState.saveTestState(entry); @@ -219,7 +214,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, expectInternalStateChange, expectSeqNrWrite, expectSeqNrWrite, this.expectIsDataOwner()); + this.testState.verifyProtectedStorageRemove(beforeState, entry, expectedHashMapAndDataStoreUpdated, expectedListenersSignaled, expectedBroadcast, expectedSeqNrWrite, this.expectIsDataOwner()); } /// Valid Add Tests (isValidForAdd() and matchesRelevantPubKey() return true) @@ -272,7 +267,7 @@ public void addProtectectedStorageEntry_afterRemoveSameSeqNr() { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, false, false, false); doProtectedStorageAddAndVerify(entryForAdd, false, false); } @@ -320,7 +315,7 @@ public void remove_seqNrEqAddSeqNr() { doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, false, false, false); } // TESTCASE: Removing an item after successfully added (remove seq # > add seq #) @@ -330,15 +325,15 @@ public void remove_seqNrGtAddSeqNr() { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true, true, true, true); } - // TESTCASE: Removing an item before it was added. This triggers a SequenceNumberMap write, but nothing else + // TESTCASE: Removing an item before it was added. This triggers a SequenceNumberMap write and broadcast @Test public void remove_notExists() { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); - doProtectedStorageRemoveAndVerify(entryForRemove, true, false, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, false, false, true, true); } // TESTCASE: Removing an item after successfully adding (remove seq # < add seq #) @@ -348,7 +343,7 @@ public void remove_seqNrLessAddSeqNr() { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, false, false, false); } // TESTCASE: Add after removed (same seq #) @@ -358,7 +353,7 @@ public void add_afterRemoveSameSeqNr() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true, true, true, true); doProtectedStorageAddAndVerify(entryForAdd, false, false); } @@ -370,7 +365,7 @@ public void add_afterRemoveGreaterSeqNr() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true, true, true, true); entryForAdd = this.getProtectedStorageEntryForAdd(3); doProtectedStorageAddAndVerify(entryForAdd, true, true); @@ -385,7 +380,7 @@ public void remove_EntryNotisValidForRemoveOperation() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2, false, true); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, false, false, false); } // TESTCASE: Remove fails if Entry is valid for remove, but metadata doesn't match remove target @@ -395,7 +390,7 @@ public void remove_EntryNotmatchesRelevantPubKey() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2, true, false); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, false, false, false); } // TESTCASE: Remove fails if Entry is not valid for remove and metadata doesn't match remove target @@ -405,7 +400,7 @@ public void remove_EntryNotisValidForRemoveOperationNotmatchesRelevantPubKey() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2, false, false); - doProtectedStorageRemoveAndVerify(entryForRemove, false, false); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false, false, false, false); } @@ -416,7 +411,7 @@ public void add_afterRemoveLessSeqNr() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(3); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true, true, true, true); entryForAdd = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entryForAdd, false, false); @@ -567,7 +562,7 @@ public void refreshTTL_refreshAfterRemove() throws CryptoException { ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true, true, true, true); doRefreshTTLAndVerify(buildRefreshOfferMessage(entryForAdd, this.payloadOwnerKeys,3), false, false); } @@ -664,7 +659,7 @@ public void add_afterRemoveGreaterSeqNr() { doProtectedStorageAddAndVerify(entryForAdd, true, true); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true, true, true, true); entryForAdd = this.getProtectedStorageEntryForAdd(3); doProtectedStorageAddAndVerify(entryForAdd, false, false); From 3d571c4ca30693f59cb2b3fc2aa81103fec23b00 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 15:05:17 -0800 Subject: [PATCH 18/22] [BUGFIX] Fix duplicate sequence number use case (startup) Fix a bug introduced in d484617385276d033223ad8f36e821504e116f96 that did not properly handle a valid use case for duplicate sequence numbers. For in-memory-only ProtectedStoragePayloads, the client nodes need a way to reconstruct the Payloads after startup from peer and seed nodes. This involves sending a ProtectedStorageEntry with a sequence number that is equal to the last one the client had already seen. This patch adds tests to confirm the bug and fix as well as the changes necessary to allow adding of Payloads that were previously seen, but removed during a restart. --- .../network/p2p/storage/P2PDataStorage.java | 18 +++++++-- ...PDataStorageProtectedStorageEntryTest.java | 40 +++++++++++++++++++ .../P2PDataStorageRemoveExpiredTest.java | 10 ----- 3 files changed, 54 insertions(+), 14 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 0aae51e3e84..9e2f296dfb6 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -390,16 +390,26 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; } - // If we have seen a more recent operation for this payload, we ignore the current one - if(!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + + // If we have seen a more recent operation for this payload and we have a payload locally, ignore it + if (storedEntry != null && + !hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) { return false; + } + + // We want to allow add operations for equal sequence numbers if we don't have the payload locally. This is + // the case for non-persistent Payloads that need to be reconstructed from peer and seed nodes each startup. + MapValue sequenceNumberMapValue = sequenceNumberMap.get(hashOfPayload); + if (sequenceNumberMapValue != null && + protectedStorageEntry.getSequenceNumber() < sequenceNumberMapValue.sequenceNr) { + return false; + } // Verify the ProtectedStorageEntry is well formed and valid for the add operation if (!protectedStorageEntry.isValidForAddOperation()) return false; - ProtectedStorageEntry storedEntry = map.get(hashOfPayload); - // If we have already seen an Entry with the same hash, verify the metadata is equal if (storedEntry != null && !protectedStorageEntry.matchesRelevantPubKey(storedEntry)) return false; diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index e457fd6c84f..9f5bd539234 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -576,6 +576,34 @@ public void refreshTTL_refreshEntryOwnerOriginalOwnerMismatch() throws CryptoExc KeyPair notOwner = TestUtils.generateKeyPair(); doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, notOwner, 2), false, false); } + + // TESTCASE: After restart, identical sequence numbers are accepted ONCE. We need a way to reconstruct + // in-memory ProtectedStorageEntrys from seed and peer nodes around startup time. + @Test + public void addProtectedStorageEntry_afterRestartCanAddDuplicateSeqNr() { + ProtectedStorageEntry toAdd1 = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(toAdd1, true, true); + + this.testState.simulateRestart(); + + // Can add equal seqNr only once + doProtectedStorageAddAndVerify(toAdd1, true, true); + + // Can't add equal seqNr twice + doProtectedStorageAddAndVerify(toAdd1, false, false); + } + + // TESTCASE: After restart, old sequence numbers are not accepted + @Test + public void addProtectedStorageEntry_afterRestartCanNotAddLowerSeqNr() { + ProtectedStorageEntry toAdd1 = this.getProtectedStorageEntryForAdd(1); + ProtectedStorageEntry toAdd2 = this.getProtectedStorageEntryForAdd(2); + doProtectedStorageAddAndVerify(toAdd2, true, true); + + this.testState.simulateRestart(); + + doProtectedStorageAddAndVerify(toAdd1, false, false); + } } /** @@ -608,6 +636,18 @@ public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629Reg Assert.assertEquals(beforeRestart, this.testState.mockedStorage.getMap()); } + + // TESTCASE: After restart, identical sequence numbers are not accepted for persistent payloads + @Test + public void addProtectedStorageEntry_afterRestartCanNotAddDuplicateSeqNr() { + ProtectedStorageEntry toAdd1 = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(toAdd1, true, true); + + this.testState.simulateRestart(); + + // Can add equal seqNr only once + doProtectedStorageAddAndVerify(toAdd1, false, false); + } } /** diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java index f365abdf7d9..454799ed515 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java @@ -188,15 +188,5 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA SavedTestState beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); this.testState.verifyProtectedStorageRemove(beforeState, expectedRemoves, true, true, false, false, false); - - // Which means that an addition of a purged entry should succeed. - beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); - Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, TestState.getTestNodeAddress(), null, false)); - this.testState.verifyProtectedStorageAdd(beforeState, purgedProtectedStorageEntry, true, false); - - // The last entry (5 days old) should still exist in the SequenceNumberMap which means trying to add it again should fail. - beforeState = this.testState.saveTestState(keepProtectedStorageEntry); - Assert.assertFalse(this.testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, TestState.getTestNodeAddress(), null, false)); - this.testState.verifyProtectedStorageAdd(beforeState, keepProtectedStorageEntry, false, false); } } From 22080037bae3325a89690693ce009613b91bf1c7 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Mon, 25 Nov 2019 13:31:05 -0800 Subject: [PATCH 19/22] Clean up AtomicBoolean usage in FileManager Although the code was correct, it was hard to understand the relationship between the to-be-written object and the savePending flag. Trade two dependent atomics for one and comment the code to make it more clear for the next reader. --- .../java/bisq/common/storage/FileManager.java | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/common/src/main/java/bisq/common/storage/FileManager.java b/common/src/main/java/bisq/common/storage/FileManager.java index c66d08803d7..d5a4e86927a 100644 --- a/common/src/main/java/bisq/common/storage/FileManager.java +++ b/common/src/main/java/bisq/common/storage/FileManager.java @@ -36,20 +36,21 @@ import java.util.concurrent.Callable; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantLock; import lombok.extern.slf4j.Slf4j; +import static com.google.common.base.Preconditions.checkNotNull; + @Slf4j public class FileManager { private final File dir; private final File storageFile; private final ScheduledThreadPoolExecutor executor; - private final AtomicBoolean savePending; private final long delay; private final Callable saveFileTask; - private T persistable; + private final AtomicReference nextWrite; private final PersistenceProtoResolver persistenceProtoResolver; private final ReentrantLock writeLock = CycleDetectingLockFactory.newInstance(CycleDetectingLockFactory.Policies.THROW).newReentrantLock("writeLock"); @@ -61,25 +62,22 @@ public FileManager(File dir, File storageFile, long delay, PersistenceProtoResol this.dir = dir; this.storageFile = storageFile; this.persistenceProtoResolver = persistenceProtoResolver; + this.nextWrite = new AtomicReference<>(null); executor = Utilities.getScheduledThreadPoolExecutor("FileManager", 1, 10, 5); // File must only be accessed from the auto-save executor from now on, to avoid simultaneous access. - savePending = new AtomicBoolean(); this.delay = delay; saveFileTask = () -> { try { Thread.currentThread().setName("Save-file-task-" + new Random().nextInt(10000)); - // Runs in an auto save thread. - // TODO: this looks like it could cause corrupt data as the savePending is unset before the actual - // save. By moving to after the save there might be some persist operations that are not performed - // and data would be lost. Probably all persist operations should happen sequencially rather than - // skip one when there is already one scheduled - if (!savePending.getAndSet(false)) { - // Some other scheduled request already beat us to it. - return null; - } + + // Atomically take the next object to write and set the value to null so `saveLater` callers can + // determine if there is a pending write. + T persistable = this.nextWrite.getAndSet(null); + checkNotNull(persistable); + saveNowInternal(persistable); } catch (Throwable e) { log.error("Error during saveFileTask", e); @@ -111,12 +109,13 @@ public void saveLater(T persistable) { } public void saveLater(T persistable, long delayInMilli) { - this.persistable = persistable; - - if (savePending.getAndSet(true)) - return; // Already pending. + // Atomically set the value of the next write. This allows batching of multiple writes of the same data + // structure if there are multiple calls to saveLater within a given `delayInMillis`. + T pendingWrite = this.nextWrite.getAndSet(persistable); - executor.schedule(saveFileTask, delayInMilli, TimeUnit.MILLISECONDS); + // If there isn't a pending write. Schedule one for `persistable`. + if (pendingWrite == null) + executor.schedule(saveFileTask, delayInMilli, TimeUnit.MILLISECONDS); } @SuppressWarnings("unchecked") From 189580268197ace9a2dc493d8e0f46ec933e537d Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Mon, 25 Nov 2019 14:00:27 -0800 Subject: [PATCH 20/22] [DEADCODE] Clean up FileManager.java --- .../java/bisq/common/storage/FileManager.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/common/src/main/java/bisq/common/storage/FileManager.java b/common/src/main/java/bisq/common/storage/FileManager.java index d5a4e86927a..d4772861dae 100644 --- a/common/src/main/java/bisq/common/storage/FileManager.java +++ b/common/src/main/java/bisq/common/storage/FileManager.java @@ -94,17 +94,10 @@ public FileManager(File dir, File storageFile, long delay, PersistenceProtoResol // API /////////////////////////////////////////////////////////////////////////////////////////// - /** - * Actually write the wallet file to disk, using an atomic rename when possible. Runs on the current thread. - */ - public void saveNow(T persistable) { - saveNowInternal(persistable); - } - /** * Queues up a save in the background. Useful for not very important wallet changes. */ - public void saveLater(T persistable) { + void saveLater(T persistable) { saveLater(persistable, delay); } @@ -133,7 +126,7 @@ public synchronized T read(File file) { } } - public synchronized void removeFile(String fileName) { + synchronized void removeFile(String fileName) { File file = new File(dir, fileName); boolean result = file.delete(); if (!result) @@ -154,7 +147,7 @@ public synchronized void removeFile(String fileName) { /** * Shut down auto-saving. */ - void shutDown() { + private void shutDown() { executor.shutdown(); try { executor.awaitTermination(5, TimeUnit.SECONDS); @@ -174,11 +167,11 @@ public static void removeAndBackupFile(File dbDir, File storageFile, String file FileUtil.renameFile(storageFile, corruptedFile); } - public synchronized void removeAndBackupFile(String fileName) throws IOException { + synchronized void removeAndBackupFile(String fileName) throws IOException { removeAndBackupFile(dir, storageFile, fileName, "backup_of_corrupted_data"); } - public synchronized void backupFile(String fileName, int numMaxBackupFiles) { + synchronized void backupFile(String fileName, int numMaxBackupFiles) { FileUtil.rollingBackup(dir, fileName, numMaxBackupFiles); } From d4d2f262d6809bbaee6cbf4025d9f59138b8dc6f Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Mon, 25 Nov 2019 14:09:23 -0800 Subject: [PATCH 21/22] [BUGFIX] Shorter delay values not taking precedence Fix a bug in the FileManager where a saveLater called with a low delay won't execute until the delay specified by a previous saveLater call. The trade off here is the execution of a task that returns early vs. losing the requested delay. --- .../java/bisq/common/storage/FileManager.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/common/src/main/java/bisq/common/storage/FileManager.java b/common/src/main/java/bisq/common/storage/FileManager.java index d4772861dae..3889a782a00 100644 --- a/common/src/main/java/bisq/common/storage/FileManager.java +++ b/common/src/main/java/bisq/common/storage/FileManager.java @@ -41,8 +41,6 @@ import lombok.extern.slf4j.Slf4j; -import static com.google.common.base.Preconditions.checkNotNull; - @Slf4j public class FileManager { private final File dir; @@ -73,10 +71,13 @@ public FileManager(File dir, File storageFile, long delay, PersistenceProtoResol try { Thread.currentThread().setName("Save-file-task-" + new Random().nextInt(10000)); - // Atomically take the next object to write and set the value to null so `saveLater` callers can - // determine if there is a pending write. + // Atomically take the next object to write and set the value to null so concurrent saveFileTask + // won't duplicate work. T persistable = this.nextWrite.getAndSet(null); - checkNotNull(persistable); + + // If null, a concurrent saveFileTask already grabbed the data. Don't duplicate work. + if (persistable == null) + return null; saveNowInternal(persistable); } catch (Throwable e) { @@ -104,11 +105,11 @@ void saveLater(T persistable) { public void saveLater(T persistable, long delayInMilli) { // Atomically set the value of the next write. This allows batching of multiple writes of the same data // structure if there are multiple calls to saveLater within a given `delayInMillis`. - T pendingWrite = this.nextWrite.getAndSet(persistable); + this.nextWrite.set(persistable); - // If there isn't a pending write. Schedule one for `persistable`. - if (pendingWrite == null) - executor.schedule(saveFileTask, delayInMilli, TimeUnit.MILLISECONDS); + // Always schedule a write. It is possible that a previous saveLater was called with a larger `delayInMilli` + // and we want the lower delay to execute. The saveFileTask handles concurrent operations. + executor.schedule(saveFileTask, delayInMilli, TimeUnit.MILLISECONDS); } @SuppressWarnings("unchecked") From 685824b0d9ae63eb2bd9343b845d1c1e3dbc3ac1 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Mon, 25 Nov 2019 14:10:42 -0800 Subject: [PATCH 22/22] [REFACTOR] Inline saveNowInternal Only one caller after deadcode removal. --- .../src/main/java/bisq/common/storage/FileManager.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/common/src/main/java/bisq/common/storage/FileManager.java b/common/src/main/java/bisq/common/storage/FileManager.java index 3889a782a00..ddf44301ff0 100644 --- a/common/src/main/java/bisq/common/storage/FileManager.java +++ b/common/src/main/java/bisq/common/storage/FileManager.java @@ -79,7 +79,9 @@ public FileManager(File dir, File storageFile, long delay, PersistenceProtoResol if (persistable == null) return null; - saveNowInternal(persistable); + long now = System.currentTimeMillis(); + saveToFile(persistable, dir, storageFile); + log.debug("Save {} completed in {} msec", storageFile, System.currentTimeMillis() - now); } catch (Throwable e) { log.error("Error during saveFileTask", e); } @@ -180,12 +182,6 @@ synchronized void backupFile(String fileName, int numMaxBackupFiles) { // Private /////////////////////////////////////////////////////////////////////////////////////////// - private void saveNowInternal(T persistable) { - long now = System.currentTimeMillis(); - saveToFile(persistable, dir, storageFile); - log.debug("Save {} completed in {} msec", storageFile, System.currentTimeMillis() - now); - } - private synchronized void saveToFile(T persistable, File dir, File storageFile) { File tempFile = null; FileOutputStream fileOutputStream = null;