ChainSafe / ssz

Typescript implementation of Simple Serialize (SSZ)
https://simpleserialize.com/
Other
53 stars 20 forks source link

UintNum64 is invalid and will create all sorts of issues #406

Closed paulmillr closed 1 month ago

paulmillr commented 1 month ago
  1. UintNum64 is UintNumberType, instead of UintBigintType
  2. JS can't handle numbers larger than ~2**53
  3. This causes issues. For example, encode/decode of 'consensus-spec-tests': test case overflows f64 in js, which causes 'encode(decode(x))!==x'. Are you using consensus-spec-tests?
> 2**64
< 18446744073709552000
> 2**64-1
< 18446744073709552000
> 2**64-100
< 18446744073709552000
g11tech commented 1 month ago

correct, for expected 64 bit range however UintBn64 is supposed to be used

wemeetagain commented 1 month ago

As you probably know, BigInt has severe performance penalty over number in basically every way. And of course, number can only accurately represent integers <53 bits. UintNum64 is useful in performance-sensitive cases where the application logic can enforce that a valid value will never be >= 2^53. It is a tradeoff -- vastly better performance, but inability to encode/decode the full range, and additional enforcement / ability-to-footgun at the application layer.

In the case of ethereum, there are several places where this can be utilized (and is utilized in Lodestar). The best example is Slot, an alias of uint64, the consensus layer type for time. Slots start at 0, and increment by 1 every SECONDS_PER_SLOT seconds. With mainnet's current configuration, it will take billions of years to approach 2^53. In most cases, large slot values, not even approaching 2^53, are ruled as "too far in the future". Thus, in most cases, Slot can be treated as a number safely. It's only cases where large slot values can get ingested by consensus that this must be revisited (eg slashings). In those cases, Slot must be represented as a BigInt.

paulmillr commented 1 month ago

The repo doesn't pass https://github.com/ethereum/consensus-spec-tests though. Is it expected?

wemeetagain commented 1 month ago

Yes, this UintNum64 type will not pass all consensus spec tests for the uint64 type

paulmillr commented 1 month ago

Thanks for clarification.