feat(l1): [5] add EthL1StateProvider backed by the ws client - #3873
Conversation
|
Claude finished @brbrr's task in 5m 47s —— View job Review summary
Solid piece of work — the lifecycle handling is careful and I could not find a correctness bug in it. Specifically, I traced and found no issue with: the Findings below, all posted inline. Nothing here is a blocker. Important
Test coverage
Nits
Static review only — |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| select { | ||
| case s.dialSem <- struct{}{}: | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| } | ||
| defer func() { <-s.dialSem }() | ||
|
|
||
| s.clientMu.Lock() | ||
| if s.closed { | ||
| s.clientMu.Unlock() | ||
| return nil, ErrClosed | ||
| } | ||
| if s.client != stale { | ||
| // Someone else won the race; their fresh client is current. | ||
| c := s.client | ||
| s.clientMu.Unlock() | ||
| return c, nil | ||
| } | ||
| s.clientMu.Unlock() | ||
|
|
||
| stale.Close() | ||
| s.logger.Info("L1 transport closed; redialing") | ||
| dialCtx, cancel := context.WithTimeout(ctx, redialTimeout) | ||
| defer cancel() | ||
| c, err := client.New(dialCtx, s.url, s.clientOpts...) | ||
| if err != nil { | ||
| s.logger.Trace("L1 redial failed", zap.Error(err)) | ||
| return nil, fmt.Errorf("redialing L1: %w", err) | ||
| } |
There was a problem hiding this comment.
important — failed dials aren't remembered, so callers queue up N × 60s
The coalescing only works when a dial succeeds. On failure s.client is left pointing at stale, so the next caller that acquires dialSem re-evaluates s.client != stale as false and starts another full redialTimeout (60s) dial:
- caller A takes the sem, dials 60s, fails, releases
- caller B has been blocked on
s.dialSem <- struct{}{}that whole time, now takes it and dials another 60s - caller C waits 120s before even starting…
During a sustained L1 outage every queued caller serialises behind a fresh minute-long dial. That includes TransactionReceipt, which rpccore.L1Client calls from the starknet_getMessageStatus handler — so RPC requests can hang for minutes rather than failing fast (the caller's own ctx deadline is the only bound, and redial doesn't shorten it).
Worth recording the last failure under clientMu and short-circuiting subsequent callers within a small backoff window, so a burst of callers shares one dial attempt and its outcome:
lastDialErr error
lastDialFail time.Time…checked alongside the s.closed / s.client != stale conditions at line 113.
| defer cancel() | ||
| c, err := client.New(dialCtx, s.url, s.clientOpts...) | ||
| if err != nil { | ||
| s.logger.Trace("L1 redial failed", zap.Error(err)) |
There was a problem hiding this comment.
important — a failed redial is invisible at default log levels
Trace hides this. Previously reconnection was go-ethereum's problem; now that Juno owns it, a failing redial means the node has lost L1 connectivity entirely and the operator gets no signal (the only other line, "L1 transport closed; redialing" at :127, is Info and logs the attempt, not the outcome). Suggest Warn, matching how l1.Client.ensureChainID logs its retry loop.
| s.logger.Trace("L1 redial failed", zap.Error(err)) | |
| s.logger.Warn("L1 redial failed", zap.Error(err)) |
| ev.GlobalRoot.SetBytes(log.Data[0:32]) | ||
| ev.BlockHash.SetBytes(log.Data[64:96]) |
There was a problem hiding this comment.
important — SetBytes silently reduces mod p, which is inconsistent with the rest of this decoder
globalRoot and blockHash are uint256 on-chain but felt.Felt here. felt.SetBytes reduces modulo the STARK prime and cannot fail, so a 256-bit value ≥ p silently aliases to a different felt rather than being rejected. StateRoot ends up in core.L1Head and is served over RPC, so a silent alias is a correctness hazard, and this is untrusted input from whatever L1 endpoint the operator configured.
That sits oddly next to the defensive posture two lines up, where an out-of-range blockNumber is explicitly rejected rather than truncated. felt.SetBytesCanonical returns an error on non-canonical input and would make the three fields consistent:
ev := &LogStateUpdate{
BlockNumber: binary.BigEndian.Uint64(log.Data[56:64]),
Raw: *log,
}
if err := ev.GlobalRoot.SetBytesCanonical(log.Data[0:32]); err != nil {
return nil, fmt.Errorf("globalRoot not a canonical felt: %w", err)
}
if err := ev.BlockHash.SetBytesCanonical(log.Data[64:96]); err != nil {
return nil, fmt.Errorf("blockHash not a canonical felt: %w", err)
}
return ev, nilNote this is a deliberate behaviour change vs. the geth path (stateUpdateFromGethContract uses SetBigInt, which also reduces silently) — flagging so it's a conscious call rather than an accident of the rewrite. If bug-for-bug parity during the migration is preferred, a comment saying so would help.
| assert.Equal(t, uint64(1), got[0].L2BlockNumber) | ||
| assert.Equal(t, uint64(1_000), got[0].L1RefHeight) | ||
| assert.False(t, got[0].Removed) | ||
| require.NotNil(t, got[0].L2BlockHash) | ||
| require.NotNil(t, got[0].StateRoot) | ||
|
|
||
| assert.Equal(t, uint64(2), got[1].L2BlockNumber) | ||
| assert.Equal(t, uint64(1_001), got[1].L1RefHeight) | ||
| assert.True(t, got[1].Removed, "Removed flag must round-trip from the raw log envelope") |
There was a problem hiding this comment.
important — these two assertions are vacuous, and they're guarding the one mapping nothing else covers
StateUpdate.L2BlockHash and StateUpdate.StateRoot are felt.Felt values, not pointers (l1/l1_state_provider.go:16,19). require.NotNil on a non-nillable struct always passes, so lines 252-253 assert nothing.
That matters because stateUpdateFromContract (eth_l1_state_provider.go:268) is the only place GlobalRoot → StateRoot / BlockHash → L2BlockHash is wired up, and no test in the PR checks it. starknet_test.go:TestDecode_Success verifies the contract-level decode with distinct 0x11…/0x22… values, but if the two fields were transposed in stateUpdateFromContract — swapping the Starknet state root and block hash on every L1 head — the whole suite would still be green. stateUpdateLogJSON makes it worse: for blockNumber=7 it writes globalRoot=7 and blockHash=0, so the watch test wouldn't catch a swap either.
Give the two fields distinct, asserted values:
| assert.Equal(t, uint64(1), got[0].L2BlockNumber) | |
| assert.Equal(t, uint64(1_000), got[0].L1RefHeight) | |
| assert.False(t, got[0].Removed) | |
| require.NotNil(t, got[0].L2BlockHash) | |
| require.NotNil(t, got[0].StateRoot) | |
| assert.Equal(t, uint64(2), got[1].L2BlockNumber) | |
| assert.Equal(t, uint64(1_001), got[1].L1RefHeight) | |
| assert.True(t, got[1].Removed, "Removed flag must round-trip from the raw log envelope") | |
| assert.Equal(t, uint64(1), got[0].L2BlockNumber) | |
| assert.Equal(t, uint64(1_000), got[0].L1RefHeight) | |
| assert.False(t, got[0].Removed) | |
| assert.Equal(t, "0x1", got[0].StateRoot.String()) | |
| assert.Equal(t, "0x0", got[0].L2BlockHash.String()) | |
| assert.Equal(t, uint64(2), got[1].L2BlockNumber) | |
| assert.Equal(t, uint64(1_001), got[1].L1RefHeight) | |
| assert.True(t, got[1].Removed, "Removed flag must round-trip from the raw log envelope") |
(better still: change stateUpdateLogJSON to take independent globalRoot/blockHash args so the two can never collide.)
| srv.SetHandler(func(req clienttest.TestRequest) (any, *clienttest.TestRPCError) { | ||
| if req.Method == "eth_getLogs" { | ||
| return []any{ | ||
| stateUpdateLogJSON(1, 1_000, false), | ||
| stateUpdateLogJSON(2, 1_001, true), | ||
| }, nil | ||
| } | ||
| return nil, &clienttest.TestRPCError{Code: -32601, Message: req.Method} | ||
| }) |
There was a problem hiding this comment.
test coverage — nothing asserts the eth_getLogs request is well-formed
The handler ignores req.Params and unconditionally returns logs, and contract's own tests use fakeLogClient, which also discards the query. So no test in the PR verifies that FilterStateUpdate actually sends the contract address, the LogStateUpdate topic filter, or the right fromBlock/toBlock.
That leaves the newly hand-rolled FilterQuery marshalling unguarded: swapping FromBlock/ToBlock in contract.FilterLogStateUpdate (starknet.go:88-89), dropping the address, or emitting the range as decimal instead of hex would all pass. Since the whole point of this PR is replacing abigen's query construction, that's the part most worth pinning down. All the tests also pass eth.Address{}, so an omitted address is indistinguishable from a zero one.
Suggest asserting inside the handler with a real address:
srv.SetHandler(func(req clienttest.TestRequest) (any, *clienttest.TestRPCError) {
if req.Method == "eth_getLogs" {
gotQuery = req.Params // assert address / topics / 0x64 / 0xc8 after the call
return []any{...}, nil
}
...
})
| case <-time.After(time.Second): | ||
| t.Fatal("redial ignored ctx cancellation while another dial was in flight") | ||
| } | ||
| } |
There was a problem hiding this comment.
test coverage — the coalescing invariant itself is untested
This covers ctx cancellation while a dial is in flight, which is good, but the documented contract of redial is "coalesces concurrent callers: losers wait on dialSem and pick up the winner's client without dialing again" — and nothing asserts that. The s.client != stale fast path at eth_l1_state_provider.go:118 could be deleted and the whole suite would stay green, while every caller would then redial independently and thrash the endpoint.
A white-box test in this file could fire N goroutines at redial(ctx, c0) after killing the conn and assert exactly one new connection was established (the clienttest server can count accepted conns), plus that all N receive the same *client.Client pointer.
Worth adding a retry test for at least one non-ChainID method too — FilterStateUpdate, TransactionReceipt and WatchStateUpdate all go through withRetryOnClosed but only ChainID exercises the redial path (eth_l1_state_provider_test.go:47, :75). WatchStateUpdate is the interesting one, since its retry re-subscribes on the same raw channel against a different client.
| func (s *EthL1StateProvider) WatchStateUpdate( | ||
| ctx context.Context, | ||
| sink chan<- *StateUpdate, | ||
| ) (Subscription, error) { | ||
| raw := make(chan *eth.Log, watchForwarderBuffer) | ||
| inner, err := withRetryOnClosed(ctx, s, func(c *client.Client) (Subscription, error) { | ||
| return c.SubscribeLogs(ctx, contract.LogStateUpdateFilter(s.contractAddress), raw) | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nit — missing error wrap
Every other method's failure carries a call-site prefix, and the geth implementation wraps this one as fmt.Errorf("subscribing to LogStateUpdate: %w", err) (geth_l1_state_provider.go:175). Here a subscribe failure surfaces as a bare subscribing to logs: … from the client layer, with no indication it was the state-update watch. l1.Client.subscribeToUpdates logs this error on every retry, so the context is worth keeping.
Separately, WatchStateUpdate is the one L1StateProvider method with no s.observe(...) — the geth provider has the same gap, so this is parity rather than a regression, but if subscribe latency/failure rate is worth a metric, this is the place.
| func (s *EthL1StateProvider) FinalisedHeight(ctx context.Context) (uint64, error) { | ||
| defer s.observe("eth_getBlockByNumber")() | ||
| h, err := withRetryOnClosed(ctx, s, func(c *client.Client) (eth.Header, error) { | ||
| return c.HeaderByNumber(ctx, client.BlockFinalized) | ||
| }) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return uint64(h.Number), nil | ||
| } |
There was a problem hiding this comment.
nit — the not-found error reaches callers with no context
client.HeaderByNumber returns a bare eth.ErrNotFound for a null result (l1/eth/client/client.go:87), not a wrapped one, so FinalisedHeight propagates the literal string "not found" with nothing identifying the call. The geth path produced "finalised block not found: not found" (geth_l1_state_provider.go:122). TestEthL1StateProvider_PreservesErrNotFound only checks errors.Is, so the lost context isn't visible in the suite.
if err != nil {
return 0, fmt.Errorf("getting finalised Ethereum block: %w", err)
}still satisfies errors.Is(err, eth.ErrNotFound) while restoring the operator-facing detail.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/l1-ws-client #3873 +/- ##
=====================================================
+ Coverage 75.49% 75.58% +0.08%
=====================================================
Files 448 450 +2
Lines 40484 40623 +139
=====================================================
+ Hits 30563 30704 +141
- Misses 7797 7800 +3
+ Partials 2124 2119 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c727702 to
05fe09e
Compare
a102ccc to
6b9df44
Compare
brbrr
changed the title
6b9df44 to
f1bff2a
Compare
f1bff2a to
9e317a9
Compare
9e317a9 to
54e9f7a
Compare
0badb8c to
bd117c2
Compare
54e9f7a to
15d5092
Compare
15d5092 to
bb4dce5
Compare
1 participant
No description provided.