-
Notifications
You must be signed in to change notification settings - Fork 26
introduce max message size in webrtc config #442
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
base: master
Are you sure you want to change the base?
introduce max message size in webrtc config #442
Conversation
…mi/buffer-size-webrtc-config # Conflicts: # src/transport/webrtc/opening.rs
src/transport/webrtc/config.rs
Outdated
| .parse() | ||
| .expect("valid multiaddress")], | ||
| datagram_buffer_size: 2048, | ||
| max_message_size: 512 * 1024, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where this constant is coming from? What is libp2p way of handling this?
What will happen in case of high throughput transfer — shouldn't we be able to parse the payload split over multiple max_message_size buffers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should make this optional, not entirely sure if this interferes with @timwu20 upcoming work 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest hardcoding the constant from libp2p spec, as discussed in #352.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oki doki makes sense 🙏 Let's hardcode it instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, @lexnv should i remove the max_message_size entirely from the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey yep, let's pick: #352 (comment) 16kIB for this
closes #352