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 external_addr to config (network section) #2035

Closed
wants to merge 4 commits into from

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Apr 19, 2021

Motivation

Allow zebrad advertise address specified by user.

Solution

Add external_addr to config (network section).

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Related Issues

#1890

Follow Up Work

Allow set only IP address in listen_addr (default port from Network enum).

// TODO: use the right port if the port is unspecified
// split the address and port configs?

@teor2345
Copy link
Contributor

  • In the diagram, I see that listen port (address) should be in AddressBook but I can not find code where listen_addr added to AddressBook. Can you help with this?

I think that diagram is a bit confusing, maybe we should say "Inbound Peer Connections to the zebrad listener port".

@teor2345
Copy link
Contributor

  • In the diagram, I see that listen port (address) should be in AddressBook but I can not find code where listen_addr added to AddressBook. Can you help with this?

I think that diagram is a bit confusing, maybe we should say "Inbound Peer Connections to the zebrad listener port".

#2036 might help here, I updated the docs.

@dconnolly
Copy link
Contributor

Looks like rustfmt doesn't like the space on one line, causing rustfmt job to fail

address_from: (our_services, config.listen_addr),
address_from: (
our_services,
config.external_addr.unwrap_or(config.listen_addr),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you turn this into an external_addr() method on Config?
(And make Config.external_addr private or pub(crate).)

That way, we'll always get the external address the right way.

(We'll also want to use the external address in #1892 and similar tickets.)

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think we'll want to have separate IPv4 and IPv6 external addresses, and choose the external address based on the connection's IP version. (See #1890.)

We'll also want to:

  • add the external address for the connection's IP version to Zebra's Version messages
  • add both external addresses to Zebra's Peers responses
  • fallback to the internal address if there is no external address

but that depends on #2033 merging:
https://github.com/ZcashFoundation/zebra/pull/2033/files#diff-3f14971a74a109913a4809bd1a77f4e0145e1edfa72aaded8f1fdcb2cc45f4f1R243-R244

As part of that change, we'll want to make the AddressBook ignore any null addresses (0.0.0.0 or [::]).

@teor2345

This comment has been minimized.

@teor2345 teor2345 added A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement I-privacy Zebra discloses private information P-Low labels Apr 20, 2021
@teor2345
Copy link
Contributor

I'm going to mark this PR as draft until the Zebra team decides which changes we actually need here.

@teor2345 teor2345 marked this pull request as draft October 21, 2021 23:22
@teor2345
Copy link
Contributor

We might merge this as part of ticket #3117 when we get to it.

@teor2345 teor2345 closed this Dec 28, 2021
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-rust Area: Updates to Rust code C-enhancement Category: This is an improvement I-privacy Zebra discloses private information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants