Add connectAndFetchPeers and new Discovery function Closes#1152 #1170

ishantiw · 2019-03-07T16:00:04Z

Description

Discovery should just take IP address and wPort and should be able to connect and fetch peer list independently of Peer object. This will remove the need to create Peer objects for the seed peers just to fetch peer list from them. Also, it will be independent of the peer pool in terms of choosing peers with Peer objects already and could be handled independently.

Add new connectAndFetchPeers method that will fetch all the peers from P2PPeerInfo ({ipAddress, wsPort}), this will assign an outbound socket and destroy it after fetching peers.
Update discovery function discoverPeer to use connectAndFetchPeers to fetch peers from the peers passed as arguments.
Update P2P start() method and subsequent _startDiscovery() methods to use and accept new updated discovery method.

Review checklist

The PR resolves Add a function to connectAndFetch peers to be used in discovery process #1152
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

…P2PPeerInfo to do the probe

…nect_fetch_peers_and_new_discovery * improve_discovery_connect_fetch: ♻️ Add usage of new discovery process across p2p lib ♻️ Update exisiting discovery to use connectAndFetchPeers and accept P2PPeerInfo to do the probe 🌱 Add connectAndRequest to fetch peers and status independently

ishantiw · 2019-03-07T16:15:16Z

I have kept it in the draft state because there are a lot of changes that affect overall discovery, connecting with incoming connections and maintaining Peers in PeerPool.

NOTE: Need to resolve a couple of tests related to stopping the node. I have skipped the partial network test (where nodes start at once without going into discovery) because it's not relevant anymore due to the change in initial discovery process.

ishantiw · 2019-03-11T12:38:49Z

I'm reverting back the changes related to the use of independent connectToFetchPeers call in discovery as it was it creating a lot of instability in tests. Since its making independent calls in some interval, the connection doesn't close properly sometimes. Also, in the beginning, if there are no peers in the peer list of any node in the network then it cannot fetch peers of each other and hence throws the error. I'm still keeping the fetch connectAndRequest to fetch status of seed peers and adding them to peer pool in the initial discovery process.

shuse2 · 2019-03-11T13:17:27Z

packages/lisk-p2p/src/peer.ts

+): Promise<ConnectAndFetchResponse> =>
+	new Promise<ConnectAndFetchResponse>(
+		(
+			resolve: (result: ConnectAndFetchResponse) => void,


probably no need to define type for resolve and reject here

shuse2 · 2019-03-11T13:17:53Z

packages/lisk-p2p/src/peer.ts

+			const outboundSocket = socketClusterClient.create(clientOptions);
+			// Attaching handlers for various events that could be used future for logging or any other application
+			outboundSocket.on('error', async (error: Error) =>
+				Promise.resolve(error),


it should use the resolve from input?
also i think this should be reject

Ah, I was using it for debugging but forgot to change it, but now we don't need it as it will be handled within Peer object

shuse2 · 2019-03-11T13:18:03Z

packages/lisk-p2p/src/peer.ts

+				Promise.resolve(error),
+			);
+			outboundSocket.on('close', async () =>
+				Promise.resolve('Connection closed'),


same as above, also it should rejected with Error object?

will be handled in Peer object now

shuse2 · 2019-03-11T13:18:18Z

packages/lisk-p2p/src/peer.ts

+				Promise.resolve('Connection closed'),
+			);
+			outboundSocket.on('connect', async () =>
+				Promise.resolve('Connection Successful'),


should we already resolve on connection...?

same as above

shuse2 · 2019-03-11T13:28:17Z

packages/lisk-p2p/src/peer.ts

+	}
+};
+
+export const connectAndFetchStatus = async (


should this and fetchPeer happen on the separate socket?

Now we are reusing the socket and creating a peer object

shuse2 · 2019-03-11T13:29:15Z

packages/lisk-p2p/test/integration/p2p.ts

@@ -72,7 +72,7 @@ describe('Integration tests for P2P library', () => {
 		});
 	});

-	describe('Partially connected network: All nodes launch at the same time. The seedPeers list of each node contains the next node in the sequence', () => {
+	describe.skip('Partially connected network: All nodes launch at the same time. The seedPeers list of each node contains the next node in the sequence', () => {


if it's not relevant anymore why not remove it?

shuse2 · 2019-03-11T13:38:55Z

packages/lisk-p2p/src/peer.ts

+		},
+	);
+
+export const connectAndFetchPeers = async (


this function is not used...?

jondubois · 2019-03-11T13:29:06Z

packages/lisk-p2p/src/peer.ts

+			const clientOptions: ClientOptionsUpdated = {
+				hostname: basicPeerInfo.ipAddress,
+				port: basicPeerInfo.wsPort,
+				query: querystring.stringify(nodeInfoForDiscovery),


Does querystring.stringify(...) support nested objects (not just primitive types)? They should be allowed.

Maybe you can add a test case in the integration tests; maybe try adding a custom nested object to the nodeInfo object passed to the constructor of P2P instances for one of the test cases?

I haven't tested it yet but I will cover this in #1175 as @shuse2 already pointed this out there

This issue will now be covered in this PR. Please ignore the previous comment.

jondubois · 2019-03-11T13:34:26Z

packages/lisk-p2p/src/peer.ts

+	}
+};
+
+export const connectAndFetchStatus = async (


Can this be renamed to connectAndFetchPeerInfo?

jondubois · 2019-03-11T13:37:38Z

packages/lisk-p2p/src/p2p.ts

+		};
+		const seedPeerUpdatedInfos = await Promise.all(
+			seedPeers.map(async seedPeer =>
+				connectAndFetchStatus(seedPeer, this._nodeInfo, peerConfig),


If connectAndFetchStatus fails for some of the peers, we should log it but it shouldn't stop us from continuing with the discovery process (so long as if succeeded for some of the peers).

Currently, the Promise.all call means that it's either all pass or all fail. If one of the seed peers is offline, our P2P node will fail to start completely.

I'm returning the error instead of throwing and we can log or emit it if we want to

jondubois · 2019-03-11T13:42:31Z

packages/lisk-p2p/src/peer.ts

+	readonly socket?: SCClientSocket;
+}
+
+export const connectAndRequest = async (


About connectAndRequest: I think it would be safer if the socket was destroyed at the end of this function after it has fetched whatever data was required; that way the entire lifecycle of the socket will be encapsulated within this function and it avoids the possibility that someone will forget to destroy the socket after calling this function.

I have made the changes as discussed earlier

jondubois · 2019-03-11T13:45:25Z

packages/lisk-p2p/src/peer.ts

+		);
+
+		if (socket) {
+			socket.destroy();


Maybe this can be moved inside the connectAndRequest function (see previous comment).

jondubois · 2019-03-11T13:45:33Z

packages/lisk-p2p/src/peer.ts

+			peerConfig,
+		);
+		if (socket) {
+			socket.destroy();


Maybe this can be moved inside the connectAndRequest function (see previous comment).

jondubois · 2019-03-11T14:23:22Z

packages/lisk-p2p/src/peer_discovery.ts

@@ -18,6 +18,7 @@ import { constructPeerIdFromPeerInfo, Peer } from './peer';
 export interface FilterPeerOptions {
 	readonly blacklist: ReadonlyArray<string>;
 }
+
 // TODO later: Implement LIPS to handle fixed and white list
 export const discoverPeers = async (


We need to completely stop using peer.fetchPeers() for discovery or else we have two different discovery protocols. We should always use the new single-socket discovery approach.

It might seem more efficient to reuse the existing socket on the peer if it exists, but it's going to be a very difficult to manage these two different discovery approaches.

Changed as per discussion 👍

jondubois · 2019-03-11T14:57:34Z

packages/lisk-p2p/src/peer.ts

+				? convertNodeInfoToLegacyFormat(nodeInfo)
+				: undefined;
+			// Add a new field discovery to tell the receiving side that the connection will be short lived
+			const nodeInfoForDiscovery = { discovery: true, ...legacyNodeInfo };


This discovery: true is good, but I don't see it being used on the receiving end of the connection when the connection event triggers on the server.

This field is no longer needed with the new approach

… peerpool

…erInfo calls

shuse2 · 2019-03-12T13:31:15Z

packages/lisk-p2p/src/p2p.ts

+			if (!this._triedPeers.has(peerId)) {
+				this._triedPeers.set(peerId, seedInfo);
+			}
+		});
 		// TODO: Once we will a new peer discovery then we can remove this typecasting to P2PDiscoveredPeerInfo


maybe remove this comment?

shuse2 · 2019-03-12T13:36:57Z

packages/lisk-p2p/src/peer_pool.ts

+		// Check for received discovery info and then find it in peer pool and then update it
+		discoveredPeers.forEach((peerInfo: P2PDiscoveredPeerInfo) => {
+			const peerId = constructPeerIdFromPeerInfo(peerInfo);
+			if (this.hasPeer(peerId)) {


instead of calling hasPeer and casting getPeer result, maybe just call getPeer and check undefined?

shuse2 · 2019-03-12T13:37:04Z

packages/lisk-p2p/src/peer_pool.ts

@@ -293,6 +337,11 @@ export class PeerPool extends EventEmitter {
 			if (!this.hasPeer(peerId)) {
 				this.addPeer(peerInfo);
 			}
+			const existingPeer = this.getPeer(peerId) as Peer;


same as above

shuse2 · 2019-03-12T13:37:21Z

packages/lisk-p2p/src/peer_pool.ts

+		const peersForDiscovery = knownPeers.map(peerInfo => {
+			const peerId = constructPeerIdFromPeerInfo(peerInfo);
+			if (this.hasPeer(peerId)) {
+				const existingPeer = this.getPeer(peerId) as Peer;


same as below

jondubois · 2019-03-12T15:05:02Z

packages/lisk-p2p/src/peer.ts

@@ -316,6 +316,13 @@ export class Peer extends EventEmitter {
 		this._outboundSocket.connect();
 	}

+	public createPeerFromOutboundConnection(


We should pass the outbound socket to the peer's constructor as an optional argument instead of having a separate createPeerFromOutboundConnection method. This is dangerous here because it could be called multiple times. If we move this logic to the constructor then we guarantee that this cannot happen.

The logic which instantiates the Peer can just set the inboundSocket as undefined explicitly if there is no inbound socket.

jondubois · 2019-03-12T15:27:39Z

packages/lisk-p2p/src/peer_pool.ts

+	): Promise<ReadonlyArray<P2PDiscoveredPeerInfo>> {
+		const seedPeerUpdatedInfosAndSocket = await Promise.all(
+			seedPeers.map(async seedPeer =>
+				connectAndFetchPeerInfo(seedPeer, nodeInfo, peerConfig),


Because connectAndFetchPeerInfo is a pass or fail operation, it should throw; that way the function signature doesn't need to return dual types and we don't need to use instanceof statement to separate the two outputs.

The fetchStatusAndCreatePeer function can succeed even if it has errors, so returning the error should happen at this level. Maybe this function could return an object with an errors and a peerInfoList field. If no errors occurred, then the errors field would be an empty array. Also, it doesn't seem that we are using the errors from connectAndFetchPeerInfo anywhere at the moment (and this function is not returning them).

We should emit them on the top level P2P instance using the existing EVENT_FAILED_TO_FETCH_PEER_INFO event.

jondubois · 2019-03-12T15:31:42Z

packages/lisk-p2p/src/peer_pool.ts

+			ackTimeout: this._peerPoolConfig.ackTimeout,
+		};
+		const peer = new Peer(peerInfo, peerConfig);
+		peer.createPeerFromOutboundConnection(socket);


As mentioned in a previous comment. It would be safer to pass the outbound socket to the Peer constructor.

shuse2

Just small question

shuse2 · 2019-03-14T10:29:14Z

packages/lisk-p2p/src/peer_pool.ts

+		};
+		const peer = new Peer(peerInfo, peerConfig, { outbound: socket });
+
+		// Throw an error because adding a peer multiple times is a common developer error which is very difficult to itentify and debug.


where does it throw an error?

Oh, it was a mistake, we don;t need to throw an error here as we are already checking above if it's present in peer pool or not. I corrected it

jondubois · 2019-03-14T14:45:57Z

packages/lisk-p2p/src/peer.ts

@@ -713,6 +713,6 @@ export const connectAndFetchPeerInfo = async (

 		return { peerInfo, socket };
 	} catch (error) {
-		return error;
+		throw error;


Since the error is just being re-thrown as-is and we're not doing anything else with it here, we don't need the try-catch block in this function.

I think you commented on the outdated code, but now I'm throwing an error but capturing peer ip and port for better logging

It appears that you already fixed this in a later commit so ignore.

jondubois · 2019-03-14T14:53:17Z

packages/lisk-p2p/src/peer_pool.ts

@@ -253,33 +256,79 @@ export class PeerPool extends EventEmitter {
 		});
 	}

+	public async fetchStatusAndCreatePeer(


This method name should be plural because it's fetching and adding multiple peers.

…nd_new_discovery

ishantiw added 4 commits March 5, 2019 16:55

🌱 Add connectAndRequest to fetch peers and status independently

1110adb

♻️ Update exisiting discovery to use connectAndFetchPeers and accept …

8cfca6e

…P2PPeerInfo to do the probe

♻️ Add usage of new discovery process across p2p lib

64bacdc

ishantiw requested review from jondubois and shuse2 March 7, 2019 16:08

ishantiw mentioned this pull request Mar 7, 2019

Add connectAndFetchPeers and new Discovery function #1154

Closed

ishantiw self-assigned this Mar 7, 2019

ishantiw added the 🏗️ in progress label Mar 7, 2019

ishantiw added 3 commits March 11, 2019 10:59

✅ Fix discovery unit tests

c352c84

🔙 Revert changes to connectAndFetchPeers and update failing tests

18df75d

🔙 Use selectRandomPeerSample to discover and revert test wait times

591dd9d

ishantiw added 👁️ pending review and removed 🏗️ in progress labels Mar 11, 2019

ishantiw marked this pull request as ready for review March 11, 2019 12:39

shuse2 suggested changes Mar 11, 2019

View reviewed changes

jondubois suggested changes Mar 11, 2019

View reviewed changes

jondubois reviewed Mar 11, 2019

View reviewed changes

♻️ Reuse outbound socket from fetchStatus call and addOutboundPeer in…

8de0dd1

… peerpool

ishantiw requested review from jondubois and shuse2 March 11, 2019 23:02

ishantiw force-pushed the 1152_add_func_coonect_fetch_peers_and_new_discovery branch from ba6198c to ed34e03 Compare March 12, 2019 10:22

♻️ Handle error to log instead of failing bundle of connectAndFetchPe…

7574391

…erInfo calls

ishantiw force-pushed the 1152_add_func_coonect_fetch_peers_and_new_discovery branch from ed34e03 to 7574391 Compare March 12, 2019 10:23

shuse2 suggested changes Mar 12, 2019

View reviewed changes

ishantiw mentioned this pull request Mar 12, 2019

Integration test custom peer select functions - Closes1131 #1175

Merged

ishantiw added the 🏗️ in progress label Mar 12, 2019

ishantiw removed the 👁️ pending review label Mar 12, 2019

ishantiw added 2 commits March 12, 2019 16:12

🌱 Handle custom fields passed around by a node or received from peer

c6a66d8

✅ Add integration test for checking communication of custom fields

ecab628

ishantiw added 👁️ pending review and removed 🏗️ in progress labels Mar 12, 2019

jondubois suggested changes Mar 12, 2019

View reviewed changes

ishantiw added 5 commits March 12, 2019 16:42

💅 Improve check for existing peer and remove stale comment

305ce1f

♻️ Add new type for Peer constructor to accept sockets

f55ae97

♻️ Improve fetchStatusAndCreatePeer and handle error

977da77

🌱 Add a new custom error class for fetchPeerStatus

9fed690

♻️ Emit error on peer pool for failed fetch peer status

c64893d

ishantiw requested review from jondubois and shuse2 March 14, 2019 09:18

shuse2 reviewed Mar 14, 2019

View reviewed changes

💅 Remove unnecessary comment and improve grammar

409dbf7

ishantiw mentioned this pull request Mar 14, 2019

PeerPool should not respond to get peers list - Closes#1186 #1187

Merged

jondubois suggested changes Mar 14, 2019

View reviewed changes

💅 Improve function name

98cf227

jondubois approved these changes Mar 14, 2019

View reviewed changes

shuse2 approved these changes Mar 14, 2019

View reviewed changes

shuse2 added ✅ ready and removed 👁️ pending review labels Mar 14, 2019

Merge branch 'release/2.1.0' into 1152_add_func_coonect_fetch_peers_a…

2708aaa

…nd_new_discovery

shuse2 merged commit 07d8749 into release/2.1.0 Mar 14, 2019

shuse2 deleted the 1152_add_func_coonect_fetch_peers_and_new_discovery branch March 14, 2019 20:36

Add connectAndFetchPeers and new Discovery function Closes#1152 #1170

Add connectAndFetchPeers and new Discovery function Closes#1152 #1170

Conversation

ishantiw commented Mar 7, 2019

Description

Review checklist

ishantiw commented Mar 7, 2019 • edited Loading

ishantiw commented Mar 11, 2019 • edited Loading

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

jondubois Mar 11, 2019 • edited Loading

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

shuse2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw Mar 14, 2019 • edited Loading

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

ishantiw commented Mar 7, 2019 •

edited

Loading

ishantiw commented Mar 11, 2019 •

edited

Loading

jondubois Mar 11, 2019 •

edited

Loading

ishantiw Mar 14, 2019 •

edited

Loading