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

Implement peer banning mechanism - Closes #3343 #3664 #3665

Merged
merged 17 commits into from
May 29, 2019
Merged

Implement peer banning mechanism - Closes #3343 #3664 #3665

merged 17 commits into from
May 29, 2019

Conversation

mitsuaki-u
Copy link
Contributor

@mitsuaki-u mitsuaki-u commented May 17, 2019 •
edited
Loading

What was the problem?

No mechanism was in place for penalizing peers bad behavior as per LIP 0004.

How did I fix it?

Added banning functionality to P2P library.

How to test it?

Review checklist

@mitsuaki-u mitsuaki-u changed the base branch from development to feature/implement-lip-p2p May 20, 2019 10:28
@mitsuaki-u mitsuaki-u marked this pull request as ready for review May 21, 2019 12:48
diego-G
diego-G previously requested changes May 21, 2019
View reviewed changes
elements/lisk-p2p/src/p2p.ts Show resolved Hide resolved
framework/src/modules/network/network.js Outdated Show resolved Hide resolved
@mitsuaki-u mitsuaki-u changed the title Implement peer banning mechanism - Closes #3343 Implement peer banning mechanism - Closes #3343 #3664 May 21, 2019
Repository owner deleted a comment from shuse2 May 22, 2019
shuse2
shuse2 suggested changes May 22, 2019
View reviewed changes
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/peer.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/peer.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/peer.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/peer_pool.ts Outdated Show resolved Hide resolved
shuse2
shuse2 previously requested changes May 24, 2019
View reviewed changes
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.

Would it be possible to add banning integration test or it's still too early?

elements/lisk-p2p/src/p2p.ts Show resolved Hide resolved
@mitsuaki-u mitsuaki-u dismissed stale reviews from shuse2 and diego-G May 24, 2019 12:04

Addressed.

@mitsuaki-u mitsuaki-u requested a review from diego-G May 27, 2019 14:16
diego-G
diego-G previously requested changes May 28, 2019
View reviewed changes
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.

LGTM. Just two comments.

elements/lisk-p2p/test/integration/p2p.ts Outdated Show resolved Hide resolved
framework/src/modules/network/network.js Outdated Show resolved Hide resolved
@mitsuaki-u mitsuaki-u requested a review from diego-G May 28, 2019 09:21
diego-G
diego-G reviewed May 28, 2019
View reviewed changes
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 think we're not properly passing the config parameters to the network module and later to the library. Please, rebase your branch to the feature.

jondubois
jondubois previously requested changes May 28, 2019
View reviewed changes
elements/lisk-p2p/src/p2p.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p_request.ts Outdated Show resolved Hide resolved
elements/lisk-p2p/src/p2p_types.ts Outdated Show resolved Hide resolved
@mitsuaki-u mitsuaki-u requested a review from jondubois May 29, 2019 08:13
jondubois
jondubois reviewed May 29, 2019
View reviewed changes
elements/lisk-p2p/test/integration/p2p.ts
@@ -168,6 +169,63 @@ describe('Integration tests for P2P library', () => {
});
});

describe('Peer banning mechanism', () => {
Copy link
Contributor

@jondubois jondubois May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Nice test cases 👍

jondubois
jondubois previously requested changes May 29, 2019
View reviewed changes
framework/src/modules/network/network.js Show resolved Hide resolved
@jondubois jondubois mentioned this pull request May 29, 2019
Closed
@jondubois jondubois merged commit d38c08f into feature/implement-lip-p2p May 29, 2019
@shuse2 shuse2 deleted the 3343_ban_manager branch June 19, 2019 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jondubois jondubois jondubois approved these changes

@diego-G diego-G diego-G approved these changes

@shuse2 shuse2 shuse2 approved these changes

Assignees

@mitsuaki-u mitsuaki-u

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mitsuaki-u @jondubois @diego-G @shuse2 @karmacoma