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 }