Take the header size into account for the total block size #10804
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Differential Tests Results (PolkaVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
bkchr
changed the title
skunert
reviewed
Jan 16, 2026
| if env::var("RUN_TEST").is_ok() { | ||
| let (client, parent_head) = create_test_client(); | ||
|
|
||
| let digest_data_exceeding_max_head_data_size = vec![0u8; 1_048_576 + 1024]; |
Contributor
There was a problem hiding this comment.
Can we use MAX_HEAD_DATA_SIZE + 1 here instead of this hardcoded value?
cumulus/test/runtime/src/lib.rs
Outdated
| pub const Version: RuntimeVersion = VERSION; | ||
| pub RuntimeBlockLength: BlockLength = | ||
| BlockLength::max_with_normal_ratio(5 * 1024 * 1024, NORMAL_DISPATCH_RATIO); | ||
| BlockLength::max_with_normal_ratio(12 * 1024 * 1024, NORMAL_DISPATCH_RATIO); |
Member
Author
There was a problem hiding this comment.
So that we can test the assert in validate_block. Otherwise the digests there are too big :D
substrate/frame/system/src/lib.rs
Outdated
| let overhead = digest_size.saturating_add(empty_header_size) as u32; | ||
| AllExtrinsicsLen::<T>::put(overhead); | ||
|
|
||
| // Ensure inherent digests don't exceed 20% of the max block size. |
Contributor
There was a problem hiding this comment.
Is this 20% arbitrary or we had this rule already?
Member
Author
There was a problem hiding this comment.
Arbitrary. Should be more than enough space for inherent digests. Everything we are doing is not bigger than some bytes, so getting ~2MiB for 10MiB sounds more than enough.
skunert
approved these changes
Jan 16, 2026
bkchr
force-pushed
the
bkchr-ensure-headers-use-some-max
branch
from
e6359a9 to
52afdef
Compare
franciscoaguirre
approved these changes
Jan 30, 2026
| BlockLength::max_with_normal_ratio(5 * 1024 * 1024, NORMAL_DISPATCH_RATIO); | ||
| BlockLength::builder() | ||
| .max_length(5 * 1024 * 1024) | ||
| .modify_max_length_for_class(DispatchClass::Normal, |m| { *m = NORMAL_DISPATCH_RATIO * *m; }) |
Contributor
There was a problem hiding this comment.
Can't you also reuse m on the other places where you copied and pasted the max length?
substrate/frame/system/src/limits.rs
Outdated
| @@ -40,22 +40,39 @@ pub struct BlockLength { | |||
| /// In the worst case, the total block length is going to be: | |||
| /// `MAX(max)` | |||
| pub max: PerDispatchClass<u32>, | |||
| /// Optional maximum header size in bytes. | |||
| /// | |||
| /// It is still possible that a header goes above this limit, if the runtime deposits to may | |||
Contributor
There was a problem hiding this comment.
Suggested change
| /// It is still possible that a header goes above this limit, if the runtime deposits to may | |
| /// It is still possible that a header goes above this limit, if the runtime deposits too many |
substrate/frame/system/src/lib.rs
Outdated
Comment on lines
1001
to
1002
| /// In contrast to its name it also includes the header overhead and digest size to accurately | ||
| /// track block size. |
Contributor
There was a problem hiding this comment.
Do we want to change the name? Too much of a breaking change?
Member
There was a problem hiding this comment.
Yea, I dont think anyone is reading this anyway, besides frame_system or?
Member
Author
There was a problem hiding this comment.
Yeah I can change if you like.
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
ggwpez
reviewed
Jan 30, 2026
Comment on lines
+444
to
+448
| block_data.header.len() + | ||
| block_data.body.iter().map(|ex| ex.len()).sum::<usize>() + | ||
| block_data.indexed_body.iter().map(|ex| ex.len()).sum::<usize>(); | ||
| block_data.indexed_body.iter().map(|ex| ex.len()).sum::<usize>() + | ||
| block_data.justification.len() + | ||
| block_data.justifications.len(); |
Member
There was a problem hiding this comment.
Why do you have to go through these manually instead of something like block_data.encoded_size()?
Member
Author
There was a problem hiding this comment.
Good question. Just kept it as it was before.
| assert_eq!(net.peer(1).client.info().best_number, 33); | ||
| } | ||
|
|
||
| /// Test syncing 512 blocks with ~1.2 MiB headers (empty bodies) to test large header handling. |
Member
There was a problem hiding this comment.
I see 900 KiB digests, where are the other 300 KiB coming from in the header?
substrate/frame/system/src/lib.rs
Outdated
Comment on lines
1001
to
1002
| /// In contrast to its name it also includes the header overhead and digest size to accurately | ||
| /// track block size. |
Member
There was a problem hiding this comment.
Yea, I dont think anyone is reading this anyway, besides frame_system or?
ggwpez
reviewed
Jan 30, 2026
Comment on lines
+1939
to
+1946
| let empty_header = <<T as Config>::Block as traits::Block>::Header::new( | ||
| *number, | ||
| Default::default(), | ||
| Default::default(), | ||
| *parent_hash, | ||
| Default::default(), | ||
| ); | ||
| let empty_header_size = empty_header.encoded_size(); |
Member
There was a problem hiding this comment.
I guess we cannot hard-code an upper bound and have an integrity test for this encoded size since it is generic over the runtime types? hm...
Member
Author
There was a problem hiding this comment.
Yeah not super perfect, but also not that much better solvable.
ggwpez
reviewed
Jan 30, 2026
substrate/frame/system/src/limits.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl BlockLength { | ||
| /// Create new `BlockLength` with `max` for every class. | ||
| #[deprecated(since = "TBD", note = "Use `BlockLength::builder().max(value).build()` instead")] |
Member
There was a problem hiding this comment.
Suggested change
| #[deprecated(since = "TBD", note = "Use `BlockLength::builder().max(value).build()` instead")] | |
| #[deprecated(note = "`BlockLength::max(value)` will be removed after July 2026. Use `BlockLength::builder().max(value).build()` instead")] |
Or do you want to put a version?
Member
Author
There was a problem hiding this comment.
We don't have any proper version? :D
ggwpez
reviewed
Jan 30, 2026
substrate/frame/system/src/limits.rs
Outdated
| } | ||
|
|
||
| /// Create new `BlockLength` with `max` for `Operational` & `Mandatory` | ||
| /// and `normal * max` for `Normal`. | ||
| #[deprecated( | ||
| since = "TBD", |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.