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

fixed kbucket bug :) #117

Merged
merged 2 commits into from
May 16, 2023
Merged

fixed kbucket bug :) #117

merged 2 commits into from
May 16, 2023

Conversation

JamesHertz
Copy link
Contributor

@JamesHertz JamesHertz commented May 10, 2023

I just added a test called TestBugFinder which fails because when adding a new peer to a bucket in which another peer will be evicted to make space for the new one (having the bucketsize = 1), the code just doesn't take into account that the bucket where the new peer info is going to be added may be deleted from the routing table.
I just added lines that take care of it. I know the test is very specific and that the probability of the bucketsize being equals to 1 is very little, but when someone uses the code she/he usually assumes it will work just fine.

@guillaumemichel guillaumemichel self-assigned this May 15, 2023
@guillaumemichel guillaumemichel self-requested a review May 15, 2023 10:35
@guillaumemichel
Copy link
Contributor

Thanks @JamesHertz for spotting this bug! I edited your fix, so that the new peer gets added to the bucket before the replaceable one gets evicted, so that the bucket doesn't get removed.

Does it look good to you?

@JamesHertz
Copy link
Contributor Author

Yes it does.

@guillaumemichel guillaumemichel merged commit 06cb923 into libp2p:master May 16, 2023
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