feat: fast confirmation rule#8837
Conversation
Summary of ChangesHello @nazarhussain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: an assumption-based Fast Confirmation Rule (FCR) for the beacon node. The primary goal is to accelerate the confirmation of blocks by implementing a sophisticated algorithm that evaluates various network and validator-related metrics. This involves deep integration into the existing fork choice mechanism, providing a new configurable option for users, and updating how 'safe' blocks are identified within the system. The changes are comprehensive, spanning core logic, configuration, state management, and observability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fast confirmation rule, an experimental feature designed to provide faster block confirmations. The changes are comprehensive, including the core logic in a new FastConfirmationRule class, configuration options to enable it, and integration into the existing ForkChoice and safe block determination logic. The implementation appears to align well with the provided specification. My review includes a couple of minor suggestions to enhance code quality and readability.
Performance Report✔️ no performance regression detected Full benchmark results
|
|
Hey @nazarhussain, we chatted about running FCR tests at the meeting today. This is how you can generate these tests:
This will write the tests to Note: There is currently a failing test with Gloas, which sort of breaks test generation. So you could generate the reference tests for a specific fork instead, something like the following. After Mikhail fixes the issue, you can use the command above. |
nazarhussain
force-pushed
the
feature/fast-confirmation
branch
from
10a6c01 to
8a1a41f
Compare
nazarhussain
force-pushed
the
feature/fast-confirmation
branch
from
77c0c84 to
47f2f6b
Compare
|
@nflaig Related to your feedback earlier. I checked that the fast confirmation specs are generated as |
twoeths
left a comment
twoeths
left a comment
There was a problem hiding this comment.
need to remove CheckpointWithPayloadStatus because we removed it for gloas due to ethereum/consensus-specs#5094
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Multi-persona review — PR #8837 (feat: fast confirmation rule)
🤖 Generated with AI assistance: multi-persona review fan-out (architect, wisdom, devil's advocate, security, bugs). Findings consolidated by @lodekeeper. Reviewed commit:
8066f6b.
@nazarhussain — large but well-structured PR. The algorithm itself looks spec-aligned; the structural feedback below is mostly about API surface, opt-in scope, and code organization. No security findings. Bugs reviewer timed out, so the bug-class is under-covered — flagging that explicitly.
TL;DR
| Reviewer | Verdict |
|---|---|
| Security | No issues |
| Architecture | 2 coupling concerns (fork-choice ↔ state-view, store interface bloat) |
| Devil's Advocate | RECONSIDER — 3 structural pushbacks (API surface, opt-in scope, PR shape) |
| Wisdom | 12 maintainability findings + 5 smaller notes |
| Bugs |
🔴 Structural concerns (devil's advocate)
These are the heaviest items — worth deciding on before merge, not as follow-ups.
1. Public API mismatch with the merged cross-client standard
- This PR adds
GET /eth/v1/lodestar/fast_confirmation_info(polling, Lodestar-namespaced) and the docs position it as the consumer-facing API for bridges/exchanges/custodians. - The merged cross-client standard is
beacon-APIs#598(merged 2026-04-20) — afast_confirmationSSE event on/eth/v1/eventswith fields{block, slot}.beacon-APIs#611(open since 2026-05-22) refines same-slot dedup. - Nimbus already implemented the standardized event in
nimbus-eth2#8479(merged 2026-05-20). - The PR's own docs concede the divergence: "Consumers should not assume that every Ethereum client exposes the same signal or the same API."
Of the 4 fields returned by the polling endpoint, 3 are already available via existing standard endpoints (/eth/v1/beacon/headers/head, /eth/v1/beacon/states/{state_id}/finality_checkpoints). Only confirmed is new. Also: when --chain.fastConfirmation is off, getConfirmedRoot() returns the justified root, so the confirmed field becomes silently misleading (renamed-justified).
Counter-proposal: Implement the standardized fast_confirmation SSE event on /eth/v1/events to match beacon-APIs#598. If a debug/observability polling endpoint is still wanted, ship it explicitly namespaced as Lodestar-internal in the docs and consider deferring it to a follow-up.
2. "Opt-in / experimental" claim is half-true
The --chain.fastConfirmation flag only gates rule execution. The supporting surface lands unconditionally:
| Surface | Gated by flag? |
|---|---|
IForkChoiceStore extends IFastConfirmationStore (9 new fields) |
❌ schema change for every store |
ForkChoiceStore ctor reads finalized state + initializes 9 FCR fields |
❌ runs unconditionally |
stateGetter as required ctor argument on every ForkChoice construction |
❌ always wired |
/eth/v1/lodestar/fast_confirmation_info registration |
❌ endpoint always responds |
safeBlocks.ts::getSafeBeaconBlockRoot / getSafeExecutionBlockHash routing |
❌ now reads getConfirmedRoot() first (falls back to justified, but code path differs from baseline) |
Compare Lighthouse sigp/lighthouse#8951: "I have implemented this without touching any production code."
Counter-proposal — pick a side:
- A — Commit fully: drop the flag, ship on by default. Spec is merged, supporting surface is already always-on.
- B — Actually isolate (recommended): keep the flag and make every checked row above gated — sub-store composition, optional
stateGetter, conditional route registration, baseline-identicalsafeBlocks.tswhen disabled.
3. Single 4549-line PR vs. Nimbus-style stack
- Nimbus: ~30 incremental PRs Feb–May 2026. Still merging spec follow-ups —
nimbus-eth2#8512("Ensure confirmed chain includes greatest unrealized checkpoint") merged 2026-05-26, 10 days after spec merge. - Lighthouse: single-PR shape, WIP for 3 months.
- This PR follows the Lighthouse shape and will pay the same costs: review depth on 4 consensus-critical rules in a 4549-line diff, rebase cost across 44 files touching shared interfaces, spec-sync cost when Nimbus-style follow-ups land, bisect cost if a consensus bug surfaces.
Counter-proposal — 5-PR stack:
- Config + types + spec test runner (~600 lines) — establishes spec-correctness gate immediately
- Pure helpers (~1000 lines) —
fastConfirmation/utils.tsminus glue - Snapshot + rules (~500 lines) — the algorithm
- ForkChoice wiring (~300 lines) —
runFastConfirmation, store schema (ideally as the optional sub-store from #2) - Public surface (~200 lines + dashboard) — SSE event, docs
(I realize a restructure-before-merge ask is a big lift on an already-large PR. If the team's call is "land as-is, fix in follow-ups," at least #1 (API surface) should be settled here so we don't ship a Lodestar-only consumer API that we'd then deprecate.)
🟡 Architecture concerns
A1. Fork-choice now reaches through a full state-transition state view
Files: packages/fork-choice/src/forkChoice/fastConfirmation/{types,utils}.ts, packages/beacon-node/src/chain/chain.ts, packages/state-transition/src/stateView/{interface,beaconStateView}.ts
@lodestar/fork-choice now consumes IBeaconStateView through ForkChoiceStateGetter and calls processSlots(), committee lookup, shuffling access, validator access, and balance extraction. The state-view interface is widened to satisfy these fork-choice calls.
Impact: Fork-choice becomes coupled to the full BeaconStateView surface and to beacon-node's state-cache lifetime semantics. Future state-view or cache changes can break fast confirmation even when fork-choice DAG/vote contracts remain valid.
Recommendation: Keep FCR orchestration in fork-choice, but move state-derived calculations behind a narrow adapter. Have beacon-node/state-transition provide primitive balance / active-validator / committee data through explicit callbacks, without exposing IBeaconStateView or processSlots() to fork-choice.
A2. Optional FCR state merged into core fork-choice store contract
Files: packages/fork-choice/src/forkChoice/store.ts, packages/fork-choice/src/forkChoice/fastConfirmation/types.ts, packages/fork-choice/src/index.ts
IForkChoiceStore now extends IFastConfirmationStore. ForkChoiceStore requires a stateGetter ctor argument. All FCR fields are initialized on every store even when the feature is disabled. The package root exports several FCR internals (FCRBalanceSource, FCRContext, IFCRStore, ...).
Impact: Every fork-choice consumer, mock, and test must now model FCR fields even when disabled. Experimental rule's implementation shape becomes part of the broader package contract.
Recommendation: Compose FastConfirmationStore as a separate, optionally-owned store. Keep IForkChoiceStore focused on canonical fork-choice state. Expose only stable public methods (getConfirmedRoot(), getConfirmedBlock()). Avoid root-level exports of FCR internals.
This pairs with Devil's-Advocate #2 — solving the "actually isolate" Option B naturally resolves A2.
🟢 Readability / maintainability (wisdom — 12 findings)
Pick the ones that resonate; none are merge blockers individually.
-
loop1Condition/loop2ConditioninfindLatestConfirmedDescendant—packages/fork-choice/src/forkChoice/fastConfirmation/utils.ts:3184-3300. The logger already uses the better names ("Fast confirmation previous-epoch loop ..."/"... current-epoch loop ..."). Rename toshouldAdvanceThroughPreviousEpoch/shouldAdvanceThroughCurrentEpoch, optionally extract each loop into a helper so the top-level reads as a 4-5 line orchestration. -
Stop encoding tuples into string keys in
getCurrentTargetScore—utils.ts:2999-3024. Uses`${msg.root}:${msg.epoch}`thenlastIndexOf(":")+Number(...)to parse back. Elsewhere in the same module the order is reversed (`${epoch}:${rootHex}`). UseMap<RootHex, Map<Epoch, number>>instead — eliminates parse round-trip, removes the fragilelastIndexOfinvariant. -
Cache map keyed by generic
stringinstead of literal union —types.ts:2298,utils.ts:2667-2723.voteWeightBySource: Map<string, ...>with a doc comment saying"current" | "previous". Let the type system enforce it:type BalanceSourceKey = "current" | "previous". Currently a typo'd"latest"would silently miss the cache. -
Duplicated branches in
ensureVoteMaps—utils.ts:2681-2700. Two near-identical loops differ only by iteration source and slashed-check. Compute iteration set + per-index "is slashed?" predicate up front, run one loop. Cuts ~20 lines, future exclusions only need to be added once. -
confirmed_resetreason loses cause —rules.ts:2129-2152. Three distinct conditions (confirmedEpochBehindHead,notAncestorOfHead,allChildrenNotConfirmed) collapse into a singlereason: "confirmed_reset". Emit which guard fired (reset_behind/reset_not_ancestor/reset_chain_unsafe) or include flags in metadata — first question on a prod reset is "which of the three?". -
Repeated neutral-threshold object literal —
utils.ts:2884-2904. Two early-return paths incomputeSafetyThresholdproduce the same{threshold: POSITIVE_INFINITY, proposerScore: 0, ...}. Name the concept:SAFETY_THRESHOLD_UNREACHABLE(frozen) → return from both early exits. -
Optional chaining on a required interface method —
packages/fork-choice/src/forkChoice/safeBlocks.ts:3500, 3520.IForkChoicedeclaresgetConfirmedRoot(): RootHexas required (interface.ts:3477), butsafeBlocks.tscallsfc.getConfirmedRoot?.(). Drop the?.. If tests pass partial mocks, fix the mocks. -
FCR*re-export aliases —packages/fork-choice/src/index.ts:3617-3627. Internal names areFastConfirmation*; public re-export renames them toFCR*(FCRContext,FCRResult,IFCRStore, ...). Pick one — two names for one concept will fragmentgrepand rename refactors. -
Dead
decision.stopfield —types.ts:2275-2280,rules.ts:2213-2216.FastConfirmationDecision.stop?: booleandeclared and honored withif (decision.stop) break;, but no rule ever sets it. Remove, or document the early-exit hook as part of the public rule API. -
getSupportDiscountis a transparent passthrough —utils.ts:2861-2869.getSupportDiscount(...)does nothing but callcomputeEmptySlotSupportDiscount(...)with the same args. Pick one name (or inline at the single caller). -
COMMITTEE_WEIGHT_ESTIMATION_ADJUSTMENT_FACTOR = 5—utils.ts:2352. Constant name is good but no spec citation for the5. Add a spec link/section ("rounded-up safety margin from<spec> §X"). Same applies to the999ceiling-division trick in the same function. -
Empty meta on FCR failure warning —
packages/fork-choice/src/forkChoice/forkChoice.ts:3414.this.logger?.warn("Fast confirmation failed", {}, err as Error). Include slot/head/confirmedRootin meta so an operator can act without correlating with surrounding logs.
Smaller notes
utils.ts:2354-2361etc. — "has → get ?? default" caching dance repeated 5× (getBlock,getAncestorRoots,getCheckpointState,getSlotCommittee,isDescendantCached). Useconst cached = cache.x.get(key); if (cached !== undefined) return cached;to skip the double-hash.utils.ts:2396-2402,2453-2457—try { ctx.getAncestor(...) } catch { return null }— add a one-line comment to make the catch intentional rather than apologetic.fastConfirmationRule.ts:2027-2041—updateFastConfirmationMetricssetsconfirmedSlot/confirmedEpochto0when block is missing.0is a real slot/epoch; considerNaN(Prometheus drops it) or skipping.set.packages/utils/src/metrics.ts:5260-5276—EndTimer = (() => number) | undefinedis a surprising shape for the type alias; consider moving the optionality to the parameter site.forkChoice.ts:3456-3463—getTrackedVotesCountdoes a linear scan ofvoteNextIndiceseach slot. If call frequency increases, maintain a counter alongside the array.
🟢 Security — clean
Reviewed: new endpoint, FCR execution timing & lifecycle, fastConfirmation/*, related safeBlocks.ts changes. Focus on DoS/resource usage, trust boundaries (latest messages, equivocations, execution-status), validator-balance arithmetic, adversarial vote handling.
- Endpoint is read-only, comparable to existing chain-status surfaces.
- Disabled by default; rebuild runs on per-slot tick when enabled, not per gossip message.
- Vote/committee scans use bounded consensus data structures; locally-known equivocators excluded; execution status checked before treating block as one-confirmed.
- Balance arithmetic in effective-balance increments; well within JS safe-integer limits.
⚠️ Bugs review — TIMED OUT
The bug-class reviewer hit a 50-tool-call timeout and a re-spawn aborted. The other reviews touched correctness incidentally (e.g. wisdom #2 flagging the lastIndexOf invariant) but no dedicated bug pass landed. Given the 4549-line diff and that the rules gate safe_block_hash in engine API, I'd recommend either a targeted manual bug-class pass on fastConfirmation/{rules,utils,data}.ts before merge, or an explicit acknowledgement that bug coverage here came from existing reviewer attention rather than a fresh dedicated pass.
Reports are on disk at ~/.openclaw/workspace/notes/review-reports/pr-8837-*.md if anyone wants the unedited per-reviewer output. Happy to dig deeper on any specific item.
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Inline-comment batch supplementing the earlier structural review (#8837 (review)). These are the file:line-anchored findings from the wisdom + architecture passes. None are individual merge blockers; the structural pushbacks remain the main asks.
🤖 Generated with AI assistance.
| const loop1Condition = | ||
| confirmedEpoch !== null && | ||
| confirmedEpoch + 1 === currentEpoch && | ||
| previousSlotVotingSource !== null && | ||
| previousSlotVotingSource.epoch + 2 >= currentEpoch && | ||
| (isStartSlotOfEpoch(snapshot.currentSlot) || | ||
| (willNoConflictingCheckpointBeJustified(ctx, store, cache) && | ||
| ((prevSlotJustification !== null && prevSlotJustification.epoch + 1 >= currentEpoch) || | ||
| (headJustification !== null && headJustification.epoch + 1 >= currentEpoch)))); |
There was a problem hiding this comment.
Self-documenting names for the descendant-search loops. The logger already uses the better names ("Fast confirmation previous-epoch loop ..." and "... current-epoch loop ..."). Consider renaming loop1Condition / loop2Condition to shouldAdvanceThroughPreviousEpoch / shouldAdvanceThroughCurrentEpoch so the top-level reads as a 4–5 line orchestration. Optionally extract each loop body into a small helper.
Why: loop1 / loop2 forces every reader to scroll the body to discover what each branch does.
| const groupKey = `${msg.root}:${msg.epoch}`; | ||
| voteGroups.set(groupKey, (voteGroups.get(groupKey) ?? 0) + weight); | ||
| } | ||
|
|
||
| // Check each unique vote group's checkpoint against the target | ||
| const targetKey = `${target.epoch}:${target.rootHex}`; | ||
| let score = 0; | ||
| for (const [groupKey, weight] of voteGroups) { | ||
| const sepIdx = groupKey.lastIndexOf(":"); | ||
| const root = groupKey.slice(0, sepIdx); | ||
| const epoch = Number(groupKey.slice(sepIdx + 1)) as Epoch; |
There was a problem hiding this comment.
Stop encoding tuples into string keys. This uses `${msg.root}:${msg.epoch}` then parses it back with lastIndexOf(":") + Number(...). Elsewhere in the same module the order is reversed (`${epoch}:${rootHex}`).
Suggested:
const voteGroups = new Map<RootHex, Map<Epoch, number>>();
// ...
for (const [root, byEpoch] of voteGroups) {
for (const [epoch, weight] of byEpoch) {
const cp = getCheckpointForBlock(ctx, root, epoch);
if (cp && cp.epoch === target.epoch && cp.rootHex === target.rootHex) score += weight;
}
}Why: eliminates a class of bugs (key-order mismatches), removes the manual parse, and avoids relying on the unspoken invariant that RootHex never contains :.
| committeeBySlot: Map<Slot, Set<ValidatorIndex>>; | ||
| isDescendantByRootPair: Map<string, boolean>; | ||
| /** voteRoot -> totalWeight, keyed by sourceKey ("current" | "previous") */ | ||
| voteWeightBySource: Map<string, Map<RootHex, number>>; |
There was a problem hiding this comment.
Cache map keyed by generic string instead of the literal union. The call sites already use "current" | "previous" (see utils.ts:341). Encoding it in the type avoids silent cache misses on a typo'd key:
export type BalanceSourceKey = "current" | "previous";
voteWeightBySource: Map<BalanceSourceKey, Map<RootHex, number>>;| if (activeIndices !== null && state) { | ||
| for (const i of activeIndices) { | ||
| if (state.getValidator(i).slashed) continue; | ||
| if (equivocating.has(i)) continue; | ||
| const msg = ctx.getLatestMessage(i); | ||
| if (!msg) continue; | ||
| const weight = balances[i] ?? 0; | ||
| if (weight === 0) continue; | ||
| voteMap.set(msg.root, (voteMap.get(msg.root) ?? 0) + weight); | ||
| } | ||
| } else { | ||
| for (let i = 0; i < balances.length; i++) { | ||
| const weight = balances[i] ?? 0; | ||
| if (weight === 0) continue; | ||
| if (equivocating.has(i)) continue; | ||
| const msg = ctx.getLatestMessage(i); | ||
| if (!msg) continue; | ||
| voteMap.set(msg.root, (voteMap.get(msg.root) ?? 0) + weight); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated branches in ensureVoteMaps. The two loops differ only by (a) iteration source (activeIndices vs. 0..balances.length) and (b) whether state.getValidator(i).slashed is checked. Compute both up front and run a single loop:
const indices: Iterable<number> = activeIndices ?? balances.keys();
const isSlashed = state ? (i: number) => state.getValidator(i).slashed : () => false;
for (const i of indices) {
if (isSlashed(i)) continue;
if (equivocating.has(i)) continue;
const w = balances[i] ?? 0;
if (w === 0) continue;
const m = ctx.getLatestMessage(i);
if (!m) continue;
voteMap.set(m.root, (voteMap.get(m.root) ?? 0) + w);
}Future exclusions then only need to be added once.
|
|
||
| if (confirmedEpochBehindHead || notAncestorOfHead || allChildrenNotConfirmed) { | ||
| const didReset = decision.didReset || decision.confirmedRoot !== snapshot.finalizedRoot; | ||
| return {confirmedRoot: snapshot.finalizedRoot, didReset, reason: "confirmed_reset"}; |
There was a problem hiding this comment.
confirmed_reset reason loses the actual cause. This rule has three distinct guards (confirmedEpochBehindHead, notAncestorOfHead, allChildrenNotConfirmed) that all collapse into a single reason: "confirmed_reset". When an operator sees a reset in production, the first question is "which of the three?".
Either emit a discriminant (reset_behind / reset_not_ancestor / reset_chain_unsafe) or include the three boolean flags in the metadata so post-incident analysis doesn't need a local reproduction with verbose logging.
| const result = this.fastConfirmationRule.onSlotStartAfterPastAttestationsApplied(this.fastConfirmationContext); | ||
| this.fcStore.confirmedRoot = result.confirmedRoot; | ||
| } catch (err) { | ||
| this.logger?.warn("Fast confirmation failed", {}, err as Error); |
There was a problem hiding this comment.
Empty meta object on the FCR failure warning. If this ever fires in production, the empty {} forces the operator to correlate the warn with surrounding logs just to learn the slot. Two extra fields make every report self-contained:
this.logger?.warn(
"Fast confirmation failed",
{slot: this.fcStore.currentSlot, head: this.head.blockRoot, confirmedRoot: this.fcStore.confirmedRoot},
err as Error,
);| this.metrics.fastConfirmation.confirmedSlot.set(0); | ||
| this.metrics.fastConfirmation.confirmedEpoch.set(0); |
There was a problem hiding this comment.
confirmedSlot=0 / confirmedEpoch=0 is ambiguous on a missing block. 0 is a real slot/epoch, so a dashboard reader can't distinguish "no confirmed block" from "confirmed block at slot 0". Either set Number.NaN (Prometheus drops NaN so the gauge keeps its last value), or skip the .set so the gauge stays at its last value explicitly.
| static<Labels extends LabelsGeneric = NoLabels>(config: StaticConfig<Labels>): void; | ||
| } | ||
|
|
||
| export type EndTimer = (() => number) | undefined; |
There was a problem hiding this comment.
EndTimer = (() => number) | undefined is a surprising shape for a type alias. Optionality usually belongs at the parameter site, not baked into the type. Consider renaming to MaybeEndTimer, or making EndTimer = () => number and declaring withObservedDuration(endTimer: EndTimer | undefined, ...).
Why: readers of an EndTimer value reasonably expect they can call it directly without a null check.
| }), | ||
| getFinalizedCheckpoint: () => this.fcStore.finalizedCheckpoint, | ||
| getEquivocatingIndices: () => this.fcStore.equivocatingIndices, | ||
| getTrackedVotesCount: () => { |
There was a problem hiding this comment.
getTrackedVotesCount does a linear scan of voteNextIndices each slot. Cheap today, but if the call frequency increases (e.g. exposed via metrics on every slot), prefer maintaining a counter alongside the array.
| * - `time` is represented using `Slot` instead of UNIX epoch `u64`. | ||
| */ | ||
| export interface IForkChoiceStore { | ||
| export interface IForkChoiceStore extends IFastConfirmationStore { |
There was a problem hiding this comment.
Architect concern — optional FCR state is merged into the core fork-choice store contract. IForkChoiceStore extends IFastConfirmationStore makes every fork-choice consumer, mock, and test model 9 FCR fields even when --chain.fastConfirmation is off. The ctor also reads finalized state via stateGetter (line 104) and initializes all FCR fields unconditionally.
Combined with the FCR* root-level exports in packages/fork-choice/src/index.ts, an experimental rule's implementation shape becomes part of the broader fork-choice package contract.
Recommendation: Compose FastConfirmationStore as a separate, optionally-owned store. Keep IForkChoiceStore focused on canonical fork-choice state. Expose only stable public methods (getConfirmedRoot(), getConfirmedBlock()). This also resolves the devil's-advocate "actually isolate" sub-point in the structural review.
|
@lodekeeper Most of the inline comments are handled in recent commit. Following still open, see my responses. getTrackedVotesCount State getter coupling Optional FCR store isolation: SSE event vs Lodestar specific endpoint |
|
Thanks for the responses, quick acks + one ask:
|
|
@lodekeeper Here are the followup issues #9438 #9437 |
|
@lodekeeper If all points are addressed please approve the RP. :) |
|
Walking through my points against the current head (`9a0c038` / `500226c2d8` "as per feedback"): Inline — all resolved in `500226c2`:
Issue-level — deferred but tracked:
From my review surface every point I raised is addressed or tracked. One caveat I want to repeat from the original review (`pullrequestreview-4395743466`): the bugs-class reviewer timed out and I flagged "recommend a focused bug-class pass before merge" — that part hasn't happened yet, just noting. On the formal APPROVE — I'm staying with a COMMENTED review here (that's the lodekeeper pattern across PRs #9350 / #9332 / #9317 / #9430 / #9334). The merge gate is better held by maintainer review — Nico's "did you confirm on mainnet? consistent with Prysm?" question + twoeths' `onFastConfirmation` integration discussion are the active ones from my side. |
5 participants
Motivation
Introduce assumption based fast confirmation rule.
Description
Specs: ethereum/consensus-specs#4747