Skip to content

Add thin CGo zstd wrapper for packfile compression - #650

Merged
tamirms merged 1 commit into
feature/full-historyfrom
add-zstd-wrapper
Apr 2, 2026
Merged

Add thin CGo zstd wrapper for packfile compression#650
tamirms merged 1 commit into
feature/full-historyfrom
add-zstd-wrapper

Conversation

@tamirms

@tamirms tamirms commented Apr 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Second in a series of PRs checking in the packfile implementation for full history RPC v2. See #633 for the design doc and #649 for the first PR (FOR encoding + offset index).

This PR adds a thin CGo wrapper around system libzstd (>= 1.5.7) for compression and decompression, and updates build infrastructure to provide the required library version.

Why a custom wrapper instead of existing Go bindings? (see zstd.go package doc for details)

  • klauspost/compress (pure Go): ~4x slower than system libzstd on our 28KB blocks at level 3
  • DataDog/zstd: vendored C symbols cause ELF symbol interposition with RocksDB on Linux, making RocksDB 5x slower

Why 1.5.7? Benchmarked on representative production data (28KB blocks of 128 events):

libzstd 1.5.5 libzstd 1.5.7 Change
Compress 717 MB/s 766 MB/s +6.9% (p=0.000, n=10)
Decompress 3.43 GB/s 3.65 GB/s +6.3% (p=0.000, n=10)

Linking strategy:

  • Linux: statically linked via #cgo linux LDFLAGS: -l:libzstd.a — the binary carries libzstd 1.5.7 baked in, no runtime dependency. This applies to Docker builds, CI, and the Jenkins .deb package pipeline (which uses our Dockerfile).
  • macOS: dynamically linked via pkg-config/homebrew for local development.

Build infrastructure changes:

  • Dockerfile: switch build stage from golang:1.25-bookworm to golang:1.25-trixie (Debian 13 ships libzstd 1.5.7 in default repos), add libzstd-dev and pkg-config
  • CI: build libzstd 1.5.7 from source in the setup-go action on Linux (with SHA256 verification); install via brew on macOS

Test plan

  • Roundtrip compression/decompression
  • Corrupt data detection (truncated, header-corrupted, bit-flipped)
  • Empty/nil input handling (no SIGSEGV on nil C pointer)
  • Context reuse across multiple calls with varying payloads
  • Scratch buffer aliasing contract
  • dst buffer reuse (no unnecessary allocation)
  • Close idempotency (no double-free)
  • Checksum detection of single-bit corruption
  • WithoutChecksum option
  • Concurrent independent instances
  • Encode/Decode after Close returns error

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings April 1, 2026 22:25
@tamirms
tamirms changed the base branch from main to feature/full-history April 1, 2026 22:28
@tamirms
tamirms force-pushed the add-zstd-wrapper branch from 5e66034 to c188fb0 Compare April 1, 2026 22:30
Copilot AI reviewed Apr 1, 2026
View reviewed changes

Copilot AI 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.

Pull request overview

Adds an internal CGO-based zstd wrapper (backed by system libzstd) to support upcoming packfile compression work, along with CI/Docker changes to ensure an appropriate libzstd is available.

Changes:

  • Introduces cmd/stellar-rpc/internal/zstd with reusable compressor/decompressor contexts plus convenience Encode/Decode helpers.
  • Adds unit tests covering roundtrips, corruption handling, buffer reuse/aliasing, checksum behavior, and close semantics.
  • Updates build infrastructure (Docker + GitHub composite action) to provide libzstd >= 1.5.7 and pkg-config.

Reviewed changes

Copilot reviewed 82 out of 85 changed files in this pull request and generated 6 comments.

File Description
cmd/stellar-rpc/internal/zstd/zstd.go New thin CGO wrapper over system libzstd with reusable contexts and options.
cmd/stellar-rpc/internal/zstd/zstd_test.go New test suite exercising correctness and safety contracts.
cmd/stellar-rpc/docker/Dockerfile Moves builder image to Debian trixie; installs libzstd-dev + pkg-config.
.github/actions/setup-go/action.yml Installs libzstd (1.5.7) in CI before setting up Go.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/stellar-rpc/internal/zstd/zstd.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread .github/actions/setup-go/action.yml Outdated
Comment thread .github/actions/setup-go/action.yml Outdated
Comment thread cmd/stellar-rpc/docker/Dockerfile
@tamirms
tamirms force-pushed the add-zstd-wrapper branch 4 times, most recently from 96dc9cc to 7f93516 Compare April 1, 2026 22:48
@tamirms
tamirms requested a review from Copilot April 1, 2026 22:50
@tamirms
tamirms force-pushed the add-zstd-wrapper branch 2 times, most recently from f920751 to 9ec5045 Compare April 1, 2026 22:54
Copilot AI reviewed Apr 1, 2026
View reviewed changes

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd_test.go
Comment thread cmd/stellar-rpc/internal/zstd/zstd.go Outdated
Comment thread cmd/stellar-rpc/internal/zstd/zstd.go
Comment thread cmd/stellar-rpc/docker/Dockerfile
@tamirms
tamirms force-pushed the add-zstd-wrapper branch 7 times, most recently from c0128cd to d2e7954 Compare April 1, 2026 23:29
@tamirms
tamirms requested a review from a team April 1, 2026 23:37
Shaptic
Shaptic reviewed Apr 1, 2026
View reviewed changes
Comment thread .github/actions/setup-go/action.yml Outdated
Shaptic
Shaptic approved these changes Apr 2, 2026
View reviewed changes

@Shaptic Shaptic left a comment

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.

LGTM pending one CI comment

Shaptic
Shaptic reviewed Apr 2, 2026
View reviewed changes
Comment thread .github/actions/setup-go/action.yml Outdated
karthikiyer56
karthikiyer56 reviewed Apr 2, 2026
View reviewed changes
Comment thread cmd/stellar-rpc/internal/zstd/zstd.go
@@ -0,0 +1,253 @@
// Package zstd provides compression and decompression using system libzstd

@karthikiyer56 karthikiyer56 Apr 2, 2026

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.

can these files be moved form under cmd/stellar-rpc/internal to something like a top level?
under pkg/zstd ?
atleast for the time being till such time that the new rpc binary is being designed
once all is done, we can converge on what the final step looks like.

if you put it under cmd/stellar-rpc/internal, I cannot use it from under full-history/ (which is where I am doing the current backfill implementation)

@tamirms
tamirms force-pushed the add-zstd-wrapper branch 4 times, most recently from 3ae1e8f to f3163f4 Compare April 2, 2026 08:51
@tamirms
tamirms force-pushed the add-zstd-wrapper branch from f3163f4 to 707c97f Compare April 2, 2026 09:36
@tamirms @claude
Add thin CGo zstd wrapper for packfile compression
8b35b5a 
Second in a series of PRs checking in the packfile implementation for
full history RPC v2. This PR adds a thin CGo wrapper around system
libzstd (>= 1.5.7) for compression and decompression.

- Compressor: reusable context with configurable content checksums
- Decompressor: reusable context with buffer reuse
- Convenience Encode/Decode functions for one-off use

Also updates build infrastructure to provide libzstd >= 1.5.7:
- Dockerfile: switch build stage from bookworm to trixie (Debian 13)
  which ships libzstd 1.5.7 in its default repos
- CI: build libzstd 1.5.7 from source in the setup-go action (Linux),
  or install via brew (macOS)

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@tamirms
tamirms force-pushed the add-zstd-wrapper branch from 707c97f to 8b35b5a Compare April 2, 2026 09:46
@tamirms
tamirms merged commit 229bcd8 into feature/full-history Apr 2, 2026
14 checks passed
@tamirms
tamirms deleted the add-zstd-wrapper branch April 2, 2026 16:52
@tamirms tamirms mentioned this pull request Apr 23, 2026
Merged
8 tasks
This was referenced May 5, 2026
Closed
Closed
Closed
Closed
@karthikiyer56 karthikiyer56 linked an issue May 5, 2026 that may be closed by this pull request
Packfile 2: Zstd CGo wrapper for packfile compression #716
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@Shaptic Shaptic Shaptic approved these changes
@karthikiyer56 karthikiyer56 karthikiyer56 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Packfile 2: Zstd CGo wrapper for packfile compression

4 participants

@tamirms @Shaptic @karthikiyer56