Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 73 additions & 8 deletions evmrpc/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

"github.com/ethereum/go-ethereum/rpc"
"github.com/sei-protocol/sei-chain/ratelimiter"
servertypes "github.com/sei-protocol/sei-chain/sei-cosmos/server/types"
"github.com/sei-protocol/sei-chain/sei-db/ledger_db/receipt"
"github.com/spf13/cast"
Expand Down Expand Up @@ -236,12 +237,26 @@ type Config struct {
TraceBakeSnapshotWindow int64 `mapstructure:"trace_bake_snapshot_window"` // recent snapshots to keep (default 64)

// IPRateLimitRPS is the per-IP sustained request rate in requests/second.
// Zero disables per-IP rate limiting (all requests pass through).
// Zero disables the token bucket (no HTTP 429 rejections). When
// rate_limiting_enabled is true, the admission middleware still runs: bodies
// are parsed and oversize/malformed requests are rejected before dispatch.
IPRateLimitRPS float64 `mapstructure:"ip_rate_limit_rps"`

// IPRateLimitBurst is the maximum per-IP burst size.
// IPRateLimitBurst is the maximum per-IP burst size. Zero disables the token
// bucket (same effect as ip_rate_limit_rps = 0) and does not bypass the
// admission middleware when rate_limiting_enabled is true. Should be at least
// batch_request_limit because the rate limiter charges one token per batch element.
IPRateLimitBurst int `mapstructure:"ip_rate_limit_burst"`

// RateLimitingEnabled is the master switch for the rate-limit admission
// middleware on the EVM HTTP plane. When false, requests bypass method
// extraction and all rejections from that layer (HTTP 400/413/429).
RateLimitingEnabled bool `mapstructure:"rate_limiting_enabled"`

// TrustedProxyCIDRs lists CIDRs whose X-Forwarded-For headers are trusted when
// resolving the client IP for rate limiting. Empty means trust no proxy.
TrustedProxyCIDRs []string `mapstructure:"trusted_proxy_cidrs"`

Comment thread
claude[bot] marked this conversation as resolved.
// BatchRequestLimit is the maximum number of requests allowed in a single
// JSON-RPC batch (HTTP and WebSocket). Set to 0 to disable the limit.
BatchRequestLimit int `mapstructure:"batch_request_limit"`
Expand All @@ -252,8 +267,8 @@ type Config struct {

// 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).
// before the body is buffered or JSON-decoded, including at the rate limiter
// method-extraction layer. 0 uses the go-ethereum default (5 MiB).
MaxRequestBodyBytes int64 `mapstructure:"max_request_body_bytes"`

// MaxConcurrentRequestBytes bounds the total size, in bytes, of HTTP
Expand All @@ -270,6 +285,8 @@ type Config struct {
MaxOpenConnections int `mapstructure:"max_open_connections"`
}

const defaultBatchRequestLimit = 1000

var DefaultConfig = Config{
HTTPEnabled: true,
HTTPPort: 8545,
Expand Down Expand Up @@ -321,8 +338,10 @@ var DefaultConfig = Config{
TraceBakeUseSnapshot: false,
TraceBakeSnapshotWindow: 64,
IPRateLimitRPS: 200,
IPRateLimitBurst: 400,
BatchRequestLimit: 1000,
IPRateLimitBurst: defaultBatchRequestLimit,

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] The "should be at least batch_request_limit" relationship is load-bearing but only documented, never enforced. Because one token is charged per batch element, any batch with more than burst elements gets a permanent 429 — it can never succeed no matter how idle the node is.

The upgrade path makes this concrete: existing nodes already have ip_rate_limit_burst = 400 written into app.toml from the old default, while batch_request_limit stays 1000. On those nodes, flipping rate_limiting_enabled = true permanently breaks every batch of >400 calls, with a 429 that gives the client no hint why.

Suggest validating at startup (error or warn + clamp burst up to batch_request_limit) rather than relying on operators reading the comment. Raising the default here doesn't help nodes with an existing config file.

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 changes the ip_rate_limit_burst default from 400 to 1000, which is a pinned contract: evmrpc/config/testdata/evm.golden still records IPRateLimitBurst = int(400), so TestDefaultsMatchTheRecordedValues fails. Regenerate the golden (go test ./evmrpc/config/ -run TestDefaultsMatchTheRecordedValues -update) and name the old/new value in the PR body per testutil/configtest/AGENTS.md §"Changing a Default". The two new fields on lines 342-343 also need golden entries.

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] This raises the default per-IP burst 2.5× (400 → 1000) for every deployment, not just ones that enable rate limiting — a security-relevant relaxation being made to satisfy the new burst ≥ batch_request_limit invariant rather than because 1000 is the right burst.

If the per-batch charge were capped (see the AllowN note on rate_limit.go), the invariant disappears and the 400 default can stand. If you keep the coupling, the golden-file diff is the right place to have surfaced it (and it did), but the PR description should call out the default change explicitly since it changes behavior for nodes that never opt in.

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] Aliasing the burst default to defaultBatchRequestLimit raises the per-IP burst 400 → 1000 (a 2.5x increase in the allowed non-batch burst, not just batch headroom) and couples the two defaults permanently: a future change to batch_request_limit would silently move ip_rate_limit_burst with it. Two separate constants, with the invariant enforced only by the validation at line 693, keeps the intent explicit.

RateLimitingEnabled: false,

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] The PR description says rate_limiting_enabled defaults to true ("stabilisation escape hatch"), but this ships false — and TestReadConfigRateLimiting asserts false, so the disabled default is deliberate. As written, none of the wiring in this PR takes effect on any node: newRateLimitMiddleware returns inner unchanged when !gate.enabled.

