feat: add ConnectionGater by randomshinichi · Pull Request #610 · evstack/ev-node

randomshinichi · 2022-11-17T13:55:25Z

Overview

Resolves#386
Blacklists/whitelists are only applied while starting the Client, and do not update while the Client is running.

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

tzdybal

Thanks for the submission!
I left some comments.

codecov-commenter · 2022-11-28T15:36:19Z

Codecov Report

Merging #610 (ff454bb) into main (90ff206) will decrease coverage by 0.03%.
The diff coverage is 52.27%.

@@ Coverage Diff @@## main #610 +/- ##
==========================================
- Coverage 55.06% 55.02% -0.04% 
==========================================
Files 48 48 Lines 10206 10224 +18 ==========================================
+ Hits 5620 5626 +6 - Misses 3724 3732 +8 - Partials 862 866 +4

Impacted Files	Coverage Δ
node/node.go	`60.60% <40.00%> (+0.17%)`	⬆️
p2p/client.go	`60.72% <53.84%> (-2.32%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tzdybal

Thanks for the submission! I left some comments.

Manav-Aggarwal

Thanks for the submission! Left some comments. Had one question, when are whitelists useful as opposed to just having a blocklist? Or is the point to have overlap in whitelists and blocklists when unblocking a peer? Also, what happens when there are duplicate peer IDs in these blocklists/whitelists?

tzdybal · 2022-12-20T06:14:20Z

When we get to the point where peers are blocked automatically (because of for example spamming, sending invalid FPs, etc), we might want to manually unblock specific peer.

Manav-Aggarwal · 2022-12-20T16:36:07Z

When we get to the point where peers are blocked automatically (because of for example spamming, sending invalid FPs, etc), we might want to manually unblock specific peer.

That makes sense but instead of having that peer in both the whitelist and blacklist, is it possible to just remove the peer from the blacklist instead since this is done during client initialization?

TestClientStartup() ensures that using blacklists/whitelists don't result in an error log which peers exactly were white/blacklisted don't reuse gater between subtests blacklist/whitelist should be setup before gossiping/DHT/peer discovery do not create ConnectionGater yourself if gater was nil style revisions

tzdybal

@randomshinichi thanks again for your submission!

nashqueue

No additional comments to the ones that were mentioned above. LGTM if they are resolved

tzdybal · 2023-01-16T14:08:51Z

I changed the logic a bit - connection gater is created in NewNode to make sure that it's always present - this simplifies the code.

Also addressed all the comments.

To simplify our code, lets just depend on gater ability to block/unblock right peers.

gupadhyaya

LGTM.

nashqueue

LGTM

gupadhyaya

LGTM again :P

randomshinichi requested a review from tzdybal as a code owner November 17, 2022 13:55

tzdybal marked this pull request as draft November 17, 2022 14:04

tzdybal changed the title ~~DRAFT: feature/connectiongater~~feature/connectiongaterNov 17, 2022

tzdybal reviewed Nov 17, 2022
View reviewed changes

Comment threadp2p/client_test.go Outdated
Comment threadp2p/client.go
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go

randomshinichi force-pushed the feature/connectiongater branch from fcb57fd to b17b4baCompare November 21, 2022 13:33

tzdybal mentioned this pull request Nov 21, 2022
Use Datasource interface #620
Closed
2 tasks

randomshinichi marked this pull request as ready for review November 28, 2022 15:29

tzdybal requested review from a team, Manav-Aggarwal and gupadhyaya and removed request for a teamNovember 29, 2022 06:34

tzdybal suggested changes Dec 1, 2022
View reviewed changes

Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated

gupadhyaya reviewed Dec 3, 2022
View reviewed changes

Comment threadp2p/client.go Outdated

tzdybal added the T:enhancement label Dec 6, 2022

randomshinichi force-pushed the feature/connectiongater branch 2 times, most recently from 768aa05 to 3687d3eCompare December 11, 2022 11:16

tzdybal previously approved these changes Dec 19, 2022
View reviewed changes

tzdybal force-pushed the feature/connectiongater branch from 3687d3e to dad8975Compare December 19, 2022 20:17

tzdybal enabled auto-merge (squash) December 19, 2022 20:19

Manav-Aggarwal previously requested changes Dec 19, 2022
View reviewed changes

Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client.go Outdated
Comment threadp2p/client_test.go Outdated

tzdybal dismissed their stale review via dad8975December 20, 2022 18:03

randomshinichi added 4 commits January 12, 2023 14:13

initial ConnectionGater design
4bf05d5

rename Client.getSeedAddrInfo() to Client.parseAddrInfoList()
a102bd1

subtests for TestClientStartup
1f7aa04

tzdybal force-pushed the feature/connectiongater branch from dad8975 to 1f7aa04Compare January 12, 2023 13:15

tzdybal self-assigned this Jan 12, 2023

tzdybal requested review from gupadhyaya and tzdybal January 13, 2023 08:21

tzdybal previously approved these changes Jan 13, 2023
View reviewed changes

tzdybal changed the title ~~feature/connectiongater~~feat: add ConnectionGaterJan 13, 2023

gupadhyaya reviewed Jan 13, 2023
View reviewed changes

Comment threadp2p/client_test.go Outdated

gupadhyaya reviewed Jan 13, 2023
View reviewed changes

Comment threadp2p/client.go Outdated

gupadhyaya reviewed Jan 13, 2023
View reviewed changes

Comment threadp2p/client.go Outdated

gupadhyaya reviewed Jan 13, 2023
View reviewed changes

Comment threadp2p/client.go Outdated

nashqueue previously approved these changes Jan 13, 2023
View reviewed changes

Always create connection gater
3d9e055

tzdybal dismissed stale reviews from nashqueue and themself via 3d9e055January 16, 2023 14:03

tzdybal requested review from gupadhyaya, nashqueue and tzdybal January 16, 2023 14:04

fix: properly log peer black/white list
ff454bb

gupadhyaya previously approved these changes Jan 16, 2023
View reviewed changes

nashqueue previously approved these changes Jan 16, 2023
View reviewed changes

tzdybal dismissed stale reviews from nashqueue and gupadhyaya via 107539bJanuary 16, 2023 14:29

tzdybal requested review from gupadhyaya and nashqueue January 16, 2023 14:29

Pass MapStore in all p2p/Client test. Denser formatting
7ac66dc

tzdybal force-pushed the feature/connectiongater branch from 107539b to 7ac66dcCompare January 16, 2023 14:31

gupadhyaya approved these changes Jan 16, 2023
View reviewed changes

nashqueue approved these changes Jan 16, 2023
View reviewed changes

tzdybal approved these changes Jan 17, 2023
View reviewed changes

tzdybal merged commit e5ec6a8 into evstack:mainJan 17, 2023

Uh oh!

Conversation

randomshinichi commented Nov 17, 2022• edited by tzdybal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checklist

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 28, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Manav-Aggarwal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tzdybal commented Dec 20, 2022

Uh oh!

Manav-Aggarwal commented Dec 20, 2022

Uh oh!

tzdybal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nashqueue left a comment

Choose a reason for hiding this comment

Uh oh!

tzdybal commented Jan 16, 2023

Uh oh!

gupadhyaya left a comment

Choose a reason for hiding this comment

Uh oh!

nashqueue left a comment

Choose a reason for hiding this comment

Uh oh!

gupadhyaya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

randomshinichi commented Nov 17, 2022•
edited by tzdybal
Loading

codecov-commenter commented Nov 28, 2022•
edited
Loading