-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Define requirements for the blob schedule #4370
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,11 @@ specification. | |
| *[New in EIP7892]* This schedule defines the maximum blobs per block limit for a | ||
| given epoch. | ||
|
|
||
| There MUST NOT exist multiple blob schedule entries with the same epoch value. | ||
| The maximum blobs per block limit for blob schedules entries MUST be less than | ||
| or equal to `MAX_BLOB_COMMITMENTS_PER_BLOCK`. The blob schedule entries SHOULD | ||
| be sorted by epoch in ascending order. The blob schedule MAY be empty. | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it must be sorted should we also remove
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Id probably prefer leaving the should be sorted above, and just sorting in the fn (or reading the config obviously would be the alternative). If we make it must then i'd see a validation if its not sorted correctly and failing to start, that'd probably be the alternative (but a can of worms imo, we'd need to be super sure its sorted from beacon-api etc)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the sorting requirement here is should not must. So yeah I agree with @rolfyone here; I think it would be best to force sort when reading the config, if that makes sense. The |
||
| *Note*: The blob schedule is to be determined. | ||
|
|
||
| <!-- list-of-records:blob_schedule --> | ||
|
|
||
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.
We did descending sort when parsing from configs to avoid sorting every time
get_max_blobs_per_blockcalled. If we sort blob schedule in ascending order, then we need to reverse it every timeThere 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.
given we just read the config once, sorting at startup is prudent...
I like being explicit about only having at most 1 entry at any given epoch. this sounds like an easy startup validation too...
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.
@hangleang I think it's okay for clients to do what works best for them. I guess I just wanted to define the order here (in the specs) so that it's consistent in the future. Personally, I believe ascending order is easier to comprehend; where new entries are appended to the end, rather than added to the beginning.