Switch custody_subnet_count from u64 to u8 #6323
Closed
Conversation
This file contains 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
pawanjay176
added
ready-for-review
jimmygchen
reviewed
Aug 29, 2024
| let u8_max = u8::MAX as u64; | ||
| if custody_requirement > u8_max | ||
| || data_column_sidecar_subnet_count > u8_max | ||
| || number_of_columns > u8_max |
There was a problem hiding this comment.
number_of_columns is still uint64 in the spec
| `NUMBER_OF_COLUMNS` | `uint64(CELLS_PER_EXT_BLOB)` (= 128) | Number of columns in the extended data matrix |
jimmygchen
requested changes
Aug 29, 2024
There was a problem hiding this comment.
I just realized the change isn't strictly necessary because RLP encoding doesn't actually fill any 0 bytes, so encoding 128 is the same for u8 and u64. This change just put a upper bound to the number 🤔
I'm not sure if the spec change was necessary. it just limit us to go above 255 subnets and has no impact on the encoded value. Will ask on R&D.
jimmygchen
added
under-review
|
This isn't required after ethereum/consensus-specs#3908 |
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.
Issue Addressed
ethereum/consensus-specs#3897
Proposed Changes
Change metadata and enr fields to take a u8 instead of a u64.
I have chosen not to change the config and chainspec params to u8 as well since they permeate throughout the codebase and we'll have to do a bunch of unnecessary
u8 as u64sort of conversions. Instead, I made apply_to_chainspec fallible in case the parameters in the config exceed u8::MAX.