fix: resubscribe to beacon subnets on current epoch reorg #8570
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #8570 +/- ##
============================================
+ Coverage 51.92% 52.01% +0.09%
============================================
Files 848 848
Lines 65806 65825 +19
Branches 4799 4807 +8
============================================
+ Hits 34168 34240 +72
+ Misses 31569 31517 -52
+ Partials 69 68 -1 🚀 New features to boost your workflow:
|
|
@nflaig Can you please review this? |
nflaig
left a comment
There was a problem hiding this comment.
Thanks @phertyameen for taking a look at this, few remarks mostly regarding the tests as those do not follow the usual patterns we use, please take a look at existing tests and try to align test / mock data setup a bit more (see comments)
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
|
Understood. Will make these corrections right away. Thanks for the detailed review. |
…BeaconCommitteeSubnets method in both pollBeaconAttesters and handleAttesterDutiesReorg
…//github.com/phertyameen/lodestar into phertyameen/resubscribeOnAttesterDutiesReorg
nflaig
left a comment
There was a problem hiding this comment.
Looks pretty good, few remarks on the unit tests, will push some minor updates then we are good to merge this
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
packages/validator/test/unit/services/attestationDuties.test.ts
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where validators were not resubscribing to beacon subnets after a reorg affecting their duties. The fix involves adding logic to handleAttesterDutiesReorg to rebuild and resubmit subnet subscriptions based on the updated duties. The introduction of a dedicated subscribeToBeaconCommitteeSubnets method is a good refactoring choice. The accompanying tests are thorough, covering reorg scenarios for both the current and next epochs, as well as the case where no change occurs, ensuring the new logic is well-validated. I have one suggestion to further improve maintainability by reducing some code duplication.
|
Noted the corrections. Thank you again for the assistance. I have learned a lot from this 🙏 |
|
🎉 This PR is included in v1.36.0 🎉 |
Motivation
Not resubscribing to beacon subnets (prepareBeaconCommitteeSubnet) as dependent root for current epoch changes. When this happens, previous subscriptions are no longer valid
validator duties changed (slot, committee index, etc.)
is_aggregator results are different
Description
Added subnet resubscription logic to
handleAttesterDutiesReorgthat fetches updated attester duties, rebuild beaconCommitteeSubscriptions and resubscribe validators to the correct beacon subnets by calling the prepareBeaconCommitteeSubnet apiAdded test to handle:
There was intentional use of claude AI in writing the test.
Closes #6034
Steps to test or reproduce