Add Hasher.Clone and binary state marshaling - #13
Open
AskAlexSharov wants to merge 12 commits into
Open
Conversation
Clone enables the 'absorb a shared prefix once, fork per suffix'
pattern common in trie workloads, and gives portable copy semantics:
plain struct copies of Hasher work on asm builds (value semantics)
but silently alias shared state on fallback builds, where the state
is held behind a pointer.
MarshalBinary/AppendBinary/UnmarshalBinary allow persisting and
resuming hash state. The native sponge uses a compact versioned
encoding (magic 'fk1n') that marshals only the live portion of the
input buffer; the x/crypto fallback delegates to sha3's own marshaler
under a distinct magic ('fk1x'), so restoring state on a build with a
different implementation fails with a descriptive error instead of
corrupting.
Also make padAndSqueeze reset absorbed: it previously stayed at its
pre-squeeze value, which was harmless for hashing (Write/Sum panic
once squeezing) but leaked stale buffered input into the marshaled
encoding.
There was a problem hiding this comment.
Pull request overview
Adds snapshotting capabilities to the keccak.Hasher so callers can (1) fork hashers via a deep Clone() and (2) persist/restore in-progress sponge state via Go’s standard binary marshaling interfaces, with separate encodings (and explicit errors) for the native sponge vs. the x/crypto fallback to avoid silent corruption.
Changes:
- Add
(*Hasher).Clone() (*Hasher, error)to safely fork hashers without shared-state aliasing on fallback builds. - Implement
encoding.BinaryMarshaler,encoding.BinaryAppender, andencoding.BinaryUnmarshalerforHasher, including strict magic-based implementation discrimination. - Fix native sponge
padAndSqueezeto resetabsorbed(prevents stale buffered bytes from being included in marshaled state).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| keccak.go | Adds binary marshaling helpers, format magics, and compile-time interface assertions. |
| keccak_default.go | Implements Clone and binary marshal/unmarshal for the pure x/crypto-backed hasher. |
| keccak_asm.go | Implements Clone and native/x-crypto marshaling logic; fixes padAndSqueeze absorbed reset. |
| keccak_marshal_test.go | Adds tests for clone independence and marshal/unmarshal round-trips and error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review (3 independent angles, one reproduced under Rosetta) showed the dual fk1n/fk1x format was selected by runtime CPU detection, not build: the same amd64 binary emits fk1n on a BMI2 host and fk1x on a non-BMI2 host and refuses to restore the other's blobs — persisted states were CPU-feature-locked, and the docs misstated this as a build property. Replace both formats with one canonical 206-byte encoding (magic || state[200] || flags || pos) with partial input always XORed into the state, which is exactly x/crypto/sha3's internal convention, so the fallback translates losslessly in both directions via its stable marshal layout. Restoring works everywhere regardless of which implementation wrote the state. pos == rate is accepted while squeezing (x/crypto permutes lazily at block boundaries); the native decoder normalizes it by permuting eagerly. Also from review: - AppendBinary no longer lazily initializes h.xc (a nominally read-only snapshot mutated the receiver); the zero state is encoded directly with no allocation. - MarshalBinary pre-sizes its buffer (1 alloc); appendState uses slices.Grow. - Absorbing states can no longer smuggle a nonzero read index (the single pos field expresses one offset). - cloneXC round-trips x/crypto's own encoding directly instead of wrapping and stripping a magic prefix. - Tests: golden fingerprint test pins the wire format and proves cross-implementation byte equality (runs on native and purego in CI); error cases cover flag/pos/length tampering; test helper replaced with slices.Concat.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 10 changed files in this pull request and generated 6 comments.
Files not reviewed (5)
- .idea/.gitignore: Generated file
- .idea/fastkeccak.iml: Generated file
- .idea/go.imports.xml: Generated file
- .idea/modules.xml: Generated file
- .idea/vcs.xml: Generated file
new([200]byte) was already non-escaping here (appendState doesn't retain its argument), but the explicit form makes the no-allocation claim self-evident.
The hashing methods can only panic if x/crypto's hasher stopped satisfying KeccakState (their interface signatures allow no error), but xcFromState/cloneXC have error returns — degrade to errXCFormat there instead of panicking on a hypothetical dependency change.
useASM is a runtime CPU decision, so the x/crypto fallback arms in keccak_asm.go (taken on amd64 without BMI2 or arm64 without SHA3 — old Xeons, Graviton 2) are compiled into every binary but were never executed by any CI configuration: purego builds compile keccak_default.go, which is different code, and native builds take the asm arms. A regression there would have kept every test green. forceFallback flips useASM (restored via t.Cleanup) after capturing expected digests and squeeze streams from the active implementation, then re-runs the full surface through the fallback: streaming writes, non-destructive Sum/Sum256, extended Read, Reset, Clone, marshal round-trips mid-absorb and mid-squeeze (including the pos == rate lazy block-boundary state only the fallback produces), and zero-value marshaling. TestCrossImplementationMarshal additionally proves the canonical format's portability in-process, both directions (native encode -> fallback decode and vice versa), and that both implementations emit byte-identical encodings for the same logical state — the scenario that motivated the single-format design. Native branch coverage: 54% -> 90% of statements; what remains is defensive error branches unreachable with the pinned dependency.
staticcheck QF1008 flagged the redundant embedded-field selectors (h.sponge.state and friends). Assembling a local sponge and storing it once also removes the window where the receiver holds a partially restored state.
AskAlexSharov
force-pushed
the
feat/clone-marshal
branch
from
July 20, 2026 08:16
24dc296 to
ff5e613
Compare
xcAppendState dropped the caller's prefix by returning nil on its error paths, violating the encoding.BinaryAppender contract (on error, return b unchanged). It is the error path of both Hasher.AppendBinary implementations. Return b instead, and add a test that constructs a state without BinaryMarshaler to exercise that path directly.
Write the marshal blob into a fixed [xcSize]byte with named offsets instead of a make+append chain, so the encode side mirrors the decode side in xcAppendState (magic || rate || state || n || direction) and the layout is self-documenting. Not an allocation change: measured 1 alloc/op either way (the sole allocation is x/crypto's state object; the blob never escaped).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Clone() (*Hasher, error)— deep copy of the hasher at its current position. Enables absorbing a shared prefix once and forking per suffix (trie/state-key pattern). It also papers over a portability footgun: plain struct copies ofHasherwork on asm builds (value semantics) but alias shared state on fallback builds where the x/crypto state is a pointer.MarshalBinary/AppendBinary/UnmarshalBinary— persist and resume hash state, mirroring what std-lib hashes provide.Hashernow satisfiesencoding.BinaryMarshaler,encoding.BinaryAppender,encoding.BinaryUnmarshaler(compile-time asserted).Encoding (canonical, single format)
magic "fk1�" (4) || state (200) || flags (1) || pos (1)— 206 bytes, with partially absorbed input always XORed into the state (x/crypto's own internal convention).The format is deliberately independent of the active implementation: which backend runs is a runtime CPU-feature decision (
useASMfrom BMI2 / FEAT_SHA3 detection), so the same binary can be on either path on different hosts. The high-effort review caught that an earlier two-format design CPU-feature-locked persisted states (reproduced under Rosetta: same amd64 binary, different encoding). Now:sha� || rate || state || n || direction).pos == ratevs0— because x/crypto permutes lazily; both decode identically everywhere.)pos == rateonly valid while squeezing).Bonus fix
padAndSqueezeleftabsorbedat its pre-squeeze value. Harmless for hashing (Write/Sum panic once squeezing), but it violated the marshaling invariant — now reset to 0.Testing
Clone independence, prefix-fork over block boundaries, clone mid-squeeze, zero-value clone, marshal round-trips at 8 split points, mid-squeeze round-trips including the exact-block-boundary lazy-permute state, append semantics, malformed-input rejection (magic/length/flags/pos tampering), and the golden cross-implementation fingerprint test. All pass on native arm64,
purego, and under-race(both builds).🤖 Generated with Claude Code
Fallback-arm coverage (added after review)
useASMis a runtime CPU decision, so the x/crypto fallback arms inkeccak_asm.go(taken on amd64 without BMI2 / arm64 without SHA3 — old Xeons, Graviton 2) were compiled into every binary but executed by no CI configuration (puregocompileskeccak_default.go, different code).keccak_fallback_test.gonow forcesuseASM = false(restored viat.Cleanup) and re-runs the whole surface through those arms, plusTestCrossImplementationMarshal: an in-process, both-directions proof that the canonical format is portable between implementations and that both emit byte-identical encodings for the same logical state. Native-build coverage: 54% → 91.5% of statements; the remainder is defensive error branches unreachable with the pinned dependency.