Implement new/tried peer list and peer bucket management Closes #3334 #4030

ishantiw · 2019-07-31T14:43:24Z

Description

Implementation of peer management according to the LIP0004.

Changes:

A new folder peer_directory that will encapsulate new/triedPeers list and the bucket management within it.
peer_directory/new_peers includes management of newPeers list and newPeers related eviction logic
peer_directory/tried_peers includes management of triedPeers list and triedPeers related eviction logic
peer_directory/peer_book exposes the interface for peer management at a higher level, it exposes,
- addPeer
- removePeer
- updatePeer
- downgradePeer
- upgradePeer
- getPeer
- getAllPeers
p2p to use peer_book instead of dealing with new/triedPeers list separately

Review checklist

The PR resolves Introduce management of peer buckets within new/tried peers list #3334 Fix _newPeers and _triedPeers to match the LIP #3330
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

diego-G

First batch of comments. I will go deep into the peer directory folder now. But before, what do you think about changing the folder name to directory only?

elements/lisk-p2p/src/p2p.ts

ManuGowda

@ishantiw just made few comments, otherwise, the PR looks good

elements/lisk-p2p/src/p2p.ts

elements/lisk-p2p/src/peer_directory/peer_book.ts

elements/lisk-p2p/src/peer_directory/new_peers.ts

diego-G

Another batch of comments. I will give you a third review after you solve them. It only remains me to have a deep look to new_peers and tried_peers files.

elements/lisk-p2p/src/p2p.ts

elements/lisk-p2p/src/utils.ts

elements/lisk-p2p/src/directory/peer_book.ts

elements/lisk-p2p/src/directory/new_peers.ts

elements/lisk-p2p/src/directory/tried_peers.ts

elements/lisk-p2p/src/directory/new_peers.ts

elements/lisk-p2p/src/directory/tried_peers.ts

elements/lisk-p2p/src/p2p.ts

mitsuaki-u

Few minor comments regarding function naming. Great job!

elements/lisk-p2p/src/directory/new_peers.ts

elements/lisk-p2p/src/directory/tried_peers.ts

elements/lisk-p2p/src/directory/index.ts

diego-G

@ishantiw remember to add test integration scenarios to prove the eviction process, upgrades and downgrades are working as expected.

ishantiw · 2019-08-05T23:28:44Z

@diego-G There are unit tests related to downgrade and upgrade a peer. we need extra effort and time to test eligibleDaysForEviction and bucket full scenario as we need to populate buckets with valid peers that will be added to a specific bucket. Let's tackle that in a separate issue that I have created #4045

diego-G

I suggest again to add a scenario to check if peers get downgraded as expected. For the eviction process I agree we should tackle it in another PR.

elements/lisk-p2p/src/directory/new_peers.ts

elements/lisk-p2p/src/directory/tried_peers.ts

elements/lisk-p2p/src/peer_directory/new_peers.ts

elements/lisk-p2p/src/peer_directory/tried_peers.ts

…and triedPeersList

… in milliseconds

ishantiw · 2019-08-06T11:01:35Z

@diego-G I have added few more scenarios on PeerBook for upgrade/downgrade of a peer and its movement between the two lists.

elements/lisk-p2p/src/peer_directory/peer_book.ts

elements/lisk-p2p/src/peer_directory/new_peers.ts

elements/lisk-p2p/src/peer_directory/peer_book.ts

ishantiw requested review from diego-G, jondubois and mitsuaki-u July 31, 2019 15:12

ishantiw self-assigned this Jul 31, 2019

ishantiw added the status: pending review label Jul 31, 2019

ishantiw marked this pull request as ready for review July 31, 2019 15:19

ishantiw changed the title ~~Implement new/tried peer list and peer bucket management~~ Implement new/tried peer list and peer bucket management Closes #3334 Jul 31, 2019

diego-G suggested changes Aug 1, 2019

View reviewed changes

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

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

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

ManuGowda reviewed Aug 1, 2019

View reviewed changes

ishantiw force-pushed the 3334-peer_buckets branch 2 times, most recently from 258df5a to bd4f5b0 Compare August 2, 2019 12:03

ManuGowda approved these changes Aug 2, 2019

View reviewed changes

ishantiw requested a review from diego-G August 2, 2019 12:51

diego-G suggested changes Aug 2, 2019

View reviewed changes

jondubois suggested changes Aug 2, 2019

View reviewed changes

mitsuaki-u approved these changes Aug 5, 2019

View reviewed changes

mitsuaki-u reviewed Aug 5, 2019

View reviewed changes

ishantiw changed the base branch from feature/implement-lip-p2p to development August 5, 2019 15:06

shuse2 suggested changes Aug 5, 2019

View reviewed changes

ishantiw requested review from diego-G and jondubois August 5, 2019 15:23

diego-G mentioned this pull request Aug 5, 2019

Netgroup inbound peer eviction and secret generation - Closes #3976 #4023

Merged

5 tasks

diego-G suggested changes Aug 5, 2019

View reviewed changes

ishantiw requested review from mitsuaki-u, diego-G and shuse2 August 5, 2019 23:44

diego-G suggested changes Aug 6, 2019

View reviewed changes

shuse2 approved these changes Aug 6, 2019

View reviewed changes

mitsuaki-u approved these changes Aug 6, 2019

View reviewed changes

ishantiw added 14 commits August 6, 2019 12:18

Destructure the arguments to constructor

9096054

Remove invalid comment

a0ad55d

Use constructPeerIdFromPeerInfo from utils and remove from base peer

9c59631

Rename peer list size property to peer bucket count

0d03c45

Remove the use of array.reduce from getPeerlist

c95e284

Make eligibleDaysForEviction configurable for newPeers

f86bb40

Revert rename changes directory->peer_directory

d1fcb51

More comments related to eviction and failedConnectionAction

f596c62

Remove findPeer and use addPeer instead

1e3d9e4

Remove getter of peerMap and rename get func of list to newPeersList …

b641631

…and triedPeersList

Use new Array and start bucket from zero

cc6561c

Use early return and remove else

b5f367a

Use for of instead of forEach

c722686

Change eligibleDaysForEviction -> evictionThresholdTime and use value…

acc8dbc

… in milliseconds

ishantiw force-pushed the 3334-peer_buckets branch from cc9c10f to acc8dbc Compare August 6, 2019 10:18

ishantiw added 2 commits August 6, 2019 12:36

Check if peer exists on discovery and only then upgrade

e707fc9

Add more scenario in peer_book to test a peer on downgrade and upgrade

32bc60c

ishantiw requested a review from diego-G August 6, 2019 11:01

diego-G suggested changes Aug 6, 2019

View reviewed changes

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

elements/lisk-p2p/src/peer_directory/new_peers.ts Show resolved Hide resolved

diego-G suggested changes Aug 6, 2019

View reviewed changes

elements/lisk-p2p/src/peer_directory/peer_book.ts Show resolved Hide resolved

Remove bannedPeers from peerBook

3086e95

ishantiw requested a review from diego-G August 6, 2019 12:41

diego-G approved these changes Aug 6, 2019

View reviewed changes

Merge development into 3334-peer_buckets

d9a8177

shuse2 added status: ready and removed status: pending review labels Aug 6, 2019

jondubois approved these changes Aug 6, 2019

View reviewed changes

shuse2 merged commit 1a16e6f into development Aug 6, 2019

shuse2 deleted the 3334-peer_buckets branch August 6, 2019 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new/tried peer list and peer bucket management Closes #3334 #4030

Implement new/tried peer list and peer bucket management Closes #3334 #4030

ishantiw commented Jul 31, 2019 •

edited

Loading

diego-G left a comment

ManuGowda left a comment

diego-G left a comment •

edited

Loading

mitsuaki-u left a comment

diego-G left a comment

ishantiw commented Aug 5, 2019 •

edited

Loading

diego-G left a comment

ishantiw commented Aug 6, 2019

Implement new/tried peer list and peer bucket management Closes #3334 #4030

Implement new/tried peer list and peer bucket management Closes #3334 #4030

Conversation

ishantiw commented Jul 31, 2019 • edited Loading

Description

Review checklist

diego-G left a comment

Choose a reason for hiding this comment

ManuGowda left a comment

Choose a reason for hiding this comment

diego-G left a comment • edited Loading

Choose a reason for hiding this comment

mitsuaki-u left a comment

Choose a reason for hiding this comment

diego-G left a comment

Choose a reason for hiding this comment

ishantiw commented Aug 5, 2019 • edited Loading

diego-G left a comment

Choose a reason for hiding this comment

ishantiw commented Aug 6, 2019

ishantiw commented Jul 31, 2019 •

edited

Loading

diego-G left a comment •

edited

Loading

ishantiw commented Aug 5, 2019 •

edited

Loading