primitives: make block header gas limit field compatible with u128 #11130
Conversation
tcoratger
requested review from
joshieDo,
Rjected,
mattsse,
rkrasiuk and
gakonst
as code owners
mattsse
added
the
C-debt
| @@ -23,7 +23,7 @@ struct Header { | |||
| logs_bloom: Bloom, | |||
| difficulty: U256, | |||
| number: BlockNumber, | |||
| gas_limit: u64, | |||
There was a problem hiding this comment.
this would break.
additionally we should add an existing recent header to tests to ensure backwards compatibility
There was a problem hiding this comment.
I've just undo this change. I suggest that for now we leave the codec as it is without touching it so we don't risk breaking anything. There are other fields to change first. When everything is done we can come back to it and fix things properly, wdyt?
There was a problem hiding this comment.
we should still add the test.
this kind of change, changing an existing field type like this, is not possible unfortunately. Added some additional docs here https://github.com/paradigmxyz/reth/pull/11131/files#diff-e5f90be56b8337d7431ad21b53260ca57f13efed8310e8c48f2a0bfeca43e82f
in a short summary ser/deser of certain types are done by using an helper struct that has fixed sizes (bitflag struct). changing u64 to u128 would increase how many bits are required for that field
There was a problem hiding this comment.
Since everything is converging towards using u64 for these fields, I wonder if it would not be better to do what is described here alloy-rs/alloy#1241, that is to say to convert everything to u64 in alloy. This will reduce a lot of modifications to be made in reth and it would naturally eliminate all the conversions when applying the clippy...
I do not really see any specific cases where the use of a u128 is justified except perhaps very specific devnets or local developments but as a general rule, I would say that switching everything to u64 seems more justified to me. What do you think @joshieDo @mattsse @klkvr ?
There was a problem hiding this comment.
if it would not be better to do what is described here alloy-rs/alloy#1241,
yeah I know think that'd be better, especially because eth consensus has these fields as u64 and u64::MAX should be plenty for all things gas
|
closing since we should change the header fields instead |
Follow up on #10691 cc @mattsse