Implement P2P.getNetworkStatus() functionality - Closes #985 and #897, addresses #896 and #953 #986

jondubois · 2019-01-08T18:02:29Z

What was the problem?

In order to implement the integration tests, there needs to be a way to get information about the network's state from the P2P library. This feature depends on the P2P.getNetworkStatus() method of the P2P library which itself depends on the peer request/send functionality (to allow peers to send us information about themselves).

How did I fix it?

Implemented outbound request/send functionality (at the Peer level only)
Implemented inbound request/message handling (at the Peer level only)
Implemented sending of P2PNodeInfo to remote peers.
Implemented receiving PeerInfo from remote peers.
Implemented getNetworkStatus() method on P2P class.

How to test it?

Will add integration tests for all these features as part of #953 then, once passing, the logic can be locked down with unit tests.

Review checklist

The PR resolves Implement P2P.getNetworkStatus() functionality #985 and Add send/request data mechanism #897
The PR addresses Add receive data and handle response mechanism #896 and Add integration tests for discovery for lisk-p2p #953
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

packages/lisk-p2p/src/peer.ts

…fo from peer and cast to PeerInfo

…g P2PNodeInfo

ishantiw · 2019-01-11T10:17:17Z

packages/lisk-p2p/src/p2p_types.ts

@@ -44,28 +44,45 @@ export interface P2PConfig {
 	readonly connectTimeout: number;
 	readonly ipAddress?: string;


Why ipAddress is optional here?

This is the ipAddress of the node itself.
It's this property from the current config.json: https://github.com/LiskHQ/lisk/blob/development/config/default/config.json#L4 - Default is 0.0.0.0. We can rename this if it's confusing.

maybe we can rename it to nodeIp to have a clear differentiation from the peer IP address. What do you think?

ishantiw · 2019-01-11T10:36:40Z

packages/lisk-p2p/src/p2p_types.ts

 }

-export interface NetworkStatus {}
+// Network info exposed by the P2P library.
+export interface P2PNetworkStatus {


since we have a concept of newPeers and triedPeers, maybe for clarity we should rename newPeers as availablePeers or unConnectedPeers at the network level.

Also, triedPeers can contain peers that are connected or disconnected. So it should also be checked as well for peers that are not connected

export interface P2PNetworkStatus { connectedPeers: ReadonlyArray<PeerInfo>; // peers from peer pool unConnectedPeers: ReadonlyArray<PeerInfo>; // or availablePeers, peers from newPeers + triedPeers (check with peerPool) }

unConnectedPeers is slightly different because some peers in the _newPeers map could be connected inbound only; maybe it's OK but slightly more ambiguous.

I think newPeers is OK. Once we start moving peers into the _triedPeers table, it will make more sense. So there will be newPeers, triedPeers and ~~connectedPeers~~ activePeers. I think we should use the same name to refer to the same thing.

Actually, according to the definition in LIPS, the moment you make a connection with any peer from newPeer we remove it from there and add it to triedPeer. So, newPeer should never have any peer for which we made connection already. Whereas triedPeers will have peers with whom we have a connection or had a connection in the past.

so, active peers should have, triedPeers peers which have a connection already and available peers will be the combination of (newPeers + triedPeers(with no socket))

We could have availablePeers be newPeers + triedPeers and connectedPeers be peers from the PeerPool?

availablePeers be newPeers + triedPeers and connectedPeers be peers from the PeerPool

I think this is good, im not sure if we need to expose newPeers or triedPeers

Isn't availablePeers and connectedPeers is enough? I thought we don't want to expose newPeers and triedPeers

ishantiw · 2019-01-11T10:39:20Z

packages/lisk-p2p/src/peer.ts

-	readonly os: string;
-	readonly version: string;
+	readonly os?: string;
+	readonly version?: string;


I thought we were making version compulsory. If we don't passversion value then at sanitization it will always throw an error

That's a tricky one.

The reason why version is optional is because the list of seedPeers which is fetched from the config file has this format:

readonly seedPeers: ReadonlyArray<PeerInfo>;

We cannot know what the version number of the seed peer is before we connect to it.
I think it's OK to expect the version in the sanitization logic when checking inbound PeerInfo from a remote peer, but it shouldn't be compulsory (based on our current types) at the type level because the PeerInfo object may be created directly in our own code or from a config file before we know anything about the peer (also would be tricky to know their OS before we connect to them).

Not sure if it's better to add yet another Peer... type? It's convenient that PeerInfo can be used for both getting information about peers and also for creating peers.

one idea to have BasePeerInfo which is extended to SeedPeerIndo and PeerInfo where the difference is version?

@shuse2 Sounds good but I think that SeedPeerInfo might end up having the same properties as BasePeerInfo so maybe just BasePeerInfo and PeerInfo?

Or it could be PeerInfo (for constructing) and DetailedPeerInfo (when getting information out of the node with getNetworkStatus())?

BasePeerInfo and PeerInfo

sounds good =)

ishantiw · 2019-01-11T10:46:44Z

packages/lisk-p2p/src/peer.ts

-		return this._id;
+	private _handlePeerInfo(request: ProtocolInboundRPCRequest): void {
+		// Update peerInfo with the latest values from the remote peer.
+		Object.assign(this._peerInfo, request.data as PeerInfo);


I think we are missing sanitization of the peer object from the response. we can wrap request.data with instantiatePeerFromResponse()

validation needs to be added later here too

packages/lisk-p2p/src/peer.ts

ishantiw · 2019-01-11T10:54:35Z

packages/lisk-p2p/src/peer.ts

+		return this._ipAddress;
+	}
+
+	public set outboundSocket(value: any) {


I think this was not rebased properly, instead of any it should be SCServerSocket

ishantiw · 2019-01-11T10:54:59Z

packages/lisk-p2p/src/peer.ts

+		return this._id;
+	}
+
+	public set inboundSocket(value: any) {


it should be SCServerSocket instead of any

ishantiw · 2019-01-11T10:56:01Z

packages/lisk-p2p/src/peer.ts

+	public get nodeInfo(): P2PNodeInfo | undefined {
+		return this._nodeInfo;
+	}
+
 	public connect(): void {
 		if (!this.outboundSocket) {


It should be this._outboundSocket

ishantiw · 2019-01-11T10:56:12Z

packages/lisk-p2p/src/peer.ts

 	public connect(): void {
 		if (!this.outboundSocket) {
 			this.outboundSocket = this._createOutboundSocket();
 		}
-
 		this.outboundSocket.connect();


this._outboundSocket

ishantiw · 2019-01-11T10:58:29Z

packages/lisk-p2p/src/peer.ts

-			this.inboundSocket.disconnect(code, reason);
+		if (this._inboundSocket) {
+			this._inboundSocket.disconnect(code, reason);
+			this._stopListeningForRPCs(this._inboundSocket);
 		}
 		if (this.outboundSocket) {


this._outboundSocket

shuse2

minor thing and some question/comment

packages/lisk-p2p/src/p2p_types.ts

shuse2 · 2019-01-12T15:53:28Z

packages/lisk-p2p/src/peer.ts

-	readonly os: string;
-	readonly version: string;
+	readonly os?: string;
+	readonly version?: string;


one idea to have BasePeerInfo which is extended to SeedPeerIndo and PeerInfo where the difference is version?

packages/lisk-p2p/src/peer.ts

shuse2 · 2019-01-12T15:56:28Z

packages/lisk-p2p/src/peer.ts

-		return this._id;
+	private _handlePeerInfo(request: ProtocolInboundRPCRequest): void {
+		// Update peerInfo with the latest values from the remote peer.
+		Object.assign(this._peerInfo, request.data as PeerInfo);


validation needs to be added later here too

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

ishantiw

Some comments related to getNetworkStatus()

ishantiw · 2019-01-14T09:39:34Z

packages/lisk-p2p/src/p2p_types.ts

 }

-export interface NetworkStatus {}
+// Network info exposed by the P2P library.
+export interface P2PNetworkStatus {


Isn't availablePeers and connectedPeers is enough? I thought we don't want to expose newPeers and triedPeers

ishantiw · 2019-01-14T09:46:45Z

packages/lisk-p2p/src/p2p.ts

+	public getNetworkStatus(): P2PNetworkStatus {
+		const newPeers = [...this._newPeers.values()];
+		const triedPeers = [...this._triedPeers.values()];
+		const availablePeers = newPeers.concat(triedPeers);


is availablePeers are all the peers that are available but don't have any connection yet? or, does it include all the peers available or known including the connected peers? If its the former case then I think we need to filter newPeers and triedPeers to remove peers that we are already connected to and then concat.

Since the getNetworkStatus call will be used by integration tests (not just API module), it's important that it shows the internal state of the node as closely as possible.

I think it's more important that this interface be detailed rather than user-friendly. Users of the method can do their own sanitisation of the raw data.

Integration test cases will want to check the state of the _newPeers and _triedPeers maps explicitly (since it's a core concept in the LIPs), so we need the name of the exposed properties to reflect the name of internal properties for which we want to allow introspection. We could call it newPeersList instead of newPeers maybe?

The less sanitisation/formatting we do on our end, the more performant this call will be.

@shuse2 What is your opinion about this?

I think without availablePeers is fine.
availablePeers seems ambiguous in this case because it will include all the peers regardless of connection since triedPeer will include the peer with outbound connection.
Also, not really useful if it's just combination of newPeers and triedPeers, user can just combine by themselves?

In case we want to have a concatenated list of newPeers and triedPeers and if we are exposing them then we don't need availablePeers list as its redundant or the user can make it out by themselves. I was thinking that network status is just about knowing (connected and unconnected Peers) but since newPeers and triedPeers will be used in the network test then its a better to add it.

I guess in future we will need a separate class for handling peer bucket or peer directory (like I discussed once) that includes newPeer/triedPeer/bannedPeer and then it will be easier to test these lists.

jondubois requested review from ishantiw and shuse2 January 8, 2019 18:02

shuse2 assigned jondubois Jan 9, 2019

shuse2 added the 👁️ pending review label Jan 9, 2019

shuse2 added this to Open PRs in Version 2.1.0 via automation Jan 9, 2019

shuse2 suggested changes Jan 9, 2019

View reviewed changes

shuse2 added 🏗️ in progress and removed 👁️ pending review labels Jan 9, 2019

jondubois added 10 commits January 10, 2019 12:08

Add script for integration tests

5f13947

Changes to type definitions

943ea7c

Lint cleanup

d0f81c1

Cleanup Peer class and add support for sending an receiving P2PNodeIn…

9bd408b

…fo from peer and cast to PeerInfo

Convert PeerConfig to PeerInfo and add support for getting and sendin…

f5ad9fe

…g P2PNodeInfo

Update tests to account for changing PeerConfig to PeerInfo

b34917f

Add comment to justify declaring a function in a constructor

6896274

Handle inbound requests and messages at Peer level

51eb545

getNetworkStatus should return newPeers and activePeers

6c15d6d

Fix ordering

83f9b71

jondubois force-pushed the p2p_jon branch from 6c2dc77 to 83f9b71 Compare January 10, 2019 14:33

Use constant instead of string

28301c6

jondubois added 👁️ pending review and removed 🏗️ in progress labels Jan 10, 2019

jondubois added 2 commits January 10, 2019 17:25

Fix issue with Peer parameters

deb041e

Improve constant name and fix type error

a14ba28

ishantiw suggested changes Jan 11, 2019

View reviewed changes

jondubois added 2 commits January 11, 2019 16:01

Fix rebase issues; use socket types instead of any

ec85ebf

Add more details to getNetworkStatus and rename

a838910

shuse2 suggested changes Jan 12, 2019

View reviewed changes

shuse2 added 🏗️ in progress and removed 👁️ pending review labels Jan 12, 2019

Minor code style improvements and add comment

4f970ee

jondubois added 👁️ pending review and removed 🏗️ in progress labels Jan 14, 2019

shuse2 approved these changes Jan 14, 2019

View reviewed changes

ishantiw reviewed Jan 14, 2019

View reviewed changes

No need for availablePeers

d91c7ad

ishantiw approved these changes Jan 14, 2019

View reviewed changes

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

shuse2 merged commit c9164dc into add_p2p_experiment Jan 14, 2019

Version 2.1.0 automation moved this from Open PRs to Closed PRs Jan 14, 2019

shuse2 deleted the p2p_jon branch January 14, 2019 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement P2P.getNetworkStatus() functionality - Closes #985 and #897, addresses #896 and #953 #986

Implement P2P.getNetworkStatus() functionality - Closes #985 and #897, addresses #896 and #953 #986

jondubois commented Jan 8, 2019

ishantiw Jan 11, 2019

jondubois Jan 11, 2019

ishantiw Jan 11, 2019

ishantiw Jan 11, 2019

jondubois Jan 11, 2019 •

edited

Loading

ishantiw Jan 11, 2019

jondubois Jan 11, 2019

shuse2 Jan 12, 2019

ishantiw Jan 14, 2019

ishantiw Jan 11, 2019

jondubois Jan 11, 2019 •

edited

Loading

shuse2 Jan 12, 2019

jondubois Jan 14, 2019

shuse2 Jan 14, 2019

ishantiw Jan 11, 2019

shuse2 Jan 12, 2019

ishantiw Jan 11, 2019

ishantiw Jan 11, 2019

ishantiw Jan 11, 2019

ishantiw Jan 11, 2019

ishantiw Jan 11, 2019

shuse2 left a comment

shuse2 Jan 12, 2019

shuse2 Jan 12, 2019

ishantiw left a comment

ishantiw Jan 14, 2019

ishantiw Jan 14, 2019

jondubois Jan 14, 2019 •

edited

Loading

jondubois Jan 14, 2019

shuse2 Jan 14, 2019

ishantiw Jan 14, 2019 •

edited

Loading

		@@ -44,28 +44,45 @@ export interface P2PConfig {
		readonly connectTimeout: number;
		readonly ipAddress?: string;

Implement P2P.getNetworkStatus() functionality - Closes #985 and #897, addresses #896 and #953 #986

Implement P2P.getNetworkStatus() functionality - Closes #985 and #897, addresses #896 and #953 #986

Conversation

jondubois commented Jan 8, 2019

What was the problem?

How did I fix it?

How to test it?

Review checklist

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 Jan 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

jondubois Jan 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

shuse2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishantiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jondubois Jan 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

ishantiw Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

jondubois Jan 11, 2019 •

edited

Loading

jondubois Jan 11, 2019 •

edited

Loading

jondubois Jan 14, 2019 •

edited

Loading

ishantiw Jan 14, 2019 •

edited

Loading