From 88bd47ffb579a5b8e54577db1d1fd109faee7df0 Mon Sep 17 00:00:00 2001 From: Minhyuk Kim Date: Wed, 5 Jun 2024 16:53:58 +0900 Subject: [PATCH] Fix NewPayload Validation during header download (#10093) 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. --- turbo/engineapi/engine_server.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/turbo/engineapi/engine_server.go b/turbo/engineapi/engine_server.go index f7ed8993008..92741dd45f2 100644 --- a/turbo/engineapi/engine_server.go +++ b/turbo/engineapi/engine_server.go @@ -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 }