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: duplicate and old attestations aggregates #1303

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

rodrigo-o
Copy link
Collaborator

@rodrigo-o rodrigo-o commented Sep 23, 2024

Motivation

Right now we are publishing old aggregates (slot -2) instead of the current (slot -1), also we publish several aggregates for the same subcommittee, this will be solved by this PR.

Description

This PR solves both issues explained in 1254, on one hand the old attestation (being slot - 2 the most recen attestation always) was due to picking the slot on ignore/2 from the store (slot - 1) instead of taking it from the current block (slot). For the second issue, duplications was due to a lacking of filtering in the block builder steps, as with sync committees, we need to group attestations by committee index and just keep the ones with the greatest pop count (the most amount of set bits)

Resolves #1254

@rodrigo-o rodrigo-o marked this pull request as ready for review September 23, 2024 20:05
@rodrigo-o rodrigo-o requested a review from a team as a code owner September 23, 2024 20:05
@rodrigo-o
Copy link
Collaborator Author

Now we can see an small amount of attestations per block (previously we had 4/6 even with just one committee)
image

And we could also see that for a particular slot we have published the most recent attestation:
image

Comment on lines 213 to 222
defp process_attestations(attestations) do
attestations
|> Enum.group_by(& &1.data.index)
|> Enum.map(fn {_, attestations} ->
Enum.max_by(
attestations,
&(&1.aggregation_bits |> BitVector.count())
)
end)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe "filter_latest_attestations" or something more descriptive? or deduplicate_attestations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified it to select_best_aggregates/1 to convey the actual functionality, selecting between aggregates the one with the most set bits

@rodrigo-o rodrigo-o enabled auto-merge (squash) September 26, 2024 16:21
@rodrigo-o rodrigo-o merged commit 86f4df2 into main Sep 26, 2024
13 checks passed
@rodrigo-o rodrigo-o deleted the duplicate-and-old-aggregates branch September 26, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Duplicated and old aggregate attestations
2 participants