From 5fcd18c4b52513729d6045c32ef39c068151c12c Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 20 Nov 2019 10:05:23 -0800 Subject: [PATCH 01/28] [TESTS] Add tests of requestData This will allow us to push the GetData creation inside P2PDataStorage safely. --- .../p2p/peers/getdata/RequestDataHandler.java | 4 +- .../RequestDataHandlerRequestDataTest.java | 249 ++++++++++++++++++ .../bisq/network/p2p/storage/TestState.java | 5 +- .../mocks/PersistableNetworkPayloadStub.java | 12 +- 4 files changed, 267 insertions(+), 3 deletions(-) create mode 100644 p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java 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 a116d171b7d..161e44e7e99 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 @@ -38,6 +38,7 @@ import bisq.common.proto.network.NetworkEnvelope; import bisq.common.proto.network.NetworkPayload; +import com.google.common.annotations.VisibleForTesting; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.SettableFuture; @@ -92,7 +93,8 @@ public interface Listener { private final PeerManager peerManager; private final Listener listener; private Timer timeoutTimer; - private final int nonce = new Random().nextInt(); + @VisibleForTesting + final int nonce = new Random().nextInt(); private boolean stopped; diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java new file mode 100644 index 00000000000..e757ce07f04 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java @@ -0,0 +1,249 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.peers.getdata; + +import bisq.network.p2p.NodeAddress; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.network.Connection; +import bisq.network.p2p.network.NetworkNode; +import bisq.network.p2p.peers.PeerManager; +import bisq.network.p2p.peers.getdata.messages.GetDataRequest; +import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; +import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; +import bisq.network.p2p.storage.P2PDataStorage; +import bisq.network.p2p.storage.TestState; +import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import bisq.common.app.Capabilities; +import bisq.common.app.Capability; + +import com.google.common.util.concurrent.SettableFuture; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.util.Set; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + + + +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import static org.mockito.Mockito.*; + +public class RequestDataHandlerRequestDataTest { + private TestState testState; + + @Mock + NetworkNode networkNode; + + private NodeAddress localNodeAddress; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + this.testState = new TestState(); + + this.localNodeAddress = new NodeAddress("localhost", 8080); + when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); + + // Set up basic capabilities to ensure message contains it + Capabilities.app.addAll(Capability.MEDIATION); + } + + /** + * Returns true if the target bytes are found in the container set. + */ + private boolean byteSetContains(Set container, byte[] target) { + // Set.contains() doesn't do a deep compare, so generate a Set so equals() does what + // we want + Set translatedContainer = + P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(container); + + return translatedContainer.contains(new P2PDataStorage.ByteArray(target)); + } + + /** + * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state + * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers + * is not supported. + */ + private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(1); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + // TESTCASE: P2PDataStorage with no entries returns an empty PreliminaryGetDataRequest + @Test + public void requestData_EmptyP2PDataStore_PreliminaryGetDataRequest() { + SettableFuture sendFuture = mock(SettableFuture.class); + + final ArgumentCaptor captor = ArgumentCaptor.forClass(PreliminaryGetDataRequest.class); + when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); + + RequestDataHandler handler = new RequestDataHandler( + this.networkNode, + this.testState.getMockedStorage(), + mock(PeerManager.class), + mock(RequestDataHandler.Listener.class) + ); + + handler.requestData(this.localNodeAddress, true); + + // expect empty message since p2pDataStore is empty + PreliminaryGetDataRequest getDataRequest = captor.getValue(); + + Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); + Assert.assertEquals(getDataRequest.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataRequest.getExcludedKeys().isEmpty()); + } + + // TESTCASE: P2PDataStorage with no entries returns an empty PreliminaryGetDataRequest + @Test + public void requestData_EmptyP2PDataStore_GetUpdatedDataRequest() { + SettableFuture sendFuture = mock(SettableFuture.class); + + final ArgumentCaptor captor = ArgumentCaptor.forClass(GetUpdatedDataRequest.class); + when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); + + RequestDataHandler handler = new RequestDataHandler( + this.networkNode, + this.testState.getMockedStorage(), + mock(PeerManager.class), + mock(RequestDataHandler.Listener.class) + ); + + handler.requestData(this.localNodeAddress, false); + + // expect empty message since p2pDataStore is empty + GetUpdatedDataRequest getDataRequest = captor.getValue(); + + Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); + Assert.assertEquals(getDataRequest.getSenderNodeAddress(), this.localNodeAddress); + Assert.assertTrue(getDataRequest.getExcludedKeys().isEmpty()); + } + + // TESTCASE: P2PDataStorage with PersistableNetworkPayloads and ProtectedStorageEntry generates + // correct GetDataRequestMessage with both sets of keys. + @Test + public void requestData_FilledP2PDataStore_PreliminaryGetDataRequest() throws NoSuchAlgorithmException { + SettableFuture sendFuture = mock(SettableFuture.class); + + PersistableNetworkPayload toAdd1 = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload toAdd2 = new PersistableNetworkPayloadStub(new byte[] { 2 }); + ProtectedStorageEntry toAdd3 = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry toAdd4 = getProtectedStorageEntryForAdd(); + + this.testState.getMockedStorage().addPersistableNetworkPayload( + toAdd1, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addPersistableNetworkPayload( + toAdd2, this.localNodeAddress, false, false, false, false); + + this.testState.getMockedStorage().addProtectedStorageEntry(toAdd3, this.localNodeAddress, null, false); + this.testState.getMockedStorage().addProtectedStorageEntry(toAdd4, this.localNodeAddress, null, false); + + final ArgumentCaptor captor = ArgumentCaptor.forClass(PreliminaryGetDataRequest.class); + when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); + + RequestDataHandler handler = new RequestDataHandler( + this.networkNode, + this.testState.getMockedStorage(), + mock(PeerManager.class), + mock(RequestDataHandler.Listener.class) + ); + + handler.requestData(this.localNodeAddress, true); + + // expect empty message since p2pDataStore is empty + PreliminaryGetDataRequest getDataRequest = captor.getValue(); + + Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); + Assert.assertEquals(getDataRequest.getSupportedCapabilities(), Capabilities.app); + Assert.assertEquals(4, getDataRequest.getExcludedKeys().size()); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd1.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd2.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd3.getProtectedStoragePayload()))); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd4.getProtectedStoragePayload()))); + } + + // TESTCASE: P2PDataStorage with PersistableNetworkPayloads and ProtectedStorageEntry generates + // correct GetDataRequestMessage with both sets of keys. + @Test + public void requestData_FilledP2PDataStore_GetUpdatedDataRequest() throws NoSuchAlgorithmException { + SettableFuture sendFuture = mock(SettableFuture.class); + + PersistableNetworkPayload toAdd1 = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload toAdd2 = new PersistableNetworkPayloadStub(new byte[] { 2 }); + ProtectedStorageEntry toAdd3 = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry toAdd4 = getProtectedStorageEntryForAdd(); + + this.testState.getMockedStorage().addPersistableNetworkPayload( + toAdd1, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addPersistableNetworkPayload( + toAdd2, this.localNodeAddress, false, false, false, false); + + this.testState.getMockedStorage().addProtectedStorageEntry(toAdd3, this.localNodeAddress, null, false); + this.testState.getMockedStorage().addProtectedStorageEntry(toAdd4, this.localNodeAddress, null, false); + + final ArgumentCaptor captor = ArgumentCaptor.forClass(GetUpdatedDataRequest.class); + when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); + + RequestDataHandler handler = new RequestDataHandler( + this.networkNode, + this.testState.getMockedStorage(), + mock(PeerManager.class), + mock(RequestDataHandler.Listener.class) + ); + + handler.requestData(this.localNodeAddress, false); + + // expect empty message since p2pDataStore is empty + GetUpdatedDataRequest getDataRequest = captor.getValue(); + + Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); + Assert.assertEquals(getDataRequest.getSenderNodeAddress(), this.localNodeAddress); + Assert.assertEquals(4, getDataRequest.getExcludedKeys().size()); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd1.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd2.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd3.getProtectedStoragePayload()))); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd4.getProtectedStoragePayload()))); + } +} 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 c4ceab5ca2a..88d465c5734 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -51,6 +51,8 @@ import java.util.Set; import java.util.concurrent.TimeUnit; +import lombok.Getter; + import org.junit.Assert; import org.mockito.ArgumentCaptor; @@ -65,6 +67,7 @@ public class TestState { static final int MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE = 5; + @Getter P2PDataStorage mockedStorage; final Broadcaster mockBroadcaster; @@ -74,7 +77,7 @@ public class TestState { private final ProtectedDataStoreService protectedDataStoreService; final ClockFake clockFake; - TestState() { + public TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); this.clockFake = new ClockFake(); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java b/p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java index 6ab73155480..057cb2c42b8 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/mocks/PersistableNetworkPayloadStub.java @@ -28,9 +28,19 @@ */ public class PersistableNetworkPayloadStub implements PersistableNetworkPayload { private final boolean hashSizeValid; + private final byte[] hash; public PersistableNetworkPayloadStub(boolean hashSizeValid) { + this(hashSizeValid, new byte[] { 1 }); + } + + public PersistableNetworkPayloadStub(byte[] hash) { + this(true, hash); + } + + private PersistableNetworkPayloadStub(boolean hashSizeValid, byte[] hash) { this.hashSizeValid = hashSizeValid; + this.hash = hash; } @Override @@ -40,7 +50,7 @@ public protobuf.PersistableNetworkPayload toProtoMessage() { @Override public byte[] getHash() { - return new byte[] { 1 }; + return hash; } @Override From 1e814d9f1d1cc16a59665a7390a603f1fec2040d Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 20 Nov 2019 10:38:04 -0800 Subject: [PATCH 02/28] [REFACTOR] Introduce buildGetDataRequest variants As part of changing the GetData path, we want to move all creation and processing of GetData messages inside P2PDataStorage. This will allow easier unit testing of the behavior as well as cleaner code in the request handlers that can just focus on nonces, connections, etc. --- .../p2p/peers/getdata/RequestDataHandler.java | 20 +-------- .../network/p2p/storage/P2PDataStorage.java | 42 +++++++++++++++++++ 2 files changed, 44 insertions(+), 18 deletions(-) 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 161e44e7e99..10b1b7ae905 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 @@ -49,7 +49,6 @@ import java.util.Random; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; -import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -126,25 +125,10 @@ void requestData(NodeAddress nodeAddress, boolean isPreliminaryDataRequest) { if (!stopped) { GetDataRequest getDataRequest; - // We collect the keys of the PersistableNetworkPayload items so we exclude them in our request. - // PersistedStoragePayload items don't get removed, so we don't have an issue with the case that - // an object gets removed in between PreliminaryGetDataRequest and the GetUpdatedDataRequest and we would - // miss that event if we do not load the full set or use some delta handling. - Set excludedKeys = dataStorage.getAppendOnlyDataStoreMap().keySet().stream() - .map(e -> e.bytes) - .collect(Collectors.toSet()); - - Set excludedKeysFromPersistedEntryMap = dataStorage.getMap().keySet() - .stream() - .map(e -> e.bytes) - .collect(Collectors.toSet()); - - excludedKeys.addAll(excludedKeysFromPersistedEntryMap); - if (isPreliminaryDataRequest) - getDataRequest = new PreliminaryGetDataRequest(nonce, excludedKeys); + getDataRequest = dataStorage.buildPreliminaryGetDataRequest(nonce); else - getDataRequest = new GetUpdatedDataRequest(networkNode.getNodeAddress(), nonce, excludedKeys); + getDataRequest = dataStorage.buildGetUpdatedDataRequest(networkNode.getNodeAddress(), nonce); if (timeoutTimer == null) { timeoutTimer = UserThread.runAfter(() -> { // setup before sending to avoid race conditions 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 c0a22c691b2..a66557ccd3c 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -25,6 +25,8 @@ import bisq.network.p2p.network.NetworkNode; import bisq.network.p2p.peers.BroadcastHandler; import bisq.network.p2p.peers.Broadcaster; +import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; +import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; import bisq.network.p2p.storage.messages.AddDataMessage; import bisq.network.p2p.storage.messages.AddOncePayload; import bisq.network.p2p.storage.messages.AddPersistableNetworkPayloadMessage; @@ -177,6 +179,46 @@ public synchronized void readFromResources(String postFix) { map.putAll(protectedDataStoreService.getMap()); } + /////////////////////////////////////////////////////////////////////////////////////////// + // RequestData API + /////////////////////////////////////////////////////////////////////////////////////////// + + /** + * Returns a PreliminaryGetDataRequest that can be sent to a peer node to request missing Payload data. + */ + public PreliminaryGetDataRequest buildPreliminaryGetDataRequest(int nonce) { + return new PreliminaryGetDataRequest(nonce, this.getKnownPayloadHashes()); + } + + /** + * Returns a GetUpdatedDataRequest that can be sent to a peer node to request missing Payload data. + */ + public GetUpdatedDataRequest buildGetUpdatedDataRequest(NodeAddress senderNodeAddress, int nonce) { + return new GetUpdatedDataRequest(senderNodeAddress, nonce, this.getKnownPayloadHashes()); + } + + /** + * Returns the set of known payload hashes. This is used in the GetData path to request missing data from peer nodes + */ + private Set getKnownPayloadHashes() { + // We collect the keys of the PersistableNetworkPayload items so we exclude them in our request. + // PersistedStoragePayload items don't get removed, so we don't have an issue with the case that + // an object gets removed in between PreliminaryGetDataRequest and the GetUpdatedDataRequest and we would + // miss that event if we do not load the full set or use some delta handling. + Set excludedKeys =this.appendOnlyDataStoreService.getMap().keySet().stream() + .map(e -> e.bytes) + .collect(Collectors.toSet()); + + Set excludedKeysFromPersistedEntryMap = this.map.keySet() + .stream() + .map(e -> e.bytes) + .collect(Collectors.toSet()); + + excludedKeys.addAll(excludedKeysFromPersistedEntryMap); + + return excludedKeys; + } + /////////////////////////////////////////////////////////////////////////////////////////// // API From a927ed42ac38035dc9d1b024dacec737a3bf443a Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 20 Nov 2019 10:54:53 -0800 Subject: [PATCH 03/28] [TESTS] Add tests of new RequestData APIs These are identical test cases to the requestHandler tests, but with much fewer dependencies. The requestHandler tests will eventually be deleted, but they are going to remain throughout development as an extra safety net. --- .../P2PDataStorageRequestDataTest.java | 175 ++++++++++++++++++ 1 file changed, 175 insertions(+) create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRequestDataTest.java diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRequestDataTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRequestDataTest.java new file mode 100644 index 00000000000..55d16344d64 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRequestDataTest.java @@ -0,0 +1,175 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage; + +import bisq.network.p2p.NodeAddress; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; +import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; +import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import bisq.common.app.Capabilities; +import bisq.common.app.Capability; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.util.Set; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import org.mockito.MockitoAnnotations; + +import static org.mockito.Mockito.*; + +public class P2PDataStorageRequestDataTest { + private TestState testState; + + private NodeAddress localNodeAddress; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + this.testState = new TestState(); + + this.localNodeAddress = new NodeAddress("localhost", 8080); + + // Set up basic capabilities to ensure message contains it + Capabilities.app.addAll(Capability.MEDIATION); + } + + /** + * Returns true if the target bytes are found in the container set. + */ + private boolean byteSetContains(Set container, byte[] target) { + // Set.contains() doesn't do a deep compare, so generate a Set so equals() does what + // we want + Set translatedContainer = + P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(container); + + return translatedContainer.contains(new P2PDataStorage.ByteArray(target)); + } + + /** + * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state + * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers + * is not supported. + */ + private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(1); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + // TESTCASE: P2PDataStorage with no entries returns an empty PreliminaryGetDataRequest + @Test + public void buildPreliminaryGetDataRequest_EmptyP2PDataStore() { + PreliminaryGetDataRequest getDataRequest = this.testState.mockedStorage.buildPreliminaryGetDataRequest(1); + + Assert.assertEquals(getDataRequest.getNonce(), 1); + Assert.assertEquals(getDataRequest.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataRequest.getExcludedKeys().isEmpty()); + } + + // TESTCASE: P2PDataStorage with no entries returns an empty PreliminaryGetDataRequest + @Test + public void buildGetUpdatedDataRequest_EmptyP2PDataStore() { + GetUpdatedDataRequest getDataRequest = + this.testState.mockedStorage.buildGetUpdatedDataRequest(this.localNodeAddress, 1); + + Assert.assertEquals(getDataRequest.getNonce(), 1); + Assert.assertEquals(getDataRequest.getSenderNodeAddress(), this.localNodeAddress); + Assert.assertTrue(getDataRequest.getExcludedKeys().isEmpty()); + } + + // TESTCASE: P2PDataStorage with PersistableNetworkPayloads and ProtectedStorageEntry generates + // correct GetDataRequestMessage with both sets of keys. + @Test + public void buildPreliminaryGetDataRequest_FilledP2PDataStore() throws NoSuchAlgorithmException { + PersistableNetworkPayload toAdd1 = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload toAdd2 = new PersistableNetworkPayloadStub(new byte[] { 2 }); + ProtectedStorageEntry toAdd3 = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry toAdd4 = getProtectedStorageEntryForAdd(); + + this.testState.mockedStorage.addPersistableNetworkPayload( + toAdd1, this.localNodeAddress, false, false, false, false); + this.testState.mockedStorage.addPersistableNetworkPayload( + toAdd2, this.localNodeAddress, false, false, false, false); + + this.testState.mockedStorage.addProtectedStorageEntry(toAdd3, this.localNodeAddress, null, false); + this.testState.mockedStorage.addProtectedStorageEntry(toAdd4, this.localNodeAddress, null, false); + + PreliminaryGetDataRequest getDataRequest = this.testState.mockedStorage.buildPreliminaryGetDataRequest(1); + + Assert.assertEquals(getDataRequest.getNonce(), 1); + Assert.assertEquals(getDataRequest.getSupportedCapabilities(), Capabilities.app); + Assert.assertEquals(4, getDataRequest.getExcludedKeys().size()); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd1.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd2.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd3.getProtectedStoragePayload()))); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd4.getProtectedStoragePayload()))); + } + + // TESTCASE: P2PDataStorage with PersistableNetworkPayloads and ProtectedStorageEntry generates + // correct GetDataRequestMessage with both sets of keys. + @Test + public void requestData_FilledP2PDataStore_GetUpdatedDataRequest() throws NoSuchAlgorithmException { + PersistableNetworkPayload toAdd1 = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload toAdd2 = new PersistableNetworkPayloadStub(new byte[] { 2 }); + ProtectedStorageEntry toAdd3 = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry toAdd4 = getProtectedStorageEntryForAdd(); + + this.testState.mockedStorage.addPersistableNetworkPayload( + toAdd1, this.localNodeAddress, false, false, false, false); + this.testState.mockedStorage.addPersistableNetworkPayload( + toAdd2, this.localNodeAddress, false, false, false, false); + + this.testState.mockedStorage.addProtectedStorageEntry(toAdd3, this.localNodeAddress, null, false); + this.testState.mockedStorage.addProtectedStorageEntry(toAdd4, this.localNodeAddress, null, false); + + GetUpdatedDataRequest getDataRequest = + this.testState.mockedStorage.buildGetUpdatedDataRequest(this.localNodeAddress, 1); + + Assert.assertEquals(getDataRequest.getNonce(), 1); + Assert.assertEquals(getDataRequest.getSenderNodeAddress(), this.localNodeAddress); + Assert.assertEquals(4, getDataRequest.getExcludedKeys().size()); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd1.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd2.getHash())); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd3.getProtectedStoragePayload()))); + Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), + P2PDataStorage.get32ByteHash(toAdd4.getProtectedStoragePayload()))); + } +} From daffe6dc382f68b56ac89ebf20a9b0d1104ac5ce Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 20 Nov 2019 17:47:56 -0800 Subject: [PATCH 04/28] [TESTS] Add tests of GetDataRequestHandler Add some basic sanity tests prior to the refactor to help catch issues. --- .../getdata/GetDataRequestHandlerTest.java | 199 ++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java new file mode 100644 index 00000000000..10a7555e785 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java @@ -0,0 +1,199 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.peers.getdata; + +import bisq.network.p2p.NodeAddress; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.network.Connection; +import bisq.network.p2p.network.NetworkNode; +import bisq.network.p2p.peers.getdata.messages.GetDataResponse; +import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; +import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; +import bisq.network.p2p.storage.P2PDataStorage; +import bisq.network.p2p.storage.TestState; +import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import bisq.common.Proto; +import bisq.common.app.Capabilities; +import bisq.common.app.Capability; + +import com.google.common.util.concurrent.SettableFuture; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import static org.mockito.Mockito.*; + +public class GetDataRequestHandlerTest { + private TestState testState; + + @Mock + NetworkNode networkNode; + + private NodeAddress localNodeAddress; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + this.testState = new TestState(); + + this.localNodeAddress = new NodeAddress("localhost", 8080); + when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); + + // Set up basic capabilities to ensure message contains it. Ensure it is unique from other tests + // so we catch mismatch bugs. + Capabilities.app.addAll(Capability.DAO_FULL_NODE); + } + + /** + * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state + * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers + * is not supported. + */ + private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(1); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + // TESTCASE: Construct a request that requires excluding duplicates and adding missing entrys for + // PersistableNetworkPayloads and ProtectedStorageEntrys to verify the correct keys are added to the response. + @Test + public void handle_PreliminaryGetDataRequestTest() throws NoSuchAlgorithmException { + + // Construct a seed node w/ 2 PersistableNetworkPayloads and 2 ProtectedStorageEntrys and a PreliminaryGetDataRequest + // that only contains 1 known PersistableNetworkPayload and 1 known ProtectedStorageEntry as well as 2 unknowns + PersistableNetworkPayload pnp_onSeedNodeNotInRequest = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload pnp_onSeedNodeAndInRequest = new PersistableNetworkPayloadStub(new byte[] { 2 }); + PersistableNetworkPayload pnp_onlyInRequest = new PersistableNetworkPayloadStub(new byte[] { 3 }); + ProtectedStorageEntry pse_onSeedNodeNotInRequest = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry pse_onSeedNodeAndInRequest = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry pse_onlyInRequest = getProtectedStorageEntryForAdd(); + + this.testState.getMockedStorage().addPersistableNetworkPayload( + pnp_onSeedNodeNotInRequest, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addPersistableNetworkPayload( + pnp_onSeedNodeAndInRequest, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeNotInRequest, this.localNodeAddress, null, false); + this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeAndInRequest, this.localNodeAddress, null, false); + + SettableFuture sendFuture = mock(SettableFuture.class); + final ArgumentCaptor captor = ArgumentCaptor.forClass(GetDataResponse.class); + when(this.networkNode.sendMessage(any(Connection.class), captor.capture())).thenReturn(sendFuture); + + PreliminaryGetDataRequest getDataRequest = mock(PreliminaryGetDataRequest.class); + HashSet knownKeysSet = new HashSet<>(Arrays.asList( + pnp_onSeedNodeAndInRequest.getHash(), + pnp_onlyInRequest.getHash(), + P2PDataStorage.get32ByteHash(pse_onSeedNodeAndInRequest.getProtectedStoragePayload()), + P2PDataStorage.get32ByteHash(pse_onlyInRequest.getProtectedStoragePayload()))); + when(getDataRequest.getNonce()).thenReturn(1); + when(getDataRequest.getExcludedKeys()).thenReturn(knownKeysSet); + + Connection connection = mock(Connection.class); + when(connection.noCapabilityRequiredOrCapabilityIsSupported(any(Proto.class))).thenReturn(true); + + GetDataRequestHandler handler = + new GetDataRequestHandler(this.networkNode, this.testState.getMockedStorage(), null); + handler.handle(getDataRequest, connection); + + // Verify the request node is sent back only the 2 missing payloads + GetDataResponse getDataResponse = captor.getValue(); + Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); + Assert.assertFalse(getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getPersistableNetworkPayloadSet(), new HashSet<>(Collections.singletonList(pnp_onSeedNodeNotInRequest))); + Assert.assertEquals(getDataResponse.getDataSet(), new HashSet<>(Collections.singletonList(pse_onSeedNodeNotInRequest))); + } + + // TESTCASE: Same as above, but with an GetUpdatedDataRequest + @Test + public void handle_GetUpdatedDataRequestTest() throws NoSuchAlgorithmException { + + // Construct a seed node w/ 2 PersistableNetworkPayloads and 2 ProtectedStorageEntrys and a PreliminaryGetDataRequest + // that only contains 1 known PersistableNetworkPayload and 1 known ProtectedStorageEntry as well as 2 unknowns + PersistableNetworkPayload pnp_onSeedNodeNotInRequest = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload pnp_onSeedNodeAndInRequest = new PersistableNetworkPayloadStub(new byte[] { 2 }); + PersistableNetworkPayload pnp_onlyInRequest = new PersistableNetworkPayloadStub(new byte[] { 3 }); + ProtectedStorageEntry pse_onSeedNodeNotInRequest = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry pse_onSeedNodeAndInRequest = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry pse_onlyInRequest = getProtectedStorageEntryForAdd(); + + this.testState.getMockedStorage().addPersistableNetworkPayload( + pnp_onSeedNodeNotInRequest, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addPersistableNetworkPayload( + pnp_onSeedNodeAndInRequest, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeNotInRequest, this.localNodeAddress, null, false); + this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeAndInRequest, this.localNodeAddress, null, false); + + SettableFuture sendFuture = mock(SettableFuture.class); + final ArgumentCaptor captor = ArgumentCaptor.forClass(GetDataResponse.class); + when(this.networkNode.sendMessage(any(Connection.class), captor.capture())).thenReturn(sendFuture); + + GetUpdatedDataRequest getDataRequest = mock(GetUpdatedDataRequest.class); + HashSet knownKeysSet = new HashSet<>(Arrays.asList( + pnp_onSeedNodeAndInRequest.getHash(), + pnp_onlyInRequest.getHash(), + P2PDataStorage.get32ByteHash(pse_onSeedNodeAndInRequest.getProtectedStoragePayload()), + P2PDataStorage.get32ByteHash(pse_onlyInRequest.getProtectedStoragePayload()))); + when(getDataRequest.getNonce()).thenReturn(1); + when(getDataRequest.getExcludedKeys()).thenReturn(knownKeysSet); + + Connection connection = mock(Connection.class); + when(connection.noCapabilityRequiredOrCapabilityIsSupported(any(Proto.class))).thenReturn(true); + + GetDataRequestHandler handler = + new GetDataRequestHandler(this.networkNode, this.testState.getMockedStorage(), null); + handler.handle(getDataRequest, connection); + + // Verify the request node is sent back only the 2 missing payloads + GetDataResponse getDataResponse = captor.getValue(); + Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); + Assert.assertTrue(getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getPersistableNetworkPayloadSet(), new HashSet<>(Collections.singletonList(pnp_onSeedNodeNotInRequest))); + Assert.assertEquals(getDataResponse.getDataSet(), new HashSet<>(Collections.singletonList(pse_onSeedNodeNotInRequest))); + } +} From 944b3fffbc74ad3a30e4e65615ce38e529f791f9 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 20 Nov 2019 17:55:41 -0800 Subject: [PATCH 05/28] [REFACTOR] Introduce buildGetDataResponse This is just a strict move of code to reduce errors. --- .../peers/getdata/GetDataRequestHandler.java | 93 +------------------ .../network/p2p/storage/P2PDataStorage.java | 91 ++++++++++++++++++ 2 files changed, 92 insertions(+), 92 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java index 0972cf32c8c..c1aff5476a5 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java @@ -22,28 +22,16 @@ import bisq.network.p2p.network.NetworkNode; import bisq.network.p2p.peers.getdata.messages.GetDataRequest; import bisq.network.p2p.peers.getdata.messages.GetDataResponse; -import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; import bisq.network.p2p.storage.P2PDataStorage; -import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; -import bisq.network.p2p.storage.payload.PersistableNetworkPayload; -import bisq.network.p2p.storage.payload.ProtectedStorageEntry; -import bisq.network.p2p.storage.payload.ProtectedStoragePayload; import bisq.common.Timer; import bisq.common.UserThread; -import bisq.common.util.Utilities; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.SettableFuture; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.stream.Collectors; - import lombok.extern.slf4j.Slf4j; import org.jetbrains.annotations.NotNull; @@ -51,7 +39,6 @@ @Slf4j public class GetDataRequestHandler { private static final long TIMEOUT = 90; - private static final int MAX_ENTRIES = 10000; /////////////////////////////////////////////////////////////////////////////////////////// @@ -93,10 +80,7 @@ public GetDataRequestHandler(NetworkNode networkNode, P2PDataStorage dataStorage public void handle(GetDataRequest getDataRequest, final Connection connection) { long ts = System.currentTimeMillis(); - GetDataResponse getDataResponse = new GetDataResponse(getFilteredProtectedStorageEntries(getDataRequest, connection), - getFilteredPersistableNetworkPayload(getDataRequest, connection), - getDataRequest.getNonce(), - getDataRequest instanceof GetUpdatedDataRequest); + GetDataResponse getDataResponse = dataStorage.buildGetDataResponse(getDataRequest, connection); if (timeoutTimer == null) { timeoutTimer = UserThread.runAfter(() -> { // setup before sending to avoid race conditions @@ -136,81 +120,6 @@ public void onFailure(@NotNull Throwable throwable) { log.info("handle GetDataRequest took {} ms", System.currentTimeMillis() - ts); } - private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, - Connection connection) { - Set tempLookupSet = new HashSet<>(); - String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() - .map(e -> "node address " + e.getFullAddress()) - .orElseGet(() -> "connection UID " + connection.getUid()); - - Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); - AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); - Set result = dataStorage.getAppendOnlyDataStoreMap().entrySet().stream() - .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) - .filter(e -> maxSize.decrementAndGet() >= 0) - .map(Map.Entry::getValue) - .filter(connection::noCapabilityRequiredOrCapabilityIsSupported) - .filter(payload -> { - boolean notContained = tempLookupSet.add(new P2PDataStorage.ByteArray(payload.getHash())); - return notContained; - }) - .collect(Collectors.toSet()); - if (maxSize.get() <= 0) { - log.warn("The getData request from peer with {} caused too much PersistableNetworkPayload " + - "entries to get delivered. We limited the entries for the response to {} entries", - connectionInfo, MAX_ENTRIES); - } - log.info("The getData request from peer with {} contains {} PersistableNetworkPayload entries ", - connectionInfo, result.size()); - return result; - } - - private Set getFilteredProtectedStorageEntries(GetDataRequest getDataRequest, - Connection connection) { - Set filteredDataSet = new HashSet<>(); - Set lookupSet = new HashSet<>(); - String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() - .map(e -> "node address " + e.getFullAddress()) - .orElseGet(() -> "connection UID " + connection.getUid()); - - AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); - Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); - Set filteredSet = dataStorage.getMap().entrySet().stream() - .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) - .filter(e -> maxSize.decrementAndGet() >= 0) - .map(Map.Entry::getValue) - .collect(Collectors.toSet()); - if (maxSize.get() <= 0) { - log.warn("The getData request from peer with {} caused too much ProtectedStorageEntry " + - "entries to get delivered. We limited the entries for the response to {} entries", - connectionInfo, MAX_ENTRIES); - } - log.info("getFilteredProtectedStorageEntries " + filteredSet.size()); - - for (ProtectedStorageEntry protectedStorageEntry : filteredSet) { - final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); - boolean doAdd = false; - if (protectedStoragePayload instanceof CapabilityRequiringPayload) { - if (connection.getCapabilities().containsAll(((CapabilityRequiringPayload) protectedStoragePayload).getRequiredCapabilities())) - doAdd = true; - else - log.debug("We do not send the message to the peer because they do not support the required capability for that message type.\n" + - "storagePayload is: " + Utilities.toTruncatedString(protectedStoragePayload)); - } else { - doAdd = true; - } - if (doAdd) { - boolean notContained = lookupSet.add(protectedStoragePayload.hashCode()); - if (notContained) - filteredDataSet.add(protectedStorageEntry); - } - } - - log.info("The getData request from peer with {} contains {} ProtectedStorageEntry entries ", - connectionInfo, filteredDataSet.size()); - return filteredDataSet; - } - public void stop() { cleanup(); } 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 a66557ccd3c..64a71cb65b8 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -25,6 +25,8 @@ import bisq.network.p2p.network.NetworkNode; import bisq.network.p2p.peers.BroadcastHandler; import bisq.network.p2p.peers.Broadcaster; +import bisq.network.p2p.peers.getdata.messages.GetDataRequest; +import bisq.network.p2p.peers.getdata.messages.GetDataResponse; import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; import bisq.network.p2p.storage.messages.AddDataMessage; @@ -34,6 +36,7 @@ import bisq.network.p2p.storage.messages.RefreshOfferMessage; import bisq.network.p2p.storage.messages.RemoveDataMessage; import bisq.network.p2p.storage.messages.RemoveMailboxDataMessage; +import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; import bisq.network.p2p.storage.payload.DateTolerantPayload; import bisq.network.p2p.storage.payload.ExpirablePayload; import bisq.network.p2p.storage.payload.MailboxStoragePayload; @@ -90,6 +93,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; import lombok.EqualsAndHashCode; @@ -110,6 +114,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; + private static final int MAX_ENTRIES = 10000; + private final Broadcaster broadcaster; private final AppendOnlyDataStoreService appendOnlyDataStoreService; private final ProtectedDataStoreService protectedDataStoreService; @@ -219,6 +225,91 @@ private Set getKnownPayloadHashes() { return excludedKeys; } + /** + * Returns a GetDataResponse object that contains the Payloads known locally, but not remotely. + */ + public GetDataResponse buildGetDataResponse(GetDataRequest getDataRequest, Connection connection) { + return new GetDataResponse(getFilteredProtectedStorageEntries(getDataRequest, connection), + getFilteredPersistableNetworkPayload(getDataRequest, connection), + getDataRequest.getNonce(), + getDataRequest instanceof GetUpdatedDataRequest); + } + + private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, + Connection connection) { + Set tempLookupSet = new HashSet<>(); + String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() + .map(e -> "node address " + e.getFullAddress()) + .orElseGet(() -> "connection UID " + connection.getUid()); + + Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); + AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); + Set result = this.appendOnlyDataStoreService.getMap().entrySet().stream() + .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) + .filter(e -> maxSize.decrementAndGet() >= 0) + .map(Map.Entry::getValue) + .filter(connection::noCapabilityRequiredOrCapabilityIsSupported) + .filter(payload -> { + boolean notContained = tempLookupSet.add(new P2PDataStorage.ByteArray(payload.getHash())); + return notContained; + }) + .collect(Collectors.toSet()); + if (maxSize.get() <= 0) { + log.warn("The getData request from peer with {} caused too much PersistableNetworkPayload " + + "entries to get delivered. We limited the entries for the response to {} entries", + connectionInfo, MAX_ENTRIES); + } + log.info("The getData request from peer with {} contains {} PersistableNetworkPayload entries ", + connectionInfo, result.size()); + return result; + } + + private Set getFilteredProtectedStorageEntries(GetDataRequest getDataRequest, + Connection connection) { + Set filteredDataSet = new HashSet<>(); + Set lookupSet = new HashSet<>(); + String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() + .map(e -> "node address " + e.getFullAddress()) + .orElseGet(() -> "connection UID " + connection.getUid()); + + AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); + Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); + Set filteredSet = this.map.entrySet().stream() + .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) + .filter(e -> maxSize.decrementAndGet() >= 0) + .map(Map.Entry::getValue) + .collect(Collectors.toSet()); + if (maxSize.get() <= 0) { + log.warn("The getData request from peer with {} caused too much ProtectedStorageEntry " + + "entries to get delivered. We limited the entries for the response to {} entries", + connectionInfo, MAX_ENTRIES); + } + log.info("getFilteredProtectedStorageEntries " + filteredSet.size()); + + for (ProtectedStorageEntry protectedStorageEntry : filteredSet) { + final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); + boolean doAdd = false; + if (protectedStoragePayload instanceof CapabilityRequiringPayload) { + if (connection.getCapabilities().containsAll(((CapabilityRequiringPayload) protectedStoragePayload).getRequiredCapabilities())) + doAdd = true; + else + log.debug("We do not send the message to the peer because they do not support the required capability for that message type.\n" + + "storagePayload is: " + Utilities.toTruncatedString(protectedStoragePayload)); + } else { + doAdd = true; + } + if (doAdd) { + boolean notContained = lookupSet.add(protectedStoragePayload.hashCode()); + if (notContained) + filteredDataSet.add(protectedStorageEntry); + } + } + + log.info("The getData request from peer with {} contains {} ProtectedStorageEntry entries ", + connectionInfo, filteredDataSet.size()); + return filteredDataSet; + } + /////////////////////////////////////////////////////////////////////////////////////////// // API From 8208f7883716f61a20bd34ca18554f80b2dd5377 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 07:40:51 -0800 Subject: [PATCH 06/28] [REFACTOR] Extract connectionInfo String --- .../p2p/peers/getdata/GetDataRequestHandler.java | 6 +++++- .../bisq/network/p2p/storage/P2PDataStorage.java | 14 +++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java index c1aff5476a5..dc298541ff1 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java @@ -80,7 +80,11 @@ public GetDataRequestHandler(NetworkNode networkNode, P2PDataStorage dataStorage public void handle(GetDataRequest getDataRequest, final Connection connection) { long ts = System.currentTimeMillis(); - GetDataResponse getDataResponse = dataStorage.buildGetDataResponse(getDataRequest, connection); + String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() + .map(e -> "node address " + e.getFullAddress()) + .orElseGet(() -> "connection UID " + connection.getUid()); + + GetDataResponse getDataResponse = dataStorage.buildGetDataResponse(getDataRequest, connectionInfo, connection); if (timeoutTimer == null) { timeoutTimer = UserThread.runAfter(() -> { // setup before sending to avoid race conditions 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 64a71cb65b8..5facada9bd3 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -228,19 +228,17 @@ private Set getKnownPayloadHashes() { /** * Returns a GetDataResponse object that contains the Payloads known locally, but not remotely. */ - public GetDataResponse buildGetDataResponse(GetDataRequest getDataRequest, Connection connection) { - return new GetDataResponse(getFilteredProtectedStorageEntries(getDataRequest, connection), - getFilteredPersistableNetworkPayload(getDataRequest, connection), + public GetDataResponse buildGetDataResponse(GetDataRequest getDataRequest, String connectionInfo, Connection connection) { + return new GetDataResponse(getFilteredProtectedStorageEntries(getDataRequest, connectionInfo, connection), + getFilteredPersistableNetworkPayload(getDataRequest, connectionInfo, connection), getDataRequest.getNonce(), getDataRequest instanceof GetUpdatedDataRequest); } private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, + String connectionInfo, Connection connection) { Set tempLookupSet = new HashSet<>(); - String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() - .map(e -> "node address " + e.getFullAddress()) - .orElseGet(() -> "connection UID " + connection.getUid()); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); @@ -265,12 +263,10 @@ private Set getFilteredPersistableNetworkPayload(GetD } private Set getFilteredProtectedStorageEntries(GetDataRequest getDataRequest, + String connectionInfo, Connection connection) { Set filteredDataSet = new HashSet<>(); Set lookupSet = new HashSet<>(); - String connectionInfo = "connectionInfo" + connection.getPeersNodeAddressOptional() - .map(e -> "node address " + e.getFullAddress()) - .orElseGet(() -> "connection UID " + connection.getUid()); AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); From 54021551bf54e112202e379ad7dd2899aaac14e1 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 07:47:32 -0800 Subject: [PATCH 07/28] [REFACTOR] Extract getDataResponse logging Changed the log to reference getDataResponse instead of getData. Now that we might truncate the response, it ins't true that this is exactly what the peer asked. --- .../network/p2p/peers/getdata/GetDataRequestHandler.java | 5 +++++ .../main/java/bisq/network/p2p/storage/P2PDataStorage.java | 6 ------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java index dc298541ff1..91516c0989e 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java @@ -86,6 +86,11 @@ public void handle(GetDataRequest getDataRequest, final Connection connection) { GetDataResponse getDataResponse = dataStorage.buildGetDataResponse(getDataRequest, connectionInfo, connection); + log.info("The getDataResponse to peer with {} contains {} ProtectedStorageEntries and {} PersistableNetworkPayloads", + connectionInfo, + getDataResponse.getDataSet().size(), + getDataResponse.getPersistableNetworkPayloadSet().size()); + if (timeoutTimer == null) { timeoutTimer = UserThread.runAfter(() -> { // setup before sending to avoid race conditions String errorMessage = "A timeout occurred for getDataResponse " + 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 5facada9bd3..9ee971f5009 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -257,8 +257,6 @@ private Set getFilteredPersistableNetworkPayload(GetD "entries to get delivered. We limited the entries for the response to {} entries", connectionInfo, MAX_ENTRIES); } - log.info("The getData request from peer with {} contains {} PersistableNetworkPayload entries ", - connectionInfo, result.size()); return result; } @@ -280,7 +278,6 @@ private Set getFilteredProtectedStorageEntries(GetDataReq "entries to get delivered. We limited the entries for the response to {} entries", connectionInfo, MAX_ENTRIES); } - log.info("getFilteredProtectedStorageEntries " + filteredSet.size()); for (ProtectedStorageEntry protectedStorageEntry : filteredSet) { final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); @@ -300,9 +297,6 @@ private Set getFilteredProtectedStorageEntries(GetDataReq filteredDataSet.add(protectedStorageEntry); } } - - log.info("The getData request from peer with {} contains {} ProtectedStorageEntry entries ", - connectionInfo, filteredDataSet.size()); return filteredDataSet; } From a6e8868563a1487b19fa4740106ec7b835ace300 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 08:02:38 -0800 Subject: [PATCH 08/28] [REFACTOR] Extract truncation logging Move the logging that utilizes connection information into the request handler. Now, buildGetDataResponse just returns whether or not the list is truncated which will make it easier to test. --- .../peers/getdata/GetDataRequestHandler.java | 24 ++++++++++- .../network/p2p/storage/P2PDataStorage.java | 43 +++++++++++-------- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java index 91516c0989e..eb2b56a3f71 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java @@ -32,6 +32,8 @@ import com.google.common.util.concurrent.SettableFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + import lombok.extern.slf4j.Slf4j; import org.jetbrains.annotations.NotNull; @@ -40,6 +42,7 @@ public class GetDataRequestHandler { private static final long TIMEOUT = 90; + private static final int MAX_ENTRIES = 10000; /////////////////////////////////////////////////////////////////////////////////////////// // Listener @@ -84,7 +87,26 @@ public void handle(GetDataRequest getDataRequest, final Connection connection) { .map(e -> "node address " + e.getFullAddress()) .orElseGet(() -> "connection UID " + connection.getUid()); - GetDataResponse getDataResponse = dataStorage.buildGetDataResponse(getDataRequest, connectionInfo, connection); + AtomicBoolean outPersistableNetworkPayloadOutputTruncated = new AtomicBoolean(false); + AtomicBoolean outProtectedStoragePayloadOutputTruncated = new AtomicBoolean(false); + GetDataResponse getDataResponse = dataStorage.buildGetDataResponse( + getDataRequest, + MAX_ENTRIES, + outPersistableNetworkPayloadOutputTruncated, + outProtectedStoragePayloadOutputTruncated, + connection); + + if (outPersistableNetworkPayloadOutputTruncated.get()) { + log.warn("The getData request from peer with {} caused too much PersistableNetworkPayload " + + "entries to get delivered. We limited the entries for the response to {} entries", + connectionInfo, MAX_ENTRIES); + } + + if (outProtectedStoragePayloadOutputTruncated.get()) { + log.warn("The getData request from peer with {} caused too much ProtectedStorageEntry " + + "entries to get delivered. We limited the entries for the response to {} entries", + connectionInfo, MAX_ENTRIES); + } log.info("The getDataResponse to peer with {} contains {} ProtectedStorageEntries and {} PersistableNetworkPayloads", connectionInfo, 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 9ee971f5009..458e22442e2 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -93,6 +93,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; @@ -114,8 +115,6 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; - private static final int MAX_ENTRIES = 10000; - private final Broadcaster broadcaster; private final AppendOnlyDataStoreService appendOnlyDataStoreService; private final ProtectedDataStoreService protectedDataStoreService; @@ -228,20 +227,28 @@ private Set getKnownPayloadHashes() { /** * Returns a GetDataResponse object that contains the Payloads known locally, but not remotely. */ - public GetDataResponse buildGetDataResponse(GetDataRequest getDataRequest, String connectionInfo, Connection connection) { - return new GetDataResponse(getFilteredProtectedStorageEntries(getDataRequest, connectionInfo, connection), - getFilteredPersistableNetworkPayload(getDataRequest, connectionInfo, connection), + public GetDataResponse buildGetDataResponse( + GetDataRequest getDataRequest, + int maxEntriesPerType, + AtomicBoolean outPersistableNetworkPayloadOutputTruncated, + AtomicBoolean outProtectedStorageEntryOutputTruncated, + Connection connection) { + + return new GetDataResponse( + getFilteredProtectedStorageEntries(getDataRequest, maxEntriesPerType, outProtectedStorageEntryOutputTruncated, connection), + getFilteredPersistableNetworkPayload(getDataRequest, maxEntriesPerType, outPersistableNetworkPayloadOutputTruncated, connection), getDataRequest.getNonce(), getDataRequest instanceof GetUpdatedDataRequest); } private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, - String connectionInfo, + int maxEntries, + AtomicBoolean outPersistableNetworkPayloadOutputTruncated, Connection connection) { Set tempLookupSet = new HashSet<>(); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); - AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); + AtomicInteger maxSize = new AtomicInteger(maxEntries); Set result = this.appendOnlyDataStoreService.getMap().entrySet().stream() .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) .filter(e -> maxSize.decrementAndGet() >= 0) @@ -252,32 +259,30 @@ private Set getFilteredPersistableNetworkPayload(GetD return notContained; }) .collect(Collectors.toSet()); - if (maxSize.get() <= 0) { - log.warn("The getData request from peer with {} caused too much PersistableNetworkPayload " + - "entries to get delivered. We limited the entries for the response to {} entries", - connectionInfo, MAX_ENTRIES); - } + + if (maxSize.get() <= 0) + outPersistableNetworkPayloadOutputTruncated.set(true); + return result; } private Set getFilteredProtectedStorageEntries(GetDataRequest getDataRequest, - String connectionInfo, + int maxEntries, + AtomicBoolean outProtectedStorageEntryOutputTruncated, Connection connection) { Set filteredDataSet = new HashSet<>(); Set lookupSet = new HashSet<>(); - AtomicInteger maxSize = new AtomicInteger(MAX_ENTRIES); + AtomicInteger maxSize = new AtomicInteger(maxEntries); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); Set filteredSet = this.map.entrySet().stream() .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) .filter(e -> maxSize.decrementAndGet() >= 0) .map(Map.Entry::getValue) .collect(Collectors.toSet()); - if (maxSize.get() <= 0) { - log.warn("The getData request from peer with {} caused too much ProtectedStorageEntry " + - "entries to get delivered. We limited the entries for the response to {} entries", - connectionInfo, MAX_ENTRIES); - } + + if (maxSize.get() <= 0) + outProtectedStorageEntryOutputTruncated.set(true); for (ProtectedStorageEntry protectedStorageEntry : filteredSet) { final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); From dafc76200a67bf1ec68aac005d726e88dd40bd43 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 08:17:23 -0800 Subject: [PATCH 09/28] [REFACTOR] Pass peerCapabilities into buildGetDataResponse Remove the dependence on the connection object by having the handler pass in the peer's capabilities. This now allows unit testing of buildGetDataResponse without any connection dependencies. --- .../peers/getdata/GetDataRequestHandler.java | 2 +- .../network/p2p/storage/P2PDataStorage.java | 32 +++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java index eb2b56a3f71..bb2c4949f9f 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/GetDataRequestHandler.java @@ -94,7 +94,7 @@ public void handle(GetDataRequest getDataRequest, final Connection connection) { MAX_ENTRIES, outPersistableNetworkPayloadOutputTruncated, outProtectedStoragePayloadOutputTruncated, - connection); + connection.getCapabilities()); if (outPersistableNetworkPayloadOutputTruncated.get()) { log.warn("The getData request from peer with {} caused too much PersistableNetworkPayload " + 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 458e22442e2..db09ef94493 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -53,6 +53,7 @@ import bisq.common.Timer; import bisq.common.UserThread; +import bisq.common.app.Capabilities; import bisq.common.crypto.CryptoException; import bisq.common.crypto.Hash; import bisq.common.crypto.Sig; @@ -232,19 +233,36 @@ public GetDataResponse buildGetDataResponse( int maxEntriesPerType, AtomicBoolean outPersistableNetworkPayloadOutputTruncated, AtomicBoolean outProtectedStorageEntryOutputTruncated, - Connection connection) { + Capabilities peerCapabilities) { return new GetDataResponse( - getFilteredProtectedStorageEntries(getDataRequest, maxEntriesPerType, outProtectedStorageEntryOutputTruncated, connection), - getFilteredPersistableNetworkPayload(getDataRequest, maxEntriesPerType, outPersistableNetworkPayloadOutputTruncated, connection), + getFilteredProtectedStorageEntries(getDataRequest, maxEntriesPerType, outProtectedStorageEntryOutputTruncated, peerCapabilities), + getFilteredPersistableNetworkPayload(getDataRequest, maxEntriesPerType, outPersistableNetworkPayloadOutputTruncated, peerCapabilities), getDataRequest.getNonce(), getDataRequest instanceof GetUpdatedDataRequest); } + /** + * Returns true if a Payload should be transmit to a peer given the peer's supported capabilities. + */ + private boolean shouldTransmitPayloadToPeer(Capabilities peerCapabilities, NetworkPayload payload) { + + // Sanity check to ensure this isn't used outside P2PDataStorage + if (!(payload instanceof ProtectedStoragePayload || payload instanceof PersistableNetworkPayload)) + return false; + + // If the payload doesn't have a required capability, we should transmit it + if (!(payload instanceof CapabilityRequiringPayload)) + return true; + + // Otherwise, only transmit the Payload if the peer supports all capabilities required by the payload + return peerCapabilities.containsAll(((CapabilityRequiringPayload) payload).getRequiredCapabilities()); + } + private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, int maxEntries, AtomicBoolean outPersistableNetworkPayloadOutputTruncated, - Connection connection) { + Capabilities peerCapabilities) { Set tempLookupSet = new HashSet<>(); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); @@ -253,7 +271,7 @@ private Set getFilteredPersistableNetworkPayload(GetD .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) .filter(e -> maxSize.decrementAndGet() >= 0) .map(Map.Entry::getValue) - .filter(connection::noCapabilityRequiredOrCapabilityIsSupported) + .filter(persistableNetworkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, persistableNetworkPayload)) .filter(payload -> { boolean notContained = tempLookupSet.add(new P2PDataStorage.ByteArray(payload.getHash())); return notContained; @@ -269,7 +287,7 @@ private Set getFilteredPersistableNetworkPayload(GetD private Set getFilteredProtectedStorageEntries(GetDataRequest getDataRequest, int maxEntries, AtomicBoolean outProtectedStorageEntryOutputTruncated, - Connection connection) { + Capabilities peerCapabilities) { Set filteredDataSet = new HashSet<>(); Set lookupSet = new HashSet<>(); @@ -288,7 +306,7 @@ private Set getFilteredProtectedStorageEntries(GetDataReq final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); boolean doAdd = false; if (protectedStoragePayload instanceof CapabilityRequiringPayload) { - if (connection.getCapabilities().containsAll(((CapabilityRequiringPayload) protectedStoragePayload).getRequiredCapabilities())) + if (shouldTransmitPayloadToPeer(peerCapabilities, protectedStoragePayload)) doAdd = true; else log.debug("We do not send the message to the peer because they do not support the required capability for that message type.\n" + From 5630b3575594d2eae9fe5b728b8cef9b5eb9a7c8 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 10:08:31 -0800 Subject: [PATCH 10/28] [TESTS] Unit tests of buildGetDataResponse Write a full set of unit tests for buildGetDataResponse. This provides a safety net with additional refactoring work. --- ...2PDataStorageBuildGetDataResponseTest.java | 483 ++++++++++++++++++ 1 file changed, 483 insertions(+) create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java new file mode 100644 index 00000000000..fd9dea92bfd --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java @@ -0,0 +1,483 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage; + +import bisq.network.p2p.NodeAddress; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.network.NetworkNode; +import bisq.network.p2p.peers.getdata.messages.GetDataRequest; +import bisq.network.p2p.peers.getdata.messages.GetDataResponse; +import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; +import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; +import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; +import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; +import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import bisq.common.app.Capabilities; +import bisq.common.app.Capability; +import bisq.common.crypto.Sig; + +import com.google.protobuf.Message; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; + +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.when; +import static org.mockito.Mockito.withSettings; + + + +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +public class P2PDataStorageBuildGetDataResponseTest { + abstract static class P2PDataStorageBuildGetDataResponseTestBase { + // GIVEN null & non-null supportedCapabilities + private TestState testState; + + abstract GetDataRequest buildGetDataRequest(int nonce, Set knownKeys); + + @Mock + NetworkNode networkNode; + + private NodeAddress localNodeAddress; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + this.testState = new TestState(); + + this.localNodeAddress = new NodeAddress("localhost", 8080); + when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); + + // Set up basic capabilities to ensure message contains it + Capabilities.app.addAll(Capability.MEDIATION); + } + + static class RequiredCapabilitiesPNPStub extends PersistableNetworkPayloadStub + implements CapabilityRequiringPayload { + Capabilities capabilities; + + RequiredCapabilitiesPNPStub(Capabilities capabilities, byte[] hash) { + super(hash); + this.capabilities = capabilities; + } + + @Override + public Capabilities getRequiredCapabilities() { + return capabilities; + } + } + + /** + * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state + * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers + * is not supported. + */ + private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { + return getProtectedStorageEntryForAdd(null); + } + + private ProtectedStorageEntry getProtectedStorageEntryForAdd(Capabilities requiredCapabilities) + throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + // Payload stub + ProtectedStoragePayload protectedStoragePayload; + + if (requiredCapabilities == null) + protectedStoragePayload = mock(ProtectedStoragePayload.class); + else { + protectedStoragePayload = mock(ProtectedStoragePayload.class, + withSettings().extraInterfaces(CapabilityRequiringPayload.class)); + when(((CapabilityRequiringPayload) protectedStoragePayload).getRequiredCapabilities()) + .thenReturn(requiredCapabilities); + } + + Message messageMock = mock(Message.class); + when(messageMock.toByteArray()).thenReturn(Sig.getPublicKeyBytes(ownerKeys.getPublic())); + when(protectedStoragePayload.toProtoMessage()).thenReturn(messageMock); + + // Entry stub + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(1); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + // TESTCASE: Given a GetDataRequest w/ unknown PNP, nothing is sent back + @Test + public void buildGetDataResponse_unknownPNPDoNothing() { + PersistableNetworkPayload fromPeer = new PersistableNetworkPayloadStub(new byte[]{1}); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, new HashSet<>(Collections.singletonList(fromPeer.getHash()))); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/ known PNP, nothing is sent back + @Test + public void buildGetDataResponse_knownPNPDoNothing() { + PersistableNetworkPayload fromPeerAndLocal = new PersistableNetworkPayloadStub(new byte[]{1}); + + this.testState.mockedStorage.addPersistableNetworkPayload( + fromPeerAndLocal, this.localNodeAddress, false, false, false, false); + + GetDataRequest getDataRequest = + this.buildGetDataRequest( + 1, + new HashSet<>(Collections.singletonList(fromPeerAndLocal.getHash()))); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP, send it back + // XXXBUGXXX: Truncation return has off-by-one error + @Test + public void buildGetDataResponse_unknownPNPSendBack() { + PersistableNetworkPayload onlyLocal = new PersistableNetworkPayloadStub(new byte[]{1}); + + this.testState.mockedStorage.addPersistableNetworkPayload( + onlyLocal, this.localNodeAddress, false, false, false, false); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, new HashSet<>()); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().contains(onlyLocal)); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP, don't send more than truncation limit + @Test + public void buildGetDataResponse_unknownPNPSendBackTruncation() { + PersistableNetworkPayload onlyLocal1 = new PersistableNetworkPayloadStub(new byte[]{1}); + PersistableNetworkPayload onlyLocal2 = new PersistableNetworkPayloadStub(new byte[]{2}); + + this.testState.mockedStorage.addPersistableNetworkPayload( + onlyLocal1, this.localNodeAddress, false, false, false, false); + this.testState.mockedStorage.addPersistableNetworkPayload( + onlyLocal2, this.localNodeAddress, false, false, false, false); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, new HashSet<>()); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertTrue(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertEquals(1, getDataResponse.getPersistableNetworkPayloadSet().size()); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().contains(onlyLocal1)); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP, but missing required capabilities, nothing is sent back + @Test + public void buildGetDataResponse_unknownPNPCapabilitiesMismatchDontSendBack() { + PersistableNetworkPayload onlyLocal = + new RequiredCapabilitiesPNPStub(new Capabilities(Collections.singletonList(Capability.MEDIATION)), + new byte[]{1}); + + this.testState.mockedStorage.addPersistableNetworkPayload( + onlyLocal, this.localNodeAddress, false, false, false, false); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, new HashSet<>()); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP that requires capabilities (and they match) send it back + @Test + public void buildGetDataResponse_unknownPNPCapabilitiesMatch() { + PersistableNetworkPayload onlyLocal = + new RequiredCapabilitiesPNPStub(new Capabilities(Collections.singletonList(Capability.MEDIATION)), + new byte[]{1}); + + this.testState.mockedStorage.addPersistableNetworkPayload( + onlyLocal, this.localNodeAddress, false, false, false, false); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, new HashSet<>()); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(Collections.singletonList(Capability.MEDIATION)); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().contains(onlyLocal)); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/ unknown PSE, nothing is sent back + @Test + public void buildGetDataResponse_unknownPSEDoNothing() throws NoSuchAlgorithmException { + ProtectedStorageEntry fromPeer = getProtectedStorageEntryForAdd(); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, + new HashSet<>(Collections.singletonList( + P2PDataStorage.get32ByteHash(fromPeer.getProtectedStoragePayload())))); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/ known PSE, nothing is sent back + @Test + public void buildGetDataResponse_knownPSEDoNothing() throws NoSuchAlgorithmException { + ProtectedStorageEntry fromPeerAndLocal = getProtectedStorageEntryForAdd(); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, + new HashSet<>(Collections.singletonList( + P2PDataStorage.get32ByteHash(fromPeerAndLocal.getProtectedStoragePayload())))); + + this.testState.mockedStorage.addProtectedStorageEntry( + fromPeerAndLocal, this.localNodeAddress, null, false); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/o known PSE, send it back + // XXXBUGXXX: Truncation return has off-by-one error + @Test + public void buildGetDataResponse_unknownPSESendBack() throws NoSuchAlgorithmException { + ProtectedStorageEntry onlyLocal = getProtectedStorageEntryForAdd(); + + GetDataRequest getDataRequest = this.buildGetDataRequest(1, new HashSet<>()); + + this.testState.mockedStorage.addProtectedStorageEntry( + onlyLocal, this.localNodeAddress, null, false); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().contains(onlyLocal)); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP, don't send more than truncation limit + @Test + public void buildGetDataResponse_unknownPSESendBackTruncation() throws NoSuchAlgorithmException { + ProtectedStorageEntry onlyLocal1 = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry onlyLocal2 = getProtectedStorageEntryForAdd(); + + GetDataRequest getDataRequest = this.buildGetDataRequest(1, new HashSet<>()); + + this.testState.mockedStorage.addProtectedStorageEntry( + onlyLocal1, this.localNodeAddress, null, false); + this.testState.mockedStorage.addProtectedStorageEntry( + onlyLocal2, this.localNodeAddress, null, false); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertTrue(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertEquals(1, getDataResponse.getDataSet().size()); + Assert.assertTrue( + getDataResponse.getDataSet().contains(onlyLocal1) + || getDataResponse.getDataSet().contains(onlyLocal2)); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP, but missing required capabilities, nothing is sent back + @Test + public void buildGetDataResponse_unknownPSECapabilitiesMismatchDontSendBack() throws NoSuchAlgorithmException { + ProtectedStorageEntry onlyLocal = + getProtectedStorageEntryForAdd(new Capabilities(Collections.singletonList(Capability.MEDIATION))); + + this.testState.mockedStorage.addProtectedStorageEntry( + onlyLocal, this.localNodeAddress, null, false); + + GetDataRequest getDataRequest = this.buildGetDataRequest(1, new HashSet<>()); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().isEmpty()); + } + + // TESTCASE: Given a GetDataRequest w/o known PNP that requires capabilities (and they match) send it back + @Test + public void buildGetDataResponse_unknownPSECapabilitiesMatch() throws NoSuchAlgorithmException { + ProtectedStorageEntry onlyLocal = + getProtectedStorageEntryForAdd(new Capabilities(Collections.singletonList(Capability.MEDIATION))); + + this.testState.mockedStorage.addProtectedStorageEntry( + onlyLocal, this.localNodeAddress, null, false); + + GetDataRequest getDataRequest = + this.buildGetDataRequest(1, new HashSet<>()); + + AtomicBoolean outPNPTruncated = new AtomicBoolean(false); + AtomicBoolean outPSETruncated = new AtomicBoolean(false); + Capabilities peerCapabilities = new Capabilities(Collections.singletonList(Capability.MEDIATION)); + GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( + getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + + Assert.assertFalse(outPNPTruncated.get()); + Assert.assertFalse(outPSETruncated.get()); + Assert.assertEquals(1, getDataResponse.getRequestNonce()); + Assert.assertEquals(getDataRequest instanceof GetUpdatedDataRequest, getDataResponse.isGetUpdatedDataResponse()); + Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); + Assert.assertTrue(getDataResponse.getPersistableNetworkPayloadSet().isEmpty()); + Assert.assertTrue(getDataResponse.getDataSet().contains(onlyLocal)); + } + } + + public static class P2PDataStorageBuildGetDataResponseTestPreliminary extends P2PDataStorageBuildGetDataResponseTestBase { + + @Override + GetDataRequest buildGetDataRequest(int nonce, Set knownKeys) { + return new PreliminaryGetDataRequest(nonce, knownKeys); + } + } + + public static class P2PDataStorageBuildGetDataResponseTestUpdated extends P2PDataStorageBuildGetDataResponseTestBase { + + @Override + GetDataRequest buildGetDataRequest(int nonce, Set knownKeys) { + return new GetUpdatedDataRequest(new NodeAddress("peer", 10), nonce, knownKeys); + } + } +} From caf946dfe009c3500aef90cf61f1f4b6dc64c70a Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 10:13:20 -0800 Subject: [PATCH 11/28] Remove redundant HashSet lookups in filter functions The appendOnlyDataStoreService and map already have unique keys that are based on the hash of the payload. This would catch instances where: PersistableNetworkPayload - None: The key is based on ByteArray(payload.getHash()) which is the same as this check. ProtectedStorageEntry - Cases where multiple PSEs contain payloads that have equivalent hashCode(), but different data.toProtoMessage().toByteArray(). I don't think it is a good idea to keep 2 "unique" methods on payloads. This is likely left over from a time when Payload hashCode() needed to be different than the hash of the payload. --- .../java/bisq/network/p2p/storage/P2PDataStorage.java | 11 +---------- 1 file changed, 1 insertion(+), 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 db09ef94493..091d56788d8 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -263,8 +263,6 @@ private Set getFilteredPersistableNetworkPayload(GetD int maxEntries, AtomicBoolean outPersistableNetworkPayloadOutputTruncated, Capabilities peerCapabilities) { - Set tempLookupSet = new HashSet<>(); - Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); AtomicInteger maxSize = new AtomicInteger(maxEntries); Set result = this.appendOnlyDataStoreService.getMap().entrySet().stream() @@ -272,10 +270,6 @@ private Set getFilteredPersistableNetworkPayload(GetD .filter(e -> maxSize.decrementAndGet() >= 0) .map(Map.Entry::getValue) .filter(persistableNetworkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, persistableNetworkPayload)) - .filter(payload -> { - boolean notContained = tempLookupSet.add(new P2PDataStorage.ByteArray(payload.getHash())); - return notContained; - }) .collect(Collectors.toSet()); if (maxSize.get() <= 0) @@ -289,7 +283,6 @@ private Set getFilteredProtectedStorageEntries(GetDataReq AtomicBoolean outProtectedStorageEntryOutputTruncated, Capabilities peerCapabilities) { Set filteredDataSet = new HashSet<>(); - Set lookupSet = new HashSet<>(); AtomicInteger maxSize = new AtomicInteger(maxEntries); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); @@ -315,9 +308,7 @@ private Set getFilteredProtectedStorageEntries(GetDataReq doAdd = true; } if (doAdd) { - boolean notContained = lookupSet.add(protectedStoragePayload.hashCode()); - if (notContained) - filteredDataSet.add(protectedStorageEntry); + filteredDataSet.add(protectedStorageEntry); } } return filteredDataSet; From 703a9a0dddba77835d105c49b246eb18e58c71e3 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 10:17:01 -0800 Subject: [PATCH 12/28] [REFACTOR] Move required capabilities log Move the logging function to the common capabilities check so it can run on both ProtectedStoragePayload and PersistableNetworkPayload objects --- .../bisq/network/p2p/storage/P2PDataStorage.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 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 091d56788d8..063fd83ab11 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -256,7 +256,14 @@ private boolean shouldTransmitPayloadToPeer(Capabilities peerCapabilities, Netwo return true; // Otherwise, only transmit the Payload if the peer supports all capabilities required by the payload - return peerCapabilities.containsAll(((CapabilityRequiringPayload) payload).getRequiredCapabilities()); + boolean shouldTransmit = peerCapabilities.containsAll(((CapabilityRequiringPayload) payload).getRequiredCapabilities()); + + if (!shouldTransmit) { + log.debug("We do not send the message to the peer because they do not support the required capability for that message type.\n" + + "storagePayload is: " + Utilities.toTruncatedString(payload)); + } + + return shouldTransmit; } private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, @@ -301,9 +308,6 @@ private Set getFilteredProtectedStorageEntries(GetDataReq if (protectedStoragePayload instanceof CapabilityRequiringPayload) { if (shouldTransmitPayloadToPeer(peerCapabilities, protectedStoragePayload)) doAdd = true; - else - log.debug("We do not send the message to the peer because they do not support the required capability for that message type.\n" + - "storagePayload is: " + Utilities.toTruncatedString(protectedStoragePayload)); } else { doAdd = true; } From 3aaf8a285e61a9842e2127b669182c11f8d4317e Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 10:21:37 -0800 Subject: [PATCH 13/28] [REFACTOR] Inline capability check for ProtectedStorageEntries Move the capability check inside the stream operation. This should improve performance slightly, but more importantly it makes the two filter functions almost identical so they can be combined. --- .../network/p2p/storage/P2PDataStorage.java | 18 ++---------------- 1 file changed, 2 insertions(+), 16 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 063fd83ab11..4e2cceca806 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -289,33 +289,19 @@ private Set getFilteredProtectedStorageEntries(GetDataReq int maxEntries, AtomicBoolean outProtectedStorageEntryOutputTruncated, Capabilities peerCapabilities) { - Set filteredDataSet = new HashSet<>(); - AtomicInteger maxSize = new AtomicInteger(maxEntries); Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); Set filteredSet = this.map.entrySet().stream() .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) .filter(e -> maxSize.decrementAndGet() >= 0) .map(Map.Entry::getValue) + .filter(protectedStorageEntry -> shouldTransmitPayloadToPeer(peerCapabilities, protectedStorageEntry.getProtectedStoragePayload())) .collect(Collectors.toSet()); if (maxSize.get() <= 0) outProtectedStorageEntryOutputTruncated.set(true); - for (ProtectedStorageEntry protectedStorageEntry : filteredSet) { - final ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); - boolean doAdd = false; - if (protectedStoragePayload instanceof CapabilityRequiringPayload) { - if (shouldTransmitPayloadToPeer(peerCapabilities, protectedStoragePayload)) - doAdd = true; - } else { - doAdd = true; - } - if (doAdd) { - filteredDataSet.add(protectedStorageEntry); - } - } - return filteredDataSet; + return filteredSet; } From 4c5d8184b7761ea177cc52e2f60160178336073f Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 10:26:30 -0800 Subject: [PATCH 14/28] [REFACTOR] Inline filtering functions Removes unnecessary calculations converting Set into Set and allows additional deduplication of stream operations. --- .../network/p2p/storage/P2PDataStorage.java | 66 +++++++------------ 1 file changed, 25 insertions(+), 41 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 4e2cceca806..64296ffd1b2 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -235,9 +235,32 @@ public GetDataResponse buildGetDataResponse( AtomicBoolean outProtectedStorageEntryOutputTruncated, Capabilities peerCapabilities) { + Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); + AtomicInteger maxSizePersistableNetworkPayloads = new AtomicInteger(maxEntriesPerType); + Set filteredPersistableNetworkPayloads = this.appendOnlyDataStoreService.getMap().entrySet().stream() + .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) + .filter(e -> maxSizePersistableNetworkPayloads.decrementAndGet() >= 0) + .map(Map.Entry::getValue) + .filter(persistableNetworkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, persistableNetworkPayload)) + .collect(Collectors.toSet()); + + if (maxSizePersistableNetworkPayloads.get() <= 0) + outPersistableNetworkPayloadOutputTruncated.set(true); + + AtomicInteger maxSizeProtectedStorageEntries = new AtomicInteger(maxEntriesPerType); + Set filteredProtectedStorageEntries = this.map.entrySet().stream() + .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) + .filter(e -> maxSizeProtectedStorageEntries.decrementAndGet() >= 0) + .map(Map.Entry::getValue) + .filter(protectedStorageEntry -> shouldTransmitPayloadToPeer(peerCapabilities, protectedStorageEntry.getProtectedStoragePayload())) + .collect(Collectors.toSet()); + + if (maxSizeProtectedStorageEntries.get() <= 0) + outProtectedStorageEntryOutputTruncated.set(true); + return new GetDataResponse( - getFilteredProtectedStorageEntries(getDataRequest, maxEntriesPerType, outProtectedStorageEntryOutputTruncated, peerCapabilities), - getFilteredPersistableNetworkPayload(getDataRequest, maxEntriesPerType, outPersistableNetworkPayloadOutputTruncated, peerCapabilities), + filteredProtectedStorageEntries, + filteredPersistableNetworkPayloads, getDataRequest.getNonce(), getDataRequest instanceof GetUpdatedDataRequest); } @@ -266,45 +289,6 @@ private boolean shouldTransmitPayloadToPeer(Capabilities peerCapabilities, Netwo return shouldTransmit; } - private Set getFilteredPersistableNetworkPayload(GetDataRequest getDataRequest, - int maxEntries, - AtomicBoolean outPersistableNetworkPayloadOutputTruncated, - Capabilities peerCapabilities) { - Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); - AtomicInteger maxSize = new AtomicInteger(maxEntries); - Set result = this.appendOnlyDataStoreService.getMap().entrySet().stream() - .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) - .filter(e -> maxSize.decrementAndGet() >= 0) - .map(Map.Entry::getValue) - .filter(persistableNetworkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, persistableNetworkPayload)) - .collect(Collectors.toSet()); - - if (maxSize.get() <= 0) - outPersistableNetworkPayloadOutputTruncated.set(true); - - return result; - } - - private Set getFilteredProtectedStorageEntries(GetDataRequest getDataRequest, - int maxEntries, - AtomicBoolean outProtectedStorageEntryOutputTruncated, - Capabilities peerCapabilities) { - AtomicInteger maxSize = new AtomicInteger(maxEntries); - Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); - Set filteredSet = this.map.entrySet().stream() - .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) - .filter(e -> maxSize.decrementAndGet() >= 0) - .map(Map.Entry::getValue) - .filter(protectedStorageEntry -> shouldTransmitPayloadToPeer(peerCapabilities, protectedStorageEntry.getProtectedStoragePayload())) - .collect(Collectors.toSet()); - - if (maxSize.get() <= 0) - outProtectedStorageEntryOutputTruncated.set(true); - - return filteredSet; - } - - /////////////////////////////////////////////////////////////////////////////////////////// // API /////////////////////////////////////////////////////////////////////////////////////////// From e7673407f16b95363047f541682546638721ffa0 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 11:32:22 -0800 Subject: [PATCH 15/28] [REFACTOR] Remove duplication in filtering functions Introduce a generic function that can be used to filter Map or Map. Used to deduplicate the GetData code paths and ensure the logic is the same between the two payload types. --- .../network/p2p/storage/P2PDataStorage.java | 73 +++++++++++++------ 1 file changed, 50 insertions(+), 23 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 64296ffd1b2..2518e945d86 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -96,6 +96,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; import java.util.stream.Collectors; import lombok.EqualsAndHashCode; @@ -225,6 +226,34 @@ private Set getKnownPayloadHashes() { return excludedKeys; } + /** + * Generic function that can be used to filter a Map + * by a given set of keys and peer capabilities. + */ + static private Set filterKnownHashes( + Map toFilter, + Function objToPayload, + Set knownHashes, + Capabilities peerCapabilities, + int maxEntries, + AtomicBoolean outTruncated) { + + AtomicInteger limit = new AtomicInteger(maxEntries); + + Set filteredResults = toFilter.entrySet().stream() + .filter(e -> !knownHashes.contains(e.getKey())) + .filter(e -> limit.decrementAndGet() >= 0) + .map(Map.Entry::getValue) + .filter(networkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, + objToPayload.apply(networkPayload))) + .collect(Collectors.toSet()); + + if (limit.get() <= 0) + outTruncated.set(true); + + return filteredResults; + } + /** * Returns a GetDataResponse object that contains the Payloads known locally, but not remotely. */ @@ -235,28 +264,26 @@ public GetDataResponse buildGetDataResponse( AtomicBoolean outProtectedStorageEntryOutputTruncated, Capabilities peerCapabilities) { - Set excludedKeysAsByteArray = P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); - AtomicInteger maxSizePersistableNetworkPayloads = new AtomicInteger(maxEntriesPerType); - Set filteredPersistableNetworkPayloads = this.appendOnlyDataStoreService.getMap().entrySet().stream() - .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) - .filter(e -> maxSizePersistableNetworkPayloads.decrementAndGet() >= 0) - .map(Map.Entry::getValue) - .filter(persistableNetworkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, persistableNetworkPayload)) - .collect(Collectors.toSet()); - - if (maxSizePersistableNetworkPayloads.get() <= 0) - outPersistableNetworkPayloadOutputTruncated.set(true); - - AtomicInteger maxSizeProtectedStorageEntries = new AtomicInteger(maxEntriesPerType); - Set filteredProtectedStorageEntries = this.map.entrySet().stream() - .filter(e -> !excludedKeysAsByteArray.contains(e.getKey())) - .filter(e -> maxSizeProtectedStorageEntries.decrementAndGet() >= 0) - .map(Map.Entry::getValue) - .filter(protectedStorageEntry -> shouldTransmitPayloadToPeer(peerCapabilities, protectedStorageEntry.getProtectedStoragePayload())) - .collect(Collectors.toSet()); - - if (maxSizeProtectedStorageEntries.get() <= 0) - outProtectedStorageEntryOutputTruncated.set(true); + Set excludedKeysAsByteArray = + P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(getDataRequest.getExcludedKeys()); + + Set filteredPersistableNetworkPayloads = + filterKnownHashes( + this.appendOnlyDataStoreService.getMap(), + Function.identity(), + excludedKeysAsByteArray, + peerCapabilities, + maxEntriesPerType, + outPersistableNetworkPayloadOutputTruncated); + + Set filteredProtectedStorageEntries = + filterKnownHashes( + this.map, + ProtectedStorageEntry::getProtectedStoragePayload, + excludedKeysAsByteArray, + peerCapabilities, + maxEntriesPerType, + outProtectedStorageEntryOutputTruncated); return new GetDataResponse( filteredProtectedStorageEntries, @@ -268,7 +295,7 @@ public GetDataResponse buildGetDataResponse( /** * Returns true if a Payload should be transmit to a peer given the peer's supported capabilities. */ - private boolean shouldTransmitPayloadToPeer(Capabilities peerCapabilities, NetworkPayload payload) { + private static boolean shouldTransmitPayloadToPeer(Capabilities peerCapabilities, NetworkPayload payload) { // Sanity check to ensure this isn't used outside P2PDataStorage if (!(payload instanceof ProtectedStoragePayload || payload instanceof PersistableNetworkPayload)) From 00128d912df9b833ab715a4b0a85750c7603b226 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 11:37:55 -0800 Subject: [PATCH 16/28] [BUGFIX] Fix off-by-one in truncation logic Now, the truncation is only triggered if more than MAX_ENTRIES could have been returned. --- .../main/java/bisq/network/p2p/storage/P2PDataStorage.java | 2 +- .../p2p/storage/P2PDataStorageBuildGetDataResponseTest.java | 6 ++---- 2 files changed, 3 insertions(+), 5 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 2518e945d86..58f71b22c4c 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -248,7 +248,7 @@ static private Set filterKnownHashes( objToPayload.apply(networkPayload))) .collect(Collectors.toSet()); - if (limit.get() <= 0) + if (limit.get() < 0) outTruncated.set(true); return filteredResults; diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java index fd9dea92bfd..173d3358041 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageBuildGetDataResponseTest.java @@ -189,7 +189,6 @@ public void buildGetDataResponse_knownPNPDoNothing() { } // TESTCASE: Given a GetDataRequest w/o known PNP, send it back - // XXXBUGXXX: Truncation return has off-by-one error @Test public void buildGetDataResponse_unknownPNPSendBack() { PersistableNetworkPayload onlyLocal = new PersistableNetworkPayloadStub(new byte[]{1}); @@ -204,7 +203,7 @@ public void buildGetDataResponse_unknownPNPSendBack() { AtomicBoolean outPSETruncated = new AtomicBoolean(false); Capabilities peerCapabilities = new Capabilities(); GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( - getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); Assert.assertFalse(outPNPTruncated.get()); Assert.assertFalse(outPSETruncated.get()); @@ -355,7 +354,6 @@ public void buildGetDataResponse_knownPSEDoNothing() throws NoSuchAlgorithmExcep } // TESTCASE: Given a GetDataRequest w/o known PSE, send it back - // XXXBUGXXX: Truncation return has off-by-one error @Test public void buildGetDataResponse_unknownPSESendBack() throws NoSuchAlgorithmException { ProtectedStorageEntry onlyLocal = getProtectedStorageEntryForAdd(); @@ -369,7 +367,7 @@ public void buildGetDataResponse_unknownPSESendBack() throws NoSuchAlgorithmExce AtomicBoolean outPSETruncated = new AtomicBoolean(false); Capabilities peerCapabilities = new Capabilities(); GetDataResponse getDataResponse = this.testState.mockedStorage.buildGetDataResponse( - getDataRequest, 2, outPNPTruncated, outPSETruncated, peerCapabilities); + getDataRequest, 1, outPNPTruncated, outPSETruncated, peerCapabilities); Assert.assertFalse(outPNPTruncated.get()); Assert.assertFalse(outPSETruncated.get()); From c7bce9e999de2603beacf4ae99a73782814117d8 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 13:16:00 -0800 Subject: [PATCH 17/28] [TESTS] Add test of RequestDataHandler::onMessage Add heavy-handed test that exercises the logic to use as a safeguard for refactoring. --- .../getdata/GetDataRequestOnMessageTest.java | 160 ++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java new file mode 100644 index 00000000000..2ac1c25b884 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java @@ -0,0 +1,160 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.peers.getdata; + +import bisq.network.p2p.NodeAddress; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.network.Connection; +import bisq.network.p2p.network.NetworkNode; +import bisq.network.p2p.peers.PeerManager; +import bisq.network.p2p.peers.getdata.messages.GetDataRequest; +import bisq.network.p2p.peers.getdata.messages.GetDataResponse; +import bisq.network.p2p.storage.TestState; +import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import bisq.common.app.Capabilities; +import bisq.common.app.Capability; + +import com.google.common.util.concurrent.SettableFuture; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Optional; + +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.when; + + + +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +public class GetDataRequestOnMessageTest { + private TestState testState; + + @Mock + NetworkNode networkNode; + + private NodeAddress localNodeAddress; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + this.testState = new TestState(); + + this.localNodeAddress = new NodeAddress("localhost", 8080); + when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); + + // Set up basic capabilities to ensure message contains it. Ensure it is unique from other tests + // so we catch mismatch bugs. + Capabilities.app.addAll(Capability.DAO_FULL_NODE); + } + + /** + * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state + * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers + * is not supported. + */ + private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(1); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + static class LazyPersistableNetworkPayloadStub extends PersistableNetworkPayloadStub + implements LazyProcessedPayload { + + LazyPersistableNetworkPayloadStub(byte[] hash) { + super(hash); + } + } + + // TESTCASE: GetDataResponse processing. This large tests includes all interesting variations of state + @Test + public void onMessage_GetDataResponseTest() throws NoSuchAlgorithmException { + + PersistableNetworkPayload pnp_onLocalNodeOnly = new PersistableNetworkPayloadStub(new byte[] { 1 }); + PersistableNetworkPayload pnp_inRequestOnly = new LazyPersistableNetworkPayloadStub(new byte[]{2}); + PersistableNetworkPayload pnp_onLocalNodeAndRequest = new PersistableNetworkPayloadStub(new byte[] { 3 }); + ProtectedStorageEntry pse_onLocalNodeOnly = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry pse_inRequestOnly = getProtectedStorageEntryForAdd(); + ProtectedStorageEntry pse_onLocalNodeAndRequest = getProtectedStorageEntryForAdd(); + + this.testState.getMockedStorage().addPersistableNetworkPayload( + pnp_onLocalNodeOnly, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addPersistableNetworkPayload( + pnp_onLocalNodeAndRequest, this.localNodeAddress, false, false, false, false); + this.testState.getMockedStorage().addProtectedStorageEntry(pse_onLocalNodeOnly, this.localNodeAddress, null, false); + this.testState.getMockedStorage().addProtectedStorageEntry(pse_onLocalNodeAndRequest, this.localNodeAddress, null, false); + + RequestDataHandler handler = new RequestDataHandler( + this.networkNode, + this.testState.getMockedStorage(), + mock(PeerManager.class), + mock(RequestDataHandler.Listener.class) + ); + + GetDataResponse getDataResponse = new GetDataResponse( + new HashSet<>(Arrays.asList(pse_inRequestOnly, pse_onLocalNodeAndRequest)), + new HashSet<>(Arrays.asList(pnp_inRequestOnly, pnp_onLocalNodeAndRequest)), + handler.nonce, false); + + NodeAddress peersNodeAddress = new NodeAddress("peer", 10); + + // Make a request with the sole reason to set the peersNodeAddress + SettableFuture sendFuture = mock(SettableFuture.class); + when(networkNode.sendMessage(any(NodeAddress.class), any(GetDataRequest.class))).thenReturn(sendFuture); + handler.requestData(peersNodeAddress, true); + + Connection connection = mock(Connection.class); + when(connection.getPeersNodeAddressOptional()).thenReturn(Optional.of(peersNodeAddress)); + handler.onMessage(getDataResponse, connection); + + Assert.assertEquals(3, this.testState.getMockedStorage().getMap().size()); + Assert.assertTrue(this.testState.getMockedStorage().getAppendOnlyDataStoreMap().containsValue(pnp_onLocalNodeOnly)); + Assert.assertTrue(this.testState.getMockedStorage().getAppendOnlyDataStoreMap().containsValue(pnp_inRequestOnly)); + Assert.assertTrue(this.testState.getMockedStorage().getAppendOnlyDataStoreMap().containsValue(pnp_onLocalNodeAndRequest)); + + Assert.assertEquals(3, this.testState.getMockedStorage().getMap().size()); + Assert.assertTrue(this.testState.getMockedStorage().getMap().containsValue(pse_onLocalNodeOnly)); + Assert.assertTrue(this.testState.getMockedStorage().getMap().containsValue(pse_inRequestOnly)); + Assert.assertTrue(this.testState.getMockedStorage().getMap().containsValue(pse_onLocalNodeAndRequest)); + } +} From 873271c5ceb9664463adfa79b7439830a12608d8 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 13:30:42 -0800 Subject: [PATCH 18/28] [REFACTOR] Introduce processGetDataResponse Just a code move for now. --- .../p2p/peers/getdata/RequestDataHandler.java | 52 +----------------- .../network/p2p/storage/P2PDataStorage.java | 55 +++++++++++++++++++ 2 files changed, 57 insertions(+), 50 deletions(-) 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 10b1b7ae905..01c0f00361a 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 @@ -25,10 +25,7 @@ import bisq.network.p2p.peers.PeerManager; import bisq.network.p2p.peers.getdata.messages.GetDataRequest; import bisq.network.p2p.peers.getdata.messages.GetDataResponse; -import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; -import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; import bisq.network.p2p.storage.P2PDataStorage; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; @@ -48,7 +45,6 @@ import java.util.Map; import java.util.Random; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; import lombok.extern.slf4j.Slf4j; @@ -58,7 +54,6 @@ @Slf4j class RequestDataHandler implements MessageListener { private static final long TIMEOUT = 90; - private static boolean initialRequestApplied = false; private NodeAddress peersNodeAddress; /* @@ -193,7 +188,6 @@ public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) { GetDataResponse getDataResponse = (GetDataResponse) networkEnvelope; final Set dataSet = getDataResponse.getDataSet(); Set persistableNetworkPayloadSet = getDataResponse.getPersistableNetworkPayloadSet(); - logContents(networkEnvelope, dataSet, persistableNetworkPayloadSet); if (getDataResponse.getRequestNonce() == nonce) { @@ -204,50 +198,8 @@ public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) { return; } - final NodeAddress sender = connection.getPeersNodeAddressOptional().get(); - - long ts2 = System.currentTimeMillis(); - AtomicInteger counter = new AtomicInteger(); - dataSet.forEach(e -> { - // We don't broadcast here (last param) as we are only connected to the seed node and would be pointless - dataStorage.addProtectedStorageEntry(e, sender, null, false, false); - counter.getAndIncrement(); - - }); - log.info("Processing {} protectedStorageEntries took {} ms.", counter.get(), System.currentTimeMillis() - ts2); - - /* // engage the firstRequest logic only if we are a seed node. Normal clients get here twice at most. - if (!Capabilities.app.containsAll(Capability.SEED_NODE)) - firstRequest = true;*/ - - if (persistableNetworkPayloadSet != null /*&& firstRequest*/) { - ts2 = System.currentTimeMillis(); - persistableNetworkPayloadSet.forEach(e -> { - if (e instanceof LazyProcessedPayload) { - // We use an optimized method as many checks are not required in that case to avoid - // performance issues. - // Processing 82645 items took now 61 ms compared to earlier version where it took ages (> 2min). - // Usually we only get about a few hundred or max. a few 1000 items. 82645 is all - // trade stats stats and all account age witness data. - - // We only apply it once from first response - if (!initialRequestApplied) { - dataStorage.addPersistableNetworkPayloadFromInitialRequest(e); - - } - } else { - // We don't broadcast here as we are only connected to the seed node and would be pointless - dataStorage.addPersistableNetworkPayload(e, sender, false, - false, false, false); - } - }); - - // We set initialRequestApplied to true after the loop, otherwise we would only process 1 entry - initialRequestApplied = true; - - log.info("Processing {} persistableNetworkPayloads took {} ms.", - persistableNetworkPayloadSet.size(), System.currentTimeMillis() - ts2); - } + dataStorage.processGetDataResponse(getDataResponse, + connection.getPeersNodeAddressOptional().get()); cleanup(); listener.onComplete(); 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 58f71b22c4c..6a70f68ad26 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -39,6 +39,7 @@ import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; import bisq.network.p2p.storage.payload.DateTolerantPayload; import bisq.network.p2p.storage.payload.ExpirablePayload; +import bisq.network.p2p.storage.payload.LazyProcessedPayload; import bisq.network.p2p.storage.payload.MailboxStoragePayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedMailboxStorageEntry; @@ -117,6 +118,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; + private static boolean initialRequestApplied = false; + private final Broadcaster broadcaster; private final AppendOnlyDataStoreService appendOnlyDataStoreService; private final ProtectedDataStoreService protectedDataStoreService; @@ -316,6 +319,58 @@ private static boolean shouldTransmitPayloadToPeer(Capabilities peerCapabilities return shouldTransmit; } + /** + * Processes a GetDataResponse message and updates internal state. Does not broadcast updates to the P2P network + * or domain listeners. + */ + public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress sender) { + final Set dataSet = getDataResponse.getDataSet(); + Set persistableNetworkPayloadSet = getDataResponse.getPersistableNetworkPayloadSet(); + + long ts2 = System.currentTimeMillis(); + AtomicInteger counter = new AtomicInteger(); + dataSet.forEach(e -> { + // We don't broadcast here (last param) as we are only connected to the seed node and would be pointless + addProtectedStorageEntry(e, sender, null, false, false); + counter.getAndIncrement(); + + }); + log.info("Processing {} protectedStorageEntries took {} ms.", counter.get(), System.currentTimeMillis() - ts2); + + /* // engage the firstRequest logic only if we are a seed node. Normal clients get here twice at most. + if (!Capabilities.app.containsAll(Capability.SEED_NODE)) + firstRequest = true;*/ + + if (persistableNetworkPayloadSet != null /*&& firstRequest*/) { + ts2 = System.currentTimeMillis(); + persistableNetworkPayloadSet.forEach(e -> { + if (e instanceof LazyProcessedPayload) { + // We use an optimized method as many checks are not required in that case to avoid + // performance issues. + // Processing 82645 items took now 61 ms compared to earlier version where it took ages (> 2min). + // Usually we only get about a few hundred or max. a few 1000 items. 82645 is all + // trade stats stats and all account age witness data. + + // We only apply it once from first response + if (!initialRequestApplied) { + addPersistableNetworkPayloadFromInitialRequest(e); + + } + } else { + // We don't broadcast here as we are only connected to the seed node and would be pointless + addPersistableNetworkPayload(e, sender, false, + false, false, false); + } + }); + + // We set initialRequestApplied to true after the loop, otherwise we would only process 1 entry + initialRequestApplied = true; + + log.info("Processing {} persistableNetworkPayloads took {} ms.", + persistableNetworkPayloadSet.size(), System.currentTimeMillis() - ts2); + } + } + /////////////////////////////////////////////////////////////////////////////////////////// // API /////////////////////////////////////////////////////////////////////////////////////////// From 690b9808b148d2a7c108fb37f3f15ac67c246847 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 14:41:39 -0800 Subject: [PATCH 19/28] [TESTS] Make verify() functions more flexible Now that we want to unit test the GetData path which has different behavior w.r.t. broadcasts, the tests need a way to verify that state was updated, but not broadcast during an add. This patch changes all verification function to take each state update explicitly so the tests can do the proper verification. --- .../storage/P2PDataStorageClientAPITest.java | 6 +- ...aStoragePersistableNetworkPayloadTest.java | 4 +- ...PDataStorageProtectedStorageEntryTest.java | 8 ++- .../bisq/network/p2p/storage/TestState.java | 62 +++++++++---------- 4 files changed, 44 insertions(+), 36 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 4e0cb6c895f..7126b61770a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java @@ -73,7 +73,7 @@ public void getProtectedStorageEntry_NoExist() throws NoSuchAlgorithmException, SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, TestState.getTestNodeAddress(), null, true)); - this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, true, true); + this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, true, true, true, true, true); } // TESTCASE: Adding an entry from the getProtectedStorageEntry API of an existing item correctly updates the item @@ -90,7 +90,7 @@ public void getProtectedStorageEntry() throws NoSuchAlgorithmException, CryptoEx protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, TestState.getTestNodeAddress(), null, true); - this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, true, true); + this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, true, true, true, true, true); } // TESTCASE: Adding an entry from the getProtectedStorageEntry API of an existing item (added from onMessage path) correctly updates the item @@ -110,7 +110,7 @@ public void getProtectedStorageEntry_FirstOnMessageSecondAPI() throws NoSuchAlgo protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, TestState.getTestNodeAddress(), null, true)); - this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, true, true); + this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, true, true, true, true, true); } // TESTCASE: Updating an entry from the getRefreshTTLMessage API correctly errors if the item hasn't been seen 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 9213bcb0fbe..45c1ce17689 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java @@ -107,7 +107,9 @@ void doAddAndVerify(PersistableNetworkPayload persistableNetworkPayload, boolean testState.mockedStorage.onMessage(new AddPersistableNetworkPayloadMessage(persistableNetworkPayload), mockedConnection); } - this.testState.verifyPersistableAdd(beforeState, persistableNetworkPayload, expectedStateChange, this.expectBroadcastOnStateChange(), this.expectedIsDataOwner()); + boolean expectedBroadcast = expectedStateChange && this.expectBroadcastOnStateChange(); + + this.testState.verifyPersistableAdd(beforeState, persistableNetworkPayload, expectedStateChange, expectedBroadcast, expectedBroadcast, this.expectedIsDataOwner()); } @Before 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 9f5bd539234..04eb291f309 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -197,7 +197,13 @@ void doProtectedStorageAddAndVerify(ProtectedStorageEntry protectedStorageEntry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - this.testState.verifyProtectedStorageAdd(beforeState, protectedStorageEntry, expectedStateChange, this.expectIsDataOwner()); + if (expectedStateChange) { + this.testState.verifyProtectedStorageAdd( + beforeState, protectedStorageEntry, true, true, true, true, this.expectIsDataOwner()); + } else{ + this.testState.verifyProtectedStorageAdd( + beforeState, protectedStorageEntry, false, false, false, false, this.expectIsDataOwner()); + } } void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, diff --git a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java index 88d465c5734..15e80a9c6ac 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -174,52 +174,55 @@ private void verifySequenceNumberMapWriteContains(P2PDataStorage.ByteArray paylo void verifyPersistableAdd(SavedTestState beforeState, PersistableNetworkPayload persistableNetworkPayload, - boolean expectedStateChange, - boolean expectedBroadcastAndListenersSignaled, + boolean expectedHashMapAndDataStoreUpdated, + boolean expectedListenersSignaled, + boolean expectedBroadcast, boolean expectedIsDataOwner) { P2PDataStorage.ByteArray hash = new P2PDataStorage.ByteArray(persistableNetworkPayload.getHash()); - if (expectedStateChange) { - // Payload is accessible from get() + if (expectedHashMapAndDataStoreUpdated) Assert.assertEquals(persistableNetworkPayload, this.mockedStorage.getAppendOnlyDataStoreMap().get(hash)); - } else { - // On failure, just ensure the state remained the same as before the add - if (beforeState.persistableNetworkPayloadBeforeOp != null) - Assert.assertEquals(beforeState.persistableNetworkPayloadBeforeOp, this.mockedStorage.getAppendOnlyDataStoreMap().get(hash)); - else - Assert.assertNull(this.mockedStorage.getAppendOnlyDataStoreMap().get(hash)); - } - - if (expectedStateChange && expectedBroadcastAndListenersSignaled) { - // Broadcast Called - verify(this.mockBroadcaster).broadcast(any(AddPersistableNetworkPayloadMessage.class), any(NodeAddress.class), - eq(null), eq(expectedIsDataOwner)); + else + Assert.assertEquals(beforeState.persistableNetworkPayloadBeforeOp, this.mockedStorage.getAppendOnlyDataStoreMap().get(hash)); - // Verify the listeners were updated once + if (expectedListenersSignaled) verify(this.appendOnlyDataStoreListener).onAdded(persistableNetworkPayload); + else + verify(this.appendOnlyDataStoreListener, never()).onAdded(persistableNetworkPayload); - } else { + if (expectedBroadcast) + verify(this.mockBroadcaster).broadcast(any(AddPersistableNetworkPayloadMessage.class), any(NodeAddress.class), + eq(null), eq(expectedIsDataOwner)); + else verify(this.mockBroadcaster, never()).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), any(BroadcastHandler.Listener.class), anyBoolean()); - - // Verify the listeners were never updated - verify(this.appendOnlyDataStoreListener, never()).onAdded(persistableNetworkPayload); - } } void verifyProtectedStorageAdd(SavedTestState beforeState, ProtectedStorageEntry protectedStorageEntry, - boolean expectedStateChange, + boolean expectedHashMapAndDataStoreUpdated, + boolean expectedListenersSignaled, + boolean expectedBroadcast, + boolean expectedSequenceNrMapWrite, boolean expectedIsDataOwner) { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); - if (expectedStateChange) { + if (expectedHashMapAndDataStoreUpdated) { Assert.assertEquals(protectedStorageEntry, this.mockedStorage.getMap().get(hashMapHash)); if (protectedStorageEntry.getProtectedStoragePayload() instanceof PersistablePayload) Assert.assertEquals(protectedStorageEntry, this.protectedDataStoreService.getMap().get(hashMapHash)); + } else { + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().get(hashMapHash)); + Assert.assertEquals(beforeState.protectedStorageEntryBeforeOpDataStoreMap, this.protectedDataStoreService.getMap().get(hashMapHash)); + } + if (expectedListenersSignaled) { verify(this.hashMapChangedListener).onAdded(Collections.singletonList(protectedStorageEntry)); + } else { + verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); + } + if (expectedBroadcast) { final ArgumentCaptor captor = ArgumentCaptor.forClass(BroadcastMessage.class); verify(this.mockBroadcaster).broadcast(captor.capture(), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); @@ -227,16 +230,13 @@ void verifyProtectedStorageAdd(SavedTestState beforeState, BroadcastMessage broadcastMessage = captor.getValue(); Assert.assertTrue(broadcastMessage instanceof AddDataMessage); Assert.assertEquals(protectedStorageEntry, ((AddDataMessage) broadcastMessage).getProtectedStorageEntry()); - - this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); } else { - Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, this.mockedStorage.getMap().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()); + } - // Internal state didn't change... nothing should be notified - verify(this.hashMapChangedListener, never()).onAdded(Collections.singletonList(protectedStorageEntry)); + if (expectedSequenceNrMapWrite) { + this.verifySequenceNumberMapWriteContains(P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); + } else { verify(this.mockSeqNrStorage, never()).queueUpForSave(any(SequenceNumberMap.class), anyLong()); } } From a34488b735180fca4999a39f3c0c99f52def840d Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 15:12:52 -0800 Subject: [PATCH 20/28] [TESTS] Add unit tests for processGetDataResponse Add a full set of unit tests that uncovered some unexpected behavior w.r.t. signalers. --- .../network/p2p/storage/P2PDataStorage.java | 4 +- .../P2PDataStorageProcessGetDataResponse.java | 276 ++++++++++++++++++ 2 files changed, 279 insertions(+), 1 deletion(-) create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 6a70f68ad26..fca70ae96ad 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -118,7 +118,9 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; - private static boolean initialRequestApplied = false; + // TODO: Remove static + @VisibleForTesting + static boolean initialRequestApplied = false; private final Broadcaster broadcaster; private final AppendOnlyDataStoreService appendOnlyDataStoreService; diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java new file mode 100644 index 00000000000..37b76542fd5 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java @@ -0,0 +1,276 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage; + +import bisq.network.p2p.NodeAddress; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.peers.getdata.messages.GetDataResponse; +import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; + +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.when; + +import org.mockito.MockitoAnnotations; + +public class P2PDataStorageProcessGetDataResponse { + private TestState testState; + + private NodeAddress peerNodeAddress; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + this.testState = new TestState(); + P2PDataStorage.initialRequestApplied = false; + + this.peerNodeAddress = new NodeAddress("peer", 8080); + } + + static private GetDataResponse buildGetDataResponse(PersistableNetworkPayload persistableNetworkPayload) { + return buildGetDataResponse(Collections.emptyList(), Collections.singletonList(persistableNetworkPayload)); + } + + static private GetDataResponse buildGetDataResponse(ProtectedStorageEntry protectedStorageEntry) { + return buildGetDataResponse(Collections.singletonList(protectedStorageEntry), Collections.emptyList()); + } + + static private GetDataResponse buildGetDataResponse( + List protectedStorageEntries, + List persistableNetworkPayloads) { + return new GetDataResponse( + new HashSet<>(protectedStorageEntries), + persistableNetworkPayloads == null ? null : new HashSet<>(persistableNetworkPayloads), + 1, + false); + } + + /** + * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state + * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers + * is not supported. + */ + private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(1); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + static class LazyPersistableNetworkPayloadStub extends PersistableNetworkPayloadStub + implements LazyProcessedPayload { + + LazyPersistableNetworkPayloadStub(byte[] hash) { + super(hash); + } + + LazyPersistableNetworkPayloadStub(boolean validHashSize) { + super(validHashSize); + } + } + + // TESTCASE: GetDataResponse w/ missing PNP is added with no broadcast or listener signal + // XXXBUGXXX: We signal listeners w/ non LazyProcessedPayloads + @Test + public void processGetDataResponse_newPNPUpdatesState() { + PersistableNetworkPayload persistableNetworkPayload = new PersistableNetworkPayloadStub(new byte[] { 1 }); + + GetDataResponse getDataResponse = buildGetDataResponse(persistableNetworkPayload); + + TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, persistableNetworkPayload, true, true, false, false); + } + + // TESTCASE: GetDataResponse w/ invalid PNP does nothing (LazyProcessed) + @Test + public void processGetDataResponse_newInvalidPNPDoesNothing() { + PersistableNetworkPayload persistableNetworkPayload = new LazyPersistableNetworkPayloadStub(false); + + GetDataResponse getDataResponse = buildGetDataResponse(persistableNetworkPayload); + + TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, persistableNetworkPayload, false, false, false, false); + } + + // TESTCASE: GetDataResponse w/ existing PNP changes no state + @Test + public void processGetDataResponse_duplicatePNPDoesNothing() { + PersistableNetworkPayload persistableNetworkPayload = new PersistableNetworkPayloadStub(new byte[] { 1 }); + this.testState.mockedStorage.addPersistableNetworkPayload(persistableNetworkPayload, + this.peerNodeAddress, false, false, false, false); + + GetDataResponse getDataResponse = buildGetDataResponse(persistableNetworkPayload); + + TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, persistableNetworkPayload, false, false, false, false); + } + + // TESTCASE: GetDataResponse w/ missing PNP is added with no broadcast or listener signal (LazyProcessedPayload) + @Test + public void processGetDataResponse_newPNPUpdatesState_LazyProcessed() { + PersistableNetworkPayload persistableNetworkPayload = new LazyPersistableNetworkPayloadStub(new byte[] { 1 }); + + GetDataResponse getDataResponse = buildGetDataResponse(persistableNetworkPayload); + + TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, persistableNetworkPayload, true, false, false, false); + } + + // TESTCASE: GetDataResponse w/ existing PNP changes no state (LazyProcessedPayload) + @Test + public void processGetDataResponse_duplicatePNPDoesNothing_LazyProcessed() { + PersistableNetworkPayload persistableNetworkPayload = new LazyPersistableNetworkPayloadStub(new byte[] { 1 }); + this.testState.mockedStorage.addPersistableNetworkPayload(persistableNetworkPayload, + this.peerNodeAddress, false, false, false, false); + + GetDataResponse getDataResponse = buildGetDataResponse(persistableNetworkPayload); + + TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, persistableNetworkPayload, false, false, false, false); + } + + // TESTCASE: GetDataResponse w/ null PNP changes no state + @Test + public void processGetDataResponse_nullPNPSetDoesNothing() { + PersistableNetworkPayload persistableNetworkPayload = new PersistableNetworkPayloadStub(new byte[] { 1 }); + GetDataResponse getDataResponse = buildGetDataResponse(Collections.emptyList(), null); + + TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, persistableNetworkPayload, false, false, false, false); + } + + // TESTCASE: Second call to processGetDataResponse adds PNP for non-LazyProcessedPayloads + @Test + public void processGetDataResponse_secondProcessNewPNPUpdatesState() { + PersistableNetworkPayload addFromFirstProcess = new PersistableNetworkPayloadStub(new byte[] { 1 }); + GetDataResponse getDataResponse = buildGetDataResponse(addFromFirstProcess); + + TestState.SavedTestState beforeState = this.testState.saveTestState(addFromFirstProcess); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, addFromFirstProcess, true, true, false, false); + + PersistableNetworkPayload addFromSecondProcess = new PersistableNetworkPayloadStub(new byte[] { 2 }); + getDataResponse = buildGetDataResponse(addFromSecondProcess); + beforeState = this.testState.saveTestState(addFromSecondProcess); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, addFromSecondProcess, true, true, false, false); + } + + // TESTCASE: Second call to processGetDataResponse does not add any PNP (LazyProcessed) + @Test + public void processGetDataResponse_secondProcessNoPNPUpdates_LazyProcessed() { + PersistableNetworkPayload addFromFirstProcess = new LazyPersistableNetworkPayloadStub(new byte[] { 1 }); + GetDataResponse getDataResponse = buildGetDataResponse(addFromFirstProcess); + + TestState.SavedTestState beforeState = this.testState.saveTestState(addFromFirstProcess); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, addFromFirstProcess, true, false, false, false); + + PersistableNetworkPayload addFromSecondProcess = new LazyPersistableNetworkPayloadStub(new byte[] { 2 }); + getDataResponse = buildGetDataResponse(addFromSecondProcess); + beforeState = this.testState.saveTestState(addFromSecondProcess); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyPersistableAdd( + beforeState, addFromSecondProcess, false, false, false, false); + } + + // TESTCASE: GetDataResponse w/ missing PSE is added with no broadcast or listener signal + // XXXBUGXXX: We signal listeners for all ProtectedStorageEntrys + @Test + public void processGetDataResponse_newPSEUpdatesState() throws NoSuchAlgorithmException { + ProtectedStorageEntry protectedStorageEntry = getProtectedStorageEntryForAdd(); + GetDataResponse getDataResponse = buildGetDataResponse(protectedStorageEntry); + + TestState.SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyProtectedStorageAdd( + beforeState, protectedStorageEntry, true, true, false, true, false); + } + + // TESTCASE: GetDataResponse w/ existing PSE changes no state + @Test + public void processGetDataResponse_duplicatePSEDoesNothing() throws NoSuchAlgorithmException { + ProtectedStorageEntry protectedStorageEntry = getProtectedStorageEntryForAdd(); + this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, this.peerNodeAddress, null, false); + + GetDataResponse getDataResponse = buildGetDataResponse(protectedStorageEntry); + + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + TestState.SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); + this.testState.verifyProtectedStorageAdd( + beforeState, protectedStorageEntry, false, false, false, false, false); + } + + // TESTCASE: GetDataResponse w/ missing PSE is added with no broadcast or listener signal + // XXXBUGXXX: We signal listeners for all ProtectedStorageEntrys + @Test + public void processGetDataResponse_secondCallNewPSEUpdatesState() throws NoSuchAlgorithmException { + ProtectedStorageEntry protectedStorageEntry = getProtectedStorageEntryForAdd(); + GetDataResponse getDataResponse = buildGetDataResponse(protectedStorageEntry); + + TestState.SavedTestState beforeState = this.testState.saveTestState(protectedStorageEntry); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyProtectedStorageAdd( + beforeState, protectedStorageEntry, true, true, false, true, false); + + protectedStorageEntry = getProtectedStorageEntryForAdd(); + getDataResponse = buildGetDataResponse(protectedStorageEntry); + beforeState = this.testState.saveTestState(protectedStorageEntry); + this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); + this.testState.verifyProtectedStorageAdd( + beforeState, protectedStorageEntry, true, true, false, true, false); + } +} From 3d6e9fbef587ceaf0691808bc96db158f9029248 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 15:14:19 -0800 Subject: [PATCH 21/28] Remove static from initialRequestApplied Previously, multiple handlers needed to signal off one global variable. Now, that this check is inside the singleton P2PDataStorage, make it non-static and private. --- .../main/java/bisq/network/p2p/storage/P2PDataStorage.java | 4 +--- .../p2p/storage/P2PDataStorageProcessGetDataResponse.java | 1 - 2 files changed, 1 insertion(+), 4 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 fca70ae96ad..786a236cb8e 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -118,9 +118,7 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; - // TODO: Remove static - @VisibleForTesting - static boolean initialRequestApplied = false; + private boolean initialRequestApplied = false; private final Broadcaster broadcaster; private final AppendOnlyDataStoreService appendOnlyDataStoreService; diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java index 37b76542fd5..c6b349183b5 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java @@ -52,7 +52,6 @@ public class P2PDataStorageProcessGetDataResponse { public void setUp() { MockitoAnnotations.initMocks(this); this.testState = new TestState(); - P2PDataStorage.initialRequestApplied = false; this.peerNodeAddress = new NodeAddress("peer", 8080); } From f92893b097add8fb67c357893e3294d724f0c441 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 21 Nov 2019 15:43:12 -0800 Subject: [PATCH 22/28] [TESTS] Write synchronization integration tests Write a few integration test that exercises the exercise interesting synchronization states including the lost remove bug. This fails with the proper validation, but will pass at the end of the new feature development. --- .../P2PDataStorageGetDataIntegrationTest.java | 193 ++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageGetDataIntegrationTest.java diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageGetDataIntegrationTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageGetDataIntegrationTest.java new file mode 100644 index 00000000000..3b40575c633 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageGetDataIntegrationTest.java @@ -0,0 +1,193 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage; + +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.peers.getdata.messages.GetDataRequest; +import bisq.network.p2p.peers.getdata.messages.GetDataResponse; +import bisq.network.p2p.storage.mocks.PersistableExpirableProtectedStoragePayloadStub; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.network.p2p.storage.payload.ProtectedStorageEntry; +import bisq.network.p2p.storage.payload.ProtectedStoragePayload; + +import bisq.common.app.Capabilities; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; +import java.security.PublicKey; + +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.Assert; +import org.junit.Test; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class P2PDataStorageGetDataIntegrationTest { + + /** + * Generates a unique ProtectedStorageEntry that is valid for add and remove. + */ + private ProtectedStorageEntry getProtectedStorageEntry() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + + return getProtectedStorageEntry( + ownerKeys.getPublic(), new ProtectedStoragePayloadStub(ownerKeys.getPublic()), 1); + } + + private ProtectedStorageEntry getProtectedStorageEntry( + PublicKey ownerPubKey, + ProtectedStoragePayload protectedStoragePayload, + int sequenceNumber) { + ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); + when(stub.getOwnerPubKey()).thenReturn(ownerPubKey); + when(stub.isValidForAddOperation()).thenReturn(true); + when(stub.isValidForRemoveOperation()).thenReturn(true); + when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); + when(stub.getSequenceNumber()).thenReturn(sequenceNumber); + when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); + + return stub; + } + + // TESTCASE: Basic synchronization of a ProtectedStorageEntry works between a seed node and client node + @Test + public void basicSynchronizationWorks() throws NoSuchAlgorithmException { + TestState seedNodeTestState = new TestState(); + P2PDataStorage seedNode = seedNodeTestState.mockedStorage; + + TestState clientNodeTestState = new TestState(); + P2PDataStorage clientNode = clientNodeTestState.mockedStorage; + + ProtectedStorageEntry onSeedNode = getProtectedStorageEntry(); + seedNode.addProtectedStorageEntry(onSeedNode, null, null, false); + + GetDataRequest getDataRequest = clientNode.buildPreliminaryGetDataRequest(1); + + GetDataResponse getDataResponse = seedNode.buildGetDataResponse( + getDataRequest, 1, new AtomicBoolean(), new AtomicBoolean(), new Capabilities()); + + TestState.SavedTestState beforeState = clientNodeTestState.saveTestState(onSeedNode); + clientNode.processGetDataResponse(getDataResponse, null); + + clientNodeTestState.verifyProtectedStorageAdd( + beforeState, onSeedNode, true, true, false, true, false); + } + + // TESTCASE: Synchronization after peer restart works for in-memory ProtectedStorageEntrys + @Test + public void basicSynchronizationWorksAfterRestartTransient() throws NoSuchAlgorithmException { + ProtectedStorageEntry transientEntry = getProtectedStorageEntry(); + + TestState seedNodeTestState = new TestState(); + P2PDataStorage seedNode = seedNodeTestState.mockedStorage; + + TestState clientNodeTestState = new TestState(); + P2PDataStorage clientNode = clientNodeTestState.mockedStorage; + + seedNode.addProtectedStorageEntry(transientEntry, null, null, false); + + clientNode.addProtectedStorageEntry(transientEntry, null, null, false); + + clientNodeTestState.simulateRestart(); + clientNode = clientNodeTestState.mockedStorage; + + GetDataRequest getDataRequest = clientNode.buildPreliminaryGetDataRequest(1); + + GetDataResponse getDataResponse = seedNode.buildGetDataResponse( + getDataRequest, 1, new AtomicBoolean(), new AtomicBoolean(), new Capabilities()); + + TestState.SavedTestState beforeState = clientNodeTestState.saveTestState(transientEntry); + clientNode.processGetDataResponse(getDataResponse, null); + + clientNodeTestState.verifyProtectedStorageAdd( + beforeState, transientEntry, true, true, false, true, false); + } + + // TESTCASE: Synchronization after peer restart works for in-memory ProtectedStorageEntrys + @Test + public void basicSynchronizationWorksAfterRestartPersistent() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload persistentPayload = + new PersistableExpirableProtectedStoragePayloadStub(ownerKeys.getPublic()); + ProtectedStorageEntry persistentEntry = getProtectedStorageEntry(ownerKeys.getPublic(), persistentPayload, 1); + + TestState seedNodeTestState = new TestState(); + P2PDataStorage seedNode = seedNodeTestState.mockedStorage; + + TestState clientNodeTestState = new TestState(); + P2PDataStorage clientNode = clientNodeTestState.mockedStorage; + + seedNode.addProtectedStorageEntry(persistentEntry, null, null, false); + + clientNode.addProtectedStorageEntry(persistentEntry, null, null, false); + + clientNodeTestState.simulateRestart(); + clientNode = clientNodeTestState.mockedStorage; + + GetDataRequest getDataRequest = clientNode.buildPreliminaryGetDataRequest(1); + + GetDataResponse getDataResponse = seedNode.buildGetDataResponse( + getDataRequest, 1, new AtomicBoolean(), new AtomicBoolean(), new Capabilities()); + + TestState.SavedTestState beforeState = clientNodeTestState.saveTestState(persistentEntry); + clientNode.processGetDataResponse(getDataResponse, null); + + clientNodeTestState.verifyProtectedStorageAdd( + beforeState, persistentEntry, false, false, false, false, false); + Assert.assertTrue(clientNodeTestState.mockedStorage.getMap().containsValue(persistentEntry)); + } + + // TESTCASE: Removes seen only by the seednode should be replayed on the client node + // during startup + // XXXBUGXXX: #3610 Lost removes are never replayed. + @Test + public void lostRemoveNeverUpdated() throws NoSuchAlgorithmException { + TestState seedNodeTestState = new TestState(); + P2PDataStorage seedNode = seedNodeTestState.mockedStorage; + + TestState clientNodeTestState = new TestState(); + P2PDataStorage clientNode = clientNodeTestState.mockedStorage; + + // Both nodes see the add + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + ProtectedStorageEntry onSeedNodeAndClientNode = getProtectedStorageEntry( + ownerKeys.getPublic(), protectedStoragePayload, 1); + seedNode.addProtectedStorageEntry(onSeedNodeAndClientNode, null, null, false); + clientNode.addProtectedStorageEntry(onSeedNodeAndClientNode, null, null, false); + + // Seed node sees the remove, but client node does not + seedNode.remove(getProtectedStorageEntry( + ownerKeys.getPublic(), protectedStoragePayload, 2), null, false); + + GetDataRequest getDataRequest = clientNode.buildPreliminaryGetDataRequest(1); + + GetDataResponse getDataResponse = seedNode.buildGetDataResponse( + getDataRequest, 1, new AtomicBoolean(), new AtomicBoolean(), new Capabilities()); + + TestState.SavedTestState beforeState = clientNodeTestState.saveTestState(onSeedNodeAndClientNode); + clientNode.processGetDataResponse(getDataResponse, null); + + // Should succeed + clientNodeTestState.verifyProtectedStorageRemove( + beforeState, onSeedNodeAndClientNode, false, false, false, false, false); + } +} From 5db128587f7b8d6913a9eda27a83224294424e4e Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 09:13:36 -0800 Subject: [PATCH 23/28] [REFACTOR] Clean up processGetDataResponse - Add more comments - Use Clock instead of System - Remove unnecessary AtomicInteger --- .../network/p2p/storage/P2PDataStorage.java | 23 ++++++++----------- 1 file changed, 9 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 786a236cb8e..fa24c4c45c6 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -328,21 +328,15 @@ public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress Set persistableNetworkPayloadSet = getDataResponse.getPersistableNetworkPayloadSet(); long ts2 = System.currentTimeMillis(); - AtomicInteger counter = new AtomicInteger(); dataSet.forEach(e -> { // We don't broadcast here (last param) as we are only connected to the seed node and would be pointless addProtectedStorageEntry(e, sender, null, false, false); - counter.getAndIncrement(); }); - log.info("Processing {} protectedStorageEntries took {} ms.", counter.get(), System.currentTimeMillis() - ts2); + log.info("Processing {} protectedStorageEntries took {} ms.", dataSet.size(), this.clock.millis() - ts2); - /* // engage the firstRequest logic only if we are a seed node. Normal clients get here twice at most. - if (!Capabilities.app.containsAll(Capability.SEED_NODE)) - firstRequest = true;*/ - - if (persistableNetworkPayloadSet != null /*&& firstRequest*/) { - ts2 = System.currentTimeMillis(); + if (persistableNetworkPayloadSet != null) { + ts2 = this.clock.millis(); persistableNetworkPayloadSet.forEach(e -> { if (e instanceof LazyProcessedPayload) { // We use an optimized method as many checks are not required in that case to avoid @@ -362,13 +356,14 @@ public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress false, false, false); } }); - - // We set initialRequestApplied to true after the loop, otherwise we would only process 1 entry - initialRequestApplied = true; - log.info("Processing {} persistableNetworkPayloads took {} ms.", - persistableNetworkPayloadSet.size(), System.currentTimeMillis() - ts2); + persistableNetworkPayloadSet.size(), this.clock.millis() - ts2); } + + // We only process PersistableNetworkPayloads implementing LazyProcessedPayload once. It can cause performance + // issues and since the data is rarely out of sync it is not worth it to apply them from multiple peers during + // startup. + initialRequestApplied = true; } /////////////////////////////////////////////////////////////////////////////////////////// From ecae31eddb8c4cf0a88d89da52517453ebe9e088 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 09:22:12 -0800 Subject: [PATCH 24/28] [RENAME] LazyProcessedPayload to ProcessOncePersistableNetworkPayload Name is left over from previous implementation. Change it to be more relevant to the current code and update comments to indicate the current usage. --- .../java/bisq/core/account/sign/SignedWitness.java | 4 ++-- .../bisq/core/account/witness/AccountAgeWitness.java | 4 ++-- .../proposal/storage/temp/TempProposalPayload.java | 4 ++-- .../bisq/core/trade/statistics/TradeStatistics.java | 4 ++-- .../bisq/core/trade/statistics/TradeStatistics2.java | 4 ++-- .../bisq/network/p2p/storage/P2PDataStorage.java | 6 +++--- ...ava => ProcessOncePersistableNetworkPayload.java} | 7 ++++--- .../peers/getdata/GetDataRequestOnMessageTest.java | 4 ++-- .../P2PDataStorageProcessGetDataResponse.java | 12 ++++++------ 9 files changed, 25 insertions(+), 24 deletions(-) rename p2p/src/main/java/bisq/network/p2p/storage/payload/{LazyProcessedPayload.java => ProcessOncePersistableNetworkPayload.java} (72%) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitness.java b/core/src/main/java/bisq/core/account/sign/SignedWitness.java index e5af940470c..d2268400ac6 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitness.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitness.java @@ -20,7 +20,7 @@ import bisq.network.p2p.storage.P2PDataStorage; import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; import bisq.network.p2p.storage.payload.DateTolerantPayload; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.common.app.Capabilities; @@ -45,7 +45,7 @@ // Supports signatures made from EC key (arbitrators) and signature created with DSA key. @Slf4j @Value -public class SignedWitness implements LazyProcessedPayload, PersistableNetworkPayload, PersistableEnvelope, +public class SignedWitness implements ProcessOncePersistableNetworkPayload, PersistableNetworkPayload, PersistableEnvelope, DateTolerantPayload, CapabilityRequiringPayload { public enum VerificationMethod { diff --git a/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java b/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java index 9578df22973..07f05096602 100644 --- a/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java +++ b/core/src/main/java/bisq/core/account/witness/AccountAgeWitness.java @@ -19,7 +19,7 @@ import bisq.network.p2p.storage.P2PDataStorage; import bisq.network.p2p.storage.payload.DateTolerantPayload; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.common.proto.persistable.PersistableEnvelope; @@ -40,7 +40,7 @@ // so only the newly added objects since the last release will be retrieved over the P2P network. @Slf4j @Value -public class AccountAgeWitness implements LazyProcessedPayload, PersistableNetworkPayload, PersistableEnvelope, DateTolerantPayload { +public class AccountAgeWitness implements ProcessOncePersistableNetworkPayload, PersistableNetworkPayload, PersistableEnvelope, DateTolerantPayload { private static final long TOLERANCE = TimeUnit.DAYS.toMillis(1); private final byte[] hash; // Ripemd160(Sha256(concatenated accountHash, signature and sigPubKey)); 20 bytes diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalPayload.java b/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalPayload.java index 6ea17a819d1..f5334fa6030 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalPayload.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalPayload.java @@ -20,7 +20,7 @@ import bisq.core.dao.state.model.governance.Proposal; import bisq.network.p2p.storage.payload.ExpirablePayload; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; import bisq.common.crypto.Sig; @@ -55,7 +55,7 @@ @Getter @EqualsAndHashCode @FieldDefaults(makeFinal = true, level = AccessLevel.PRIVATE) -public class TempProposalPayload implements LazyProcessedPayload, ProtectedStoragePayload, +public class TempProposalPayload implements ProcessOncePersistableNetworkPayload, ProtectedStoragePayload, ExpirablePayload, PersistablePayload { protected final Proposal proposal; diff --git a/core/src/main/java/bisq/core/trade/statistics/TradeStatistics.java b/core/src/main/java/bisq/core/trade/statistics/TradeStatistics.java index 54412a5de10..41c77aeb36e 100644 --- a/core/src/main/java/bisq/core/trade/statistics/TradeStatistics.java +++ b/core/src/main/java/bisq/core/trade/statistics/TradeStatistics.java @@ -24,7 +24,7 @@ import bisq.core.offer.OfferPayload; import bisq.network.p2p.storage.payload.ExpirablePayload; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; import bisq.common.crypto.Sig; @@ -60,7 +60,7 @@ @Slf4j @EqualsAndHashCode(exclude = {"signaturePubKeyBytes"}) @Value -public final class TradeStatistics implements LazyProcessedPayload, ProtectedStoragePayload, ExpirablePayload, PersistablePayload { +public final class TradeStatistics implements ProcessOncePersistableNetworkPayload, ProtectedStoragePayload, ExpirablePayload, PersistablePayload { private final OfferPayload.Direction direction; private final String baseCurrency; private final String counterCurrency; diff --git a/core/src/main/java/bisq/core/trade/statistics/TradeStatistics2.java b/core/src/main/java/bisq/core/trade/statistics/TradeStatistics2.java index 601284f4e74..ba1f6abaf25 100644 --- a/core/src/main/java/bisq/core/trade/statistics/TradeStatistics2.java +++ b/core/src/main/java/bisq/core/trade/statistics/TradeStatistics2.java @@ -25,7 +25,7 @@ import bisq.core.offer.OfferUtil; import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.common.app.Capabilities; @@ -63,7 +63,7 @@ @Slf4j @Value -public final class TradeStatistics2 implements LazyProcessedPayload, PersistableNetworkPayload, PersistableEnvelope, CapabilityRequiringPayload { +public final class TradeStatistics2 implements ProcessOncePersistableNetworkPayload, PersistableNetworkPayload, PersistableEnvelope, CapabilityRequiringPayload { //We don't support arbitrators anymore so this entry will be only for pre v1.2. trades @Deprecated 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 fa24c4c45c6..d143ba1f4c7 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -39,7 +39,7 @@ import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; import bisq.network.p2p.storage.payload.DateTolerantPayload; import bisq.network.p2p.storage.payload.ExpirablePayload; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.MailboxStoragePayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedMailboxStorageEntry; @@ -338,7 +338,7 @@ public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress if (persistableNetworkPayloadSet != null) { ts2 = this.clock.millis(); persistableNetworkPayloadSet.forEach(e -> { - if (e instanceof LazyProcessedPayload) { + if (e instanceof ProcessOncePersistableNetworkPayload) { // We use an optimized method as many checks are not required in that case to avoid // performance issues. // Processing 82645 items took now 61 ms compared to earlier version where it took ages (> 2min). @@ -360,7 +360,7 @@ public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress persistableNetworkPayloadSet.size(), this.clock.millis() - ts2); } - // We only process PersistableNetworkPayloads implementing LazyProcessedPayload once. It can cause performance + // We only process PersistableNetworkPayloads implementing ProcessOncePersistableNetworkPayload once. It can cause performance // issues and since the data is rarely out of sync it is not worth it to apply them from multiple peers during // startup. initialRequestApplied = true; diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/LazyProcessedPayload.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProcessOncePersistableNetworkPayload.java similarity index 72% rename from p2p/src/main/java/bisq/network/p2p/storage/payload/LazyProcessedPayload.java rename to p2p/src/main/java/bisq/network/p2p/storage/payload/ProcessOncePersistableNetworkPayload.java index f63d1cf9b62..70fe1c35abe 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/LazyProcessedPayload.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProcessOncePersistableNetworkPayload.java @@ -20,8 +20,9 @@ import bisq.common.Payload; /** - * Marker interface for payload which gets delayed processed at startup so we don't hit performance too much. - * Used for TradeStatistics and AccountAgeWitness. + * Marker interface for PersistableNetworkPayloads that are only added during the FIRST call to + * P2PDataStorage::processDataResponse. This improves performance for objects that don't go out + * of sync frequently. */ -public interface LazyProcessedPayload extends Payload { +public interface ProcessOncePersistableNetworkPayload extends Payload { } diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java index 2ac1c25b884..deaf85a85b1 100644 --- a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java @@ -27,7 +27,7 @@ import bisq.network.p2p.storage.TestState; import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; @@ -99,7 +99,7 @@ private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgo } static class LazyPersistableNetworkPayloadStub extends PersistableNetworkPayloadStub - implements LazyProcessedPayload { + implements ProcessOncePersistableNetworkPayload { LazyPersistableNetworkPayloadStub(byte[] hash) { super(hash); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java index c6b349183b5..284ff197e81 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java @@ -22,7 +22,7 @@ import bisq.network.p2p.peers.getdata.messages.GetDataResponse; import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; -import bisq.network.p2p.storage.payload.LazyProcessedPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; @@ -95,7 +95,7 @@ private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgo } static class LazyPersistableNetworkPayloadStub extends PersistableNetworkPayloadStub - implements LazyProcessedPayload { + implements ProcessOncePersistableNetworkPayload { LazyPersistableNetworkPayloadStub(byte[] hash) { super(hash); @@ -107,7 +107,7 @@ static class LazyPersistableNetworkPayloadStub extends PersistableNetworkPayload } // TESTCASE: GetDataResponse w/ missing PNP is added with no broadcast or listener signal - // XXXBUGXXX: We signal listeners w/ non LazyProcessedPayloads + // XXXBUGXXX: We signal listeners w/ non ProcessOncePersistableNetworkPayloads @Test public void processGetDataResponse_newPNPUpdatesState() { PersistableNetworkPayload persistableNetworkPayload = new PersistableNetworkPayloadStub(new byte[] { 1 }); @@ -148,7 +148,7 @@ public void processGetDataResponse_duplicatePNPDoesNothing() { beforeState, persistableNetworkPayload, false, false, false, false); } - // TESTCASE: GetDataResponse w/ missing PNP is added with no broadcast or listener signal (LazyProcessedPayload) + // TESTCASE: GetDataResponse w/ missing PNP is added with no broadcast or listener signal (ProcessOncePersistableNetworkPayload) @Test public void processGetDataResponse_newPNPUpdatesState_LazyProcessed() { PersistableNetworkPayload persistableNetworkPayload = new LazyPersistableNetworkPayloadStub(new byte[] { 1 }); @@ -161,7 +161,7 @@ public void processGetDataResponse_newPNPUpdatesState_LazyProcessed() { beforeState, persistableNetworkPayload, true, false, false, false); } - // TESTCASE: GetDataResponse w/ existing PNP changes no state (LazyProcessedPayload) + // TESTCASE: GetDataResponse w/ existing PNP changes no state (ProcessOncePersistableNetworkPayload) @Test public void processGetDataResponse_duplicatePNPDoesNothing_LazyProcessed() { PersistableNetworkPayload persistableNetworkPayload = new LazyPersistableNetworkPayloadStub(new byte[] { 1 }); @@ -188,7 +188,7 @@ public void processGetDataResponse_nullPNPSetDoesNothing() { beforeState, persistableNetworkPayload, false, false, false, false); } - // TESTCASE: Second call to processGetDataResponse adds PNP for non-LazyProcessedPayloads + // TESTCASE: Second call to processGetDataResponse adds PNP for non-ProcessOncePersistableNetworkPayloads @Test public void processGetDataResponse_secondProcessNewPNPUpdatesState() { PersistableNetworkPayload addFromFirstProcess = new PersistableNetworkPayloadStub(new byte[] { 1 }); From a0fae120687ed83ec3c7ed3628c27f018d4209cc Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 09:50:48 -0800 Subject: [PATCH 25/28] Remove @Nullable around persistableNetworkPayloadSet Checking for null creates hard-to-read code and it is simpler to just create an empty set if we receive a pre-v0.6 GetDataResponse protobuf message that does not have the field set. --- .../bisq/monitor/metric/P2PMarketStats.java | 19 +++----- .../monitor/metric/P2PSeedNodeSnapshot.java | 19 +++----- .../p2p/peers/getdata/RequestDataHandler.java | 20 ++++----- .../getdata/messages/GetDataResponse.java | 24 +++++----- .../network/p2p/storage/P2PDataStorage.java | 44 +++++++++---------- .../P2PDataStorageProcessGetDataResponse.java | 14 +----- 6 files changed, 55 insertions(+), 85 deletions(-) diff --git a/monitor/src/main/java/bisq/monitor/metric/P2PMarketStats.java b/monitor/src/main/java/bisq/monitor/metric/P2PMarketStats.java index edebbf13d44..21a17fd9945 100644 --- a/monitor/src/main/java/bisq/monitor/metric/P2PMarketStats.java +++ b/monitor/src/main/java/bisq/monitor/metric/P2PMarketStats.java @@ -221,20 +221,15 @@ protected boolean treatMessage(NetworkEnvelope networkEnvelope, Connection conne versions.log(protectedStoragePayload); }); - Set persistableNetworkPayloadSet = dataResponse - .getPersistableNetworkPayloadSet(); - if (persistableNetworkPayloadSet != null) { - persistableNetworkPayloadSet.forEach(persistableNetworkPayload -> { + dataResponse.getPersistableNetworkPayloadSet().forEach(persistableNetworkPayload -> { + // memorize message hashes + //Byte[] bytes = new Byte[persistableNetworkPayload.getHash().length]; + //Arrays.setAll(bytes, n -> persistableNetworkPayload.getHash()[n]); - // memorize message hashes - //Byte[] bytes = new Byte[persistableNetworkPayload.getHash().length]; - //Arrays.setAll(bytes, n -> persistableNetworkPayload.getHash()[n]); + //hashes.add(bytes); - //hashes.add(bytes); - - hashes.add(persistableNetworkPayload.getHash()); - }); - } + hashes.add(persistableNetworkPayload.getHash()); + }); bucketsPerHost.put(connection.getPeersNodeAddressProperty().getValue(), result); versionBucketsPerHost.put(connection.getPeersNodeAddressProperty().getValue(), versions); diff --git a/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshot.java b/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshot.java index 77462aa4e5e..aefb227189c 100644 --- a/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshot.java +++ b/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshot.java @@ -333,20 +333,15 @@ protected boolean treatMessage(NetworkEnvelope networkEnvelope, Connection conne result.log(protectedStoragePayload); }); - Set persistableNetworkPayloadSet = dataResponse - .getPersistableNetworkPayloadSet(); - if (persistableNetworkPayloadSet != null) { - persistableNetworkPayloadSet.forEach(persistableNetworkPayload -> { + dataResponse.getPersistableNetworkPayloadSet().forEach(persistableNetworkPayload -> { + // memorize message hashes + //Byte[] bytes = new Byte[persistableNetworkPayload.getHash().length]; + //Arrays.setAll(bytes, n -> persistableNetworkPayload.getHash()[n]); - // memorize message hashes - //Byte[] bytes = new Byte[persistableNetworkPayload.getHash().length]; - //Arrays.setAll(bytes, n -> persistableNetworkPayload.getHash()[n]); + //hashes.add(bytes); - //hashes.add(bytes); - - hashes.add(persistableNetworkPayload.getHash()); - }); - } + hashes.add(persistableNetworkPayload.getHash()); + }); bucketsPerHost.put(connection.getPeersNodeAddressProperty().getValue(), result); return true; 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 01c0f00361a..1685a96a659 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 @@ -248,24 +248,20 @@ private void logContents(NetworkEnvelope networkEnvelope, payloadByClassName.get(className).add(protectedStoragePayload); }); + persistableNetworkPayloadSet.forEach(persistableNetworkPayload -> { + // For logging different data types + String className = persistableNetworkPayload.getClass().getSimpleName(); + if (!payloadByClassName.containsKey(className)) + payloadByClassName.put(className, new HashSet<>()); - if (persistableNetworkPayloadSet != null) { - persistableNetworkPayloadSet.forEach(persistableNetworkPayload -> { - // For logging different data types - String className = persistableNetworkPayload.getClass().getSimpleName(); - if (!payloadByClassName.containsKey(className)) - payloadByClassName.put(className, new HashSet<>()); - - payloadByClassName.get(className).add(persistableNetworkPayload); - }); - } + payloadByClassName.get(className).add(persistableNetworkPayload); + }); // Log different data types StringBuilder sb = new StringBuilder(); sb.append("\n#################################################################\n"); sb.append("Connected to node: " + peersNodeAddress.getFullAddress() + "\n"); - final int items = dataSet.size() + - (persistableNetworkPayloadSet != null ? persistableNetworkPayloadSet.size() : 0); + final int items = dataSet.size() + persistableNetworkPayloadSet.size(); sb.append("Received ").append(items).append(" instances\n"); payloadByClassName.forEach((key, value) -> sb.append(key) .append(": ") diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java index 0386d1abe1b..bb6e9b83aeb 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java @@ -37,6 +37,8 @@ import lombok.Value; import lombok.extern.slf4j.Slf4j; +import org.jetbrains.annotations.NotNull; + import javax.annotation.Nullable; @Slf4j @@ -47,8 +49,7 @@ public final class GetDataResponse extends NetworkEnvelope implements SupportedC private final Set dataSet; // Set of PersistableNetworkPayload objects - // We added that in v 0.6 and we would get a null object from older peers, so keep it annotated with @Nullable - @Nullable + // We added that in v 0.6 and the fromProto code will create an empty HashSet if it doesn't exist private final Set persistableNetworkPayloadSet; private final int requestNonce; @@ -57,7 +58,7 @@ public final class GetDataResponse extends NetworkEnvelope implements SupportedC private final Capabilities supportedCapabilities; public GetDataResponse(Set dataSet, - @Nullable Set persistableNetworkPayloadSet, + @NotNull Set persistableNetworkPayloadSet, int requestNonce, boolean isGetUpdatedDataResponse) { this(dataSet, @@ -73,7 +74,7 @@ public GetDataResponse(Set dataSet, /////////////////////////////////////////////////////////////////////////////////////////// private GetDataResponse(Set dataSet, - @Nullable Set persistableNetworkPayloadSet, + @NotNull Set persistableNetworkPayloadSet, int requestNonce, boolean isGetUpdatedDataResponse, @Nullable Capabilities supportedCapabilities, @@ -101,12 +102,12 @@ public protobuf.NetworkEnvelope toProtoNetworkEnvelope() { .build()) .collect(Collectors.toList())) .setRequestNonce(requestNonce) - .setIsGetUpdatedDataResponse(isGetUpdatedDataResponse); + .setIsGetUpdatedDataResponse(isGetUpdatedDataResponse) + .addAllPersistableNetworkPayloadItems(persistableNetworkPayloadSet.stream() + .map(PersistableNetworkPayload::toProtoMessage) + .collect(Collectors.toList())); Optional.ofNullable(supportedCapabilities).ifPresent(e -> builder.addAllSupportedCapabilities(Capabilities.toIntList(supportedCapabilities))); - Optional.ofNullable(persistableNetworkPayloadSet).ifPresent(set -> builder.addAllPersistableNetworkPayloadItems(set.stream() - .map(PersistableNetworkPayload::toProtoMessage) - .collect(Collectors.toList()))); protobuf.NetworkEnvelope proto = getNetworkEnvelopeBuilder() .setGetDataResponse(builder) @@ -124,14 +125,11 @@ public static GetDataResponse fromProto(protobuf.GetDataResponse proto, .map(entry -> (ProtectedStorageEntry) resolver.fromProto(entry)) .collect(Collectors.toSet())); - Set persistableNetworkPayloadSet = proto.getPersistableNetworkPayloadItemsList().isEmpty() ? - null : - new HashSet<>( - proto.getPersistableNetworkPayloadItemsList().stream() + Set persistableNetworkPayloadSet = new HashSet<>( + proto.getPersistableNetworkPayloadItemsList().stream() .map(e -> (PersistableNetworkPayload) resolver.fromProto(e)) .collect(Collectors.toSet())); - //PersistableNetworkPayload return new GetDataResponse(dataSet, persistableNetworkPayloadSet, proto.getRequestNonce(), 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 d143ba1f4c7..551b15bae49 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -335,30 +335,28 @@ public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress }); log.info("Processing {} protectedStorageEntries took {} ms.", dataSet.size(), this.clock.millis() - ts2); - if (persistableNetworkPayloadSet != null) { - ts2 = this.clock.millis(); - persistableNetworkPayloadSet.forEach(e -> { - if (e instanceof ProcessOncePersistableNetworkPayload) { - // We use an optimized method as many checks are not required in that case to avoid - // performance issues. - // Processing 82645 items took now 61 ms compared to earlier version where it took ages (> 2min). - // Usually we only get about a few hundred or max. a few 1000 items. 82645 is all - // trade stats stats and all account age witness data. - - // We only apply it once from first response - if (!initialRequestApplied) { - addPersistableNetworkPayloadFromInitialRequest(e); - - } - } else { - // We don't broadcast here as we are only connected to the seed node and would be pointless - addPersistableNetworkPayload(e, sender, false, - false, false, false); + ts2 = this.clock.millis(); + persistableNetworkPayloadSet.forEach(e -> { + if (e instanceof ProcessOncePersistableNetworkPayload) { + // We use an optimized method as many checks are not required in that case to avoid + // performance issues. + // Processing 82645 items took now 61 ms compared to earlier version where it took ages (> 2min). + // Usually we only get about a few hundred or max. a few 1000 items. 82645 is all + // trade stats stats and all account age witness data. + + // We only apply it once from first response + if (!initialRequestApplied) { + addPersistableNetworkPayloadFromInitialRequest(e); + } - }); - log.info("Processing {} persistableNetworkPayloads took {} ms.", - persistableNetworkPayloadSet.size(), this.clock.millis() - ts2); - } + } else { + // We don't broadcast here as we are only connected to the seed node and would be pointless + addPersistableNetworkPayload(e, sender, false, + false, false, false); + } + }); + log.info("Processing {} persistableNetworkPayloads took {} ms.", + persistableNetworkPayloadSet.size(), this.clock.millis() - ts2); // We only process PersistableNetworkPayloads implementing ProcessOncePersistableNetworkPayload once. It can cause performance // issues and since the data is rarely out of sync it is not worth it to apply them from multiple peers during diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java index 284ff197e81..c7177b005d7 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProcessGetDataResponse.java @@ -69,7 +69,7 @@ static private GetDataResponse buildGetDataResponse( List persistableNetworkPayloads) { return new GetDataResponse( new HashSet<>(protectedStorageEntries), - persistableNetworkPayloads == null ? null : new HashSet<>(persistableNetworkPayloads), + new HashSet<>(persistableNetworkPayloads), 1, false); } @@ -176,18 +176,6 @@ public void processGetDataResponse_duplicatePNPDoesNothing_LazyProcessed() { beforeState, persistableNetworkPayload, false, false, false, false); } - // TESTCASE: GetDataResponse w/ null PNP changes no state - @Test - public void processGetDataResponse_nullPNPSetDoesNothing() { - PersistableNetworkPayload persistableNetworkPayload = new PersistableNetworkPayloadStub(new byte[] { 1 }); - GetDataResponse getDataResponse = buildGetDataResponse(Collections.emptyList(), null); - - TestState.SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload); - this.testState.mockedStorage.processGetDataResponse(getDataResponse, this.peerNodeAddress); - this.testState.verifyPersistableAdd( - beforeState, persistableNetworkPayload, false, false, false, false); - } - // TESTCASE: Second call to processGetDataResponse adds PNP for non-ProcessOncePersistableNetworkPayloads @Test public void processGetDataResponse_secondProcessNewPNPUpdatesState() { From c503bcbaed54175fc35400175befdc26145896f7 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 11:36:53 -0800 Subject: [PATCH 26/28] Remove @Nullable around supportedCapabilities in GetDataResponse The only two users of this constructor are the fromProto path which already creates an empty Capabilities object if one is not provided and the internal usage of Capabilities.app which is initialized to empty. Remove the @Nullable so future readers aren't confused. --- .../getdata/messages/GetDataResponse.java | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java index bb6e9b83aeb..a86d6f27834 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java @@ -29,7 +29,6 @@ import bisq.common.proto.network.NetworkProtoResolver; import java.util.HashSet; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -39,8 +38,6 @@ import org.jetbrains.annotations.NotNull; -import javax.annotation.Nullable; - @Slf4j @EqualsAndHashCode(callSuper = true) @Value @@ -54,10 +51,9 @@ public final class GetDataResponse extends NetworkEnvelope implements SupportedC private final int requestNonce; private final boolean isGetUpdatedDataResponse; - @Nullable private final Capabilities supportedCapabilities; - public GetDataResponse(Set dataSet, + public GetDataResponse(@NotNull Set dataSet, @NotNull Set persistableNetworkPayloadSet, int requestNonce, boolean isGetUpdatedDataResponse) { @@ -73,11 +69,11 @@ public GetDataResponse(Set dataSet, // PROTO BUFFER /////////////////////////////////////////////////////////////////////////////////////////// - private GetDataResponse(Set dataSet, + private GetDataResponse(@NotNull Set dataSet, @NotNull Set persistableNetworkPayloadSet, int requestNonce, boolean isGetUpdatedDataResponse, - @Nullable Capabilities supportedCapabilities, + @NotNull Capabilities supportedCapabilities, int messageVersion) { super(messageVersion); @@ -101,13 +97,12 @@ public protobuf.NetworkEnvelope toProtoNetworkEnvelope() { .setProtectedStorageEntry((protobuf.ProtectedStorageEntry) protectedStorageEntry.toProtoMessage()) .build()) .collect(Collectors.toList())) - .setRequestNonce(requestNonce) - .setIsGetUpdatedDataResponse(isGetUpdatedDataResponse) .addAllPersistableNetworkPayloadItems(persistableNetworkPayloadSet.stream() .map(PersistableNetworkPayload::toProtoMessage) - .collect(Collectors.toList())); - - Optional.ofNullable(supportedCapabilities).ifPresent(e -> builder.addAllSupportedCapabilities(Capabilities.toIntList(supportedCapabilities))); + .collect(Collectors.toList())) + .setRequestNonce(requestNonce) + .setIsGetUpdatedDataResponse(isGetUpdatedDataResponse) + .addAllSupportedCapabilities(Capabilities.toIntList(supportedCapabilities)); protobuf.NetworkEnvelope proto = getNetworkEnvelopeBuilder() .setGetDataResponse(builder) From b1a06fe4e20586448e8e8a7a6be45653ee0cd10b Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 10:16:47 -0800 Subject: [PATCH 27/28] Remove @Nullable around supportedCapabilities in PreliminaryGetDataRequest The only two users of this constructor are the fromProto path which now creates an empty Capabilities object similar to GetDataResponse. The other internal usage of Capabilities.app which is initialized to empty. --- .../messages/PreliminaryGetDataRequest.java | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/PreliminaryGetDataRequest.java b/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/PreliminaryGetDataRequest.java index 0702b0f1756..db0e51eb7f6 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/PreliminaryGetDataRequest.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/PreliminaryGetDataRequest.java @@ -26,7 +26,6 @@ import com.google.protobuf.ByteString; -import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; @@ -34,9 +33,7 @@ import lombok.Value; import lombok.extern.slf4j.Slf4j; -import javax.annotation.Nullable; - - +import org.jetbrains.annotations.NotNull; import protobuf.NetworkEnvelope; @@ -44,11 +41,10 @@ @EqualsAndHashCode(callSuper = true) @Value public final class PreliminaryGetDataRequest extends GetDataRequest implements AnonymousMessage, SupportedCapabilitiesMessage { - @Nullable private final Capabilities supportedCapabilities; public PreliminaryGetDataRequest(int nonce, - Set excludedKeys) { + @NotNull Set excludedKeys) { this(nonce, excludedKeys, Capabilities.app, Version.getP2PMessageVersion()); } @@ -58,8 +54,8 @@ public PreliminaryGetDataRequest(int nonce, /////////////////////////////////////////////////////////////////////////////////////////// private PreliminaryGetDataRequest(int nonce, - Set excludedKeys, - @Nullable Capabilities supportedCapabilities, + @NotNull Set excludedKeys, + @NotNull Capabilities supportedCapabilities, int messageVersion) { super(messageVersion, nonce, excludedKeys); @@ -69,13 +65,12 @@ private PreliminaryGetDataRequest(int nonce, @Override public protobuf.NetworkEnvelope toProtoNetworkEnvelope() { final protobuf.PreliminaryGetDataRequest.Builder builder = protobuf.PreliminaryGetDataRequest.newBuilder() + .addAllSupportedCapabilities(Capabilities.toIntList(supportedCapabilities)) .setNonce(nonce) .addAllExcludedKeys(excludedKeys.stream() .map(ByteString::copyFrom) .collect(Collectors.toList())); - Optional.ofNullable(supportedCapabilities).ifPresent(e -> builder.addAllSupportedCapabilities(Capabilities.toIntList(supportedCapabilities))); - NetworkEnvelope proto = getNetworkEnvelopeBuilder() .setPreliminaryGetDataRequest(builder) .build(); @@ -85,13 +80,9 @@ public protobuf.NetworkEnvelope toProtoNetworkEnvelope() { public static PreliminaryGetDataRequest fromProto(protobuf.PreliminaryGetDataRequest proto, int messageVersion) { log.info("Received a PreliminaryGetDataRequest with {} kB", proto.getSerializedSize() / 1000d); - Capabilities supportedCapabilities = proto.getSupportedCapabilitiesList().isEmpty() ? - null : - Capabilities.fromIntList(proto.getSupportedCapabilitiesList()); - return new PreliminaryGetDataRequest(proto.getNonce(), ProtoUtil.byteSetFromProtoByteStringList(proto.getExcludedKeysList()), - supportedCapabilities, + Capabilities.fromIntList(proto.getSupportedCapabilitiesList()), messageVersion); } } From 4fe19aeec20e3587571640f74b2541c49e6755c9 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 10:33:12 -0800 Subject: [PATCH 28/28] [DEADCODE] Remove old request handler tests Now that all the implementations are unit tested in P2PDataStorage, the old tests can be removed. --- .../p2p/peers/getdata/RequestDataHandler.java | 4 +- .../getdata/GetDataRequestHandlerTest.java | 199 -------------- .../getdata/GetDataRequestOnMessageTest.java | 160 ----------- .../RequestDataHandlerRequestDataTest.java | 249 ------------------ .../bisq/network/p2p/storage/TestState.java | 5 +- 5 files changed, 2 insertions(+), 615 deletions(-) delete mode 100644 p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java delete mode 100644 p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java delete mode 100644 p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java 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 1685a96a659..4d1ada32941 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 @@ -35,7 +35,6 @@ import bisq.common.proto.network.NetworkEnvelope; import bisq.common.proto.network.NetworkPayload; -import com.google.common.annotations.VisibleForTesting; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.SettableFuture; @@ -87,8 +86,7 @@ public interface Listener { private final PeerManager peerManager; private final Listener listener; private Timer timeoutTimer; - @VisibleForTesting - final int nonce = new Random().nextInt(); + private final int nonce = new Random().nextInt(); private boolean stopped; diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java deleted file mode 100644 index 10a7555e785..00000000000 --- a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestHandlerTest.java +++ /dev/null @@ -1,199 +0,0 @@ -/* - * This file is part of Bisq. - * - * Bisq is free software: you can redistribute it and/or modify it - * under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at - * your option) any later version. - * - * Bisq is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public - * License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with Bisq. If not, see . - */ - -package bisq.network.p2p.peers.getdata; - -import bisq.network.p2p.NodeAddress; -import bisq.network.p2p.TestUtils; -import bisq.network.p2p.network.Connection; -import bisq.network.p2p.network.NetworkNode; -import bisq.network.p2p.peers.getdata.messages.GetDataResponse; -import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; -import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; -import bisq.network.p2p.storage.P2PDataStorage; -import bisq.network.p2p.storage.TestState; -import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; -import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; -import bisq.network.p2p.storage.payload.PersistableNetworkPayload; -import bisq.network.p2p.storage.payload.ProtectedStorageEntry; -import bisq.network.p2p.storage.payload.ProtectedStoragePayload; - -import bisq.common.Proto; -import bisq.common.app.Capabilities; -import bisq.common.app.Capability; - -import com.google.common.util.concurrent.SettableFuture; - -import java.security.KeyPair; -import java.security.NoSuchAlgorithmException; - -import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; - -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; - -import org.mockito.ArgumentCaptor; -import org.mockito.Mock; -import org.mockito.MockitoAnnotations; - -import static org.mockito.Mockito.*; - -public class GetDataRequestHandlerTest { - private TestState testState; - - @Mock - NetworkNode networkNode; - - private NodeAddress localNodeAddress; - - @Before - public void setUp() { - MockitoAnnotations.initMocks(this); - this.testState = new TestState(); - - this.localNodeAddress = new NodeAddress("localhost", 8080); - when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); - - // Set up basic capabilities to ensure message contains it. Ensure it is unique from other tests - // so we catch mismatch bugs. - Capabilities.app.addAll(Capability.DAO_FULL_NODE); - } - - /** - * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state - * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers - * is not supported. - */ - private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { - KeyPair ownerKeys = TestUtils.generateKeyPair(); - - ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); - - ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); - when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); - when(stub.isValidForAddOperation()).thenReturn(true); - when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); - when(stub.getSequenceNumber()).thenReturn(1); - when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); - - return stub; - } - - // TESTCASE: Construct a request that requires excluding duplicates and adding missing entrys for - // PersistableNetworkPayloads and ProtectedStorageEntrys to verify the correct keys are added to the response. - @Test - public void handle_PreliminaryGetDataRequestTest() throws NoSuchAlgorithmException { - - // Construct a seed node w/ 2 PersistableNetworkPayloads and 2 ProtectedStorageEntrys and a PreliminaryGetDataRequest - // that only contains 1 known PersistableNetworkPayload and 1 known ProtectedStorageEntry as well as 2 unknowns - PersistableNetworkPayload pnp_onSeedNodeNotInRequest = new PersistableNetworkPayloadStub(new byte[] { 1 }); - PersistableNetworkPayload pnp_onSeedNodeAndInRequest = new PersistableNetworkPayloadStub(new byte[] { 2 }); - PersistableNetworkPayload pnp_onlyInRequest = new PersistableNetworkPayloadStub(new byte[] { 3 }); - ProtectedStorageEntry pse_onSeedNodeNotInRequest = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry pse_onSeedNodeAndInRequest = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry pse_onlyInRequest = getProtectedStorageEntryForAdd(); - - this.testState.getMockedStorage().addPersistableNetworkPayload( - pnp_onSeedNodeNotInRequest, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addPersistableNetworkPayload( - pnp_onSeedNodeAndInRequest, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeNotInRequest, this.localNodeAddress, null, false); - this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeAndInRequest, this.localNodeAddress, null, false); - - SettableFuture sendFuture = mock(SettableFuture.class); - final ArgumentCaptor captor = ArgumentCaptor.forClass(GetDataResponse.class); - when(this.networkNode.sendMessage(any(Connection.class), captor.capture())).thenReturn(sendFuture); - - PreliminaryGetDataRequest getDataRequest = mock(PreliminaryGetDataRequest.class); - HashSet knownKeysSet = new HashSet<>(Arrays.asList( - pnp_onSeedNodeAndInRequest.getHash(), - pnp_onlyInRequest.getHash(), - P2PDataStorage.get32ByteHash(pse_onSeedNodeAndInRequest.getProtectedStoragePayload()), - P2PDataStorage.get32ByteHash(pse_onlyInRequest.getProtectedStoragePayload()))); - when(getDataRequest.getNonce()).thenReturn(1); - when(getDataRequest.getExcludedKeys()).thenReturn(knownKeysSet); - - Connection connection = mock(Connection.class); - when(connection.noCapabilityRequiredOrCapabilityIsSupported(any(Proto.class))).thenReturn(true); - - GetDataRequestHandler handler = - new GetDataRequestHandler(this.networkNode, this.testState.getMockedStorage(), null); - handler.handle(getDataRequest, connection); - - // Verify the request node is sent back only the 2 missing payloads - GetDataResponse getDataResponse = captor.getValue(); - Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); - Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); - Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); - Assert.assertFalse(getDataResponse.isGetUpdatedDataResponse()); - Assert.assertEquals(getDataResponse.getPersistableNetworkPayloadSet(), new HashSet<>(Collections.singletonList(pnp_onSeedNodeNotInRequest))); - Assert.assertEquals(getDataResponse.getDataSet(), new HashSet<>(Collections.singletonList(pse_onSeedNodeNotInRequest))); - } - - // TESTCASE: Same as above, but with an GetUpdatedDataRequest - @Test - public void handle_GetUpdatedDataRequestTest() throws NoSuchAlgorithmException { - - // Construct a seed node w/ 2 PersistableNetworkPayloads and 2 ProtectedStorageEntrys and a PreliminaryGetDataRequest - // that only contains 1 known PersistableNetworkPayload and 1 known ProtectedStorageEntry as well as 2 unknowns - PersistableNetworkPayload pnp_onSeedNodeNotInRequest = new PersistableNetworkPayloadStub(new byte[] { 1 }); - PersistableNetworkPayload pnp_onSeedNodeAndInRequest = new PersistableNetworkPayloadStub(new byte[] { 2 }); - PersistableNetworkPayload pnp_onlyInRequest = new PersistableNetworkPayloadStub(new byte[] { 3 }); - ProtectedStorageEntry pse_onSeedNodeNotInRequest = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry pse_onSeedNodeAndInRequest = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry pse_onlyInRequest = getProtectedStorageEntryForAdd(); - - this.testState.getMockedStorage().addPersistableNetworkPayload( - pnp_onSeedNodeNotInRequest, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addPersistableNetworkPayload( - pnp_onSeedNodeAndInRequest, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeNotInRequest, this.localNodeAddress, null, false); - this.testState.getMockedStorage().addProtectedStorageEntry(pse_onSeedNodeAndInRequest, this.localNodeAddress, null, false); - - SettableFuture sendFuture = mock(SettableFuture.class); - final ArgumentCaptor captor = ArgumentCaptor.forClass(GetDataResponse.class); - when(this.networkNode.sendMessage(any(Connection.class), captor.capture())).thenReturn(sendFuture); - - GetUpdatedDataRequest getDataRequest = mock(GetUpdatedDataRequest.class); - HashSet knownKeysSet = new HashSet<>(Arrays.asList( - pnp_onSeedNodeAndInRequest.getHash(), - pnp_onlyInRequest.getHash(), - P2PDataStorage.get32ByteHash(pse_onSeedNodeAndInRequest.getProtectedStoragePayload()), - P2PDataStorage.get32ByteHash(pse_onlyInRequest.getProtectedStoragePayload()))); - when(getDataRequest.getNonce()).thenReturn(1); - when(getDataRequest.getExcludedKeys()).thenReturn(knownKeysSet); - - Connection connection = mock(Connection.class); - when(connection.noCapabilityRequiredOrCapabilityIsSupported(any(Proto.class))).thenReturn(true); - - GetDataRequestHandler handler = - new GetDataRequestHandler(this.networkNode, this.testState.getMockedStorage(), null); - handler.handle(getDataRequest, connection); - - // Verify the request node is sent back only the 2 missing payloads - GetDataResponse getDataResponse = captor.getValue(); - Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); - Assert.assertEquals(getDataResponse.getSupportedCapabilities(), Capabilities.app); - Assert.assertEquals(getDataResponse.getRequestNonce(), getDataRequest.getNonce()); - Assert.assertTrue(getDataResponse.isGetUpdatedDataResponse()); - Assert.assertEquals(getDataResponse.getPersistableNetworkPayloadSet(), new HashSet<>(Collections.singletonList(pnp_onSeedNodeNotInRequest))); - Assert.assertEquals(getDataResponse.getDataSet(), new HashSet<>(Collections.singletonList(pse_onSeedNodeNotInRequest))); - } -} diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java deleted file mode 100644 index deaf85a85b1..00000000000 --- a/p2p/src/test/java/bisq/network/p2p/peers/getdata/GetDataRequestOnMessageTest.java +++ /dev/null @@ -1,160 +0,0 @@ -/* - * This file is part of Bisq. - * - * Bisq is free software: you can redistribute it and/or modify it - * under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at - * your option) any later version. - * - * Bisq is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public - * License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with Bisq. If not, see . - */ - -package bisq.network.p2p.peers.getdata; - -import bisq.network.p2p.NodeAddress; -import bisq.network.p2p.TestUtils; -import bisq.network.p2p.network.Connection; -import bisq.network.p2p.network.NetworkNode; -import bisq.network.p2p.peers.PeerManager; -import bisq.network.p2p.peers.getdata.messages.GetDataRequest; -import bisq.network.p2p.peers.getdata.messages.GetDataResponse; -import bisq.network.p2p.storage.TestState; -import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; -import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; -import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; -import bisq.network.p2p.storage.payload.PersistableNetworkPayload; -import bisq.network.p2p.storage.payload.ProtectedStorageEntry; -import bisq.network.p2p.storage.payload.ProtectedStoragePayload; - -import bisq.common.app.Capabilities; -import bisq.common.app.Capability; - -import com.google.common.util.concurrent.SettableFuture; - -import java.security.KeyPair; -import java.security.NoSuchAlgorithmException; - -import java.util.Arrays; -import java.util.HashSet; -import java.util.Optional; - -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.when; - - - -import org.mockito.Mock; -import org.mockito.MockitoAnnotations; - -public class GetDataRequestOnMessageTest { - private TestState testState; - - @Mock - NetworkNode networkNode; - - private NodeAddress localNodeAddress; - - @Before - public void setUp() { - MockitoAnnotations.initMocks(this); - this.testState = new TestState(); - - this.localNodeAddress = new NodeAddress("localhost", 8080); - when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); - - // Set up basic capabilities to ensure message contains it. Ensure it is unique from other tests - // so we catch mismatch bugs. - Capabilities.app.addAll(Capability.DAO_FULL_NODE); - } - - /** - * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state - * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers - * is not supported. - */ - private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { - KeyPair ownerKeys = TestUtils.generateKeyPair(); - - ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); - - ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); - when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); - when(stub.isValidForAddOperation()).thenReturn(true); - when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); - when(stub.getSequenceNumber()).thenReturn(1); - when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); - - return stub; - } - - static class LazyPersistableNetworkPayloadStub extends PersistableNetworkPayloadStub - implements ProcessOncePersistableNetworkPayload { - - LazyPersistableNetworkPayloadStub(byte[] hash) { - super(hash); - } - } - - // TESTCASE: GetDataResponse processing. This large tests includes all interesting variations of state - @Test - public void onMessage_GetDataResponseTest() throws NoSuchAlgorithmException { - - PersistableNetworkPayload pnp_onLocalNodeOnly = new PersistableNetworkPayloadStub(new byte[] { 1 }); - PersistableNetworkPayload pnp_inRequestOnly = new LazyPersistableNetworkPayloadStub(new byte[]{2}); - PersistableNetworkPayload pnp_onLocalNodeAndRequest = new PersistableNetworkPayloadStub(new byte[] { 3 }); - ProtectedStorageEntry pse_onLocalNodeOnly = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry pse_inRequestOnly = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry pse_onLocalNodeAndRequest = getProtectedStorageEntryForAdd(); - - this.testState.getMockedStorage().addPersistableNetworkPayload( - pnp_onLocalNodeOnly, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addPersistableNetworkPayload( - pnp_onLocalNodeAndRequest, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addProtectedStorageEntry(pse_onLocalNodeOnly, this.localNodeAddress, null, false); - this.testState.getMockedStorage().addProtectedStorageEntry(pse_onLocalNodeAndRequest, this.localNodeAddress, null, false); - - RequestDataHandler handler = new RequestDataHandler( - this.networkNode, - this.testState.getMockedStorage(), - mock(PeerManager.class), - mock(RequestDataHandler.Listener.class) - ); - - GetDataResponse getDataResponse = new GetDataResponse( - new HashSet<>(Arrays.asList(pse_inRequestOnly, pse_onLocalNodeAndRequest)), - new HashSet<>(Arrays.asList(pnp_inRequestOnly, pnp_onLocalNodeAndRequest)), - handler.nonce, false); - - NodeAddress peersNodeAddress = new NodeAddress("peer", 10); - - // Make a request with the sole reason to set the peersNodeAddress - SettableFuture sendFuture = mock(SettableFuture.class); - when(networkNode.sendMessage(any(NodeAddress.class), any(GetDataRequest.class))).thenReturn(sendFuture); - handler.requestData(peersNodeAddress, true); - - Connection connection = mock(Connection.class); - when(connection.getPeersNodeAddressOptional()).thenReturn(Optional.of(peersNodeAddress)); - handler.onMessage(getDataResponse, connection); - - Assert.assertEquals(3, this.testState.getMockedStorage().getMap().size()); - Assert.assertTrue(this.testState.getMockedStorage().getAppendOnlyDataStoreMap().containsValue(pnp_onLocalNodeOnly)); - Assert.assertTrue(this.testState.getMockedStorage().getAppendOnlyDataStoreMap().containsValue(pnp_inRequestOnly)); - Assert.assertTrue(this.testState.getMockedStorage().getAppendOnlyDataStoreMap().containsValue(pnp_onLocalNodeAndRequest)); - - Assert.assertEquals(3, this.testState.getMockedStorage().getMap().size()); - Assert.assertTrue(this.testState.getMockedStorage().getMap().containsValue(pse_onLocalNodeOnly)); - Assert.assertTrue(this.testState.getMockedStorage().getMap().containsValue(pse_inRequestOnly)); - Assert.assertTrue(this.testState.getMockedStorage().getMap().containsValue(pse_onLocalNodeAndRequest)); - } -} diff --git a/p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java b/p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java deleted file mode 100644 index e757ce07f04..00000000000 --- a/p2p/src/test/java/bisq/network/p2p/peers/getdata/RequestDataHandlerRequestDataTest.java +++ /dev/null @@ -1,249 +0,0 @@ -/* - * This file is part of Bisq. - * - * Bisq is free software: you can redistribute it and/or modify it - * under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at - * your option) any later version. - * - * Bisq is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public - * License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with Bisq. If not, see . - */ - -package bisq.network.p2p.peers.getdata; - -import bisq.network.p2p.NodeAddress; -import bisq.network.p2p.TestUtils; -import bisq.network.p2p.network.Connection; -import bisq.network.p2p.network.NetworkNode; -import bisq.network.p2p.peers.PeerManager; -import bisq.network.p2p.peers.getdata.messages.GetDataRequest; -import bisq.network.p2p.peers.getdata.messages.GetUpdatedDataRequest; -import bisq.network.p2p.peers.getdata.messages.PreliminaryGetDataRequest; -import bisq.network.p2p.storage.P2PDataStorage; -import bisq.network.p2p.storage.TestState; -import bisq.network.p2p.storage.mocks.PersistableNetworkPayloadStub; -import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; -import bisq.network.p2p.storage.payload.PersistableNetworkPayload; -import bisq.network.p2p.storage.payload.ProtectedStorageEntry; -import bisq.network.p2p.storage.payload.ProtectedStoragePayload; - -import bisq.common.app.Capabilities; -import bisq.common.app.Capability; - -import com.google.common.util.concurrent.SettableFuture; - -import java.security.KeyPair; -import java.security.NoSuchAlgorithmException; - -import java.util.Set; - -import org.junit.Assert; -import org.junit.Before; -import org.junit.Test; - - - -import org.mockito.ArgumentCaptor; -import org.mockito.Mock; -import org.mockito.MockitoAnnotations; - -import static org.mockito.Mockito.*; - -public class RequestDataHandlerRequestDataTest { - private TestState testState; - - @Mock - NetworkNode networkNode; - - private NodeAddress localNodeAddress; - - @Before - public void setUp() { - MockitoAnnotations.initMocks(this); - this.testState = new TestState(); - - this.localNodeAddress = new NodeAddress("localhost", 8080); - when(networkNode.getNodeAddress()).thenReturn(this.localNodeAddress); - - // Set up basic capabilities to ensure message contains it - Capabilities.app.addAll(Capability.MEDIATION); - } - - /** - * Returns true if the target bytes are found in the container set. - */ - private boolean byteSetContains(Set container, byte[] target) { - // Set.contains() doesn't do a deep compare, so generate a Set so equals() does what - // we want - Set translatedContainer = - P2PDataStorage.ByteArray.convertBytesSetToByteArraySet(container); - - return translatedContainer.contains(new P2PDataStorage.ByteArray(target)); - } - - /** - * Generates a unique ProtectedStorageEntry that is valid for add. This is used to initialize P2PDataStorage state - * so the tests can validate the correct behavior. Adds of identical payloads with different sequence numbers - * is not supported. - */ - private ProtectedStorageEntry getProtectedStorageEntryForAdd() throws NoSuchAlgorithmException { - KeyPair ownerKeys = TestUtils.generateKeyPair(); - - ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); - - ProtectedStorageEntry stub = mock(ProtectedStorageEntry.class); - when(stub.getOwnerPubKey()).thenReturn(ownerKeys.getPublic()); - when(stub.isValidForAddOperation()).thenReturn(true); - when(stub.matchesRelevantPubKey(any(ProtectedStorageEntry.class))).thenReturn(true); - when(stub.getSequenceNumber()).thenReturn(1); - when(stub.getProtectedStoragePayload()).thenReturn(protectedStoragePayload); - - return stub; - } - - // TESTCASE: P2PDataStorage with no entries returns an empty PreliminaryGetDataRequest - @Test - public void requestData_EmptyP2PDataStore_PreliminaryGetDataRequest() { - SettableFuture sendFuture = mock(SettableFuture.class); - - final ArgumentCaptor captor = ArgumentCaptor.forClass(PreliminaryGetDataRequest.class); - when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); - - RequestDataHandler handler = new RequestDataHandler( - this.networkNode, - this.testState.getMockedStorage(), - mock(PeerManager.class), - mock(RequestDataHandler.Listener.class) - ); - - handler.requestData(this.localNodeAddress, true); - - // expect empty message since p2pDataStore is empty - PreliminaryGetDataRequest getDataRequest = captor.getValue(); - - Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); - Assert.assertEquals(getDataRequest.getSupportedCapabilities(), Capabilities.app); - Assert.assertTrue(getDataRequest.getExcludedKeys().isEmpty()); - } - - // TESTCASE: P2PDataStorage with no entries returns an empty PreliminaryGetDataRequest - @Test - public void requestData_EmptyP2PDataStore_GetUpdatedDataRequest() { - SettableFuture sendFuture = mock(SettableFuture.class); - - final ArgumentCaptor captor = ArgumentCaptor.forClass(GetUpdatedDataRequest.class); - when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); - - RequestDataHandler handler = new RequestDataHandler( - this.networkNode, - this.testState.getMockedStorage(), - mock(PeerManager.class), - mock(RequestDataHandler.Listener.class) - ); - - handler.requestData(this.localNodeAddress, false); - - // expect empty message since p2pDataStore is empty - GetUpdatedDataRequest getDataRequest = captor.getValue(); - - Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); - Assert.assertEquals(getDataRequest.getSenderNodeAddress(), this.localNodeAddress); - Assert.assertTrue(getDataRequest.getExcludedKeys().isEmpty()); - } - - // TESTCASE: P2PDataStorage with PersistableNetworkPayloads and ProtectedStorageEntry generates - // correct GetDataRequestMessage with both sets of keys. - @Test - public void requestData_FilledP2PDataStore_PreliminaryGetDataRequest() throws NoSuchAlgorithmException { - SettableFuture sendFuture = mock(SettableFuture.class); - - PersistableNetworkPayload toAdd1 = new PersistableNetworkPayloadStub(new byte[] { 1 }); - PersistableNetworkPayload toAdd2 = new PersistableNetworkPayloadStub(new byte[] { 2 }); - ProtectedStorageEntry toAdd3 = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry toAdd4 = getProtectedStorageEntryForAdd(); - - this.testState.getMockedStorage().addPersistableNetworkPayload( - toAdd1, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addPersistableNetworkPayload( - toAdd2, this.localNodeAddress, false, false, false, false); - - this.testState.getMockedStorage().addProtectedStorageEntry(toAdd3, this.localNodeAddress, null, false); - this.testState.getMockedStorage().addProtectedStorageEntry(toAdd4, this.localNodeAddress, null, false); - - final ArgumentCaptor captor = ArgumentCaptor.forClass(PreliminaryGetDataRequest.class); - when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); - - RequestDataHandler handler = new RequestDataHandler( - this.networkNode, - this.testState.getMockedStorage(), - mock(PeerManager.class), - mock(RequestDataHandler.Listener.class) - ); - - handler.requestData(this.localNodeAddress, true); - - // expect empty message since p2pDataStore is empty - PreliminaryGetDataRequest getDataRequest = captor.getValue(); - - Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); - Assert.assertEquals(getDataRequest.getSupportedCapabilities(), Capabilities.app); - Assert.assertEquals(4, getDataRequest.getExcludedKeys().size()); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd1.getHash())); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd2.getHash())); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), - P2PDataStorage.get32ByteHash(toAdd3.getProtectedStoragePayload()))); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), - P2PDataStorage.get32ByteHash(toAdd4.getProtectedStoragePayload()))); - } - - // TESTCASE: P2PDataStorage with PersistableNetworkPayloads and ProtectedStorageEntry generates - // correct GetDataRequestMessage with both sets of keys. - @Test - public void requestData_FilledP2PDataStore_GetUpdatedDataRequest() throws NoSuchAlgorithmException { - SettableFuture sendFuture = mock(SettableFuture.class); - - PersistableNetworkPayload toAdd1 = new PersistableNetworkPayloadStub(new byte[] { 1 }); - PersistableNetworkPayload toAdd2 = new PersistableNetworkPayloadStub(new byte[] { 2 }); - ProtectedStorageEntry toAdd3 = getProtectedStorageEntryForAdd(); - ProtectedStorageEntry toAdd4 = getProtectedStorageEntryForAdd(); - - this.testState.getMockedStorage().addPersistableNetworkPayload( - toAdd1, this.localNodeAddress, false, false, false, false); - this.testState.getMockedStorage().addPersistableNetworkPayload( - toAdd2, this.localNodeAddress, false, false, false, false); - - this.testState.getMockedStorage().addProtectedStorageEntry(toAdd3, this.localNodeAddress, null, false); - this.testState.getMockedStorage().addProtectedStorageEntry(toAdd4, this.localNodeAddress, null, false); - - final ArgumentCaptor captor = ArgumentCaptor.forClass(GetUpdatedDataRequest.class); - when(networkNode.sendMessage(any(NodeAddress.class), captor.capture())).thenReturn(sendFuture); - - RequestDataHandler handler = new RequestDataHandler( - this.networkNode, - this.testState.getMockedStorage(), - mock(PeerManager.class), - mock(RequestDataHandler.Listener.class) - ); - - handler.requestData(this.localNodeAddress, false); - - // expect empty message since p2pDataStore is empty - GetUpdatedDataRequest getDataRequest = captor.getValue(); - - Assert.assertEquals(getDataRequest.getNonce(), handler.nonce); - Assert.assertEquals(getDataRequest.getSenderNodeAddress(), this.localNodeAddress); - Assert.assertEquals(4, getDataRequest.getExcludedKeys().size()); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd1.getHash())); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), toAdd2.getHash())); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), - P2PDataStorage.get32ByteHash(toAdd3.getProtectedStoragePayload()))); - Assert.assertTrue(byteSetContains(getDataRequest.getExcludedKeys(), - P2PDataStorage.get32ByteHash(toAdd4.getProtectedStoragePayload()))); - } -} 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 15e80a9c6ac..51d831df41a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/TestState.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/TestState.java @@ -51,8 +51,6 @@ import java.util.Set; import java.util.concurrent.TimeUnit; -import lombok.Getter; - import org.junit.Assert; import org.mockito.ArgumentCaptor; @@ -67,7 +65,6 @@ public class TestState { static final int MAX_SEQUENCE_NUMBER_MAP_SIZE_BEFORE_PURGE = 5; - @Getter P2PDataStorage mockedStorage; final Broadcaster mockBroadcaster; @@ -77,7 +74,7 @@ public class TestState { private final ProtectedDataStoreService protectedDataStoreService; final ClockFake clockFake; - public TestState() { + TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); this.clockFake = new ClockFake();