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 wrong sync committee participation association #2682

Merged
merged 12 commits into from
Nov 27, 2023

Conversation

manuelsc
Copy link
Collaborator

@manuelsc manuelsc commented Nov 9, 2023

Note that this PR restores functionality of the sync committee stats and does not aim to fully support the edge case of a validator being part of the same sync committee twice as this requires a wider refactor of how we handle sync committees on the frontend. Will create a jira task for that special case.

Main culprit of current approach was that the de duplication on statistics exporter level shuffled the sync committee index. Removed de duplication in this level since a validator can be elected into the same sync committee multiple times and kept current frontend handling (which does not handle this case at all atm, but so far this hasn't occurred on mainnet or prater yet.)

Requires a re-export via misc tool, command:

misc \
--command=export-sync-committee-periods \
--dry-run=false \
--config path_to_config.yml \
&& misc \
--command=export-sync-committee-validator-stats \
--dry-run=false \
--config path_to_config.yml \
&& misc \
--command=export-stats-totals \
--columns=missed_attestations_total,participated_sync_total,missed_sync_total,orphaned_sync_total \
--day-start=0 \
--day-end=0 \
--dry-run=false \
--config path_to_config.yml

Fun Fact: Sync period 827 is utterly wrong on prater db, every one of the 512 validators has a second entry with a different committee indices. Should be fixed via re-export also.

🤖 Generated by Copilot at b3baa4a

This pull request fixes various bugs and inconsistencies in the sync committee data for validators, both in the exporter and the API handler. It also improves the SQL queries and adds comments to make the code more clear and reliable.

@manuelsc manuelsc marked this pull request as draft November 9, 2023 13:23
@manuelsc manuelsc marked this pull request as ready for review November 13, 2023 15:19
exporter/sync_committees.go Show resolved Hide resolved
exporter/sync_committees.go Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
exporter/sync_committees.go Show resolved Hide resolved
handlers/api.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@LuccaBitfly LuccaBitfly left a comment

Choose a reason for hiding this comment

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

Sorry, wasn't aware that the sync reexport tools are gone so I didn't look at exportSyncCommitteeValidatorStats in my first review.

cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
cmd/misc/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@LuccaBitfly LuccaBitfly left a comment

Choose a reason for hiding this comment

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

lgtm

@manuelsc manuelsc merged commit 601e32b into gobitfly:master Nov 27, 2023
3 checks passed
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.

2 participants