Skip to content

Commit

Permalink
Fix NewPayload Validation during header download (#10093)
Browse files Browse the repository at this point in the history
This PR addresses an issue with the current implementation of the
`engine_newPayload` request handling during the initial sync process.

## Problem
The `engine_newPayload` request triggers validation of newly received
payload, and during the initial sync process, Erigon downloads the block
headers and blocks since more data is required for validation.

Inside this download logic is validating the chain data after it has
finished downloading all the headers. When the node is initially
downloading the headers, there is no validated blocks in the Erigon yet
as it hasn't run the staged sync yet. Because of this, chain validation
is skipped and mark the [download process as
complete](https://github.com/ledgerwatch/erigon/blob/5d92302004b81af3ab95d0af31b0de63a6cb3ac7/turbo/engineapi/engine_block_downloader/core.go#L94-L98).

This in turn marks the execution as synced and [currently
returns](https://github.com/ledgerwatch/erigon/blob/ab0f6336a2cdf69d0a60d19b557085692ea938d5/turbo/engineapi/engine_server.go#L757-L771)
`ValidStatus` to the `engine_newPayload` request even though no
validation has actually been done.

I think this behavior deviates from the engine API specification and
incorrectly returns VALID even though Erigon doesn't have any synced
blocks.

## Proposed Change
This PR modifies the `handleNewPayload` function to additionally
validate the chain. If this chain validation is failed(or skipped), it
will return `SYNCING` as a result for the `engine_newPayload`. When the
header download and staged sync is complete, only then will `newPayload`
return `VALID` after actually validating the chain data.

---

This is my first time contributing to Erigon, and I'd appreciate any
feedback for this kind of change. I'd also love to know if current logic
is an effort for performance improvement or if there's other rationale
for it.
  • Loading branch information
mininny authored and yperbasis committed Jun 21, 2024
1 parent e8c5632 commit 88bd47f
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion turbo/engineapi/engine_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,18 @@ func (e *EngineServer) HandleNewPayload(
if !success {
return &engine_types.PayloadStatus{Status: engine_types.SyncingStatus}, nil
}
return &engine_types.PayloadStatus{Status: engine_types.ValidStatus, LatestValidHash: &headerHash}, nil

status, _, latestValidHash, err := e.chainRW.ValidateChain(ctx, headerHash, headerNumber)
if err != nil {
return nil, err
}

if status == execution.ExecutionStatus_Busy || status == execution.ExecutionStatus_TooFarAway {
e.logger.Debug(fmt.Sprintf("[%s] New payload: Client is still syncing", logPrefix))
return &engine_types.PayloadStatus{Status: engine_types.SyncingStatus}, nil
} else {
return &engine_types.PayloadStatus{Status: engine_types.ValidStatus, LatestValidHash: &latestValidHash}, nil
}
} else {
return &engine_types.PayloadStatus{Status: engine_types.SyncingStatus}, nil
}
Expand Down

0 comments on commit 88bd47f

Please sign in to comment.