Skip to content
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

Merged
merged 18 commits into from
Mar 21, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Mar 11, 2023

Description

Currently, banning peers is a first-class feature of Swarm. With the new connection management capabilities of NetworkBehaviour, we can now implement allow and block lists as a separate module.

We introduce a new crate libp2p-allow-block-list and deprecate Swarm::ban_peer_id in favor of that.

Related #2824.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@mergify
Copy link
Contributor

mergify bot commented Mar 12, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

Copy link
Member

@mxinden mxinden left a 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.

libp2p/Cargo.toml Show resolved Hide resolved
misc/allow-block-list/src/lib.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

#3547 was such a good idea. The amount of times this job has failed on me is insane 😂

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Mar 18, 2023

Depends-On: #3615.

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.

@thomaseizinger
Copy link
Contributor Author

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.

@thomaseizinger
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2023

refresh

✅ Pull request refreshed

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mxinden
Copy link
Member

mxinden commented Mar 21, 2023

👍 for going ahead with the new changelog format.

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Mar 21, 2023

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.

@mergify mergify bot merged commit f641870 into master Mar 21, 2023
@mergify mergify bot deleted the 2824-blocklist branch March 21, 2023 20:58
@bajtos
Copy link

bajtos commented Apr 13, 2023

@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 SwarmEvent::BannedPeer, which is expected.

However, when I remove this deprecated arm from the match statement, I get a compiler error:

non-exhaustive patterns: `SwarmEvent::BannedPeer { .. }` not covered

I fixed the problem by adding #[allow(deprecated)] to the BannedPeer arm, but it feels like it's defeating the point of deprecating this enum variant.

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.

@thomaseizinger
Copy link
Contributor Author

Thank you!

Yes, that is an unfortunate issue with deprecating enum variants. You can use a _ match arm to silence the deprecation and not receive a compile error once we remove it!

Copy link
Contributor

@divagant-martian divagant-martian left a 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);
Copy link
Contributor

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@thomaseizinger
Copy link
Contributor Author

A couple of questions since we do use this feature

Correct on both occasions, sorry for the oversight and thanks for the in-depth review!

@thomaseizinger
Copy link
Contributor Author

Submitted a bugfix here: #3789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants