Gloas payload bid consensus#8801
Conversation
michaelsproul
added
ready-for-review
consensus/state_processing/src/per_block_processing/signature_sets.rs
Outdated
Show resolved
Hide resolved
consensus/state_processing/src/per_block_processing/signature_sets.rs
Outdated
Show resolved
Hide resolved
dapplion
left a comment
dapplion
left a comment
There was a problem hiding this comment.
Spec comparison review — compared against consensus-specs/specs/gloas/beacon-chain.md and potuz's annotated GLOAS spec.
|
All comments reviewed, thanks gang |
| impl Builder { | ||
| /// Check if a builder is active in a state with `finalized_epoch`. | ||
| /// | ||
| /// This implements `is_active_builder` from the spec. | ||
| pub fn is_active_at_finalized_epoch(&self, finalized_epoch: Epoch, spec: &ChainSpec) -> bool { | ||
| self.deposit_epoch < finalized_epoch && self.withdrawable_epoch == spec.far_future_epoch | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: the spec calls this is_active_builder. Would it be worth renaming to is_active_builder for easier cross-referencing? The method-on-Builder approach is fine to avoid a redundant list lookup.
There was a problem hiding this comment.
I thought about this, but I figure including builder in the name is redundant when it's a method on builder, and putting finalized_epoch in the name makes it clear at the call site that you must pass the finalized epoch and not e.g. the current epoch
| /// Check if a builder is active in a state with `finalized_epoch`. | ||
| /// | ||
| /// This implements `is_active_builder` from the spec. | ||
| pub fn is_active_at_finalized_epoch(&self, finalized_epoch: Epoch, spec: &ChainSpec) -> bool { |
There was a problem hiding this comment.
Should this be named is_active_builder to match spec naming?
There was a problem hiding this comment.
Farming comment credits? Isn't this the same as your other comment? #8801 (comment)
michaelsproul
added
ready-for-merge
Merge Queue StatusRule:
This pull request spent 29 minutes 45 seconds in the queue, including 27 minutes 36 seconds running CI. Required conditions to merge
|
4 participants
Proposed Changes
block_header-- no changes required).