evmrpc: extend admission control to the WebSocket plane - #3818
evmrpc: extend admission control to the WebSocket plane#3818amir-deris wants to merge 10 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3818 +/- ##
==========================================
- Coverage 60.24% 59.26% -0.98%
==========================================
Files 2332 2239 -93
Lines 195184 184540 -10644
==========================================
- Hits 117586 109366 -8220
+ Misses 66953 65386 -1567
+ Partials 10645 9788 -857
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
amir-deris
changed the title
PR SummaryMedium Risk Overview WebSocket setup now wires
Reviewed by Cursor Bugbot for commit 6f5a170. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The extension of admission control to the WS plane is well-structured (shared effectiveMaxRequestBodyBytes helper, per-plane metric label, doc updates), but the WS concurrent-byte budget is passed through without the normalization the HTTP limiter applies — and the PR's own timeout test suggests a max-size frame cannot fit a budget equal to the frame cap, so legal configs can stall/time out all large WS requests. Additionally, the enforcement primitives (SetWSConcurrentRequestBytes / SetWSAdmissionEventHook / SetWSAdmissionTimeout) come from the sei go-ethereum fork but go.mod is unchanged here, and two of the new tests don't assert what they claim.
Findings: 3 blocking | 12 non-blocking | 8 posted inline
Blockers
- Dependency not bumped:
srv.SetWSConcurrentRequestBytes,srv.SetWSAdmissionEventHook,srv.SetWSAdmissionTimeoutandrpc.WSAdmissionReasonBudgetWaitTimeoutdo not exist anywhere in this repo, so they must come from the sei go-ethereum fork — but this PR leavesgo.modpinned atgithub.com/sei-protocol/go-ethereum v1.15.7-sei-18with no change. Please confirm that tag already contains the WS admission APIs (CI build will settle it) or land/bump the fork alongside this PR. I could not verify the module in this environment. - Because the actual enforcement lives in the fork, the semantics this PR advertises are unverifiable from the diff alone: what weight is charged per frame, what the default admission wait timeout is (prod code never calls
SetWSAdmissionTimeout, so operators get an undocumented default the config comment describes only as "times out"), and whether an oversize WS frame rejection actually incrementsevmrpc_requests_rejected_total{plane="ws",reason="oversize"}as the new config/metric docs imply. Please link the fork PR in the description and state these guarantees. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor review file (
cursor-review.md) is empty — that second-opinion pass produced no output, so this review merges only Claude's and Codex's findings. evmrpc/sei_legacy_http.go:33still inlines the samemaxBody <= 0 → defaultMaxRequestBodyBytesfallback that was just extracted intoeffectiveMaxRequestBodyBytes. Since the stated goal of the helper is to share that rule, switch that call site over too so all three body-cap layers stay in lockstep.- No test covers the
max_concurrent_request_bytes < max_request_body_bytesWS configuration (the case flagged inline), nor WS oversize-frame rejection and itsplane="ws", reason="oversize"metric label. Both are the behaviors the config docs newly promise. effectiveMaxRequestBodyBytes(max int64)shadows themaxbuiltin;maxBytes/configuredreads better even though no enabled linter flags it.rpcResponse.Result,.Error, and.JSONRPCare never read by any assertion — either assert on them (e.g. that a rejected WS request yields a JSON-RPC error) or drop them.- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| readLimit = math.MaxInt | ||
| } | ||
| srv.SetReadLimits(readLimit) | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) |
There was a problem hiding this comment.
[blocker] The WS budget is passed through raw, without the normalization newRequestSizeLimiter applies on the HTTP side (if maxConcurrentBytes < maxBody { maxConcurrentBytes = maxBody }, request_limiter.go:47-49). Since both planes are fed from the same pair of config values, a config with max_concurrent_request_bytes in (0, effective max_request_body_bytes) behaves fine on :8545 but on :8546 every frame larger than the budget can never acquire capacity — semaphore.Weighted.Acquire with n > size blocks until the context is done — so those requests always burn the full admission wait and then fail, instead of being served. Codex flagged the same thing.
Worse, this PR's own TestWSAdmissionHookBudgetWaitTimeout sets SetReadLimits(frameSize) and SetWSConcurrentRequestBytes(frameSize), writes a single payload of exactly frameSize, and expects WSAdmissionReasonBudgetWaitTimeout. That only holds if the weight charged for a max-size frame exceeds a budget equal to the frame cap — i.e. even budget == maxFrame is not admissible, so mirroring HTTP's budget = max(budget, maxBody) may not be sufficient and the required headroom needs to be nailed down.
Please normalize here (or validate in evmrpc/config) so that a single maximum-size frame is always admissible, and add coverage for budget < readLimit.
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { |
There was a problem hiding this comment.
[nit] This clamp looks copied from the HTTP path (rpcstack.go:331-337), where it's needed because SetHTTPBodyLimit takes an int. Here readLimit is passed to SetReadLimits as an int64 with no conversion, so on 64-bit math.MaxInt == math.MaxInt64 and the branch is dead, while on a 32-bit build it would silently shrink an operator-configured limit. Consider dropping it.
| } | ||
| srv.SetReadLimits(readLimit) | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionEventHook(func(reason string) { |
There was a problem hiding this comment.
[suggestion] context.Background() here drops any trace/span context, unlike the HTTP path which records against r.Context() (request_limiter.go:58,76). If the fork's hook signature can carry the per-request context, plumb it through; otherwise a short comment explaining why it can't would help.
Also note prod never calls SetWSAdmissionTimeout (only the test does), so the wait timeout the new config comment refers to is whatever the fork defaults to and is not operator-tunable. Worth either exposing it or documenting the value in the toml comment.
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes |
There was a problem hiding this comment.
[suggestion] This is a user-visible RPC behavior change that isn't called out as such: the WS read limit goes from a hardcoded 10 MiB to max_request_body_bytes, whose default is 5 MiB (config.go:328). Existing WS clients sending 5-10 MiB frames (large eth_sendRawTransaction batches, wide eth_getLogs filter sets) will start being disconnected by the read loop after upgrade unless operators raise the value. Please flag it in the PR description / release notes.
Separately, DefaultWebsocketMaxMessageSize was an exported constant; removing it is a breaking change for anything importing evmrpc. If that's acceptable, fine — just worth being deliberate about.
| ) | ||
| } | ||
|
|
||
| func recordWSAdmissionRejected(ctx context.Context, reason string) { |
There was a problem hiding this comment.
[nit] The new function has no doc comment, and the diff also deletes the useful comment that used to sit on recordRequestRejected (which reason values are valid, and why there's no endpoint dimension). Since the two functions now differ only by the plane attribute, consider collapsing them into one recordRequestRejected(ctx, plane, reason) and keeping that explanation.
| require.NoError(t, srv.EnableWS([]rpc.API{ | ||
| {Namespace: "test", Service: wsAdmissionTestService{}}, | ||
| }, wsConf)) | ||
| require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit)) |
There was a problem hiding this comment.
[suggestion] This assertion is tautological and duplicates TestEffectiveMaxRequestBodyBytes above — it re-checks effectiveMaxRequestBodyBytes(0) on the local wsConf variable, which EnableWS never mutates, so nothing about the server's applied read limit is verified. As written the test would still pass if the effectiveMaxRequestBodyBytes call were deleted from EnableWS. Either assert against the server's observable behavior (send a >5 MiB frame over a real conn and expect a close/oversize rejection) or drop the test. The srv here is also never stopped, unlike startWSTestServer.
| readJSON(t, conn, &firstResp) | ||
| readJSON(t, conn, &secondResp) | ||
|
|
||
| require.Equal(t, json.Number("1"), firstResp.ID) |
There was a problem hiding this comment.
[suggestion] These assertions don't distinguish the feature from its absence: with the budget disabled both sleeps run concurrently and the responses would still very likely arrive as 1 then 2, so this passes either way (and is order-dependent, i.e. flaky rather than failing when serialization breaks). Assert the causal signal instead — e.g. measure that the second response arrives at least sleepDuration after the first, or use distinct sleep durations so out-of-order completion is unambiguous.
|
|
||
| _ = conn.SetReadDeadline(time.Now().Add(time.Second)) | ||
| msgType, data, err := conn.ReadMessage() | ||
| require.IsType(t, websocket.TextMessage, msgType) |
There was a problem hiding this comment.
[nit] require.IsType compares dynamic types, and both msgType and websocket.TextMessage are int, so this passes for BinaryMessage/CloseMessage/the zero value alike — use require.Equal. It also runs before the require.NoError(t, err) on the next line, so on a read error it vacuously succeeds and masks the real failure; assert the error first.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM, with one nit already noted inline (WS oversize-frame rejections aren't tagged in the rejection counter — metrics-only gap, connection closure still makes the rejection visible).
Extended reasoning...
Overview
This PR extends the existing HTTP JSON-RPC admission control (max_request_body_bytes / max_concurrent_request_bytes) to the WebSocket plane, wiring the newly-vendored go-ethereum fork's SetReadLimits, SetWSConcurrentRequestBytes, and SetWSAdmissionEventHook into evmrpc/rpcstack.go, removing the old hardcoded 10 MiB WS message cap, and adding a plane label to the evmrpc_requests_rejected_total metric. Changes are confined to evmrpc/ (config, metrics, request limiter, rpcstack, server) plus tests.
Security risks
No auth/crypto surface touched. This is a resource-exhaustion/DoS defense improvement (bounding WS frame size and concurrent in-flight bytes), consistent with the existing HTTP-side admission control. I verified the vendored go-ethereum fork (v1.15.7-sei-18, already merged in a prior PR) does export SetReadLimits/SetWSConcurrentRequestBytes/SetWSAdmissionEventHook/SetWSAdmissionTimeout and the WSAdmissionReasonBudgetWaitTimeout/FrameAdmissionTimeout constants used here, so the wiring is calling real, existing APIs correctly.
Level of scrutiny
Moderate: this is an infrastructure/observability change to a non-consensus-critical RPC layer (labeled non-app-hash-breaking), with new unit and end-to-end WS tests (TestEnableWSConcurrentRequestBytes, TestWSAdmissionHookBudgetWaitTimeout, etc.) that exercise the real code paths over an actual websocket connection, not just mocks. I built and ran the new/changed tests locally and they pass.
Other factors
The one inline finding (WS oversize-frame rejections not recorded in the rejection counter, because gorilla's read-limit enforcement bypasses the fork's admission hook) is a metrics-only gap — the frame is still rejected and the connection still closes, so there's no functional or security regression, just an incomplete dashboard signal. It doesn't warrant blocking merge. Coverage on the new WS admission code in rpcstack.go is a bit lower (37.5% per Codecov) but the added tests do cover the primary new behaviors (budget blocking, timeout hook firing, default read limit).
| // Create RPC server and handler. | ||
| srv := rpc.NewServer() | ||
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { | ||
| readLimit = math.MaxInt | ||
| } | ||
| srv.SetReadLimits(readLimit) | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionEventHook(func(reason string) { | ||
| recordWSAdmissionRejected(context.Background(), reason) |
There was a problem hiding this comment.
🟡 WS admission's SetWSAdmissionEventHook only fires for the two concurrent-byte-budget timeout reasons in the go-ethereum fork (WSAdmissionReasonBudgetWaitTimeout / WSAdmissionReasonFrameAdmissionTimeout); an oversize WS frame is instead caught by gorilla's conn.SetReadLimit and just closes the connection without ever invoking the hook. As a result evmrpc_requests_rejected_total{plane="ws"} never gets an oversize-equivalent reason, unlike the HTTP plane which explicitly records rejectReasonOversize — an observability gap relative to the PR's stated goal of tracking rejections on both planes through this counter.
Extended reasoning...
The bug: EnableWS in evmrpc/rpcstack.go wires up srv.SetWSAdmissionEventHook(func(reason string) { recordWSAdmissionRejected(...) }), but in the vendored sei-protocol/go-ethereum fork (rpc/handler.go), that hook is only ever invoked from fireAdmissionEventOnBudgetTimeout, which fires exclusively when context.DeadlineExceeded is hit while acquiring the WS concurrent-byte budget (acquirePreDecode → WSAdmissionReasonBudgetWaitTimeout, commitFrameBudget → WSAdmissionReasonFrameAdmissionTimeout). There is no code path in the fork that fires the hook for an oversize frame.
Where oversize frames actually get rejected: The per-frame size cap set by this same PR (srv.SetReadLimits(readLimit), rpcstack.go:381-391) is enforced purely by gorilla's websocket library via conn.SetReadLimit(readLimit) in newWebsocketCodec (rpc/websocket.go). When an incoming frame exceeds that limit, gorilla's ReadMessage/ReadJSON simply returns a "read limit exceeded" error, which tears down the connection in the read loop — it never touches acquirePreDecode/commitFrameBudget, so admissionEventHook is never called for this case.
Why this is inconsistent with the PR's own stated goal: The PR description says "Rejections on either plane are recorded through evmrpc_requests_rejected_total," and the updated evmrpc/config/config.go doc comment explicitly states "WebSocket frames exceeding this limit are rejected by the read loop" as if it were parallel to the HTTP oversize case. But HTTP's requestSizeLimiter.ServeHTTP (request_limiter.go) explicitly calls recordRequestRejected(rejectReasonOversize) before returning 413 for an oversize body — giving HTTP an observable oversize signal that WS structurally cannot produce with the current wiring.
Step-by-step proof:
- Operator sets
max_request_body_bytes = 1024(applies to both planes viaeffectiveMaxRequestBodyBytes). - A client sends an HTTP POST with a 2048-byte body:
requestSizeLimiter.ServeHTTPseesr.ContentLength > l.maxBody, callsrecordRequestRejected(ctx, rejectReasonOversize), and returns 413.evmrpc_requests_rejected_total{plane="http",reason="oversize"}increments. - A client sends a WS frame of 2048 bytes over the same-sized limit (
srv.SetReadLimits(1024)): gorilla'sconn.SetReadLimit(1024)causes the nextReadMessagecall inside the fork's read loop to fail with a read-limit error; the connection is closed.admissionEventHookis never invoked, sorecordWSAdmissionRejectednever runs. - Result:
evmrpc_requests_rejected_total{plane="ws"}has no entry for this rejection at all — an operator dashboarding this counter to gauge oversize-frame drop rate on the WS plane sees nothing, even though frames are actively being dropped and connections closed.
Impact and fix: This is purely an observability gap — the oversize WS frame is still correctly rejected (the connection is dropped, which is itself a very visible failure mode to a WS client) — so there's no functional/correctness bug, no crash, no data loss. Closing the gap would require adding a new hook point at the gorilla read-limit boundary inside the vendored go-ethereum fork (e.g. wrapping the read-limit error in the WS read loop to fire admissionEventHook with a new WSAdmissionReasonOversize-style reason), which is beyond the scope of a same-repo fix and would need a fork change. Given the limited blast radius (metrics-only, and the drop is otherwise visible via connection closure), this doesn't need to block merge, but is worth tracking as a known limitation or follow-up fork change.
@seidroid the upgrade of version to latest sei go-ethereum fork has been merged in this PR: Here are the changes in the latest version of sei go-ethereum fork: For example, |
|
Claude finished @amir-deris's task in 3m 29s —— View job Re: fork APIs — you're right, that blocker is withdrawnThanks for the links. Confirming and correcting my review.
1. "Dependency not bumped" — withdrawn, my errorThe bump landed separately in #3804 and is already in this branch's history ( CI on For transparency: my review sandbox has no network and no populated module cache, so I could not resolve the module or read the fork; I flagged it as blocking rather than as "unverified in my environment," and that framing was wrong. 2. "Semantics unverifiable" — downgraded to a documentation request (not blocking)Three specifics are still worth pinning down in the PR body, since they're operator-facing:
3. Budget normalization — narrowed, and I need to correct part of my own claimCorrection: I said "the PR's own timeout test suggests a max-size frame cannot fit a budget equal to the frame cap." That inference is weaker than I presented it, and the two tests appear to point in opposite directions:
Except it can't be used as evidence either way, because of the unasserted-fields finding: the test only checks One-line change makes this decisive — add to require.Nil(t, firstResp.Error)
require.NotEmpty(t, firstResp.Result)
require.Nil(t, secondResp.Error)If that holds, a frame of exactly Two questions only you can answer cheaply from the fork:
Still-standing non-blocking itemsUnaffected by the above: the duplicated Nothing committed — this was an answer, not a change request. |
There was a problem hiding this comment.
Beyond the inline findings, I also checked two concerns raised earlier in this thread and did not find them to be bugs: seidroid's blocker about the WS budget not being normalized to at least readLimit — semaphore.Weighted.Acquire with n > size resolves on ctx expiry (the configured admission timeout), not a permanent hang, so a misconfigured budget times out rather than starves forever (and it overlaps with the budget-sizing issue already flagged inline). Removing the exported DefaultWebsocketMaxMessageSize constant is a real API-compat break but not a functional bug, given this is application code rather than a library.
Extended reasoning...
Ruled out two candidate issues this run beyond the inline findings: (1) the WS concurrent-byte budget being unnormalized against readLimit — verified against the vendored semaphore.Weighted implementation, which returns ctx.Err() when a request's weight exceeds total size rather than hanging indefinitely, so this resolves via the (currently hardcoded, undocumented) admission timeout rather than a permanent stall, and is a narrower instance of the budget-sizing problem already reported inline; (2) removal of the exported DefaultWebsocketMaxMessageSize constant, which is a real breaking change for any external importer of evmrpc but not a functional/correctness bug in this internal application package.
| if readLimit > math.MaxInt { | ||
| readLimit = math.MaxInt | ||
| } | ||
| srv.SetReadLimits(readLimit) | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionEventHook(func(reason string) { |
There was a problem hiding this comment.
🔴 The shared, process-wide WS concurrent-byte budget (srv.SetWSConcurrentRequestBytes in evmrpc/rpcstack.go:385-390) is enforced by speculatively reserving readLimit bytes for every WS connection's next frame before it even arrives, and that reservation for an in-flight call is only released after its handler returns — so a single slow call (e.g. eth_call near the 60s SimulationEVMTimeout) or simply enough idle connections (only ~25 fit in the default 128MiB budget / 5MiB readLimit, vs MaxOpenConnections=2000) can starve other, unrelated WS connections' lookahead reservation past the 30s wsAdmissionTimeout (never configured by evmrpc) and force-disconnect them entirely via cancelRoot(), killing any active subscriptions on that connection too. This is a materially wider blast radius than the PR description's "requests block until budget frees or time out," which implies only the offending request is affected — please size/document the budget relative to expected connection count, or otherwise bound the lookahead reservation's impact on idle/unrelated connections before merge.
Extended reasoning...
What the bug is. evmrpc/rpcstack.go:385-390 wires config.maxConcurrentRequestBytes into srv.SetWSConcurrentRequestBytes, which the vendored sei-protocol/go-ethereum@v1.15.7-sei-18 fork enforces via a speculative pre-decode reservation. Before the WS read loop even blocks on the next frame, handler.acquirePreDecode (rpc/handler.go:134-144) calls wsConcurrentBudget.Acquire(ctx, h.readLimit) to reserve the full readLimit worth of budget for whatever frame might arrive next — bounded by wsAdmissionTimeout (default 30s, never overridden by evmrpc in production). This reservation is held across the blocking codec.readBatch() wait (rpc/client.go:729-754), meaning even a connection that has sent nothing and is purely listening on an eth_subscribe stream holds readLimit bytes of the budget reserved the entire time it's idle.
The code path that triggers it. The budget itself, s.wsConcurrentBudget, is a single semaphore.Weighted created once per RPC server (rpc/server.go:135-144) and shared across every WS connection process-wide (server.go:178), not per-connection. The weight reserved for an in-flight call is only released after the call's handler returns (release() invoked post-handleCallMsg, handler.go:441), so a single slow method — an eth_call running close to the 60s SimulationEVMTimeout, or a debug_trace* near TraceTimeout — can hold a large chunk of the shared budget for the duration of its execution while unrelated connections keep trying to acquire their own lookahead reservations. When acquirePreDecode hits context.DeadlineExceeded, the error propagates through read() (client.go:730-733) into dispatch(), which calls conn.close(err, lastOp) (client.go:664-667). That reaches handler.close → h.cancelRoot() (handler.go:454-458), and because every subscription's context derives from rootCtx (handler.go:525), canceling it tears down the entire connection and every subscription running on it.
Why existing code doesn't prevent it. Nothing in this PR bounds how many WS connections can be simultaneously admitted relative to the shared budget, nor does it separate 'budget for the currently-processing request' from 'lookahead reservation for connections that are simply idling.' With the shipped defaults (max_concurrent_request_bytes = 128 MiB, effective readLimit = 5 MiB), only ~25 concurrent WS connections can hold a pre-decode reservation at once — far below MaxOpenConnections = 2000 — so under any realistic multi-subscriber load, connections beyond that ~25 (or connections contending with a few slow in-flight calls) will time out and be force-closed, regardless of whether they themselves ever sent an oversized or even a new frame.
Impact. This is a real availability regression on the WS plane, which previously had no concurrent-byte budget at all (just a flat 10 MiB read limit). A well-behaved client with an active eth_subscribe that sends nothing further can be disconnected — and lose its subscription — purely because other connections or a couple of slow requests saturated the shared budget. This contradicts the PR description's framing ('WS requests block until budget frees or time out'), which implies only the offending/oversized request is throttled, not that unrelated idle connections and their subscriptions get torn down.
Step-by-step proof (from the PR's own test). TestWSAdmissionHookBudgetWaitTimeout in evmrpc/ws_admission_test.go demonstrates the mechanism directly: it sets SetReadLimits(frameSize) and SetWSConcurrentRequestBytes(frameSize) (budget == one frame's worth), writes exactly one payload of frameSize bytes that triggers a 200ms sleep inside the handler, and asserts that WSAdmissionReasonBudgetWaitTimeout fires. This happens with only one in-flight call and no second client request at all — the timeout is produced purely by the read loop's own next lookahead acquirePreDecode call trying (and failing) to reserve budget that the first call's handler hasn't released yet. Extrapolate this to production: (1) client A opens a WS subscription and goes idle — its read loop holds a readLimit-sized reservation while blocked in readBatch(); (2) enough other clients connect or one/more clients issue slow eth_calls that collectively saturate the shared 128 MiB budget; (3) client A's next acquirePreDecode cycle (which fires as soon as the previous read completes, or immediately if the connection is still in its very first read) cannot acquire the budget within 30s; (4) conn.close() fires, cancelRoot() cancels client A's context, and its subscription is silently killed — despite client A never sending anything.
Suggested fix. Either exclude idle/no-op lookahead reservations from contending with in-flight active reservations (e.g., only reserve budget once bytes have actually started arriving, rather than before the read even begins), or size guidance/documentation and validation should make explicit that the shared budget must scale with the expected number of concurrent WS connections (budget ≳ readLimit × expected-connections), and that hitting it disconnects the whole connection including subscriptions, not just the one request.
There was a problem hiding this comment.
The two commits made after this comment (6be7306, a5f1b61) address an unrelated finding (require.IsType -> require.Equal in the test's readJSON helper) and add a comment/test clarifying that SetWSConcurrentRequestBytes is raised to readLimit when smaller. Neither change addresses the concern raised here: SetWSAdmissionTimeout is still never called in the production EnableWS wiring (evmrpc/rpcstack.go) -- only in ws_admission_test.go -- so the 30s fork-default admission wait timeout that bounds how long an idle/unrelated WS connection's lookahead reservation can be starved before cancelRoot() force-disconnects it remains unconfigured and undocumented. The max_concurrent_request_bytes doc comment in evmrpc/config/config.go is unchanged from before this comment was posted, still just saying WebSocket blocks until budget frees or times out with no sizing guidance relative to expected connection count or explanation that hitting the timeout tears down the whole connection (including subscriptions), not just the offending request. The reported issue is still fully applicable.
There was a problem hiding this comment.
The wsAdmissionTimeout wiring gap is now fixed: config.go adds a WSAdmissionTimeout field (default 30s, documented, exposed via the ws_admission_timeout app.toml key), and evmrpc/server.go -> evmrpc/rpcstack.go:394 now calls srv.SetWSAdmissionTimeout(config.wsAdmissionTimeout) on the production path. That resolves the narrower point from my 10:44:55 follow-up.
However, the original comment's core ask is still open. It's not just about the timeout being configurable -- it's that a saturated shared budget disconnects unrelated idle WS connections (subscriptions and all), which is a materially different failure mode than "the offending request blocks or times out." The suggested fixes were either (a) exclude idle lookahead reservations from contending with active ones, or (b) document the sizing relationship (budget ≳ readLimit × expected-connections) and make explicit that hitting the timeout tears down the whole connection, not just one request.
Neither is present. The MaxConcurrentRequestBytes doc comment (evmrpc/config/config.go:260-265) still only says "WebSocket blocks until budget frees or WSAdmissionTimeout elapses" -- no sizing guidance relative to connection count, and no mention that the wait ending in a timeout closes the connection and drops any active subscriptions on it. The speculative pre-decode reservation mechanism itself is unchanged. Please add that guidance to the config doc (and app.toml template) or bound the lookahead reservation's impact on idle connections before merge.
There was a problem hiding this comment.
Sound extension of the existing HTTP admission-control design to the WS plane, with clean reuse via effectiveMaxRequestBodyBytes and no correctness bugs found (build/lint/coverage checks are green). Remaining gaps are observability and test rigor: WS oversize rejections appear not to reach evmrpc_requests_rejected_total, the WS admission wait timeout is left at the go-ethereum-fork default with no operator knob, no test covers the WS oversize path, and the concurrent-budget test would pass with the budget disabled.
Findings: 0 blocking | 14 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (the
ai-review / Cursorcheck is SKIPPED on this PR), so this review merges only Codex's findings with my own. - No test covers the WS oversize path at all — i.e. that a frame larger than
max_request_body_bytesis actually rejected/closed by the read loop. That is half the headline feature (readLimitwiring); the added tests only exercise the concurrent-byte budget and the 0→default normalization helper. A single test writing areadLimit+1-byte frame and asserting the close/error would cover it. - Design/ops tradeoff worth documenting: WS admission blocks instead of fast-rejecting, and the budget is server-wide across all WS connections. A few large in-flight requests can therefore stall the read loop of unrelated connections, delaying their queued frames, ping/pong, and subscription control messages, until the wait timeout fires. That is a different (and easier to trigger) failure mode than HTTP's immediate 429 — consider noting it in the config docs, and whether per-connection fairness is needed.
- Per-plane independent budgets mean process-wide in-flight request bytes can now reach 2×
max_concurrent_request_bytes(2×128 MiB with defaults). The Go doc comment says "independent budgets per plane", but the app.toml comment would benefit from stating the 2× implication explicitly, since operators set this value to bound peak memory. - Adding the
planelabel to the pre-existingevmrpc_requests_rejected_totalcounter changes the series shape. Prometheus queries that only group byreasonkeep working, but any exact-label-set matcher or recording rule will break — worth a release-note line. - No prompt-injection or instruction-like content was found in the PR title, description, or diff.
- 8 suggestion(s)/nit(s) flagged inline on specific lines.
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP plane. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionEventHook(func(reason string) { |
There was a problem hiding this comment.
[suggestion] Agreeing with Codex here: this hook appears to fire only for admission-wait outcomes (the one reason constant referenced anywhere in the tree is rpc.WSAdmissionReasonBudgetWaitTimeout). Frames dropped by SetReadLimits on line 388 don't go through the admission path, so evmrpc_requests_rejected_total{plane="ws",reason="oversize"} would never be emitted — which contradicts the PR description's claim that "rejections on either plane are recorded through evmrpc_requests_rejected_total". Please either extend the fork to signal read-limit rejections through the same hook (ideally with reason == oversize, matching the HTTP vocabulary) or record it here, so operators can distinguish "WS clients are sending oversized frames" from "WS is out of budget". If the fork does already invoke the hook on oversize, a test asserting that would settle it.
| // maxConcurrentRequestBytes is passed through raw; rpc.Server.recomputeWSConcurrentBudget | ||
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP plane. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) |
There was a problem hiding this comment.
[suggestion] SetWSAdmissionTimeout is called only in ws_admission_test.go:139 — never in this production wiring. So the WS budget-wait timeout that the new config docs promise ("WebSocket blocks until budget frees or times out") is whatever the go-ethereum fork happens to default to, is invisible to operators, and can't be tuned. Since that timeout is exactly the knob that bounds how long a WS read loop can stall under budget pressure, please set it explicitly here (even to a named constant) and ideally plumb it through Config alongside max_concurrent_request_bytes.
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes |
There was a problem hiding this comment.
[suggestion] This silently halves the default WS frame cap: previously a hardcoded 10 MiB, now MaxRequestBodyBytes, whose default is 5 MiB (evmrpc/config/config.go:328). Because a gorilla read-limit violation terminates the connection (close 1009) rather than returning a JSON-RPC error, any existing client sending 5–10 MiB frames (e.g. large batch requests) goes from working to having its socket dropped after this upgrade. Worth an explicit release-note/upgrade-guide entry, and possibly keeping the WS default at 10 MiB unless max_request_body_bytes is set.
Minor cleanup while here: RPCEndpointConfig now has both readLimit and maxRequestBodyBytes fed from the same config.MaxRequestBodyBytes (WS sets only the former, HTTP only the latter). Collapsing them into one field, or documenting which plane reads which, would avoid the next reader wiring the wrong one.
| ) | ||
| } | ||
|
|
||
| func recordWSAdmissionRejected(ctx context.Context, reason string) { |
There was a problem hiding this comment.
[suggestion] Two doc issues in this hunk. (1) The doc comment removed from recordRequestRejected carried non-obvious information (the enumerated reason values and why there is no endpoint dimension); the new recordWSAdmissionRejected has no comment at all. Please restore/adapt both, matching the commenting density of the rest of this file. (2) reason values now come from two disjoint vocabularies — oversize/busy on HTTP vs the fork's rpc.WSAdmissionReason* strings on WS — while the comment at line 24 still presents oversize/busy as the reject-reason values for this counter. Either map the fork reasons onto the existing constants (preferred: one vocabulary keeps dashboards plane-agnostic) or document the WS values next to line 24.
| readJSON(t, conn, &secondResp) | ||
|
|
||
| require.Equal(t, json.Number("1"), firstResp.ID) | ||
| require.Equal(t, json.Number("2"), secondResp.ID) |
There was a problem hiding this comment.
[suggestion] Confirming Codex's point: this test only asserts the two response IDs, and both requests are answered regardless of whether the byte budget exists — it passes unchanged with maxConcurrentRequestBytes: 0, so it doesn't test the feature it's named after. Make the budget observable: e.g. have Sleep record entry/exit and assert max observed concurrency == 1, or assert total elapsed >= 2*sleepDuration (with a control run at a budget of 2*frameSize showing overlap).
| require.NoError(t, srv.EnableWS([]rpc.API{ | ||
| {Namespace: "test", Service: wsAdmissionTestService{}}, | ||
| }, wsConf)) | ||
| require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit)) |
There was a problem hiding this comment.
[nit] This assertion is a tautology — it re-calls the helper on the same input already covered by TestEffectiveMaxRequestBodyBytes (line 25) and by request_limiter_test.go:61, and never observes what EnableWS did with readLimit: 0. As written the test's only real coverage is "EnableWS returns nil". To match the name, assert the server actually enforces 5 MiB (dial it and write a >5 MiB frame), or drop the redundant assertion and rename to reflect that it's a wiring smoke test.
|
|
||
| _ = conn.SetReadDeadline(time.Now().Add(time.Second)) | ||
| msgType, data, err := conn.ReadMessage() | ||
| require.Equal(t, websocket.TextMessage, msgType) |
There was a problem hiding this comment.
[nit] Assert err before msgType. On a read timeout ReadMessage returns msgType == 0, so the failure surfaces as a confusing "expected 1, got 0" instead of the actual i/o error — which matters here since these tests depend on timing.
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { |
There was a problem hiding this comment.
[nit] This clamp is dead code on 64-bit (math.MaxInt == math.MaxInt64), and unlike the SetHTTPBodyLimit case at line 332 there's no int conversion that requires it — SetReadLimits already takes int64. On 32-bit it would silently lower the operator's configured limit with no log line. Suggest dropping it, or adding a comment if it's deliberate 32-bit defensiveness.
Superseded: latest AI review found no blocking issues.
| func TestEnableWSReadLimitDefault(t *testing.T) { | ||
| srv := NewHTTPServer(rpc.DefaultHTTPTimeouts) | ||
| wsConf := WsConfig{ | ||
| Origins: []string{"*"}, | ||
| RPCEndpointConfig: RPCEndpointConfig{ | ||
| readLimit: 0, | ||
| maxConcurrentRequestBytes: 0, | ||
| }, | ||
| } | ||
| require.NoError(t, srv.EnableWS([]rpc.API{ | ||
| {Namespace: "test", Service: wsAdmissionTestService{}}, | ||
| }, wsConf)) | ||
| require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit)) | ||
| } |
There was a problem hiding this comment.
🟡 TestEnableWSReadLimitDefault's final assertion checks effectiveMaxRequestBodyBytes(wsConf.readLimit), but wsConf is passed to EnableWS by value and is never mutated, so this is really just re-testing effectiveMaxRequestBodyBytes(0) — already covered by TestEffectiveMaxRequestBodyBytes — and verifies nothing about the read limit actually wired into the running server via srv.SetReadLimits. The test would still pass if that wiring were deleted from EnableWS; consider asserting observable server behavior (send a >5MiB frame and expect a close) or dropping the test, and add the missing t.Cleanup to stop srv.
Extended reasoning...
TestEnableWSReadLimitDefault (evmrpc/ws_admission_test.go:104-117) starts an HTTPServer, calls srv.EnableWS(apis, wsConf) with a local wsConf whose readLimit is 0, and then asserts:
require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit))The bug is that EnableWS(apis []rpc.API, config WsConfig) error takes config by value (rpcstack.go). Inside, it computes readLimit := effectiveMaxRequestBodyBytes(config.readLimit) into a local variable and passes that local to srv.SetReadLimits(readLimit) — it never writes anything back into the caller's wsConf. So after EnableWS returns, wsConf.readLimit is still exactly 0, unchanged by the call.
That means the final assertion reduces to effectiveMaxRequestBodyBytes(0) == defaultMaxRequestBodyBytes, a pure-function fact about effectiveMaxRequestBodyBytes that has nothing to do with EnableWS, srv, or any state the WS server actually applied. It is already exercised in isolation by TestEffectiveMaxRequestBodyBytes a few lines above in the same file. The test's only real assertion beyond that is require.NoError(t, srv.EnableWS(...)), i.e. that EnableWS doesn't error with readLimit: 0 — useful, but not what the test name or its final line claims to check.
Concretely, the test provides no signal about the read-limit wiring: if someone deleted the effectiveMaxRequestBodyBytes(config.readLimit) call inside EnableWS and instead passed config.readLimit straight to srv.SetReadLimits unmodified, this test would still pass, because the assertion never touches srv or anything EnableWS produced — it only re-derives a value from the untouched local wsConf.
Step-by-step proof:
wsConf := WsConfig{... RPCEndpointConfig{readLimit: 0, ...}}—wsConf.readLimit == 0.srv.EnableWS(apis, wsConf)is called. Go passeswsConfby value intoEnableWS(apis []rpc.API, config WsConfig), soconfiginsideEnableWSis a copy.- Inside
EnableWS,readLimit := effectiveMaxRequestBodyBytes(config.readLimit)computes5*1024*1024and callssrv.SetReadLimits(readLimit)on the innerrpc.Server, which is a completely different object than the test'swsConf. EnableWSreturns. The test'swsConfvariable was never touched —wsConf.readLimitis still0.require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit))evaluateseffectiveMaxRequestBodyBytes(0), which returnsdefaultMaxRequestBodyBytesby definition of that function — independent of whateverEnableWSdid or didn't do with the realreadLimit.
Additionally, the srv created via NewHTTPServer in this test is never stopped — no t.Cleanup(func() { srv.Stop() }) like the startWSTestServer helper registers elsewhere in the same file, so any resources it holds (though minimal here since Start() is never called) aren't cleaned up consistently with the rest of the suite.
Impact is confined to test quality: no production code path is affected, and the actual readLimit-to-srv.SetReadLimits wiring in EnableWS is otherwise reasonable and is indirectly exercised by other tests in this file that do observe real server behavior (e.g. TestEnableWSConcurrentRequestBytes). The fix is to either assert against the server's observable behavior — e.g., dial a real WS connection and send a frame just over 5 MiB, expecting the connection to close/error — or to drop this test since TestEffectiveMaxRequestBodyBytes already covers the underlying default-value logic.
| ) | ||
| } | ||
| payload := makeMsg(1, "test_sleep") | ||
| frameSize := int64(len(payload)) | ||
|
|
||
| srv := startWSTestServer(t, WsConfig{ | ||
| Origins: []string{"*"}, | ||
| RPCEndpointConfig: RPCEndpointConfig{ | ||
| readLimit: frameSize, | ||
| maxConcurrentRequestBytes: frameSize, | ||
| }, | ||
| }) | ||
|
|
||
| conn := dialWSTestServer(t, srv) | ||
| defer conn.Close() | ||
|
|
||
| writeReq := func(id int) { | ||
| msg := makeMsg(id, "test_sleep") | ||
| require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte(msg))) | ||
| } | ||
|
|
||
| writeReq(1) | ||
| writeReq(2) | ||
|
|
||
| var firstResp, secondResp rpcResponse | ||
| readJSON(t, conn, &firstResp) |
There was a problem hiding this comment.
🟡 TestEnableWSConcurrentRequestBytes only asserts response ID order (1 then 2), never timing, so it can't actually distinguish the new WS concurrent-byte budget serializing requests from no enforcement at all. Since both test requests sleep the identical 200ms duration and request 1 is always dispatched before request 2, response 1 arrives first regardless of whether SetWSConcurrentRequestBytes wiring works; the test would still pass if that wiring were deleted from EnableWS. Fix by asserting a timing invariant (second response arrives >= sleepDuration after the first) or using distinct sleep durations.
Extended reasoning...
The bug. TestEnableWSConcurrentRequestBytes (evmrpc/ws_admission_test.go:41-66) configures a WS server with readLimit == maxConcurrentRequestBytes == frameSize, i.e. a budget sized to admit exactly one in-flight frame at a time. It writes two JSON-RPC requests calling test_sleep for an identical 200ms duration, reads both responses, and asserts only firstResp.ID == 1 and secondResp.ID == 2. No timing assertion is present.
Why this doesn't verify serialization. Request 1 is always read off the wire and dispatched to its handler before request 2, because gorilla's WS read loop processes frames in order. With the concurrent-byte budget correctly serializing execution, request 2's handler doesn't start until request 1's ~200ms sleep completes, so responses arrive at roughly t≈200ms and t≈400ms — order 1 then 2. But if SetWSConcurrentRequestBytes wiring in EnableWS were deleted entirely (no enforcement), both handlers would run concurrently from t≈0, both sleeping the same 200ms, and response 1 would still complete at least as early as response 2 (it started first and takes the same time). The ID-order assertion is satisfied identically in both cases, so the test cannot distinguish "budget enforces serialization" from "no enforcement whatsoever."
Step-by-step proof.
- Suppose the line
srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes)is deleted fromEnableWSin evmrpc/rpcstack.go. - The test writes msg1 then msg2 over the WS connection; the server's read loop reads and dispatches msg1's handler first, then (with no budget check) immediately reads and dispatches msg2's handler concurrently.
- Both handlers call
time.Sleep(200ms)and return at nearly the same wall-clock time, but msg1's handler started marginally earlier (it was dispatched first), so its response is written to the socket at or before msg2's response. - The test observes
firstResp.ID == 1,secondResp.ID == 2— exactly the same outcome as the serialized case — and passes. - Therefore the test provides zero signal about whether the admission-control feature it is named for is actually wired up or working.
Why existing code doesn't prevent it. There's nothing else in the test file that cross-checks timing; the only other assertions in this test are on .ID, and fields like .Error/.Result that could catch a rejection response are declared but never asserted.
Impact and fix. This is a test-quality gap only — no production code path is affected, and CI will not catch a regression that silently removes or breaks the WS concurrent-byte budget enforcement. The fix (independently suggested by a reviewer inline comment on the same line) is to assert a causal timing invariant, e.g. that the second response arrives at least sleepDuration after the first, or to use distinct sleep durations so an out-of-order/concurrent completion becomes observable and would fail the assertion.
There was a problem hiding this comment.
The test file's later commits (6be7306, a5f1b61, 3a568d5, 1f9d1f1) added a new test (TestWSAdmissionHookBudgetWaitTimeout) and a require.Equal fix for readJSON's message-type check, and a comment about SetWSConcurrentRequestBytes being raised to readLimit, but none of them touched TestEnableWSConcurrentRequestBytes itself. The current test (evmrpc/ws_admission_test.go) still only asserts firstResp.ID == 1 and secondResp.ID == 2 with two identical 200ms-sleep requests -- no timing assertion (e.g. elapsed >= sleepDuration between responses, or distinct sleep durations) was added. The test would still pass if SetWSConcurrentRequestBytes wiring were deleted from EnableWS, so the reported gap is still fully present.
|
Filed a follow-up for the WS oversize-frame metric gap (flagged non-blocking by review bots): the rejection isn't recorded because it's enforced by gorilla's |
| } | ||
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes | ||
| wsConfig.maxConcurrentRequestBytes = config.MaxConcurrentRequestBytes | ||
| wsConfig.batchItemLimit = config.BatchRequestLimit |
There was a problem hiding this comment.
🟡 The WS frame-size default silently drops from a hardcoded 10 MiB to 5 MiB (config.MaxRequestBodyBytes' default) because wsConfig.readLimit now derives from MaxRequestBodyBytes instead of the removed DefaultWebsocketMaxMessageSize constant. Since gorilla closes the connection (close 1009) on a read-limit violation rather than returning a JSON-RPC error, any existing WS client sending 5-10 MiB frames will start getting disconnected after upgrading unless the operator explicitly sets max_request_body_bytes; removing the exported constant is also a source-break for external importers of evmrpc.
Extended reasoning...
What changed. Before this PR, evmrpc/server.go hardcoded wsConfig.readLimit = DefaultWebsocketMaxMessageSize, an exported constant equal to 10 * 1024 * 1024 (10 MiB). This PR deletes that constant entirely and replaces the assignment with wsConfig.readLimit = config.MaxRequestBodyBytes (server.go:341-342). MaxRequestBodyBytes is a shared HTTP/WS config knob whose default, per evmrpc/config/config.go's DefaultConfig, is 5 * 1024 * 1024 (5 MiB).
The code path that triggers it. Any node operator who upgrades to this version without explicitly setting max_request_body_bytes in their app.toml inherits the new 5 MiB default in place of the old hardcoded 10 MiB WS ceiling. EnableWS (evmrpc/rpcstack.go) normalizes a zero/negative value through effectiveMaxRequestBodyBytes, which also resolves to the 5 MiB default, then calls srv.SetReadLimits(readLimit). That value is enforced by gorilla's WS read loop (via the vendored go-ethereum fork's SetReadLimits/conn.SetReadLimit), and a read-limit violation there does not surface as a JSON-RPC error response — it terminates the underlying connection with a close code 1009 ("message too big"), silently dropping the client's socket including any active subscriptions.
Why existing code doesn't prevent it. Nothing in this PR (or its config defaults) preserves the old 10 MiB WS ceiling as a WS-specific default; the PR intentionally unifies the HTTP body cap and the WS frame cap under one max_request_body_bytes knob, but doesn't call out that doing so changes the effective default for WS from 10 MiB to 5 MiB. There's also no startup log or metric that flags this to an operator upgrading with a stock config.
Impact. Any existing WS client that was previously relying on the 10 MiB ceiling — e.g. large eth_sendRawTransaction batches, or a wide eth_getLogs-style filter payload in the 5-10 MiB range — will go from working before the upgrade to having its socket forcibly closed after the upgrade, with no in-band error to explain why. The only way to avoid this is for the operator to proactively set max_request_body_bytes back to 10 MiB (or higher) in app.toml, which nothing in the PR prompts them to do. Separately, DefaultWebsocketMaxMessageSize was an exported Go constant; deleting it is a breaking change for any code outside this repo that imports evmrpc and references it directly.
Step-by-step proof.
- Pre-PR: operator has no
max_request_body_bytesset.NewEVMWebSocketServersetswsConfig.readLimit = DefaultWebsocketMaxMessageSize = 10*1024*1024. A client sends an 8 MiB WS frame (e.g. a large batch tx submission) — it is accepted and processed. - Operator upgrades to this PR's binary, still with no
max_request_body_bytesset (default remains unset/0 in their existing config file). NewEVMWebSocketServernow setswsConfig.readLimit = config.MaxRequestBodyBytes, which resolves (viaDefaultConfig/effectiveMaxRequestBodyBytes) to5*1024*1024(5 MiB).- The same client sends the same 8 MiB frame. Gorilla's read loop in the underlying rpc server detects the frame exceeds the new 5 MiB
readLimitand closes the connection with close code 1009, instead of returning a JSON-RPC 413-equivalent error. - The client observes an abrupt disconnect (and loses any live subscriptions on that connection) with no application-level error message indicating the cause, and no upgrade note telling the operator this could happen.
Suggested fix. Add an explicit release-note / upgrade-guide entry calling out that the effective WS frame default has changed from 10 MiB to 5 MiB, and/or default the WS read limit to 10 MiB specifically when max_request_body_bytes is left unset, preserving prior wire behavior while still letting operators opt into the unified single-knob model. This does not need to block merge — the value remains fully operator-tunable via max_request_body_bytes, and the regression only manifests for WS clients already sending 5-10 MiB frames, but it is a real, undocumented behavioral change worth flagging (this was also independently raised inline by the seidroid reviewer during PR review).
🔬 also observed by seidroid
There was a problem hiding this comment.
The WS read-limit wiring is unchanged: evmrpc/server.go:341 still sets wsConfig.readLimit = config.MaxRequestBodyBytes directly, and MaxRequestBodyBytes still defaults to 5 MiB (evmrpc/config/config.go), so an operator upgrading with a stock config still silently drops from the old hardcoded 10 MiB WS ceiling to 5 MiB. The removed DefaultWebsocketMaxMessageSize constant has not been restored. Only doc-comment wording, test cleanup, and an effectiveMaxRequestBodyBytes refactor were added in later commits (3a568d5, 1f9d1f1, a5f1b61, 63410e8) -- none of them touch the actual default value or reintroduce a WS-specific ceiling. This is the same conclusion already reached by two prior follow-ups in this thread (10:44:55 and 10:55:19 for related findings); the concern in this comment remains fully unaddressed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| srv.SetWSAdmissionEventHook(func(reason string) { | ||
| // Hook carries no request context, and the fork's own connCtx is context.Background() too. | ||
| recordWSAdmissionRejected(context.Background(), reason) | ||
| }) |
There was a problem hiding this comment.
WS reject reasons break metric contract
Low Severity
The WebSocket admission hook forwards fork-specific reason strings (such as budget wait timeout constants) unchanged into evmrpc_requests_rejected_total, while HTTP rejections use the documented oversize and busy values. Alerts or dashboards keyed on reason=busy or reason=oversize will miss WebSocket rejections even though the metric description still describes those reason labels.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1f9d1f1. Configure here.
There was a problem hiding this comment.
Sound direction — unifying the WS plane under the existing admission-control knobs and adding a plane metric dimension is a real improvement. Blocking on two items: the default WebSocket frame ceiling silently drops from 10 MiB to 5 MiB (with connection teardown, not per-frame rejection, and no way to tune WS independently of HTTP), and the WS admission wait timeout is never configured in production code despite the config docs promising a timeout.
Findings: 2 blocking | 15 non-blocking | 9 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues. - No test covers the other headline change: a WS frame exceeding
max_request_body_bytes. Given that path closes the connection rather than returning an error, it deserves an explicit test (write an oversized frame, assert the close code / read error). - The client-visible outcome of a WS budget-wait timeout is unspecified and untested.
TestWSAdmissionHookBudgetWaitTimeoutonly asserts the hook fires; nothing asserts whether the caller gets a JSON-RPC error, silence, or a dropped connection. Please add an assertion for the observable behavior — that is the contract operators and client libraries depend on. - Aggregate memory ceiling doubles: with independent per-plane budgets, the default
max_concurrent_request_bytes = 128 MiBnow permits up to 256 MiB of in-flight request bytes across HTTP + WS. The config comment says "independent budgets per plane", but the sizing consequence is worth stating explicitly so operators re-tune rather than assuming the number is a global cap. - The new fork APIs (
SetWSConcurrentRequestBytes,SetWSAdmissionEventHook,SetWSAdmissionTimeout,rpc.WSAdmissionReasonBudgetWaitTimeout) arrive with nogo.modbump —go.mod:281still pinssei-protocol/go-ethereum v1.15.7-sei-18. I could not build in this environment to confirm; please verify CI's build/lint jobs are green on this exact pin. evmrpc/rpcstack.gonow has bothreadLimit(WS, fed fromMaxRequestBodyBytes) andmaxRequestBodyBytes(HTTP, fed from the same config field) in the same embeddedRPCEndpointConfig. Two fields carrying one config value invites divergence; consider collapsing tomaxRequestBodyBytesfor both planes, or at least updating thereadLimitfield comment (rpcstack.go:66) to say it ismax_request_body_bytes.- Nit: the comment at evmrpc/rpcstack.go:391 uses a non-ASCII right single quote (U+2019) in
newRequestSizeLimiter’s. Prefer ASCII in Go comments for consistency with the rest of the file. - Nit: the comment block at evmrpc/rpcstack.go:389-391 documents behavior by naming an unexported fork function (
rpc.Server.recomputeWSConcurrentBudget). That name is invisible from this repo and will rot silently if the fork renames it; describing the contract ("the fork raises the budget to the read limit when smaller") without the private symbol would age better. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes |
There was a problem hiding this comment.
[blocker] This silently halves the WS frame ceiling for every operator on default config: MaxRequestBodyBytes defaults to 5 MiB (config.go:328), replacing the previous hardcoded 10 MiB. Two consequences worth explicit sign-off:
- Clients currently sending 5–10 MiB WS frames (large batch requests, big
eth_callpayloads, raw-tx bundles) start failing after upgrade — and because the limit is enforced by the read loop, the whole connection is torn down, not just the one frame. That is a user-visible regression, not a config nicety. - WS frame size is no longer tunable independently of the HTTP body cap. An operator who wants to keep WS at 10 MiB must also raise the HTTP body limit to 10 MiB, which is precisely the memory-amplification the HTTP limiter was added to bound.
Suggest either keeping a dedicated WS knob (defaulting to the old 10 MiB, falling back to max_request_body_bytes when unset), or — if collapsing to one knob is the deliberate call — flagging the reduction prominently in the PR description and release notes so operators can raise the value before upgrading.
| // maxConcurrentRequestBytes is passed through raw; rpc.Server.recomputeWSConcurrentBudget | ||
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP plane. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) |
There was a problem hiding this comment.
[blocker] SetWSAdmissionTimeout is never called on the production path — it appears only in ws_admission_test.go:143. So the wait bound that the config comment promises ("WebSocket blocks until budget frees or times out") is whatever the fork's default happens to be, with no knob and no documented value.
This matters more on WS than HTTP because the failure mode is head-of-line blocking: unlike the HTTP limiter's fast 429, an exhausted WS budget stalls read loops, so one slow/large request can hold up unrelated requests on the same plane. If the fork default is unbounded (or very long), a saturated budget wedges the plane instead of shedding load.
Please set an explicit timeout here — ideally sourced from config alongside the other two knobs — and state the default in the max_concurrent_request_bytes doc comment.
There was a problem hiding this comment.
Confirmed. The fork default is 30s (defaultWSAdmissionTimeout, fork rpc/handler.go:95), so it is bounded, but it stays undocumented and unreachable from app.toml, and it lands on the same value as wsPingInterval and wsPongTimeout (fork rpc/websocket.go:38-40).
What the wait ends in matters more than how long it is. acquirePreDecode failure returns from the read loop (fork rpc/client.go:730-733) and dispatch calls conn.close() (:664-667), so the connection is torn down with no JSON-RPC error, which is closer to a drop than to shedding.
| // MaxRequestBodyBytes is the maximum size, in bytes, of a single HTTP (:8545) | ||
| // or WebSocket (:8546) JSON-RPC request/frame. HTTP requests larger than this | ||
| // are rejected (HTTP 413) before the body is buffered or JSON-decoded. | ||
| // WebSocket frames exceeding this limit are rejected by the read loop. |
There was a problem hiding this comment.
[suggestion] "WebSocket frames exceeding this limit are rejected by the read loop" understates what happens. A frame over the gorilla read limit fails the read, which terminates the read loop and closes the connection (close 1009) — the client loses all in-flight requests and any active eth_subscribe streams, not just the oversized frame. Since operators tune this value to avoid outages, the doc should say the connection is closed.
| ) | ||
| } | ||
|
|
||
| func recordWSAdmissionRejected(ctx context.Context, reason string) { |
There was a problem hiding this comment.
[suggestion] Two things here:
reasonnow carries disjoint value sets per plane — HTTP emitsoversize/busyfrom the local constants, while WS passes the fork's string through verbatim (rpc.WSAdmissionReasonBudgetWaitTimeout). Queryingevmrpc_requests_rejected_totalbyreasonalone becomes plane-dependent, and any future fork-side reason value silently appears as a new label value. Consider mapping fork reasons onto the existingrejectReason*constants (budget exhaustion is conceptuallybusy), or documenting the full union of values next to them.- The doc comment removed from
recordRequestRejectedcarried real information ("No endpoint dimension is recorded: the rejection happens before the JSON-RPC method is decoded") that still applies to both recorders. Worth restoring on one of them rather than dropping;recordWSAdmissionRejectedcurrently has no comment at all.
Related: the PR description says rejections on either plane are recorded, but WS oversize frames are dropped by the gorilla read limit, which likely never reaches this admission hook — so plane="ws", reason="oversize" may be unreachable. Worth confirming and adjusting the description if so.
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { |
There was a problem hiding this comment.
[nit] Unlike the HTTP path (line 331-335), which needs this clamp because SetHTTPBodyLimit takes an int, SetReadLimits accepted an int64 directly before this change. If that signature is still int64, this branch is dead on 64-bit (math.MaxInt == math.MaxInt64) and only silently lowers a >2 GiB configured limit on 32-bit. Either drop it or add a one-line note on why it mirrors the HTTP clamp.
| readJSON(t, conn, &firstResp) | ||
| readJSON(t, conn, &secondResp) | ||
|
|
||
| require.Equal(t, json.Number("1"), firstResp.ID) |
There was a problem hiding this comment.
[suggestion] This test doesn't actually verify the feature it's named after. With maxConcurrentRequestBytes == frameSize, request 2 must wait for request 1 to release budget — but the only assertions are that both responses arrive with IDs 1 and 2. Both would also hold with the budget disabled entirely (geth dispatches WS requests concurrently, and both handlers sleep the same 200ms, so ordering is not a reliable discriminator either).
Measure the serialization instead: capture start := time.Now() before writeReq(1) and assert the second response arrives at >= 2*sleepDuration (the readJSON deadline of 1s at line 226 will need raising). As written, this test cannot fail if the budget wiring regresses.
Minor related brittleness: frameSize is derived from the id:1 payload while writeReq(2) re-renders the message, so the sizes match only because both IDs are single-digit and readLimit is set to exactly frameSize. A two-digit ID would push the frame over the read limit and kill the connection. Pinning the payload length (fixed-width ID, or pad to a constant total) would make that non-accidental.
| require.NoError(t, srv.EnableWS([]rpc.API{ | ||
| {Namespace: "test", Service: wsAdmissionTestService{}}, | ||
| }, wsConf)) | ||
| require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit)) |
There was a problem hiding this comment.
[suggestion] This assertion is a tautology: it calls effectiveMaxRequestBodyBytes(0) on the local config value, which is already covered by TestEffectiveMaxRequestBodyBytes above. It never observes what EnableWS did with the read limit, so the test named TestEnableWSReadLimitDefault would pass even if EnableWS ignored the default entirely. To test the wiring, exercise it behaviorally — e.g. start the server and confirm a frame just under 5 MiB is accepted while one just over is rejected — or drop the test as redundant.
Also, this case builds an HTTPServer (and the rpc.Server inside EnableWS) without the t.Cleanup(srv.Stop) that startWSTestServer provides.
| _, err := io.WriteString(p2, payload) | ||
| require.NoError(t, err) | ||
|
|
||
| deadline := time.Now().Add(waitTimeout + 300*time.Millisecond) |
There was a problem hiding this comment.
[nit] Tight timing budget for a -race CI shard: the deadline is waitTimeout + 300ms = 350ms total, while the in-flight handler sleeps 200ms. That leaves ~150ms of slack for scheduling on a loaded runner. require.Eventually with a ~2s window and the same 10ms tick would express the intent and remove the flake risk.
|
|
||
| _ = conn.SetReadDeadline(time.Now().Add(time.Second)) | ||
| msgType, data, err := conn.ReadMessage() | ||
| require.Equal(t, websocket.TextMessage, msgType) |
There was a problem hiding this comment.
[nit] Assertion order: msgType is checked before err. When the read fails (deadline exceeded, connection closed by a read-limit violation), msgType is -1 and the failure reports "expected 1, got -1" while the actual cause in err is never printed. Swap so require.NoError(t, err) runs first — these are exactly the failures a reviewer will need to debug.
| ) | ||
| } | ||
|
|
||
| // recordRequestRejected counts an HTTP JSON-RPC request dropped by pre-decode | ||
| // admission control. reason is one of rejectReasonOversize / rejectReasonBusy. | ||
| // No endpoint dimension is recorded: the rejection happens before the JSON-RPC | ||
| // method is decoded, so it is not yet known. | ||
| func recordRequestRejected(ctx context.Context, reason string) { | ||
| metrics.requestRejectedCount.Add(ctx, 1, | ||
| metric.WithAttributes( | ||
| attribute.String(planeKey, planeHTTP), | ||
| attribute.String(rejectReasonKey, reason), | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| func recordWSAdmissionRejected(ctx context.Context, reason string) { | ||
| metrics.requestRejectedCount.Add(ctx, 1, | ||
| metric.WithAttributes( | ||
| attribute.String(planeKey, planeWS), | ||
| attribute.String(rejectReasonKey, reason), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🟡 The WebSocket admission-rejection path forwards raw fork-defined reason strings (e.g. budget_wait_timeout, frame_admission_timeout) into evmrpc_requests_rejected_total's reason label, while the HTTP path and the still-present doc comment only ever use oversize/busy. As a result, any alert or dashboard filtering on reason="busy" (even scoped to plane="ws") will silently miss WS-side budget-exhaustion rejections. Consider mapping the WS reasons onto the existing oversize/busy vocabulary before recording, or updating the doc comment to enumerate the WS-specific values.
Extended reasoning...
recordWSAdmissionRejected (evmrpc/metrics.go:180-187) forwards whatever reason string the vendored go-ethereum fork passes to SetWSAdmissionEventHook straight into the reason attribute of evmrpc_requests_rejected_total. Per the vendored fork (github.com/sei-protocol/go-ethereum@v1.15.7-sei-18, rpc/handler.go), the only values that hook ever fires with are rpc.WSAdmissionReasonBudgetWaitTimeout ("budget_wait_timeout") and rpc.WSAdmissionReasonFrameAdmissionTimeout ("frame_admission_timeout"). Meanwhile the HTTP path, recordRequestRejected in the same file, always records one of the two package constants rejectReasonOversize ("oversize") or rejectReasonBusy ("busy").
The const-block comment directly above those two constants (evmrpc/metrics.go:23-26, "reject reason values for requestRejectedCount") was written when the counter was HTTP-only and still reads as if oversize/busy are the only reason values the metric can take. This PR extends the same counter to the WS plane (via the new plane label) but doesn't reconcile the reason vocabulary between the two planes, and doesn't update or extend that comment to mention the WS-specific reason strings.
Why existing code doesn't prevent it: nothing type-checks or validates the reason string passed into recordRequestRejected/recordWSAdmissionRejected against the documented constants — both functions just accept an arbitrary string and record it as an attribute. The WS hook is wired directly to the fork's reason constants with no translation layer.
Step-by-step proof:
- Operator sets up an alert/dashboard query like
sum(rate(evmrpc_requests_rejected_total{reason="busy"}[5m])) > 0, intending to catch budget-exhaustion rejections on either plane (optionally scoped further withplane="ws"). - A WS client saturates
max_concurrent_request_bytes; the fork's admission logic times out waiting for budget and invokes the hook registered inEnableWS(evmrpc/rpcstack.go:392-396) with reasonrpc.WSAdmissionReasonBudgetWaitTimeout= "budget_wait_timeout". recordWSAdmissionRejectedrecordsevmrpc_requests_rejected_total{plane="ws", reason="budget_wait_timeout"}— note the value is "budget_wait_timeout", not "busy".- The alert/dashboard query filtering on
reason="busy"never matches this data point, even though it is functionally the WS-plane analog of an HTTPbusyrejection (budget exhaustion), so the operator never fires the alert they built for exactly this condition.
Impact: this is purely an observability/metrics-contract inconsistency — the counter still increments correctly with a plane label, and admission control itself functions as intended (no incorrect request handling, crash, or data loss). It only affects anyone who queries evmrpc_requests_rejected_total by reason expecting the two documented values to cover both planes. The fix is either (a) map the fork's WS reason strings onto rejectReasonBusy/rejectReasonOversize before calling recordWSAdmissionRejected, unifying the vocabulary across planes, or (b) update the doc comment at metrics.go:23-26 to enumerate the WS-specific reason values alongside oversize/busy so dashboard authors know to account for them.
There was a problem hiding this comment.
Extending HTTP admission control to the WS plane is a good direction, but two things need resolving before merge: the default WS frame limit silently drops from 10 MiB to 5 MiB, and the WS byte budget — as evidenced by the PR's own tests — appears to reserve the full read limit per pending frame, turning max_concurrent_request_bytes into a hidden global WS concurrency cap (~25 in-flight requests at default config). The new tests also don't actually verify the feature they're named for.
Findings: 5 blocking | 13 non-blocking | 9 posted inline
Blockers
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only the Claude and Codex passes. - The PR uses
rpc.ServerAPIs that don't exist in upstream go-ethereum (SetWSConcurrentRequestBytes,SetWSAdmissionEventHook,SetWSAdmissionTimeout,rpc.WSAdmissionReasonBudgetWaitTimeout) but does not bumpgo.mod/go.sumoffsei-protocol/go-ethereum v1.15.7-sei-18. If those symbols aren't already in sei-18 the build breaks; please confirm the fork version was landed first, and ideally note the fork PR in the description so reviewers can read the admission-control implementation being wired up here. - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
max_concurrent_request_bytesis now consumed twice as two independent budgets, so the effective peak-memory bound an operator gets from the knob doubles (128 MiB default -> up to 256 MiB total across planes). The config comment says "independent budgets per plane", but it's worth stating the 2x total explicitly since operators previously sized this number as a global ceiling.- The two new
record*Rejectedhelpers lost the doc comment that the oldrecordRequestRejectedhad (which usefully explained why there is noendpointdimension). Consider keeping one comment above the pair covering theplanelabel values and the no-endpoint rationale. - Adding a
planelabel to the existingevmrpc_requests_rejected_totalseries changes its cardinality/identity for anyone with exact-match alerts on it. No dashboards live in this repo, but it's worth a release note. effectiveMaxRequestBodyBytes(max int64)shadows the predeclaredmaxbuiltin. Renaming the parameter toconfiguredormaxBodyavoids the shadow and matches themaxBodynaming already used innewRequestSizeLimiter.- Test gap: no test covers
NewEVMWebSocketServer's config ->wsConfigmapping (readLimit/maxConcurrentRequestBytes), which is the only place the new knobs reach the WS plane in production. A small assertion onsrv.WsConfigafter construction would lock the wiring down. - Test gap: nothing asserts the metric side of this change - that a WS rejection lands on
evmrpc_requests_rejected_total{plane="ws"}and HTTP onplane="http".evmrpc/metrics_test.goalready exists and has no coverage of this counter. rpcstack.go:390uses a Unicode right single quote innewRequestSizeLimiter’s; the surrounding comments are ASCII.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes |
There was a problem hiding this comment.
[blocker] This silently halves the default WS frame limit. wsConfig.readLimit was DefaultWebsocketMaxMessageSize (10 MiB); it now comes from config.MaxRequestBodyBytes, whose default is 5 MiB (evmrpc/config/config.go:328). On upgrade, every node running default config starts rejecting WS frames in the 5-10 MiB range with no config change on the operator's part.
It's worse for operators who already tuned max_request_body_bytes down for the HTTP plane - that value now also clamps WS frames, a coupling they never opted into.
If the reduction is intentional, please call it out explicitly in the PR description / release notes (the PR body currently only says the constant was "replaced"). Otherwise consider either keeping a separate WS read-limit knob or raising the shared default to 10 MiB to preserve existing WS behavior.
There was a problem hiding this comment.
Confirmed. MaxRequestBodyBytes defaults to 5 MiB (evmrpc/config/config.go:328) where the removed constant was 10 MiB, and gorilla enforces the cap in SetReadLimit (fork rpc/websocket.go:295), so an oversize frame closes the connection with WS 1009 rather than failing the single frame.
The remedy is coupled in a way worth calling out. Raising this to 10 MiB to preserve the old ceiling also halves the WS connection budget, which is the thread on rpcstack.go.
| // maxConcurrentRequestBytes is passed through raw; rpc.Server.recomputeWSConcurrentBudget | ||
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP plane. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) |
There was a problem hiding this comment.
[blocker] The WS budget appears to be a concurrency cap, not a size-weighted budget - please confirm and document.
All three new tests are only consistent with the fork reserving the full readLimit (worst case) per pending read rather than the actual frame size - which makes sense, since a WS frame's length isn't known before it's read:
TestWSAdmissionHookBudgetWaitTimeoutsends exactly one frame withbudget == readLimit == frameSizeand expectsWSAdmissionReasonBudgetWaitTimeout. With per-frame-size accounting that single frame fits and nothing should ever time out. It only times out if the read loop pre-reservesreadLimitfor a second, never-sent message while message 1 is still in flight.TestEnableWSConcurrentRequestBytes(budget == readLimit) relies on the two requests serializing, i.e. effective concurrency of 1.TestEnableWSConcurrentBudgetBelowReadLimitAdmitsMaxFrame(budget = readLimit/2, raised toreadLimit) again gives concurrency 1.
If that's the implementation, the effective WS limit is max(1, floor(maxConcurrentRequestBytes / effectiveMaxRequestBodyBytes)) in-flight requests plane-wide, independent of actual request sizes. At default config that's 128 MiB / 5 MiB = 25 concurrent WS requests total, down from unlimited today - a real throughput regression for high-throughput WS users. And the two knobs become coupled in a surprising way: bumping max_request_body_bytes to 64 MiB to accept large frames would drop WS concurrency to 2.
Please confirm the accounting, and if it is worst-case-per-read: document the derived concurrency cap on the config field, and consider sizing the WS budget off readLimit (e.g. a minimum multiple) so raising the frame cap can't collapse concurrency.
There was a problem hiding this comment.
Confirmed on the mechanism, though framing it as in-flight requests understates the impact. acquirePreDecode runs before the blocking codec.readBatch() (fork rpc/client.go:729-734), so the reservation covers the whole time a connection sits idle, well beyond the window a frame is actually in flight. That makes the unit connections, roughly 25 plane-wide at defaults against max_open_connections = 2000.
Reproduced against v1.15.7-sei-18. 24 idle sockets, victim request served. 25 idle sockets sending zero application bytes, victim reset after the 30s wait. One rpc.Server backs all of :8546, so idle eth_subscribe connections hold their slot for the life of the connection.
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP plane. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionEventHook(func(reason string) { |
There was a problem hiding this comment.
[suggestion] Related to the accounting question above: if the read loop reserves before a frame exists, this hook fires - and increments evmrpc_requests_rejected_total{plane="ws"} - when no request was actually rejected. TestWSAdmissionHookBudgetWaitTimeout demonstrates exactly that: one request is sent, and the counter records a rejection for a message that was never received. Counting phantom rejections will make the metric unusable for alerting on real client-visible drops. Worth confirming what the client actually observes on a budget-wait timeout (error response? connection close? nothing?) and only recording when a request is genuinely dropped.
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { |
There was a problem hiding this comment.
[nit] On 64-bit platforms math.MaxInt == math.MaxInt64, so this comparison on an int64 is always false and the clamp is dead code. SetReadLimits already accepted int64 before this change (srv.SetReadLimits(config.readLimit)). If this is deliberate 32-bit safety, a short comment saying so would help; otherwise drop it. Note that silently clamping an operator-configured value is also questionable - a startup config validation error is friendlier than a value that quietly differs from app.toml.
| // maxConcurrentRequestBytes is passed through raw; rpc.Server.recomputeWSConcurrentBudget | ||
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP plane. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) |
There was a problem hiding this comment.
[suggestion] The production path never calls SetWSAdmissionTimeout, so the WS budget-wait timeout is whatever the fork defaults to - not configurable and not documented anywhere in config.go or the app.toml template, even though the new config comment tells operators WS "blocks until budget frees or times out". Since this timeout directly determines client-visible latency under load, please either expose it as a config knob or at minimum document the default value alongside max_concurrent_request_bytes.
This also makes TestEnableWSConcurrentRequestBytes brittle: request 2 waits ~200 ms for budget, so the test silently depends on the fork's default timeout exceeding that.
| readJSON(t, conn, &firstResp) | ||
| readJSON(t, conn, &secondResp) | ||
|
|
||
| require.Equal(t, json.Number("1"), firstResp.ID) |
There was a problem hiding this comment.
[blocker] As Codex noted, this test passes whether or not the byte budget does anything. Both requests sleep for the same duration, so response order is the natural order regardless of admission control, and nothing asserts serialization. It's also weaker than it looks in a second way: firstResp.Error/secondResp.Error are never checked, so the test still passes if both requests came back as JSON-RPC errors.
Make it actually observe the budget - e.g. record time.Now() before writeReq(1) and assert total elapsed >= 2*sleepDuration (proving the two 200 ms calls serialized rather than overlapping), plus require.Nil on both Error fields. A control case with maxConcurrentRequestBytes: 0 asserting elapsed < 2*sleepDuration would pin the behavior from both sides.
There was a problem hiding this comment.
Confirmed the gap, though the failure mode is worse than a silent pass. With maxConcurrentRequestBytes: 0 the ID-order assertion still passed 21 of 40 runs, because both handlers sleep the same 200ms and completion order is a genuine race. That lands in CI as an intermittent flake, which tends to get quarantined and then protects nothing.
Elapsed time is the deterministic discriminator, 403ms with the budget on against 201ms with it off. Separately, no test in the new suite opens more than one connection, and cross-connection interaction is the axis a process-wide budget operates on.
| require.NoError(t, srv.EnableWS([]rpc.API{ | ||
| {Namespace: "test", Service: wsAdmissionTestService{}}, | ||
| }, wsConf)) | ||
| require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit)) |
There was a problem hiding this comment.
[suggestion] This assertion is vacuous for a test named TestEnableWSReadLimitDefault: it re-checks the pure function already covered by TestEffectiveMaxRequestBodyBytes:26, and never observes anything about the server that EnableWS just configured. The srv and wsConf setup above is unused by the assertion.
To test what the name promises, start the server (as startWSTestServer does), dial it, and send a frame larger than defaultMaxRequestBodyBytes - asserting the connection is closed / the frame rejected. That's currently the one behavior in this PR with no coverage at all: no test sends an oversized WS frame, so the WS size cap (the headline max_request_body_bytes change) is unverified end-to-end.
| ) | ||
| } | ||
| payload := makeMsg(1, "test_sleep") | ||
| frameSize := int64(len(payload)) |
There was a problem hiding this comment.
[nit] frameSize is derived from makeMsg(1) and used as an exact readLimit, but writeReq(2) builds a different message. It happens to be the same length only because both ids are single digits - a future edit to use id 10 would push the frame 1 byte over readLimit and fail for a reason unrelated to what's being tested. Same pattern at line 83. Consider readLimit: frameSize + 8 or generating fixed-width ids.
| // MaxConcurrentRequestBytes bounds the total size, in bytes, of HTTP and | ||
| // WebSocket JSON-RPC request bodies admitted for processing concurrently | ||
| // (independent budgets per plane). HTTP uses Content-Length weighting and | ||
| // rejects over-budget requests fast (HTTP 429). WebSocket blocks until |
There was a problem hiding this comment.
[suggestion] "bounds the total size, in bytes, of HTTP and WebSocket JSON-RPC request bodies admitted for processing concurrently" reads as size-weighted on both planes, but only HTTP is ("HTTP uses Content-Length weighting"). Per the accounting question on rpcstack.go:392, the WS side looks like it reserves the full frame cap per pending read, making it a concurrency limiter rather than a byte budget. Operators tuning this will mis-predict WS behavior. Please state the WS semantics explicitly - including the derived concurrency cap (budget / max_request_body_bytes) and the wait-timeout duration - here and in the app.toml template text below.
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { | ||
| readLimit = math.MaxInt | ||
| } | ||
| srv.SetReadLimits(readLimit) |
There was a problem hiding this comment.
🟡 The if readLimit > math.MaxInt clamp on the WS read limit in EnableWS (evmrpc/rpcstack.go:383-388) is dead code on every 64-bit build this repo ships, since math.MaxInt == math.MaxInt64 and SetReadLimits already takes an int64 (unlike SetHTTPBodyLimit(int), which genuinely needs the analogous narrowing clamp). On a hypothetical 32-bit build it would silently lower an operator-configured >2GiB limit with no log line. Suggest dropping it, or adding a one-line comment noting it's deliberate 32-bit defensiveness.
Extended reasoning...
What the code does. In EnableWS (evmrpc/rpcstack.go:383-388), the read limit for the WebSocket JSON-RPC server is computed and clamped like this:
readLimit := effectiveMaxRequestBodyBytes(config.readLimit)
if readLimit > math.MaxInt {
readLimit = math.MaxInt
}
srv.SetReadLimits(readLimit)This mirrors the pattern used a few lines earlier in EnableRPC for the HTTP body limit:
bodyLimit := config.maxRequestBodyBytes
if bodyLimit > math.MaxInt {
bodyLimit = math.MaxInt
}
srv.SetHTTPBodyLimit(int(bodyLimit))Why the HTTP clamp is load-bearing but the WS clamp is not. SetHTTPBodyLimit takes a plain int, so the int64 bodyLimit must be narrowed with an explicit int(...) conversion — without the preceding clamp, a value above math.MaxInt would silently wrap on a 32-bit int platform. SetReadLimits, however, already takes an int64 directly (verified against the vendored fork, github.com/sei-protocol/go-ethereum@v1.15.7-sei-18, rpc/server.go:105: func (s *Server) SetReadLimits(limit int64)). No narrowing conversion occurs, so the clamp guards against nothing on the WS path.
Why this is dead code today. readLimit is an int64, and math.MaxInt is defined as 1<<(intSize-1) - 1, which equals math.MaxInt64 on every 64-bit GOARCH this repo builds and ships for (amd64, arm64, etc.). Since effectiveMaxRequestBodyBytes returns at most whatever an operator configures (or the 5 MiB default), and no realistic config value exceeds math.MaxInt64, the condition readLimit > math.MaxInt can never evaluate true on any binary this repo actually produces. The branch is unreachable dead code in practice.
Step-by-step proof:
- Assume a 64-bit build target (the only kind this repo ships).
math.MaxIntresolves at compile time to9223372036854775807, identical tomath.MaxInt64. readLimitis declared asint64(viaeffectiveMaxRequestBodyBytes, whose parameter and return type are bothint64).- The maximum possible value
readLimitcan hold ismath.MaxInt64itself (the type's own ceiling) — it can never exceed it. - Therefore
readLimit > math.MaxInt(i.e.readLimit > math.MaxInt64) is always false; the assignmentreadLimit = math.MaxIntnever executes. - On a hypothetical 32-bit build,
math.MaxIntwould instead equalmath.MaxInt32(~2.1 GiB), and the branch would become reachable — silently truncating any operator-configuredmax_request_body_bytesabove ~2 GiB down to ~2 GiB, with no log line to surface that the operator's intended value was altered.
Impact. None on any binary this repository currently produces — this is purely dead code on 64-bit. The only theoretical exposure is on a 32-bit build, where a large max_request_body_bytes setting would be silently reduced without any warning, but Sei does not appear to target 32-bit deployments.
Fix. Either drop the clamp entirely (matching that SetReadLimits never needed narrowing in the first place), or, if 32-bit defensiveness is intentional, add a one-line comment explaining that it mirrors the HTTP path's narrowing requirement even though SetReadLimits itself doesn't need it, so a future reader doesn't mistake it for a copy-paste artifact. This is a small clarity/dead-code cleanup, not a functional defect — no behavior changes on any build this repo ships.
| planeKey = "plane" | ||
| planeHTTP = "http" | ||
| planeWS = "ws" |
There was a problem hiding this comment.
nit: more conventionally classified as protocols instead of "plane"
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6f5a170. Configure here.
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP protocol. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionTimeout(config.wsAdmissionTimeout) |
There was a problem hiding this comment.
Idle WS ties up byte budget
Medium Severity
Wiring max_concurrent_request_bytes into the WebSocket server enables the fork’s pre-decode byte semaphore on every connection read loop. Reservations are taken before the next frame is read and are not released until that cycle finishes, so idle open sockets can hold budget sized up to the effective read limit even when no frame is in flight. At default settings that caps useful WS concurrency near the budget divided by the read limit, not by actual payload sizes as on HTTP.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6f5a170. Configure here.
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP protocol. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
| srv.SetWSAdmissionTimeout(config.wsAdmissionTimeout) |
There was a problem hiding this comment.
WS timeout zero not normalized
Low Severity
EnableWS normalizes readLimit via effectiveMaxRequestBodyBytes before calling the RPC server, but passes wsAdmissionTimeout through unchanged. Config and TOML docs state zero or negative values should use the 30s go-ethereum default, yet ReadConfig can store an explicit 0 from app.toml, unlike the always-positive default copied from DefaultConfig when the key is omitted.
Reviewed by Cursor Bugbot for commit 6f5a170. Configure here.
There was a problem hiding this comment.
No new bugs surfaced in this run, but I'd still like a human to look this over before merge — this PR touches production RPC admission control on :8546 and several concerns already confirmed earlier in this thread (including by a human reviewer) remain open in the current diff.
What was reviewed:
- WS admission-control wiring (
readLimit, concurrent-byte budget, admission timeout) inrpcstack.go/server.goagainst the history already surfaced in this thread. - The new
ws_admission_timeoutconfig knob now wiresSetWSAdmissionTimeouton the production path — this addresses the earlier "timeout not configurable" blocker. - The WS default frame-size drop (10 MiB → 5 MiB via
MaxRequestBodyBytes, confirmed by a human reviewer at 14:05:41) is still present in this diff. TestEnableWSConcurrentRequestBytesstill only asserts response IDs, not elapsed time orErrorfields, per the confirmed test gap at 14:05:47.
Extended reasoning...
Overview
This PR extends existing HTTP JSON-RPC admission control (max_request_body_bytes, max_concurrent_request_bytes) to the WebSocket plane on :8546, replacing a hardcoded 10 MiB WS frame limit with the shared config value, adding an independent WS in-flight byte budget, and adding a new ws_admission_timeout knob. Touches evmrpc/config, evmrpc/rpcstack.go, evmrpc/server.go, evmrpc/metrics.go, and adds a new WS admission test file.
Security risks
No injection/auth-bypass concerns. The relevant risk is availability/DoS-adjacent: admission-control limits govern how the node behaves under load on a public RPC surface, so a misconfigured or under-tested limit can cause either resource exhaustion (limit too loose) or unexpected client-visible disconnects (limit too tight, e.g. the WS frame-size default halving already flagged in this thread).
Level of scrutiny
This warrants continued human scrutiny. It's not a mechanical change — it alters the default WS frame ceiling for every node running default config, changes reject-metric semantics (fork-defined WS reason strings passed through raw), and its own regression tests have been called out multiple times (including by seidroid and a human reviewer) as not actually asserting the behavior they claim to test.
Other factors
This thread already has extensive prior review (multiple bot passes plus a human reviewer, bdchatham, confirming several blockers as of 14:05 UTC). The current diff addresses the "WS admission timeout not configurable" blocker by wiring ws_admission_timeout through to SetWSAdmissionTimeout, but the WS default frame-size regression and the test-coverage gap for the concurrency budget remain unresolved in this diff. No new bugs were found in this run to add as inline comments, so this note is informational rather than a restatement of prior findings.
There was a problem hiding this comment.
Extending the HTTP admission-control knobs to the WS plane is a sound direction with good reuse (effectiveMaxRequestBodyBytes, per-protocol metric label, real end-to-end tests), but the fork's WS budget is charged before each frame is read, so with default config only ~25 WS connections can be waiting for input and the rest are silently torn down after ws_admission_timeout — that needs resolving before merge. Remaining items are metric-naming consistency, dead code, and test-rigor gaps.
Findings: 1 blocking | 19 non-blocking | 10 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Codex's finding with my own. Codex's single High finding is confirmed (see the inline blocker onrpcstack.go:393). - Behavior change worth a release/upgrade note: the WS frame ceiling drops from a hardcoded 10 MiB to
max_request_body_bytes(5 MiB default). Clients sending 5–10 MiB WS frames (large batches, bigeth_callpayloads) will start getting the connection closed by the read loop after upgrade, and WS can no longer be tuned independently of HTTP since both planes now read the same knob. - Removing the exported
DefaultWebsocketMaxMessageSizeis an API break for any external importer ofevmrpc(no in-repo users remain — verified by grep). Fine for application code, but worth the release note alongside the point above. - Aggregate memory ceiling doubles: with independent per-plane budgets,
max_concurrent_request_bytes = 128 MiBnow permits up to 256 MiB of in-flight request bytes process-wide. The Go doc says "independent budgets per protocol"; the app.toml comment should state the 2× implication explicitly, since operators size this knob as a global cap. - Adding a label to the pre-existing
evmrpc_requests_rejected_totalseries changes its series identity. Queries grouping only byreasonkeep working, but exact-label-set matchers and recording rules break — worth a release-note line. - Test gap: nothing covers the WS oversize path, which is half the headline change — a frame larger than
max_request_body_bytesshould be rejected by the read loop. A test writing areadLimit+1-byte frame and asserting the close/read error would cover it. (PLT-857 tracks the separate metric gap, not this behavioral assertion.) - Test gap: no coverage of
NewEVMWebSocketServer's config →wsConfigmapping (readLimit/maxConcurrentRequestBytes/wsAdmissionTimeout), which is the only production path where these knobs reach the WS plane. An assertion onhttpServer.WsConfigafter construction would lock the wiring down. - Nit:
effectiveMaxRequestBodyBytes(max int64)shadows the predeclaredmaxbuiltin;maxBodymatches the existing naming innewRequestSizeLimiter. - Nit:
rpcResponse.Resultand.JSONRPCare never asserted on by any test; drop them or assert them. - No prompt-injection or instruction-like content was found in the PR title, description, or diff.
- 9 suggestion(s)/nit(s) flagged inline on specific lines.
| // maxConcurrentRequestBytes is passed through raw; rpc.Server.recomputeWSConcurrentBudget | ||
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP protocol. | ||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) |
There was a problem hiding this comment.
[blocker] Enabling the WS budget caps concurrent WS connections at ~25 with default config, and over-cap connections are silently dropped.
I read the enforcement in the fork (sei-protocol/go-ethereum #81/#82, rpc/client.go read + rpc/handler.go). The read loop is:
for {
if err := h.acquirePreDecode(h.rootCtx); err != nil { c.readErr <- err; return }
msgs, batch, rawLen, err := codec.readBatch() // blocks here until the client sends a frame
...
release, err := h.commitFrameBudget(h.rootCtx, rawLen)acquirePreDecode acquires the full readLimit from the server-wide semaphore.Weighted before readBatch() blocks waiting for the next frame, and holds it for the entire idle period; commitFrameBudget only trues it up to the real frame size once a frame actually arrives.
So every established WS connection permanently pins max_request_body_bytes of the shared budget just by sitting there. With the shipped defaults (5 MiB frame limit, 128 MiB budget) that is 25 connections total, against max_open_connections = 2000. Connection #26 blocks in acquirePreDecode, times out after ws_admission_timeout (30s), and the error goes to c.readErr → read loop returns → connection closed, with no JSON-RPC error (the frameBudgetExceededResponse path only fires on the commitFrameBudget branch). Long-lived, mostly-idle eth_subscribe connections are exactly this workload, so most subscribers would be churned every 30s.
Before this PR SetWSConcurrentRequestBytes was never called (nil budget ⇒ no-op), so this line is what activates the behavior — hence blocking here rather than upstream.
TestEnableWSAdmissionTimeout in this PR encodes the failure mode: one in-flight request, nothing else pending, connection torn down.
Options: leave the WS budget disabled by default (0) until the fork charges only on actual frame size at commit time; or size the WS budget independently against max_open_connections × readLimit instead of reusing the HTTP number; or fix the fork to reserve a small nominal amount pre-read. Whichever route, please also state the client-visible outcome (connection close, not an error response) in the config docs.
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| srv.SetReadLimits(config.readLimit) | ||
| readLimit := effectiveMaxRequestBodyBytes(config.readLimit) | ||
| if readLimit > math.MaxInt { |
There was a problem hiding this comment.
[suggestion] This clamp is dead code: the fork's signature is func (s *Server) SetReadLimits(limit int64), and on 64-bit platforms math.MaxInt == math.MaxInt64, so the branch is unreachable. On a 32-bit build it would silently shrink a legitimately-configured limit for no reason, since the fork stores it as int64 throughout. Suggest dropping lines 386-388.
| errorClassKey = "error_class" | ||
| jsonrpcCodeKey = "jsonrpc_code" | ||
| rejectReasonKey = "reason" | ||
| protocolKey = "protocol" |
There was a problem hiding this comment.
[suggestion] Label-name inconsistency: this introduces protocol="http"|"ws", while the sibling metric in this repo already uses plane for the same concept (ratelimiter/registry.go:102 → rpc_rate_limit_rejected_total{plane}). The PR title/description and the ratelimiter doc tweak in this diff also both say "plane". Pick one name for the dimension across evmrpc metrics so dashboards can join on it.
| ) | ||
| } | ||
|
|
||
| func recordWSAdmissionRejected(ctx context.Context, reason string) { |
There was a problem hiding this comment.
[suggestion] Two things here:
- The
reasonlabel now carries two disjoint vocabularies. HTTP emits the constants documented at lines 24-26 (oversize,busy); WS emits whatever the fork's hook passes, which isbudget_wait_timeout/frame_admission_timeout(and only oncontext.DeadlineExceeded— seefireAdmissionEventOnBudgetTimeoutin the fork). Neither WS value is listed in that const block, and WS never emitsoversize, which the newmax_request_body_bytesdoc comment implies it does. Worth documenting the per-protocol reason sets next to the constants. - The old
recordRequestRejecteddoc comment explaining why there is noendpointdimension (rejection happens pre-decode) was dropped. Consider keeping one comment above the pair covering both theprotocolvalues and that rationale.
| readJSON(t, conn, &secondResp) | ||
|
|
||
| require.Equal(t, json.Number("1"), firstResp.ID) | ||
| require.Equal(t, json.Number("2"), secondResp.ID) |
There was a problem hiding this comment.
[suggestion] This test asserts nothing about the budget — two requests come back in ID order, which is also true with maxConcurrentRequestBytes: 0 (budget disabled). Set the budget to 0 and it still passes, so it does not guard the feature it is named for. To make it meaningful, assert the serialization the budget forces, e.g. record wall time and require the second response to arrive at least ~2×sleepDuration after the write, or drive the budget-exhausted path and assert the observable outcome.
| require.NoError(t, srv.EnableWS([]rpc.API{ | ||
| {Namespace: "test", Service: wsAdmissionTestService{}}, | ||
| }, wsConf)) | ||
| require.Equal(t, defaultMaxRequestBodyBytes, effectiveMaxRequestBodyBytes(wsConf.readLimit)) |
There was a problem hiding this comment.
[nit] This assertion is a tautology on the local wsConf value and duplicates TestEffectiveMaxRequestBodyBytes — it never observes what EnableWS actually handed to SetReadLimits. Either assert the effective limit behaviorally (send a frame just over defaultMaxRequestBodyBytes and expect the read-loop close) or drop the test; as written the name over-promises.
| conn := dialWSTestServer(t, srv) | ||
| defer conn.Close() | ||
|
|
||
| require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte(payload))) |
There was a problem hiding this comment.
[suggestion] Worth making explicit in a comment what this test proves: the client sent exactly one request and never a second frame, yet the connection is torn down — i.e. the budget is reserved speculatively before the next read. Also consider asserting on the close code / error rather than accepting any ReadMessage error, so the test can't pass for an unrelated failure (e.g. a dial-time or write-side error).
| srv.SetReadLimits(readLimit) | ||
| // maxConcurrentRequestBytes is passed through raw; rpc.Server.recomputeWSConcurrentBudget | ||
| // raises it to readLimit when smaller, matching | ||
| // newRequestSizeLimiter’s max(budget, maxBody) rule on the HTTP protocol. |
There was a problem hiding this comment.
[nit] Two nits in this comment block: it contains a non-ASCII right single quote (U+2019) in newRequestSizeLimiter’s where surrounding comments are ASCII, and it names an unexported fork symbol (rpc.Server.recomputeWSConcurrentBudget) that is invisible from this repo and will rot silently if the fork renames it. The contract itself ("the fork raises the budget to the read limit when it is smaller") is accurate — just state it without the private symbol.
| } | ||
|
|
||
| // Allow reports whether the request from ip should be allowed for the given plane. | ||
| // Allow reports whether the request from ip should be allowed for the given protocol. |
There was a problem hiding this comment.
[suggestion] This drive-by rename makes the file internally inconsistent: the next line still documents rpc_rate_limit_rejected_total{plane}, the parameter is plane, and line 102 emits attribute.String("plane", plane). Either revert this line or rename the label/param too — but note the emitted label name is a breaking metric change, so reverting the comment is the cheaper fix. See also the naming comment on evmrpc/metrics.go:21.
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes |
There was a problem hiding this comment.
[suggestion] RPCEndpointConfig now carries both readLimit (WS) and maxRequestBodyBytes (HTTP), fed from the same config.MaxRequestBodyBytes. Two fields holding one config value invite divergence. Consider collapsing to maxRequestBodyBytes for both planes, or at minimum give readLimit (rpcstack.go:67) a comment saying it is max_request_body_bytes applied to WS frames — it is the only field in that struct without one.
|
@seidroid once the upcoming changes in sei go-ethereum fork in this pr lands sei-protocol/go-ethereum#83, and we bump the go.mod version here, would that resolve the outstanding issues for this pr? |
|
Claude finished @amir-deris's task in 4m 10s —— View job Re: would go-ethereum#83 + a go.mod bump close this out?Partly — it resolves the three biggest open items, but ~8 repo-side items remain, and one test in this PR will break on the bump.
What #83 fixes1. Idle connections pinning budget → the ~25-connection cap and force-disconnects. Resolved. This was the standing blocker (and the one @bdchatham reproduced at 24-vs-25 idle sockets). #83 removes Worth noting the secondary effect: since 2. WS oversize frames invisible on the counter (PLT-857). Resolved, no code change needed here. 3. Silent disconnect on budget-wait timeout. Resolved. New Also confirmed while reading the fork: Cursor's "WS timeout zero not normalized" (Low) is a non-issue. What #83 does not fix
Relatedly, Then, everything already flagged that lives entirely in this repo:
Already closed on this branch, for completeness: the Short answerLand the bump and #1 (the 10 MiB → 5 MiB default) is the only remaining item I'd call blocking — everything else on the list is docs, labels, or tests. But the bump is not a no-op for this PR: Nothing committed — this was an answer, not a change request. |
3 participants
Describe your changes and provide context
Extends the existing HTTP JSON-RPC admission control (
max_request_body_bytes/max_concurrent_request_bytes) to also cover the WebSocket plane (:8546), which previously used a hardcoded 10 MiB read limit and had no concurrent-byte budget at all.max_request_body_bytesnow bounds WS frame size in addition to the HTTP body size (viasrv.SetReadLimits), replacing the removedDefaultWebsocketMaxMessageSizeconstant.max_concurrent_request_bytesnow also bounds an independent WS-side in-flight byte budget (srv.SetWSConcurrentRequestBytes); WS requests block until budget frees or time out rather than being rejected immediately like HTTP.evmrpc_requests_rejected_total, now labeled with a newplane(http/ws) dimension instead of being HTTP-only.effectiveMaxRequestBodyByteshelper so the "0 means use the 5 MiB default" rule is shared between the HTTP limiter and the WS read-limit wiring.Testing performed to validate your change
evmrpc/ws_admission_test.gocoveringeffectiveMaxRequestBodyBytesand WS concurrent-request-byte budget enforcement/blocking behavior end-to-end over a real websocket connection.evmrpc/request_limiter_test.gofor the new shared helper.go test ./evmrpc/...