Repository navigation
Add fallible try_ variants of the Hash finalize and digest functions - #301
Conversation
Hash::try_finalize, try_finalize_truncated, try_digest and try_digest_truncated return the io-pattern error instead of a panic. The existing panicking functions now wrap them, so valid input gives the same output. Correct the # Panics docs on Hash and HashGadget: no input, an empty chunk passed to update, and a Merkle arity mismatch all panic.
The link pointed at safe::Sponge, which does not resolve, so cargo doc with -D warnings failed.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Consensus-critical hash finalization warrants maintainer confirmation of regression results before approval.
Review effort: Balanced
Findings: None
What changed in this PR
Adds fallible native hashing APIs for #300 while retaining the existing panicking APIs and valid-input behavior.
Changes:
- Adds four
Hash::try_*methods that return existing error variants. - Expands panic/error documentation and fixes an intra-doc link.
- Adds error-handling and compatibility tests, plus a changelog entry.
| File | Description |
|---|---|
| tests/hash_errors.rs | Tests invalid inputs, API agreement, and retained panics. |
| src/hash/gadget.rs | Documents additional panic conditions; no behavior changes. |
| src/hash.rs | Adds fallible methods and delegates existing APIs to them. |
| src/error.rs | Corrects documentation wording and the sponge link. |
| CHANGELOG.md | Records the new APIs under Unreleased. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
herr-seppia
left a comment
herr-seppia
left a comment
There was a problem hiding this comment.
Reviewed the complete diff against master and issue #300. No blocking or non-blocking code findings: the checked APIs propagate the existing SAFE errors, the infallible APIs retain their intentional panic behavior, and valid-input permutation, domain, padding, borrowing, and constant-time behavior are unchanged.
Verification passed: formatting; Clippy with all features and no default features; release tests with default and all features; doctests with rustdoc warnings denied; thumbv6m no_std build; benchmark compilation; and an independent release-mode differential comparison against master covering 9,700 valid cases across every domain, native and truncated outputs, rate-boundary output lengths, exhaustive chunk partitions through length 10, selected lengths through 64, and randomized chunkings. Base reproduced both one-shot and incremental empty-input panics; the new APIs returned the documented errors. Repeated finalization and recovery after failed no-input or incomplete-Merkle finalization also behaved correctly.
Non-blocking nits: permanent tests for repeated/failed-finalize state behavior would be worthwhile, and AGENTS.md references a non-portable ~/dusk/CLAUDE.md path unavailable in an isolated checkout. Separately, downstream Rusk must migrate its contract-facing host query to try_digest before the originating availability issue is considered fixed; this PR correctly provides the required library API.
The sentence pointed to ~/dusk/CLAUDE.md, which no checkout has.
|
Thanks for the thorough review and the differential run.
|
Closes #300