feat: implement shared compression writer pools

[rodrodros]

No description provided.

[github-actions]

Claude finished @rodrodros's task in 5m 31s —— View job

---

Review: shared compression writer pools

• Gather context (diff, call sites, git history)
• Review utils/compression/compression.go
• Review call-site integrations
• Review tests & benchmarks
• Post inline comments + summary

The package split and the per-level pool design are the right shape — levels genuinely must not share a pool, since gzip.Writer.Reset preserves the level a writer was built with, and the tests encode that reasoning well. But there's one blocker that makes the package unusable as it stands.

Blocker

GzipWriterLevel indexes the pool array by raw level instead of level - minLevel — utils/compression/compression.go:67. Index i holds level minLevel + i where minLevel = gzip.HuffmanOnly = -2, so DefaultCompression (-1) indexes [-1] and panics. That's the default path, so it hits every caller: jsonrpc/http.go:127 panics on any request sending Accept-Encoding: gzip, clients/gateway/gateway.go:149 on every body over gzipMinSize, and Gzip64Encode on the sn2core / rpc-v8 / rpc-v9 declare paths. rpc/v10/transaction.go:239 doesn't panic but silently drops from BestSpeed to DefaultCompression.

The poolIndex() helper that did this mapping — along with its bounds check — was removed in adae25e ("chore(compression): minor improvements"); the raw index replaced it.

The existing tests do catch this (TestGzip64, TestGzipWriterLevelsDoNotMix, TestGzipWriterLevelRejectsOutOfRange all fail), so this looks like it was pushed without a local test run. Juno Test was still in progress when I looked — I was not able to run go test ./utils/compression/... myself in this environment, so the failure above is from reading the code rather than from a run.

Important

• Double reset per acquire/release — compression.go:52-55. Release resets to io.Discard and GzipWriterLevel resets again onto dst. Each reset zeroes ~544 KiB of flate hash tables, so a pooled use clears ~1 MiB instead of ~544 KiB — on the per-response JSON-RPC path. The reference-dropping intent is sound (an idle pooled writer otherwise pins an http.ResponseWriter), but it can be achieved without the extra reset. Given the PR is a performance change, a BenchmarkGzipWriter number in the description would help — post-fix, since the current benchmark isn't measuring the levels it asks for.
• Unbounded decompression on untrusted input — compression.go:92-103. Pre-existing, carried over verbatim, but Gzip64Decode is reachable directly from user-submitted declare transactions via rpc/v8/class.go:70 and rpc/v9/class.go:76, and io.ReadAll has no ceiling. The move is a natural moment to cap it.

Nits

• Embedding *gzip.Writer re-exports Reset, letting callers repoint a pooled writer behind the pool's back; use-after-Release is silent while the less-dangerous double-Release panics (compression.go:40-43).
• gzipReader not closed on the io.ReadAll error path; bare error returns in Gzip64Decode where Gzip64Encode wraps.
• wrting → writing (compression.go:79); it's own state → its own state.
• TestGzipWriterLevelRejectsOutOfRange should assert the panic message, so it distinguishes a rejected level from an out-of-bounds index.

What's good

The call-site conversions are all correct — Release is deferred everywhere, Close precedes reads of the destination buffer in prepareRequestBody and ContractClassToGatewayPayload, and rpc/v10 correctly preserves its BestSpeed intent. The test file is unusually thoughtful: the non-monotonic sizes in TestGzip64EncodeAcrossSuccessiveCalls, the untainted gzipAtLevel oracle, the poisoned-destination reuse case, and the explicit premise guard at the end of TestGzipWriterLevelsDoNotMix all test things that would otherwise fail silently. I left inline comments on each item above.
· branch feature/compression-package

[github-actions]

Claude finished @rodrodros's task in 6m 20s —— View job

---

Re-review: shared compression writer pools

• Gather context (force-pushed diff, call sites)
• Re-check the blocker from the previous review
• Review the rest of utils/compression/compression.go
• Check the touched packages compile (go vet)
• Post inline comments + summary

The branch was force-pushed since my last pass (the previous head adae25e is gone; the current tip is 26f714e), so I re-reviewed from scratch. The blocker is fixed, along with most of the smaller items.

Resolved since last review

Item	Status
Pool indexed by raw level instead of level - minLevel	Fixed — compression.go:95
Embedded *gzip.Writer re-exporting Reset	Fixed — gz is unexported, only Write/Close/Flush forwarded
Use-after-Release silently succeeding	Fixed — ErrWriterNotAcquired guard + TestGzipWriterUseAfterReleaseErrors
wrting / it's own state typos	Fixed