Please reconcile one way or the other. If shipping dark is intentional, fix the description and state which release flips it; if the intent was on-by-default, this line and the test need to change. Right now a reviewer trusting the description would believe :8545 is protected when it isn't.

TrustedProxyCIDRs: nil,
BatchRequestLimit: defaultBatchRequestLimit,
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
Expand Down Expand Up @@ -377,6 +396,8 @@ const (
flagTraceBakeSnapshotWindow = "evm.trace_bake_snapshot_window"
flagIPRateLimitRPS = "evm.ip_rate_limit_rps"
flagIPRateLimitBurst = "evm.ip_rate_limit_burst"
flagRateLimitingEnabled = "evm.rate_limiting_enabled"
flagTrustedProxyCIDRs = "evm.trusted_proxy_cidrs"
flagBatchRequestLimit = "evm.batch_request_limit"
flagBatchResponseMaxSize = "evm.batch_response_max_size"
flagMaxRequestBodyBytes = "evm.max_request_body_bytes"
Expand Down Expand Up @@ -631,6 +652,16 @@ func ReadConfig(opts servertypes.AppOptions) (Config, error) {
return cfg, err
}
}
if v := opts.Get(flagRateLimitingEnabled); v != 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.

[blocker] These two new reads need rows in the evmKeys manifest in evmrpc/config/config_fuzz_test.go (inserted here, in reader order), describing the read as written:

{Key: "evm.rate_limiting_enabled", Path: "RateLimitingEnabled", Cast: configtest.CastBool, Checked: true},
{Key: "evm.trusted_proxy_cidrs", Path: "TrustedProxyCIDRs", Cast: configtest.CastStringSlice, Checked: true},

Without them TestManifestNamesEveryField fails — CheckManifestCoversEveryField requires every resolved field to be claimed by a row or exempted, and RateLimitingEnabled/TrustedProxyCIDRs are neither. Also add the nil + malformed fuzz seeds so an ordinary go test run reaches the new rows (the loop at config_fuzz_test.go:108 covers that automatically, but double-check the hand-written positional seeds below it still point at the keys they name after the insertion).

