Skip to content

Conversation

@forkfury
Copy link
Contributor

Fix

Fixes #9921 - Occasional "Parent beacon block root must be set" error when processing engine_newPayloadV4 requests from op-node.

Changes

  • Modified ExecutionPayloadParams.ValidateParams() to fall back to executionPayload.ParentBeaconBlockRoot when the parentBeaconBlockRoot parameter is null
  • Parameter value still takes precedence when provided (backward compatible)
  • Allows valid op-node requests that set parentBeaconBlockRoot in the payload object instead of as a method parameter

if (parentBeaconBlockRoot is null)
// Use parentBeaconBlockRoot parameter if provided, otherwise fall back to executionPayload.ParentBeaconBlockRoot
// This handles cases where op-node sends valid requests with parentBeaconBlockRoot in the payload itself
Hash256? finalParentBeaconBlockRoot = parentBeaconBlockRoot ?? executionPayload.ParentBeaconBlockRoot;
Copy link
Member

Choose a reason for hiding this comment

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

but normally executionPayload.ParentBeaconBlockRoot is null and is set few lines below. Unless the mechanism is different in Optimism and it comes from PayloadDecoder?

@emlautarom1 @deffrian ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been taking a look at our implementation of the OP-CL and also in OP-node, and in both cases the ParentBeaconBlockRoot sent as parameter is the same in the executionPayload.ParentBeaconBlockRoot parameter, so the assignment is a no-op (this is the case for all post-Ecotone blocks).

The proposed fix most likely does not work as intended. Have you tested this @forkfury ?

Copy link
Member

Choose a reason for hiding this comment

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

@emlautarom1 What about others op-cl implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK there are only two implementations: op-node (official CL) and ours.

@emlautarom1
Copy link
Contributor

We are looking into this as a potential fix but we're not fully sure that it's the correct approach or even if it will have any semantic effect.

Could you add the following logs:

  • When the input executionPayload does have a non null ParentBeaconBlockRoot as "info"
  • When the input executionPayload does have a non null ParentBeaconBlockRoot and it does not match the input parentBeaconBlockRoot as "warning"
  • When the finalParentBeaconBlockRoot variable is null as "error".

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

@forkfury please add above logging

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.

Ocasional missing Paren beacon block root missing

3 participants