-
Notifications
You must be signed in to change notification settings - Fork 964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: introduce libp2p-allow-block-list
connection management module
#3590
Conversation
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very neat implementation! Minor comments.
#3547 was such a good idea. The amount of times this job has failed on me is insane 😂 |
No sure why this isn't working (mergify tried to merge anyway) but this PR is already using the "new" changelog convention so I'd like to get that approved and merged first. |
I am removing the dependency on #3615. Based on #3386 (comment), I am assuming that @mxinden is in favor of the new changelog syntax so I am using this here and we can iterate on the exact wording in #3615 after that. |
@Mergifyio refresh |
✅ Pull request refreshed |
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
👍 for going ahead with the new changelog format. |
This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏 |
I've resolved 3 merge conflicts on this branch in last hours that are only related to CHANGELOG.md. Even though the semver aspect of https://github.com/thomaseizinger/semverlog isn't as compelling anymore with our strategy of holding back breaking changes, not having to merge changelog files would be nice. |
@thomaseizinger @mxinden After upgrading Zinnia from libp2p 0.51.1 to 0.51.3, I am getting a compiler warning about using a deprecated type However, when I remove this deprecated arm from the match statement, I get a compiler error:
I fixed the problem by adding This is not a big deal, as I am not using peer banning. I just wanted to share my experience as a libp2p consumer in case it's useful. |
Thank you! Yes, that is an unfortunate issue with deprecating enum variants. You can use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions since we do use this feature
/// | ||
/// All active connections to this peer will be closed immediately. | ||
pub fn disallow_peer(&mut self, peer: PeerId) { | ||
self.state.peers.insert(peer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok commenting on the original PR: It looks to me like this should be a remove?
|
||
/// Unblock connections to a given peer. | ||
pub fn unblock_peer(&mut self, peer: PeerId) { | ||
self.state.peers.insert(peer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Correct on both occasions, sorry for the oversight and thanks for the in-depth review! |
Submitted a bugfix here: #3789 |
Description
Currently, banning peers is a first-class feature of
Swarm
. With the new connection management capabilities ofNetworkBehaviour
, we can now implement allow and block lists as a separate module.We introduce a new crate
libp2p-allow-block-list
and deprecateSwarm::ban_peer_id
in favor of that.Related #2824.
Notes & open questions
Change checklist