Skip to content

Expose PeerId in connection-limits crate #6025

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

Closed
DougAnderson444 opened this issue May 20, 2025 · 5 comments · Fixed by #6032
Closed

Expose PeerId in connection-limits crate #6025

DougAnderson444 opened this issue May 20, 2025 · 5 comments · Fixed by #6032

Comments

@DougAnderson444
Copy link
Contributor

Description

Expose the PeerId of the peer who exceeded the limit, and also expose Kind as pub so we can use that PeerId

Motivation

We'd like insight into which Peer is exceeding the connection limit

Current Implementation

Opaquely states the limit was exceeded, but not by who.

Are you planning to do it yourself in a pull request?

Yes

@DougAnderson444
Copy link
Contributor Author

EstablishedPerPeer,

@elenaf9
Copy link
Member

elenaf9 commented May 22, 2025

Would it make sense to, instead of including the PeerId only in this error variant, to have in the general SwarmEvent::IncomingConnectionError as Option<PeerId>? I could imagine that this info is also useful in other scenarios.

We also already added it with #4818 to the matching ListenFailure event that is sent to the behaviors in this situation:

match self.behaviour.handle_established_inbound_connection(
id,
peer_id,
&local_addr,
&send_back_addr,
) {
Ok(handler) => handler,
Err(cause) => {
let listen_error = ListenError::Denied { cause };
self.behaviour.on_swarm_event(FromSwarm::ListenFailure(
ListenFailure {
local_addr: &local_addr,
send_back_addr: &send_back_addr,
error: &listen_error,
connection_id: id,
peer_id: Some(peer_id),
},
));
self.pending_swarm_events.push_back(
SwarmEvent::IncomingConnectionError {
connection_id: id,
send_back_addr,
local_addr,
error: listen_error,
},
);
return;
}

@DougAnderson444
Copy link
Contributor Author

I'm always a big fan of exposing as much troubleshooting data as possible, so I agree, also a great idea @elenaf9!

Does it make sense to have that as a separate pull request from #6027 ?

@elenaf9
Copy link
Member

elenaf9 commented May 23, 2025

Does it make sense to have that as a separate pull request from #6027?

I was wondering if #6027 is still needed if we add the peer-id in IncomingConnectionError. I don't have anything major against #6027, but since downcasting is required to extract the peer-id from the boxed error I am wondering how much it will actually be used.

@DougAnderson444
Copy link
Contributor Author

Ah I see what you mean, I've opened #6032 as a result of this discussion. Yes you're right, if I get the PeerId early enough we can gain the insight from there.

@mergify mergify bot closed this as completed in #6032 May 27, 2025
mergify bot pushed a commit that referenced this issue May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants