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 6356515f18..d1f3aeb22c 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 @@ public class P2PModule extends AppModule { 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 04313763d1..7ed3ee77f6 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 bisq.common.util.Utilities; 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 7814004cd2..aec700c8ab 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 class P2PDataStorageRemoveExpiredTest { 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 b13ec2a97a..942cd668e1 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 @@ import static org.mockito.Mockito.*; * 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 c8a3d9c613..9213bcb0fb 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 @@ import static bisq.network.p2p.storage.TestState.*; * 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 2aa8bc0360..82e58623ca 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 @@ import static bisq.network.p2p.storage.TestState.*; * 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 7a54138a92..6b1ce7db93 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.Assert; 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 class P2PDataStoreDisconnectTest { 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 942cd668e1..a0fa8a00d8 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.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedMailboxStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; -import bisq.network.p2p.storage.persistence.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 bisq.common.storage.Storage; 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 d6be958138..5d9a9d3784 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 05a3872a7f..0fba35a283 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.security.SignatureException; import java.math.BigInteger; +import java.util.Collection; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -79,22 +81,26 @@ public class AlertManager { 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 9be6942b6c..1d75c5519f 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.ListChangeListener; 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 class ProposalListPresentation implements DaoStateListener, HashMapChange /////////////////////////////////////////////////////////////////////////////////////////// @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 6da0d38751..594ce6c10a 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 javax.inject.Named; 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 class ProposalService implements HashMapChangedListener, AppendOnlyDataSt /////////////////////////////////////////////////////////////////////////////////////////// @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 86687d92c3..c32c27b07b 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.security.SignatureException; 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 class FilterManager { 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 5ebe152453..e9b36e8921 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 javax.inject.Inject; 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 class OfferBookService { 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 16cc1b38e8..ac6480bb1b 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.security.SignatureException; 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 abstract class DisputeAgentManager { 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 d90fdeb256..733cc385cc 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 javafx.beans.property.SimpleIntegerProperty; 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 class P2PService implements SetupListener, MessageListener, ConnectionLis /////////////////////////////////////////////////////////////////////////////////////////// @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 a3ac1c2025..8bdcb28715 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 @@ package bisq.network.p2p.storage; 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 7ed3ee77f6..513abca058 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.security.PublicKey; 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers // 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 a0fa8a00d8..594e00fb5f 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 bisq.common.storage.Storage; import java.security.PublicKey; +import java.util.Collections; import java.util.concurrent.TimeUnit; import org.junit.Assert; @@ -174,7 +175,7 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 513abca058..6c0992c6d7 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 com.google.inject.name.Named; 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.security.PublicKey; 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers /////////////////////////////////////////////////////////////////////////////////////////// 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 6c0992c6d7..c08cde13d1 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 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers } } }); + + 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 | 85 +++++++++++++------ 3 files changed, 72 insertions(+), 35 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 c08cde13d1..c676297519 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 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers // 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 aec700c8ab..1a30e2cc78 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 bisq.common.crypto.CryptoException; 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { // 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 594e00fb5f..8e74b5845f 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 bisq.common.storage.Storage; 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 @@ public class TestState { 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)); - - verify(this.protectedDataStoreListener).onRemoved(protectedStorageEntry); - } - - verify(this.hashMapChangedListener).onRemoved(Collections.singletonList(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)); - else - verify(this.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); - } + Set actual = new HashSet<>(argument.getValue()); + Set expected = new HashSet<>(protectedStorageEntries); + // Ensure we didn't remove duplicates + Assert.assertEquals(protectedStorageEntries.size(), expected.size()); + Assert.assertEquals(argument.getValue().size(), actual.size()); + Assert.assertEquals(expected, actual); } 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()); + verify(this.hashMapChangedListener, never()).onRemoved(any()); } + + 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)); + + 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)); + 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 594ce6c10a..7953d403b6 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 javax.inject.Named; 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 class ProposalService implements HashMapChangedListener, AppendOnlyDataSt @Override public void onRemoved(Collection protectedStorageEntries) { - protectedStorageEntries.forEach(protectedStorageEntry -> { - onProtectedDataRemoved(protectedStorageEntry); - }); + onProtectedDataRemoved(protectedStorageEntries); } @@ -271,30 +270,39 @@ public class ProposalService implements HashMapChangedListener, AppendOnlyDataSt } } - 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 0000000000..7945a241b2 --- /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 1d75c5519f..49aa219281 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 @@ package bisq.core.dao.governance.proposal; 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.ListChangeListener; 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 @@ import lombok.extern.slf4j.Slf4j; * 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 class ProposalListPresentation implements DaoStateListener, HashMapChange this.validatorProvider = validatorProvider; daoStateService.addDaoStateListener(this); - p2PDataStorage.addHashMapChangedListener(this); myProposalListService.addListener(this); proposalListChangeListener = c -> updateLists(); @@ -125,48 +115,6 @@ public class ProposalListPresentation implements DaoStateListener, HashMapChange 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 8bdcb28715..ce48388970 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 c676297519..3bfaa94672 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 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers .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 3bfaa94672..ce69d64d8e 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 82e58623ca..1f582272cb 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.security.NoSuchAlgorithmException; 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 8e74b5845f..7eef681236 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.ProtectedMailboxStorageEntry; 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 @@ import static org.mockito.Mockito.*; 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 81ef3c0590..169ba028a6 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 ce69d64d8e..1d79d6384c 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers // 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 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 1f582272cb..dda63aa1df 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 @@ public class P2PDataStorageProtectedStorageEntryTest { // 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 class P2PDataStorageProtectedStorageEntryTest { 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 7eef681236..214f2cceac 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 @@ public class TestState { 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 @@ public class TestState { // 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 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 f0c972780e..a116d171b7 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 @@ class RequestDataHandler implements MessageListener { .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 1d79d6384c..fc5a2b1d19 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 214f2cceac..37fe4e4b08 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 @@ public class TestState { // 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 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 867cbea229..4783cb2581 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 class P2PDataStorageClientAPITest { 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 dda63aa1df..3475558610 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 37fe4e4b08..b601fc92c0 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 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 fc5a2b1d19..01023019b7 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 4783cb2581..867cbea229 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 class P2PDataStorageClientAPITest { 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 3475558610..3d39762e6b 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { } // 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 class P2PDataStorageProtectedStorageEntryTest { 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 867cbea229..e669038ffc 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 class P2PDataStorageClientAPITest { 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 class P2PDataStorageClientAPITest { 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 class P2PDataStorageClientAPITest { 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 3d39762e6b..67223aee32 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 1a30e2cc78..f365abdf7d 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { 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 class P2PDataStorageRemoveExpiredTest { // 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 6b1ce7db93..1f58b81518 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 @@ public class P2PDataStoreDisconnectTest { 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 b601fc92c0..645a2b2245 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 @@ public class TestState { 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 @@ public class TestState { 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 @@ public class TestState { 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 01023019b7..0aae51e3e8 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 e669038ffc..4e0cb6c895 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 class P2PDataStorageClientAPITest { 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 67223aee32..3787687227 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 3787687227..e457fd6c84 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 @@ public class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 0aae51e3e8..9e2f296dfb 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 class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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 e457fd6c84..9f5bd53923 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 class P2PDataStorageProtectedStorageEntryTest { 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 class P2PDataStorageProtectedStorageEntryTest { 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 f365abdf7d..454799ed51 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 class P2PDataStorageRemoveExpiredTest { 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 c66d08803d..d5a4e86927 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.Random; 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 class FileManager { 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 class FileManager { } public void saveLater(T persistable, long delayInMilli) { - this.persistable = persistable; + // 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); - if (savePending.getAndSet(true)) - return; // Already pending. - - 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 d5a4e86927..d4772861da 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 class FileManager { // 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 class FileManager { } } - 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 class FileManager { /** * Shut down auto-saving. */ - void shutDown() { + private void shutDown() { executor.shutdown(); try { executor.awaitTermination(5, TimeUnit.SECONDS); @@ -174,11 +167,11 @@ public class FileManager { 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 d4772861da..3889a782a0 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 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; @@ -73,10 +71,13 @@ public class FileManager { 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 @@ public class FileManager { 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 3889a782a0..ddf44301ff 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 class FileManager { 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 @@ public class FileManager { // 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;