feat: enforce MemoSize invariants at compile time and add DashMemo coverage#7
Conversation
…verage The MemoSize trait documented but did not enforce the relationships between MEMO_SIZE and its associated byte types, so a mis-sized third-party implementation compiled cleanly and then panicked at runtime -- during note encryption and, worse, inside the ZIP-244-style txid digest where unguarded `enc.len() - 16` slicing could underflow. Add a MEMO_SIZE associated const and a SIZE_CHECK const assertion that is referenced by the encryption, decryption, and txid-hashing paths, turning a mis-sized implementation into a monomorphization-time compile error. DashMemo also had zero test coverage anywhere in the crate. Add: - tests/dash_memo.rs: a bundle round-trip exercising full trial decryption, compact decryption, OVK output recovery, and txid commitment computation with 36-byte memos through the public API, plus a deterministic golden vector pinning the 104-byte Dash note ciphertext wire format - txid digest golden vectors for both ZcashMemo and DashMemo bundles, built entirely from fixed inputs - a compile-time regression test that the size checks hold for both provided implementations Also generalize `Bundle<EffectsOnly, V>::from_parts` over MemoSize; it was pinned to the ZcashMemo default, making effects-only bundles unconstructible for other memo sizes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe MemoSize trait gains MEMO_SIZE and SIZE_CHECK associated constants for compile-time validation of memo/plaintext/ciphertext byte sizes. These checks are wired into note_encryption.rs and bundle/commitments.rs, and Bundle::from_parts for EffectsOnly is generalized over M. New tests validate golden vectors and Dash memo round-trips. ChangesMemoSize Compile-Time Validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/memo.rs (1)
47-69: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDocument the
SIZE_CHECKinvariant for customNoteBytestypes.size_ofonly works as intended for implementations whose exposed byte length stays aligned with the associated size constants; a short note here would make that contract explicit for futureMemoSizeimpls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memo.rs` around lines 47 - 69, Add a short doc note on MemoSize::SIZE_CHECK explaining that custom NoteBytes/Memo implementations must keep their exposed byte lengths exactly aligned with MEMO_SIZE, COMPACT_NOTE_SIZE, and AEAD_TAG_SIZE. Mention that SIZE_CHECK enforces the compile-time contract for MemoSize::Memo, NotePlaintextBytes, and NoteCiphertextBytes so future impls understand the invariant and don’t rely on runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/memo.rs`:
- Around line 47-69: Add a short doc note on MemoSize::SIZE_CHECK explaining
that custom NoteBytes/Memo implementations must keep their exposed byte lengths
exactly aligned with MEMO_SIZE, COMPACT_NOTE_SIZE, and AEAD_TAG_SIZE. Mention
that SIZE_CHECK enforces the compile-time contract for MemoSize::Memo,
NotePlaintextBytes, and NoteCiphertextBytes so future impls understand the
invariant and don’t rely on runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6750635f-9971-4e66-bdc9-983b1e9a219b
📒 Files selected for processing (5)
src/bundle.rssrc/bundle/commitments.rssrc/memo.rssrc/note_encryption.rstests/dash_memo.rs
* perf: build note plaintexts in a stack buffer instead of a heap Vec note_plaintext_bytes assembled the secret note plaintext (diversifier, value, rseed, memo) in a 52-byte stack array, copied it into a transient alloc::vec! buffer, and copied that again via from_slice -- one heap allocation plus two redundant copies per encrypted output on a previously allocation-free path, with the Vec freed without zeroization leaving the only heap copy of note plaintext in the encryption path. Build the plaintext directly in a single max-size stack buffer (COMPACT_NOTE_SIZE + MAX_MEMO_SIZE = 564 bytes) and convert once with from_slice(&buf[..len]). A new MAX_MEMO_SIZE bound in SIZE_CHECK guarantees at compile time that the buffer is large enough for any MemoSize implementation that compiles. Output is byte-for-byte unchanged, as pinned by the note encryption test vectors and the golden vectors added in #7. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: validate Memo::as_ref() length before the plaintext buffer copy SIZE_CHECK constrains the size of the Memo type, but a custom AsRef implementation could still return a slice of a different length, surfacing as an opaque slice-index panic in the buffer copy. Assert the slice length explicitly and derive the plaintext length from M::MEMO_SIZE, which SIZE_CHECK already bounds by MAX_MEMO_SIZE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Closes findings #2 and #3 from the MemoSize PR review (see #1 for context):
MemoSizesize invariants are now enforced at compile time. The trait documented — but nothing enforced — thatNotePlaintextBytes= 52 + memo andNoteCiphertextBytes= 52 + memo + 16. A mis-sized third-party impl compiled cleanly and then panicked at runtime: at theexpectinnote_plaintext_bytesduring encryption, and via unguardedenc.len() - 16slicing inside the consensus-critical txid digest (hash_bundle_txid_data). The trait now carries aMEMO_SIZEassociated const plus aSIZE_CHECKconst assertion referenced by the encryption, decryption, and txid-hashing paths — a mis-sized implementation fails at monomorphization with an explicit message instead of panicking in production (verified manually: a copy-paste-broken impl fails to build withE0080: evaluation of SIZE_CHECK failed).First DashMemo test coverage in the crate. Previously no test instantiated
M = DashMemo; the 88/104-byte code paths only ever ran in downstream consumers.Changes
src/memo.rs:const MEMO_SIZE: usizeandconst SIZE_CHECK: ()on theMemoSizetrait; consts on both impls; compile-time regression testsrc/note_encryption.rs:SIZE_CHECKforced innote_plaintext_bytes(encrypt) andsplit_plaintext_at_memo(decrypt)src/bundle/commitments.rs:SIZE_CHECKforced inhash_bundle_txid_data, guaranteeing the ciphertext slicing cannot panic; golden txid digest vectors for both memo sizes from fully deterministic bundlessrc/bundle.rs:Bundle<EffectsOnly, V>::from_partsgeneralized overM: MemoSize— it was pinned to theZcashMemodefault (same lock-in pattern as the PCZT impls fixed in fix: make PCZT trial decryption generic over MemoSize #6), making effects-only bundles unconstructible for Dashtests/dash_memo.rs(new): public-API round-trip for a 36-byte-memo bundle — full trial decryption, compact decryption, OVK output recovery, txid commitment — plus a deterministic golden vector pinning the 104-byte Dash note ciphertext wire formatThe golden vectors double as the compatibility contract for any future migration (e.g. onto upstream's OrchardZSA flavor architecture): a byte-exact reimplementation must reproduce these exact digests and ciphertexts.
API note: adding a required associated const to the public
MemoSizetrait is a breaking change for external implementors of the trait (not for users ofZcashMemo/DashMemo) — intentional, and cheap to absorb pre-release.Test plan
cargo test— 86 tests pass (full suite including proving tests)MemoSizeimpl fails to compile withE0080onSIZE_CHECKcargo clippy --all-features --all-targetscleancargo fmt -- --checkRUSTDOCFLAGS="-D warnings" cargo doc --all-features --document-private-itemsclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests