Merge pull request LiskArchive#4616 from LiskHQ/4609-peerbook-list-di…

…sjoint-fix P2P peerBook disjoint fix and test improvements - Closes LiskArchive#4609 & LiskArchive#4599 sign: Diego Garcia <diego.gm@protonmail.com>
diego-G · Dec 10, 2019 · 8d477d4 · 8d477d4
2 parents 5fa75b3 + dc50f15
commit 8d477d4
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 39 deletions.
diff --git a/elements/lisk-p2p/src/peer_book/base_list.ts b/elements/lisk-p2p/src/peer_book/base_list.ts
@@ -38,7 +38,7 @@ export interface BucketInfo {
 export class BaseList {
 	protected bucketIdToBucket: Map<number, Bucket>;
 	/* 
-		Auxilliary map for direct peerId => peerInfo lookups
+		Auxillary map for direct peerId => peerInfo lookups
 		Required because peerLists may be provided by discrete sources
 	*/
 	protected peerIdToPeerInfo: Map<string, P2PEnhancedPeerInfo>;
@@ -58,19 +58,10 @@ export class BaseList {
 			secret,
 		};
 		this.bucketIdToBucket = new Map();
-		this.initBuckets(this.bucketIdToBucket);
+		this._initBuckets();
 		this.peerIdToPeerInfo = new Map();
 	}
 
-	public initBuckets(bucketIdToBucket: Map<number, Bucket>): void {
-		// Init the Map with all the buckets
-		for (const bucketId of [
-			...new Array(this.peerListConfig.numOfBuckets).keys(),
-		]) {
-			bucketIdToBucket.set(bucketId, new Map<string, P2PEnhancedPeerInfo>());
-		}
-	}
-
 	public get peerList(): ReadonlyArray<P2PPeerInfo> {
 		const peerListMap: P2PPeerInfo[] = [];
 
@@ -192,7 +183,7 @@ export class BaseList {
 	protected getBucket(peerId: string): Bucket | undefined {
 		const internalPeerInfo = this.peerIdToPeerInfo.get(peerId);
 
-		if (!internalPeerInfo?.bucketId) {
+		if (typeof internalPeerInfo?.bucketId !== 'number') {
 			return undefined;
 		}
 
@@ -204,4 +195,16 @@ export class BaseList {
 
 		return bucket;
 	}
+
+	private _initBuckets(): void {
+		// Init the Map with all the buckets
+		for (const bucketId of [
+			...new Array(this.peerListConfig.numOfBuckets).keys(),
+		]) {
+			this.bucketIdToBucket.set(
+				bucketId,
+				new Map<string, P2PEnhancedPeerInfo>(),
+			);
+		}
+	}
 }
diff --git a/elements/lisk-p2p/src/peer_book/peer_book.ts b/elements/lisk-p2p/src/peer_book/peer_book.ts
@@ -135,16 +135,9 @@ export class PeerBook {
 		return false;
 	}
 
-	public removePeer(peerInfo: P2PPeerInfo): boolean {
-		if (this._triedPeers.getPeer(peerInfo.peerId)) {
-			return this._triedPeers.removePeer(peerInfo);
-		}
-
-		if (this._newPeers.getPeer(peerInfo.peerId)) {
-			return this._newPeers.removePeer(peerInfo);
-		}
-
-		return false;
+	public removePeer(peerInfo: P2PPeerInfo): void {
+		this._newPeers.removePeer(peerInfo);
+		this._triedPeers.removePeer(peerInfo);
 	}
 
 	public upgradePeer(peerInfo: P2PEnhancedPeerInfo): boolean {
@@ -153,7 +146,7 @@ export class PeerBook {
 		}
 
 		if (this._newPeers.hasPeer(peerInfo.peerId)) {
-			this._newPeers.removePeer(peerInfo);
+			this.removePeer(peerInfo);
 			this._triedPeers.addPeer(peerInfo);
 
 			return true;

diff --git a/elements/lisk-p2p/src/peer_book/tried_list.ts b/elements/lisk-p2p/src/peer_book/tried_list.ts
@@ -42,8 +42,6 @@ export class TriedList extends BaseList {
 		this._maxReconnectTries = maxReconnectTries
 			? maxReconnectTries
 			: DEFAULT_MAX_RECONNECT_TRIES;
-
-		this.initBuckets(this.bucketIdToBucket);
 	}
 
 	public get triedPeerConfig(): TriedListConfig {

diff --git a/elements/lisk-p2p/test/unit/peer_book/peer_book.ts b/elements/lisk-p2p/test/unit/peer_book/peer_book.ts
@@ -244,25 +244,18 @@ describe('peerBook', () => {
 		});
 
 		describe('when peer exists in the tried peers list', () => {
-			it('should return true', () => {
+			it('should be removed', () => {
 				peerBook.addPeer(samplePeers[0]);
 				peerBook.upgradePeer(samplePeers[0]);
-				expect(peerBook.removePeer(samplePeers[0])).to.be.true;
+				peerBook.removePeer(samplePeers[0]);
 				expect(peerBook.getPeer(samplePeers[0])).to.be.undefined;
 			});
 		});
 
 		describe('when peer exists in the new peers list', () => {
-			it('should return true', () => {
+			it('should be removed', () => {
 				peerBook.addPeer(samplePeers[0]);
-				expect(peerBook.removePeer(samplePeers[0])).to.be.true;
-				expect(peerBook.getPeer(samplePeers[0])).to.be.undefined;
-			});
-		});
-
-		describe('when peer does not exists in any of the peer book lists', () => {
-			it('should return false', () => {
-				expect(peerBook.removePeer(samplePeers[0])).to.be.false;
+				peerBook.removePeer(samplePeers[0]);
 				expect(peerBook.getPeer(samplePeers[0])).to.be.undefined;
 			});
 		});
@@ -402,4 +395,31 @@ describe('peerBook', () => {
 			).to.be.lt(maxPeerListLength + 1);
 		});
 	});
+
+	describe('when PeerBook populated and cleaned up', () => {
+		beforeEach(() => {
+			samplePeers = initPeerInfoListWithSuffix('204.123.64', 3500);
+			peerBook = new PeerBook(peerBookConfig);
+
+			samplePeers.forEach(samplePeer => {
+				if (!peerBook.hasPeer(samplePeer)) {
+					peerBook.addPeer(samplePeer);
+				}
+
+				peerBook.upgradePeer(samplePeer);
+			});
+		});
+
+		it('should return empty Peer lists', () => {
+			const AllPeers = peerBook.allPeers;
+
+			AllPeers.forEach(peer => {
+				peerBook.removePeer(peer);
+			});
+
+			expect(peerBook.newPeers).to.be.empty;
+			expect(peerBook.triedPeers).to.be.empty;
+			expect(peerBook.allPeers).to.be.empty;
+		});
+	});
 });
diff --git a/elements/lisk-p2p/test/unit/utils/select.ts b/elements/lisk-p2p/test/unit/utils/select.ts
@@ -24,6 +24,7 @@ import {
 } from '../../../src/utils/select';
 import { P2PNodeInfo, P2PPeerInfo } from '../../../src/p2p_types';
 import { DEFAULT_SEND_PEER_LIMIT } from '../../../src/constants';
+import sinon = require('sinon');
 
 describe('peer selector', () => {
 	const nodeInfo: P2PNodeInfo = {
@@ -352,7 +353,17 @@ describe('peer selector', () => {
 		});
 
 		describe('when there are less than 100 peers', () => {
+			let mathRandom: sinon.SinonStub;
+			before(function() {
+				mathRandom = sinon.stub(Math, 'random');
+			});
+
+			after(function() {
+				sandbox.restore();
+			});
+
 			it('should return peers uniformly from both lists', () => {
+				mathRandom.returns(0.499);
 				const triedPeers = initPeerInfoListWithSuffix('111.112.113', 25);
 				const newPeers = initPeerInfoListWithSuffix('111.112.114', 75);
 
@@ -369,14 +380,48 @@ describe('peer selector', () => {
 
 				let triedCount = 0;
 				let newCount = 0;
+
 				for (const peer of selectedPeers) {
 					if (triedPeers.find(triedPeer => peer.peerId === triedPeer.peerId)) {
 						triedCount++;
+					} else {
+						newCount++;
 					}
-					newCount++;
 				}
-				expect(triedCount).to.gte(23);
-				expect(newCount).to.gte(23);
+
+				expect(triedCount).to.eql(25);
+				expect(newCount).to.eql(25);
+			});
+
+			it('should return only new peer list', () => {
+				mathRandom.returns(0.5);
+				const triedPeers = initPeerInfoListWithSuffix('111.112.113', 25);
+				const newPeers = initPeerInfoListWithSuffix('111.112.114', 75);
+
+				const selectedPeers = selectPeersForConnection({
+					triedPeers,
+					newPeers,
+					peerLimit: 50,
+				});
+				expect(selectedPeers)
+					.to.be.an('array')
+					.of.length(50);
+
+				expect([...triedPeers, ...newPeers]).to.include.members(selectedPeers);
+
+				let triedCount = 0;
+				let newCount = 0;
+
+				for (const peer of selectedPeers) {
+					if (triedPeers.find(triedPeer => peer.peerId === triedPeer.peerId)) {
+						triedCount++;
+					} else {
+						newCount++;
+					}
+				}
+
+				expect(triedCount).to.eql(0);
+				expect(newCount).to.eql(50);
 			});
 		});