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 1 commit into
base: main
Choose a base branch
from

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to return outbound and inbound peer addresses in the getpeerinfo response. We currently return only outbound connections as these are the only ones we have access to by the address book.

Solution

  • Add an AddressBookType to the AddressBook structure so we can have 2 books, one for each type without interfering one with the other.
  • Change the returned peers of the rpc to be only the ones that are likely to be connected to us.

There are several ways to do this, i decided for upgrading the current address book structure with the addition of the type because there are multiple fields in the zcashd response that are available in the address book structure we might need to add in the future.

Tests

A basic unit tests was extended to check the inbound address book case. Existing tests were modified to have the outbound type so everything keeps passing. More tests can be created.

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.

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
2 participants