if cfg.RateLimitingEnabled, err = cast.ToBoolE(v); err != nil {
return cfg, err
}
}
if v := opts.Get(flagTrustedProxyCIDRs); v != nil {
if cfg.TrustedProxyCIDRs, err = cast.ToStringSliceE(v); err != nil {
return cfg, err
}
}
Comment on lines +655 to +664

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note for later: as part of the configuration manager project we are creating comprehensive test coverage that asserts the behavior of config values. I know you have added lots of new config. It would be good to have a follow-up action item on these to make sure we have full test coverage over these values. I've created an AGENTS.md in #3816 that should make this simple

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for feedback. Created https://linear.app/seilabs/issue/PLT-874/ to make sure we have full coverage for the newly added config fields.

if v := opts.Get(flagBatchRequestLimit); v != nil {
if cfg.BatchRequestLimit, err = cast.ToIntE(v); err != nil {
return cfg, err
Expand Down Expand Up @@ -659,6 +690,14 @@ func ReadConfig(opts servertypes.AppOptions) (Config, error) {
return cfg, fmt.Errorf("%s must be >= 0 (0 disables the limit), got %d", flagMaxOpenConnections, cfg.MaxOpenConnections)
}
}
if cfg.RateLimitingEnabled && cfg.IPRateLimitBurst > 0 && cfg.BatchRequestLimit > 0 &&

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] Good guard. Worth adding a companion warning for the bigger operational footgun: rate_limiting_enabled = true with an empty trusted_proxy_cidrs behind a proxy/LB. IPFromHTTPRequest then falls back to RemoteAddr, every client collapses into the LB's single bucket, and one caller can 429 the entire node's traffic. Trust-none is the right default for a directly-exposed node so this can't be a hard error — but a startup logger.Warn when RateLimitingEnabled && len(TrustedProxyCIDRs) == 0 would catch the misconfiguration before a canary does.

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] This turns a previously-valid config into a hard boot failure, and the affected population is large: every existing node has ip_rate_limit_burst = 400 materialized into its app.toml from the old default, so the first thing an operator sees after setting rate_limiting_enabled = true is a node that won't start. The error text is good, but consider either clamping burst up to batch_request_limit with a logger.Warn, or calling the two-step config change out explicitly in the rollout notes so the opt-in isn't a surprise outage.

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] Good hard failure with an actionable message. One note on charging model: with burst == batch_request_limit, a single full-size batch drains the entire bucket, and at RPS 200 that client is throttled for ~5s before the next batch. If large batches are a normal client pattern on mainnet, consider charging one token per request (or a sub-linear weight per batch) rather than one per element — otherwise batching, which is cheaper for the node than N separate requests, is penalized harder than not batching.

cfg.IPRateLimitBurst < cfg.BatchRequestLimit {
return cfg, fmt.Errorf(
"%s (%d) must be >= %s (%d): the rate limiter charges one token per batch element, "+
"so a lower burst would permanently reject any full-size batch",
flagIPRateLimitBurst, cfg.IPRateLimitBurst, flagBatchRequestLimit, cfg.BatchRequestLimit,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Burst check ignores zero RPS

Low Severity

The startup check that ip_rate_limit_burst must be at least batch_request_limit runs whenever rate limiting is enabled and burst is positive, even if ip_rate_limit_rps is zero. With RPS at or below zero the token bucket is disabled and never charges, so this rejects a documented “admission only, no 429” configuration for no functional reason.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9054c11. Configure here.

}
Comment on lines +693 to +700

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The new burst-vs-batch validation in ReadConfig (config.go:693-700) checks IPRateLimitBurst < BatchRequestLimit but never checks IPRateLimitRPS, even though Registry.Allow treats RPS<=0 as a full bucket bypass (always allow, batch draining is impossible). This means the documented admission-only setup (rate_limiting_enabled=true, ip_rate_limit_rps=0, ip_rate_limit_burst=400, batch_request_limit=1000) now fails ReadConfig with a spurious error and blocks node startup, even though the guarded failure mode can't occur in that mode. Fix: also require cfg.IPRateLimitRPS > 0 before enforcing the burst>=batch check.

Extended reasoning...

The bug. ReadConfig (evmrpc/config/config.go:693-700) added in this PR rejects configs where RateLimitingEnabled && IPRateLimitBurst>0 && BatchRequestLimit>0 && IPRateLimitBurst<BatchRequestLimit, on the theory (documented in the new field comments) that the token bucket charges one token per batch element, so a burst smaller than the batch limit would permanently reject any full-size batch. That check never inspects IPRateLimitRPS.

Why the check is incomplete. Registry.Allow (ratelimiter/registry.go:93) short-circuits with if r.cfg.RPS <= 0 || r.cfg.Burst <= 0 { return true }. So whenever IPRateLimitRPS<=0, the token bucket is fully disabled and Allow always returns true — the per-method loop in RateLimitGate.Check never drains a token and can never reject a batch. The very failure mode the new validation guards against (batch draining causing a permanent 429) is impossible once RPS<=0. Yet ReadConfig still applies the burst-vs-batch check in that case, because it never looks at IPRateLimitRPS.

Why this contradicts documented behavior. This same PR's new doc comment on IPRateLimitRPS explicitly describes rate_limiting_enabled=true + ip_rate_limit_rps=0 as a supported "admission-only" mode: "Zero disables the token bucket (no HTTP 429 rejections). When rate_limiting_enabled is true, the admission middleware still runs: bodies are parsed and oversize/malformed requests are rejected before dispatch." A config like rate_limiting_enabled=true, ip_rate_limit_rps=0, ip_rate_limit_burst=400, batch_request_limit=1000 is exactly that mode — harmless, since the bucket is off — but it now fails ReadConfig with "ip_rate_limit_burst (400) must be >= batch_request_limit (1000)" and prevents the node from starting at all.

Why nothing catches it today. The new test TestReadConfigRateLimitingBurstBelowBatchLimitRejected only exercises ipRateLimitBurst=0 as the "disabled" case; getDefaultOpts() always sets ipRateLimitRPS=200.0, so the RPS=0-with-burst>0 combination is never exercised by any test.

Step-by-step proof.

  1. Operator upgrades a node that already has ip_rate_limit_burst = 400 in its app.toml (the old default before this PR bumped it to 1000), and separately decides to run the newly-documented admission-only mode by setting rate_limiting_enabled = true and ip_rate_limit_rps = 0 (leaving batch_request_limit at its default 1000).
  2. ReadConfig evaluates the new guard: RateLimitingEnabled is true, IPRateLimitBurst=400>0, BatchRequestLimit=1000>0, and 400 < 1000 — all four conditions hold, so it returns fmt.Errorf("... must be >= ...").
  3. This happens regardless of IPRateLimitRPS, even though with RPS=0, Registry.Allow would return true unconditionally and the batch-draining scenario the check exists to prevent cannot happen.
  4. The node fails to start, even though the chosen configuration is explicitly documented as valid and is in fact safe.

Fix. Add && cfg.IPRateLimitRPS > 0 to the guard's condition, mirroring the existing IPRateLimitBurst > 0 check, so the burst-vs-batch validation only fires when the token bucket is actually active.

Severity. This is a real correctness gap in the new validation and it does block node startup for an operator using a mode the PR itself documents as valid — but it's narrow: rate limiting ships disabled by default, the default burst(1000)==batch(1000) so the common enabled path is unaffected, the failure is a clear, immediate startup error (not silent misbehavior or data loss), and the workaround (raise or zero out ip_rate_limit_burst) is trivial once diagnosed. Given that, this is a nit rather than a blocking issue.

return cfg, nil
}

Expand All @@ -682,6 +721,15 @@ func normalizeNativeTracerNames(flagName string, names []string) ([]string, erro
return out, nil
}

// RateLimiterConfig builds the ratelimiter.Config used by EVM JSON-RPC admission.
func (c Config) RateLimiterConfig() ratelimiter.Config {
return ratelimiter.Config{
RPS: c.IPRateLimitRPS,
Burst: c.IPRateLimitBurst,
TrustedProxyCIDRs: c.TrustedProxyCIDRs,
}
}

// ConfigTemplate defines the TOML configuration template for EVM RPC
const ConfigTemplate = `
###############################################################################
Expand Down Expand Up @@ -902,12 +950,28 @@ trace_bake_use_snapshot = {{ .EVM.TraceBakeUseSnapshot }}
trace_bake_snapshot_window = {{ .EVM.TraceBakeSnapshotWindow }}

# ip_rate_limit_rps is the per-IP sustained request rate in requests/second.
# Set to 0 to disable per-IP rate limiting (all requests pass through).
# Set to 0 to disable per-IP throttling (no HTTP 429). Does not bypass the
# admission middleware; set rate_limiting_enabled = false for a full bypass.
ip_rate_limit_rps = {{ .EVM.IPRateLimitRPS }}

# ip_rate_limit_burst is the maximum per-IP burst above the sustained rate.
# Set to 0 to disable per-IP throttling (same effect as ip_rate_limit_rps = 0).
# Must be at least batch_request_limit when both are positive and rate_limiting_enabled
# is true (one token is charged per batch element) - enforced at startup.
ip_rate_limit_burst = {{ .EVM.IPRateLimitBurst }}

# rate_limiting_enabled is the master switch for the rate-limit admission
# middleware on the EVM HTTP plane (:8545). When false, requests bypass method
# extraction and all rejections from that layer (HTTP 400/413/429).
rate_limiting_enabled = {{ .EVM.RateLimitingEnabled }}

# trusted_proxy_cidrs lists CIDRs whose X-Forwarded-For headers are trusted when
# resolving the client IP for rate limiting. Empty means trust no proxy — set
# this to your ingress/LB CIDRs when the node sits behind a reverse proxy.
# Do NOT use the full RFC-1918 private ranges unless every address in them is
# your own controlled infrastructure.
trusted_proxy_cidrs = [{{- range $i, $c := .EVM.TrustedProxyCIDRs }}{{- if $i }}, {{ end }}"{{ $c }}"{{- end }}]

# batch_request_limit is the maximum number of requests allowed in a single
# JSON-RPC batch (HTTP and WebSocket). Set to 0 to disable the limit.
batch_request_limit = {{ .EVM.BatchRequestLimit }}
Expand All @@ -918,7 +982,8 @@ 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).
# is buffered or JSON-decoded, including at the rate limiter method-extraction
# layer. 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
Expand Down
2 changes: 2 additions & 0 deletions evmrpc/config/config_fuzz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ var evmKeys = []configtest.KeySpec{
{Key: "evm.trace_bake_snapshot_window", Path: "TraceBakeSnapshotWindow", Cast: configtest.CastInt64, Checked: true},
{Key: "evm.ip_rate_limit_rps", Path: "IPRateLimitRPS", Cast: configtest.CastFloat64, Checked: true},
{Key: "evm.ip_rate_limit_burst", Path: "IPRateLimitBurst", Cast: configtest.CastInt, Checked: true},
{Key: "evm.rate_limiting_enabled", Path: "RateLimitingEnabled", Cast: configtest.CastBool, Checked: true},
{Key: "evm.trusted_proxy_cidrs", Path: "TrustedProxyCIDRs", Cast: configtest.CastStringSlice, Checked: true},
{Key: "evm.batch_request_limit", Path: "BatchRequestLimit", Cast: configtest.CastInt, Checked: true},
{Key: "evm.batch_response_max_size", Path: "BatchResponseMaxSize", Cast: configtest.CastInt, Checked: true},
{Key: "evm.max_request_body_bytes", Path: "MaxRequestBodyBytes", Cast: configtest.CastInt64, Checked: true},
Expand Down
70 changes: 69 additions & 1 deletion evmrpc/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ type opts struct {
workerQueueSize interface{}
ipRateLimitRPS interface{}
ipRateLimitBurst interface{}
rateLimitingEnabled interface{}
trustedProxyCIDRs interface{}
batchRequestLimit interface{}
batchResponseMaxSize interface{}
maxRequestBodyBytes interface{}
Expand Down Expand Up @@ -177,6 +179,12 @@ func (o *opts) Get(k string) interface{} {
if k == "evm.ip_rate_limit_burst" {
return o.ipRateLimitBurst
}
if k == "evm.rate_limiting_enabled" {
return o.rateLimitingEnabled
}
if k == "evm.trusted_proxy_cidrs" {
return o.trustedProxyCIDRs
}
if k == "evm.batch_request_limit" {
return o.batchRequestLimit
}
Expand Down Expand Up @@ -247,7 +255,9 @@ func getDefaultOpts() opts {
32,
1000,
200.0,
400,
1000,
nil,
nil,
1000,
25 * 1000 * 1000,
int64(5 * 1024 * 1024),
Expand Down Expand Up @@ -632,3 +642,61 @@ func TestReadConfigMaxSubscriptionsLogs(t *testing.T) {
_, err = config.ReadConfig(&opts)
require.Error(t, err)
}

func TestReadConfigRateLimiting(t *testing.T) {
cfg, err := config.ReadConfig(&opts{})
require.NoError(t, err)
require.False(t, cfg.RateLimitingEnabled)
require.Nil(t, cfg.TrustedProxyCIDRs)
require.Equal(t, config.DefaultConfig.IPRateLimitRPS, cfg.IPRateLimitRPS)
require.Equal(t, config.DefaultConfig.IPRateLimitBurst, cfg.IPRateLimitBurst)
require.GreaterOrEqual(t, cfg.IPRateLimitBurst, cfg.BatchRequestLimit)

o := getDefaultOpts()
o.rateLimitingEnabled = false

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] This sets rateLimitingEnabled = false, which is also the default, so the assertion two lines down can't distinguish "the flag was read" from "the flag was ignored". Set it to true and assert true to actually cover the ReadConfig path for evm.rate_limiting_enabled (the false case is already covered by the zero-opts block above).

o.trustedProxyCIDRs = []string{"10.0.0.0/8", "203.0.113.0/24"}
cfg, err = config.ReadConfig(&o)
require.NoError(t, err)
require.False(t, cfg.RateLimitingEnabled)
require.Equal(t, []string{"10.0.0.0/8", "203.0.113.0/24"}, cfg.TrustedProxyCIDRs)

rlCfg := cfg.RateLimiterConfig()
require.Equal(t, cfg.IPRateLimitRPS, rlCfg.RPS)
require.Equal(t, cfg.IPRateLimitBurst, rlCfg.Burst)
require.Equal(t, cfg.TrustedProxyCIDRs, rlCfg.TrustedProxyCIDRs)
}

func TestReadConfigRateLimitingBurstBelowBatchLimitRejected(t *testing.T) {
// A burst below batch_request_limit would permanently reject full-size
// batches (one token is charged per batch element), so ReadConfig refuses
// it outright rather than letting an operator discover it in production.
o := getDefaultOpts()
o.rateLimitingEnabled = true
o.ipRateLimitBurst = 400
o.batchRequestLimit = 1000
_, err := config.ReadConfig(&o)
require.Error(t, err)

// Equal burst and batch limit is allowed.
o.ipRateLimitBurst = 1000
_, err = config.ReadConfig(&o)
require.NoError(t, err)

// The check is skipped when rate limiting is disabled...
o.rateLimitingEnabled = false
o.ipRateLimitBurst = 400
_, err = config.ReadConfig(&o)
require.NoError(t, err)

// ...when the token bucket itself is disabled (burst <= 0)...
o.rateLimitingEnabled = true
o.ipRateLimitBurst = 0
_, err = config.ReadConfig(&o)
require.NoError(t, err)

// ...and when the batch limit is unbounded (batch_request_limit <= 0).
o.ipRateLimitBurst = 400
o.batchRequestLimit = 0
_, err = config.ReadConfig(&o)
require.NoError(t, err)
}
4 changes: 3 additions & 1 deletion evmrpc/config/testdata/evm.golden
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ TraceBakeWindowBlocks = int64(0)
TraceBakeUseSnapshot = bool(false)
TraceBakeSnapshotWindow = int64(64)
IPRateLimitRPS = float64(200)
IPRateLimitBurst = int(400)
IPRateLimitBurst = int(1000)
RateLimitingEnabled = bool(false)
TrustedProxyCIDRs = <nil-slice>
BatchRequestLimit = int(1000)
BatchResponseMaxSize = int(25000000)
MaxRequestBodyBytes = int64(5242880)
Expand Down
9 changes: 6 additions & 3 deletions evmrpc/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ const (
jsonrpcCodeKey = "jsonrpc_code"
rejectReasonKey = "reason"
// reject reason values for requestRejectedCount.
rejectReasonOversize = "oversize" // body exceeded max_request_body_bytes
rejectReasonBusy = "busy" // max_concurrent_request_bytes budget exhausted
rejectReasonOversize = "oversize" // body exceeded max_request_body_bytes
rejectReasonBusy = "busy" // max_concurrent_request_bytes budget exhausted
rejectReasonRateLimited = "rate_limited" // per-IP token bucket exhausted
rejectReasonUnparseable = "unparseable" // rate-limit method parse failed (malformed JSON-RPC)
// error_class values; empty string ("") means success.
errorClassPanic = "panic"
errorClassExecutionReverted = "execution_reverted"
Expand Down Expand Up @@ -167,7 +169,8 @@ 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.
// admission control. reason is one of rejectReasonOversize / rejectReasonBusy /
// rejectReasonRateLimited / rejectReasonUnparseable.
// 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) {
Expand Down
66 changes: 66 additions & 0 deletions evmrpc/rate_limit.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package evmrpc

import (
"context"
"io"
"math"

"github.com/sei-protocol/sei-chain/ratelimiter"
)

// RateLimitGate applies per-IP token-bucket rate limiting after extracting JSON-RPC
// method names from the entire request body (bounded by max_request_body_bytes).
// When enabled, method extraction and fail-closed rejection (HTTP 400/413) run even
// if the registry is configured with RPS or burst zero (no HTTP 429 only).
type RateLimitGate struct {
registry *ratelimiter.Registry
parser *ratelimiter.MethodParser
maxBodyBytes int64
enabled bool
plane string
}

// NewRateLimitGate returns a gate for the given plane ("evm"). registry must be non-nil.
// maxBodyBytes is the same cap as max_request_body_bytes; non-positive values use
// defaultMaxRequestBodyBytes.
func NewRateLimitGate(registry *ratelimiter.Registry, maxBodyBytes int64, enabled bool, plane string) *RateLimitGate {
if maxBodyBytes <= 0 {
maxBodyBytes = defaultMaxRequestBodyBytes
}
if maxBodyBytes == math.MaxInt64 {
maxBodyBytes = math.MaxInt64 - 1
}
return &RateLimitGate{
registry: registry,
parser: ratelimiter.NewMethodParser(maxBodyBytes),
maxBodyBytes: maxBodyBytes,
enabled: enabled,
plane: plane,
}
}

// Check parses body for JSON-RPC method names and applies per-IP rate limits.
// rejectMethod is the method that exhausted the bucket when allowed=false.
// Parse errors still charge the bucket under ratelimiter.MethodInvalid so
// malformed bodies can't bypass rate limiting; if that exhausts the bucket,
// Check reports a rate-limit rejection instead of returning the parse error.
func (g *RateLimitGate) Check(ctx context.Context, ip string, body io.Reader) (allowed bool, rejectMethod string, err error) {
if !g.enabled {
return true, "", nil
}

methods, _, parseErr := g.parser.Parse(body)
if parseErr != nil {
if !g.registry.Allow(ctx, ip, g.plane, ratelimiter.MethodInvalid) {
return false, ratelimiter.MethodInvalid, nil
}
return false, "", parseErr
}
Comment thread
cursor[bot] marked this conversation as resolved.

for _, method := range methods {

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] One token is charged per batch element, but the defaults are ip_rate_limit_burst = 400 and batch_request_limit = 1000. rate.Limiter.Allow() never waits, so any batch with more than Burst elements can never succeed regardless of how idle the client is: the first 400 Allow calls drain the bucket, the 401st fails, and the whole batch is rejected with 429. Batches of 401–1000 items are permitted by batch_request_limit but unconditionally fail once this ships enabled by default.

Also note that the tokens consumed by the prefix methods are not refunded on rejection, so a client retrying a large batch keeps draining its bucket and can never make progress.

Suggestions: use limiter.AllowN(now, len(methods)) (which fails atomically without partial consumption), and/or reconcile the defaults so ip_rate_limit_burst >= batch_request_limit, and/or cap the charge per request. Whichever you pick, the two defaults should be consistent with each other.

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] Batch charging is not atomic: tokens for elements 0..i-1 are consumed and never returned when element i is rejected, even though nothing executes. A client retrying a batch that doesn't fit can drain each partial refill indefinitely and never make progress.

This interacts badly with the fact that batch_request_limit is enforced downstream by go-ethereum (srv.SetBatchLimits, rpcstack.go:332), after this loop: a 50k-element batch inside a 5 MiB body drains the whole bucket here and returns 429 instead of a batch-too-large error.

Prefer reserving len(methods) tokens up front (limiter.AllowN) and rejecting the batch without consuming anything if the reservation fails — and consider checking the element count against batch_request_limit before charging. Same point raised by Codex.

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] Two issues with charging one token per element in a loop:

  1. Tokens are consumed on a rejected batch. If element 5 of 10 exhausts the bucket, elements 1-4 already spent tokens and the whole batch is still rejected — the caller pays and gets nothing, and a retry loop drains its own bucket faster than the configured rate. AllowN(now, len(methods)) would make the charge atomic and match the documented "whole batch rejected" semantics.
  2. batch_request_limit isn't enforced yet at this layer. geth's batch-item limit is applied inside NewHTTPHandlerStack, downstream of here, so a 100k-element batch (trivially under 5 MiB) is fully parsed and charges 100k tokens before geth rejects it for being oversized. Checking len(methods) against the batch limit before charging would avoid both the wasted parse and the self-inflicted bucket drain.

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] batch_request_limit isn't consulted at this layer. readBatchMethods has no element cap, so a 5 MiB body of [{"method":"a"},...] produces ~350k parsed strings (plus a json.RawMessage copy per skipped value) before a single token is checked — and the request is then rejected downstream anyway for exceeding the 1000-element batch limit.

The token loop itself is self-bounding (it breaks at the first denial, so at most burst+1 Allow calls), so this is allocation waste rather than a lock-contention problem. Bailing out of the parse once the element count exceeds batch_request_limit would make the gate reject early and cheaply instead of doing the full parse for a request that can't succeed.

if !g.registry.Allow(ctx, ip, g.plane, method) {

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] Tokens consumed by earlier batch elements are not refunded when a later element is rejected. A 1000-element batch whose 999th element exhausts the bucket burns 998 tokens and returns 429 having served nothing — the client is throttled harder than the configured RPS implies, and an attacker can convert one request into a full bucket drain plus zero work.

Also note this charges before go-ethereum's batch_request_limit is enforced, so an over-limit batch (e.g. 5000 elements inside 5 MiB) drains up to the whole burst and returns 429 instead of the batch-limit error.

Consider reserving atomically up-front instead of per-element — e.g. limiter.AllowN(now, len(methods)) for the bucket decision, then use the per-method loop only to pick the label for the rejection metric. That also removes the need to raise the burst default to match batch_request_limit.

return false, method, 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.

Batch reject burns prior tokens

Medium Severity

Check admits batch methods by calling Allow per element. On a mid-batch denial the whole request is rejected, but tokens already taken for earlier methods are not returned. Clients (and any peers sharing an IP) lose budget for work that never ran; a batch just over the remaining burst can drain the bucket in one refused call.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c7edbde. Configure here.

}
return true, "", nil
Comment thread
claude[bot] marked this conversation as resolved.
}
Loading
Loading