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

fix(libp2p): sort addresses to dial as public, then relay #2031

Merged
merged 5 commits into from
Sep 8, 2023

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Sep 8, 2023

Adds a new default address sorter that sorts by:

  1. public addresses
  2. public relay addresess
  3. private addresses
  4. private relay addresses

Where they are equal, certified addresses take priority, otherwise the sort order is stable.

Refs: #2010

Adds a new default address sorter that sorts by:

1. public addresses
2. public relay addresess
3. private addresses
4. private relay addresses

Where they are equal, certified addresses take priority, otherwise
the sort order is stable.
@achingbrain achingbrain requested a review from a team as a code owner September 8, 2023 10:23
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Looks good, I would just like to understand the rationale for prioritizing a certified private address over a public address, wouldn't it be less likely that the private address is routable?

packages/utils/test/address-sort.spec.ts Show resolved Hide resolved
@achingbrain
Copy link
Member Author

I would just like to understand the rationale for prioritizing a certified private address over a public address, wouldn't it be less likely that the private address is routable?

It doesn't - take a look at the relevant test - public addresses are prioritised over private addresses, and certified addresses are prioritised over non-certified addresses but certified private addresses are not prioritised over public addresses.

@maschad
Copy link
Member

maschad commented Sep 8, 2023

It doesn't - take a look at the relevant test - public addresses are prioritised over private addresses, and certified addresses are prioritised over non-certified addresses but certified private addresses are not prioritised over public addresses.

What about this test? It seems to contradict that.

@achingbrain
Copy link
Member Author

What about this test? It seems to contradict that.

That test only tests the certifiedAddressesFirst comparator, the defaultAddressSort comparator combines the publicAddressesFirst, certifiedAddressesFirst and circuitRelayAddressesLast comparators.

@maschad
Copy link
Member

maschad commented Sep 8, 2023

That's true, but I suppose if a consumer is using the certifiedAddressesFirst utility they wouldn't expect that order. Alternatively they could pass in their own custom address sorter.

@achingbrain achingbrain merged commit 5294f14 into master Sep 8, 2023
17 checks passed
@achingbrain achingbrain deleted the fix/sort-relay-addresses-last branch September 8, 2023 16:22
This was referenced Jan 18, 2024
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.

2 participants