diff --git a/elements/lisk-p2p/src/peer_book/base_list.ts b/elements/lisk-p2p/src/peer_book/base_list.ts index 53163c0dbe8..314b8bbc7ad 100644 --- 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; /* - 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; @@ -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): void { - // Init the Map with all the buckets - for (const bucketId of [ - ...new Array(this.peerListConfig.numOfBuckets).keys(), - ]) { - bucketIdToBucket.set(bucketId, new Map()); - } - } - public get peerList(): ReadonlyArray { 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(), + ); + } + } } diff --git a/elements/lisk-p2p/src/peer_book/peer_book.ts b/elements/lisk-p2p/src/peer_book/peer_book.ts index d5bc710f310..75ca9b96179 100644 --- 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 index 2933e96fc70..2c02803e15c 100644 --- 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 index 91c73fad8a7..c072b109463 100644 --- 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 index 50ab15059b3..27d82e7eccd 100644 --- 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); }); });