Skip to content
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

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

Closed
ensi321 opened this issue Sep 20, 2024 · 1 comment
Closed

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

ensi321 opened this issue Sep 20, 2024 · 1 comment
Labels
meta-bug Issues that identify a bug and require a fix.

Comments

@ensi321
Copy link
Contributor

ensi321 commented Sep 20, 2024

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

const sszTypeNoUint = replaceUintTypeWithUintBigintType(sszType);

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 ensi321 added the meta-bug Issues that identify a bug and require a fix. label Sep 20, 2024
@ensi321
Copy link
Contributor Author

ensi321 commented Sep 20, 2024

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.

@ensi321 ensi321 closed this as completed Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-bug Issues that identify a bug and require a fix.
Projects
None yet
Development

No branches or pull requests

1 participant