Clarify network limits #7175
Clarify network limits #7175
Conversation
| //1MB | ||
| 1024 * 1024, |
There was a problem hiding this comment.
Why max it at 1MB? Doesn't this void the 10MB limit for mainnet?
There was a problem hiding this comment.
Just following the spec here. https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md#max_message_size
consensus/types/src/chain_spec.rs
Outdated
| fn max_compressed_len_snappy(n: usize) -> usize { | ||
| 32 + n + (n / 6) | ||
| } |
There was a problem hiding this comment.
shouldn't we add guard rails here to not overflow?
I.e.
| fn max_compressed_len_snappy(n: usize) -> usize { | |
| 32 + n + (n / 6) | |
| } | |
| fn max_compressed_len_snappy(n: usize) -> Option<usize> { | |
| 32_usize.checked_add(n)?.checked_add(n/6) | |
| } |
and then unwrap_or_() or expect with a message when using it
There was a problem hiding this comment.
Yeah sorry was being lazy. Made it safe arithematic and added a test to make sure default values don't overflow
consensus/types/src/chain_spec.rs
Outdated
| pub fn max_message_size(&self) -> usize { | ||
| std::cmp::max( | ||
| // 1024 to account for framing + encoding overhead | ||
| Self::max_compressed_len_snappy(self.max_payload_size as usize) + 1024, |
There was a problem hiding this comment.
same as above regarding guard rails
|
@michaelsproul marking this as v7.0.0 as I think we should have this before the mainnet release |
| /// https://github.com/google/snappy/blob/32ded457c0b1fe78ceb8397632c416568d6714a0/snappy.cc#L218C1-L218C47 | ||
| /// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/p2p-interface.md#max_compressed_len | ||
| fn max_compressed_len_snappy(n: usize) -> Option<usize> { | ||
| 32_usize.checked_add(n)?.checked_add(n / 6) |
There was a problem hiding this comment.
Rather than panic'ing (we have expects following this), why not just return usize::MAX if it were to overflow?
There was a problem hiding this comment.
I think silently failing returning a higher max value might not be safe here and could potentially lead to consensus issues.
I have added a test at the end of this file to check that there are no panics based on default mainnet values.
Squashed commit of the following: commit bd3afbd Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Fri Mar 21 17:05:21 2025 -0700 Make arithematic safe commit d6f26e9 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Wed Mar 19 15:55:40 2025 -0700 Update max length conditions commit e3b8168 Author: Pawan Dhananjay <pawandhananjay@gmail.com> Date: Fri Feb 21 18:39:10 2025 -0800 Rename max_gossip_size and remove max_chunk_size
Issue Addressed
Resolves #6811
Proposed Changes
Rename
GOSSIP_MAX_SIZEtoMAX_PAYLOAD_SIZEand removeMAX_CHUNK_SIZEin accordance with the spec.The spec also "clarifies" the message size limits at different levels. The rpc limits are equivalent to what we had before imo.
The gossip limits have additional checks.
I have gotten rid of the
is_bellatrix_enabledchecks that used a lower limit (1mb) pre-merge. Since all networks we run start from the merge, I don't think this will break any setups.