Skip to content

fix: error severity for invalid FC state -> reset#2551

Merged
meyer9 merged 1 commit into
releases/v0.8.0from
meyer9/fix-error-severity-for-invalid-fcu-state
May 6, 2026
Merged

fix: error severity for invalid FC state -> reset#2551
meyer9 merged 1 commit into
releases/v0.8.0from
meyer9/fix-error-severity-for-invalid-fcu-state

Conversation

@meyer9

@meyer9 meyer9 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Change the error severity for an invalid forkchoice state error to Reset so the engine resets if we get this error. This matches SynchronizationTask and op-node behavior.

Disables flaky test test_online_discv5_driver_bootstrap_mainnet.

@vercel

vercel Bot commented May 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
base Ignored Ignored Preview May 6, 2026 10:08pm

Request Review

Comment on lines +32 to +33
#[error("Invalid forkchoice state")]
ForkchoiceStateInvalid,

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.

Missing doc comment on this variant. Every other variant in EngineBuildError has a /// doc comment, and the corresponding variant in SynchronizeTaskError (InvalidForkchoiceState) has one too. Per the project conventions (CLAUDE.md: no #![allow(missing_docs)], fix the underlying issue), this will likely trigger a missing_docs lint if it's enabled.

Also, minor naming inconsistency: SynchronizeTaskError uses InvalidForkchoiceState while this uses ForkchoiceStateInvalid. Not blocking, but worth considering aligning with the existing convention.

Suggested change
#[error("Invalid forkchoice state")]
ForkchoiceStateInvalid,
/// The forkchoice state is invalid.
#[error("Invalid forkchoice state")]
ForkchoiceStateInvalid,
@meyer9 meyer9 force-pushed the meyer9/fix-error-severity-for-invalid-fcu-state branch 3 times, most recently from 849fceb to 14dda10 Compare May 6, 2026 21:30
@meyer9 meyer9 force-pushed the meyer9/fix-error-severity-for-invalid-fcu-state branch from 14dda10 to 9581dfc Compare May 6, 2026 22:08

@refcell refcell left a comment

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.

Took an initial pass and this LGTM

}

#[tokio::test]
#[ignore = "flaky: depends on live mainnet bootstrap nodes responding"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is failing rn, will open an issue to fix this flake in a forward PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@meyer9 meyer9 marked this pull request as ready for review May 6, 2026 22:33
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Review Summary

Overall: This PR looks correct. The change properly adds ForkchoiceStateInvalid error handling to BuildTask, mapping INVALID_FORK_CHOICE_STATE_ERROR (-32008) to Reset severity. This aligns with the existing SynchronizeTask behavior.

What was reviewed

  • error.rs: New ForkchoiceStateInvalid variant and Reset severity mapping
  • task.rs: Error code detection in the map_err closure
  • task_test.rs: New test covering both FCU v2/v3 paths
  • engine_client.rs: Mock error injection support for FCU errors
  • driver.rs: Flaky test annotation (reasonable — depends on live mainnet nodes)

Findings

No new issues beyond what the existing inline comment already raised (naming inconsistency: ForkchoiceStateInvalid here vs InvalidForkchoiceState in SynchronizeTask).

The error handling logic in task.rs:129-140 correctly:

  1. Logs the original RpcError before inspecting it
  2. Checks as_error_resp() for the specific error code
  3. Falls back to AttributesInsertionFailed for all other RPC errors
  4. Mirrors the identical pattern in SynchronizeTask (synchronize/task.rs:138-151)
@meyer9 meyer9 merged commit 3049ce2 into releases/v0.8.0 May 6, 2026
25 of 27 checks passed
@meyer9 meyer9 deleted the meyer9/fix-error-severity-for-invalid-fcu-state branch May 6, 2026 23:08
crazywriter1 pushed a commit to crazywriter1/base that referenced this pull request May 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants