-
-
Notifications
You must be signed in to change notification settings - Fork 311
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 attestation compatibility issue in distributed validator cluster #5258
Conversation
8c0888b
to
6deb360
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
deacc45
to
2c323b6
Compare
Is this solution really simpler than just branching on the original class? |
@dapplion I need to further refactor this so get a better picture, the 4 options I see are
IMO our attestation service is really easy to reason about at the moment and I really want to keep it like that. Completely isolating the logic seems like the best idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's much more simple to have branching logic, see full implementation here https://github.com/ChainSafe/lodestar/compare/dapplion/attestation-service-no-grouping?expand=1
fe962d9
to
090104f
Compare
@dapplion thanks for the input, after further refactoring and reviewing I think you are right, I also like the branching solution more Updated the code based on your initial implementation |
090104f
to
ef86934
Compare
ef86934
to
ccdf9f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to merge! Just a few nits
617b089
to
0bea203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
59f3fd7
to
69827ad
Compare
This should not be merged yet, waiting for confirmation from Obol that this actually fixes the DVT compatibility issues. |
a3f0728
to
ebf265d
Compare
ebf265d
to
7a5e0c9
Compare
602c0fc
to
9ab6aad
Compare
Testing was successful 👍 |
🎉 This PR is included in v1.7.0 🎉 |
Motivation
Closes #5103 and #4687
Description
--disableAttestationGrouping
)--distributed
), this will also disable attestation groupingOpen tasks
runAttestationTasksPerCommittee