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

Use uint64 for GeneralizedIndex #13

Merged
merged 3 commits into from
Jun 23, 2022
Merged

Use uint64 for GeneralizedIndex #13

merged 3 commits into from
Jun 23, 2022

Conversation

etan-status
Copy link
Contributor

To support adressing any nimbus-eth2 BeaconState member, 32-bit wide
indices are not enough. BeaconState requires 5 levels of depth;
VALIDATOR_REGISTRY_LIMIT for the validators array requires 40 levels
for the contents plus 2 levels for len and the root. Another 4 levels
are required for Validator. This means that 52-bit indices are needed.
Extending to uint64 should provide enough leeway without requiring
further implementation adjustments.

@etan-status
Copy link
Contributor Author

I think that the internal mergedDataHash etc which operate on int64 do not need adjustment, because they only operate on sub-trees where 1.GeneralizedIndex through 3.GeneralizedIndex (length) are mapped to array element 0, and 4.GeneralizedIndex is mapped to element 1 (i.e., two bits are stripped). So, even if a full-width uint64 index was hypothetically used, this should still work.

@tersec
Copy link
Contributor

tersec commented Nov 24, 2021

nimbus-eth2 doesn't support more than int32-allowed validatorindices, regardless. That's arguably an unfortunate implementation detail, but also a bottleneck regardless of this change.

@etan-status etan-status changed the title use uint64 for GeneralizedIndex. Use uint64 for GeneralizedIndex Nov 24, 2021
@etan-status
Copy link
Contributor Author

Wait for #19 before merging this.

@etan-status etan-status marked this pull request as draft December 16, 2021 10:43
To support adressing any nimbus-eth2 `BeaconState` member, 32-bit wide
indices are not enough. `BeaconState` requires 5 levels of depth;
`VALIDATOR_REGISTRY_LIMIT` for the `validators` array requires 40 levels
for the contents plus 2 levels for `len` and the root. Another 4 levels
are required for `Validator`. This means that 52-bit indices are needed.
Extending to `uint64` should provide enough leeway without requiring
further implementation adjustments.
@etan-status etan-status marked this pull request as ready for review December 17, 2021 16:06
@etan-status etan-status marked this pull request as draft December 17, 2021 16:19
@etan-status
Copy link
Contributor Author

Wait for #16 before merging this.

@etan-status etan-status marked this pull request as ready for review June 23, 2022 12:50
@etan-status etan-status merged commit 9e754ec into status-im:master Jun 23, 2022
@etan-status etan-status deleted the generalized-64 branch June 23, 2022 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants