Duty logic validation #228
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diegomrsantos
force-pushed
the
duty-logic
branch
from
e99fdc7 to
89f0306
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors duty logic validations by introducing new parameters and modules to support BeaconNetwork and Duties tracking while updating the Validator to use a DutiesProvider.
- Introduces the new slots_per_epoch field into validation contexts and tests.
- Refactors the Validator to take a DutiesProvider along with a BeaconNetwork.
- Adds new modules for duties tracking and beacon network functionalities.
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| anchor/message_validator/src/partial_signature.rs | Added slots_per_epoch field in test contexts for partial signature validation. |
| anchor/message_validator/src/lib.rs | Updated Validator struct and validation functions to use BeaconNetwork and DutiesProvider. |
| anchor/message_validator/src/consensus_message.rs | Updated consensus message validation to utilize the new duty logic and error variants. |
| anchor/message_validator/src/duties/* | New modules that implement duties tracking and the DutiesProvider trait. |
| Other files | Updates updating generics and dependencies across message sender/receiver and client to support the new duty logic features. |
Files not reviewed (1)
- .githooks/pre-commit: Language not supported
Comments suppressed due to low confidence (2)
anchor/message_validator/src/partial_signature.rs:237
- [nitpick] Consider replacing the hard-coded magic number '32' with a named constant to improve clarity and maintainability in test configurations.
+ slots_per_epoch: 32,
anchor/message_validator/src/consensus_message.rs:51
- [nitpick] Cloning the entire slot clock might incur unnecessary performance overhead if the underlying implementation is not lightweight; consider passing a reference or ensuring that the SlotClock type implements a cheap clone.
beacon_network.slot_clock().clone(),
diegomrsantos
commented
Apr 17, 2025
| Ok(()) | ||
| } | ||
|
|
||
| pub fn start(self: Arc<Self>, executor: TaskExecutor) { |
There was a problem hiding this comment.
@dknopik do we need to handle cancellation or an orderly shutdown hook when starting tasks like that?
dknopik
reviewed
Apr 17, 2025
Comment on lines
6
to
121
| } | ||
|
|
||
| /// Returns the slot clock | ||
| pub fn slot_clock(&self) -> &S { | ||
| &self.slot_clock | ||
| } | ||
|
|
||
| /// Returns the slot duration | ||
| pub fn slot_duration(&self) -> Duration { | ||
| self.slot_clock.slot_duration() | ||
| } | ||
|
|
||
| /// Returns the number of slots per epoch | ||
| pub fn slots_per_epoch(&self) -> u64 { | ||
| self.slots_per_epoch | ||
| } | ||
|
|
||
| /// Estimates the current slot | ||
| pub fn estimated_current_slot(&self) -> Slot { | ||
| self.slot_clock.now().unwrap_or_default() | ||
| } | ||
|
|
||
| /// Estimates the slot at the given time | ||
| pub fn estimated_slot_at_time(&self, time: SystemTime) -> Slot { | ||
| let since_unix = time | ||
| .duration_since(SystemTime::UNIX_EPOCH) | ||
| .unwrap_or_default(); | ||
|
|
||
| self.slot_clock.slot_of(since_unix).unwrap_or_default() | ||
| } | ||
|
|
||
| /// Estimates the time at the given slot | ||
| pub fn estimated_time_at_slot(&self, slot: Slot) -> SystemTime { | ||
| let duration = self.slot_clock.start_of(slot).unwrap_or_default(); | ||
| SystemTime::UNIX_EPOCH + duration | ||
| } | ||
|
|
||
| /// Estimates the current epoch | ||
| pub fn estimated_current_epoch(&self) -> Epoch { | ||
| self.estimated_epoch_at_slot(self.estimated_current_slot()) | ||
| } | ||
|
|
||
| /// Estimates the epoch at the given slot | ||
| pub fn estimated_epoch_at_slot(&self, slot: Slot) -> Epoch { | ||
| Epoch::new(slot.as_u64() / self.slots_per_epoch) | ||
| } | ||
|
|
||
| /// Returns the first slot at the given epoch | ||
| pub fn first_slot_at_epoch(&self, epoch: u64) -> Slot { | ||
| Slot::new(epoch * self.slots_per_epoch) | ||
| } | ||
|
|
||
| /// Returns the start time of the given epoch | ||
| pub fn epoch_start_time(&self, epoch: u64) -> SystemTime { | ||
| self.estimated_time_at_slot(self.first_slot_at_epoch(epoch)) | ||
| } | ||
|
|
||
| /// Returns the start time of the given slot | ||
| pub fn get_slot_start_time(&self, slot: Slot) -> SystemTime { | ||
| self.estimated_time_at_slot(slot) | ||
| } | ||
|
|
||
| /// Returns the end time of the given slot | ||
| pub fn get_slot_end_time(&self, slot: Slot) -> SystemTime { | ||
| self.estimated_time_at_slot(slot + 1) | ||
| } | ||
|
|
||
| /// Checks if the given slot is the first slot of its epoch | ||
| pub fn is_first_slot_of_epoch(&self, slot: Slot) -> bool { | ||
| slot.as_u64() % self.slots_per_epoch == 0 | ||
| } | ||
|
|
||
| /// Returns the first slot of the given epoch | ||
| pub fn get_epoch_first_slot(&self, epoch: u64) -> Slot { | ||
| self.first_slot_at_epoch(epoch) | ||
| } | ||
|
|
||
| /// Returns the number of epochs per sync committee period | ||
| pub fn epochs_per_sync_committee_period(&self) -> u64 { | ||
| self.epochs_per_sync_committee_period | ||
| } | ||
|
|
||
| /// Estimates the sync committee period at the given epoch | ||
| pub fn estimated_sync_committee_period_at_epoch(&self, epoch: Epoch) -> Epoch { | ||
| epoch / self.epochs_per_sync_committee_period | ||
| } | ||
|
|
||
| /// Returns the first epoch of the given sync committee period | ||
| pub fn first_epoch_of_sync_period(&self, period: u64) -> u64 { | ||
| period * self.epochs_per_sync_committee_period | ||
| } | ||
|
|
||
| /// Returns the last slot of the given sync committee period | ||
| pub fn last_slot_of_sync_period(&self, period: u64) -> Slot { | ||
| let last_epoch = self.first_epoch_of_sync_period(period + 1) - 1; | ||
| // If we are in the sync committee that ends at slot x we do not generate a message | ||
| // during slot x-1 as it will never be included, hence -2. | ||
| self.get_epoch_first_slot(last_epoch + 1) - 2 | ||
| } | ||
| } |
There was a problem hiding this comment.
As the functionality of each function is minimal, I am not sure if this wrapper is worth it.
Comment on lines
64
to
67
| /// Estimates the epoch at the given slot | ||
| pub fn estimated_epoch_at_slot(&self, slot: Slot) -> Epoch { | ||
| Epoch::new(slot.as_u64() / self.slots_per_epoch) | ||
| } |
There was a problem hiding this comment.
There is no estimation happening in the function, why is this named estimated?
Comment on lines
69
to
72
| /// Returns the first slot at the given epoch | ||
| pub fn first_slot_at_epoch(&self, epoch: u64) -> Slot { | ||
| Slot::new(epoch * self.slots_per_epoch) | ||
| } |
Comment on lines
105
to
107
| pub fn estimated_sync_committee_period_at_epoch(&self, epoch: Epoch) -> Epoch { | ||
| epoch / self.epochs_per_sync_committee_period | ||
| } |
There was a problem hiding this comment.
There is Epoch::sync_committee_epoch
Comment on lines
116
to
119
| let last_epoch = self.first_epoch_of_sync_period(period + 1) - 1; | ||
| // If we are in the sync committee that ends at slot x we do not generate a message | ||
| // during slot x-1 as it will never be included, hence -2. | ||
| self.get_epoch_first_slot(last_epoch + 1) - 2 |
There was a problem hiding this comment.
you subtract 1, just to re-add 1 in the next statement.
Besides that, this seems to be unused.
| } | ||
|
|
||
| let msg_slot = Slot::new(consensus_message.height); | ||
| let randao_msg = false; // Default to false as in the Go code |
There was a problem hiding this comment.
# Conflicts: # anchor/client/src/lib.rs # anchor/message_receiver/src/manager.rs
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the duty validation logic for SSV messages to improve clarity and correctness, while also integrating a new duties management module.
- Updated validation functions to accept a generic SlotClock and DutiesProvider.
- Introduced a new duties module and duties_tracker for handling sync committee and proposer duties.
- Modified the Validator and related methods across message validation, sending, and receiving to propagate the new duties provider.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| anchor/message_validator/src/partial_signature.rs | Updated validation context to include a generic SlotClock; minor adjustments for new duty fields. |
| anchor/message_validator/src/lib.rs | Modified ValidationContext and Validator to include DutiesProvider; updated message validation functions accordingly. |
| anchor/message_validator/src/duties/mod.rs | Introduced new duties module with types and traits for duty management. |
| anchor/message_validator/src/duties/duties_tracker.rs | Added duties tracker to poll sync committee and proposer duties from beacon nodes. |
| anchor/message_validator/src/consensus_message.rs | Integrated duty-based validation logic within consensus messages; adjusted round and timing validations. |
| Other files (message_sender, message_receiver, database, client, etc.) | Updated generics and dependencies to accommodate the new duties provider integration. |
Comments suppressed due to low confidence (1)
anchor/message_validator/src/lib.rs:302
- Verify that substituting slot_clock with duties_provider in the call to validate_consensus_message fully preserves the original timing logic and that duties_provider properly encapsulates the necessary timing behavior.
self.duties_provider.clone(),
Comment on lines
524
to
526
| .unwrap_or_else(|| { | ||
| SystemTime::now() // Fallback if overflow occurs | ||
| }); |
There was a problem hiding this comment.
Using SystemTime::now() as a fallback in case of an overflow in the slot start time calculation may lead to inconsistent timing behavior; consider handling the overflow error explicitly.
Suggested change
| .unwrap_or_else(|| { | |
| SystemTime::now() // Fallback if overflow occurs | |
| }); | |
| .ok_or(ValidationFailure::DeadlineOverflow { slot })?; |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
diegomrsantos
commented
Apr 18, 2025
Comment on lines
+395
to
+396
| // - duty's starting slot + 34 (committee and aggregation) | ||
| // - duty's starting slot + 3 (other types) |
There was a problem hiding this comment.
@dknopik, any idea why these numbers?
diegomrsantos
commented
Apr 18, 2025
| let validator_index_count = validator_indices.len() as u64; | ||
| let slots_per_epoch_val = validation_context.slots_per_epoch; | ||
|
|
||
| // Skip duty search if validators * 2 exceeds slots per epoch |
There was a problem hiding this comment.
@Zacholme7 @dknopik, does this make sense? If the number of validators in the committee is >= slots per epoch, it doesn't mean one of them is in the sync committee. Additionally, do they use the Committee role for the sync committee? What's the difference to SyncCommittee?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
#161
Proposed Changes
This PR adds duty validation logic for SSV messages and integrates a new duties management module.
Additional Info
This is pending #277 and will be implemented in a future PR.