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

Fix bug limiting MTU to 1280 #881

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Fix bug limiting MTU to 1280 #881

merged 3 commits into from
Jan 30, 2024

Conversation

Juiceman
Copy link
Contributor

@Juiceman Juiceman commented Jan 9, 2024

In UdpSocketHandler.innerCalculateMaxPacketSize() the lesser of the configurable variable node.maxPacketSize and UdpSocketHandler.MAX_ALLOWED_MTU is used, the latter of which was hard coded to 1280.

In UdpSocketHandler.innerCalculateMaxPacketSize() the lesser of the configurable variable node.maxPacketSize and UdpSocketHandler.MAX_ALLOWED_MTU is used, the latter of which was hard coded to 1280.
@ArneBab
Copy link
Contributor

ArneBab commented Jan 26, 2024

@Juiceman Thank you for your PR!

Is this change tested on different networks? Is the reasoning for 1280 no longer valid?

@Juiceman
Copy link
Contributor Author

Juiceman commented Jan 28, 2024 via email

@Bombe
Copy link
Contributor

Bombe commented Jan 29, 2024

Unfortunately, this value is used for both IPv6 and IPv4. The 'safe' 1280 is only relevant to IPv6 (and cloudflare has an interesting write up https://blog.cloudflare.com/increasing-ipv6-mtu) We set the config menu default to 1280, the config menu allows values up to 1492 but would the node would never use a value larger than 1280. The user is never informed and the config still shows their selected value.

Ah, alright, so it only brings the displayed value in line with the one that’s used? That’s fine, then. 🙂

@ArneBab
Copy link
Contributor

ArneBab commented Jan 30, 2024

Approved by @Bombe ⇒ Merged. Thank you both!

@ArneBab ArneBab merged commit 3cf2617 into hyphanet:next Jan 30, 2024
1 check passed
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.

3 participants