Harden sponge lifecycle, pattern encoding and secret-output storage

[HDauven]

Summary

Closes #37. Not a release or formal cryptographic audit.

This PR covers:

• Invariants/encoding: terminal failure after errors or explicit zeroization, width/31-bit aggregation checks, redacted Debug, corrected-domain tampering tests, frozen mechanics vectors and backend guidance.
• Storage: allocate→copy→wipe→release output growth, including clones; amortized growth with smaller final requests; terminal allocation failures and guarded AE temporaries. Retain the measured hot-path optimizations.

All seven new Unreleased entries link #37. No library dependency requirement, public function signature, error variant, version or valid wire-encoding change.

Behavior and limits

• Valid tag bytes, per-call patterns and representative native outputs remain unchanged. Invalid-use behavior, unsupported widths/lengths and Debug output intentionally change. Failed instances and subsequent clones stay invalid; independent clones made before failure are not revoked.
• Storage grows for actual demand, not the full declared pattern. InvalidIOPattern covers unavailable/unrepresentable output growth; other allocation failures retain platform behavior. Callers must bound memory/work. Old and replacement buffers temporarily coexist; exact Vec capacity is not promised.
• Backend field size/arithmetic, permutation and tag hash determine security. Native equality must be constant-time; circuit equality may defer enforcement to successful proving/verification. Returned output and backend-owned secrets retain their ownership/cleanup responsibilities.
• Cleanup does not certify erasure of compiler copies or handle process abort/overcommit. Reproductions establish mechanics—not authentication bypass, freed-memory extraction or deployed exploitability.

Review follow-up

Addressed the test assertion ordering, fixture provenance and changelog notes in a00265e. The original Python generator is retained by the archival, non-release tag provenance/encryption-fixtures-python; the Rust example records its immutable source and SHA-256. No Python executable was added to the current tree.

The pre-existing whole-sponge equality API is tracked separately in #39. Inlining is unchanged pending a controlled comparison; no new performance claim is made by this cleanup.

Validation

Review-cleanup checks at a00265e: 19 stable debug + 19 release, 12 no-default, 19 MSRV 1.96.1; the pinned zeroize 1.5.4 consumer; nightly formatting and prescribed Clippy; docs; bare-metal builds with/without encryption; all four frozen mechanics vectors. All passed in an isolated offline source copy. Runtime code, fixture payloads and the example algorithm are unchanged. Miri, downstream suites and timing were not rerun for this test/docs/changelog-only update.

Earlier local validation on bf63c00:

• 19 debug + 19 release, 12 no-default, 19 MSRV 1.96.1, 4 Miri storage/failure/unwind tests.
• Prescribed CQ/docs and bare-metal builds without/with encryption pass. The Rust generator reproduces all four frozen vectors.
• make test now checks an isolated consumer pinned to zeroize 1.5.4. Explicitly borrowing the inner message slice avoids requiring the newer AsRef implementation on Zeroizing. That consumer also passes release/MSRV/encryption-enabled bare-metal checks. Reverting the fix makes the new gate fail with E0277 while ordinary tests still pass.
• New controls verify buffered-output wiping immediately after allocation failure, before finish/Drop can mask it, and temporary-message-buffer cleanup on an injected backend panic. Removing either protection fails its regression; all mutations restored.
• 15 external invariants per debug/release profile; downstream Poseidon 20 native/circuit/docs, Phoenix-core 29, Merkle 2 including opening proof, Rusk-core 65, all resolving this source.

Retained earlier compatibility/negative-control evidence:

• Six 14-case native corpora (baseline/prior/final × two build profiles) are byte-identical; untouched decryptions succeed and tested mutations return DecryptionFailed. SHA-256: 52e2b08003df3fc51a4a6c39a2d80ad3eb32445674bf5fc45fdf618cdc9a5d45.
• Negative controls detect missing old-buffer wiping, late-error plaintext cleanup, encoding reset and caller growth guard. Capacity-policy variation passes the corrected wiping test, including Miri. Labelled key/nonce/domain rejection cases retain seeded inputs and distinct-input assertions; each correct-parameter substitution fails its negative assertion. No mutation remains.

cargo run --locked --example generate_encryption_fixtures checks the frozen file; append -- --write to regenerate explicitly. The Rust reference uses existing dev-dependencies and calls no SAFE implementation. Original Python-generated payloads and pinned provenance are preserved; field arithmetic/hashing/serialization are shared with the Rust test backend. No Python executable is shipped or required. These are mechanics vectors, not production cryptographic KATs.

Full-node execution, ARM/WASM runtime timing and side-channel/formal-security certification were not performed. Optional stricter all-target Clippy had historical baseline diagnostics; it is not claimed as a green check.

Performance and memory

External harness: Rust 1.98.0, shared WSL2/Hyper-V, Ultra 9 275HX, virtual CPU 2 affinity; frozen dependencies/inputs and rotating variant order. 2,619 timing and 486 allocation samples. Normal-allocator timing binaries and separately instrumented allocation binaries; raw evidence/harness retained locally, not added as a dependency or CI job.

The initial ~47% regression in 1,024 single-element toy squeezes was resolved before publication:

Release build	Baseline	Earlier hardening	Submitted implementation
LTO / 1 codegen unit	4.308 µs	6.262 µs	3.815 µs
No LTO / 16 codegen units	4.556 µs	6.832 µs	4.275 µs

Other measured stream lengths (1,16,256,4096) also improved in both profiles. This noncryptographic bookkeeping benchmark is not whole-node/proof throughput. Native results remain noisy: longer rechecks showed +2.3% LTO decrypt(1), +3.2% LTO encrypt(4096), +0.2% no-LTO decrypt(5). Less favorable broad-matrix observations, including roughly +23% small-decrypt medians, remain retained. No universal speedup, zero-overhead guarantee or tight native regression bound.

Peak requested live heap per operation, excluding prepared inputs, stack and allocator internals—not RSS:

Case	Baseline	Submitted implementation
Native encrypt, 4096 scalars	393,328 B	262,288 B (~−33%)
Native decrypt, 4096 scalars	393,296 B	393,328 B (+32 B)
Toy clone continuation, 4096	65,584 B	32,832 B (~−50%)
Toy stream, 1024	24,592 B	28,688 B (~+16.7%)

The streaming increase is the explicit old/new buffer overlap. Benchmarks predate the explicit-slice compatibility fix; algorithms and allocation policy are unchanged. Timing was not rerun for that borrow change or the comments/tests/tooling edits.

Author-side Ponytail and scoped preflight checks are complete, not independent approval. Review focus: terminal state, canonical encoding, wiping/allocation-failure ordering, and native/circuit compatibility/resource contracts in #37.

[Copilot]

🔵 Needs a closer look

It changes failure semantics, IO-pattern encoding, and secret-bearing storage behavior in a cryptographic primitive, warranting careful human review despite strong accompanying tests.

Pull request overview

This PR hardens the SAFE sponge’s lifecycle and IO-pattern/tag encoding while tightening secret-output storage behavior, aiming to preserve valid encodings and existing outputs while making misuse/failure states terminal and reducing secret exposure through Debug and allocation behavior.

Changes:

• Make sponge failure/zeroization terminal (including subsequent clones), reject widths < 2, and redact Debug output.
• Canonicalize and harden IO-pattern encoding by enforcing 31-bit aggregated run length bounds and encoding runs directly.
• Harden secret-output storage growth to avoid allocator-driven reallocation of live secret buffers, and extend encryption tests/fixtures (including independent Python-generated mechanics vectors and corrected-domain tamper coverage at rate boundaries).

File summaries

File	Description
tests/sponge.rs	Adds regressions for terminal failure/zeroization behavior, width rejection, and redacted Debug output.
tests/encryption.rs	Adds independent mechanics vectors and strengthens tamper coverage across message lengths/positions under the correct domain.
tests/fixtures/generate.py	Introduces an independent Python model to generate/check frozen mechanics vectors.
tests/fixtures/encryption.txt	Adds frozen, Python-generated mechanics vectors consumed by tests.
src/sponge.rs	Implements terminal failure state, redacted Debug, width checks, and hardened output-growth allocation/wipe strategy plus allocator-boundary tests.
src/lib.rs	Reworks tag input encoding to emit canonical runs directly and reject aggregated-run overflow.
src/error.rs	Clarifies InvalidIOPattern semantics to include aggregated-run limits, width limits, and output-storage failures.
src/encryption.rs	Uses Zeroizing for temporary plaintext/cipher buffers and clarifies native/circuit equality obligations.
README.md	Updates API/security explanations around field/security assumptions, lifecycle, and output storage limits.
CHANGELOG.md	Adds issue-linked Unreleased entries documenting the hardening work and references #37.
AGENTS.md	Corrects encryption description to reflect field addition/subtraction (not XOR).

Review details

• Files reviewed: 11/11 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.

[Copilot]

🔵 Needs a closer look

