Skip to content

fix(alpen-ee): avoid invalid finalized forkchoice updates#1883

Open
peter941221 wants to merge 3 commits into
alpenlabs:mainfrom
peter941221:fix/alpen-ee-forkchoice-finalized
Open

fix(alpen-ee): avoid invalid finalized forkchoice updates#1883
peter941221 wants to merge 3 commits into
alpenlabs:mainfrom
peter941221:fix/alpen-ee-forkchoice-finalized

Conversation

@peter941221
Copy link
Copy Markdown

@peter941221 peter941221 commented May 30, 2026

Description

Fixes forkchoiceUpdated handling when OL reports a finalized EE hash that this Reth node still sees as non-canonical.

Before this change, crates/alpen-ee/engine/src/control.rs canonical-checked confirmed / safe but forwarded finalized unchanged. During sync or fork-switch windows, that could send a non-canonical finalized hash into forkchoiceUpdated, which Reth rejects as invalid forkchoice state.

This patch canonical-checks finalized_block_hash, clears it to B256::ZERO when it is inconsistent with the chosen fork, and keeps finalized when the update is switching to OL's fork and the finalized block is an ancestor of the new target head. On the engine side it also stops retrying deterministic forkchoice validation failures while still retrying communication-style failures.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Notes to Reviewers

The serialized interface is unchanged.

The key behavioral boundaries are:

  • finalized now drops to zero only when it is inconsistent with the chosen fork
  • a finalized block on the OL target fork stays preserved across the fork switch
  • ExecutionEngineError::ForkChoiceUpdate(...) no longer goes through the retry loop

AI was used to assist in this PR.

Is this PR addressing any specification, design doc or external reference document?

  • Yes
  • No

If yes, please add relevant links:

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added (where necessary) tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.
  • I have disclosed my use of AI in the body of this PR.

Related Issues

@peter941221 peter941221 marked this pull request as ready for review May 30, 2026 02:02
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5914edb14c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread crates/alpen-ee/engine/src/control.rs Outdated
@peter941221
Copy link
Copy Markdown
Author

peter941221 commented May 30, 2026

Re-ran the engine-side validation after the final test-only follow-up commit.

Environment

  • WSL2 temporary verification copy
  • normalized copied crates/**/ssz/* inputs to LF in the Linux copy only
  • branch head includes 5914edb test(alpen-ee): tighten forkchoice retry assertion

Commands

  1. cargo test -p alpen-ee-engine update_consensus_state_maps_invalid_state_to_forkchoice_error_without_retry -- --nocapture
  2. cargo test -p alpen-ee-engine update_consensus_state_retries_engine_unavailable_errors -- --nocapture
  3. cargo test -p alpen-ee-engine -- --nocapture

Results

  1. update_consensus_state_maps_invalid_state_to_forkchoice_error_without_retry: passed
  2. update_consensus_state_retries_engine_unavailable_errors: passed
  3. alpen-ee-engine package tests: 22 passed, 0 failed

The follow-up test change does not touch production logic. It only tightens the retry assertion from a sleep-and-abort shape to a bounded fail-once-then-succeed path with an exact attempt count.

@peter941221 peter941221 requested a review from prajwolrg as a code owner May 30, 2026 02:40
@peter941221
Copy link
Copy Markdown
Author

Addressed the finalized-handling bug on fork switches.

Problem

  • the previous patch evaluated finalized against the pre-update canonical view
  • when OL safe was non-canonical and this update switched head to that OL fork, a finalized block on the same OL fork could be dropped to zero even though it would become valid after the head switch

Change

  • when the update switches to OL's safe fork, keep finalized if it is an ancestor of the chosen target head on that fork
  • still drop finalized when it belongs to a different branch
  • kept the existing non-switching behavior for the current canonical fork

Validation

  • cargo test -p alpen-ee-engine forkchoice_keeps_noncanonical_finalized_when_switching_to_same_ol_fork -- --nocapture
  • cargo test -p alpen-ee-engine forkchoice_drops_finalized_from_different_noncanonical_fork -- --nocapture
  • cargo test -p alpen-ee-engine -- --nocapture

Results

  • both new targeted tests passed
  • alpen-ee-engine package tests: 24 passed, 0 failed

This required adding alloy-consensus as a direct dependency for alpen-ee-engine so the header trait used by the provider-backed parent walk is in scope in this crate.

@peter941221
Copy link
Copy Markdown
Author

peter941221 commented Jun 1, 2026

This branch is unchanged since the last test-only follow-up and the earlier review thread is resolved.

If someone wants another look, the latest head already includes the finalized fork follow-up and the package test rerun.

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.

1 participant