packfile: checksum index.pack record payloads and the app-data section by tamirms · Pull Request #910 · stellar/stellar-rpc

tamirms · 2026-07-31T10:13:50Z

What

index.pack is the only cold artifact whose payload bytes have no integrity coverage, and the consequence is worse than a missing check. roaring's UnmarshalBinary accepts a flipped container bit and returns a different posting set, so a corrupt index answers queries wrongly rather than failing.

The existing coverage stops short of it in every direction. The packfile trailer's CRC32C covers the 76-byte trailer. A multi-item record's tail CRC32C covers only its FOR-encoded item sizes, since decodeForIndex checksums the FOR region with the payload sitting before it and excluded. events.pack and the ledgers pack get payload coverage incidentally, from the content checksum in each zstd frame. index.pack is written in passthrough mode, so nothing supplies one.

Design

Integrity is an axis of its own rather than a codec. Whether a record carries a checksum is independent of which codec produced its bytes, and the Format value the codec is chosen by cannot express it. So instead of adding a codec that appends a checksum, WriterOptions.RecordChecksum widens the coverage of the CRC32C that is already the last four bytes of every multi-item record, and a trailer flag records which range it covers:

flag clear:  [payload][FOR sizes][1B W][4B min][4B crc over the FOR region]
flag set:    [payload][FOR sizes][1B W][4B min][4B crc over the whole record]

Same offset, same width. That buys four things:

index.pack pays no disk at all. The field already exists. Four new bytes are needed only for itemsPerRecord == 1 records, which have no FOR index, and index.pack is not that case.
No copy on read. Verification happens in place on the raw read buffer before anything parses it, so the passthrough path keeps aliasing its read buffer.
No reader-side configuration. The flag is on disk, so the reader is self-describing and there is no writer/reader pairing to get wrong. An older reader meeting a newer file fails loudly through the existing unknown-flags check rather than misreading it.
The compressed artifacts leave the flag clear and pay nothing. That matters most for full-history: hot-ingest p99 + memory campaign — packed rows, sorted-run tier, spill-and-merge cold build, zero-decompression freeze #902's freeze path, which copies zstd frames verbatim: an unconditional checksum would have added a pass over 1.28GB per pubnet chunk, 2.3s per stress chunk, to re-cover bytes those frames already cover.

It also settles an ordering wart. decodeForIndex ran intpack.DecodeGroup on unverified bytes and then located the CRC window using the length that parse reported. The widened range follows from the record bounds in the offsets index, which Open has already verified, so nothing the record claims about itself selects the bytes being checked.

Cost where it applies: one CRC32C pass at a measured 8.3GB/s, about 1.5% of the I/O that fetched the bytes, and 4.5ms per chunk on the write side.

App data

The same audit turned up a second gap. The offsets index carries its own CRC32C and the trailer's covers itself, but the app-data section had nothing, on the documented stance that callers own their own app-data integrity. No caller took it up, and the data is the kind where that matters: a flipped byte inside events.pack's cumulative LedgerOffsets blob can preserve monotonicity and the final total, pass every check in DecodeLedgerOffsets and loadMeta, and silently shift which ledger a query resolves to. The ledgers pack's four-byte firstSeq sits behind nothing but an overflow check.

The trailer's reserved bytes at 68:72 hold a crc32c(appData) now, verified at open. Zero size cost. An artifact built before this fails to open and gets rebuilt, which is fine for an unreleased format.

Error translation

stores/errors.go documents stores.ErrCorrupt as the sentinel each domain translates its backing primitive's corruption signal into, and the ledger store has a translateReaderErr doing exactly that. The event store had none, so an integrity failure surfaced wrapped in an events-prefixed message but invisible to errors.Is(err, stores.ErrCorrupt). That was survivable while index.pack reported nothing to translate. Now that its records and the app-data section are checked, the signal exists and needs somewhere to land.

Verification

Two /simplify passes and two independent code reviews, one of them adversarial against malformed and crafted input.

The evidence worth quoting: a byte-by-byte corruption sweep over every position of a checked file, at itemsPerRecord 1 and 4, with masks {0x01, 0xFF, 0x80}, produced zero silently-wrong reads and zero panics. The same sweep with ChecksumNone produced 42 silently-wrong reads out of 150 bytes, which is what makes the clean result meaningful rather than trivially green.

Also verified: the narrow flag-clear layout is byte-identical to what the pre-change writer produced, including the partial final record; the pooled record buffer still round-trips without a realloc in all four combinations of {itemsPerRecord 1, >1} x {ChecksumNone, ChecksumCRC32C}, including an expanding codec; and Verify() still matches with a content hash, an extract function, and the record checksum all enabled.

Two bugs the reviews found are fixed here. validateMPHF returned index.pack's open error raw, so corruption in one half of that artifact reached the house sentinel and corruption in the other half did not. And a crafted trailer claiming totalItems > 0 with itemsPerRecord == 0 slipped between two guards, one gated on recordCount > 0 and the other on itemsPerRecord > 0, and panicked on the first read. That one is pre-existing and reproduces on this branch's base.

Notes for review

No Format bump. The multi-item record layout is unchanged and the trailer flag is the discriminator, so a reader dispatching on Format still applies the right codec.
index.pack's checksum choice lives in cold_format.go beside its format ID and record size, not at the packfile.Create call site. It is part of that artifact's on-disk identity and every builder has to agree on it; the streaming builder on full-history: hot-ingest p99 + memory campaign — packed rows, sorted-run tier, spill-and-merge cold build, zero-decompression freeze #902's branch writes the same artifact and would otherwise have produced an unchecked one.
The cold reader rejects an index.pack built without the checksum. Serving unprotected bitmaps is the silent-wrong-answer case this exists to prevent, and validateMPHF already checks every other pairing invariant right there.
design-docs/packfile-library.md owned the format's integrity model and contradicted the code in six places, including two that recommended implementing integrity as a codec. Updated.

🤖 Generated with Claude Code

index.pack was the only cold artifact whose payload bytes had no integrity coverage, and the consequence is worse than a missing check: roaring's UnmarshalBinary accepts a flipped container bit and yields a DIFFERENT posting set, so a corrupt index answers queries wrongly rather than failing. events.pack and the ledgers pack are covered, but only incidentally, by the content checksum in each zstd frame. index.pack is written in passthrough mode, where nothing supplies one. The trailer CRC32C covers the trailer. A multi-item record's tail CRC32C covers only its FOR-encoded item sizes. Neither reaches the payload. Integrity is an axis of its own, not a codec: whether a record carries a checksum is independent of which codec produced its bytes, and the Format value the codec is chosen by cannot express it. So instead of adding a codec that appends a checksum, WriterOptions.RecordChecksum widens the coverage of the CRC32C that is already the last four bytes of every multi-item record, and a trailer flag records which range it covers: flag clear: [payload][FOR sizes][1B W][4B min][4B crc over the FOR region] flag set: [payload][FOR sizes][1B W][4B min][4B crc over the whole record] Same offset, same width. index.pack therefore pays no bytes at all, and the reader needs no configuration, because the flag is on disk: no writer/reader pairing to get wrong, and an older reader meeting a newer file fails loudly through the existing unknown-flags check rather than misreading it. The compressed artifacts leave the flag clear and pay nothing, which matters most on the freeze path, where copying ledger frames verbatim would otherwise have meant a pass over 1.28GB per pubnet chunk (2.3s per stress chunk) to re-cover bytes the frames already cover. Only single-item records, which have no FOR index to share, grow by four. Verification runs before anything parses the record. That also settles an ordering wart in the FOR path, which ran DecodeGroup on unverified bytes and then located the CRC window with the length that parse reported. The widened range follows from the record bounds in the offsets index, which Open has already verified, so nothing the record claims about itself selects the bytes being checked. Cost where it applies: one CRC32C pass at a measured 8.3GB/s, about 1.5% of the I/O that fetched the bytes, and no copy — the reader verifies in place and keeps aliasing its read buffer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

App data was the one tail section with no integrity coverage. The offsets index carries its own CRC32C and the trailer's covers itself, but the trailer CRC stops before the app-data bytes, and the library's stance was that callers are responsible for their own app-data integrity. No caller took it up, and the data is the kind where that matters: a flipped byte inside events.pack's cumulative ledger offsets can preserve monotonicity and the final total, passing every structural check its decoder makes, and silently shift which ledger a query resolves to. The ledgers pack's firstSeq is four bytes behind nothing but an overflow check. The trailer has four reserved bytes at offset 68, so this costs no space: put a CRC32C over the section there and verify it at open, where the bytes have just been read anyway. A flag bit says the field is populated, so a file written before it meant anything still opens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

stores/errors.go documents stores.ErrCorrupt as the per-domain integrity sentinel that stores are supposed to translate their backing primitive's corruption signal into, and the ledger store has a translateReaderErr doing exactly that. The event store had none, so a packfile integrity failure surfaced wrapped in an events-prefixed message but invisible to errors.Is(err, stores.ErrCorrupt). That was survivable while index.pack reported nothing to translate. Now that its records and the app-data section are checked, the signal exists and needs somewhere to land, so add the ledger store's translation and apply it wherever a packfile error leaves a public method: the two read paths, the range scan, and the metadata load. The two tests corrupt real artifacts and assert on the sentinel, which covers the whole chain: the writer's trailer flag, the reader verifying without being configured to, and the translation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Cleanups from a /simplify review of the three preceding commits. The on-disk record layout is untouched; the app-data checksum is no longer optional (below). packfile-library.md was the largest item. It owns the format's integrity model and it contradicted the code in six places: it still said app data had no packfile-level check and that the library never wraps payloads, still showed offset 68 as reserved with one flag bit, and in two places recommended implementing integrity as a codec — the pattern codec.go now tells callers not to use. A reader following it would have written a CRC-appending encoder and ended up with two checksums, one of which the library cannot see. The record section now carries both layouts and the ordering property as a table. flagAppDataCRC is gone; app data is verified unconditionally. The writer always set the flag, so the reader's guarded branch could never be false and its test had to hand-forge a trailer no writer emits. The only thing it bought was opening a file built before the field meant anything, and v2 is not live: rebuilding a chunk costs ~100s, while a permanently-true flag bit costs forever. Such a file now fails to open, and the error says it has to be rebuilt rather than implying corruption. Also: - sealRecord takes the payload and the FOR group rather than an assembled record plus the group's length, which both call sites were passing after doing the append themselves. The covered range is then named directly instead of recovered by slice arithmetic. - index.pack's checksum choice moves to cold_format.go beside its format ID and record size. It is part of the artifact's on-disk identity, and every builder has to agree on it; the streaming builder on the p99 branch writes the same artifact and would otherwise have silently produced an unchecked one. - The cold reader now rejects an index.pack built without the checksum. Serving unprotected bitmaps is the silent-wrong-answer case this exists to prevent, and validateMPHF already checks every other pairing invariant right there. - Comments: the "verified before parsing" argument was written four times, so it stays on verifyRecordCRC alone. packfile no longer explains an app-data checksum in terms of events.pack's ledger offsets, and the event store no longer asserts packfile's record tail layout to justify a cost figure. Tests: the RecordChecksum validation case joins TestCreateValidation's table, which also gains it an error-message assertion it did not have standalone; the app-data write sequence is one helper shared with TestAppDataRoundTrip instead of a third copy; a single-use closure factory and a trailer field parsed but never asserted are gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two independent review passes over the four preceding commits. No correctness bug was found in the record or app-data checksum itself; both reviewers reproduced clean failures across a byte-by-byte corruption sweep, crafted trailers, degenerate records, and the concurrent read paths. These are the real findings. validateMPHF returned index.pack's open error raw, so corruption in one half of that artifact reached the house sentinel and corruption in the other half did not: a flipped byte in a record surfaced as stores.ErrCorrupt through LookupKeys, while a flipped byte in the offsets index, the trailer, or the app-data section surfaced from the same call with no sentinel at all. A consumer keying rebuild off stores.ErrCorrupt, which stores/errors.go promises it can, would have handled one and silently mishandled the other. The stale-build rejection added in the review pass had the same shape and is now the sentinel too, for the same reason: the artifact cannot answer queries. A crafted trailer claiming totalItems > 0 with itemsPerRecord == 0 passed Open and then panicked on the first read, because the guard was gated on recordCount > 0 and the cross-validation on itemsPerRecord > 0, so a trailer setting neither slipped between them. Pre-existing, reproduced on the base branch, one condition to close. Tests: the narrow row of TestRecordChecksumVerifiesBeforeParsing asserted ErrCorrupt, which ErrChecksum wraps, so it passed whether the parser or the checksum rejected the record and could not pin the ordering it names. A new test covers a checksummed file that also carries app data, with an encoder whose output exceeds its input — the one case buildRecord's pre-size cannot absorb, so recordWorker reallocs before sealing. Another covers index.pack corruption at open, the path the sentinel gap was on. And the app-data corruption test regained the zero-size guard that inlining its closure dropped, without which a regression would flip the trailer magic instead and fail as ErrMagic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

tamirms · 2026-08-03T12:25:31Z

@codex[agent] review

Copilot

🟡 Not ready to approve

Ledger app-data checksum failures bypass the required stores.ErrCorrupt translation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds CRC32C integrity coverage for packfile records and app data, then applies it to the cold event index.

Changes:

Adds configurable record checksums and app-data validation.
Protects index.pack and translates event-store corruption errors.
Expands integrity documentation and regression tests.

File summaries

File	Description
`design-docs/packfile-library.md`	Documents checksum layouts and validation.
`stores/event/cold_reader.go`	Validates checksummed indexes and translates errors.
`stores/event/cold_reader_test.go`	Tests cold-artifact corruption handling.
`stores/event/cold_index.go`	Enables index record checksums.
`stores/event/cold_format.go`	Defines the index checksum policy.
`packfile/writer.go`	Writes record and app-data CRC32Cs.
`packfile/writer_test.go`	Tests checksum option validation.
`packfile/recordchecksum_test.go`	Tests checksum behavior and corruption detection.
`packfile/record.go`	Verifies record checksums before decoding.
`packfile/record_test.go`	Covers widened record decoding.
`packfile/reader.go`	Verifies app data and loads checksum metadata.
`packfile/reader_test.go`	Tests app-data corruption detection.
`packfile/packfile.go`	Extends trailer fields and flags.
`packfile/codec.go`	Clarifies codec versus integrity responsibilities.

Review details

Files reviewed: 14/14 changed files
Comments generated: 2
Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

+	tr, err := packfile.Open(indexPath, packfile.ReaderOptions{}).Trailer()
+	require.NoError(t, err)


+	if computed := crc32c(appData); computed != trailer.AppDataCRC {
+		return openResult{err: fmt.Errorf("%w: app data CRC32C (stored %08x, computed %08x)",
+			ErrChecksum, trailer.AppDataCRC, computed)}


chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b46f4108b2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Codex · 2026-08-03T12:33:31Z

@codex[agent] review

Reviewed current head b46f410 and didn’t find a code change to make. go test ./cmd/stellar-rpc/internal/rpcv2/packfile passed as part of the targeted run; stores/event could not build in this environment because rocksdb/c.h is missing.

chatgpt-codex-connector · 2026-08-03T12:33:40Z

To use Codex here, create a Codex account and connect to github.

The ledger cold reader translated packfile errors to the stores sentinels on its record reads but not in loadHeader, which reaches the packfile through Trailer and AppData. Corruption there surfaced as a raw packfile.ErrCorrupt, so LastSeq, GetLedgerRaw and IterateLedgers all failed without matching stores.ErrCorrupt, against the contract in stores/errors.go that names the trailer explicitly. The gap predates the app-data checksum: magic, version, size and trailer CRC failures all took the same untranslated path. Covering app data with a CRC32C only added another way to reach it, which is how it was noticed. loadHeader has six return points and every one of them reaches callers through the sync.OnceValues init, so the translation goes there rather than at each return. A return added later is covered without anyone remembering, and it costs nothing: init runs once per reader. The helper itself was duplicated byte for byte in the ledger and event stores, which is what let the two drift apart in the first place. It is now one stores.TranslatePackErr. Considered and rejected: having packfile.ErrCorrupt wrap the store sentinel, which would need no call sites at all but contradicts the library's caller-agnostic design and would cover only one of the sentinel's sources — the hot store and the index validator construct it directly. A translating reader wrapper was also tried and measured: it costs 2.77ns per element on ReadRange (1.9ns of interposition, 0.9ns of translation), all of it on the success path, to guard an error that is never hit on the path that pays. The regression test drives all three affected methods over a pack whose app data has one flipped bit, and fails on all three without the change. Also closes a packfile reader the index-offsets corruption test opened inline and left open for the rest of the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Not ready to approve

Cold-reader Close paths can still expose raw packfile corruption errors instead of the store sentinel.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

cmd/stellar-rpc/internal/rpcv2/stores/translate.go:13

packfile.Reader.Close waits for the asynchronous open and can therefore be the first place an app-data checksum failure surfaces. Neither the event nor ledger ColdReader.Close passes that error through this helper, so a close-only path returns raw packfile.ErrCorrupt instead of stores.ErrCorrupt, contrary to the public-boundary contract stated here and in stores/errors.go:3-7. Translate the packfile close results as well and add a close-only corruption test.

// Every store reading a packfile applies it at its public-method boundaries;
// it lives here rather than in each store so the two cannot drift.

Files reviewed: 17/17 changed files
Comments generated: 0 new
Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

packfile.Reader.Close joins the deferred open error into its result, so on a reader that is opened and closed without a read, Close is the first place an open-time failure surfaces — and the only one, since nothing else ran. Both cold readers returned that straight through, so a close-only path reported raw packfile corruption instead of the store sentinel. Close is a public method, so it owes callers the same sentinel as every other. The tests cover the close-only shape specifically, since a reader that has already been read gets its error from the read instead and would hide this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The two preceding commits fixed the two places that forgot to translate a packfile error, one found by review after the other. Two independent misses in one review cycle is not forgetfulness, it is what opt-in translation costs: every call site is a chance to skip it, and skipping it is silent. So the stores no longer hold a *packfile.Reader. They hold a stores.PackReader whose methods translate, which makes the L2 boundary a property of the handle rather than something each caller remembers. There is no way to obtain an untranslated error from it, and a method added later has to translate to compile — which already earned its keep: sizing the type from the existing call sites missed ReadItems, and the compiler caught it where a reviewer would otherwise have had to. This was measured and rejected earlier in review on the strength of a bad benchmark. Wrapping the ReadRange iterator costs ~3ns an element, which was compared against bare packfile iteration at ~16ns an element and read as +17%. The real scan decodes each frame and costs ~470ns an element, so the true figure is near 1% — and it buys the two gaps above plus the next one. The earlier number measured a component and drew a conclusion about the system. Both loadHeader's chokepoint and the Close translation the preceding commits added are now redundant and go away, along with the standalone helper. The regression tests are unchanged: they assert the behaviour, not the mechanism, and all four still fail if the translation is broken. What this does not cover is unchanged. The sentinel has producers that never touch a packfile — the hot store's decode failures and the index validator construct it directly — and rocksdb-backed stores translate their own. This closes the packfile boundary, which is where both defects were. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Human review recommended

The on-disk format and corruption semantics span core packfile logic and multiple cold-store consumers, warranting final human validation.

Review details

Files reviewed: 17/17 changed files
Comments generated: 0 new
Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

tamirms and others added 5 commits July 30, 2026 22:43

tamirms marked this pull request as ready for review July 31, 2026 13:36

tamirms requested review from a team and Copilot July 31, 2026 13:39

Copilot started reviewing on behalf of tamirms August 3, 2026 12:25 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 3, 2026

View reviewed changes

Comment thread cmd/stellar-rpc/internal/rpcv2/packfile/reader.go

Codex started work on behalf of tamirms August 3, 2026 12:29 View session

Codex finished work on behalf of tamirms August 3, 2026 12:34

tamirms requested a review from Copilot August 3, 2026 13:21

Copilot started reviewing on behalf of tamirms August 3, 2026 13:22 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

tamirms and others added 2 commits August 3, 2026 15:37

tamirms requested a review from Copilot August 3, 2026 13:51

Copilot started reviewing on behalf of tamirms August 3, 2026 13:52 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

tamirms added the rpc-sdk-scrum label Aug 4, 2026

tamirms added this to Platform Scrum Aug 4, 2026

github-project-automation Bot moved this to To Do in Platform Scrum Aug 4, 2026

tamirms moved this from To Do to Needs Review in Platform Scrum Aug 4, 2026

tamirms added this to the platform sprint 74 milestone Aug 4, 2026

Navigation Menu

Uh oh!

packfile: checksum index.pack record payloads and the app-data section - #910

packfile: checksum index.pack record payloads and the app-data section#910
tamirms wants to merge 8 commits into
feature/full-historyfrom
indexpack-record-checksum

tamirms commented Jul 31, 2026

Uh oh!

tamirms commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Codex AI commented Aug 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		tr, err := packfile.Open(indexPath, packfile.ReaderOptions{}).Trailer()
		require.NoError(t, err)

Uh oh!

Conversation

tamirms commented Jul 31, 2026

What

Design

App data

Error translation

Verification

Notes for review

Uh oh!

tamirms commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Codex AI commented Aug 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Human review recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants