-
Notifications
You must be signed in to change notification settings - Fork 887
evmrpc: extend admission control to the WebSocket plane #3818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b3c9c5e
b55b32f
bab3825
6be7306
a5f1b61
3a568d5
1f9d1f1
63410e8
1e025cf
6f5a170
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -250,19 +250,26 @@ type Config struct { | |
| // batched JSON-RPC call (HTTP and WebSocket). Set to 0 to disable the limit. | ||
| BatchResponseMaxSize int `mapstructure:"batch_response_max_size"` | ||
|
|
||
| // MaxRequestBodyBytes is the maximum size, in bytes, of a single HTTP | ||
| // JSON-RPC request body. Requests larger than this are rejected (HTTP 413) | ||
| // before the body is buffered or JSON-decoded. 0 uses the go-ethereum | ||
| // default (5 MiB). | ||
| // 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. | ||
| // 0 uses the go-ethereum default (5 MiB). | ||
| MaxRequestBodyBytes int64 `mapstructure:"max_request_body_bytes"` | ||
|
|
||
| // MaxConcurrentRequestBytes bounds the total size, in bytes, of HTTP | ||
| // JSON-RPC request bodies admitted for processing concurrently, weighted by | ||
| // each request's Content-Length. Requests that would exceed the budget are | ||
| // rejected fast (HTTP 429) before decode, capping peak memory under load. | ||
| // Set to 0 to disable the limit. | ||
| // MaxConcurrentRequestBytes bounds the total size, in bytes, of HTTP and | ||
| // WebSocket JSON-RPC request bodies admitted for processing concurrently | ||
| // (independent budgets per protocol). HTTP uses Content-Length weighting and | ||
| // rejects over-budget requests fast (HTTP 429). WebSocket blocks until | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 |
||
| // budget frees or WSAdmissionTimeout elapses. Set to 0 to disable the limit | ||
| // on either protocol. | ||
| MaxConcurrentRequestBytes int64 `mapstructure:"max_concurrent_request_bytes"` | ||
|
|
||
| // WSAdmissionTimeout bounds how long a WebSocket connection waits for | ||
| // concurrent-byte budget to free before the next frame is read or committed. | ||
| // Zero or negative values use the go-ethereum default (30s). | ||
| WSAdmissionTimeout time.Duration `mapstructure:"ws_admission_timeout"` | ||
|
|
||
| // MaxOpenConnections caps the number of simultaneously accepted connections | ||
| // on the EVM HTTP and WebSocket listeners. The limit is applied per listener | ||
| // (HTTP and WS each get their own budget). Excess connections block in the | ||
|
|
@@ -326,6 +333,7 @@ var DefaultConfig = Config{ | |
| BatchResponseMaxSize: 25 * 1000 * 1000, // 25MB | ||
| MaxRequestBodyBytes: 5 * 1024 * 1024, // 5 MiB (matches go-ethereum rpc default body limit) | ||
| MaxConcurrentRequestBytes: 128 * 1024 * 1024, // 128 MiB of request bodies admitted concurrently | ||
| WSAdmissionTimeout: 30 * time.Second, // matches go-ethereum rpc defaultWSAdmissionTimeout | ||
| MaxOpenConnections: 2000, | ||
| } | ||
|
|
||
|
|
@@ -381,6 +389,7 @@ const ( | |
| flagBatchResponseMaxSize = "evm.batch_response_max_size" | ||
| flagMaxRequestBodyBytes = "evm.max_request_body_bytes" | ||
| flagMaxConcurrentRequestBytes = "evm.max_concurrent_request_bytes" | ||
| flagWSAdmissionTimeout = "evm.ws_admission_timeout" | ||
| flagMaxOpenConnections = "evm.max_open_connections" | ||
| ) | ||
|
|
||
|
|
@@ -651,6 +660,11 @@ func ReadConfig(opts servertypes.AppOptions) (Config, error) { | |
| return cfg, err | ||
| } | ||
| } | ||
| if v := opts.Get(flagWSAdmissionTimeout); v != nil { | ||
| if cfg.WSAdmissionTimeout, err = cast.ToDurationE(v); err != nil { | ||
| return cfg, err | ||
| } | ||
| } | ||
| if v := opts.Get(flagMaxOpenConnections); v != nil { | ||
| if cfg.MaxOpenConnections, err = cast.ToIntE(v); err != nil { | ||
| return cfg, err | ||
|
|
@@ -916,17 +930,23 @@ batch_request_limit = {{ .EVM.BatchRequestLimit }} | |
| # batched JSON-RPC call (HTTP and WebSocket). Set to 0 to disable the limit. | ||
| batch_response_max_size = {{ .EVM.BatchResponseMaxSize }} | ||
|
|
||
| # max_request_body_bytes is the maximum size, in bytes, of a single HTTP | ||
| # JSON-RPC request body. Larger requests are rejected (HTTP 413) before the body | ||
| # is buffered or JSON-decoded. Set to 0 to use the default (5 MiB). | ||
| # max_request_body_bytes is the maximum size, in bytes, of a single HTTP (:8545) | ||
| # or WebSocket (:8546) JSON-RPC request/frame. HTTP larger requests are rejected | ||
| # (HTTP 413) before decode; WS frames exceeding this limit are rejected by the | ||
| # read loop. Set to 0 to use the default (5 MiB). | ||
| max_request_body_bytes = {{ .EVM.MaxRequestBodyBytes }} | ||
|
|
||
| # max_concurrent_request_bytes bounds the total size, in bytes, of HTTP JSON-RPC | ||
| # request bodies admitted for processing concurrently (weighted by each request's | ||
| # Content-Length). Requests that would exceed the budget are rejected fast | ||
| # (HTTP 429) before decode, capping peak memory under load. Set to 0 to disable. | ||
| # max_concurrent_request_bytes bounds total request bytes admitted concurrently | ||
| # on HTTP (:8545) and WebSocket (:8546) (independent budgets per protocol). HTTP | ||
| # rejects over-budget requests fast (HTTP 429); WS blocks until budget frees or | ||
| # ws_admission_timeout elapses. Set to 0 to disable on either protocol. | ||
| max_concurrent_request_bytes = {{ .EVM.MaxConcurrentRequestBytes }} | ||
|
|
||
| # ws_admission_timeout bounds how long a WebSocket connection waits for | ||
| # concurrent-byte budget before the next frame is read or committed. Zero or | ||
| # negative values use the go-ethereum default (30s). | ||
| ws_admission_timeout = "{{ .EVM.WSAdmissionTimeout }}" | ||
|
|
||
| # max_open_connections caps the number of simultaneously accepted connections on | ||
| # the EVM HTTP and WebSocket listeners. Set to 0 to disable the limit. | ||
| max_open_connections = {{ .EVM.MaxOpenConnections }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ const ( | |
| errorClassKey = "error_class" | ||
| jsonrpcCodeKey = "jsonrpc_code" | ||
| rejectReasonKey = "reason" | ||
| protocolKey = "protocol" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Label-name inconsistency: this introduces |
||
| protocolHTTP = "http" | ||
| protocolWS = "ws" | ||
| // reject reason values for requestRejectedCount. | ||
| rejectReasonOversize = "oversize" // body exceeded max_request_body_bytes | ||
| rejectReasonBusy = "busy" // max_concurrent_request_bytes budget exhausted | ||
|
|
@@ -76,7 +79,7 @@ var ( | |
| )), | ||
| requestRejectedCount: must(rpcTelemetryMeter.Int64Counter( | ||
| "evmrpc_requests_rejected_total", | ||
| metric.WithDescription("Number of HTTP JSON-RPC requests rejected by pre-decode admission control (labeled by reason)"), | ||
| metric.WithDescription("Number of JSON-RPC requests rejected by admission control (labeled by protocol and reason)"), | ||
| metric.WithUnit("{count}"), | ||
| )), | ||
| } | ||
|
|
@@ -166,13 +169,19 @@ func recordHistoricalDebugTraceAttempt(ctx context.Context, endpoint, connection | |
| ) | ||
| } | ||
|
|
||
| // 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(protocolKey, protocolHTTP), | ||
| attribute.String(rejectReasonKey, reason), | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| func recordWSAdmissionRejected(ctx context.Context, reason string) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] The new function has no doc comment, and the diff also deletes the useful comment that used to sit on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Two doc issues in this hunk. (1) The doc comment removed from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Two things here:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Two things here:
|
||
| metrics.requestRejectedCount.Add(ctx, 1, | ||
| metric.WithAttributes( | ||
| attribute.String(protocolKey, protocolWS), | ||
| attribute.String(rejectReasonKey, reason), | ||
| ), | ||
| ) | ||
|
Comment on lines
169
to
187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The WebSocket admission-rejection path forwards raw fork-defined reason strings (e.g. Extended reasoning...
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 Why existing code doesn't prevent it: nothing type-checks or validates the Step-by-step proof:
Impact: this is purely an observability/metrics-contract inconsistency — the counter still increments correctly with a |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,9 +53,10 @@ type HTTPConfig struct { | |
|
|
||
| // WsConfig is the JSON-RPC/Websocket configuration | ||
| type WsConfig struct { | ||
| Origins []string | ||
| Modules []string | ||
| prefix string // path prefix on which to mount ws handler | ||
| Origins []string | ||
| Modules []string | ||
| prefix string // path prefix on which to mount ws handler | ||
| wsAdmissionTimeout time.Duration | ||
| RPCEndpointConfig | ||
| } | ||
|
|
||
|
|
@@ -381,7 +382,20 @@ func (h *HTTPServer) EnableWS(apis []rpc.API, config WsConfig) error { | |
| // 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 { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] This clamp looks copied from the HTTP path (rpcstack.go:331-337), where it's needed because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] This clamp is dead code on 64-bit ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Unlike the HTTP path (line 331-335), which needs this clamp because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] On 64-bit platforms There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] This clamp is dead code: the fork's signature is |
||
| readLimit = math.MaxInt | ||
| } | ||
| srv.SetReadLimits(readLimit) | ||
|
Comment on lines
384
to
+389
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The Extended reasoning...What the code does. In readLimit := effectiveMaxRequestBodyBytes(config.readLimit)
if readLimit > math.MaxInt {
readLimit = math.MaxInt
}
srv.SetReadLimits(readLimit)This mirrors the pattern used a few lines earlier in 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. Why this is dead code today. Step-by-step proof:
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 Fix. Either drop the clamp entirely (matching that |
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Two nits in this comment block: it contains a non-ASCII right single quote (U+2019) in |
||
| srv.SetWSConcurrentRequestBytes(config.maxConcurrentRequestBytes) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocker] The WS budget is passed through raw, without the normalization Worse, this PR's own Please normalize here (or validate in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocker] 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. The fork default is 30s ( What the wait ends in matters more than how long it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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
If that's the implementation, the effective WS limit is 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed on the mechanism, though framing it as in-flight requests understates the impact. Reproduced against There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] The production path never calls This also makes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 ( 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)
So every established WS connection permanently pins Before this PR
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 |
||
| srv.SetWSAdmissionTimeout(config.wsAdmissionTimeout) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idle WS ties up byte budgetMedium Severity Wiring Additional Locations (1)Reviewed by Cursor Bugbot for commit 6f5a170. Configure here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WS timeout zero not normalizedLow Severity
Reviewed by Cursor Bugbot for commit 6f5a170. Configure here. |
||
| srv.SetWSAdmissionEventHook(func(reason string) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Also note prod never calls
Comment on lines
+386
to
+395
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The shared, process-wide WS concurrent-byte budget ( Extended reasoning...What the bug is. The code path that triggers it. The budget itself, 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 ( 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 Step-by-step proof (from the PR's own test). 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] Related to the accounting question above: if the read loop reserves before a frame exists, this hook fires - and increments |
||
| // Hook carries no request context, and the fork's own connCtx is context.Background() too. | ||
| recordWSAdmissionRejected(context.Background(), reason) | ||
|
Comment on lines
382
to
+397
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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: Where oversize frames actually get rejected: The per-frame size cap set by this same PR ( Why this is inconsistent with the PR's own stated goal: The PR description says "Rejections on either plane are recorded through Step-by-step proof:
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 |
||
| }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WS reject reasons break metric contractLow Severity The WebSocket admission hook forwards fork-specific reason strings (such as budget wait timeout constants) unchanged into Additional Locations (1)Reviewed by Cursor Bugbot for commit 1f9d1f1. Configure here. |
||
| logger.Info("Registering apis for evm websocket") | ||
| if err := RegisterApis(apis, config.Modules, srv); err != nil { | ||
| return err | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,6 @@ var ConnectionTypeWS ConnectionType = "websocket" | |
| var ConnectionTypeHTTP ConnectionType = "http" | ||
|
|
||
| const LocalAddress = "0.0.0.0" | ||
| const DefaultWebsocketMaxMessageSize = 10 * 1024 * 1024 | ||
|
|
||
| type EVMServer interface { | ||
| Start() error | ||
|
|
@@ -339,7 +338,9 @@ func NewEVMWebSocketServer( | |
| } | ||
|
|
||
| wsConfig := WsConfig{Origins: strings.Split(config.WSOrigins, ",")} | ||
| wsConfig.readLimit = DefaultWebsocketMaxMessageSize | ||
| wsConfig.readLimit = config.MaxRequestBodyBytes | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 Separately, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] This silently halves the default WS frame cap: previously a hardcoded 10 MiB, now Minor cleanup while here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocker] This silently halves the WS frame ceiling for every operator on default config:
Suggest either keeping a dedicated WS knob (defaulting to the old 10 MiB, falling back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocker] This silently halves the default WS frame limit. It's worse for operators who already tuned 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] |
||
| wsConfig.maxConcurrentRequestBytes = config.MaxConcurrentRequestBytes | ||
| wsConfig.wsAdmissionTimeout = config.WSAdmissionTimeout | ||
| wsConfig.batchItemLimit = config.BatchRequestLimit | ||
|
Comment on lines
338
to
344
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The WS frame-size default silently drops from a hardcoded 10 MiB to 5 MiB (config.MaxRequestBodyBytes' default) because Extended reasoning...What changed. Before this PR, The code path that triggers it. Any node operator who upgrades to this version without explicitly setting 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 Impact. Any existing WS client that was previously relying on the 10 MiB ceiling — e.g. large Step-by-step proof.
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 🔬 also observed by seidroid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| wsConfig.batchResponseSizeLimit = config.BatchResponseMaxSize | ||
| if err := httpServer.EnableWS(apis, wsConfig); err != nil { | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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_subscribestreams, not just the oversized frame. Since operators tune this value to avoid outages, the doc should say the connection is closed.