Remove redundant P2P interfaces - Closes #4493 #4525

diego-G · 2019-11-18T14:26:40Z

How did I solve it?

Remove P2PProtocolPeerInfo interface
Replace P2PNodeInfo with adjusted P2PSharedState
Rename node Info concept to shared state
Introduce P2PBasicPeerInfo interface as P2PPeerInfo's parent
Rename objects inside the P2PPeerLists interface
Replace P2P blacklistedPeers by blacklistedIPs an array of IPs
Remove unused own sharedState from P2P peer selection functions
Remove redundant advertiseAddress field from P2PInternalState interface
Sanitize I/O data inside the P2P library in order to make the usage easier

How to manually test it?

npm t

Review checklist

The PR resolves Remove interface ProtocolPeerInfo from P2P #4493
All new code is covered with unit tests
Relevant integration / functional tests are added
Commit messages follow the commit guidelines
Documentation has been added/updated

As result, there is no need to sanitize any IncomingPeerInfo.

- P2PPeerInfo.sharedState becomes non-optional - wsPort field appears as non-optional field of P2PSharedState

elements/lisk-p2p/src/p2p.ts

elements/lisk-p2p/src/peer/base.ts

It is useful to receive sharedState on every Peer class from the config and keep it updated in a separate variable.

ishantiw · 2019-11-21T11:06:07Z

elements/lisk-p2p/src/p2p_types.ts

 }

