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

Do not add addresses if they are already there in peerdata #325

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

moshemalawach
Copy link
Contributor

In peerdata, if you call add_addrs, the function doesn't check if the addresses are already there.
This can result in the list growing huge if we reconnect often to the same hosts.

An alternative solution to the one proposed would be to change self.addrs to a set and not a list. But that would need some work in places where we use addrs[0] (like in swarm).

Copy link
Contributor

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

Great catch and solve! Would you mind adding some tests for this PR for the case we "add duplicated addrs"? Except for the tests, this looks good to me.

Comment on lines +29 to +31
for addr in addrs:
if addr not in self.addrs:
self.addrs.append(addr)
Copy link
Contributor

@mhchia mhchia Oct 14, 2019

Choose a reason for hiding this comment

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

What I'm thinking is probably we should change addrs from List[Multiaddr] to Set[Multiaddr] to reduce time complexity.
However, this solution works for me so far. 👍


Edited: removed the thought on replacing List with Set since it is already mentioned in the PR.

@ralexstokes
Copy link
Contributor

i'd suggest we use a set for this use case.

do you want to update the PR @moshemalawach ?

if not, i'll move this to a tracking issue and we can go ahead and merge this in.

@ralexstokes
Copy link
Contributor

and +1 on a test for this scenario :)

moshemalawach and others added 8 commits November 12, 2019 15:54
pyaleph depends on this version of `py-libp2p` and on `py-substrate-interface`.

`py-substrate-interface` requires `base58>=2.0.1`, which conflicts with the dependency on `base58>=1.0.3,<2.0.0`
in this setup.py.
Fix dependency issue on base58 with pyaleph
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

None yet

5 participants