Skip to content

refactor(proof): always propose against finalized head, drop allow_non_finalized#3883

Open
xenoliss wants to merge 4 commits into
mainfrom
bo/proposer-finalized-only
Open

refactor(proof): always propose against finalized head, drop allow_non_finalized#3883
xenoliss wants to merge 4 commits into
mainfrom
bo/proposer-finalized-only

Conversation

@xenoliss

@xenoliss xenoliss commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Removes the allow_non_finalized flag from the proposer and always selects the finalized head for both L1 and L2.

Changes

  • Remove allow_non_finalized from CLI, ProposerConfig, DriverConfig, ProofDispatcherConfig, ProofRecoveryConfig, and all plumbing.
  • select_l1_head_for_target uses finalized_l1/finalized_l2 only; it errors if the target is above the finalized head, or if the finalized L1 head is below the finalized L2 origin.
  • Recovery, dispatch, and collection use the finalized L2 head; the boundary value and related logs are renamed safe_head -> finalized_head.
  • Metric: safe_head is kept as a deprecated alias and a new finalized_head gauge is added; both report the finalized head. safe_head is to be removed once monitors/dashboards are migrated.
  • Simplify the dispatcher test helper to build finalized-only sync statuses.

@cb-heimdall

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@xenoliss xenoliss force-pushed the bo/proposer-finalized-only branch from 42892aa to eff474c Compare July 8, 2026 15:17
@xenoliss xenoliss requested review from jackchuma and leopoldjoy and removed request for jackchuma July 8, 2026 18:51
};

Some((state, safe_head))
Some((state, finalized_head))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recovery should be bounded by finalized_head here; otherwise existing safe/non-finalized games can advance the cursor and later be used as proposal parents.

sync_status
}

fn headers_for_sync_status(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This helper seems heavier than the test needs now that only finalized_l1 is covered.

headers_for_sync_status() builds a valid header map, but the only caller immediately overwrites that same key to force the mismatch case. I think this can shrink to an inline HashMap::from([(sync_status.finalized_l1.hash, test_l1_header(sync_status.finalized_l1.hash, sync_status.finalized_l1.number + 1))]) inside the test

@xenoliss xenoliss requested review from jackchuma and leopoldjoy July 9, 2026 09:57
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Summary

Clean refactoring that removes the allow_non_finalized code path and enforces finalized-only semantics throughout the proposer. No correctness, safety, or concurrency issues found.

Key observations

  • Cache invalidation for rising finalized head (proof_recovery.rs:146-153): The new three-way short-circuit condition correctly handles the case where game_count is unchanged but finalized_head has advanced enough to cover previously-unseen games. The corresponding change from count > cached.game_count to count >= cached.game_count on line 156 is needed so the walk resumes from the cached tip in that scenario.

  • select_l1_head_for_target simplification (proof_dispatcher.rs:118-138): Properly collapsed from a multi-branch safe/finalized selection to finalized-only. Return type changed from (&'static str, L1BlockRef) to L1BlockRef — all call sites updated consistently.

  • Metric deprecation (metrics.rs, pipeline.rs): Both safe_head and finalized_head gauges are set in tandem during the transition period. The deprecated alias is clearly documented for future removal.

  • Test coverage: New test_recovery_walk_bounded_by_finalized_head test exercises the important scenario where finalized_head rises to cover existing games without any new game creation. Existing tests adapted correctly.

  • Minor nit (not blocking): test_recovery_forward_walk_stops_at_safe_head at proof_recovery.rs:487 still uses the old "safe_head" naming. This test was not modified by this PR and tests a different mechanism (max_safe_block mock field), but could be renamed in a follow-up for consistency.

No issues to block merge.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

✅ base-std fork tests: all 616 passed

base/base is fully in sync with the base-std spec.

Dependency Ref Commit
base-std main 4658f1b7
base-anvil 0092692587d8d064dd2c6923ce26a682c58f3694 00926925

@xenoliss xenoliss enabled auto-merge July 9, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants