Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

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

Merged
merged 9 commits into from
Aug 6, 2019

Conversation

mitsuaki-u
Copy link
Contributor

@mitsuaki-u mitsuaki-u commented Jul 31, 2019

What was the problem?

Eviction of inbound peers based on netgroup value was not implemented.

Node required a random 32-bit entropy secret on startup.

How did I solve it?

Peers are now evicted based on 4 categories including netgroup value as per new protocol.
Secret generated on node startup.

How to manually test it?

Start node. Set netgroupProtectionRatio to a high value for P2PConfig. Check which peers are evicted.

Review checklist

elements/lisk-p2p/src/p2p_types.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/peer/base.ts Outdated Show resolved Hide resolved
framework/src/modules/network/network.js Outdated Show resolved Hide resolved
@mitsuaki-u mitsuaki-u force-pushed the 3976_netgroup_inbound_eviction branch 2 times, most recently from 2c31ed1 to 93fb61f Compare July 31, 2019 10:17
@mitsuaki-u mitsuaki-u changed the base branch from feature/implement-lip-p2p to development August 5, 2019 15:12
Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this issue also save the secret to the database?

elements/lisk-p2p/src/peer_pool.ts Show resolved Hide resolved
@mitsuaki-u
Copy link
Contributor Author

Shouldn't this issue also save the secret to the database?

After discussion with @jondubois we decided to delay persistence to db until a later release. The effect was minimal enough that it can be seen as an optimization rather than necessity.

Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Please open an issue for saving the secret to the database =)

diego-G
diego-G previously requested changes Aug 5, 2019
Copy link

@diego-G diego-G left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left few comments. Besides, I'd like to relapse if the features we are adding in this PR is actually covered with any of the existing integration tests.

.github/ISSUE_TEMPLATE/ISSUE_TEMPLATE_BUG_REPORT.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/ISSUE_TEMPLATE_FEATURE.md Outdated Show resolved Hide resolved
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 Outdated Show resolved Hide resolved
elements/lisk-p2p/test/integration/p2p.ts Show resolved Hide resolved
@mitsuaki-u
Copy link
Contributor Author

mitsuaki-u commented Aug 5, 2019

I've left few comments. Besides, I'd like to relapse if the features we are adding in this PR is actually covered with any of the existing integration tests.

There is the limiting factor of not being able to fake the first two IP address groups in integration tests. However the getNetgroup function itself is already covered by unit tests. The new code in this PR is the use of netgroup in protecting some inbound peers from eviction. The functionality is identical with protecting peers for connectTime, which is already covered by integration test.

shuse2
shuse2 previously requested changes Aug 6, 2019
elements/lisk-p2p/package-lock.json Outdated Show resolved Hide resolved
@mitsuaki-u mitsuaki-u force-pushed the 3976_netgroup_inbound_eviction branch from 0f010d3 to b9f70e1 Compare August 6, 2019 10:29
@diego-G diego-G self-requested a review August 6, 2019 10:36
@mitsuaki-u mitsuaki-u force-pushed the 3976_netgroup_inbound_eviction branch from 3b2ac47 to 3c47ec9 Compare August 6, 2019 12:10
@shuse2 shuse2 merged commit 273388e into development Aug 6, 2019
@shuse2 shuse2 deleted the 3976_netgroup_inbound_eviction branch August 6, 2019 13:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use netgrouping for inbound evictions
4 participants