Add explicit limits to notifications sizes and adjust yamux buffer size #7925
Conversation
tomaka
added
A0-please_review
| iter::once(default_max) | ||
| .chain(requests_max).chain(responses_max).chain(notifs_max) | ||
| .max().expect("iterator known to always yield at least one element; qed") | ||
| .saturating_add(10) |
There was a problem hiding this comment.
Do I understand correctly, that the addition here accounts for the additional bytes needed for the length delimiter added via UviBytes? If so, why 10?
There was a problem hiding this comment.
Indeed.
10 is the maximum number of bytes required to encode a variable-length u64: https://docs.rs/unsigned-varint/0.6.0/unsigned_varint/encode/fn.u64_buffer.html
I'm taking the assumption that we'll never send a message larger than u64::max_size().
There was a problem hiding this comment.
👍 Would you mind documenting that?
| @@ -148,8 +153,11 @@ where TSubstream: AsyncRead + AsyncWrite + Unpin + Send + 'static, | |||
| socket.read_exact(&mut initial_message).await?; | |||
| } | |||
|
|
|||
| let mut codec = UviBytes::default(); | |||
| codec.set_max_len(usize::try_from(self.max_notification_size).unwrap_or(usize::max_value())); | |||
There was a problem hiding this comment.
Why do we need that try_from? Why not simply make max_notification_size an usize to begin with?
There was a problem hiding this comment.
The size of something that is transmitted on the network should never be a usize, as we want this size limit to be the same for all participants.
However a buffer length, however, is correctly a usize.
|
|
||
| // A "default" max is added to cover all the other protocols: ping, identify, | ||
| // kademlia. | ||
| let default_max = 1024 * 1024; |
There was a problem hiding this comment.
I am seeing this number (1024 * 1024) a lot, maybe we can define it somewhere as default value? Either just a constant or maybe even make a NotificationSize newtype which has a Default instance?
There was a problem hiding this comment.
It has a different meaning every time, and each 1024 * 1024 should ideally be tweaked individually. Here it's the maximum message size for identify/ping/kademlia. In the grandpa crate, it's the maximum message size of grandpa.
| iter::once(default_max) | ||
| .chain(requests_max).chain(responses_max).chain(notifs_max) | ||
| .max().expect("iterator known to always yield at least one element; qed") | ||
| .saturating_add(10) |
There was a problem hiding this comment.
Maybe even checked_add? Not that I think we will ever exhaust an u64 though :-)
|
bot merge |
|
Trying merge. |
ghost
merged commit 
polkadot companion: paritytech/polkadot#2287
From an API point of view, this PR adds a new field to
NonDefaultSetConfig,max_notification_size, which sets the maximum allowed limit of notifications using that notifications protocol.Before this PR, the maximum size of notifications is in theory 128MiB, coming from
UviBytes::default(). In practice, however, it was actually 1MiB because Yamux will refuse to buffer more than 1MiB.This PR also thus configures the Yamux buffer size limit to automatically match the maximum frame size of all network protocols that we use.