rpc: add gzip response compression - #3595
Conversation
PR SummaryMedium Risk Overview The middleware uses a pooled gzip writer, sets Wiring: the handler wraps the root HTTP stack on the main Tendermint RPC server (after CORS), the inspect RPC handler, and the Cosmos SDK API server (with or without unsafe CORS). Tests in Reviewed by Cursor Bugbot for commit cb74faa. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
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 #3595 +/- ##
==========================================
- Coverage 60.20% 58.06% -2.15%
==========================================
Files 2328 2135 -193
Lines 194502 172356 -22146
==========================================
- Hits 117104 100071 -17033
+ Misses 66849 63414 -3435
+ Partials 10549 8871 -1678
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
bdchatham
left a comment
bdchatham
left a comment
There was a problem hiding this comment.
A few findings from an independent cross-review (systems / networking / security lenses) of the gzip middleware. The negotiation design is solid and client-side is backward-compatible — these three are origin-side concerns worth addressing before enabling on the public RPC/REST listeners. Line comments below.
bdchatham
left a comment
bdchatham
left a comment
There was a problem hiding this comment.
Follow-up: a few stream-lifecycle (layer-2) correctness nits. A corrupt/truncated stream is worse than a missed compression, so these are worth tightening.
|
On testing — I'd lean on a single-process round-trip harness here rather than adding more The core invariant is just bytes in == bytes out, for every shape of payload. One helper serves a body through The one subtlety worth calling out: decode with Two more the recorder can't express: a concurrent variant (a couple hundred goroutines, each asserting its own payload comes back) run under The swallowed Happy to write the harness up if it's useful. And dm me if you have questions on any of this. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
amir-deris
left a comment
amir-deris
left a comment
There was a problem hiding this comment.
Addressed feedbacks!
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a well-structured, well-tested gzip response-compression middleware (NewGzipHandler) and wires it into the Tendermint RPC, inspect RPC, and Cosmos API servers. No blocking issues, but a couple of response-corruption edge cases and a largely-ineffective size threshold are worth addressing.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Performance: the
minCompressBytes(1024) skip only triggers when aContent-Lengthheader is present at first write. Most Tendermint JSON-RPC handlers write the body directly without settingContent-Length, so the threshold rarely applies and small responses get gzipped anyway — wasting CPU and potentially enlarging sub-1KB bodies. Consider buffering the first chunk to decide, or accept the cost explicitly. - Cursor's review pass (
cursor-review.md) produced no output — only the OpenAI Codex pass returned findings, which are incorporated below. - Test plan checkboxes in the PR description (build, local smoke-test, websocket check) are unchecked; confirm these were exercised before merge given this sits in the hot path for all RPC/API responses.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
A well-structured, thoroughly tested gzip response-compression middleware wired into the Tendermint RPC, inspect RPC, and Cosmos SDK API servers. The implementation is solid, but it compresses 206 Partial Content / Range responses, which corrupts range-aware clients of the swagger static file server; a couple of smaller robustness/operability notes also apply.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No runtime toggle: compression is now unconditionally enabled on all three servers (Tendermint RPC, inspect RPC, Cosmos SDK API). For a cost-optimization rollout, consider a config flag so it can be disabled without a redeploy if it causes unexpected CPU load or client issues.
- Already-compressed payloads (e.g. PNG/woff/.gz static swagger assets, or pre-gzipped upstream responses without a Content-Encoding header) will be re-compressed, wasting CPU for little/no gain. The Content-Encoding passthrough check handles explicitly-encoded responses, but there is no content-type skip-list. Minor; acceptable if JSON-RPC is the dominant traffic.
- REVIEW_GUIDELINES.md and cursor-review.md were empty, so no repository-specific standards and no Cursor second-opinion pass were available to incorporate. The Codex pass produced exactly one finding (the 206 issue), which is incorporated below.
- Test plan checkboxes in the PR description (build, local smoke-test, WebSocket verification) are unchecked; confirm these manual steps were run before merge.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, push a new commit or reopen this pull request to trigger a review.
There was a problem hiding this comment.
Adds a well-structured, well-tested gzip response-compression middleware and wires it into the Tendermint RPC, inspect RPC, and Cosmos API servers. No blocking correctness bugs found; a few edge-case and efficiency concerns are worth addressing before merge.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) and the repo's REVIEW_GUIDELINES.md were both empty, so those inputs contributed nothing to this synthesis; review relied on the diff and Codex's pass.
- Efficiency: because most handlers do not set Content-Length before writing (net/http computes it only after the handler returns), the
minCompressBytessmall-response skip in init() rarely triggers. Tiny JSON-RPC responses (e.g.{"jsonrpc":"2.0","id":1}) will therefore be gzipped and grow by ~20+ bytes of framing overhead, which slightly works against the egress-reduction goal in the design doc. Consider buffering small writes to apply the threshold even without an explicit Content-Length, or documenting the limitation. - No configuration flag to enable/disable compression: the middleware is unconditionally wired into all three servers. Consider gating it behind config so operators can opt out without a code change.
- Consider a brief note in the changelog / operator docs that RPC responses are now gzip-encoded when clients advertise
Accept-Encoding: gzip, since this changes on-the-wire behavior for existing clients/proxies. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Vary must be set on every response — compressed or not — so that CDN | ||
| // caches key on Accept-Encoding and never serve a wrong variant. | ||
| ensureVaryAcceptEncoding(w.Header()) |
There was a problem hiding this comment.
[suggestion] Vary: Accept-Encoding is set here before the inner handler runs. If a downstream handler or CORS middleware later calls w.Header().Set("Vary", ...) (rather than Add), it overwrites this value and drops Accept-Encoding. A CDN keyed only on the remaining Vary token could then serve a gzip-encoded variant to a client that never sent Accept-Encoding: gzip. Of the two wirings, rs/cors (env.go) uses Header().Add("Vary", ...) and is safe, but gorilla handlers.CORS (sei-cosmos server.go) and any future handler using Set are not guaranteed to be. Safer to (re)ensure the Vary token at header-commit time (in the wrapper's init()/WriteHeader) as well as up front. (Raised by Codex.)
| w.gz = gzPool.Get().(*gzip.Writer) | ||
| w.gz.Reset(w.resp) | ||
| hdr.Del("content-length") | ||
| hdr.Set("content-encoding", "gzip") |
There was a problem hiding this comment.
[nit] Setting Content-Encoding: gzip before the first write disables net/http's automatic Content-Type sniffing: with no explicit Content-Type, the server sniffs the compressed bytes and emits Content-Type: application/x-gzip instead of the real (uncompressed) type. Impact is low here because the Tendermint/Cosmos JSON-RPC handlers set Content-Type explicitly, but any handler relying on sniffing would be affected. Consider capturing/deriving Content-Type from the uncompressed bytes before enabling gzip. (Raised by Codex.)
Address review feedback on the gzip middleware: - Skip compression for 206 Partial Content responses and any response carrying a Content-Range header, whose offsets describe the uncompressed body (reachable via the swagger http.FileServer served through the Cosmos API router). - On a panic unwinding mid-response, return the pooled writer via abort() without emitting the gzip footer, so the recovery handler's plaintext 500 body isn't framed behind a valid gzip trailer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| } | ||
| gzPool.Put(w.gz) | ||
| w.gz = nil | ||
| } |
There was a problem hiding this comment.
Panic corrupts gzip-encoded responses
Medium Severity
abort returns the gzip writer to the pool without clearing Content-Encoding, then re-panics into recoverAndLogHandler, which writes a plaintext 500 directly to the inner ResponseWriter. After init has run, clients still see Content-Encoding: gzip with a non-gzip body, so decompression fails.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit affed4d. Configure here.
| if f, ok := w.resp.(http.Flusher); ok { | ||
| f.Flush() | ||
| } | ||
| } |
There was a problem hiding this comment.
Flush blocked by statusWriter wrapper
Low Severity
gzipResponseWriter.Flush type-asserts w.resp as http.Flusher, but in every Serve/ServeTLS path resp is statusWriter, which embeds the http.ResponseWriter interface and does not implement Flusher. Compressed bytes therefore sit in the server buffer until the handler returns, despite the streaming-flush test and comments claiming incremental delivery works.
Reviewed by Cursor Bugbot for commit affed4d. Configure here.
There was a problem hiding this comment.
Adds a well-tested gzip response-compression middleware wired into the Tendermint and Cosmos RPC/API servers; the implementation is careful (Accept-Encoding q-values, WebSocket/Hijack passthrough, bodyless-status and panic handling) but omits Content-Type preservation before compressing, which can produce a wrong sniffed Content-Type for handlers that rely on auto-detection.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdandREVIEW_GUIDELINES.mdare both empty — the Cursor pass produced no output, and no repo-specific review guidelines were available.- Security consideration (non-blocking): compressing responses that may combine sensitive data with attacker-influenced input over TLS enables BREACH-class compression oracles. RPC responses generally don't reflect secrets, but worth a note in the design doc given this sits behind CloudFront.
- Nice test coverage overall (round-trip sizes, concurrency, streaming flush, panic footer, q-values, WebSocket passthrough). Two behaviors are untested and correspond to the findings below: Content-Type auto-detection on compressed responses, and Vary preservation when an inner handler overwrites Vary via Set.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| w.gz = gzPool.Get().(*gzip.Writer) | ||
| w.gz.Reset(w.resp) | ||
| hdr.Del("content-length") | ||
| hdr.Set("content-encoding", "gzip") |
There was a problem hiding this comment.
[suggestion] Content-Type is not preserved before compression. Once Content-Encoding: gzip is set here and Write sends gzip bytes to the underlying ResponseWriter, net/http's content sniffing (triggered when the inner handler didn't set Content-Type explicitly) inspects the gzip magic bytes and emits Content-Type: application/x-gzip. Standard gzip middleware captures/sets the content type from the first pre-compression write (or calls http.DetectContentType on the uncompressed bytes) to avoid this. The wired JSON-RPC and gRPC-gateway handlers set application/json explicitly so impact is limited, but any handler relying on auto-detection will now serve a wrong Content-Type. (Raised by Codex P2.)
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Vary must be set on every response — compressed or not — so that CDN | ||
| // caches key on Accept-Encoding and never serve a wrong variant. | ||
| ensureVaryAcceptEncoding(w.Header()) |
There was a problem hiding this comment.
[nit] Vary: Accept-Encoding is set at handler entry, before the inner handler runs. An inner handler that does Header().Set("Vary", ...) would clobber it, letting a cache serve a compressed variant to a non-gzip client. The two CORS layers actually wired here (rs/cors and gorilla handlers.CORS) use Header().Add("Vary", ...), so the current wiring is safe — but this is fragile. Consider re-asserting Vary in init()/WriteHeader (when headers are committed) instead of at entry. (Raised by Codex P1; low practical impact given current wiring.)
There was a problem hiding this comment.
Adds a well-tested, generic gzip response-compression middleware and wires it into the Tendermint RPC and Cosmos API servers. No blocking correctness or security issues found; two defensive suggestions from Codex are valid but low-impact for the handlers actually wired up here. Cursor produced no output.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output.
- Compression is now applied to unauthenticated RPC/API endpoints. For this codebase the responses are public blockchain data (no session secrets reflected with attacker-controlled input), so BREACH/CRIME risk is negligible and gzip.BestSpeed bounds CPU cost — noting only for completeness; no change needed.
- Minor efficiency note: when the inner handler does not set Content-Length (e.g. chunked/streaming responses), init() always compresses even tiny bodies, so the minCompressBytes threshold only helps the known-length path. This matches the documented design intent; acceptable.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| w.gz = gzPool.Get().(*gzip.Writer) | ||
| w.gz.Reset(w.resp) | ||
| hdr.Del("content-length") | ||
| hdr.Set("content-encoding", "gzip") |
There was a problem hiding this comment.
[suggestion] Codex P2: content-type sniffing. If the inner handler writes a body without setting Content-Type, net/http sniffs the first bytes written to determine it — which will now be the gzip header, yielding Content-Type: application/x-gzip instead of the real type. Consider sniffing/preserving the uncompressed Content-Type (e.g. via http.DetectContentType on the first chunk) before the first compressed write. In practice the handlers wired up here (JSON-RPC writeHTTPResponse, gRPC-gateway, FileServer) all set Content-Type explicitly, so this is defensive rather than an active bug — hence a suggestion, but worth hardening since the middleware is generic.
| if w.gz != nil { | ||
| _ = w.gz.Flush() | ||
| } | ||
| if f, ok := w.resp.(http.Flusher); ok { |
There was a problem hiding this comment.
[suggestion] Codex P1: gzipResponseWriter statically implements http.Flusher, so any wrapped handler sees w.(http.Flusher) == true and believes a flush reached the network — but Flush() only forwards when the underlying writer is a Flusher. In both wired-in paths the underlying writer is sei-tendermint's statusWriter (from recoverAndLogHandler), which embeds only http.ResponseWriter+http.Hijacker and is NOT a Flusher, so gz.Flush() runs but the network flush is a silent no-op. Note this is not a regression: statusWriter already masked net/http's Flusher before this PR, so streaming through this server never flushed incrementally, and the final response is still a complete/valid gzip stream. The only new wrinkle is falsely advertising flush capability. Fine to merge as-is; flagging so it's a conscious choice.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cb74faa. Configure here.
| } | ||
| w.init() | ||
| w.resp.WriteHeader(status) | ||
| } |
There was a problem hiding this comment.
1xx responses disable later compression
Low Severity
WriteHeader treats 1xx statuses like final bodyless responses and sets inited permanently. Interim 1xx writes must not lock out compression, but a later final WriteHeader/Write then skips init, so an otherwise compressible response stays identity-encoded.
Reviewed by Cursor Bugbot for commit cb74faa. Configure here.
| if len(length) > 0 { | ||
| if n, err := strconv.ParseUint(length, 10, 64); err == nil { | ||
| w.hasLength = true | ||
| w.contentLength = n | ||
| } | ||
| } | ||
|
|
||
| // Skip compression if the inner handler already encoded the response, opted | ||
| // out via Transfer-Encoding: identity, or emitted a byte range whose | ||
| // Content-Range offsets describe the uncompressed body. | ||
| if hdr.Get("content-encoding") != "" || hdr.Get("transfer-encoding") == "identity" || | ||
| hdr.Get("content-range") != "" { | ||
| return | ||
| } | ||
|
|
||
| // Skip compression for small responses with a known Content-Length; below | ||
| // the threshold the gzip overhead outweighs the savings and the CPU cost | ||
| // per byte is not worth it for unauthenticated callers. | ||
| if w.hasLength && w.contentLength < minCompressBytes { | ||
| return | ||
| } | ||
|
|
||
| w.gz = gzPool.Get().(*gzip.Writer) |
There was a problem hiding this comment.
🟡 The minCompressBytes size-skip safeguard in gzipResponseWriter.init() never triggers for the main Tendermint JSON-RPC handlers, because writeHTTPResponse/writeRPCResponse (sei-tendermint/rpc/jsonrpc/server/http_server.go) call WriteHeader then Write without ever setting Content-Length — so init() runs while hasLength is still false and every response, including tiny ones, gets gzip-compressed. This makes the added CPU-amplification guard effectively dead code for the primary RPC traffic it was meant to protect (it does still work for handlers that explicitly set Content-Length, e.g. the swagger http.FileServer path).
Extended reasoning...
What the bug is. gzipResponseWriter.init() (gzip.go:41-63) is supposed to skip compression for small responses via: if w.hasLength && w.contentLength < minCompressBytes { return }. hasLength/contentLength are only populated a few lines earlier from hdr.Get(\"content-length\"). If that header is empty at the time init() runs, hasLength stays false and the size-skip branch can never be taken — the writer falls through and always compresses.
The code path that triggers it. The two real call sites for the Tendermint JSON-RPC server are writeHTTPResponse (http_server.go:144-165) and writeRPCResponse (http_server.go:173-196). Both do exactly:
w.Header().Set(\"Content-Type\", \"application/json\")
w.WriteHeader(statusCode)
w.Write(body)Neither sets Content-Length anywhere. gzipResponseWriter.WriteHeader invokes init() immediately (for any non-bodyless status), and at that point w.resp.Header() genuinely has no content-length key — net/http's own auto-computed Content-Length (when the whole body fits in one buffered write) is only applied by the standard library at a layer below this wrapper, and only after the handler returns, so this wrapper's init() never observes it. I verified this directly against the current http_server.go in the diff/tree: both functions set only Content-Type, call WriteHeader, then Write, with no Content-Length anywhere in between.
Why nothing else prevents it. The size guard is genuinely conditioned on hasLength, and there's no fallback that infers size from the body slice length that's about to be written in Write() — by the time Write() is called, init() (and the compress/no-compress decision) has already run and been locked in via WriteHeader. So for this specific call pattern (WriteHeader before Write, no explicit Content-Length), the threshold is structurally unreachable.
Concrete proof. Consider a request to /status with Accept-Encoding: gzip, returning a body like {\"jsonrpc\":\"2.0\",\"id\":1,\"result\":{...}} that happens to be 40 bytes:
NewGzipHandlerseesacceptsGzip(r) == trueand no websocket upgrade, so it wrapswin agzipResponseWriter.writeHTTPResponseruns:w.Header().Set(\"Content-Type\", ...), thenw.WriteHeader(200).- Inside
WriteHeader, status 200 isn't in the bodyless set, sow.init()runs.hdr.Get(\"content-length\")is\"\"(never set) →hasLength = false. The size-skip branchif w.hasLength && w.contentLength < minCompressBytesevaluates toif false && ...→ false → not taken.w.gzis allocated from the pool andContent-Encoding: gzipis set. w.Write(body)(40 bytes) goes throughw.gz.Write(...), gzip-compressing a 40-byte payload (which nets out larger on the wire than the original once gzip's ~20-byte header/footer overhead is added), purely for CPU cost with no size benefit.
This holds for every JSON-RPC response through this path, regardless of size — the design intent (skip compressing small bodies) never activates for the traffic it was presumably added to protect; it only works for handlers elsewhere (e.g. the Cosmos API's swagger http.FileServer) that do set Content-Length before/without an explicit early WriteHeader.
Impact and severity. This is not a correctness bug — responses remain valid, complete gzip streams and clients decode them fine. The practical cost is wasted CPU per tiny request and ~20 bytes of gzip framing overhead on payloads that gain nothing from compression. Also, the writer pool uses gzip.NewWriterLevel(io.Discard, gzip.BestSpeed) (level 1), not the default level-6 that an earlier review comment assumed, so the actual CPU cost per byte is low — this meaningfully caps the amplification concern. Given that, this doesn't warrant blocking merge; it's a nit worth fixing so the safeguard actually does what its name implies.
How to fix. Determine hasLength/contentLength from the byte slice passed to the first Write() call when Content-Length hasn't been set explicitly (i.e., defer the compress/no-compress decision until the first Write, using len(b) as a stand-in for total size when the handler is known to write once), or have writeHTTPResponse/writeRPCResponse set Content-Length explicitly before calling WriteHeader (safe here since amir-deris already confirmed on the PR thread that every handler in this server marshals to a full byte slice and writes once, never streaming).
3 participants
Summary
NewGzipHandlermiddleware (sei-tendermint/rpc/jsonrpc/server/gzip.go) that wraps anyhttp.Handlerwith transparent gzip response compression using async.Pool-backed writer for allocation efficiencysei-tendermint/internal/rpc/core/env.go) and the Cosmos SDK API server (sei-cosmos/server/api/server.go)Accept-Encoding: gzipand for WebSocket upgrade requests (preservinghttp.Hijackercompatibility)Test plan
gzip_test.gocover compressed and uncompressed paths, pool reuse, and WebSocket passthroughmake buildAccept-Encoding: gzipis sent (e.g.curl -H "Accept-Encoding: gzip" http://localhost:26657/status | file -)/websocket) are unaffectedlink to design doc: Link
🤖 Generated with Claude Code