go vet ./utils/compression/ ./jsonrpc/ ./rpc/v10/ ./clients/gateway/ ./core/ ./adapters/... ./rpc/v8/ ./rpc/v9/ is clean, so everything type-checks. I could not run go test in this environment (the command wasn't permitted), and Juno Test / golangci-lint were still in progress when I looked — the conclusions below are from reading the code. The three tests I previously said would fail all read as passing against the fixed indexing.

Important

• The double reset is avoidable, not a trade-off — compression.go:82. Your reply is fair as the code stands, but routing the gzip writer through a stable dstProxy owned by Writer gets you the instant dereference and a single flate reset per use — details inline. Also worth correcting one thing in the reasoning: it doesn't get cheaper at the faster levels. BestSpeed takes the same reset branch and zeroes the same ~640 KiB; only HuffmanOnly and NoCompression skip it.
• Unbounded decompression still open — compression.go:124. Pre-existing, so not a merge blocker for this PR, but I confirmed the path: rpc/v9/transaction.go:391 → rpc/v9/class.go:76 → here, on the raw body of a user-submitted declare. Request bodies are capped at 10 MiB by jsonrpc/http.go:112, which bounds it more than I implied last time — but ~7.5 MiB of gzip still expands to multiple GiB in one io.ReadAll, before validation, with no cap on concurrent declares. Same thread also covers the unclosed reader on the error path and the bare error returns.

Nits

• Out-of-range levels are now safe but panic with the runtime's index out of range rather than a domain message, and TestGzipWriterLevelRejectsOutOfRange only asserts that something panicked (compression.go:95).
• gzipWriterPools is a *[levelCount]sync.Pool where a plain array var indexes identically.

What's good

The Write/Close/Flush guard is a better answer than what I suggested last round — it closes the silent use-after-release hole without giving up the ergonomics, and ErrWriterNotAcquired being exported means callers can actually distinguish it. All ten call-site conversions are correct: Release is deferred everywhere, Close precedes the read of the destination in both prepareRequestBody and ContractClassToGatewayPayload (which copies via sierraBuf.String() before bufPool.Put runs, so no pooled buffer escapes), and rpc/v10 preserves its BestSpeed intent through the new API. The test file remains the strongest part of the PR — the non-monotonic sizes, the untainted gzipAtLevel oracle, the poisoned-destination reuse case, and the explicit premise guard at the end of TestGzipWriterLevelsDoNotMix all catch things that would otherwise fail silently.

Nothing here blocks merge once CI is green.
· branch feature/compression-package

[codecov]

Codecov Report

❌ Patch coverage is 77.10843% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.49%. Comparing base (654ca8e) to head (b892ba8).

Files with missing lines	Patch %	Lines
utils/compression/compression.go	75.36%	9 Missing and 8 partials ⚠️
rpc/v8/class.go	0.00%	1 Missing ⚠️
rpc/v9/class.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3956      +/-   ##
==========================================
- Coverage   74.65%   74.49%   -0.16%     
==========================================
  Files         462      462              
  Lines       40988    41029      +41     
==========================================
- Hits        30598    30566      -32     
- Misses       8291     8356      +65     
- Partials     2099     2107       +8     

Flag	Coverage Δ
jsonv2	77.36% <75.00%> (-0.05%)	⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 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.

[github-actions]

Claude finished @rodrodros's task in 7m 42s —— View job

---

Re-review: shared compression writer pools

• Gather context (new commit 67c7ad0, diff since last pass)
• Review the proxy change in utils/compression/compression.go
• Re-check open items from the previous review
• Review tests & benchmarks
• Build / vet / CI check
• Post inline comments + summary

Only 67c7ad0 is new since my last pass, and it touches compression.go plus its test. The proxy indirection is correct and does what it set out to do — nothing here blocks merge.

The proxy change

The design holds up. gz is wired once to &w.proxy in newWriter and never re-pointed at anything else, so the only reference to the caller's destination is w.proxy.dst, and Release drops it with a single store. The reset count per acquire/release cycle goes from two to one (GzipWriterLevel does the one gz.Reset that correctness requires anyway; Release no longer resets at all), and the reference is dropped earlier than before, not later. &w.proxy is an interior pointer to a heap object reached only through *Writer, so it stays stable across pool round-trips, and converting it to io.Writer doesn't allocate.

TestGzipWriterReleaseDropsDestination is a good addition — asserting the property with weak.Make and pinning gzipWriter via runtime.KeepAlive so the assertion is about the dst reference rather than the writer being collected is exactly right, and it's the kind of thing that would otherwise silently regress.

I could not run go test in this environment (the command wasn't permitted), so the conclusions above are from reading the code. go vet ./utils/compression/ ./jsonrpc/ ./rpc/v10/ ./clients/gateway/ ./core/ ./adapters/... ./rpc/v8/ ./rpc/v9/ ./utils/ is clean. Juno Test was still in progress when I looked.

CI

golangci-lint is red, but not on anything this PR touches — the six findings are in core/block.go (gci), builder/executor.go and jsonrpc/server.go (gofumpt), consensus/propeller/processor.go (gosec G404), core/trie2/triedb/hashdb/dirty_cache.go (staticcheck) and core/block_transaction_serializer.go (unparam). None of those files are in this PR's diff, so this looks like pre-existing breakage on main rather than something to fix here.

Nits (inline)

• The isAcquired guard is best-effort: it stops working the moment the writer is re-acquired by another goroutine, and w.pool is unsynchronized. Worth a comment so it isn't mistaken for a hard guarantee — compression.go:107.
• Gzip64Encode pools the writer but still allocates and grows a fresh bytes.Buffer per call, on the sync and declare paths — compression.go:128. Also a request for the benchmark numbers in the PR description.
• Writer is now self-referential (gz holds &w.proxy), so copying a Writer value would silently send output through the original's proxy. Only reachable if someone writes w2 := *w, which the *Writer-only API makes unlikely — mentioning it only because the proxy doc comment is the natural place to say "do not copy".

Previously raised, acknowledged by you

• Unbounded decompression in Gzip64Decode (plus the unclosed reader on the io.ReadAll error path and the bare error returns) — you're deferring to a follow-up. Agreed it's pre-existing and not this PR's job; flagging only so it doesn't get lost, since the reachable path is rpc/v9/transaction.go:391 → rpc/v9/class.go:76 → here on user-submitted declare bodies.
• Explicit range check on level — your call, dropping it. Out-of-range levels are safe either way now.

What's good

All ten call-site conversions remain correct, and the migration is complete — no utils.Gzip64*, gzip.NewWriter, or ad-hoc gzip pool survives outside the new package. Close still precedes the read of the destination in both prepareRequestBody and ContractClassToGatewayPayload, the defer order in ContractClassToGatewayPayload releases the writer before bufPool.Put, and rpc/v10 keeps its BestSpeed intent. The v10 conversion also quietly fixes a real leak in the old code: the previous defer gzPool.Put(gz) returned the writer still pointing at the base64 encoder and its buffer. The test file is still the strongest part of the PR.
· branch feature/compression-package

[github-actions]

Claude finished @rodrodros's task in 3m 15s —— View job

---

Re-review: shared compression writer pools

• Gather context (new head after force-push, diff since last pass)
• Review utils/compression/compression.go
• Re-check open items from the previous review
• Review call sites
• Build / vet / CI check
• Post summary

This push is a rebase, not a code change. The previous head 67c7ad0 is gone and the tip is now b892ba8, but git diff 67c7ad0..HEAD over the PR's own files is empty — utils/compression/compression.go, compression_test.go, compression_bench_test.go and all ten call sites are byte-for-byte what I reviewed last round. The only deltas are commits pulled in from main (the exhaustruct_v5 rename, the gci/gofumpt/gosec/staticcheck/unparam fixes, the bench/rpc corpus-gen work). No new inline comments this pass — nothing in this PR changed to comment on.

CI

golangci-lint is now green, which confirms the six findings I flagged last round were pre-existing breakage on main rather than anything this PR introduced — the rebase picked up their fixes. CodeQL, Dependency Review, Code Quality and Test Documentation Build all pass; Juno Test, Container Security and Deploy and Test on Dev were still in progress when I looked.

I could not run go test in this environment (the command wasn't permitted), so I can't independently confirm the suite — Juno Test in CI is the authority there. go vet ./utils/compression/ ./jsonrpc/ ./rpc/v10/ ./clients/gateway/ ./adapters/... ./rpc/v8/ ./rpc/v9/ ./core/ is clean against the rebased tree, so the rebase didn't break any of the touched packages.

Re-verified against the new base

Since the base moved I re-read the parts most likely to be disturbed by a rebase, and they're intact:

• clients/gateway/gateway.go:149-158 — Close still precedes the return &buf, and defer Release fires after.
• rpc/v10/transaction.go:234-252 — defer order is still Release (LIFO) before bufPool.Put, gz.Close() then b64.Close() precede the read of sierraBuf, and gzip.BestSpeed is still threaded through GzipWriterLevel.
• jsonrpc/http.go:124-135 — gw.Close() before gw.Release(), both in the deferred closure.
• compression.go:119 — level-minLevel indexing, proxy wired once in newWriter, Release doing a single w.proxy.dst = nil store.

Still open (both deferred by you, both agreed)

• Unbounded io.ReadAll in Gzip64Decode (compression.go:149) — pre-existing, follow-up. Reachable path is rpc/v9/transaction.go → rpc/v9/class.go:76 → here on user-submitted declare bodies; bounded to 10 MiB of input by jsonrpc/http.go, so ~GiB of output. Same thread covers the unclosed reader on the error path and the bare error returns.
• Gzip64Encode still allocates a fresh bytes.Buffer per call (compression.go:128) — follow-up.

Your calls on the isAcquired best-effort comment and the explicit level range check stand; neither affects correctness.

Verdict

Unchanged from last round: nothing here blocks merge once Juno Test goes green. The proxy indirection is correct, the per-level pooling is right, all ten call-site conversions are correct, and the test file remains the strongest part of the PR. The one thing still worth adding is the before/after BenchmarkGzipWriter / BenchmarkGzip64Encode numbers in the PR description, since the stated goal is a performance win and the description is currently empty.
· branch feature/compression-package