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

Return error on AttestingIndices bitfield length check #8285

Merged
merged 15 commits into from Jan 20, 2021

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Jan 19, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Return error if attestation's bitfield length is greater than the committee length. This hardens and brings the defensive against invalid bitflied into the helper level

Which issues(s) does this PR fix?

N/A

Other notes for review

N/A

@terencechain terencechain self-assigned this Jan 19, 2021
@terencechain terencechain added the Ready For Review A pull request ready for code review label Jan 19, 2021
rauljordan
rauljordan previously approved these changes Jan 19, 2021
@@ -65,14 +68,17 @@ func ConvertToIndexed(ctx context.Context, attestation *ethpb.Attestation, commi
// """
// committee = get_beacon_committee(state, data.slot, data.index)
// return set(index for i, index in enumerate(committee) if bits[i])
func AttestingIndices(bf bitfield.Bitfield, committee []uint64) []uint64 {
func AttestingIndices(bf bitfield.Bitfield, committee []uint64) ([]uint64, error) {
if bf.Len() > uint64(len(committee)) {
Copy link
Member

Choose a reason for hiding this comment

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

This should return an error if they are not equal.

Copy link
Member Author

Choose a reason for hiding this comment

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

True

@terencechain
Copy link
Member Author

TestService_BroadcastAttestationWithDiscoveryAttempts is consistently failing on develop branch in my local runs

@prylabs-bulldozer prylabs-bulldozer bot merged commit 5d84187 into develop Jan 20, 2021
@delete-merged-branch delete-merged-branch bot deleted the return-err-committee-len branch January 20, 2021 03:00
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

❗ No coverage uploaded for pull request base (develop@ab9d596). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #8285   +/-   ##
==========================================
  Coverage           ?   57.09%           
==========================================
  Files              ?      449           
  Lines              ?    31419           
  Branches           ?        0           
==========================================
  Hits               ?    17940           
  Misses             ?    10689           
  Partials           ?     2790           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants