From 238b160a677d346f1cfbadfd54b5c5bb6b05d9fb Mon Sep 17 00:00:00 2001 From: ozpool Date: Thu, 30 Apr 2026 16:19:26 +0530 Subject: [PATCH 1/3] internal/ethapi, eth, eth/catalyst: gate eth_syncing on CL handshake eth_syncing currently returns false as soon as the local downloader believes the chain to be done. On a freshly started node this happens before the consensus client has talked to it: the persisted head loads into memory, no CL handshake has occurred, the downloader sees nothing to do, Progress.Done() is true, eth_syncing reports synced. That is wrong from an operator perspective. Load balancers (HAProxy, NGINX), L2 supervisors and multi-node setups commonly gate routing on eth_syncing. They start sending live traffic to a node that has not actually learned about any new head yet, which surfaces as missing state, stale reads, and unhealthy upstreams. Maintainer-endorsed direction in the issue thread: "default geth to 'syncing' on startup and only switch to 'synced' once we learn about a new block". Implement that with a sticky atomic.Bool on *Ethereum, set the first time the consensus layer drives the node via the Engine API (ForkchoiceUpdated or NewPayload), and consulted from eth_syncing. - eth/backend.go: add Ethereum.clContacted with MarkConsensusContacted/ConsensusContacted helpers - eth/catalyst/api.go: call MarkConsensusContacted at the same point where lastForkchoiceUpdate / lastNewPayloadUpdate are stamped, so the gate flips on every CL message regardless of the response status (handshake recorded even when we reply STATUS_SYNCING) - internal/ethapi/backend.go: add ConsensusContacted() to the Backend interface and to the two test mocks (api_test.go testBackend, transaction_args_test.go backendMock; both default to true so existing tests keep their original semantics) - eth/api_backend.go: implement ConsensusContacted on EthAPIBackend - internal/ethapi/api.go: in EthereumAPI.Syncing, only short-circuit to "false" when both progress.Done() AND ConsensusContacted() are true; otherwise return the progress map as during an active sync Adds dedicated tests in internal/ethapi/syncing_test.go covering: - the new gate (Done but no CL contact -> truthy progress) - normal post-handshake behavior (Done + CL contact -> false) - active-sync behavior is unchanged regardless of the gate Refs #33687. --- eth/api_backend.go | 6 ++ eth/backend.go | 21 ++++++ eth/catalyst/api.go | 6 ++ internal/ethapi/api.go | 13 +++- internal/ethapi/api_test.go | 1 + internal/ethapi/backend.go | 4 + internal/ethapi/syncing_test.go | 96 ++++++++++++++++++++++++ internal/ethapi/transaction_args_test.go | 1 + 8 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 internal/ethapi/syncing_test.go diff --git a/eth/api_backend.go b/eth/api_backend.go index 33fe4fe5d9..d2fc5199d2 100644 --- a/eth/api_backend.go +++ b/eth/api_backend.go @@ -422,6 +422,12 @@ func (b *EthAPIBackend) SyncProgress(ctx context.Context) ethereum.SyncProgress return prog } +// ConsensusContacted reports whether the consensus layer has driven this node +// via the Engine API at least once since process start. +func (b *EthAPIBackend) ConsensusContacted() bool { + return b.eth.ConsensusContacted() +} + func (b *EthAPIBackend) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { return b.gpo.SuggestTipCap(ctx) } diff --git a/eth/backend.go b/eth/backend.go index af8b04bda6..9be4768825 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -25,6 +25,7 @@ import ( "math/big" "runtime" "sync" + "sync/atomic" "time" "github.com/ethereum/go-ethereum/accounts" @@ -123,6 +124,26 @@ type Ethereum struct { lock sync.RWMutex // Protects the variadic fields (e.g. gas price and etherbase) shutdownTracker *shutdowncheck.ShutdownTracker // Tracks if and when the node has shutdown ungracefully + + // clContacted records whether the consensus client has ever spoken to us + // via the Engine API. Until that happens, the node has not learned about + // any new head from the network and cannot truthfully report itself as + // "synced" — eth_syncing falls back to reporting an in-progress sync. + clContacted atomic.Bool +} + +// 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. eth_syncing uses it to avoid reporting a +// freshly started node as "synced" before any CL handshake has occurred. +func (s *Ethereum) MarkConsensusContacted() { + s.clContacted.Store(true) +} + +// ConsensusContacted reports whether the consensus layer has ever driven this +// node via the Engine API since process start. +func (s *Ethereum) ConsensusContacted() bool { + return 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 1def169ae0..6fe3b2616f 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -251,6 +251,9 @@ 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() // 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 @@ -846,6 +849,9 @@ 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() // 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 6d38c6c7c8..c9629e4849 100644 --- a/internal/ethapi/api.go +++ b/internal/ethapi/api.go @@ -153,11 +153,20 @@ 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 +// +// Until the consensus layer has driven the node at least once via the Engine +// API, the node has not actually learned about any new chain head and cannot +// truthfully report itself as "synced". In that case Syncing returns the +// progress object regardless of whether progress.Done() would be true. func (api *EthereumAPI) Syncing(ctx context.Context) (interface{}, error) { progress := api.b.SyncProgress(ctx) - // Return not syncing if the synchronisation already completed - if progress.Done() { + // Return not syncing if the synchronisation already completed AND we have + // observed at least one Engine API call from the consensus layer. The CL + // gate prevents a freshly started node from being advertised as synced + // before any CL handshake has happened (a common operational footgun for + // load balancers and L2 stacks gating on eth_syncing). + if progress.Done() && api.b.ConsensusContacted() { return false, nil } // Otherwise gather the block sync stats diff --git a/internal/ethapi/api_test.go b/internal/ethapi/api_test.go index 63e75bd3e3..03fa95881d 100644 --- a/internal/ethapi/api_test.go +++ b/internal/ethapi/api_test.go @@ -493,6 +493,7 @@ func newTestBackend(t *testing.T, n int, gspec *core.Genesis, engine consensus.E func (b testBackend) SyncProgress(ctx context.Context) ethereum.SyncProgress { return ethereum.SyncProgress{} } +func (b testBackend) ConsensusContacted() bool { return true } func (b testBackend) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { return big.NewInt(0), nil } diff --git a/internal/ethapi/backend.go b/internal/ethapi/backend.go index af3d592b82..efcf24a9f8 100644 --- a/internal/ethapi/backend.go +++ b/internal/ethapi/backend.go @@ -42,6 +42,10 @@ import ( type Backend interface { // General Ethereum API SyncProgress(ctx context.Context) ethereum.SyncProgress + // ConsensusContacted reports whether the consensus layer has driven this + // node via the Engine API at least once since process start. eth_syncing + // uses it to avoid reporting "synced" before any CL handshake has happened. + ConsensusContacted() bool SuggestGasTipCap(ctx context.Context) (*big.Int, error) FeeHistory(ctx context.Context, blockCount uint64, lastBlock rpc.BlockNumber, rewardPercentiles []float64) (*big.Int, [][]*big.Int, []*big.Int, []float64, []*big.Int, []float64, error) diff --git a/internal/ethapi/syncing_test.go b/internal/ethapi/syncing_test.go new file mode 100644 index 0000000000..5b800b4bb0 --- /dev/null +++ b/internal/ethapi/syncing_test.go @@ -0,0 +1,96 @@ +// Copyright 2026 The go-ethereum Authors +// This file is part of the go-ethereum library. +// +// The go-ethereum library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The go-ethereum library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the go-ethereum library. If not, see . + +package ethapi + +import ( + "context" + "testing" + + "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 + contacted bool +} + +func (b *syncingBackend) SyncProgress(_ context.Context) ethereum.SyncProgress { return b.progress } +func (b *syncingBackend) ConsensusContacted() bool { return b.contacted } + +// TestSyncingReportsBeforeConsensusContact verifies that eth_syncing returns a +// truthy progress object until the consensus layer has driven the node via the +// Engine API at least once, even when the local downloader believes itself to +// be done. +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{}, + contacted: false, + }) + res, err := api.Syncing(context.Background()) + if err != nil { + t.Fatalf("Syncing returned error: %v", err) + } + if v, ok := res.(bool); ok && !v { + t.Fatal("expected truthy syncing payload before CL handshake, got false") + } +} + +// TestSyncingReportsFalseAfterConsensusContact verifies that once the +// consensus layer has handshaken at least once and progress.Done() is true, +// eth_syncing reports false. +func TestSyncingReportsFalseAfterConsensusContact(t *testing.T) { + api := NewEthereumAPI(&syncingBackend{ + progress: ethereum.SyncProgress{}, + contacted: true, + }) + res, err := api.Syncing(context.Background()) + if err != nil { + t.Fatalf("Syncing returned error: %v", err) + } + v, ok := res.(bool) + if !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) { + api := NewEthereumAPI(&syncingBackend{ + progress: ethereum.SyncProgress{ + StartingBlock: 100, + CurrentBlock: 150, + HighestBlock: 200, // CurrentBlock < HighestBlock => Done()=false + }, + contacted: false, + }) + res, err := api.Syncing(context.Background()) + if err != nil { + t.Fatalf("Syncing returned error: %v", err) + } + if _, ok := res.(map[string]interface{}); !ok { + t.Fatalf("expected progress map during active sync, got %T", res) + } +} diff --git a/internal/ethapi/transaction_args_test.go b/internal/ethapi/transaction_args_test.go index 30791f32b5..cd67190fcb 100644 --- a/internal/ethapi/transaction_args_test.go +++ b/internal/ethapi/transaction_args_test.go @@ -326,6 +326,7 @@ func (b *backendMock) ChainConfig() *params.ChainConfig { return b.config } func (b *backendMock) SyncProgress(ctx context.Context) ethereum.SyncProgress { return ethereum.SyncProgress{} } +func (b *backendMock) ConsensusContacted() bool { return true } func (b *backendMock) FeeHistory(ctx context.Context, blockCount uint64, lastBlock rpc.BlockNumber, rewardPercentiles []float64) (*big.Int, [][]*big.Int, []*big.Int, []float64, []*big.Int, []float64, error) { return nil, nil, nil, nil, nil, nil, nil } From 20d6757391bfc4cba0dc97ea736c78075d9c652c Mon Sep 17 00:00:00 2001 From: ozpool Date: Thu, 30 Apr 2026 17:45:57 +0530 Subject: [PATCH 2/3] internal/ethapi, eth: only gate eth_syncing when Engine API is registered The previous version of this change unconditionally returned the progress map until the consensus client had driven the node at least once. That broke ethclient.TestEthClient/StatusFunctions and any other backend that runs without a consensus client (in-process tests, --dev mode without catalyst, light/legacy backends), where reporting "syncing" forever is clearly wrong. Split the gate into two flags: - clExpected: set in eth/catalyst.Register, the only entry point that attaches the Engine API to a node. If a backend never calls Register, it is not paired with a consensus client. - clContacted: set on every Engine API call (forkchoiceUpdated and newPayload), unchanged from before. Replace ConsensusContacted on the Backend interface with ConsensusReady, which folds the two flags into the question eth_syncing actually wants answered: "is the synced claim meaningful right now?" Backends that never expect a CL answer yes immediately, preserving legacy behavior. Backends that do expect one answer yes only after the first FCU/NewPayload. - eth/backend.go: clExpected, clContacted, MarkConsensusExpected, MarkConsensusContacted, ConsensusReady on (*Ethereum) - eth/catalyst/api.go: backend.MarkConsensusExpected() in Register - eth/api_backend.go: ConsensusReady delegates to (*Ethereum) - internal/ethapi/backend.go: rename interface method to ConsensusReady - internal/ethapi/api.go: Syncing checks ConsensusReady - internal/ethapi/{api_test,transaction_args_test}.go: rename the test mock methods (default to true so existing tests are unaffected) - internal/ethapi/syncing_test.go: rename the helper field; tests now cover (a) CL-paired node before handshake -> truthy, (b) ready node -> false, (c) active sync -> progress map regardless of gate Refs #33687. --- eth/api_backend.go | 8 +++--- eth/backend.go | 34 +++++++++++++++++------- eth/catalyst/api.go | 5 ++++ internal/ethapi/api.go | 20 +++++++------- internal/ethapi/api_test.go | 2 +- internal/ethapi/backend.go | 11 +++++--- internal/ethapi/syncing_test.go | 29 ++++++++++---------- internal/ethapi/transaction_args_test.go | 2 +- 8 files changed, 68 insertions(+), 43 deletions(-) diff --git a/eth/api_backend.go b/eth/api_backend.go index d2fc5199d2..da57e834ea 100644 --- a/eth/api_backend.go +++ b/eth/api_backend.go @@ -422,10 +422,10 @@ func (b *EthAPIBackend) SyncProgress(ctx context.Context) ethereum.SyncProgress return prog } -// ConsensusContacted reports whether the consensus layer has driven this node -// via the Engine API at least once since process start. -func (b *EthAPIBackend) ConsensusContacted() bool { - return b.eth.ConsensusContacted() +// 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() } func (b *EthAPIBackend) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { diff --git a/eth/backend.go b/eth/backend.go index 9be4768825..957c475850 100644 --- a/eth/backend.go +++ b/eth/backend.go @@ -125,24 +125,40 @@ type Ethereum struct { shutdownTracker *shutdowncheck.ShutdownTracker // Tracks if and when the node has shutdown ungracefully - // clContacted records whether the consensus client has ever spoken to us - // via the Engine API. Until that happens, the node has not learned about - // any new head from the network and cannot truthfully report itself as - // "synced" — eth_syncing falls back to reporting an in-progress sync. + // 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 } +// 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) +} + // 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. eth_syncing uses it to avoid reporting a -// freshly started node as "synced" before any CL handshake has occurred. +// for the lifetime of the process. func (s *Ethereum) MarkConsensusContacted() { s.clContacted.Store(true) } -// ConsensusContacted reports whether the consensus layer has ever driven this -// node via the Engine API since process start. -func (s *Ethereum) ConsensusContacted() bool { +// 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. +func (s *Ethereum) ConsensusReady() bool { + if !s.clExpected.Load() { + return true + } return s.clContacted.Load() } diff --git a/eth/catalyst/api.go b/eth/catalyst/api.go index 6fe3b2616f..6e83087d5a 100644 --- a/eth/catalyst/api.go +++ b/eth/catalyst/api.go @@ -49,6 +49,11 @@ 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() stack.RegisterAPIs([]rpc.API{ newTestingAPI(backend), { diff --git a/internal/ethapi/api.go b/internal/ethapi/api.go index c9629e4849..b42a5f20b6 100644 --- a/internal/ethapi/api.go +++ b/internal/ethapi/api.go @@ -154,19 +154,19 @@ func (api *EthereumAPI) BlobBaseFee(ctx context.Context) *hexutil.Big { // - pulledStates: number of state entries processed until now // - knownStates: number of known state entries that still need to be pulled // -// Until the consensus layer has driven the node at least once via the Engine -// API, the node has not actually learned about any new chain head and cannot -// truthfully report itself as "synced". In that case Syncing returns the -// progress object regardless of whether progress.Done() would be true. +// 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 we have - // observed at least one Engine API call from the consensus layer. The CL - // gate prevents a freshly started node from being advertised as synced - // before any CL handshake has happened (a common operational footgun for - // load balancers and L2 stacks gating on eth_syncing). - if progress.Done() && api.b.ConsensusContacted() { + // Return not syncing if the synchronisation already completed AND, for + // backends that expect one, the consensus client has driven the node at + // least once. + if progress.Done() && api.b.ConsensusReady() { return false, nil } // Otherwise gather the block sync stats diff --git a/internal/ethapi/api_test.go b/internal/ethapi/api_test.go index 03fa95881d..ffca92b218 100644 --- a/internal/ethapi/api_test.go +++ b/internal/ethapi/api_test.go @@ -493,7 +493,7 @@ func newTestBackend(t *testing.T, n int, gspec *core.Genesis, engine consensus.E func (b testBackend) SyncProgress(ctx context.Context) ethereum.SyncProgress { return ethereum.SyncProgress{} } -func (b testBackend) ConsensusContacted() bool { return true } +func (b testBackend) ConsensusReady() bool { return true } func (b testBackend) SuggestGasTipCap(ctx context.Context) (*big.Int, error) { return big.NewInt(0), nil } diff --git a/internal/ethapi/backend.go b/internal/ethapi/backend.go index efcf24a9f8..921c5ff112 100644 --- a/internal/ethapi/backend.go +++ b/internal/ethapi/backend.go @@ -42,10 +42,13 @@ import ( type Backend interface { // General Ethereum API SyncProgress(ctx context.Context) ethereum.SyncProgress - // ConsensusContacted reports whether the consensus layer has driven this - // node via the Engine API at least once since process start. eth_syncing - // uses it to avoid reporting "synced" before any CL handshake has happened. - ConsensusContacted() bool + // 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) FeeHistory(ctx context.Context, blockCount uint64, lastBlock rpc.BlockNumber, rewardPercentiles []float64) (*big.Int, [][]*big.Int, []*big.Int, []float64, []*big.Int, []float64, error) diff --git a/internal/ethapi/syncing_test.go b/internal/ethapi/syncing_test.go index 5b800b4bb0..f6746e1812 100644 --- a/internal/ethapi/syncing_test.go +++ b/internal/ethapi/syncing_test.go @@ -28,23 +28,24 @@ import ( // testBackend setup just to flip a single bool. type syncingBackend struct { Backend - progress ethereum.SyncProgress - contacted bool + progress ethereum.SyncProgress + ready bool } func (b *syncingBackend) SyncProgress(_ context.Context) ethereum.SyncProgress { return b.progress } -func (b *syncingBackend) ConsensusContacted() bool { return b.contacted } +func (b *syncingBackend) ConsensusReady() bool { return b.ready } -// TestSyncingReportsBeforeConsensusContact verifies that eth_syncing returns a -// truthy progress object until the consensus layer has driven the node via the -// Engine API at least once, even when the local downloader believes itself to -// be done. +// 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{}, - contacted: false, + progress: ethereum.SyncProgress{}, + ready: false, }) res, err := api.Syncing(context.Background()) if err != nil { @@ -56,12 +57,12 @@ func TestSyncingReportsBeforeConsensusContact(t *testing.T) { } // TestSyncingReportsFalseAfterConsensusContact verifies that once the -// consensus layer has handshaken at least once and progress.Done() is true, -// eth_syncing reports false. +// 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{}, - contacted: true, + progress: ethereum.SyncProgress{}, + ready: true, }) res, err := api.Syncing(context.Background()) if err != nil { @@ -84,7 +85,7 @@ func TestSyncingReportsActiveSyncEvenWithoutConsensusContact(t *testing.T) { CurrentBlock: 150, HighestBlock: 200, // CurrentBlock < HighestBlock => Done()=false }, - contacted: false, + ready: false, }) res, err := api.Syncing(context.Background()) if err != nil { diff --git a/internal/ethapi/transaction_args_test.go b/internal/ethapi/transaction_args_test.go index cd67190fcb..ba4816af2a 100644 --- a/internal/ethapi/transaction_args_test.go +++ b/internal/ethapi/transaction_args_test.go @@ -326,7 +326,7 @@ func (b *backendMock) ChainConfig() *params.ChainConfig { return b.config } func (b *backendMock) SyncProgress(ctx context.Context) ethereum.SyncProgress { return ethereum.SyncProgress{} } -func (b *backendMock) ConsensusContacted() bool { return true } +func (b *backendMock) ConsensusReady() bool { return true } func (b *backendMock) FeeHistory(ctx context.Context, blockCount uint64, lastBlock rpc.BlockNumber, rewardPercentiles []float64) (*big.Int, [][]*big.Int, []*big.Int, []float64, []*big.Int, []float64, error) { return nil, nil, nil, nil, nil, nil, nil } From 21b153df43d54554efd2c8e6926576de5f66a792 Mon Sep 17 00:00:00 2001 From: ozpool Date: Tue, 19 May 2026 13:53:34 +0530 Subject: [PATCH 3/3] 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, })