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.

Sign up for GitHub

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

Jump to bottom

is convert.RunnerRole really needed? #1561

Open
moshe-blox opened this issue Aug 6, 2024 · 2 comments · May be fixed by #1810
Open

is convert.RunnerRole really needed? #1561

moshe-blox opened this issue Aug 6, 2024 · 2 comments · May be fixed by #1810
Assignees
@anatolie-ssv
Labels
alan
Milestone
Alan

Comments

@moshe-blox
Copy link
Contributor

moshe-blox commented Aug 6, 2024 •
edited
Loading

convert.RunnerRole seems like a bit of a hack, as if we're adapting Alan to the existing code rather than the other way around.

For example in QBFT storage: is there a good reason to store QBFT instances by BeaconRole at post-fork? At post-fork we perform consensus per RunnerRole not per BeaconRole, so intuitively they should be stored by RunnerRole.

This issue exists to discuss this and come up with a PR to fix if necessary.
@moshe-blox moshe-blox added the alan label Aug 6, 2024
@y0sher
Copy link
Contributor

y0sher commented Aug 11, 2024

@moshe-blox, in exporter, we want to save consensus results (qbft storage..) in the 'old' format, so convert.RunnerRole basically enables using RunnerRole but also saving Attester and Sync committee duties. Since exporter doesn't share anything about committee consensus now and only about post-consensus.

@moshe-blox
Copy link
Contributor Author

@y0sher i see, but still i think the QBFT storage should use the spectypes role, and only in exporter we should see usage of the convert function (and not in other packages)

@moshe-blox moshe-blox added this to the Alan milestone Aug 14, 2024
@anatolie-ssv anatolie-ssv linked a pull request Oct 22, 2024 that will close this issue
Draft
@anatolie-ssv anatolie-ssv linked a pull request Oct 22, 2024 that will close this issue
fix: remove convert runner role #1810
Draft
@anatolie-ssv anatolie-ssv self-assigned this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@anatolie-ssv anatolie-ssv

Labels
alan
Projects
None yet
Milestone
Alan
Development

Successfully merging a pull request may close this issue.

fix: remove convert runner role ssvlabs/ssv
3 participants
@y0sher @moshe-blox @anatolie-ssv