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

Add DropReason and DropPeer structs to pass reason string to logs about Dropping a Neighbor #5720

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Jan 16, 2025

Closes #5714

@jcnelson
Copy link
Member

I like this approach overall. I think it may be worth consolidating DropPeer and DropNeigbor into a struct that only includes a PeerAddress and a DropReason (instead of a bare event_id or a full NeighborKey). I think at each call site where you derive a DropReason, you at least have the means of getting the PeerAddress.

Can this PR also implement a better formatter for PeerAddress, if that's what you end up using? Ideally it'd print the IPv4 or IPv6 address instead of the raw 16 byte address.

@jferrant
Copy link
Collaborator Author

jferrant commented Jan 17, 2025

I like this approach overall. I think it may be worth consolidating DropPeer and DropNeigbor into a struct that only includes a PeerAddress and a DropReason (instead of a bare event_id or a full NeighborKey). I think at each call site where you derive a DropReason, you at least have the means of getting the PeerAddress.

Can this PR also implement a better formatter for PeerAddress, if that's what you end up using? Ideally it'd print the IPv4 or IPv6 address instead of the raw 16 byte address.

Can do! EDIT: just a quick question, does this mean the event_id is not really relevant for matching on a neighbor key? just the neighbor_key.addrbytes, i.e. PeerAddress? Specifically...can multiple neighbor_keys with the same peer_address (even if sep port) match to the same event_id... Cause then it might not be so straightforward cause deregister_peer highly relies on event_id.

For example I see this in deregister_peer which makes me think that there are multiple matching neighbor keys per event id...

        let mut nk_remove: Vec<(NeighborKey, Hash160)> = vec![];
        for (neighbor_key, ev_id) in self.events.iter() {
            if event_id == ev_id {
                let pubkh = self
                    .get_p2p_convo(event_id)
                    .and_then(|convo| convo.get_public_key_hash())
                    .unwrap_or(Hash160([0x00; 20]));
                nk_remove.push((neighbor_key.clone(), pubkh, event_id));
            }
        }

…mplementation of fmt::Display

Signed-off-by: Jacinta Ferrant <[email protected]>
@jcnelson
Copy link
Member

Can do! EDIT: just a quick question, does this mean the event_id is not really relevant for matching on a neighbor key? just the neighbor_key.addrbytes, i.e. PeerAddress? Specifically...can multiple neighbor_keys with the same peer_address (even if sep port) match to the same event_id... Cause then it might not be so straightforward cause deregister_peer highly relies on event_id.

event_id is an opaque handle to the socket connected to the neighbor, which is used by mio to indicate which sockets are ready for read or write. It's not logged at the info level, so logging it for add/drop peers doesn't help us identify the peer being added/dropped.

The system won't register a new socket to a peer that already has a socket open, so there won't be multiple peer_addresses with the same event_id. The code in deregister_peer is defensively written; the local nk_remove list that gets built up in the function body should only ever have at most one element. You could get the NeighorKey for the given the event_id from the nk_remove list.

@aldur aldur added this to the 3.1.0.0.5 milestone Jan 22, 2025
@jferrant
Copy link
Collaborator Author

Can do! EDIT: just a quick question, does this mean the event_id is not really relevant for matching on a neighbor key? just the neighbor_key.addrbytes, i.e. PeerAddress? Specifically...can multiple neighbor_keys with the same peer_address (even if sep port) match to the same event_id... Cause then it might not be so straightforward cause deregister_peer highly relies on event_id.

event_id is an opaque handle to the socket connected to the neighbor, which is used by mio to indicate which sockets are ready for read or write. It's not logged at the info level, so logging it for add/drop peers doesn't help us identify the peer being added/dropped.

The system won't register a new socket to a peer that already has a socket open, so there won't be multiple peer_addresses with the same event_id. The code in deregister_peer is defensively written; the local nk_remove list that gets built up in the function body should only ever have at most one element. You could get the NeighorKey for the given the event_id from the nk_remove list.

Perfect! This was my only concern I think. so I should be able ot make this work.

@jferrant jferrant marked this pull request as ready for review January 22, 2025 19:16
@jferrant jferrant requested a review from a team as a code owner January 22, 2025 19:16
@jferrant jferrant changed the title Add DropReason, DropPeer, and DropNeighbor structs and do first pass … Add DropReason and DropPeer structs to pass reason string to logs about Dropping a Neighbor Jan 22, 2025
@jcnelson
Copy link
Member

A couple things you could add here, potentially:

  • could you log from what subsystem the peer was dropped by?
  • it might be worthwhile to only log peers' acceptance after they've completed a handshake, since we often get peers that connect but never authenticate (e.g. because they're on a different network).

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

Successfully merging this pull request may close these issues.

3 participants