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

Consider changing identify cache default to be non-zero #2933

Closed
thomaseizinger opened this issue Sep 23, 2022 · 5 comments
Closed

Consider changing identify cache default to be non-zero #2933

thomaseizinger opened this issue Sep 23, 2022 · 5 comments
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted

Comments

@thomaseizinger
Copy link
Contributor

Description

The rust-libp2p identify implementation has a cache that remembers the reported addresses from the last X connected peers even once they get disconnected. By default, X is zero.

Motivation

Most users probably don't know about this cache and thus, do not benefit from it. If we make the default value 100, the maximum memory footprint will still be < 500kb (1 identify message has max 4096 bytes, that * 100 is still only 400kb and there is more than just addresses in the message).

Current Implementation

Are you planning to do it yourself in a pull request?

Maybe.
@thomaseizinger thomaseizinger mentioned this issue Sep 23, 2022
Merged
3 tasks
@mxinden
Copy link
Member

mxinden commented Sep 26, 2022

I am in favor of this.

@thomaseizinger thomaseizinger added difficulty:easy help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Sep 27, 2022
efarg added a commit to efarg/rust-libp2p that referenced this issue Oct 3, 2022
@efarg
libp2p#2933 Change identify protocol's default cache size to 100
32bdf0c
This was referenced Oct 3, 2022
Closed
Merged
@nloadholtes nloadholtes mentioned this issue Oct 19, 2022
Closed
4 tasks
@nloadholtes
Copy link
Contributor

Hi, I've got a draft PR, but it feels too.... simple. 😁 Did I miss something big in the PR, or is that really all there is?

@thomaseizinger
Copy link
Contributor Author

Hi, I've got a draft PR, but it feels too.... simple. 😁 Did I miss something big in the PR, or is that really all there is?

We already have #2995 open for this 🙈

Sorry, should have maybe made that more obvious in the issue here!

It is quite a simple change yeah, we are just separating it so bisecting is easier in case something breaks!

@nloadholtes
Copy link
Contributor

No prob! I'm just happy my change was about 90% there 😂 I'll close out my PR.

@jxs
Copy link
Member

jxs commented Nov 25, 2022

fixed with #2995

@jxs jxs closed this as completed Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nloadholtes @jxs @thomaseizinger @mxinden