From 21b153df43d54554efd2c8e6926576de5f66a792 Mon Sep 17 00:00:00 2001 From: ozpool Date: Tue, 19 May 2026 13:53:34 +0530 Subject: [PATCH] internal/ethapi, eth, eth/catalyst: address review feedback on eth_syncing CL gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @MariusVanDerWijden's review feedback, tighten the change to match geth's existing style: - Drop the MarkConsensusExpected/MarkConsensusContacted/ConsensusReady doc paragraphs on Ethereum; collapse the field comments to single trailing lines matching eth/handler.go's atomic.Bool style. - Rename the unexported accessors to MarkCLExpected/MarkCLContacted (catalyst can't reach the fields directly). - Drop the multi-line comments at the catalyst call sites — the method names are self-describing. - Trim the Backend.ConsensusReady() interface comment and EthAPIBackend wrapper comment. - Replace the verbose docstring on EthereumAPI.Syncing with a single reference to #33687. - Drop the long doc comments on the syncing_test.go cases; rename test functions to short forms (TestSyncingBeforeCLContact, etc.). No behavioural change. Run: `go test ./internal/ethapi/ -count=1`. --- eth/api_backend.go | 2 -- eth/backend.go | 41 ++++++++------------------------- eth/catalyst/api.go | 14 +++-------- internal/ethapi/api.go | 13 +++-------- internal/ethapi/backend.go | 6 ----- internal/ethapi/syncing_test.go | 40 ++++++++------------------------ 6 files changed, 25 insertions(+), 91 deletions(-) diff --git a/eth/api_backend.go b/eth/api_backend.go index da57e834ea..80751b023b 100644 --- a/eth/api_backend.go +++ b/eth/api_backend.go @@ -422,8 +422,6 @@ func (b *EthAPIBackend) SyncProgress(ctx context.Context) ethereum.SyncProgress return prog } -// ConsensusReady reports whether the node's "synced" claim is currently -// meaningful. See (*Ethereum).ConsensusReady for the gating rules. func (b *EthAPIBackend) ConsensusReady() bool { return b.eth.ConsensusReady() } diff --git a/eth/backend.go b/eth/backend.go index 957c475850..78aeb6dd0e 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -125,41 +125,20 @@ type Ethereum struct { shutdownTracker *shutdowncheck.ShutdownTracker // Tracks if and when the node has shutdown ungracefully - // clExpected is set when the Engine API is registered on this node - // (catalyst.Register). When unset (no Engine API attached, e.g. in - // tests, in --dev mode without catalyst, in light/legacy backends), - // the node cannot be paired with a consensus client and should not be - // gated on a CL handshake. clContacted records whether the consensus - // client has spoken to us via the Engine API at least once. - clExpected atomic.Bool - clContacted atomic.Bool + clExpected atomic.Bool // Set when catalyst.Register attaches the Engine API + clContacted atomic.Bool // Set on first Engine API call (newPayload / FCU) } -// MarkConsensusExpected records that this node expects to be driven by a -// consensus layer via the Engine API. It should be invoked once during node -// setup, when the Engine API is registered. -func (s *Ethereum) MarkConsensusExpected() { - s.clExpected.Store(true) -} +// MarkCLExpected and MarkCLContacted are setters for the two clXxx flags; +// catalyst calls them from its package and so cannot reach the fields directly. +func (s *Ethereum) MarkCLExpected() { s.clExpected.Store(true) } +func (s *Ethereum) MarkCLContacted() { s.clContacted.Store(true) } -// MarkConsensusContacted records that the consensus layer has driven this node -// at least once via the Engine API. The flag is sticky: once set, it stays set -// for the lifetime of the process. -func (s *Ethereum) MarkConsensusContacted() { - s.clContacted.Store(true) -} - -// ConsensusReady reports whether the node's "synced" claim is meaningful right -// now. If the node does not expect a consensus client (no Engine API), the -// answer is always yes. If it does, the answer is yes only after the consensus -// client has driven the node at least once. eth_syncing uses this to avoid -// claiming "synced" before any CL handshake has occurred on freshly started -// post-merge nodes. +// ConsensusReady reports whether eth_syncing should be allowed to return false. +// On nodes without an Engine API, always true. On nodes that expect a CL, true +// only after the CL has driven the node at least once. func (s *Ethereum) ConsensusReady() bool { - if !s.clExpected.Load() { - return true - } - return s.clContacted.Load() + return !s.clExpected.Load() || s.clContacted.Load() } // New creates a new Ethereum object (including the initialisation of the common Ethereum object), diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 6e83087d5a..87ff503fb1 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -49,11 +49,7 @@ import ( // Register adds the engine API and related APIs to the full node. func Register(stack *node.Node, backend *eth.Ethereum) error { - // Mark the backend as expecting a consensus client. eth_syncing uses - // this to gate "synced" responses on a CL handshake: a node configured - // to take direction from a CL should not advertise itself as synced - // until that CL has actually driven it at least once. - backend.MarkConsensusExpected() + backend.MarkCLExpected() stack.RegisterAPIs([]rpc.API{ newTestingAPI(backend), { @@ -256,9 +252,7 @@ func (api *ConsensusAPI) forkchoiceUpdated(ctx context.Context, update engine.Fo } // Stash away the last update to warn the user if the beacon client goes offline api.lastForkchoiceUpdate.Store(time.Now().Unix()) - // Record that the consensus layer has driven us at least once. eth_syncing - // uses this to avoid claiming "synced" before any CL handshake has occurred. - api.eth.MarkConsensusContacted() + api.eth.MarkCLContacted() // Check whether we have the block yet in our database or not. If not, we'll // need to either trigger a sync, or to reject this forkchoice update for a @@ -854,9 +848,7 @@ func (api *ConsensusAPI) newPayload(ctx context.Context, params engine.Executabl } // Stash away the last update to warn the user if the beacon client goes offline api.lastNewPayloadUpdate.Store(time.Now().Unix()) - // Record that the consensus layer has driven us at least once. eth_syncing - // uses this to avoid claiming "synced" before any CL handshake has occurred. - api.eth.MarkConsensusContacted() + api.eth.MarkCLContacted() // If we already have the block locally, ignore the entire execution and just // return a fake success. diff --git a/internal/ethapi/api.go b/internal/ethapi/api.go index b42a5f20b6..2c444a245b 100644 --- a/internal/ethapi/api.go +++ b/internal/ethapi/api.go @@ -153,19 +153,12 @@ func (api *EthereumAPI) BlobBaseFee(ctx context.Context) *hexutil.Big { // - highestBlock: block number of the highest block header this node has received from peers // - pulledStates: number of state entries processed until now // - knownStates: number of known state entries that still need to be pulled -// -// On post-merge nodes paired with a consensus client, Syncing also waits for -// the consensus client to drive the node at least once before reporting -// "false". This avoids advertising a freshly started node as synced before any -// CL handshake has happened — see (*Ethereum).ConsensusReady. Backends without -// an Engine API (tests, --dev mode without catalyst, light clients) report -// ready immediately, preserving legacy behavior. func (api *EthereumAPI) Syncing(ctx context.Context) (interface{}, error) { progress := api.b.SyncProgress(ctx) - // Return not syncing if the synchronisation already completed AND, for - // backends that expect one, the consensus client has driven the node at - // least once. + // Don't claim "synced" until the CL has driven us at least once (post-merge + // nodes with Engine API attached). Backends without a CL report ready + // immediately via ConsensusReady. Refs #33687. if progress.Done() && api.b.ConsensusReady() { return false, nil } diff --git a/internal/ethapi/backend.go b/internal/ethapi/backend.go index 921c5ff112..cc2dbaac43 100644 --- a/internal/ethapi/backend.go +++ b/internal/ethapi/backend.go @@ -42,12 +42,6 @@ import ( type Backend interface { // General Ethereum API SyncProgress(ctx context.Context) ethereum.SyncProgress - // ConsensusReady reports whether the node's "synced" claim is currently - // meaningful. For backends that do not expect a consensus client (no - // Engine API attached) the answer is always true. For backends that do, - // the answer flips to true only after the consensus client has driven - // the node at least once. eth_syncing uses this to avoid reporting - // "synced" before any CL handshake has happened. ConsensusReady() bool SuggestGasTipCap(ctx context.Context) (*big.Int, error) diff --git a/internal/ethapi/syncing_test.go b/internal/ethapi/syncing_test.go index f6746e1812..27f188163d 100644 --- a/internal/ethapi/syncing_test.go +++ b/internal/ethapi/syncing_test.go @@ -23,9 +23,6 @@ import ( "github.com/ethereum/go-ethereum" ) -// syncingBackend is a minimal Backend embedding that only implements the two -// methods Syncing calls. Embedding the interface avoids pulling in the full -// testBackend setup just to flip a single bool. type syncingBackend struct { Backend progress ethereum.SyncProgress @@ -35,18 +32,9 @@ type syncingBackend struct { func (b *syncingBackend) SyncProgress(_ context.Context) ethereum.SyncProgress { return b.progress } func (b *syncingBackend) ConsensusReady() bool { return b.ready } -// TestSyncingReportsBeforeConsensusContact verifies that on a CL-paired node -// (ConsensusReady false), eth_syncing returns a truthy progress object even -// when the local downloader believes itself to be done. This is the bug fix -// for issue #33687: a freshly started node must not advertise itself as -// "synced" before the consensus client has actually driven it. -func TestSyncingReportsBeforeConsensusContact(t *testing.T) { - api := NewEthereumAPI(&syncingBackend{ - // progress.Done() returns true on a zero-valued struct because all - // remaining counters are zero and CurrentBlock >= HighestBlock. - progress: ethereum.SyncProgress{}, - ready: false, - }) +// Issue #33687: a Done downloader but no CL handshake yet must report syncing. +func TestSyncingBeforeCLContact(t *testing.T) { + api := NewEthereumAPI(&syncingBackend{progress: ethereum.SyncProgress{}, ready: false}) res, err := api.Syncing(context.Background()) if err != nil { t.Fatalf("Syncing returned error: %v", err) @@ -56,34 +44,24 @@ func TestSyncingReportsBeforeConsensusContact(t *testing.T) { } } -// TestSyncingReportsFalseAfterConsensusContact verifies that once the -// consensus layer has handshaken at least once (or the backend does not -// expect one) and progress.Done() is true, eth_syncing reports false. -func TestSyncingReportsFalseAfterConsensusContact(t *testing.T) { - api := NewEthereumAPI(&syncingBackend{ - progress: ethereum.SyncProgress{}, - ready: true, - }) +func TestSyncingAfterCLContact(t *testing.T) { + api := NewEthereumAPI(&syncingBackend{progress: ethereum.SyncProgress{}, ready: true}) res, err := api.Syncing(context.Background()) if err != nil { t.Fatalf("Syncing returned error: %v", err) } - v, ok := res.(bool) - if !ok || v { + if v, ok := res.(bool); !ok || v { t.Fatalf("expected false after CL handshake when sync is done, got %v", res) } } -// TestSyncingReportsActiveSyncEvenWithoutConsensusContact verifies that when -// the downloader is actively syncing, eth_syncing returns the progress map -// regardless of the CL gate. This preserves the legacy semantics for the case -// the issue thread did not affect. -func TestSyncingReportsActiveSyncEvenWithoutConsensusContact(t *testing.T) { +// Active sync stays truthy regardless of the CL gate. +func TestSyncingActiveSyncIgnoresCLGate(t *testing.T) { api := NewEthereumAPI(&syncingBackend{ progress: ethereum.SyncProgress{ StartingBlock: 100, CurrentBlock: 150, - HighestBlock: 200, // CurrentBlock < HighestBlock => Done()=false + HighestBlock: 200, }, ready: false, })