Skip to content

test(math): pin stream_bytes byte parity with as_bytes#832

Open
MauroToscano wants to merge 1 commit into
mainfrom
test/stream-bytes-parity
Open

test(math): pin stream_bytes byte parity with as_bytes#832
MauroToscano wants to merge 1 commit into
mainfrom
test/stream-bytes-parity

Conversation

@MauroToscano

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #828, which rewired the two Merkle backends and DefaultTranscript::append_field_element from as_bytes()/to_bytes_be() to the new AsBytes::stream_bytes. That rewire is only sound because stream_bytes emits byte-identical output — it does, I checked both fields — but nothing enforces it, and #828 shipped without tests.

stream_bytes is a defaulted trait method. An override that disagrees with as_bytes compiles cleanly and then silently changes every Merkle root and Fiat-Shamir challenge that flows through it. The failure mode is proofs that no longer verify against previously committed roots, discovered at integration time — not a red test.

  • Adds crypto/math/tests/stream_bytes_parity.rs: asserts stream_bytes == as_bytes == to_bytes_be for FieldElement<GoldilocksField> and FieldElement<Degree3GoldilocksExtensionField> over ~2k pseudorandom values plus edge cases. The edge cases deliberately include non-canonical u64s (p, p+1, u64::MAX), since both encodings reduce through canonical_u64 and that is exactly where a raw-value override would diverge.
  • Pins the ext3 layout that math-cuda's keccak_leaves_ext3 kernel mirrors (canonical u64 per component, big-endian, order 0,1,2 — matching write_bytes_be). The GPU parity tests in math-cuda/tests/keccak_leaves.rs only run on a CUDA host, and gpu-tests was SKIPPED on perf(crypto): stream field-element bytes into hashers and transcript #828, so this keeps the CPU half of that contract honest on a GPU-less runner.
  • Covers the defaulted fallback path for a type that does not override stream_bytes.
  • Documents the invariant on the trait method in traits.rs, where an implementor would actually look for it.

Comment fix

The stream_bytes comment in extensions_goldilocks.rs said it makes "one sink call over a stack buffer instead of three (one per limb)" and that "call count — not indirection — is the cost being cut". The previous code never made three calls: as_bytes() returned a single 24-byte Vec, which became a single Digest::update. The three-call version was a considered alternative, never what shipped. What the override actually removes is the per-element heap allocation. Corrected, and noted that the layout is load-bearing for CPU/GPU leaf parity.

No behavior change — docs and tests only.

Test plan

  • cargo test -p math --test stream_bytes_parity — 5/5
  • Mutation-checked that the test can actually fail: flipping Goldilocks stream_bytes to little-endian fails 1 test; swapping the ext3 component order in write_bytes_be fails 3, including the GPU-contract test.
  • cargo test -p math -p crypto -p stark --lib — 418/418
  • make lint — clean (exit 0; all four clippy configs including the cuda-feature pass)

#828 rewired the Merkle backends and DefaultTranscript::append_field_element
from as_bytes()/to_bytes_be() to the new AsBytes::stream_bytes. That swap is
only sound because stream_bytes emits byte-identical output, but nothing
enforces it: stream_bytes is a defaulted trait method, so an override that
disagrees with as_bytes compiles cleanly and then silently changes Merkle
roots and Fiat-Shamir challenges rather than failing a test.

Adds a parity sweep over both Goldilocks fields, including non-canonical u64s
around the modulus where a raw-value override would diverge from as_bytes,
and pins the ext3 byte layout that math-cuda's keccak_leaves_ext3 kernel
mirrors component-by-component. The GPU parity tests only run on a CUDA host,
so this keeps the CPU half of that contract honest on a GPU-less runner.
Documents the invariant on the trait method itself, where an implementor
would look for it.

Also corrects the stream_bytes comment in extensions_goldilocks.rs: it
described cutting three sink calls down to one, but as_bytes() already
produced a single 24-byte Vec and a single Digest::update. The per-element
allocation, not the call count, is what the override removes.
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.

1 participant