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.

Sign up for GitHub

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

Jump to bottom

basichost: ensure no duplicates in Addrs output #2980

Merged
merged 2 commits into from
Oct 3, 2024
Merged

basichost: ensure no duplicates in Addrs output #2980

merged 2 commits into from
Oct 3, 2024

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Sep 26, 2024

fixes: #2972

@Wondertan
Copy link
Contributor

I can help testing this on my cluster once the fix is final

@sukunrt
Copy link
Member Author

sukunrt commented Sep 30, 2024

@Wondertan, The fix is final. Can you check this?

@Wondertan
Copy link
Contributor

Looks good!
image

@Wondertan
Copy link
Contributor

Might be worth adding a unit test

@sukunrt sukunrt requested a review from MarcoPolo September 30, 2024 16:18
MarcoPolo
MarcoPolo approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good

p2p/host/basic/basic_host.go Show resolved Hide resolved
@sukunrt sukunrt merged commit c255632 into master Oct 3, 2024
11 checks passed
MarcoPolo pushed a commit that referenced this pull request Oct 4, 2024
@sukunrt @MarcoPolo
basichost: ensure no duplicates in Addrs output (#2980)
4e2cae4
sukunrt added a commit that referenced this pull request Oct 4, 2024
@sukunrt
basichost: ensure no duplicates in Addrs output (#2980)
38cbcd0
@Wondertan Wondertan mentioned this pull request Oct 4, 2024
Merged
Wondertan added a commit to celestiaorg/celestia-node that referenced this pull request Oct 4, 2024
@Wondertan
chore: bump p2p deps (#3811)
76ac684 
Bumps https://github.com/libp2p/go-libp2p/releases/tag/v0.36.5 and https://github.com/ipfs/boxo/releases/tag/v0.24.0

Both brings changes we need. In particular libp2p/go-libp2p#2980 and ipfs/boxo#672
@dimartiro
Copy link

Is there any chance that this PR changed the Addrs order when they are returned? I'm experiment an issue in our tests because of that

@dimartiro dimartiro mentioned this pull request Jan 16, 2025
Merged
@MarcoPolo
Copy link
Collaborator

@dimartiro Yes. Removing duplicates without allocating means that you have to change the order of the returned Addrs (https://github.com/multiformats/go-multiaddr/blob/d19cf5da7dc590c829b9a4b329341ea8847d4d2a/multiaddr.go#L253).

If you don't have duplicates, it shouldn't affect the order.

@dimartiro
Copy link

@MarcoPolo I don't have duplicates but I think that the fact of using a map is breaking the order of the current slice.
I already fixed my tests, but just want to raise this discovery to you guys 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoPolo MarcoPolo MarcoPolo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BasicHost submits EvtLocalAddressesUpdated but addresses were not changed
4 participants
@sukunrt @Wondertan @dimartiro @MarcoPolo