feat(swarm): Add deny_list behaviour
#3156
Conversation
| // Check if peer is banned. | ||
| if self.banned_peers.contains(&peer_id) { | ||
| let error = DialError::Banned; | ||
| #[allow(deprecated)] | ||
| self.behaviour.inject_dial_failure(Some(peer_id), &error); | ||
| return Err(error); | ||
| } |
There was a problem hiding this comment.
We lose this early check here, i.e. we will initiate a dial and then immediately drop the connection instead of aborting the connection early. I guess that is the price for implementing this kind of functionality in a generic way.
| impl Behaviour { | ||
| pub fn add_peer(&mut self, peer: PeerId) { | ||
| self.list.insert(peer); | ||
| self.to_disconnect.push_back(peer); |
There was a problem hiding this comment.
This should also lead to waking up the swarm. Perhaps using an async channel is the easiest and most readable solution? (instead of rebuilding such functionality with a VecDeque and an Option<Waker>)
There was a problem hiding this comment.
@mxinden was talking about building a WakingVec abstraction because we are coming across this very often actually.
There was a problem hiding this comment.
I had a quick search on crates.io but couldn't find anything that does what we need. It might be possible that I used the wrong keywords!
There was a problem hiding this comment.
I made a PoC here: #3160
There was a problem hiding this comment.
While that may have independent merit (not yet reviewed), I meant simply using async-channel instead of the VecDeque.
There was a problem hiding this comment.
While that may have independent merit (not yet reviewed), I meant simply using
async-channelinstead of the VecDeque.
I always find it confusing when the same data structure holds both ends of a channel.
But I agree with you that it would solve the problem.
thomaseizinger
force-pushed
the
2824-remove-into-connection-handler
branch
from
7369cc1
to
dcb4f96
Compare
| pub struct Behaviour { | ||
| list: HashSet<PeerId>, | ||
| to_disconnect: VecDeque<PeerId>, | ||
| } | ||
|
|
||
| impl Behaviour { | ||
| pub fn add_peer(&mut self, peer: PeerId) { | ||
| self.list.insert(peer); |
There was a problem hiding this comment.
| pub struct Behaviour { | |
| list: HashSet<PeerId>, | |
| to_disconnect: VecDeque<PeerId>, | |
| } | |
| impl Behaviour { | |
| pub fn add_peer(&mut self, peer: PeerId) { | |
| self.list.insert(peer); | |
| pub struct Behaviour<T = ()> { | |
| list: HashMap<PeerId, T>, | |
| to_disconnect: VecDeque<PeerId>, | |
| } | |
| impl Behaviour<()> { | |
| pub fn add_peer(&mut self, peer: PeerId) { | |
| self.list.insert(peer, ()); | |
| self.to_disconnect.push_back(peer); | |
| } | |
| } | |
| impl<T> Behaviour<T> { | |
| pub fn add_peer_because(&mut self, peer: PeerId, reason: T) { | |
| self.list.insert(peer, reason); |
We could allow an optional reason parameter that we'd then also include in the Denied struct. Might be useful if there are multiple reasons why a peer can be banned.
If we'd additionally expose what peers are currently banned (and for what reason) it would allow a more sophisticated management of banned peers. E.g. if a peer was only banned because an Application-Layer limit was reached.
But not sure if it's worth the extra complexity. Could wait until there is actually a use-case for this.
| use std::collections::{HashSet, VecDeque}; | ||
| use std::task::{Context, Poll}; | ||
|
|
||
| #[derive(Default)] |
There was a problem hiding this comment.
| #[derive(Default)] | |
| #[derive(Default, Debug, Clone)] |
|
Closing for now as I am working on a new version of the connection management idea here: #3254 |
Description
Notes
To actually land this, we would obviously do it in a backwards-compatible way. This PR is just meant to serve as a showcase to see what it would look like to implement various capabilities on top of the new, generic connection management.
Links to any relevant issues
Open Questions
Change checklist