-export interface P2PPeerInfo {
+export interface P2PBasicPeerInfo {


We are falling back to old p2p types that we removed i.e., having two 2 peer types for holding peer information

I've added P2PBasicPeerInfo because it is handy inside the PeerBook to get and remove peers by peer id and ip. We don't have peer info splitted into different places. P2PPeerInfo is the only one with a proper structure.

ishantiw · 2019-11-21T11:09:07Z

elements/lisk-p2p/src/p2p_types.ts

-	readonly wsPort: number;
-	// tslint:disable-next-line: no-mixed-interface
-	readonly [key: string]: unknown;
+export interface P2PPeerInfo extends P2PBasicPeerInfo {


This is like P2PDisocveredPeerInfo

Can you elaborate a bit more? P2PDiscoveredPeerInfo doesn't exist anymore.

I meant that we had this old types -> new types
P2PPeerInfo -> P2PBasicPeerInfo
P2PDiscoveredInfo -> P2PPeerInfo

They weren't exactly the same. P2PPeerInfo contains now basic, shared and internal data.

ishantiw · 2019-11-21T11:12:08Z

elements/lisk-p2p/src/p2p_types.ts

-	readonly fixedPeers?: ReadonlyArray<ProtocolPeerInfo>;
-	readonly whitelistedPeers?: ReadonlyArray<ProtocolPeerInfo>;
-	readonly previousPeers?: ReadonlyArray<ProtocolPeerInfo>;
+	readonly sharedState: P2PSharedState;


IMO, nodeInfo is still sounds more clear than sharedState. Having a distinction between nodeInfo and peerInfo is important because in future if we will have more fields like advertiseAddress then it will be hard to incorporate, maybe now they seem almost the same but still they are different

What is nodeInfo for us, we share it to the rest of peers becoming sharedState when they receive it. I don't think there is a strong reasoning to keep this distinction. If more fields appear, sharedState will be the place to hold this information locally, the structure to share it and also to receive it. It just looks simple.

agree with @ishantiw , This is not really shared as mentioned above.
prefer nodeInfo

elements/lisk-p2p/src/p2p_types.ts

shuse2 · 2019-11-22T14:37:20Z

elements/lisk-p2p/src/events.ts

 export const EVENT_FAILED_TO_SEND_MESSAGE = 'failedToSendMessage';

 // Peer base
 export const REMOTE_SC_EVENT_RPC_REQUEST = 'rpc-request';
 export const REMOTE_SC_EVENT_MESSAGE = 'remote-message';
-export const REMOTE_EVENT_POST_NODE_INFO = 'postNodeInfo';


these name cannot be changed. It's defined in the LIP

shuse2 · 2019-11-22T14:37:52Z

elements/lisk-p2p/src/p2p.ts

@@ -200,14 +201,14 @@ export class P2P extends EventEmitter {
 	private readonly _bannedPeers: Set<string>;
 	private readonly _populatorInterval: number;
 	private _populatorIntervalId: NodeJS.Timer | undefined;
-	private _nodeInfo: P2PNodeInfo;
+	private _sharedState: P2PSharedState;


I think _nodeInfo make more sense as variable name

shuse2 · 2019-11-23T19:58:27Z

elements/lisk-p2p/src/p2p.ts

-				? this._config.maxPeerInfoSize
-				: DEFAULT_MAX_PEER_INFO_SIZE,
-		);
+	public applySharedState(sharedState: P2PSharedState): void {


this also should be NodeInfo. It is SharingState not SharedState

shuse2 · 2019-11-23T19:58:49Z

elements/lisk-p2p/src/p2p.ts

 	}

-	public get nodeInfo(): P2PNodeInfo {
-		return this._nodeInfo;
+	public get sharedState(): P2PSharedState {


same here. The meaning is opposite

shuse2 · 2019-11-23T19:59:18Z

elements/lisk-p2p/src/p2p.ts

@@ -546,38 +521,35 @@ export class P2P extends EventEmitter {
 		}
 	}

-	public getTriedPeers(): ReadonlyArray<ProtocolPeerInfo> {
+	public getTriedPeers(): ReadonlyArray<P2PPeerInfo> {


we still have P2PPeerInfo?

shuse2 · 2019-11-23T20:01:48Z

elements/lisk-p2p/src/p2p_types.ts

-	readonly fixedPeers?: ReadonlyArray<ProtocolPeerInfo>;
-	readonly whitelistedPeers?: ReadonlyArray<ProtocolPeerInfo>;
-	readonly previousPeers?: ReadonlyArray<ProtocolPeerInfo>;
+	readonly sharedState: P2PSharedState;


agree with @ishantiw , This is not really shared as mentioned above.
prefer nodeInfo

shuse2 · 2019-11-23T20:02:12Z

elements/lisk-p2p/src/peer/base.ts

@@ -105,13 +106,10 @@ export interface PeerConfig {
 	readonly maxPeerInfoSize: number;
 	readonly maxPeerDiscoveryResponseLength: number;
 	readonly secret: number;
-	readonly serverNodeInfo?: P2PNodeInfo;
+	readonly sharedState: P2PSharedState;


shuse2 · 2019-11-23T20:03:28Z

elements/lisk-p2p/src/peer/base.ts

-		this._id = peerInfo.peerId;
+		this._info = peerInfo;
+		this._config = peerConfig;
+		this._sharedState = peerConfig.sharedState;


having _id _wsPort and _ipAddress as parameter make sense as base peer i think

shuse2 · 2019-11-23T20:04:14Z

elements/lisk-p2p/src/peer_pool.ts

@@ -135,6 +137,7 @@ export interface PeerPoolConfig {
 	readonly rateCalculationInterval: number;
 	readonly secret: number;
 	readonly peerLists: PeerLists;
+	readonly sharedState: P2PSharedState;


same as above

shuse2 · 2019-11-23T20:04:44Z

elements/lisk-p2p/src/peer_pool.ts

@@ -629,29 +624,32 @@ export class PeerPool extends EventEmitter {
 		return openOutboundSlots;
 	}

-	private _applyNodeInfoOnPeer(peer: Peer, nodeInfo: P2PNodeInfo): void {
+	private _sendSharedStateToPeer(peer: Peer): void {


same as above

diego-G added 16 commits November 14, 2019 16:55

Remove P2P ProtocolPeerInfo interface

62705d0

As result, there is no need to sanitize any IncomingPeerInfo.

Replace P2PNodeInfo with adjusted P2PSharedState

40ae48e

- P2PPeerInfo.sharedState becomes non-optional - wsPort field appears as non-optional field of P2PSharedState

Merge branch 'development' into 4493-remove_redundant_p2p_types

8fdc301

Rename P2P serverNodeInfo variable to nodeInfo

cc8a840

Remove boilerplate P2P Peer base public updatePeerInfo()

52fcc80

Rename P2P Peer _updateFromProtocolPeerInfo() to _updatePeerNodeInfo

d38a8bd

Rename node info concept to shared state

215e6c3

Remove unnecessary nodeInfo from P2P Peer Base class

230907a

Merge branch 'development' into 4493-remove_redundant_p2p_types

c02bd8d

Remove unneeded serverNodeInfo when creating inbound peer

3a3c0a8

Capture and throw error when P2P sharedState is not valid

e804366

Merge branch 'development' into 4493-remove_redundant_p2p_types

2e32836

Rename objects inside the P2P PeerLists interface

f3b223f

Replace P2P blacklistedPeers by blacklistedIPs an array of IPs

4d463be

Flat shared state in the consolidated peer list

09d4caa

Merge branch 'development' into 4493-remove_redundant_p2p_types

413d006

diego-G added the status: pending review label Nov 18, 2019

diego-G requested review from ishantiw, mitsuaki-u and valamidev November 18, 2019 14:26

diego-G self-assigned this Nov 18, 2019

diego-G changed the title ~~Remove redundant p2p types - Closes #4493~~ Remove redundant P2P interfaces - Closes #4493 Nov 18, 2019

diego-G requested review from ManuGowda and shuse2 November 18, 2019 14:27

valamidev reviewed Nov 18, 2019

View reviewed changes

elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved

diego-G added 3 commits November 18, 2019 15:45

Fix typo

d19fbaf

Rename P2P Peer base's _peerInfo variable to just _info

4b84615

Rename P2P Peer base's _peerConfig variable to just _config

4c55aca

diego-G force-pushed the 4493-remove_redundant_p2p_types branch from d600920 to 4c55aca Compare November 18, 2019 15:06

LiskHQ deleted a comment from diego-G Nov 18, 2019

diego-G added 7 commits November 20, 2019 12:57

Merge branch 'development' into 4493-remove_redundant_p2p_types

fad6333

Introduce P2PBasicPeerInfo interface as P2PPeerInfo's parent

6ce054d

Merge branch 'development' into 4493-remove_redundant_p2p_types

070bcf7

Remove advertiseAddress when consolidating peers in network module

b3abe84

Simplify api get peers output

c75c559

Simplify P2P get peers output

fa500a4

Fix P2P tests

502a297

valamidev reviewed Nov 21, 2019

View reviewed changes

elements/lisk-p2p/src/peer/base.ts Outdated Show resolved Hide resolved

valamidev approved these changes Nov 21, 2019

View reviewed changes

valamidev self-requested a review November 21, 2019 08:33

Merge branch 'development' into 4493-remove_redundant_p2p_types

053a0d0

diego-G force-pushed the 4493-remove_redundant_p2p_types branch from 7030c74 to c557b47 Compare November 21, 2019 10:43

valamidev approved these changes Nov 21, 2019

View reviewed changes

Revert 230907a

0de5dfe

It is useful to receive sharedState on every Peer class from the config and keep it updated in a separate variable.

diego-G force-pushed the 4493-remove_redundant_p2p_types branch from c557b47 to 0de5dfe Compare November 21, 2019 10:58

ishantiw requested changes Nov 21, 2019

View reviewed changes

diego-G added 7 commits November 21, 2019 13:35

Merge branch 'development' into 4493-remove_redundant_p2p_types

39d044c

Rename newSharedState to peerSharedState in P2P validation utility

082ab45

Add P2P peer from config sanitation to simplify user usage

2ff342c

Simplify output when getting list of connected peers from lisk-p2p

93d19c2

Merge branch 'development' into 4493-remove_redundant_p2p_types

f745771

Fix P2P getDisconnectedPeers()

996108c

Fix framework mocha test common to generate peer headers

c2b09e7

diego-G requested a review from ishantiw November 22, 2019 14:59

shuse2 requested changes Nov 25, 2019

View reviewed changes

diego-G closed this Nov 25, 2019

karmacoma removed the status: pending review label Nov 27, 2019

karmacoma unassigned diego-G Dec 17, 2019

karmacoma added the status: invalid label Dec 17, 2019

shuse2 deleted the 4493-remove_redundant_p2p_types branch January 17, 2020 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant P2P interfaces - Closes #4493 #4525

Remove redundant P2P interfaces - Closes #4493 #4525

diego-G commented Nov 18, 2019 •

edited

ishantiw Nov 21, 2019

diego-G Nov 21, 2019

ishantiw Nov 21, 2019

diego-G Nov 21, 2019

ishantiw Nov 21, 2019

diego-G Nov 21, 2019

ishantiw Nov 21, 2019

diego-G Nov 21, 2019

shuse2 Nov 23, 2019

shuse2 Nov 22, 2019

shuse2 Nov 22, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

shuse2 Nov 23, 2019

Remove redundant P2P interfaces - Closes #4493 #4525

Remove redundant P2P interfaces - Closes #4493 #4525

Conversation

diego-G commented Nov 18, 2019 • edited

How did I solve it?

How to manually 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

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

diego-G commented Nov 18, 2019 •

edited