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`.
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_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.
