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

Fix bug in aggregator verification #2753

Merged
merged 4 commits into from
Jun 28, 2021
Merged

Conversation

dapplion
Copy link
Contributor

Motivation

Note: must use bytesToBigInt() otherwise a JS number is not able to represent the latest digits of the remainder, resulting in 14333559117764833000 for example, where the last three digits are always zero. Using bytesToInt() may cause isSelectionProofValid() to always return false.

Description

  • Use bytesToBigInt()
  • Use correct constant

@dapplion dapplion added the prio-high Resolve issues as soon as possible. label Jun 27, 2021
@codeclimate
Copy link

codeclimate bot commented Jun 27, 2021

Code Climate has analyzed commit 8e05473 and detected 0 issues on this pull request.

View more on Code Climate.

@dapplion
Copy link
Contributor Author

This bug slipped through because it's not covered by spec tests. This logic is purely p2p, so:

  • it's not covered by any spec tests, nor unit tests
  • it won't cause a consensus failure in our nodes

To prevent this from happening again we should move all logic not used in the state transition from the beacon-state-transition package. Put it somewhere else (like in the lodestar package) and heavily test it.

@twoeths
Copy link
Contributor

twoeths commented Jun 28, 2021

seems like we have the other isSyncCommitteeAggregator in packages/validator/src/util/aggregator.ts which uses BigInt there, we should deduplicate it

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@wemeetagain wemeetagain merged commit 3847e25 into master Jun 28, 2021
@wemeetagain wemeetagain deleted the dapplion/aggregator-check-bug branch June 28, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio-high Resolve issues as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants