-
Notifications
You must be signed in to change notification settings - Fork 247
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
Allow 3 bytes of segment padding in object fetcher #3332
Conversation
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.
Non-blocking
max_object_len: max_object_len | ||
.unwrap_or(usize::MAX) | ||
.min(MAX_SUPPORTED_OBJECT_LENGTH), |
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.
Why .unwrap_or(usize::MAX)
and not .unwrap_or(MAX_SUPPORTED_OBJECT_LENGTH)
?
Also I'd prefer making max_object_len
non-optional, this is getting too confusing (it accepts the input, but actually limits the size internally).
a6c46f3
to
b856cce
Compare
if max_object_len > MAX_SUPPORTED_OBJECT_LENGTH { | ||
warn!( | ||
max_object_len, | ||
MAX_SUPPORTED_OBJECT_LENGTH, | ||
"Object fetcher size limit exceeds maximum supported object size, \ | ||
limiting to implementation-supported size" | ||
); | ||
|
||
max_object_len = MAX_SUPPORTED_OBJECT_LENGTH; | ||
} |
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.
What is the reason for this, why can't caller specify whatever limit they want?
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.
The current implementation assumes the largest compact encoded object length is 4 bytes, or a 1 GB - 1
length:
subspace/shared/subspace-data-retrieval/src/object_fetcher.rs
Lines 29 to 34 in b856cce
/// The maximum amount of segment padding. | |
/// | |
/// This is the difference between the compact encoding of lengths 1 to 63, and the compact | |
/// encoding of lengths 2^14 to 2^30 - 1. | |
/// <https://docs.substrate.io/reference/scale-codec/#fn-1> | |
pub const MAX_SEGMENT_PADDING: usize = 3; |
We can change that assumption any time we like, but it seems pointless right now, because the largest current block size in any domain is 5 MB.
The old code for the object fetcher assumed the maximum segment padding was 2 bytes, but it is actually 3 bytes. (Assuming objects are limited to < 1 GB.)
This PR fixes that bug, and returns an error for objects larger than 1 GB. Currently the maximum object size is 5 MB, because object size is limited by domain block size.
If we need to support larger objects, we can update the code to allow more padding easily.
Close #3324.
Code contributor checklist: