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

rpc(getpeerinfo): Add inbound peers to method response #9214

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Feb 7, 2025

Motivation

We want to return outbound and inbound peer addresses in the getpeerinfo response. We currently return only outbound connections as these were the only ones we have access to by the address book. After #9201 was merged, we have inbound connections available in the MetaAddr struct so we can use that to return peers of both connection types in the RPC response.

Close #7893

Solution

  • Use is_inbound() to know if a peer is inbound/outbound.
  • Add a currently_live_peers() method to the AddressBookPeers trait so we can return peers that are likely connected to us now instead of the ones that were responding before.

Tests

Existing getpeerinfo tests were updated to reflect the change.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@oxarbitrage oxarbitrage added A-network Area: Network protocol updates or fixes A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡ labels Feb 7, 2025
@oxarbitrage oxarbitrage self-assigned this Feb 7, 2025
@oxarbitrage oxarbitrage requested review from a team as code owners February 7, 2025 00:00
@oxarbitrage oxarbitrage requested review from conradoplg and removed request for a team February 7, 2025 00:00
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks like it should work, but it seems unnecessary to add another address book.

I made an alternative change in a9014f0 of #9201 if that looks acceptable.

Related: @mpguerra It would be nice to eventually refactor the address book into a service, it may be easier to do that ahead of adding fields to the getpeerinfo response.

@oxarbitrage
Copy link
Contributor Author

This looks like it should work, but it seems unnecessary to add another address book.

I made an alternative change in a9014f0 of #9201 if that looks acceptable.

I don't think that is correct. For the getpeerinfo RPC method, when displaying an inbound connection it looks to me that the caller will expect to see the ephemeral port of the peer and not the default p2p port.

@oxarbitrage
Copy link
Contributor Author

update recently_live_peers() to check that the last connection state is responded and that the peer has responded in the last HEARTBEAT_INTERVAL + 50ms. (recently_live_peers() is only used in getpeerinfo/getinfo, we should update that instead of adding a new method)

I agree that adding a new method is not really needed so that was removed as part of d90e639

While it is almost true that recently_live_peers() is only used in the getpeerinfo RPC, there is a marginal use of it in several metrics and tracing related code that i will prefer to leave as it is for now.

In regards to calculating if the peer is live by the use of HEARTBEAT_INTERVAL, it seems unnecessary as we can just use has_connection_recently_responded() which is what the now removed currently_live_peers() was doing. https://github.com/ZcashFoundation/zebra/blob/main/zebra-network/src/meta_addr.rs#L499

So, what i did in d90e639 for the getpeerinfo RPC call was to use the recently_live_peers() method as it was before and then filter the results by has_connection_recently_responded() which should give us peers that are more likely to be connected now.

Let me know how do you see it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network Area: Network protocol updates or fixes A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: getpeerinfo RPC should show inbound connections as well as outbound
3 participants