PLT-1072: Wire rate limiter into native gRPC (:9090) unary+stream interceptors - #4021
PLT-1072: Wire rate limiter into native gRPC (:9090) unary+stream interceptors#4021amir-deris wants to merge 5 commits into
Conversation
Applied per-IP token-bucket admission on native gRPC
before handlers run, emitting rpc_rate_limit_rejected_total{plane="grpc"}.
Co-authored-by: Cursor <cursoragent@cursor.com>
amir-deris
changed the title
PR SummaryMedium Risk Overview
Metrics gain Reviewed by Cursor Bugbot for commit e633f59. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed71a10. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The config plumbing, metrics bucketing, and characterization-suite updates are solid, but the unary rate-limit interceptor is effectively dead code: BaseApp's RegisterGRPCServer wrapper discards the interceptor grpc-go hands to each MethodDesc.Handler, so no module Query service on :9090 is actually rate limited.
Findings: 1 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] All interceptor tests invoke the returned
grpc.UnaryServerInterceptor/StreamServerInterceptorclosures directly, so none of them exercise grpc-go's dispatch path. That is exactly why the BaseApp bypass is invisible to the suite. Add a test that starts a realgrpc.ServerviaStartGRPCServer(or at minimum registers a service throughBaseApp.RegisterGRPCServer), dials it, and assertscodes.ResourceExhaustedafter the burst is spent. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
BaseApp.RegisterGRPCServer rebuilt every grpc.MethodDesc with a handler that discarded the grpc.UnaryServerInterceptor grpc-go passes in. Since grpc-go hands s.opts.unaryInt to MethodDesc.Handler rather than invoking it itself, every service registered through RegisterGRPCServer -- all module Query services, cosmos.tx.v1beta1.Service and cosmos.base.tendermint.v1beta1.Service -- ran no server-level interceptor. The rate limiter wired onto :9090 therefore covered only the directly-registered reflection service. Thread the incoming interceptor into the chain at that wrapper handler, the single point every unary query passes through, so any grpc.ChainUnaryInterceptor applies without each registration site re-adding it. The query-context interceptor runs last, so admission control rejects a call before it costs a query context. Add tests that register through RegisterGRPCServer on a real grpc.Server and dial it, covering both the generic interceptor contract and the rate limiter reaching ResourceExhausted once a burst is spent. The existing interceptor tests invoked the closures directly, which is why the bypass was invisible. Also correct the stale [grpc] key counts and clamp line reference in the configuration characterization suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@seidroid review |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
The interceptor wiring and the BaseApp dispatch fix from the previous round look correct and are now covered by real-server regression tests. However, the four new [grpc] config fields are auto-derived into the config/registry key space, and config/cosmosbase was not updated to match, which breaks two characterization tests on the base branch.
Findings: 1 blocking | 5 non-blocking | 5 posted inline
Blockers
config/cosmosbasewas not updated for the four new[grpc]keys, and two of its tests will fail.registry.RegisterSection(GRPCSectionName, &srvconfig.GRPCConfig{}, grpcDefaults)(config/cosmosbase/cosmosbase.go:40) derives the section's declared keys fromGRPCConfig'smapstructuretags (config/registry/registry.go:144→deriveKeys/walk), so addingip-rate-limit-rps,ip-rate-limit-burst,rate-limiting-enabled, andtrusted-proxy-cidrstakes the registered set from 11 to 15 keys automatically. Two tests still hold the old set: (1)TestTheGRPCKeysAreTheOnesItsReaderResolves(config/cosmosbase/cosmosbase_test.go:75-81) doesreflect.DeepEqualagainst a hard-coded 11-key list and will fail; (2)TestEveryKeyTheseSectionsDeclareIsOneTheReaderResolves(config/cosmosbase/agreement_test.go:184) walks every registered key and errors for each one absent fromreaderValues, which still stops atgrpc.keepalive-permit-without-stream(agreement_test.go:102-112) — four errors. Fix: add the four keys to therequireDeclareslist and add the fourcfg.GRPC.*entries toreaderValues. While there, thegrpcDefaultsgodoc (cosmosbase.go:99) still reads "Six of these eleven keys are read only when the key is present"; it is now nine of fifteen, and per AGENTS.md that count is part of the record rather than incidental prose.
Non-blocking
- None at the file/PR level.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4021 +/- ##
==========================================
- Coverage 61.28% 60.30% -0.99%
==========================================
Files 2154 2056 -98
Lines 188439 176796 -11643
==========================================
- Hits 115491 106614 -8877
+ Misses 62219 60419 -1800
+ Partials 10729 9763 -966
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Register the four new [grpc] keys in config/cosmosbase so characterization tests stay in sync, fix config-test doc/grouping drift, and correct knownGRPCServices for reflection metrics. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
The previous round's blocker is genuinely fixed: the BaseApp dispatch bypass is closed at the one wrapper handler every unary query passes through, real-server regression tests cover it, and the four new [grpc] keys are now declared in config/cosmosbase with consistent counts and a corrected knownGRPCServices list. Two suggestions from the last review remain open (no operator signal when admission is enabled with a zeroed bucket, and the unstated per-stream admission scope), and gRPC-Web silently shares these interceptors despite being listed as out of scope.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] gRPC-Web is not actually out of scope:
StartGRPCWebwraps this same*grpc.Server(sei-cosmos/server/grpc/grpc_web.go:29), so both interceptors also govern:9091, and its traffic draws tokens from the same per-IP buckets as:9090. grpc-go's handler-server transport does set the peer fromreq.RemoteAddr, so per-IP attribution works there, but it means[grpc] trusted-proxy-cidrsalso decides whosex-forwarded-foris honoured on the browser-facing port. Worth confirming that is intended and saying so on the config keys rather than leaving the PR description's "Out of scope: gRPC-Web" to imply :9091 is untouched. - [suggestion] Nothing exercises
StartGRPCServer's enable path.rate_limit_dispatch_test.gobuilds its owngrpc.NewServer(grpc.ChainUnaryInterceptor(...)), and no test callsStartGRPCServerwithRateLimitingEnabled = true, so deleting the twoserverOptsappends — or theratelimiter.Newerror return for a malformed CIDR — leaves the suite green. That is the same class of gap that hid last round's BaseApp bypass, one layer up. - [suggestion] The three
v.IsSet-guarded keys are recorded only by name and for their absent-key resolution; no test reads a value that is actually present. The six pre-existing guarded[grpc]keys are covered on that path byTestGetConfigGRPCOverrides(config_test.go:167-190), which was not extended. A typo repeated in both theIsSetcall and the getter (e.g.grpc.ip-rate-limit-rpin both) would pass every test in the suite today. Addingip-rate-limit-rps,ip-rate-limit-burstandtrusted-proxy-cidrsto that test closes it. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| }), | ||
| ) | ||
| } | ||
| if cfg.RateLimitingEnabled { |
There was a problem hiding this comment.
[suggestion] Still no operator signal when admission is enabled but the bucket is off. Registry.Allow returns true unconditionally when RPS <= 0 || Burst <= 0 (ratelimiter/registry.go:93), so rate-limiting-enabled = true with either bucket key at zero or negative gives an unprotected :9090 with the interceptors installed and throttling nothing — silently.
The CometBFT plane logs for exactly this combination (sei-tendermint/internal/rpc/core/env.go:352-359, and again in internal/inspect/rpc/rpc.go:85-91), so the asymmetry is what a reader will trip on. StartGRPCServer takes no logger today, so this needs either a logger parameter or the check moved to the caller in sei-cosmos/server/start.go:421, which has one.
Codex flagged the same thing independently.
| // StreamRateLimitInterceptor returns a server interceptor that applies per-IP | ||
| // token-bucket rate limiting when a client stream is established. registry must | ||
| // be non-nil. | ||
| func StreamRateLimitInterceptor(registry *ratelimiter.Registry) grpc.StreamServerInterceptor { |
There was a problem hiding this comment.
[suggestion] Worth stating the residual gap in this godoc: one token is spent at stream establishment and nothing after. The only streaming surface on :9090 is server reflection, and ServerReflectionInfo is bidirectional — a client whose single stream is admitted can then send unbounded FileContainingSymbol/ListServices messages over it, each doing a descriptor lookup, with no further accounting.
If per-stream admission is the intended scope for Phase 1, say so here (and ideally note the follow-up), so the next reader does not take "rate limiting when a client stream is established" to mean streams are throttled per message.
Superseded: latest AI review found no blocking issues.
1 participant
Summary
Wires the shared
ratelimiter.Registry(from PLT-411 / PLT-800) into native gRPC on:9090via unary and stream server interceptors. Rejections returncodes.ResourceExhaustedand emitrpc_rate_limit_rejected_total{plane="grpc", method_namespace="..."}.This is the last Phase 1 plane; EVM HTTP (PLT-819), CometBFT HTTP (PLT-981), and the core registry/parser are already landed.
sei-cosmos/server/grpc/rate_limit.go) — per-IP token bucket at unary call and stream establishment; usesRegistry.IPFromGRPCContextandinfo.FullMethod(noMethodParser/ body pre-read needed on gRPC).StartGRPCServer) — interceptors chained at server creation, before BaseApp query handler registration, so admission runs beforesdk.Contextcreation.[grpc]inapp.toml) —rate_limiting_enabled,ip_rate_limit_rps,ip_rate_limit_burst,trusted_proxy_cidrs; ships disabled by default (same rollout pattern as 1b/1c).ratelimiter/method_bucket.go) — addsPlaneGRPCand low-cardinality service-name labels for known protobuf services.Out of scope: gRPC-Web (
:9091, Phase 4d).Test plan
go test ./ratelimiter/... ./sei-cosmos/server/grpc/... ./sei-cosmos/server/config/...ResourceExhausted, per-IP isolation, trusted-proxy XFF[grpc]keysgrpc.rate-limiting-enabled = trueon a dev node and confirm 429-equivalent gRPC rejections under burst load