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

bug in aggregateVote.numAggregateSig #7

Closed
djrtwo opened this issue Sep 20, 2018 · 3 comments
Closed

bug in aggregateVote.numAggregateSig #7

djrtwo opened this issue Sep 20, 2018 · 3 comments

Comments

@djrtwo
Copy link

djrtwo commented Sep 20, 2018

Issue

aggregateVote.numAggregateSig currently incorrectly uses the length of aggregate_sig to get the number of aggregate signatures.
https://github.com/ChainSafeSystems/lodestar_chain/blob/a270c7a577a3deda88f96cebb41bc3f173e26b1d/lodestar_chain/state/aggregateVote.js#L36-L38

aggregate_sig is in fact always just the length of 2 so this fails.

Proposed Implementation

You can use the number of 1s in the bitfield to get the length. If you need the length often, you might consider some sort of helper attribute that caches this results.

@djrtwo
Copy link
Author

djrtwo commented Sep 20, 2018

I'm not 100% sure what aggregateVote is being used for. Maybe it is just a relic of a previous version of the spec.

That said, this logic around BLS aggregate signatures still holds when used in other data structures.

@Mikerah
Copy link
Contributor

Mikerah commented Sep 20, 2018

Hi Danny,

Thank you so much for submitting an issue. We do indeed have relics of the previous spec in this repo. We are going to be going over the repo, and creating issues to make sure I follow your advice on making it easier for others to contribute.

@GregTheGreek
Copy link
Member

I think this can be closed due to #39

wemeetagain pushed a commit that referenced this issue Aug 5, 2019
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

No branches or pull requests

3 participants