Add Fast Confirmation Rule (FCR) implementation#656
Add Fast Confirmation Rule (FCR) implementation#656bomanaps wants to merge 4 commits intograndinetech:developfrom
Conversation
There was a problem hiding this comment.
Haven't reviewed everything yet, but some places need major refactoring. Specifically, the biggest issue is that FCR lives in forkchoice store, but probably would be better living on its own.
Also, I don't see spec tests updated - those are required to be wired & passing, before merging
UPD: looks like formatting check was failing - all lints also need to be passing, before merging. For convenience, you can run them locally, with ./scripts/ci/clippy.bash --deny warnings
| }; | ||
|
|
||
| /// Assumed maximum percentage of Byzantine validators among the validator set. | ||
| pub const CONFIRMATION_BYZANTINE_THRESHOLD: u64 = 25; |
There was a problem hiding this comment.
looks like this thing should be in config, as per spec?
| mod blob_cache; | ||
| mod data_column_cache; | ||
| mod error; | ||
| pub mod fast_confirmation; |
There was a problem hiding this comment.
don't think this should be pub mod - probably mod, and re-export needed members via pub use at the top of file
|
|
||
| /// Enable the Fast Confirmation Rule for single-slot block confirmation | ||
| #[clap(long)] | ||
| fast_confirmation_rule: bool, |
There was a problem hiding this comment.
this probably should go into either chain_options, or beacon_node_options
| // ========================================================================= | ||
| // Fast Confirmation Rule (FCR) — spec: fast-confirmation.md | ||
| // ========================================================================= |
There was a problem hiding this comment.
please remove this comment
| /// spec: `get_equivocation_score(store, state, from_slot, to_slot)` | ||
| /// | ||
| /// Sums the active balances of validators that both appear in a committee | ||
| /// between `from_slot..=to_slot` AND are in `self.equivocating_indices`. | ||
| /// | ||
| /// Callers are responsible for the short-circuit when `equivocating_indices` is empty. |
There was a problem hiding this comment.
Please format comments as others in this file, i.e.:
grandine/fork_choice_store/src/store.rs
Lines 1863 to 1867 in 4607b7a
| // === Fast Confirmation Rule (FCR) fields === | ||
| // Only meaningful when `store_config.fast_confirmation_rule` is enabled. | ||
| // spec: `confirmed_root` | ||
| fcr_confirmed_root: H256, | ||
| // spec: `previous_epoch_observed_justified_checkpoint` | ||
| fcr_prev_obs_justified: Checkpoint, | ||
| // spec: `current_epoch_observed_justified_checkpoint` | ||
| fcr_curr_obs_justified: Checkpoint, | ||
| // spec: `previous_epoch_greatest_unrealized_checkpoint` | ||
| fcr_prev_gu_checkpoint: Checkpoint, | ||
| // spec: `previous_slot_head` | ||
| fcr_prev_slot_head: H256, | ||
| // spec: `current_slot_head` | ||
| fcr_curr_slot_head: H256, | ||
| // Cached result of compute_honest_ffg_support_for_current_target, refreshed each slot. | ||
| // Avoids redundant validator-set scans when the FFG gate is queried multiple times. | ||
| fcr_honest_ffg_support: Gwei, | ||
| // Cached total active balance from the current balance source, refreshed each slot. | ||
| fcr_ffg_total_active_balance: Gwei, |
There was a problem hiding this comment.
I think this is not the place for those fields. AFAIK, the FCR shouldn't affect forkchoice logic. For this purpose, the spec defines separate FCR store, that has only readonly access to fork-choice store. https://github.com/ethereum/consensus-specs/blob/master/specs/phase0/fast-confirmation.md#fastconfirmationstore
The best solution would probably be to split FCR into its own store
| @@ -1030,11 +1059,56 @@ impl<P: Preset, S: Storage<P>> Store<P, S> { | |||
| /// [`get_safe_execution_payload_hash`](https://github.com/ethereum/consensus-specs/blob/v1.3.0/fork_choice/safe-block.md#get_safe_execution_payload_hash) | |||
There was a problem hiding this comment.
this link needs to be updated - currently it points to an outdated spec version
| if let Some(hash) = self | ||
| .chain_link(self.fcr_confirmed_root) | ||
| .and_then(ChainLink::execution_block_hash) | ||
| { | ||
| return hash; | ||
| } |
There was a problem hiding this comment.
that doesn't look right - it shouldn't fallback to justified block root if confirmed root not found. You should probably jsut do the .unwrap_or_default(), instead of pattern matching
| /// When `store_config.fast_confirmation_rule` is enabled, this is the FCR-confirmed root. | ||
| /// Otherwise falls back to the justified checkpoint root. | ||
| #[must_use] | ||
| pub fn confirmed_root(&self) -> H256 { |
There was a problem hiding this comment.
why confirmed root falls back to justified checkpoint? I think there is no such thing as "confirmed root", outside of FCR context?
There was a problem hiding this comment.
also noticed that sometimes the finalized root is treated as confirmed, so probably justified root is too weak assumption there
| /// Returns `store.current_epoch_observed_justified_checkpoint`. | ||
| #[must_use] | ||
| pub fn fcr_curr_obs_justified(&self) -> Checkpoint { | ||
| self.fcr_curr_obs_justified | ||
| } | ||
|
|
||
| /// Returns `store.previous_epoch_greatest_unrealized_checkpoint`. | ||
| #[must_use] | ||
| pub fn fcr_prev_gu_checkpoint(&self) -> Checkpoint { | ||
| self.fcr_prev_gu_checkpoint | ||
| } | ||
|
|
||
| /// Returns `store.previous_slot_head`. | ||
| #[must_use] | ||
| pub fn fcr_prev_slot_head(&self) -> H256 { | ||
| self.fcr_prev_slot_head | ||
| } | ||
|
|
||
| /// Returns `store.current_slot_head`. | ||
| #[must_use] | ||
| pub fn fcr_curr_slot_head(&self) -> H256 { | ||
| self.fcr_curr_slot_head | ||
| } |
There was a problem hiding this comment.
don't think all of those should be public?
|
While running the FCR spec-test vectors, I hit two divergences that appear to live in pre-existing in our code (not introduced by the FCR PR), and I would appreciate a sanity-check before assuming am misreading the spec (1) accessors::relative_epoch (helper_functions/src/accessors.rs:103-114) rejects epoch lookups more than ±1 from the state's current epoch, but the FCR spec note for get_slot_committee says implementations "MUST support committees of epochs starting from current_epoch − 2" is this an intentional limitation, or could the helper handle older epochs from the state's RANDAO history? (2) cl.unrealized_justified_checkpoint.epoch (computed via combined::process_justification_and_finalization at store.rs:1281-1292) appears to diverge from the spec's expected store.unrealized_justifications[block].epoch on multi-fork chains the test_fcr_previous_epoch_012 parameter block_vs_fresh=False implies the spec expects 0 for these prev-epoch blocks, but I seem to compute ≥1, which makes Loop 2's final gate over-advance confirmed_root could you point me at how the spec's compute_pulled_up_tip semantics should map onto our pulled-up path? Any pointers would be much appreciated. |
2 participants
Implements the Fast Confirmation Rule (FCR) for the fork choice store, including the confirmation logic, chain safety checks, FFG data computation, and HTTP API integration for the safe block ID. Adds FCR-specific tests covering confirmation advancement, staleness reversion, and the disabled fallback path.
ethereum/consensus-specs#4747