From c1f9efac12bd23f16be3094b121faa292ccc32a3 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Thu, 22 Aug 2019 15:27:58 +0200 Subject: [PATCH 1/9] Do not expose new or tried peers from P2P library --- elements/lisk-p2p/src/p2p.ts | 3 +- elements/lisk-p2p/src/p2p_types.ts | 3 +- elements/lisk-p2p/src/peer_pool.ts | 12 ++++++ elements/lisk-p2p/test/integration/p2p.ts | 41 ++++++++++--------- .../src/modules/http_api/controllers/peers.js | 2 +- framework/src/modules/network/filter_peers.js | 8 +--- 6 files changed, 38 insertions(+), 31 deletions(-) diff --git a/elements/lisk-p2p/src/p2p.ts b/elements/lisk-p2p/src/p2p.ts index febb89e5fdf..83dbed21386 100644 --- a/elements/lisk-p2p/src/p2p.ts +++ b/elements/lisk-p2p/src/p2p.ts @@ -533,8 +533,7 @@ export class P2P extends EventEmitter { public getNetworkStatus(): P2PNetworkStatus { return { - newPeers: [...this._peerBook.newPeers], - triedPeers: [...this._peerBook.triedPeers], + disconnectedPeers: this._peerPool.getDisconnectedPeerInfos(), connectedPeers: this._peerPool.getAllConnectedPeerInfos(), connectedUniquePeers: this._peerPool.getUniqueConnectedPeers(), }; diff --git a/elements/lisk-p2p/src/p2p_types.ts b/elements/lisk-p2p/src/p2p_types.ts index 3fcee64b2ab..124d1650127 100644 --- a/elements/lisk-p2p/src/p2p_types.ts +++ b/elements/lisk-p2p/src/p2p_types.ts @@ -115,8 +115,7 @@ export interface P2PConfig { // Network info exposed by the P2P library. export interface P2PNetworkStatus { - readonly newPeers: ReadonlyArray; - readonly triedPeers: ReadonlyArray; + readonly disconnectedPeers: ReadonlyArray; readonly connectedPeers: ReadonlyArray; readonly connectedUniquePeers: ReadonlyArray; } diff --git a/elements/lisk-p2p/src/peer_pool.ts b/elements/lisk-p2p/src/peer_pool.ts index 2f3baf79a52..2f38e3aa78f 100644 --- a/elements/lisk-p2p/src/peer_pool.ts +++ b/elements/lisk-p2p/src/peer_pool.ts @@ -537,6 +537,12 @@ export class PeerPool extends EventEmitter { ); } + public getDisconnectedPeerInfos(): ReadonlyArray { + return this.getDisconnectedPeers().map( + peer => peer.peerInfo as P2PDiscoveredPeerInfo, + ); + } + public getConnectedPeers( kind?: typeof OutboundPeer | typeof InboundPeer, ): ReadonlyArray { @@ -550,6 +556,12 @@ export class PeerPool extends EventEmitter { return peers.filter(peer => peer.state === ConnectionState.OPEN); } + public getDisconnectedPeers(): ReadonlyArray { + const peers = [...this._peerMap.values()]; + + return peers.filter(peer => peer.state !== ConnectionState.OPEN); + } + public getPeer(peerId: string): Peer | undefined { return this._peerMap.get(peerId); } diff --git a/elements/lisk-p2p/test/integration/p2p.ts b/elements/lisk-p2p/test/integration/p2p.ts index 35051b82d2f..2cbad2ea9ca 100644 --- a/elements/lisk-p2p/test/integration/p2p.ts +++ b/elements/lisk-p2p/test/integration/p2p.ts @@ -456,7 +456,7 @@ describe('Integration tests for P2P library', () => { it('should discover all peers and connect to all the peers so there should be no peer in newPeers list', () => { for (let p2p of p2pNodeList) { - const { newPeers } = p2p.getNetworkStatus(); + const newPeers = p2p['_peerBook'].newPeers; const peerPorts = newPeers.map(peerInfo => peerInfo.wsPort).sort(); @@ -466,7 +466,7 @@ describe('Integration tests for P2P library', () => { it('should discover all peers and add them to the triedPeers list within each node', () => { for (let p2p of p2pNodeList) { - const { triedPeers } = p2p.getNetworkStatus(); + const triedPeers = p2p['_peerBook'].triedPeers; const peerPorts = triedPeers.map(peerInfo => peerInfo.wsPort).sort(); @@ -481,11 +481,10 @@ describe('Integration tests for P2P library', () => { it('should not contain itself in any of its peer list', async () => { for (let p2p of p2pNodeList) { - const { - triedPeers, - connectedPeers, - newPeers, - } = p2p.getNetworkStatus(); + const { connectedPeers } = p2p.getNetworkStatus(); + + const newPeers = p2p['_peerBook'].newPeers; + const triedPeers = p2p['_peerBook'].triedPeers; const triedPeerPorts = triedPeers .map(peerInfo => peerInfo.wsPort) @@ -783,11 +782,14 @@ describe('Integration tests for P2P library', () => { peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, ); - const firstNodeInNewPeer = networkStatus.newPeers.find( + const newPeers = p2pNode['_peerBook'].newPeers; + const triedPeers = p2pNode['_peerBook'].triedPeers; + + const firstNodeInNewPeer = newPeers.find( peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, ); - const firstNodeInTriedPeer = networkStatus.triedPeers.find( + const firstNodeInTriedPeer = triedPeers.find( peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, ); @@ -1447,11 +1449,9 @@ describe('Integration tests for P2P library', () => { describe('all the nodes should be able to communicate and receive custom fields passed in nodeinfo', () => { it('should have tried peers with custom test field "modules" that was passed as nodeinfo', () => { for (let p2p of p2pNodeList) { - const { - connectedPeers, - newPeers, - triedPeers, - } = p2p.getNetworkStatus(); + const { connectedPeers } = p2p.getNetworkStatus(); + const triedPeers = p2p['_peerBook'].triedPeers; + const newPeers = p2p['_peerBook'].newPeers; for (let peer of triedPeers) { expect(peer) @@ -1569,7 +1569,8 @@ describe('Integration tests for P2P library', () => { it('should discover peers and add them to the peer lists within each node', () => { for (let p2p of p2pNodeList) { - const { newPeers, triedPeers } = p2p.getNetworkStatus(); + const triedPeers = p2p['_peerBook'].triedPeers; + const newPeers = p2p['_peerBook'].newPeers; const peerPorts = [...newPeers, ...triedPeers].map( peerInfo => peerInfo.wsPort, ); @@ -1849,7 +1850,7 @@ describe('Integration tests for P2P library', () => { it('should not add any blacklisted peer to newPeers', () => { for (let p2p of p2pNodeList) { - const { newPeers } = p2p.getNetworkStatus(); + const newPeers = p2p['_peerBook'].newPeers; const newPeersIPWS = newPeers.map(peer => { return { ipAddress: peer.ipAddress, wsPort: peer.wsPort }; }); @@ -1859,7 +1860,7 @@ describe('Integration tests for P2P library', () => { it('should not add any blacklisted peer to triedPeers', () => { for (let p2p of p2pNodeList) { - const { triedPeers } = p2p.getNetworkStatus(); + const triedPeers = p2p['_peerBook'].triedPeers; const triedPeersIPWS = triedPeers.map(peer => { return { ipAddress: peer.ipAddress, wsPort: peer.wsPort }; }); @@ -2001,7 +2002,7 @@ describe('Integration tests for P2P library', () => { it('should add every whitelisted peer to triedPeers', () => { p2pNodeList.forEach((p2p, index) => { if (![0, 9].includes(index)) { - const { triedPeers } = p2p.getNetworkStatus(); + const triedPeers = p2p['_peerBook'].triedPeers; const triedPeersIPWS = triedPeers.map(peer => { return { ipAddress: peer.ipAddress, wsPort: peer.wsPort }; }); @@ -2311,7 +2312,7 @@ describe('Integration tests for P2P library', () => { it('should return list of peers with at most the minimum discovery threshold', async () => { const firstP2PNode = p2pNodeList[0]; - const { newPeers } = firstP2PNode.getNetworkStatus(); + const newPeers = firstP2PNode['_peerBook'].newPeers; expect(newPeers.length).to.be.at.most(MINIMUM_PEER_DISCOVERY_THRESHOLD); }); }); @@ -2374,7 +2375,7 @@ describe('Integration tests for P2P library', () => { it('should return list of peers with less than maximum discovery response size', async () => { const firstP2PNode = p2pNodeList[0]; - const { newPeers } = firstP2PNode.getNetworkStatus(); + const newPeers = firstP2PNode['_peerBook'].newPeers; expect(newPeers.length).to.be.lessThan( MAX_PEER_DISCOVERY_RESPONSE_LENGTH, ); diff --git a/framework/src/modules/http_api/controllers/peers.js b/framework/src/modules/http_api/controllers/peers.js index 88b5a085bb2..3884a6ec7ea 100644 --- a/framework/src/modules/http_api/controllers/peers.js +++ b/framework/src/modules/http_api/controllers/peers.js @@ -39,7 +39,7 @@ function PeersController(scope) { * @param {function} next * @todo Add description for the function and the params */ -PeersController.getPeers = async function(context, next) { +PeersController.getPeers = async function getPeers(context, next) { const invalidParams = swaggerHelper.invalidParams(context.request); if (invalidParams.length) { diff --git a/framework/src/modules/network/filter_peers.js b/framework/src/modules/network/filter_peers.js index c0dd0bdbc1a..32ec0133bba 100644 --- a/framework/src/modules/network/filter_peers.js +++ b/framework/src/modules/network/filter_peers.js @@ -168,11 +168,7 @@ const getCountByFilter = (peers, filter) => { * @returns {int} count * @todo Add description for the params */ -const getConsolidatedPeersList = ({ - connectedPeers, - newPeers = [], - triedPeers = [], -}) => { +const getConsolidatedPeersList = ({ connectedPeers, disconnectedPeers }) => { // Assign state 2 to the connected peers const connectedList = connectedPeers.map(peer => { const { ipAddress, options, minVersion, nethash, ...peerWithoutIp } = peer; @@ -180,7 +176,7 @@ const getConsolidatedPeersList = ({ return { ip: ipAddress, ...peerWithoutIp, state: PEER_STATE_CONNECTED }; }); // For the peers that are not present in connectedList should be assigned state 1 which is a DISCONNECTED state. - const disconnectedList = [...newPeers, ...triedPeers] + const disconnectedList = disconnectedPeers .filter(peer => { const found = connectedList.find( findPeer => From 80487d55af12f4cf4e218be4e0c3d16aa2f113a4 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Fri, 23 Aug 2019 10:34:18 +0200 Subject: [PATCH 2/9] Break P2P getNetworkStatus() into several functions --- elements/lisk-p2p/src/p2p.ts | 17 ++++++++++------- elements/lisk-p2p/src/p2p_types.ts | 7 ------- framework/src/modules/network/network.js | 19 ++++++++++++------- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/elements/lisk-p2p/src/p2p.ts b/elements/lisk-p2p/src/p2p.ts index 83dbed21386..12f9f9279fa 100644 --- a/elements/lisk-p2p/src/p2p.ts +++ b/elements/lisk-p2p/src/p2p.ts @@ -49,7 +49,6 @@ import { P2PConfig, P2PDiscoveredPeerInfo, P2PMessagePacket, - P2PNetworkStatus, P2PNodeInfo, P2PPeerInfo, P2PPenalty, @@ -531,12 +530,16 @@ export class P2P extends EventEmitter { } } - public getNetworkStatus(): P2PNetworkStatus { - return { - disconnectedPeers: this._peerPool.getDisconnectedPeerInfos(), - connectedPeers: this._peerPool.getAllConnectedPeerInfos(), - connectedUniquePeers: this._peerPool.getUniqueConnectedPeers(), - }; + public getConnectedPeers(): ReadonlyArray { + return this._peerPool.getAllConnectedPeerInfos(); + } + + public getUniqueConnectedPeers(): ReadonlyArray { + return this._peerPool.getUniqueConnectedPeers(); + } + + public getDisconnectedPeers(): ReadonlyArray { + return this._peerPool.getDisconnectedPeerInfos(); } public async request(packet: P2PRequestPacket): Promise { diff --git a/elements/lisk-p2p/src/p2p_types.ts b/elements/lisk-p2p/src/p2p_types.ts index 124d1650127..508d3560099 100644 --- a/elements/lisk-p2p/src/p2p_types.ts +++ b/elements/lisk-p2p/src/p2p_types.ts @@ -113,13 +113,6 @@ export interface P2PConfig { readonly secret?: number; } -// Network info exposed by the P2P library. -export interface P2PNetworkStatus { - readonly disconnectedPeers: ReadonlyArray; - readonly connectedPeers: ReadonlyArray; - readonly connectedUniquePeers: ReadonlyArray; -} - // TODO later: Switch to LIP protocol format. // This is a representation of the outbound peer object according to the current protocol. export interface ProtocolNodeInfo { diff --git a/framework/src/modules/network/network.js b/framework/src/modules/network/network.js index 3a78f02f903..6ff48bfc4e5 100644 --- a/framework/src/modules/network/network.js +++ b/framework/src/modules/network/network.js @@ -343,27 +343,32 @@ module.exports = class Network { }, action.params.peerId, ), - getNetworkStatus: () => this.p2p.getNetworkStatus(), getPeers: action => { - const peerList = getConsolidatedPeersList(this.p2p.getNetworkStatus()); + const peerList = getConsolidatedPeersList({ + connectedPeers: this.p2p.getConnectedPeers(), + disconnectedPeers: this.p2p.getDisconnectedPeers(), + }); return getByFilter(peerList, action.params); }, getConnectedPeers: action => { - const { connectedPeers } = this.p2p.getNetworkStatus(); - const peerList = getConsolidatedPeersList({ connectedPeers }); + const peerList = getConsolidatedPeersList({ + connectedPeers: this.p2p.getConnectedPeers(), + }); return getByFilter(peerList, action.params); }, getPeersCountByFilter: action => { - const peerList = getConsolidatedPeersList(this.p2p.getNetworkStatus()); + const peerList = getConsolidatedPeersList({ + connectedPeers: this.p2p.getConnectedPeers(), + disconnectedPeers: this.p2p.getDisconnectedPeers(), + }); return getCountByFilter(peerList, action.params); }, getConnectedPeersCountByFilter: action => { - const { connectedUniquePeers } = this.p2p.getNetworkStatus(); const peerList = getConsolidatedPeersList({ - connectedPeers: connectedUniquePeers, + connectedPeers: this.p2p.getUniqueConnectedPeers(), }); return getCountByFilter(peerList, action.params); From ea2258d702293baf30165300c4057acfaaf75608 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Fri, 23 Aug 2019 11:12:27 +0200 Subject: [PATCH 3/9] Get disconnected peers from peer book --- elements/lisk-p2p/src/p2p.ts | 18 ++++- elements/lisk-p2p/src/peer_pool.ts | 12 ---- elements/lisk-p2p/test/integration/p2p.ts | 88 +++++++++-------------- 3 files changed, 50 insertions(+), 68 deletions(-) diff --git a/elements/lisk-p2p/src/p2p.ts b/elements/lisk-p2p/src/p2p.ts index 12f9f9279fa..32c298642ee 100644 --- a/elements/lisk-p2p/src/p2p.ts +++ b/elements/lisk-p2p/src/p2p.ts @@ -539,7 +539,23 @@ export class P2P extends EventEmitter { } public getDisconnectedPeers(): ReadonlyArray { - return this._peerPool.getDisconnectedPeerInfos(); + const allPeers = this._peerBook.getAllPeers(); + const connectedPeers = this.getConnectedPeers(); + const disconnectedPeers = allPeers.filter(peer => { + if ( + connectedPeers.find( + connectedPeer => + peer.ipAddress === connectedPeer.ipAddress && + peer.wsPort === connectedPeer.wsPort, + ) + ) { + return false; + } + + return true; + }); + + return disconnectedPeers as P2PDiscoveredPeerInfo[]; } public async request(packet: P2PRequestPacket): Promise { diff --git a/elements/lisk-p2p/src/peer_pool.ts b/elements/lisk-p2p/src/peer_pool.ts index 2f38e3aa78f..2f3baf79a52 100644 --- a/elements/lisk-p2p/src/peer_pool.ts +++ b/elements/lisk-p2p/src/peer_pool.ts @@ -537,12 +537,6 @@ export class PeerPool extends EventEmitter { ); } - public getDisconnectedPeerInfos(): ReadonlyArray { - return this.getDisconnectedPeers().map( - peer => peer.peerInfo as P2PDiscoveredPeerInfo, - ); - } - public getConnectedPeers( kind?: typeof OutboundPeer | typeof InboundPeer, ): ReadonlyArray { @@ -556,12 +550,6 @@ export class PeerPool extends EventEmitter { return peers.filter(peer => peer.state === ConnectionState.OPEN); } - public getDisconnectedPeers(): ReadonlyArray { - const peers = [...this._peerMap.values()]; - - return peers.filter(peer => peer.state !== ConnectionState.OPEN); - } - public getPeer(peerId: string): Peer | undefined { return this._peerMap.get(peerId); } diff --git a/elements/lisk-p2p/test/integration/p2p.ts b/elements/lisk-p2p/test/integration/p2p.ts index 2cbad2ea9ca..d03ff023a7a 100644 --- a/elements/lisk-p2p/test/integration/p2p.ts +++ b/elements/lisk-p2p/test/integration/p2p.ts @@ -151,9 +151,8 @@ describe('Integration tests for P2P library', () => { await wait(POPULATOR_INTERVAL * 10); for (let p2p of p2pNodeList) { - const { connectedPeers } = p2p.getNetworkStatus(); - - const peerPorts = connectedPeers + const peerPorts = p2p + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); const expectedPeerPorts = ALL_NODE_PORTS.filter( @@ -168,16 +167,13 @@ describe('Integration tests for P2P library', () => { describe('Peer banning mechanism', () => { it('should not ban a bad peer for a 10 point penalty', async () => { const firstP2PNode = p2pNodeList[0]; - const { connectedPeers } = firstP2PNode.getNetworkStatus(); - const badPeer = connectedPeers[1]; + const badPeer = firstP2PNode.getConnectedPeers()[1]; const peerPenalty = { peerId: `${badPeer.ipAddress}:${badPeer.wsPort}`, penalty: 10, }; firstP2PNode.applyPenalty(peerPenalty); - const { - connectedPeers: updatedConnectedPeers, - } = firstP2PNode.getNetworkStatus(); + const updatedConnectedPeers = firstP2PNode.getConnectedPeers(); expect(updatedConnectedPeers.map(peer => peer.wsPort)).to.include( badPeer.wsPort, ); @@ -185,16 +181,13 @@ describe('Integration tests for P2P library', () => { it('should ban a bad peer for a 100 point penalty', async () => { const firstP2PNode = p2pNodeList[0]; - const { connectedPeers } = firstP2PNode.getNetworkStatus(); - const badPeer = connectedPeers[2]; + const badPeer = firstP2PNode.getConnectedPeers()[2]; const peerPenalty = { peerId: `${badPeer.ipAddress}:${badPeer.wsPort}`, penalty: 100, }; firstP2PNode.applyPenalty(peerPenalty); - const { - connectedPeers: updatedConnectedPeers, - } = firstP2PNode.getNetworkStatus(); + const updatedConnectedPeers = firstP2PNode.getConnectedPeers(); expect(updatedConnectedPeers.map(peer => peer.wsPort)).to.not.include( badPeer.wsPort, @@ -203,8 +196,7 @@ describe('Integration tests for P2P library', () => { it('should unban a peer after the ban period', async () => { const firstP2PNode = p2pNodeList[0]; - const { connectedPeers } = firstP2PNode.getNetworkStatus(); - const badPeer = connectedPeers[2]; + const badPeer = firstP2PNode.getConnectedPeers()[2]; const peerPenalty = { peerId: `${badPeer.ipAddress}:${badPeer.wsPort}`, penalty: 100, @@ -212,9 +204,7 @@ describe('Integration tests for P2P library', () => { firstP2PNode.applyPenalty(peerPenalty); // Wait for ban time to expire and peer to be re-discovered await wait(1000); - const { - connectedPeers: updatedConnectedPeers, - } = firstP2PNode.getNetworkStatus(); + const updatedConnectedPeers = firstP2PNode.getConnectedPeers(); expect(updatedConnectedPeers.map(peer => peer.wsPort)).to.include( badPeer.wsPort, @@ -367,10 +357,10 @@ describe('Integration tests for P2P library', () => { }); await wait(200); - const connectedPeers = firstP2PNode.getNetworkStatus().connectedPeers; const portOfLastInactivePort = ALL_NODE_PORTS[NETWORK_PEER_COUNT / 2]; - const actualConnectedPeers = connectedPeers + const actualConnectedPeers = firstP2PNode + .getConnectedPeers() .filter( peer => peer.wsPort !== NETWORK_START_PORT && @@ -439,9 +429,8 @@ describe('Integration tests for P2P library', () => { describe('Peer discovery', () => { it('should discover all peers and add them to the connectedPeers list within each node', async () => { for (let p2p of p2pNodeList) { - const { connectedPeers } = p2p.getNetworkStatus(); - - const peerPorts = connectedPeers + const peerPorts = p2p + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); @@ -481,8 +470,6 @@ describe('Integration tests for P2P library', () => { it('should not contain itself in any of its peer list', async () => { for (let p2p of p2pNodeList) { - const { connectedPeers } = p2p.getNetworkStatus(); - const newPeers = p2p['_peerBook'].newPeers; const triedPeers = p2p['_peerBook'].triedPeers; @@ -490,7 +477,8 @@ describe('Integration tests for P2P library', () => { .map(peerInfo => peerInfo.wsPort) .sort(); const newPeerPorts = newPeers.map(peerInfo => peerInfo.wsPort).sort(); - const connectedPeerPorts = connectedPeers + const connectedPeerPorts = p2p + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); @@ -505,9 +493,9 @@ describe('Integration tests for P2P library', () => { describe('Cleanup unresponsive peers', () => { it('should remove inactive 2nd node from connected peer list of other', async () => { - const initialNetworkStatus = p2pNodeList[0].getNetworkStatus(); const secondNode = p2pNodeList[1]; - const initialPeerPorts = initialNetworkStatus.connectedPeers + const initialPeerPorts = p2pNodeList[0] + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); @@ -519,9 +507,8 @@ describe('Integration tests for P2P library', () => { await wait(200); - const networkStatusAfterPeerCrash = p2pNodeList[0].getNetworkStatus(); - - const peerPortsAfterPeerCrash = networkStatusAfterPeerCrash.connectedPeers + const peerPortsAfterPeerCrash = p2pNodeList[0] + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); @@ -748,10 +735,9 @@ describe('Integration tests for P2P library', () => { // For each peer of firstP2PNode, check that the firstP2PNode's P2PPeerInfo was updated with the new height. for (let p2pNode of p2pNodeList.slice(1)) { - const networkStatus = p2pNode.getNetworkStatus(); - const firstP2PNodePeerInfo = networkStatus.connectedPeers.find( - peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, - ); + const firstP2PNodePeerInfo = p2pNode + .getConnectedPeers() + .find(peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort); expect(firstP2PNodePeerInfo).to.exist; expect(firstP2PNodePeerInfo) .to.have.property('height') @@ -777,10 +763,9 @@ describe('Integration tests for P2P library', () => { // For each peer of firstP2PNode, check that the firstP2PNode's P2PPeerInfo was updated with the new height. for (let p2pNode of p2pNodeList.slice(1)) { - const networkStatus = p2pNode.getNetworkStatus(); - const firstNodeInConnectedPeer = networkStatus.connectedPeers.find( - peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, - ); + const firstNodeInConnectedPeer = p2pNode + .getConnectedPeers() + .find(peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort); const newPeers = p2pNode['_peerBook'].newPeers; const triedPeers = p2pNode['_peerBook'].triedPeers; @@ -944,8 +929,8 @@ describe('Integration tests for P2P library', () => { describe('Cleanup unresponsive peers', () => { it('should remove crashed nodes from network status of other nodes', async () => { - const initialNetworkStatus = p2pNodeList[0].getNetworkStatus(); - const initialPeerPorts = initialNetworkStatus.connectedPeers + const initialPeerPorts = p2pNodeList[0] + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); @@ -958,9 +943,8 @@ describe('Integration tests for P2P library', () => { await p2pNodeList[1].stop(); await wait(100); - const networkStatusAfterPeerCrash = p2pNodeList[2].getNetworkStatus(); - - const peerPortsAfterPeerCrash = networkStatusAfterPeerCrash.connectedPeers + const peerPortsAfterPeerCrash = p2pNodeList[2] + .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); @@ -1299,10 +1283,8 @@ describe('Integration tests for P2P library', () => { describe('Peer Discovery', () => { it('should run peer discovery successfully', async () => { for (let p2p of p2pNodeList) { - const connectedPeers = p2p.getNetworkStatus().connectedPeers; - expect(p2p.isActive).to.be.true; - expect(connectedPeers.length).to.gt(1); + expect(p2p.getConnectedPeers().length).to.gt(1); } }); }); @@ -1449,7 +1431,6 @@ describe('Integration tests for P2P library', () => { describe('all the nodes should be able to communicate and receive custom fields passed in nodeinfo', () => { it('should have tried peers with custom test field "modules" that was passed as nodeinfo', () => { for (let p2p of p2pNodeList) { - const { connectedPeers } = p2p.getNetworkStatus(); const triedPeers = p2p['_peerBook'].triedPeers; const newPeers = p2p['_peerBook'].newPeers; @@ -1479,7 +1460,7 @@ describe('Integration tests for P2P library', () => { } } - for (let peer of connectedPeers) { + for (let peer of p2p.getConnectedPeers()) { expect(peer) .has.property('modules') .has.property('names') @@ -1870,8 +1851,7 @@ describe('Integration tests for P2P library', () => { it('should not connect to any blacklisted peer', () => { for (let p2p of p2pNodeList) { - const { connectedPeers } = p2p.getNetworkStatus(); - const connectedPeersIPWS = connectedPeers.map(peer => { + const connectedPeersIPWS = p2p.getConnectedPeers().map(peer => { return { ipAddress: peer.ipAddress, wsPort: peer.wsPort }; }); expect(connectedPeersIPWS).not.to.deep.include.members( @@ -1943,8 +1923,7 @@ describe('Integration tests for P2P library', () => { it('everyone but itself should have a permanent connection to the fixed peer', () => { p2pNodeList.forEach((p2p, index) => { if (index != 0) { - const { connectedPeers } = p2p.getNetworkStatus(); - const connectedPeersIPWS = connectedPeers.map(peer => { + const connectedPeersIPWS = p2p.getConnectedPeers().map(peer => { return { ipAddress: peer.ipAddress, wsPort: peer.wsPort }; }); expect(connectedPeersIPWS).to.deep.include.members(fixedPeers); @@ -2022,8 +2001,7 @@ describe('Integration tests for P2P library', () => { p2pNodeList.forEach((p2p, index) => { if (![0, 9].includes(index)) { p2p.applyPenalty(peerPenalty); - const { connectedPeers } = p2p.getNetworkStatus(); - const connectedPeersIPWS = connectedPeers.map(peer => { + const connectedPeersIPWS = p2p.getConnectedPeers().map(peer => { return { ipAddress: peer.ipAddress, wsPort: peer.wsPort }; }); expect(connectedPeersIPWS).to.deep.include.members( From e95665a3cc15a07394befb6660334c0de8b37f16 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Fri, 23 Aug 2019 11:45:59 +0200 Subject: [PATCH 4/9] Add P2P integration tests to check disconnected peers --- elements/lisk-p2p/test/integration/p2p.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/elements/lisk-p2p/test/integration/p2p.ts b/elements/lisk-p2p/test/integration/p2p.ts index d03ff023a7a..2291a8ec988 100644 --- a/elements/lisk-p2p/test/integration/p2p.ts +++ b/elements/lisk-p2p/test/integration/p2p.ts @@ -1559,6 +1559,27 @@ describe('Integration tests for P2P library', () => { expect(ALL_NODE_PORTS_WITH_LIMIT).to.include.members(peerPorts); } }); + + it('should exist connected and disconnected peers', () => { + for (let p2p of p2pNodeList) { + const connectedPeers = p2p.getConnectedPeers(); + const disconnectedPeers = p2p.getDisconnectedPeers(); + + expect(connectedPeers).is.not.empty; + expect(disconnectedPeers).is.not.empty; + } + }); + + it('connected and disconnected peers should not matched', () => { + for (let p2p of p2pNodeList) { + const connectedPeers = p2p.getConnectedPeers(); + const disconnectedPeers = p2p.getDisconnectedPeers(); + + for (const connectedPeer of connectedPeers) { + expect(disconnectedPeers).to.not.deep.include(connectedPeer); + } + } + }); }); describe('P2P.request', () => { From c65b29eea7ff9a7fea1d6e9857e80774499b2576 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Fri, 23 Aug 2019 13:33:25 +0200 Subject: [PATCH 5/9] Use getAllPeers() from peer book when possible --- elements/lisk-p2p/test/integration/p2p.ts | 43 ++++++----------------- 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/elements/lisk-p2p/test/integration/p2p.ts b/elements/lisk-p2p/test/integration/p2p.ts index 2291a8ec988..12b43c00186 100644 --- a/elements/lisk-p2p/test/integration/p2p.ts +++ b/elements/lisk-p2p/test/integration/p2p.ts @@ -470,21 +470,18 @@ describe('Integration tests for P2P library', () => { it('should not contain itself in any of its peer list', async () => { for (let p2p of p2pNodeList) { - const newPeers = p2p['_peerBook'].newPeers; - const triedPeers = p2p['_peerBook'].triedPeers; + const allPeers = p2p['_peerBook'].getAllPeers(); - const triedPeerPorts = triedPeers + const allPeersPorts = allPeers .map(peerInfo => peerInfo.wsPort) .sort(); - const newPeerPorts = newPeers.map(peerInfo => peerInfo.wsPort).sort(); const connectedPeerPorts = p2p .getConnectedPeers() .map(peerInfo => peerInfo.wsPort) .sort(); expect([ - ...triedPeerPorts, - ...newPeerPorts, + ...allPeersPorts, ...connectedPeerPorts, ]).to.not.contain.members([p2p.nodeInfo.wsPort]); } @@ -767,35 +764,18 @@ describe('Integration tests for P2P library', () => { .getConnectedPeers() .find(peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort); - const newPeers = p2pNode['_peerBook'].newPeers; - const triedPeers = p2pNode['_peerBook'].triedPeers; - - const firstNodeInNewPeer = newPeers.find( - peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, - ); + const allPeersList = p2pNode['_peerBook'].getAllPeers(); - const firstNodeInTriedPeer = triedPeers.find( + const firstNodeInAllPeersList = allPeersList.find( peerInfo => peerInfo.wsPort === firstP2PNode.nodeInfo.wsPort, ); // Check if the peerinfo is updated in new peer list - if (firstNodeInNewPeer) { - expect(firstNodeInNewPeer) - .to.have.property('height') - .which.equals(10); - expect(firstNodeInNewPeer) - .to.have.property('nethash') - .which.equals( - 'da3ed6a45429278bac2666961289ca17ad86595d33b31037615d4b8e8f158bba', - ); - } - - // Check if the peerinfo is updated in tried peer list - if (firstNodeInTriedPeer) { - expect(firstNodeInTriedPeer) + if (firstNodeInAllPeersList) { + expect(firstNodeInAllPeersList) .to.have.property('height') .which.equals(10); - expect(firstNodeInTriedPeer) + expect(firstNodeInAllPeersList) .to.have.property('nethash') .which.equals( 'da3ed6a45429278bac2666961289ca17ad86595d33b31037615d4b8e8f158bba', @@ -1550,11 +1530,8 @@ describe('Integration tests for P2P library', () => { it('should discover peers and add them to the peer lists within each node', () => { for (let p2p of p2pNodeList) { - const triedPeers = p2p['_peerBook'].triedPeers; - const newPeers = p2p['_peerBook'].newPeers; - const peerPorts = [...newPeers, ...triedPeers].map( - peerInfo => peerInfo.wsPort, - ); + const allPeers = p2p['_peerBook'].getAllPeers(); + const peerPorts = allPeers.map(peerInfo => peerInfo.wsPort); expect(ALL_NODE_PORTS_WITH_LIMIT).to.include.members(peerPorts); } From 66eaeee99b0d5340ccae4a990363de268c96977f Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Fri, 23 Aug 2019 15:45:48 +0200 Subject: [PATCH 6/9] Improve descriptions of p2p integration tests --- elements/lisk-p2p/test/integration/p2p.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/elements/lisk-p2p/test/integration/p2p.ts b/elements/lisk-p2p/test/integration/p2p.ts index 12b43c00186..dd88d5c3ce6 100644 --- a/elements/lisk-p2p/test/integration/p2p.ts +++ b/elements/lisk-p2p/test/integration/p2p.ts @@ -1537,7 +1537,7 @@ describe('Integration tests for P2P library', () => { } }); - it('should exist connected and disconnected peers', () => { + it('should have connected and disconnected peers', () => { for (let p2p of p2pNodeList) { const connectedPeers = p2p.getConnectedPeers(); const disconnectedPeers = p2p.getDisconnectedPeers(); @@ -1547,7 +1547,7 @@ describe('Integration tests for P2P library', () => { } }); - it('connected and disconnected peers should not matched', () => { + it('should have disjoint connected and disconnected peers', () => { for (let p2p of p2pNodeList) { const connectedPeers = p2p.getConnectedPeers(); const disconnectedPeers = p2p.getDisconnectedPeers(); From 76b3d7b14ed762483651b16d44df779a9fe17b0b Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Mon, 26 Aug 2019 11:26:45 +0200 Subject: [PATCH 7/9] Add default argument value to getConsolidatedPeersList() --- framework/src/modules/network/filter_peers.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/framework/src/modules/network/filter_peers.js b/framework/src/modules/network/filter_peers.js index 32ec0133bba..18fcf550918 100644 --- a/framework/src/modules/network/filter_peers.js +++ b/framework/src/modules/network/filter_peers.js @@ -168,7 +168,10 @@ const getCountByFilter = (peers, filter) => { * @returns {int} count * @todo Add description for the params */ -const getConsolidatedPeersList = ({ connectedPeers, disconnectedPeers }) => { +const getConsolidatedPeersList = ({ + connectedPeers = [], + disconnectedPeers = [], +}) => { // Assign state 2 to the connected peers const connectedList = connectedPeers.map(peer => { const { ipAddress, options, minVersion, nethash, ...peerWithoutIp } = peer; From 89272568a43137119bec8c79bb24dad225994d4b Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Mon, 26 Aug 2019 11:27:09 +0200 Subject: [PATCH 8/9] Remove remaining usage of getNetworkStatus network action --- framework/src/modules/network/index.js | 3 --- framework/src/modules/network/network.js | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/framework/src/modules/network/index.js b/framework/src/modules/network/index.js index 3bede3c8aa3..1a5848938eb 100644 --- a/framework/src/modules/network/index.js +++ b/framework/src/modules/network/index.js @@ -60,9 +60,6 @@ module.exports = class NetworkModule extends BaseModule { emit: { handler: action => this.network.actions.emit(action), }, - getNetworkStatus: { - handler: () => this.network.actions.getNetworkStatus(), - }, getPeers: { handler: action => this.network.actions.getPeers(action), }, diff --git a/framework/src/modules/network/network.js b/framework/src/modules/network/network.js index 6ff48bfc4e5..f40c3bad133 100644 --- a/framework/src/modules/network/network.js +++ b/framework/src/modules/network/network.js @@ -383,7 +383,7 @@ module.exports = class Network { // TODO: In phase 2, only previousPeers will be saved to database this.logger.info('Cleaning network...'); - const peersToSave = this.p2p.getNetworkStatus().connectedPeers.map(peer => { + const peersToSave = this.p2p.getConnectedPeers().map(peer => { const { ipAddress, ...peerWithoutIp } = peer; return { From 36e4662ef33b7b110a1b9dfe00d5b7ccbf17b658 Mon Sep 17 00:00:00 2001 From: Diego Garcia Date: Mon, 26 Aug 2019 11:49:22 +0200 Subject: [PATCH 9/9] Remove redundant filter on P2P getConsolidatedPeersList() --- framework/src/modules/network/filter_peers.js | 33 +++++-------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/framework/src/modules/network/filter_peers.js b/framework/src/modules/network/filter_peers.js index 18fcf550918..0ed35fdb622 100644 --- a/framework/src/modules/network/filter_peers.js +++ b/framework/src/modules/network/filter_peers.js @@ -178,30 +178,15 @@ const getConsolidatedPeersList = ({ return { ip: ipAddress, ...peerWithoutIp, state: PEER_STATE_CONNECTED }; }); - // For the peers that are not present in connectedList should be assigned state 1 which is a DISCONNECTED state. - const disconnectedList = disconnectedPeers - .filter(peer => { - const found = connectedList.find( - findPeer => - findPeer.ip === peer.ipAddress && findPeer.wsPort === peer.wsPort, - ); - return !found; - }) - .map(peer => { - const { - ipAddress, - options, - minVersion, - nethash, - ...peerWithoutIp - } = peer; - - return { - ip: ipAddress, - ...peerWithoutIp, - state: PEER_STATE_DISCONNECTED, - }; - }); + const disconnectedList = disconnectedPeers.map(peer => { + const { ipAddress, options, minVersion, nethash, ...peerWithoutIp } = peer; + + return { + ip: ipAddress, + ...peerWithoutIp, + state: PEER_STATE_DISCONNECTED, + }; + }); return [...connectedList, ...disconnectedList]; };