feat(l1): implement hand-rolled ws-based l1 client#3807
Conversation
8f5f35d to
7b248e1
Compare
7b248e1 to
760d78f
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an alternative L1 backend that uses a hand-rolled WebSocket JSON-RPC client, plus a hand-decoded Starknet core contract event decoder, allowing Juno to run its L1 sync/RPC paths without relying on go-ethereum’s client stack.
Changes:
- Add
--l1-client/l1-clientconfig to select betweengethandjunoL1 provider implementations. - Introduce a custom WS transport/client (unary +
eth_subscribe) and a hand-decodedLogStateUpdateevent pipeline. - Implement
EthL1StateProviderwith auto-redial on transport drops and comprehensive unit tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| node/node.go | Adds L1Client config and selects between geth-based vs hand-rolled L1 providers. |
| node/migration.go | Uses the selected L1 provider when fetching the L1 head for prune migrations; tweaks error strings. |
| l1/eth/contract/starknet.go | Hand-decodes Starknet core contract LogStateUpdate events and provides filtering helpers. |
| l1/eth/contract/starknet_test.go | Tests for signature hash, decoding correctness, and filtering behavior. |
| l1/eth/client/transport_ws.go | Implements WS JSON-RPC transport with routing, subscriptions, and ping keep-alives. |
| l1/eth/client/transport_ws_test.go | Transport-level tests for unary calls, subscriptions, ping behavior, and failure modes. |
| l1/eth/client/subscribe.go | Subscription API mirroring go-ethereum semantics and per-subscription dispatch. |
| l1/eth/client/rpc.go | JSON-RPC request/response types and helpers (id parsing, sub id decoding). |
| l1/eth/client/rpc_test.go | Unit tests for JSON-RPC error formatting. |
| l1/eth/client/filter_query.go | FilterQuery type with JSON marshaling tuned for eth_subscribe semantics. |
| l1/eth/client/clienttest/server.go | Shared test WS/HTTP JSON-RPC server used by client/provider tests. |
| l1/eth/client/client.go | High-level client methods (ChainID, block/header, logs, receipts) over WS transport. |
| l1/eth/client/client_test.go | Method-level tests and FilterQuery wire-shape tests. |
| l1/eth_l1_state_provider.go | New L1 state provider backed by the custom client, with auto-redial + subscription forwarding. |
| l1/eth_l1_state_provider_test.go | Provider tests covering redial behavior, listener firing, filtering, watch delivery, and errors. |
| cmd/juno/juno.go | Adds the --l1-client flag and usage text; wires it into categories. |
| cmd/juno/juno_test.go | Extends config precedence expectations to include the L1Client default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @brbrr's task in 6m 18s —— View job Review: hand-rolled ws-based L1 client
This is a well-structured PR. The transport's concurrency model is carefully thought through and the comments document the tricky invariants (cancel-vs-register races, single-shutdown fan-out, non-blocking reply delivery). I traced the goroutine lifecycles ( No blockers. A few things worth considering: Important
Nits
On the earlier Copilot comment (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
85a2391 to
fda1546
Compare
|
Claude finished @brbrr's task in 3m 46s —— View job Review: hand-rolled ws-based L1 client
I re-reviewed the full diff (17 files, The transport's concurrency model holds up under close reading. I re-walked every cancel/register/shutdown interleaving in No blockers.Important (standing)
Nits (standing)
On the Copilot
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
l1/eth/client/clienttest/server.go:97
- KillWSConns doesn't accept a status code parameter, but the comment claims it severs connections "with the given status code". Update the comment to match the current API so callers don't go hunting for a missing argument.
// KillWSConns severs every live websocket connection with the given
// status code. Useful for testing client-side resubscribe behaviour.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3807 +/- ##
==========================================
+ Coverage 75.42% 75.53% +0.11%
==========================================
Files 440 451 +11
Lines 39605 40551 +946
==========================================
+ Hits 29871 30632 +761
- Misses 7662 7810 +148
- Partials 2072 2109 +37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @brbrr's task in 4m 4s —— View job Review: hand-rolled ws-based L1 client (re-review)
All three standing findings from the earlier reviews are now resolved, and the code-quality asks (@rodrodros, Copilot) have been implemented. I re-traced the full concurrency model against the current tree and found no new races or leaks. Standing findings — all fixed
Also addressed
Concurrency re-trace (no findings)
Nit (optional, non-blocking)
No blockers. The PR is in good shape. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
l1/eth/client/clienttest/server.go:123
- Comment typo/grammar: "frames a" should be "frames that a" (or reflowed) so the sentence reads correctly.
// PushRawFrame writes data verbatim, so tests can inject malformed frames a
// well-behaved server would never emit.
|
Claude finished @brbrr's task in 3m 42s —— View job Review: hand-rolled ws-based L1 client (re-review after sync)
I re-reviewed the full diff after the latest sync ( No blockers.Nit (optional, non-blocking)
Re-verified as sound (no findings)
The PR is in good shape — the one nit above is optional and practically unreachable for the live |
|
Claude finished @brbrr's task in 2m 55s —— View job Review: hand-rolled ws-based L1 client (re-review after sync)
I re-reviewed the diff after the latest sync. The last remaining nit from the previous review — the forwarder decode-error path leaking the inner subscription — is now fixed, and the rest of the tree is unchanged and still sound. No blockers.Standing nit — now fixed ✅
Re-verified as sound (no findings)
The PR is in good shape — all previously raised findings and code-quality asks are resolved. No new races, leaks, or correctness issues found on the current tree. |
|
Claude finished @brbrr's task in 6m 19s —— View job Review: hand-rolled ws-based L1 client (re-review after sync)
I reviewed No blockers.Important
What this commit fixes — verified
Re-verified as sound (no findings)
Note on verification
|
| s.dialMu.Lock() | ||
| defer s.dialMu.Unlock() |
There was a problem hiding this comment.
[important] dialMu is not ctx-aware: a request-scoped caller can block here for up to redialTimeout (60s) past its own deadline.
dialMu is held across the whole client.New (line 125), so losers block in sync.Mutex.Lock() — which cannot be interrupted by ctx. Since one EthL1StateProvider serves both the sync loop and the RPC handlers (rpccore.L1Client.TransactionReceipt → starknet_getMessageStatus), an RPC request with a 1–2s deadline that trips ErrTransportClosed will sit here until the winner's dial finishes — up to a minute — before it can even observe its own cancellation. Against a blackholed/SYN-dropping endpoint that's the common case, and the handler goroutines pile up for the full window.
The previous s.mu-for-the-whole-dial version had the same shape, but the explicit 60s cap makes the worst case concrete and long, so it seems worth closing now. A ctx-aware semaphore keeps the coalescing while letting cancelled callers bail immediately:
// field: dialSem chan struct{} // buffered, cap 1
select {
case s.dialSem <- struct{}{}:
defer func() { <-s.dialSem }()
case <-ctx.Done():
return nil, ctx.Err()
}Two smaller notes on the same function, both benign as written:
stale.Close()(line 121) runs befores.clientis replaced, so for the whole dial windowcurrentClient()hands out a client that is already known-dead. It self-heals —fn(c)fails,redialblocks ondialMu, the loser picks up the winner's client — but every caller that starts in that window burns one wasted round-trip first.- Lock order is consistently
dialMu → clientMu, andClose()/currentClient()only takeclientMu, so there's no deadlock. The news.closedre-check after the dial (lines 131-137) correctly plugs the Close-races-dial leak. 👍
| if !n.cfg.DisableL1Verification { | ||
| // Due to mutually exclusive flag we can do the following. | ||
| if n.cfg.EthNode == "" { | ||
| return nil, fmt.Errorf("ethereum node address not found; Use --disable-l1-verification flag if L1 verification is not required") | ||
| } |
|
|
||
| useNewL1ClientUsage = "Use the experimental L1 client implementation." | ||
|
|
There was a problem hiding this comment.
no spaces between usage lines, please remove the lines above and below
| // it was answering, so there is no caller to route the reply to. | ||
| func parseResponseID(raw json.RawMessage) (uint64, error) { | ||
| trimmed := bytes.TrimSpace(raw) | ||
| if len(trimmed) == 0 || bytes.Equal(trimmed, jsonNull) { |
There was a problem hiding this comment.
Why not using the isJSonNull utility here?
| switch { | ||
| case probe.Method == "eth_subscription": | ||
| t.dispatchNotification(data) | ||
| case len(probe.ID) > 0 && !bytes.Equal(probe.ID, jsonNull): |
|
|
||
| const BlockFinalized = "finalized" | ||
|
|
||
| var jsonNull = []byte("null") |
There was a problem hiding this comment.
This can be declared direcly inside the isJsonNUll helper
| // readLoop drops malformed frames rather than tearing the transport down — | ||
| // a misbehaving remote manifests as a call timeout. | ||
| func (t *wsTransport) readLoop() { | ||
| for { | ||
| _, data, err := t.conn.Read(context.Background()) |
There was a problem hiding this comment.
Why is the parent context not being passed?
| func isJSONNull(raw json.RawMessage) bool { | ||
| return bytes.Equal(bytes.TrimSpace(raw), jsonNull) | ||
| } | ||
|
|
There was a problem hiding this comment.
Doesn't it makes more sense for this helper method to be moved to some other place like the end of the file? It sits a bit randomly between methods
| func (c *Client) ChainID(ctx context.Context) (*big.Int, error) { | ||
| raw, err := c.tr.call(ctx, "eth_chainId") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting chain ID: %w", err) | ||
| } | ||
| return decodeQuantityBig(raw) | ||
| } |
There was a problem hiding this comment.
Why a big.Int instead of a uint256?
|
|
||
| // HeaderByNumber maps a null result — the server's signal for "the named | ||
| // block does not exist yet" — to eth.ErrNotFound. | ||
| func (c *Client) HeaderByNumber(ctx context.Context, tag string) (*eth.Header, error) { |
There was a problem hiding this comment.
Why returning a pointer?
|
|
||
| // TransactionReceipt maps a null result (receipt unknown to the node) to | ||
| // eth.ErrNotFound. | ||
| func (c *Client) TransactionReceipt(ctx context.Context, txHash eth.Hash) (*eth.Receipt, error) { |
There was a problem hiding this comment.
Why returning a pointer?
| return &StateUpdate{ | ||
| L2BlockNumber: ev.BlockNumber, | ||
| L2BlockHash: ev.BlockHash, | ||
| StateRoot: ev.GlobalRoot, | ||
| L1RefHeight: uint64(ev.Raw.BlockNumber), | ||
| Removed: ev.Raw.Removed, | ||
| } |
No description provided.