From 08fa12bff68166e65e85efc8cd9fa351b9948bf0 Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Fri, 22 Mar 2019 09:40:01 +1000 Subject: [PATCH 1/9] debug --- .../p2p/discovery/internal/PeerDiscoveryController.java | 2 ++ .../p2p/discovery/internal/RecursivePeerRefreshState.java | 2 ++ .../permissioning/node/NodePermissioningController.java | 5 ++++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 67326c5307..cff46abc61 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -191,6 +191,8 @@ public void start() { bootstrapNodes.stream() .filter(p -> isPeerPermitted(localPeer, p)) .collect(Collectors.toList()); + LOG.debug( + "PeerDiscoController.start with initial discovery peers {}", initialDiscoveryPeers.size()); if (nodePermissioningController.isPresent()) { nodePermissioningController diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java index 900bfeebd3..2a68dc3d0c 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java @@ -79,6 +79,7 @@ public class RecursivePeerRefreshState { } void start(final List initialPeers, final BytesValue target) { + LOG.debug("start with peers {}", initialPeers.size()); if (iterativeSearchInProgress) { LOG.debug("Skipping discovery because previous search is still in progress."); return; @@ -98,6 +99,7 @@ private boolean reachedMaximumNumberOfRounds() { private void addInitialPeers(final List initialPeers) { this.initialPeers = initialPeers; + LOG.debug("addInitialPeers with peers {}", initialPeers.size()); for (final DiscoveryPeer peer : initialPeers) { final MetadataPeer iterationParticipant = new MetadataPeer(peer, distance(target, peer.getId())); diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java index 07a60f7118..a707a058c2 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java @@ -37,18 +37,21 @@ public NodePermissioningController( } public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinationEnode) { - LOG.trace("Checking node permission: {} -> {}", sourceEnode, destinationEnode); + LOG.info("Checking node permission: {} -> {}", sourceEnode, destinationEnode); if (syncStatusNodePermissioningProvider.isPresent() && !syncStatusNodePermissioningProvider.get().isPermitted(sourceEnode, destinationEnode)) { + LOG.info("not permitted by sync provider"); return false; } else { for (NodePermissioningProvider provider : providers) { if (!provider.isPermitted(sourceEnode, destinationEnode)) { + LOG.info("not permitted by provider {}", provider); return false; } } } + LOG.info("YES permitted: {} -> {}", sourceEnode, destinationEnode); return true; } From 57b9e422af39c1c02469ecbc00b2b07ceadcf9db Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Mon, 25 Mar 2019 10:38:04 +1000 Subject: [PATCH 2/9] bond with permitted bootnodes --- .../internal/PeerDiscoveryController.java | 19 ++++++++++++++----- .../internal/PeerDiscoveryControllerTest.java | 8 ++++++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index cff46abc61..70e49f942e 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -173,7 +173,11 @@ public void start() { throw new IllegalStateException("The peer table had already been started"); } - bootstrapNodes.stream().filter(p -> isPeerPermitted(localPeer, p)).forEach(peerTable::tryAdd); + final List initialDiscoveryPeers = + bootstrapNodes.stream() + .filter(p -> isPeerPermitted(localPeer, p)) + .collect(Collectors.toList()); + initialDiscoveryPeers.stream().forEach(peerTable::tryAdd); recursivePeerRefreshState = new RecursivePeerRefreshState( @@ -187,18 +191,23 @@ public void start() { PEER_REFRESH_ROUND_TIMEOUT_IN_SECONDS, 100); - final List initialDiscoveryPeers = - bootstrapNodes.stream() - .filter(p -> isPeerPermitted(localPeer, p)) - .collect(Collectors.toList()); LOG.debug( "PeerDiscoController.start with initial discovery peers {}", initialDiscoveryPeers.size()); if (nodePermissioningController.isPresent()) { + + // if smart contract permissioning is enabled, bond with bootnodes + if (nodePermissioningController.get().getSyncStatusNodePermissioningProvider().isPresent()) { + for (DiscoveryPeer p : initialDiscoveryPeers) { + bond(p); + } + } + nodePermissioningController .get() .startPeerDiscoveryCallback( () -> recursivePeerRefreshState.start(initialDiscoveryPeers, localPeer.getId())); + } else { recursivePeerRefreshState.start(initialDiscoveryPeers, localPeer.getId()); } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index 9fc01b51f7..73345b26ce 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -1028,8 +1028,12 @@ public void shouldNotBondWithNonPermittedPeer() { MockPacketDataFactory.mockPongPacket(discoveryPeer, discoPeerPing.getHash()); controller.onMessage(pongFromDiscoPeer, discoveryPeer); + // since NodePermissioningController is turned on, we get a PING NOT FIND_NEIGHBORS verify(outboundMessageHandler, times(1)) - .send(eq(discoveryPeer), matchPacketOfType(PacketType.FIND_NEIGHBORS)); + .send(eq(discoveryPeer), matchPacketOfType(PacketType.PING)); + +// verify(outboundMessageHandler, times(1)) +// .send(eq(discoveryPeer), matchPacketOfType(PacketType.FIND_NEIGHBORS)); // Setup ping to be sent to otherPeer after neighbors packet is received keyPairs = PeerDiscoveryTestHelper.generateKeyPairs(1); @@ -1048,7 +1052,7 @@ public void shouldNotBondWithNonPermittedPeer() { controller.onMessage(neighborsPacket, discoveryPeer); verify(controller, times(0)).bond(notPermittedPeer); - verify(controller, times(1)).bond(permittedPeer); + verify(controller, times(1)).bond(discoveryPeer); } @Test From ae298164ccb3d054306e21fa635bad3a75137e9c Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Mon, 25 Mar 2019 10:49:37 +1000 Subject: [PATCH 3/9] exposed SyncStatusProvider --- .../p2p/discovery/internal/PeerDiscoveryControllerTest.java | 3 --- .../permissioning/node/NodePermissioningController.java | 6 ++---- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index 73345b26ce..45cc511da4 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -1032,9 +1032,6 @@ public void shouldNotBondWithNonPermittedPeer() { verify(outboundMessageHandler, times(1)) .send(eq(discoveryPeer), matchPacketOfType(PacketType.PING)); -// verify(outboundMessageHandler, times(1)) -// .send(eq(discoveryPeer), matchPacketOfType(PacketType.FIND_NEIGHBORS)); - // Setup ping to be sent to otherPeer after neighbors packet is received keyPairs = PeerDiscoveryTestHelper.generateKeyPairs(1); pingPacketData = PingPacketData.create(localEndpoint, notPermittedPeer.getEndpoint()); diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java index a707a058c2..f9c8503775 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java @@ -41,12 +41,10 @@ public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio if (syncStatusNodePermissioningProvider.isPresent() && !syncStatusNodePermissioningProvider.get().isPermitted(sourceEnode, destinationEnode)) { - LOG.info("not permitted by sync provider"); return false; } else { for (NodePermissioningProvider provider : providers) { if (!provider.isPermitted(sourceEnode, destinationEnode)) { - LOG.info("not permitted by provider {}", provider); return false; } } @@ -63,8 +61,8 @@ public void startPeerDiscoveryCallback(final Runnable peerDiscoveryCallback) { } } - @VisibleForTesting - Optional getSyncStatusNodePermissioningProvider() { + + public Optional getSyncStatusNodePermissioningProvider() { return syncStatusNodePermissioningProvider; } From 9c8030598d2662457902269fb35c8eba5347f964 Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Mon, 25 Mar 2019 11:01:50 +1000 Subject: [PATCH 4/9] formatting --- .../permissioning/node/NodePermissioningController.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java index f9c8503775..68036f64c5 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java @@ -18,7 +18,6 @@ import java.util.List; import java.util.Optional; -import com.google.common.annotations.VisibleForTesting; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -61,7 +60,6 @@ public void startPeerDiscoveryCallback(final Runnable peerDiscoveryCallback) { } } - public Optional getSyncStatusNodePermissioningProvider() { return syncStatusNodePermissioningProvider; } From f3ec5bf6886d58db0ec647f11b1f808b3f5f04ba Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Mon, 25 Mar 2019 11:03:03 +1000 Subject: [PATCH 5/9] removed debug --- .../permissioning/node/NodePermissioningController.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java index 68036f64c5..44ff3b0c16 100644 --- a/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java +++ b/ethereum/permissioning/src/main/java/tech/pegasys/pantheon/ethereum/permissioning/node/NodePermissioningController.java @@ -36,7 +36,7 @@ public NodePermissioningController( } public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinationEnode) { - LOG.info("Checking node permission: {} -> {}", sourceEnode, destinationEnode); + LOG.trace("Checking node permission: {} -> {}", sourceEnode, destinationEnode); if (syncStatusNodePermissioningProvider.isPresent() && !syncStatusNodePermissioningProvider.get().isPermitted(sourceEnode, destinationEnode)) { @@ -48,7 +48,6 @@ public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio } } } - LOG.info("YES permitted: {} -> {}", sourceEnode, destinationEnode); return true; } From 29e6c52922f9612db69f96c492d738497cf7cece Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Mon, 25 Mar 2019 11:15:54 +1000 Subject: [PATCH 6/9] Removed debug --- .../p2p/discovery/internal/RecursivePeerRefreshState.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java index 2a68dc3d0c..900bfeebd3 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java @@ -79,7 +79,6 @@ public class RecursivePeerRefreshState { } void start(final List initialPeers, final BytesValue target) { - LOG.debug("start with peers {}", initialPeers.size()); if (iterativeSearchInProgress) { LOG.debug("Skipping discovery because previous search is still in progress."); return; @@ -99,7 +98,6 @@ private boolean reachedMaximumNumberOfRounds() { private void addInitialPeers(final List initialPeers) { this.initialPeers = initialPeers; - LOG.debug("addInitialPeers with peers {}", initialPeers.size()); for (final DiscoveryPeer peer : initialPeers) { final MetadataPeer iterationParticipant = new MetadataPeer(peer, distance(target, peer.getId())); From 056f02ce0f79fb2569a27eb7068cc31ff5d20963 Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Mon, 25 Mar 2019 11:16:46 +1000 Subject: [PATCH 7/9] Removed debug --- .../p2p/discovery/internal/PeerDiscoveryController.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java index 70e49f942e..e151c8a910 100644 --- a/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java +++ b/ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java @@ -191,9 +191,6 @@ public void start() { PEER_REFRESH_ROUND_TIMEOUT_IN_SECONDS, 100); - LOG.debug( - "PeerDiscoController.start with initial discovery peers {}", initialDiscoveryPeers.size()); - if (nodePermissioningController.isPresent()) { // if smart contract permissioning is enabled, bond with bootnodes From a7f3193a93fa926986cf707c338aa7a8d925f28d Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Tue, 26 Mar 2019 14:01:21 +1000 Subject: [PATCH 8/9] add callback to spy and reinstate original test --- .../p2p/NodePermissioningControllerTestHelper.java | 12 ++++++++++++ .../internal/PeerDiscoveryControllerTest.java | 4 ++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NodePermissioningControllerTestHelper.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NodePermissioningControllerTestHelper.java index 2ae8e7112f..73fb223bbf 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NodePermissioningControllerTestHelper.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/NodePermissioningControllerTestHelper.java @@ -14,6 +14,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -27,6 +28,8 @@ import java.util.Optional; import java.util.stream.Collectors; +import org.mockito.ArgumentCaptor; + public class NodePermissioningControllerTestHelper { private final EnodeURL localNode; @@ -88,6 +91,15 @@ public NodePermissioningController build() { }); } + ArgumentCaptor callback = ArgumentCaptor.forClass(Runnable.class); + doAnswer( + (i) -> { + callback.getValue().run(); + return null; + }) + .when(nodePermissioningController) + .startPeerDiscoveryCallback(callback.capture()); + return nodePermissioningController; } } diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index 45cc511da4..08d898046c 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -1030,7 +1030,7 @@ public void shouldNotBondWithNonPermittedPeer() { // since NodePermissioningController is turned on, we get a PING NOT FIND_NEIGHBORS verify(outboundMessageHandler, times(1)) - .send(eq(discoveryPeer), matchPacketOfType(PacketType.PING)); + .send(eq(discoveryPeer), matchPacketOfType(PacketType.FIND_NEIGHBORS)); // Setup ping to be sent to otherPeer after neighbors packet is received keyPairs = PeerDiscoveryTestHelper.generateKeyPairs(1); @@ -1049,7 +1049,7 @@ public void shouldNotBondWithNonPermittedPeer() { controller.onMessage(neighborsPacket, discoveryPeer); verify(controller, times(0)).bond(notPermittedPeer); - verify(controller, times(1)).bond(discoveryPeer); + verify(controller, times(1)).bond(permittedPeer); } @Test From ebfc6047ae420e486ca2c026fc802467280b2415 Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Tue, 26 Mar 2019 14:44:40 +1000 Subject: [PATCH 9/9] removed comment --- .../p2p/discovery/internal/PeerDiscoveryControllerTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java index d849b1a5f0..bac95fda26 100644 --- a/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java +++ b/ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java @@ -1029,7 +1029,6 @@ public void shouldNotBondWithNonPermittedPeer() { MockPacketDataFactory.mockPongPacket(discoveryPeer, discoPeerPing.getHash()); controller.onMessage(pongFromDiscoPeer, discoveryPeer); - // since NodePermissioningController is turned on, we get a PING NOT FIND_NEIGHBORS verify(outboundMessageHandler, times(1)) .send(eq(discoveryPeer), matchPacketOfType(PacketType.FIND_NEIGHBORS));