It changes failure/zeroization, encoding, and allocation behavior in a cryptographic primitive, so it needs careful human review even though only minor nits were found.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core sponge/AE lifecycle and secret-handling behavior in a cryptographic primitive and should receive final human review despite only a minor test issue found.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new #[cfg(test)] allocator-boundary tests in src/sponge.rs use thread_local! keys via .set()/.get() directly, which is not MSRV-compatible and should be fixed to use with/try_with to compile reliably.

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

Review details

Suppressed comments (5)

src/sponge.rs:431

• WIPED and WATCH are thread_local! keys; calling .set(...) on them directly is not valid on the MSRV (they are LocalKey<Cell<_>>). Use with(...) to access the inner Cell.

    fn watch(value: &u64) {
        assert_ne!(*value, 0);
        WIPED.set(false);
        WATCH.set(value as *const u64 as usize);
    }

src/sponge.rs:453

• WIPED is a thread_local! key, so WIPED.get() won’t compile on the MSRV. Read the inner Cell via with(...).

            assert!(
                WIPED.get(),
                "old output must be wiped before allocator release"
            );

src/sponge.rs:477

• FAIL_AFTER_SUBTRACT/FAIL_NEXT are thread_local! keys, so calling .replace/.set on them directly won’t compile on the MSRV. Use with(...) to access the underlying Cell.

        fn subtract(&mut self, a: &u64, b: &u64) -> u64 {
            if FAIL_AFTER_SUBTRACT.replace(false) {
                watch(b);
                FAIL_NEXT.set(true);
            }

src/sponge.rs:490

• In this test, FAIL_AFTER_SUBTRACT/FAIL_NEXT are thread_local! keys; set/get need to go through with(...) for MSRV compatibility.

    fn late_allocation_failure_wipes_decrypted_message() {
        FAIL_AFTER_SUBTRACT.set(true);
        let result = crate::decrypt(Identity, 0u64, [77u64; 5], &[7, 8], &9);
        assert!(!FAIL_NEXT.get());
        assert_eq!(result, Err(Error::InvalidIOPattern));

src/sponge.rs:494

• WIPED is a thread_local! key; WIPED.get() should use with(...) to access the inner Cell on the MSRV.

        assert!(
            WIPED.get(),
            "plaintext must be wiped on the late squeeze error"
        );

• Files reviewed: 11/11 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new storage_tests in src/sponge.rs uses thread_local! keys via unsupported direct .get()/.set()/.replace() calls, which will not compile without switching to .with(...) accessors.

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

Review details

Suppressed comments (6)

src/sponge.rs:384

• std::thread_local! creates LocalKey<Cell<_>> values; it does not support calling set() directly. This line won’t compile—use WIPED.with(|c| c.set(...)) (or try_with) inside the allocator callback.

                WIPED.set(unsafe { *ptr.cast::<u64>() == 0 });

src/sponge.rs:467

• FAIL_NEXT is a thread_local! key; direct .set() / .get() calls won’t compile. Use .with(...) for both setting and reading.

        FAIL_NEXT.set(true);
        assert_eq!(sponge.squeeze(1), Err(Error::InvalidIOPattern));
        assert!(!FAIL_NEXT.get());

src/sponge.rs:492

• WIPED is a thread_local! key; .get() must be accessed via .with(...). The current call won’t compile.

            WIPED.get(),

src/sponge.rs:451

• WIPED is a thread_local! key; .get() needs to be accessed via .with(...). The current call won’t compile.

                WIPED.get(),

src/sponge.rs:476

• FAIL_AFTER_SUBTRACT/FAIL_NEXT are thread_local! keys; .replace()/.set() must be done via .with(...). As written, this won’t compile.

            if FAIL_AFTER_SUBTRACT.replace(false) {
                watch(b);
                FAIL_NEXT.set(true);

src/sponge.rs:489

• FAIL_AFTER_SUBTRACT and FAIL_NEXT are thread_local! keys; direct .set() / .get() calls won’t compile. Use .with(...) for both.

        FAIL_AFTER_SUBTRACT.set(true);
        let result = crate::decrypt(Identity, 0u64, [77u64; 5], &[7, 8], &9);
        assert!(!FAIL_NEXT.get());

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It modifies core lifecycle, encoding, and secret-handling behavior in a cryptographic primitive and should receive final human review despite no specific defects found in the diff.

Review details

• Files reviewed: 14/15 changed files
• Comments generated: 0 new
• Review effort level: Lite

[moCello]

APPROVE — the terminal-failure state, the checked aggregated-run encoding, the allocate-copy-wipe-release output growth and the zeroizing wrappers around the encryption temporaries all hold on trace, and valid tag bytes and per-call pattern checks are unchanged.

• Five notes below, all small. None asks for a change before merge. One names a pre-existing defect that belongs in a follow-up issue: the derived equality on the sponge still reads secret state in variable time.
• Advisory only: the pull request bundles three separable concerns, lifecycle and encoding, output storage, and the fixture and compatibility tooling. Each could ship as its own review. This does not gate the merge.
• Process, one line each: the commit message is descriptive and matches the diff; the change links #37 and the changelog links it; no lint suppression, no new library dependency, and the feature gate on encryption is respected.

[moCello]

NOTE: The guard assertion can fail only while FAIL_NEXT is armed, so it aborts the test binary instead of failing one test.

Context: The allocator disarms the flag as it fails, so assert!(!FAIL_NEXT.get()) can fail only while the flag is still armed. Any panic raised while armed allocates, the allocator returns null, and handle_alloc_error aborts the process. The assertion therefore aborts instead of failing, its own message never prints, and every test not yet finished loses its result. The run shows only memory allocation of N bytes failed. Such a failure needs a regression that returns the expected error without allocating, and the checked_add guard in grow_output is that shape. The same order sits in late_allocation_failure_wipes_decrypted_message, where the guard at line 529 is the first assertion after subtract arms the flag.

Fix: Capture the result, disarm the flag, then assert. For example let result = sponge.squeeze(len); assert!(!FAIL_NEXT.replace(false)); assert_eq!(result, Err(Error::InvalidIOPattern));. The decrypt test already has that order; change its get() to replace(false).

[moCello]

NOTE: The derived PartialEq still compares the secret state and buffered output in variable time; the fix is a tracked follow-up, not this change.

Context: This change redacts Debug because the derived output exposed state and output. The derived equality on the same fields is the remaining public surface that reads secrets element by element with a data-dependent early exit. Removing the derive is a breaking API change and belongs in a release that can carry it. The defect predates this change.

Fix: Open an issue to drop PartialEq from Sponge, or to replace it with a metadata-only comparison, at the next breaking release. Reference it from the issue this change closes.

[moCello]

NOTE: The provenance link points at a commit that no branch or tag contains after the squash; GitHub keeps it only as a dangling object.

Context: Commit d4802e77 exists on the remote today but is not reachable from main or from the head of this pull request. Dangling objects are not guaranteed to survive garbage collection. The fixture header in tests/fixtures/encryption.txt delegates its provenance to this comment, so both lose their reference together.

Fix: Keep the Python generator in the tree under tests/fixtures/, or record its SHA-256 next to the link so the claim stays checkable without the commit. If neither is wanted, state that the Rust example is the sole reference and drop the link.

[moCello]

NOTE: These two entries describe implementation details that a consumer of the crate cannot observe.

Context: The changelog is the headline a consumer reads to decide whether a release affects them. The word-vector removal and the inlining attributes change neither the tag bytes nor the API. The aggregated-run rejection and the allocation behaviour already have entries under Fixed.

Fix: Drop both entries. If the measured squeeze speed-up is worth recording, keep one entry that names the observable effect, for example "Reduce per-call overhead of small squeeze calls".

[moCello]

NOTE: #[inline(always)] on a public generic method forces the loop body into every downstream call site; #[inline] gives the optimizer the same option without forcing it. Yours to take or leave.

Context: The method is generic, so it is already available for cross-crate inlining. Neither the diff nor the comment records whether plain #[inline] was measured; if it keeps the single-element speed-up, the forced form buys nothing. Clippy's pedantic inline_always lint flags this shape for the same reason.

Fix: Change the attribute to #[inline] if a re-measurement of the single-element stream shows no loss. Otherwise, add one sentence to the comment that names the measured loss with #[inline], so the next reader does not remove it.

[HDauven]

Addressed the review cleanups in a00265e:

• Both allocation tests disarm the failure flag before assertions can panic.
• The original generator is retained under the archival, non-release tag provenance/encryption-fixtures-python, with its SHA-256 beside the pinned source link. No Python executable was added to the current tree.
• Removed the two implementation-only changelog entries.
• Equality is tracked separately in Remove secret-state equality from Sponge in the next breaking release #39, cross-linked from Harden sponge lifecycle, pattern encoding and secret-output storage #37.
• Kept the inlining choice unchanged pending a controlled comparison.

The scoped local test/build/format/lint/fixture checks pass, and CI on the updated head is green. No runtime code or fixture payload changed in this cleanup.