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

Make public addresses go first in authority discovery DHT records #3757

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

dmitry-markin
Copy link
Contributor

Make sure explicitly set by the operator public addresses go first in the authority discovery DHT records.

Also update Discovery behavior to eliminate duplicates in the returned addresses.

This PR should improve situation with #3519.

Obsoletes #3657.

@dmitry-markin dmitry-markin added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Mar 20, 2024
Some(address)
}
}))
.filter(move |address| {
Copy link
Contributor Author

@dmitry-markin dmitry-markin Mar 20, 2024

Choose a reason for hiding this comment

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

Not sure, may be we should always publish public addresses provided by user even if they are non-global.

Copy link
Contributor

Choose a reason for hiding this comment

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

dq: I see we have a public_non_global_ips flag which allows us to publish non-global IP addresse.
When would we want to publish non-global-ips? There is a high chance that a non-global ip could not be reached from the other peers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about a testnet in a private network case. But we can also use public_non_global_ips flag for this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah private network. When you run it locally on your computer, you also need authority discovery to work ;)

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nice one 👍

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Ty!

One thing I'm not sure if we are already doing it, could we ensure that external_addresses contains the listen addresses? Maybe we could even have an extra function for getting the listen addresses to have in the end, [public addresses, listen addresses, external addresses].

Some(address)
}
}))
.filter(move |address| {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah private network. When you run it locally on your computer, you also need authority discovery to work ;)

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Mar 21, 2024

One thing I'm not sure if we are already doing it, could we ensure that external_addresses contains the listen addresses?

external_addresses do not contain listen addresses, and I thought it's a feature and not a bug.

Maybe we could even have an extra function for getting the listen addresses to have in the end, [public addresses, listen addresses, external addresses].

Do you mean [public addresses, external addresses, listen addresses]?

cc @bkchr

@bkchr
Copy link
Member

bkchr commented Mar 22, 2024

external_addresses do not contain listen addresses, and I thought it's a feature and not a bug.

IDK :D I mean this was more some proposal by me. Also should not block this pr BTW.

Do you mean [public addresses, external addresses, listen addresses]?

Listen addresses are more "trusted" than external addresses as you can just query your local network devices to get their ip addresses etc. For validators which should be exposed to the internet, the listen addresses should also give you the correct ip addresses directly. If the operator is giving public addresses, I assume that listen and public will probably be the same.

@dmitry-markin dmitry-markin added this pull request to the merge queue Mar 22, 2024
@dmitry-markin
Copy link
Contributor Author

external_addresses do not contain listen addresses, and I thought it's a feature and not a bug.

IDK :D I mean this was more some proposal by me. Also should not block this pr BTW.

Do you mean [public addresses, external addresses, listen addresses]?

Listen addresses are more "trusted" than external addresses as you can just query your local network devices to get their ip addresses etc. For validators which should be exposed to the internet, the listen addresses should also give you the correct ip addresses directly. If the operator is giving public addresses, I assume that listen and public will probably be the same.

We can address it in a follow-up PR. Personally, I would refrain from adding listen addresses as they contain all the interfaces (including 127.0.0.1 and private networks), and it should be enough to have just one valid Identify response from a remote peer to learn a valid external address. We should take into account address translation done by libp2p (it assign a listen address port to discovered external addresses) to make an informed decision though.

Merged via the queue into master with commit 9d2963c Mar 22, 2024
143 of 146 checks passed
@dmitry-markin dmitry-markin deleted the dm-prioritize-public-addresses branch March 22, 2024 12:39
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Mar 24, 2024
…ritytech#3757)

Make sure explicitly set by the operator public addresses go first in
the authority discovery DHT records.

Also update `Discovery` behavior to eliminate duplicates in the returned
addresses.

This PR should improve situation with
paritytech#3519.

Obsoletes paritytech#3657.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants