Skip to content

fix(evmrpc): limit listener max open connections, configurable via max_open_connections (PLT-704)#3637

Open
amir-deris wants to merge 5 commits into
mainfrom
amir/plt-704-evmrpc-limit-listener-max-open-connection-config
Open

fix(evmrpc): limit listener max open connections, configurable via max_open_connections (PLT-704)#3637
amir-deris wants to merge 5 commits into
mainfrom
amir/plt-704-evmrpc-limit-listener-max-open-connection-config

Conversation

@amir-deris

@amir-deris amir-deris commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Describe your changes and provide context

Bounds the number of simultaneously accepted connections on the EVM JSON-RPC HTTP (:8545) and WebSocket (:8546) listeners to protect nodes from connection exhaustion (fd/goroutine pressure) under load or abuse.

  • Wrap the listener returned by net.Listen in Start() with netutil.LimitListener(l, maxOpenConns). Excess connections wait in the accept queue until an active connection closes.
  • Expose max_open_connections as an operator config field in evmrpc/config (evm.max_open_connections in app.toml), wired through ReadConfig and the config template.
  • Add HTTPServer.SetMaxOpenConns, set from config in both NewEVMHTTPServer and NewEVMWebSocketServer before the listener starts.
  • The limit is applied per listener (HTTP and WS each get their own budget).
  • Default is 2000. Set to 0 to disable the limit (unbounded, prior behavior). The wrap is guarded on maxOpenConns > 0, since netutil.LimitListener would otherwise block all connections at 0.

Testing performed to validate your change

  • TestMaxOpenConns (evmrpc): with a cap of 1, verifies a second connection is not served while the first holds the only slot, and is served once the first closes.
  • Updated evmrpc/config tests for the new field; go test ./evmrpc/... ./evmrpc/config/... passes.
  • go build ./evmrpc/... and gofmt -s clean.

@amir-deris amir-deris self-assigned this Jun 24, 2026
@amir-deris amir-deris changed the title Amir/plt 704 evmrpc limit listener max open connection config fix(evmrpc): limit listener max open connections, configurable via max_open_connections (PLT-704) Jun 24, 2026
@cursor

cursor Bot commented Jun 24, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes default RPC availability under heavy connection churn (clients may wait in the accept queue), but the behavior is tunable and 0 restores unbounded accepts.

Overview
Adds a configurable cap on simultaneously accepted TCP connections for the EVM JSON-RPC HTTP and WebSocket listeners to reduce connection-exhaustion risk under load or abuse.

Operators set evm.max_open_connections in app.toml (default 2000, 0 = unbounded, prior behavior). The limit applies per listener—HTTP and WS each get their own budget. HTTPServer.SetMaxOpenConns is applied from config in both server constructors; at Start(), the TCP listener is wrapped with netutil.LimitListener only when the value is > 0 (avoiding a zero cap that would block all accepts).

TestMaxOpenConns checks that with a cap of 1, a second client is not served until the first connection is released.

Reviewed by Cursor Bugbot for commit 34bc736. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJun 30, 2026, 9:26 AM

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.15%. Comparing base (b8c3929) to head (34bc736).

Files with missing lines Patch % Lines
evmrpc/config/config.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3637      +/-   ##
==========================================
- Coverage   59.17%   58.15%   -1.03%     
==========================================
  Files        2262     2176      -86     
  Lines      187055   176885   -10170     
==========================================
- Hits       110690   102859    -7831     
+ Misses      66427    64931    -1496     
+ Partials     9938     9095     -843     
Flag Coverage Δ
sei-chain-pr 68.70% <81.81%> (?)
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
evmrpc/rpcstack.go 79.00% <100.00%> (+0.45%) ⬆️
evmrpc/server.go 88.88% <100.00%> (+0.10%) ⬆️
evmrpc/config/config.go 72.05% <33.33%> (-0.88%) ⬇️

... and 128 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small, well-tested change that wraps the EVM HTTP/WS listeners with netutil.LimitListener, configurable via evm.max_open_connections. No blocking issues; a couple of minor robustness and formatting nits worth addressing.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • cursor-review.md was empty — the Cursor second-opinion pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
  • Per-listener budgeting means the documented default of 2000 actually permits up to ~4000 simultaneous connections across the HTTP (:8545) and WS (:8546) listeners combined. This is intended per the PR description, but operators may read max_open_connections = 2000 as a node-wide cap; consider clarifying the per-listener semantics in the app.toml comment as well (the struct doc comment already explains it, but the template comment does not).
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread evmrpc/config/config.go
}
}
if v := opts.Get(flagMaxOpenConnections); v != nil {
if cfg.MaxOpenConnections, err = cast.ToIntE(v); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] A negative value (e.g. an operator typo like -1) is accepted here and then silently disables the cap, because Start() only applies LimitListener when maxOpenConns > 0. The public config text states only 0 disables the limit, so this is surprising. Consider rejecting negatives (return an error) or normalizing them to 0 explicitly so the documented contract holds. (Also raised by Codex.)

Comment thread evmrpc/config/config.go
// batched JSON-RPC call (HTTP and WebSocket). Set to 0 to disable the limit.
BatchResponseMaxSize int `mapstructure:"batch_response_max_size"`


Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Stray double blank line — there are two consecutive blank lines (180 and 181) before the MaxOpenConnections comment. gofmt collapses consecutive blank lines to one, so this is not gofmt-clean; running gofmt -s -w will remove the extra line.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small, well-scoped change that bounds simultaneously-accepted connections on the EVM HTTP/WS listeners via netutil.LimitListener, configurable through max_open_connections with a sane default and a passing test. The only issue is a gofmt violation (double blank line) that will fail the lint CI gate.

Findings: 1 blocking | 2 non-blocking | 1 posted inline

Blockers

  • None at the file/PR level.
  • 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

  • Cursor's second-opinion review file (cursor-review.md) was empty — no output was produced by that pass.
  • HTTPServer.SetMaxOpenConns writes h.maxOpenConns under h.mu, but Start() reads it without holding the lock. This is safe in current call order (set is always invoked before Start in NewEVMHTTPServer/NewEVMWebSocketServer), but a brief comment or reading under the lock would make the invariant explicit.

Comment thread evmrpc/config/config.go
// batched JSON-RPC call (HTTP and WebSocket). Set to 0 to disable the limit.
BatchResponseMaxSize int `mapstructure:"batch_response_max_size"`


Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocker] This introduces a double blank line before the MaxOpenConnections comment, so the file is not gofmt-clean — gofmt -s collapses consecutive blank lines, and the golangci/CI gate will fail. Remove one of the blank lines (run gofmt -s -w evmrpc/config/config.go).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants