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(request-response): don't keep duplicate addresses #4700

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

b-zee
Copy link
Contributor

@b-zee b-zee commented Oct 20, 2023

Description

Resolves: #4699.

Notes & open questions

In kad, there is a special Addresses struct that might be worth reusing for this functionality.

I will make further CHANGELOG edits etc if this fix is thought to be sensible.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@joshuef
Copy link
Contributor

joshuef commented Oct 20, 2023

Nice one. This looks to have been a wee mem leak for us. We've been doing add_server on every Identify event and been leaking decent chunks of memory over time. 💪

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

How about using a HashSet instead?

modified   protocols/request-response/src/lib.rs
@@ -337,7 +337,7 @@ where
     /// reachable addresses, if any.
     connected: HashMap<PeerId, SmallVec<[Connection; 2]>>,
     /// Externally managed addresses via `add_address` and `remove_address`.
-    addresses: HashMap<PeerId, SmallVec<[Multiaddr; 6]>>,
+    addresses: HashMap<PeerId, HashSet<Multiaddr>>,
     /// Requests that have not yet been sent and are waiting for a connection
     /// to be established.
     pending_outbound_requests: HashMap<PeerId, SmallVec<[RequestProtocol<TCodec>; 10]>>,

@thomaseizinger
Copy link
Contributor

How about using a HashSet instead?

modified   protocols/request-response/src/lib.rs
@@ -337,7 +337,7 @@ where
     /// reachable addresses, if any.
     connected: HashMap<PeerId, SmallVec<[Connection; 2]>>,
     /// Externally managed addresses via `add_address` and `remove_address`.
-    addresses: HashMap<PeerId, SmallVec<[Multiaddr; 6]>>,
+    addresses: HashMap<PeerId, HashSet<Multiaddr>>,
     /// Requests that have not yet been sent and are waiting for a connection
     /// to be established.
     pending_outbound_requests: HashMap<PeerId, SmallVec<[RequestProtocol<TCodec>; 10]>>,

+1 for using a HashSet. Long-term, we should solve this by adding a PeerStore behaviour: #4103.

b-zee added 2 commits October 23, 2023 14:39
This prevents us from adding duplicates into the list of peer addresses we have.
@thomaseizinger thomaseizinger changed the title fix: deduplicate autonat add_server addresses fix(request-response): don't keep duplicate addresses Oct 23, 2023
@thomaseizinger
Copy link
Contributor

Thanks! Can you address the clippy failure?

@b-zee
Copy link
Contributor Author

b-zee commented Oct 24, 2023

Fixed the clippy lint.

Is v0.53.0 set to be released soon? Or, possibly, is there a patch release that this could part of soon? Thanks for all the work!

@thomaseizinger
Copy link
Contributor

Fixed the clippy lint.

Is v0.53.0 set to be released soon? Or, possibly, is there a patch release that this could part of soon? Thanks for all the work!

If you backport it to the v0.52 branch, I can cut a patch release :)

@mergify mergify bot merged commit 7305234 into libp2p:master Oct 25, 2023
71 checks passed
@b-zee b-zee deleted the fix-autonat-add-server-deduplicate branch October 26, 2023 13:58
mxinden pushed a commit that referenced this pull request Oct 26, 2023
Backport of #4700 on `v0.52` branch.

- fix: deduplicate autonat add_server addresses
- keep peers in HashSet instead of SmallVec
- change (&HashSet)::into_iter to equivalent iter

Pull-Request: #4724.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutoNAT servers added are not deduplicated
4 participants