Integration of RecursivePeerRefreshState and PeerDiscoveryController #420

smatthewenglish · 2018-12-13T23:09:32Z

As specified by [Implement iterative peer search] (NC-875).

mbaxter

My main concern is with the core RecursivePeerRefreshState functionality - we can discuss offline.

mbaxter · 2018-12-18T20:28:23Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

+                  }
+                  addToPeerTable(peer);
+                  if (interaction.isBootstrap()) {
+                    findNodes(peer, agent.getAdvertisedPeer().getId());


I think this findNodes request should be handled in recursivePeerRefreshState

mbaxter · 2018-12-18T20:37:53Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

    final PeerInteractionState interaction =
-        new PeerInteractionState(action, PacketType.NEIGHBORS, packet -> true, true, false);
-    dispatchInteraction(peer, interaction);
+        new PeerInteractionState(action, PacketType.NEIGHBORS, filter -> true, true, false);


I think the original packet var name is clearer

mbaxter · 2018-12-18T20:40:37Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

-        new PeerInteractionState(action, PacketType.NEIGHBORS, packet -> true, true, false);
-    dispatchInteraction(peer, interaction);
+        new PeerInteractionState(action, PacketType.NEIGHBORS, filter -> true, true, false);
+    interaction.execute(0);


Not sure if you still need this interaction object or not? It does contain some retry functionality. If you think its worth using interaction, you'll need to clean up the retry timer.

mbaxter · 2018-12-18T20:43:57Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

      final BytesValue target,
-      final PeerBlacklist peerBlacklist,


Any reason to cut the blacklist here? This should be passed by reference, so no need to keep sending in the same instance of the blacklist via onNeighboursPacketReceived.

mbaxter · 2018-12-18T20:48:56Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

  private final BondingAgent bondingAgent;
  private final NeighborFinder neighborFinder;
  private final List<PeerDistance> anteList;
  private final List<OutstandingRequest> outstandingRequestList;
-  private final List<BytesValue> contactedInCurrentExecution;
+  private final List<BytesValue> dispatchedFindNeighbours;
+  private final List<BytesValue> dispatchedBond;


Should these be maps rather than lists? A HashMap will have O(1) rather than O(n) lookup speed.

mbaxter · 2018-12-18T22:45:18Z

.../java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

+
+    final RecursivePeerRefreshState recursivePeerRefreshState =
+        controller.getRecursivePeerRefreshState();
+    recursivePeerRefreshState.addToOutstandingRequestList(peers[0]);


Why was this block added?

mbaxter · 2018-12-18T23:01:41Z

...c/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryBootstrappingTest.java

+
+    await()
+        .atMost(5, TimeUnit.SECONDS)
+        .untilAsserted(() -> assertThat(peerDiscoveryAgent_TEST.getPeers()).hasSize(2));


What is this test meant to prove? It looks like you set up agent0 with no peers, agent1 with peers [agent0], then wait for those two nodes to exchange messages so that they are each tracking the other. Then agent TEST is setup with peers [agent0], and TEST will ask agent0 for all of its peers. This doesn't seem get at the iterative search though since agent0 already has the other peer in its table, so no recursion is needed.

This test was initially failing as a result of the changes I made to the controller, so I deconstructed it to figure out exactly why.

So, can we cut this? Sounds like you added this as a debugging step?

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

mbaxter · 2018-12-18T23:30:56Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

+
+    recursivePeerRefreshState =
+        new RecursivePeerRefreshState(Peer.randomId(), this::bond, this::findNodes);
+    recursivePeerRefreshState.kickstartBootstrapPeers(


Don't you also need to run the recursive lookup when refreshTable is invoked?

mbaxter · 2018-12-19T01:08:10Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java


-  RecursivePeerRefreshState(
+  public RecursivePeerRefreshState(


General comment - this class doesn't seem to follow the pattern described in the docs: https://github.com/ethereum/devp2p/blob/master/discv4.md#recursive-lookup

I don't see a terminating condition for when this search would conclude.

I've added a short blerb to initiatePeerRefreshCycle that describes the terminating condition.

mbaxter · 2018-12-20T23:57:48Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

-              && !outstandingRequestList.contains(new OutstandingRequest(candidate))) {
-            outstandingRequestList.remove(i);
+              && !outstandingNeighboursRequestList.contains(new OutstandingRequest(candidate))) {
+            outstandingNeighboursRequestList.remove(i);
            executeFindNodeRequest(candidate);


Adding some notes based on our offline discussion:

We talked about reworking the way that "rounds" are handled. Where a round is defined as the point at which we issue requests to a new set of peers. As it stands now, we can cycle in one or more new requests as timeouts are processed. This means that you can end up sending out < concurrentRequestLimit requests at this point.

Instead, we should issue up to concurrentRequestLimit messages at each round. We can do this by initiating a new round when either: a) all peers contacted during the previous round have responded with their neighbors or b) the previous round has timed out.

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

mbaxter · 2018-12-21T00:11:10Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

@@ -151,19 +156,15 @@ void onNeighboursPacketReceived(

  private void queryNearestNodes() {
    final int concurrentRequestLimit = 3;
-    if (outstandingRequestList.isEmpty()) {
+    if (outstandingNeighboursRequestList.isEmpty()) {
      final List<DiscoveryPeer> queryCandidates =
          determineFindNodeCandidates(concurrentRequestLimit);


Adding some notes based on our offline discussion:

It looks to me like the algorithm for selecting nodes to query (defined in determineFindNodeCandidates) is not quite right. The docs describe how to select the next set of nodes to query:

Of the k nodes the initiator has heard of closest to the target, it picks α that it has not yet queried and resends FindNode to them.

As it stands currently, it looks like you're always selecting the peers closest to the target regardless of whether we have contacted those peers already, or if those peers are contained in the set of the k closest peers.

…tion

mbaxter

Looks like its getting close! I didn't get through everything, but left a few comments in the meantime.

mbaxter · 2019-02-05T23:25:22Z

...main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryController.java

@@ -342,24 +340,29 @@ private void refreshTableIfRequired() {
    }
  }

+  @VisibleForTesting
+  public RecursivePeerRefreshState getRecursivePeerRefreshState() {


Can the visibility here be limited? Looks like package-private should work.

mbaxter · 2019-02-05T23:36:11Z

...ava/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryTableRefreshTest.java

        captor
            .getAllValues()
            .stream()
-            .filter(p -> p.getType().equals(PacketType.FIND_NEIGHBORS))


It seems that this test isn't checking what it was originally intended to check. I think you probably want to revert this back to filtering for neighbors packets.

that's inaccurate. this test does check what it was originally intended to check, however the mechanism that effectuates the action that it evaluates has changed. the now defunct comment that reads "as the controller performs refreshes, it'll send FIND_NEIGHBORS packets with random target IDs every time" provides a clue, since that doesn't describe the refresh functionality introduced in this pr. the new comment highlights specifically what has changed.

Originally, this test was causing multiple rounds of table refreshes, and inspecting the dispatched FIND_NEIGHBORS requests in each round, verifying that a unique target was picked on each refresh. As the code currently stands, you're checking that several rounds of PINGS have been sent out, and doing extra work to verify that each PING is sent to the singular peer in the table, which doesn't seem all that useful.

I suggest adding PONG responses in your for loop from the singular peer so that you can capture the FIND_NEIGHBORS requests and run the original target checks.

mbaxter · 2019-02-06T00:39:17Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

+    for (final Map.Entry<BytesValue, MetadataPeer> entry : oneTrueMap.entrySet()) {
+      final MetadataPeer metadataPeer = entry.getValue();
+      if (metadataPeer.hasOutstandingBondRequest()) {
+        metadataPeer.bondingComplete();


It's a little confusing that we're setting bonding to "complete" here when we're "cancelling" the outstanding request. It may be worth adding an explanatory comment.

mbaxter · 2019-02-06T00:47:28Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

-  private final BytesValue target;
+public class RecursivePeerRefreshState {
+  private static final Logger LOG = LogManager.getLogger();
+  private static final int MAX_CONCURRENT_REQUESTS = 3;


Are these parameters where we want them (MAX_CONCURRENT_REQUESTS and maxRounds)? Seems like we could have a higher level of concurrency and a lower number of rounds.

mbaxter · 2019-02-06T00:53:36Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

  }

  @VisibleForTesting
-  List<OutstandingRequest> getOutstandingRequestList() {
-    return outstandingRequestList;
+  void cancelCurrentRound() {


I think plain cancel might be a better name. Since we're cancelling the search altogether, not just the current round within the search.

mbaxter · 2019-02-06T01:09:04Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/DefaultPeerId.java

@@ -39,6 +41,11 @@ public Bytes32 keccak256() {
    return keccak256;
  }

+  @VisibleForTesting
+  public void setKeccak256(final Bytes32 keccak256) {


Can we mock this where we need it for testing? Instead of adding this setter to the production code?

It has been mocked before, but the new paradigm of peer refresh requires that we have actual peers being created in the createPeersInLastBucket method of PeerDiscoveryControllerTest, so that we can query them for associated metadata.

You can stub out the behavior of real peer objects there like so:

final DiscoveryPeer peer = spy(new DiscoveryPeer( id, new Endpoint( localPeer.getEndpoint().getHost(), 100 + counter.incrementAndGet(), OptionalInt.empty()))); doReturn(keccak).when(peer).keccak256();

mbaxter · 2019-02-06T01:16:58Z

.../java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

    for (int i = 0; i < timeouts; i++) {
      timer.runTimerHandlers();
    }
-    int expectedTimerEvents = (timeouts + 1) * peerCount;
+    final int expectedTimerEvents = 1 + (timeouts + 1) * peerCount;


Why add the 1 here? Is it more accurate to change times to atLeast on line 125: verify(timer, atLeast(expectedTimerEvents)).setTimer(anyLong(), any());?

mbaxter · 2019-02-06T01:20:03Z

.../java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

-    final Packet packet = Packet.create(PacketType.PONG, packetData, keyPairs.get(0));
-    controller.onMessage(packet, peers.get(0));
+
+    for (int i = 0; i < 3; i++) {


Suggested change

for (int i = 0; i < 3; i++) {

for (int i = 0; i < peers.size(); i++) {

Actually - I'm wondering why you changed this test to have all peers responding instead of just one?

the "bonding round" needs to conclude before the "find neighbours round" can begin. one way to achieve that is for all peers to respond, the other way is for the round to timeout.

.../java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

mbaxter · 2019-02-06T01:22:04Z

.../java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerDiscoveryControllerTest.java

+          PongPacketData.create(localPeer.getEndpoint(), mockPacket.getHash());
+      final Packet packet0 = Packet.create(PacketType.PONG, packetData, keyPairs.get(i));
+      controller.onMessage(packet0, peers.get(i));
+    }

    // Ensure that the peer controller is now sending FIND_NEIGHBORS messages for this peer.
    verify(outboundMessageHandler, times(1))


You can loop over your peers here (and below) instead of explicitly checking each one by hard-coded index

…discovery/internal/PeerDiscoveryControllerTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

…tion

mbaxter · 2019-02-08T19:47:25Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/PeerTable.java

@@ -180,7 +179,6 @@ private void buildBloomFilter() {
  public List<DiscoveryPeer> nearestPeers(final BytesValue target, final int limit) {
    final BytesValue keccak256 = Hash.keccak256(target);
    return getAllPeers().stream()
-        .filter(p -> p.getStatus() == PeerDiscoveryStatus.BONDED)


Why is this status filtering being removed?

mbaxter · 2019-02-08T20:25:28Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

+  }
+
+  @VisibleForTesting
+  public BytesValue getTarget() {


Looks like this method is unused - can we cut it?

mbaxter · 2019-02-08T20:47:58Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

+    LOG.debug("Received neighbours packet with {} neighbours", neighboursPacket.getNodes().size());
+    for (final DiscoveryPeer receivedDiscoPeer : neighboursPacket.getNodes()) {
+      if (!oneTrueMap.containsKey(receivedDiscoPeer.getId())
+          && !peerBlacklist.contains(receivedDiscoPeer)


Previously, we were also filtering out known peers here (peers already in the peer table). It seems to me that we should still be checking these peers against the peer table.

mbaxter · 2019-02-08T21:43:35Z

ethereum/p2p/src/main/java/tech/pegasys/pantheon/ethereum/p2p/peers/DefaultPeerId.java

@@ -39,6 +41,11 @@ public Bytes32 keccak256() {
    return keccak256;
  }

+  @VisibleForTesting
+  public void setKeccak256(final Bytes32 keccak256) {


You can stub out the behavior of real peer objects there like so:

final DiscoveryPeer peer = spy(new DiscoveryPeer( id, new Endpoint( localPeer.getEndpoint().getHost(), 100 + counter.incrementAndGet(), OptionalInt.empty()))); doReturn(keccak).when(peer).keccak256();

mbaxter · 2019-02-08T21:45:12Z

...c/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryBootstrappingTest.java

+
+    await()
+        .atMost(5, TimeUnit.SECONDS)
+        .untilAsserted(() -> assertThat(peerDiscoveryAgent_TEST.getPeers()).hasSize(2));


So, can we cut this? Sounds like you added this as a debugging step?

...ava/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshStateTest.java

mbaxter · 2019-02-09T19:38:52Z

...in/java/tech/pegasys/pantheon/ethereum/p2p/discovery/internal/RecursivePeerRefreshState.java

+    this.maxRounds = maxRounds;
+  }
+
+  synchronized void start(final List<DiscoveryPeer> initialPeers, final BytesValue target) {


I started to add notes on some thread-safety issues I noticed, but I looked into it more and this should all actually be single-threaded within the vertx event loop. So you should be able to cut all of the synchronized keywords.

…discovery/internal/RecursivePeerRefreshStateTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

…tion

smatthewenglish added the work in progress Work on this pull request is ongoing label Dec 13, 2018

smatthewenglish force-pushed the iterative-integration branch 8 times, most recently from 15a7c0b to c04e6f3 Compare December 15, 2018 00:22

smatthewenglish removed the work in progress Work on this pull request is ongoing label Dec 15, 2018

smatthewenglish force-pushed the iterative-integration branch 4 times, most recently from 7062a0e to 6081235 Compare December 18, 2018 16:17

mbaxter suggested changes Dec 19, 2018

View reviewed changes

smatthewenglish force-pushed the iterative-integration branch 2 times, most recently from a64ab90 to 8c75ee1 Compare December 20, 2018 02:10

smatthewenglish added the work in progress Work on this pull request is ongoing label Dec 20, 2018

mbaxter reviewed Dec 21, 2018

View reviewed changes

smatthewenglish force-pushed the iterative-integration branch 8 times, most recently from 084fdee to ba895b8 Compare January 14, 2019 19:05

smatthewenglish force-pushed the iterative-integration branch 2 times, most recently from ada398d to d91761c Compare January 17, 2019 20:20

smatthewenglish removed the work in progress Work on this pull request is ongoing label Jan 17, 2019

smatthewenglish added 3 commits February 5, 2019 13:40

update

3113e80

fix to tableRefreshSingleNode

46409d7

Merge remote-tracking branch 'upstream/master' into iterative-integra…

734ab01

…tion

mbaxter reviewed Feb 6, 2019

View reviewed changes

smatthewenglish and others added 5 commits February 6, 2019 11:52

update coment

47482cd

Update ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/…

913d572

…discovery/internal/PeerDiscoveryControllerTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

update

d03888c

Merge remote-tracking branch 'upstream/master' into iterative-integra…

d55d7dd

…tion

merge

5c49e1d

mbaxter reviewed Feb 9, 2019

View reviewed changes

smatthewenglish and others added 16 commits February 11, 2019 10:32

tableRefreshSingleNode

9ab6760

deconstructedIncrementalUpdateBootstrapPeersList

88b90fc

setKeccak256

f5079c4

Update ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/…

d660ca5

…discovery/internal/RecursivePeerRefreshStateTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

Update ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/…

f7413b3

…discovery/internal/RecursivePeerRefreshStateTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

Update ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/…

c5c7906

…discovery/internal/RecursivePeerRefreshStateTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

Update ethereum/p2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/…

c03c932

…discovery/internal/RecursivePeerRefreshStateTest.java Co-Authored-By: s-matthew-english <s.matthew.english@gmail.com>

synchronized

8ce92f4

update

3503716

merge

38130c9

merge

acfa899

filtering out known peers

bcae3d5

update

eda6ae1

update

c77f9a9

Merge remote-tracking branch 'upstream/master' into iterative-integra…

b8e9aff

…tion

update II

65714cf

mbaxter approved these changes Feb 12, 2019

View reviewed changes

smatthewenglish added 2 commits February 12, 2019 16:27

merge ii

b764320

Merge branch 'master' into iterative-integration

6d2bbbd

smatthewenglish merged commit c4ad4f6 into PegaSysEng:master Feb 12, 2019

	for (int i = 0; i < 3; i++) {
	for (int i = 0; i < peers.size(); i++) {

Integration of RecursivePeerRefreshState and PeerDiscoveryController #420

Integration of RecursivePeerRefreshState and PeerDiscoveryController #420

Conversation

smatthewenglish commented Dec 13, 2018

mbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment