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

Set max network number to 8000 instead of 8192 #803

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

Conversation

Andrew-Dickinson
Copy link
Member

@Andrew-Dickinson Andrew-Dickinson commented Jan 7, 2025

The docs and configen say the max number is 8000 and we should be consistent with that

Also enforces minimum and maximum NN at node creation time

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.88%. Comparing base (8feaa16) to head (f8fa204).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #803   +/-   ##
=======================================
  Coverage   94.88%   94.88%           
=======================================
  Files          89       89           
  Lines        3809     3812    +3     
=======================================
+ Hits         3614     3617    +3     
  Misses        195      195           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@WillNilges WillNilges left a comment

Choose a reason for hiding this comment

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

If MeshDB can support it, why not update docs/configgen/etc and take advantage of the extra NNs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why were all these higher than 8192 in the first place? Were we not enforcing them before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. The PR is too old and I don't remember lol

@WillNilges
Copy link
Collaborator

WillNilges commented Jan 16, 2025

Talked about this in slack. Things will get kind of messy, and it's not worth trying to get an extra 192 addresses in this PR.

Well for one thing there are devices using 8xxx IPs without central coordination

The trouble with changing the published convention to match the behavior rather than the other way around is it’s hard to anticipate the ways in which people rely on 8k being the limit

We will certainly in the future need to resolve this issue, but probably not for years

And hopefully we get far enough ahead of it that 192 IPs won’t make much of a difference

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