Skip to content

keccak: add independent hasher cloning - #22

Draft
yperbasis wants to merge 5 commits into
masterfrom
review/hasher-clone
Draft

yperbasis wants to merge 5 commits into
masterfrom
review/hasher-clone

Conversation

@yperbasis

Copy link
Copy Markdown
Member

Summary

  • document that a used Hasher must not be copied directly
  • add Clone for an explicit, implementation-independent deep copy
  • cover absorbing, squeezing, zero-value, native, pure-Go, and runtime-fallback states

Why

Direct struct copies currently have platform-dependent behavior. The native sponge is stored by value, while the fallback stores an interface containing a pointer to the x/crypto/sha3 state. A direct copy is therefore independent on an accelerated CPU but aliases state on the fallback.

Clone copies the native sponge directly and deep-copies the fallback through its binary state implementation. This is an independent, focused split of the cloning part of #13; it does not add marshaling APIs or squeeze changes.

TDD

The first tests failed because Hasher.Clone did not exist. A separate zero-value cycle then failed in the runtime fallback with:

Clone zero value: keccak: fallback hasher does not support cloning

The zero-value path was added after that failure.

Verification

  • go test ./...
  • go test -race ./...
  • go test -tags purego ./...
  • go vet ./...
  • go vet -tags purego ./...
  • go mod tidy -diff
  • golangci-lint run
  • golangci-lint run --build-tags purego

Comment thread keccak_asm.go
Comment thread keccak.go Outdated
Comment thread keccak_clone_asm_test.go Outdated
Cloning with useASM true while the state lives in h.xc returned an empty
sponge and a nil error. Branch on which sub-state the receiver holds, use
comma-ok for the KeccakState assertion so Clone returns errCloneUnsupported
instead of panicking, and cover the fallback receiver by building it
directly rather than by writing the useASM global from a test.
The asm-side caller went away with keccak_clone_asm_test.go.
Copying only the live one made the copy dispatch differently from the
original: with both populated, Clone returned a zero sponge and the copy
hashed the empty string where the original hashed its sponge data.
@AskAlexSharov

Copy link
Copy Markdown
Contributor

Found

Clone was not indistinguishable from the original. keccak_asm.go:227 copied only the live sub-state, so a Hasher holding a populated sponge and a populated xc cloned to a zero sponge: the copy hashed the empty string (c5d24601…) where the original hashed its sponge data (935733f9…). Dispatching on useASM instead has the mirror failure — the copy carries xc that the clone's own Write/Sum256 then ignore — so neither one-line branch is correct.

TestHasherCloneFallbackState validated an object it never exercised. It asserted only through the internal xc field, so it stayed green while the clone was unusable through the public API.

Dead indirection. testHasherClone had a single caller left once keccak_clone_asm_test.go was removed.

Fixed

  • Clone carries both sub-states, so the copy dispatches the same way the original does for any useASM. 09a0f5c
  • The test now compares clone.Sum256() against original.Sum256() and checks the sponge came across. Verified: re-dropping the sponge fails it with the digests above.
  • Helper folded into TestHasherClone. b96937e

Not fixed — your call

Clone() (*Hasher, error) cannot satisfy hash.Cloner (go1.25: Clone() (Cloner, error)), and one type cannot carry both methods. crypto/hmac probes for that interface. go.mod is on go1.24 so it is not reachable yet, but the exported signature is a one-way door — I did not change it here.

@AskAlexSharov

Copy link
Copy Markdown
Contributor

hash.Cloner — decided, not changing it

Following up on the open item from my last comment. Leaving Clone() (*Hasher, error) as is:

  • Making *Hasher satisfy hash.Cloner means Clone() (hash.Cloner, error), which needs go.mod's floor moved from go 1.24.0 to 1.25 — the interface landed in go1.25.
  • It costs a type assertion at every call site that wants Sum256, since that is not on the interface. The concrete return is what the actual callers use.
  • Nothing here or in erigon consumes hash.Cloner; crypto/hmac is its main prober, and HMAC-Keccak is not a use case for this library. Add Hasher.Clone and binary state marshaling #13 chose the same concrete signature independently.
  • If a consumer ever needs the interface, switching is one line here plus an assertion there. That is a cheaper reversal than carrying assertions now.

While checking this I also confirmed cloneXCrypto cannot be simplified: x/crypto v0.48.0's sha3.NewLegacyKeccak256() returns *sha3.state, which implements MarshalBinary/UnmarshalBinary but not hash.Cloner (only ShakeHash has a Clone there). The marshal/unmarshal round-trip is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants