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

Allow setting bucket size to 16 or higher #5964

Merged
merged 3 commits into from
Jul 25, 2023
Merged

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Jul 24, 2023

  • It does not look like our discv4 implementation implement a complete walk like geth, meaning we are limited by the nodes that are available from the nodes that are in our buckets, which does not get replaced (it can happen, but its very rare that they don't reply a pong). And because of how the distance calculation works, only bucket 245ish and above is used. So it usually keep getting largely the same set of neighbour. Eventually it'll get enough peer to fill the 10000 default candidate list, but it take some time.
  • The only way to increase the range of node that it discover without a significant rewrite is to increase the bucket size, which is what this PR do, and also expose the option to increase further it in case of smaller network.

Changes

  • Set back discovery bucket size to 16, which is what the spec said.
  • Set limit of neighbour msg to 12, which prevent it from becoming too big in case of ipv6 address.
  • Allow setting discovery config from CLI.

Types of changes

What types of changes does your code introduce?

  • Optimization

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Maybe

Improved peer discovery

@LukaszRozmej
Copy link
Member

LukaszRozmej commented Jul 25, 2023

Maybe a significant rewrite would be desired? This code never got the love.

@asdacap
Copy link
Contributor Author

asdacap commented Jul 25, 2023

Maybe, but @flcl42 is already working on discv5, so this is getting deprecated anyway. WDYT?

@asdacap asdacap marked this pull request as ready for review July 25, 2023 11:09
@LukaszRozmej
Copy link
Member

Maybe, but @flcl42 is already working on discv5, so this is getting deprecated anyway. WDYT?

Not sure if it will

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Is there any negative effect on memory or performance?

@asdacap
Copy link
Contributor Author

asdacap commented Jul 25, 2023

Hmm, I guess it would have more entry in the bucket. But assuming only bucket 245-255, that would only be 160 record. Its very low. Too low even.

@LukaszRozmej
Copy link
Member

Hmm, I guess it would have more entry in the bucket. But assuming only bucket 245-255, that would only be 160 record. Its very low. Too low even.

Can you check some profiling?

@asdacap
Copy link
Contributor Author

asdacap commented Jul 25, 2023

Screenshot_2023-07-25_22-43-49

Run is 16, 12, 16, 12, 16, 12. Not correlation. Nethermind hooked to shutdown on 8000 peer in peer pool. There are other modification to make it get 8000 peer in less than 4 minute. It normally take 30 minute maybe.

@asdacap asdacap merged commit 3422b1f into master Jul 25, 2023
61 checks passed
@asdacap asdacap deleted the perf/adjust-bucket-size branch July 25, 2023 14:46
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.

None yet

3 participants