Skip to content

Conversation

@calbera
Copy link
Contributor

@calbera calbera commented Jul 11, 2025

Remnant of the past #2490:

Add todo to extend retryOnSyncingStatus = false to FinalizeBlock as well. This is not required, but will make it such that we could delete forceSyncUponFinalize completely.

Before:

  • On the first FinalizeBlock request (height N) a node sees, it used to send a NewPayload & FCU for N before processing the incoming execution payload. --> This would error out until the EL is caught up.
  • On the first ProcessProposal a node sees, node will send FCU to trigger a catchup to head.
  • On all ProcessProposals, the node will call NewPayload and if seen a SYNCING error it ignores it.

Now:

  • On ALL FinalizeBlock requests a node sees, it will call NewPayload as expected but ignore any SYNCING error.
  • On the first ProcessProposal a node sees, node will send FCU to trigger a catchup to head -- as expected, no change
  • On all ProcessProposals, the node will call NewPayload and if seen a SYNCING error, it will continue to retry

Effectively this allows us not to stall on the first FInalizeBlock. If a node's EL needs to catchup it will still do so but now it will stall when the CL is caught up to tip, i.e. on the first ProcessProposal. Since we already have this mechanism handled in ProcessProposal, we can remove it from FinalizeBlock.

@calbera calbera requested a review from a team July 11, 2025 00:55
@calbera calbera requested a review from a team as a code owner July 11, 2025 00:55
@codecov
Copy link

codecov bot commented Jul 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.85%. Comparing base (5bf596c) to head (9416e8d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2847      +/-   ##
==========================================
+ Coverage   62.69%   62.85%   +0.16%     
==========================================
  Files         353      353              
  Lines       17135    17091      -44     
==========================================
+ Hits        10742    10743       +1     
+ Misses       5537     5494      -43     
+ Partials      856      854       -2     
Files with missing lines Coverage Δ
beacon/blockchain/finalize_block.go 70.96% <ø> (+5.52%) ⬆️
beacon/blockchain/payload.go 67.74% <ø> (+14.57%) ⬆️
beacon/blockchain/service.go 79.62% <100.00%> (-0.38%) ⬇️
state-transition/core/state_processor.go 52.56% <100.00%> (+0.61%) ⬆️
state-transition/core/state_processor_payload.go 63.86% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@calbera calbera changed the title chore(state-processor): only retry NewPayload on SYNCING status if not in FinalizeBlock chore(engine): only retry NewPayload on SYNCING status if not in FinalizeBlock Jul 11, 2025
@abi87
Copy link
Contributor

abi87 commented Jul 16, 2025

@calbera I would temporarily park this, until #2812 is in.
The two interacts in way that are not fully obvious to me at this point in time and I would like to revisit this later on

Copilot AI review requested due to automatic review settings December 4, 2025 21:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors how NewPayload retries on SYNCING status to differentiate between FinalizeBlock and ProcessProposal execution paths. The change allows NewPayload to skip retrying on SYNCING errors during FinalizeBlock while maintaining retry behavior during ProcessProposal, eliminating the need for the forceSyncUponFinalize function.

Key Changes:

  • Added inFinalizeBlock boolean parameter throughout the state transition chain to distinguish execution contexts
  • Removed forceSyncUponFinalize function and its invocation from FinalizeBlock
  • Changed forceStartupSyncOnce from pointer to value type in Service struct

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
state-transition/core/state_processor_payload.go Threaded inFinalizeBlock parameter through payload processing functions to conditionally control NewPayload retry behavior
state-transition/core/state_processor.go Introduced inFinalizeBlock variable and passed it to ProcessBlock to enable context-aware execution
beacon/blockchain/service.go Changed forceStartupSyncOnce from pointer to value type and removed initialization
beacon/blockchain/payload.go Removed forceSyncUponFinalize function entirely along with related imports
beacon/blockchain/finalize_block.go Removed forceSyncUponFinalize call and setup from FinalizeBlock, cleaned up unused import

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@calbera calbera changed the title chore(engine): only retry NewPayload on SYNCING status if not in FinalizeBlock chore(engine): remove forceSyncUponFinalize Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants