Stamp grant digest state with its hash ABI version and enforce it at Open - #1121
Stamp grant digest state with its hash ABI version and enforce it at Open#1121mj-palanker wants to merge 2 commits into
Conversation
…Open GrantDigestABIVersion names the framing of the grant content hash and principal bucket hash, but nothing on disk recorded which version a file's digest nodes and hash-index rows were computed under. A future bump would have had no way to tell current state from state built by older hash code, short of an index migration, which old binaries can re-pollute after it runs. Add a durable ABI stamp (rawdb.GrantDigestABIStampKey, uint32 BE) inside the digest keyspace under a reserved meta index id, so every wholesale destroyer of digest state, including those in already shipped SDKs, erases it without knowing it exists. Every global-root write site writes the stamp first. Open reads it after the presence probe: a writable open drops all digest state whose stamp does not name the current version, and the next EndSync rebuilds from scratch; a read-only open cannot drop, so it sets grantDigestAbiStale and the digest root getters report "never built". A missing stamp over present digest nodes reads as version 1: every SDK build that predates the stamp hashed at that version, so introducing the stamp costs existing files no rebuild while the constant stays at 1. The presence probe now spans only the node keyspace, so a leftover stamp over an empty file arms nothing. The constant stays at 1 in this change. It exists to make a later bump safe on its own, and to let a rollback across that bump re-stamp at the older version on its next repair, so the newer binary detects the mixed state instead of trusting it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s a base lacks Two gaps in how the digest ABI stamp's consequences reach consumers. The read-only stale flag (and the pre-existing build-pending flag) guarded the two digest root getters but not ComputeEntitlementBucketDigest, which folds hash-index rows directly. On a read-only open of a file stamped with a different ABI it would have returned digests derived from another hash scheme's content hashes. Fold both flags into one grantDigestStateUntrusted helper and consult it at all three entry points. The bucket scan stays deliberately ungated: bucket membership is the principal bucket hash over the encoded principal identity, frozen with the v3 key encoding and unchanged by any ABI bump, so a stale file's bucket placement is still exact. The fold compactor left a base's copied digest state alone whenever the fold wrote no grants. That is right when the state is present, but the dest's writable Open may have just dropped it (a stale stamp), or the base may never have had any (digest index disabled at seal, or an earlier build failure). Such a fold shipped an output with no digest state and nothing downstream to repair it. When the digest index is enabled and no state is present, run the repair, which delegates to the full build: a one-time O(base) cost the first time a digest-less base is folded, after which the output is present and stamped and later folds pay nothing extra. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Deliberately NOT gated by grantDigestStateUntrusted, unlike | ||
| // getPartitionDigestRoot / GetGrantDigestGlobalRoot / | ||
| // ComputeEntitlementBucketDigest: a bucket's MEMBERSHIP is the | ||
| // principal bucket hash over the encoded principal identity, frozen by | ||
| // the v3 key encoding and untouched by any GrantDigestABIVersion bump | ||
| // (only the stored CONTENT hashes and digest nodes are ABI-dependent — | ||
| // see grantContentHash64 vs grantPrincipalBucketHash64). So a stale | ||
| // file's bucket placement is still exact, and this keeps yielding the | ||
| // grants a caller already knows to be dirty even on a read-only open | ||
| // over a stale ABI stamp. |
There was a problem hiding this comment.
🟡 Suggestion: this rationale contradicts GrantDigestABIVersion's own definition. That constant is documented as "the version of the content-hash / bucket-hash definitions below (grantContentHash64, grantPrincipalBucketHash64)" and PrincipalBucketHash's comment (edited in this PR, line ~258) says its "input framing changes only under a GrantDigestABIVersion bump" — so a bump can change bucket placement, and then the stored index keys' bucket bytes are from the old scheme while bucketBounds derives the range from the caller's new-scheme bucket. On a stale read-only open this yields a silently wrong grant subset with no error. Either narrow the ungating to "bumps that only change the content hash" (and say a bucket-hash bump must gate this too), or gate on grantDigestAbiStale and rely on the root getters reporting not-built. Medium confidence; no impact while the constant is 1.
| // Enforcement on the stored state itself is the durable ABI stamp | ||
| // (rawdb.GrantDigestABIStampKey), written alongside every global-root | ||
| // write and checked once per Open (verifyGrantDigestABI): digest state | ||
| // whose stamp does not name this constant is dropped (writable open) | ||
| // or reported "never built" (read-only open), so a bump here is | ||
| // sufficient by itself to force every previously-sealed file's digest | ||
| // state to be rebuilt in full at the new ABI on its next writable use. |
There was a problem hiding this comment.
🟡 Suggestion: "a bump here is sufficient by itself" holds only against stamp-aware binaries. Already-shipped pre-stamp SDKs never write GrantDigestABIStampKey, and the paths they use to mutate digest state (stageGrantDigestInvalidation / InvalidateGrantDigestPartitions + recomputeGrantDigestGlobalRootLocked) delete only the touched partitions and the global root — the stamp key survives. So after a bump to 2: a v2 binary stamps a file at 2, an older pre-stamp SDK opens it writable and rewrites some partitions at ABI 1 while leaving stamp == 2, and the next v2 binary trusts mixed-ABI state. That is the same "old binaries re-pollute after the fact" failure the PR uses to argue against an index migration. Worth stating the residual limitation here (and in index_migrations.go), or encoding the ABI into the digest index/key prefix so old and new state cannot share a keyspace. Medium confidence; no impact today.
| case destEng.GrantDigestIndexEnabled() && !destEng.GrantDigestsPresent(): | ||
| if err := destEng.RepairMissingGrantDigests(ctx); err != nil { | ||
| return "", fmt.Errorf("compactPebbleFold: build grant digests for a base with none: %w", err) | ||
| } | ||
| l.Info("compactPebbleFold: no grant writes, but the base carried no grant digest state; built it") |
There was a problem hiding this comment.
🟡 Suggestion: the "one-time O(base) cost" claim depends on the build succeeding. RepairMissingGrantDigests downgrades any non-cancellation failure to a log + dropAllGrantDigestStateLocked and returns nil, so a base whose digest build keeps failing leaves GrantDigestsPresent() false and pays the full O(base) grant scan + spill sort + SST ingest on every subsequent fold, silently. Consider narrowing the wording, and — per docs/BUG_CATCHING.md §2's cost-contract rule for compaction paths — adding a benchmark that pins the new digest-less-base fold cost curve; this PR states the delta but nothing enforces it.
| // itself: it must pass right after a clean seal, and it must detect a | ||
| // deliberately tampered hash-index row (proving it is a real oracle, | ||
| // not a tautology) — the same helper TestGrantDigestABIStaleStampDroppedAtWritableOpen | ||
| // and TestGrantDigestABIMissingStampTreatedAsStale rely on to certify a |
There was a problem hiding this comment.
🟡 Suggestion: TestGrantDigestABIMissingStampTreatedAsStale does not exist — the test in this file is TestGrantDigestABIMissingStampReadsAsVersion1. Update the reference so the doc chain stays navigable.
General PR Review: Stamp grant digest state with its hash ABI version and enforce it at OpenBlocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness across all 12 changed files, and traced the new durable ABI stamp through every digest write/drop/probe/copy site in Risk triage per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes persistent on-disk digest state semantics and Open-time enforcement in a core storage engine path, warranting final human review despite strong test coverage.
Pull request overview
Adds durable, on-disk bookkeeping for the grant-digest hash ABI version so future GrantDigestABIVersion bumps can be enforced safely at engine Open, and ensures downstream consumers/compactors treat stale or half-built digest state as untrusted.
Changes:
- Introduces a durable digest-ABI stamp key in the digest keyspace, excludes it from the “digests present” probe, and enforces it at
Open(drop on writable open; “never built” behavior on read-only open). - Unifies “untrusted digest state” handling (ABI-stale + build-pending) so root getters and bucket digest folding won’t trust stale/half-built hash-index content.
- Updates Pebble fold compaction to rebuild digests when the destination wants digests but the byte-copied base carries none; adds targeted tests covering the new behavior and ABI-stamp enforcement.
File summaries
| File | Description |
|---|---|
| pkg/synccompactor/compactor_pebble.go | Ensures fold compaction rebuilds digests when the destination has digest index enabled but no digest state is present. |
| pkg/synccompactor/compactor_grant_digest_test.go | Adds a regression test for “no-grant-write fold” rebuilding digests when the base carries none. |
| pkg/dotc1z/engine/pebble/internal/rawdb/rawdb.go | Updates digest presence probe to scan only digest node keys (excluding metadata like the ABI stamp). |
| pkg/dotc1z/engine/pebble/internal/rawdb/keyspace.go | Adds reserved digest metadata index ID, ABI stamp key, and node-only digest keyspace bounds helper. |
| pkg/dotc1z/engine/pebble/index_migrations.go | Documents why digest ABI bumps are enforced via stamps (not one-time index migrations). |
| pkg/dotc1z/engine/pebble/grant_digest.go | Implements ABI stamp read/write helpers, Open-time ABI verification, and gates digest getters/folding on untrusted-state flags. |
| pkg/dotc1z/engine/pebble/grant_digest_repair.go | Writes the ABI stamp alongside global-root writes during repair, ensuring “root implies certified ABI”. |
| pkg/dotc1z/engine/pebble/grant_digest_build.go | Writes the ABI stamp in full/zero-grant build completion paths and documents durability ordering. |
| pkg/dotc1z/engine/pebble/grant_digest_abi_test.go | Adds comprehensive tests covering stamping, enforcement, stale read-only behavior, and an oracle verifier. |
| pkg/dotc1z/engine/pebble/engine.go | Calls ABI verification during Open, exports GrantDigestsPresent, and centralizes “digest state untrusted” logic. |
| pkg/dotc1z/engine/pebble/digest.go | Switches root trust gating to the unified grantDigestStateUntrusted() helper. |
| pkg/dotc1z/engine/pebble/digest_test.go | Adjusts digest node counting to use node-only bounds (excluding ABI stamp structurally). |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
GrantDigestABIVersionnames the framing of the grant content hash and the principal bucket hash, but nothing on disk recorded which version a file's digest nodes and hash-index rows were computed under. A future bump would have had no way to tell current state from state built by older hash code, short of an index migration, which older binaries can re-pollute after it has run once. This PR adds the durable bookkeeping needed to make such a bump safe on its own, and closes two gaps in how its consequences reach consumers. The constant stays at 1; no hash changes here.Changes
ABI stamp (
pkg/dotc1z/engine/pebble)rawdb.GrantDigestABIStampKey, uint32 BE) lives inside the digest keyspace under a reserved meta index id. Every wholesale destroyer of digest state, including those in already-shipped SDKs, erases it without knowing it exists. The presence probe now spans only the node keyspace, so a leftover stamp over an empty file arms nothing.EndSyncrebuilds from scratch through the existing digests-absent path. A read-only open cannot drop, so it setsgrantDigestAbiStaleand the digest root getters report "never built", the same fail-safe shape as the interrupted-build marker.Consumers of the untrusted-state flags
grantDigestStateUntrustedhelper, consulted by both root getters and byComputeEntitlementBucketDigest, which previously folded hash-index rows with no check and would have returned digests derived from another hash scheme's content on a read-only open of a stale file.Fold compaction (
pkg/synccompactor)Rollout behavior
Docs
Three older comments that said a hash framing change "requires an index-migration bump" now point at
GrantDigestABIVersion;index_migrations.goexplains why digest ABI bumps do not belong there.Tests
TestGrantDigestABI*ingrant_digest_abi_test.go: stamp written by seal and excluded from the presence probe; stale stamp dropped on writable open and rebuilt at the current version; missing stamp reads as version 1 and is kept, then stamped on the first root rewrite; read-only open sets the stale flag and root getters report not-built; orphan stamp over an empty keyspace is ignored; stale stamp plus interrupted-build marker together; bucket digest gated while bucket scan still yields every grant; plus an oracle test proving the hash-index verifier can detect tampering.TestCompactPebbleFoldRebuildsDigestsWhenBaseHasNone: a digest-less base folded with a partial that writes no grants ends with a present global root byte-equal to both the original base's root and a from-scratch oracle build.Reviewer note
The fold change is a compactor behavior change: the O(base) build now fires on the first fold of any digest-less base, not only after an ABI bump, including a base sealed with the digest index disabled and then folded by an engine with it enabled.
🤖 Generated with Claude Code