Skip to content

feat: enforce MemoSize invariants at compile time and add DashMemo coverage#7

Merged
QuantumExplorer merged 1 commit into
dashifiedfrom
fix/memo-size-invariants
Jul 2, 2026
Merged

feat: enforce MemoSize invariants at compile time and add DashMemo coverage#7
QuantumExplorer merged 1 commit into
dashifiedfrom
fix/memo-size-invariants

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

Closes findings #2 and #3 from the MemoSize PR review (see #1 for context):

  • MemoSize size invariants are now enforced at compile time. The trait documented — but nothing enforced — that NotePlaintextBytes = 52 + memo and NoteCiphertextBytes = 52 + memo + 16. A mis-sized third-party impl compiled cleanly and then panicked at runtime: at the expect in note_plaintext_bytes during encryption, and via unguarded enc.len() - 16 slicing inside the consensus-critical txid digest (hash_bundle_txid_data). The trait now carries a MEMO_SIZE associated const plus a SIZE_CHECK const 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 with E0080: 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: usize and const SIZE_CHECK: () on the MemoSize trait; consts on both impls; compile-time regression test
  • src/note_encryption.rs: SIZE_CHECK forced in note_plaintext_bytes (encrypt) and split_plaintext_at_memo (decrypt)
  • src/bundle/commitments.rs: SIZE_CHECK forced in hash_bundle_txid_data, guaranteeing the ciphertext slicing cannot panic; golden txid digest vectors for both memo sizes from fully deterministic bundles
  • src/bundle.rs: Bundle<EffectsOnly, V>::from_parts generalized over M: MemoSize — it was pinned to the ZcashMemo default (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 Dash
  • tests/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 format

The 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 MemoSize trait is a breaking change for external implementors of the trait (not for users of ZcashMemo/DashMemo) — intentional, and cheap to absorb pre-release.

Test plan

  • cargo test — 86 tests pass (full suite including proving tests)
  • Manual negative test: a mis-sized MemoSize impl fails to compile with E0080 on SIZE_CHECK
  • cargo clippy --all-features --all-targets clean
  • cargo fmt -- --check
  • RUSTDOCFLAGS="-D warnings" cargo doc --all-features --document-private-items clean

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added support for memo-sized bundles beyond the default configuration.
    • Expanded note encryption and bundle handling to work with different memo formats.
  • Bug Fixes

    • Added safeguards to prevent invalid memo sizes from causing runtime failures.
    • Improved bundle commitment and transaction hash consistency checks.
  • Tests

    • Added end-to-end coverage for memo handling and note recovery.
    • Added fixed golden-vector tests to verify stable encrypted output.

…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>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

MemoSize Compile-Time Validation

Layer / File(s) Summary
MemoSize trait constants and implementations
src/memo.rs
Adds MEMO_SIZE and SIZE_CHECK associated constants to MemoSize, implements MEMO_SIZE=512 for ZcashMemo and MEMO_SIZE=36 for DashMemo, plus a test module validating both.
Wiring SIZE_CHECK into encryption and bundle constructor
src/note_encryption.rs, src/bundle.rs
note_plaintext_bytes and split_plaintext_at_memo force evaluation of M::SIZE_CHECK; EffectsOnly's from_parts constructor becomes generic over M: MemoSize.
Txid hashing size check and golden vectors
src/bundle/commitments.rs
hash_bundle_txid_data references M::SIZE_CHECK before slicing enc_ciphertext; new tests build deterministic bundles and assert fixed digest hex outputs for ZcashMemo and DashMemo.
Dash memo end-to-end tests
tests/dash_memo.rs
New tests cover full/compact decryption and OVK recovery for a Dash-sized memo bundle, plus a golden-vector test for note encryption with exact ciphertext assertions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • dashpay/orchard#1: Introduces the generic MemoSize/M parameterization across Action/Bundle/note-encryption that this PR extends with MEMO_SIZE/SIZE_CHECK compile-time validation.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: compile-time MemoSize invariants and added DashMemo test coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/memo-size-invariants

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/memo.rs (1)

47-69: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Document the SIZE_CHECK invariant for custom NoteBytes types. size_of only 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 future MemoSize impls.

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between 81ca424 and 47e937f.

📒 Files selected for processing (5)
  • src/bundle.rs
  • src/bundle/commitments.rs
  • src/memo.rs
  • src/note_encryption.rs
  • tests/dash_memo.rs

@QuantumExplorer QuantumExplorer merged commit 6d3bc20 into dashified Jul 2, 2026
18 checks passed
@QuantumExplorer QuantumExplorer deleted the fix/memo-size-invariants branch July 2, 2026 09:08
QuantumExplorer added a commit that referenced this pull request Jul 2, 2026
* 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>
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