Skip to content

fix(alpen-ee): preserve newPayload status semantics#1884

Open
peter941221 wants to merge 3 commits into
alpenlabs:mainfrom
peter941221:fix/alpen-ee-new-payload-status
Open

fix(alpen-ee): preserve newPayload status semantics#1884
peter941221 wants to merge 3 commits into
alpenlabs:mainfrom
peter941221:fix/alpen-ee-new-payload-status

Conversation

@peter941221
Copy link
Copy Markdown

@peter941221 peter941221 commented May 30, 2026

Description

Stops submit_payload() from flattening every successful new_payload(...) transport round-trip into Ok(()).

Before this change, Alpen EE discarded the returned PayloadStatus, so Invalid and Syncing never surfaced as engine-level outcomes. This patch keeps the retry loop but maps the status explicitly: Valid succeeds, Invalid returns ExecutionEngineError::InvalidPayload, Syncing returns ExecutionEngineError::EngineSyncing, and Accepted returns success.

That keeps Accepted as a non-fatal outcome in this Result<(), ExecutionEngineError> path instead of retrying the same payload until the retry budget expires.

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 production change is the payload-status mapping in crates/alpen-ee/engine/src/engine.rs.

The current branch also locks the retry boundary in tests:

  • invalid payload returns immediately
  • syncing stays retryable
  • accepted is non-fatal
  • transport failure still retries and can recover

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
Copy link
Copy Markdown
Author

  1. I added one follow-up hardening commit on top of the original fix:
    9c2eb8e test(alpen-ee): harden payload status retries

  2. The production behavior is unchanged. This only tightens the regression boundary around the new payload-status semantics:
    invalid payload returns immediately and does not retry
    accepted retries on the same path as syncing
    transport/channel failure still retries and can recover to success

  3. Fresh validation in a Linux-side copy passed:
    cargo fmt --all --check
    cargo test -p alpen-ee-engine -- --nocapture

  4. Result:
    alpen-ee-engine package tests passed
    21 passed, 0 failed

@peter941221 peter941221 marked this pull request as ready for review May 30, 2026 03:47
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: 9c2eb8eec7

ℹ️ 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/engine.rs Outdated
@peter941221
Copy link
Copy Markdown
Author

Follow-up on Accepted semantics after the latest commit:

  1. The current HEAD treats PayloadStatusEnum::Accepted as success in crates/alpen-ee/engine/src/engine.rs.

  2. That choice is source-backed by the Engine API spec for engine_newPayload. The Paris spec says ACCEPTED applies when:
    all transactions are non-zero length
    the payload blockHash is valid
    the payload does not extend the canonical chain
    the payload has not been fully validated
    the ancestors are known and form a well-formed chain

  3. Source:
    https://raw.githubusercontent.com/ethereum/execution-apis/main/src/engine/paris.md
    engine_newPayloadV1 specification, response rule 6

  4. So Accepted is not equivalent to Valid, but it is also not an invalid payload or a missing-data syncing state. In this submit_payload() -> Result<(), ExecutionEngineError> path, I treated it as a non-fatal acceptance result.

  5. Alpen still has a separate inconsistency in crates/evmexec/src/engine.rs, where PayloadStatusEnum::Accepted currently maps to EngineError::Unimplemented at lines 104 and 262. I left that untouched in this PR to keep the review unit narrow.

  6. If you want stricter cross-module alignment, I can follow up in one of two ways:
    promote Accepted in evmexec to an explicit non-error state
    or document why alpen-ee and evmexec should intentionally diverge here

@peter941221
Copy link
Copy Markdown
Author

peter941221 commented Jun 1, 2026

This is still at the post-fix state where Accepted is treated as a non-fatal outcome in alpen-ee, and the earlier review thread is resolved.

If you want a follow-up on the remaining evmexec side for cross-module consistency, I can keep that separate. Otherwise this is ready for another pass.

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