Skip to content

Fix filterContained#12740

Closed
mitmotw wants to merge 8 commits intoOffchainLabs:developfrom
mitmotw:fix_filterContained
Closed

Fix filterContained#12740
mitmotw wants to merge 8 commits intoOffchainLabs:developfrom
mitmotw:fix_filterContained

Conversation

@mitmotw
Copy link
Copy Markdown

@mitmotw mitmotw commented Aug 15, 2023

What type of PR is this?
Other

What does this PR do? Why is it needed?

filterContained is checking 'contains' between the 'last element of filtered' and new attestation from given attestation list.
So what I'm worrying is like below case.

let the given attestation list has 3 attestations and aggregation bits are like below.
(Just assumed aggregation bits are 6 bits for convenience)

attestation[0].AggregationBits = [1, 1, 1, 1, 0, 0]
attestation[1].AggregationBits = [0, 0, 0, 0, 1, 1]
attestation[2].AggregationBits = [0, 0, 0, 1, 0, 0]

Here I expect all the 3 elements will be included in the final returned list, filtered.

At the first iteration,
attestation[1] will be appended because attestation[0].AggregationBits does not contain attestation[1].AggregationBits.

At the second iteration,
attestation[2] will be appended because attestation[1].AggregationBits (which is the final element of filtered) does not contain attestation[2].AggregationBits.

Like this, all 3 elements will be included in filtered list even though attestation[0].AggregationBits contains attestation[2].AggregationBits.

So this fix stores coveredBits for holding the bits covered so far during the iteration, and
use this for checking 'contains' instead of the 'last element of filtered'.

Which issues(s) does this PR fix?
#12727

@mitmotw mitmotw requested a review from a team as a code owner August 15, 2023 10:26
Copy link
Copy Markdown
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

The changes look good to me, but I am curious how other attestation aggregate tests are not impacted by this change

Comment thread proto/prysm/v1alpha1/attestation/aggregation/attestations/maxcover_test.go Outdated
Comment thread proto/prysm/v1alpha1/attestation/aggregation/attestations/maxcover.go Outdated
@rauljordan
Copy link
Copy Markdown
Contributor

Thanks for the contribution @mitmotw !! We're very grateful. As Preston mentioned, here is the gosec failure:


[/home/runner/work/prysm/prysm/proto/prysm/v1alpha1/attestation/aggregation/attestations/maxcover.go:291] - G104 (CWE-703): Errors unhandled. (Confidence: HIGH, Severity: LOW)
    290: 		filtered = append(filtered, al[i])
  > 291: 		coveredBits.NoAllocOr(candidates[i], coveredBits)
    292: 	}



[/home/runner/work/prysm/prysm/proto/prysm/v1alpha1/attestation/aggregation/attestations/maxcover.go:280] - G104 (CWE-703): Errors unhandled. (Confidence: HIGH, Severity: LOW)
    279: 	coveredBits := bitfield.NewBitlist64(candidates[0].Len())
  > 280: 	coveredBits.NoAllocOr(candidates[0], coveredBits)
    281: 

@rkapka
Copy link
Copy Markdown
Contributor

rkapka commented Sep 9, 2025

Hi @mitmotw , I know it's been a very long time since the last activity in this PR. There is a conflict that prevents us from merging it. Can you please resolve it?

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.

4 participants