search

ChainSafe / lodestar

🌟 TypeScript Implementation of Ethereum Consensus
https://lodestar.chainsafe.io
Apache License 2.0
1.18k stars 289 forks source link

Lodestar is not spec testing uint field in ssz containers correctly #7097

Closed ensi321 closed 1 month ago

ensi321 commented 1 month ago

Describe the bug

Background

The amount field in WithdrawalRequest was defined as UintNum64, which can only hold up to 2^53 - 1.

In devnet-3, Lodestar was stuck because it receives a withdrawal request with amount ~= 2^54 which ultimately lead to miscomputing beacon block hash. The issue was fixed in #7085.

Description

After digging into test vectors in consensus-specs, there is a test case that tests the ssz serialization/deserialization of WithdrawalRequest with amount = 2^64 - 1, but somehow Lodestar passes the test.

This is due to ssz_static.test.ts converts all uint fields in ssz types to big int automatically here https://github.com/ChainSafe/lodestar/blob/bb40ef7eb77f9441ec66ab8f1fdf6664240bb1a3/packages/beacon-node/test/spec/presets/ssz_static.test.ts#L64 before performing serialization/deserialization test.

Expected behavior

Expect ssz static spec test to catch the described devnet-3 error.

Steps to reproduce

No response

Additional context

No response

Operating system

Linux

Lodestar version or commit hash

v1.22.0

ensi321 commented 1 month ago

Closing as I don't think there is a better solution to this other than implementers being more careful choosing UintNum64 vs UintBn64. The current ssz_static.test.ts implementation makes sense to me.