Feature: note plaintext size generalization #2

PaulLaux · 2024-04-30T17:06:55Z

(updated description 20.08.2024)
This PR continues the discussion from: zcash/librustzcash#746. Some things have changed, and we need an updated review to continue.

In order to support note encryption for ZSA, we suggest extending the current zcash_note_encryption implementation. Currently, the COMPACT_NOTE_SIZE is a constant; however, we need to support variable note sizes to include the AssetId field for ZSA notes.

Current state in `zcash_note_encryption`:

/// The size of a compact note.
pub const COMPACT_NOTE_SIZE: usize = 1 + // version
    11 + // diversifier
    8  + // value
    32; // rseed (or rcm prior to ZIP 212)
/// The size of [`NotePlaintextBytes`].
pub const NOTE_PLAINTEXT_SIZE: usize = COMPACT_NOTE_SIZE + 512;

and

pub const ENC_CIPHERTEXT_SIZE: usize = NOTE_PLAINTEXT_SIZE + AEAD_TAG_SIZE;

Proposed changes:

We suggest converting these constants into new abstract types within the Domain trait: NotePlaintextBytes, NoteCiphertextBytes, CompactNotePlaintextBytes, and CompactNoteCiphertextBytes. These types would then be implemented in the orchard and sapling-crypto crates.

After the discussion of the first version of this PR, the following methods are also proposed to be added to the Domain trait to safely convert byte slices into these new types: parse_note_plaintext_bytes, parse_note_ciphertext_bytes, and parse_compact_note_plaintext_bytes.

Updated `Domain` trait:

pub trait Domain {
    type EphemeralSecretKey: ConstantTimeEq;
    type EphemeralPublicKey;
    type PreparedEphemeralPublicKey;
    type SharedSecret;
    type SymmetricKey: AsRef<[u8]>;
    type Note;
    type Recipient;
    type DiversifiedTransmissionKey;
    type IncomingViewingKey;
    type OutgoingViewingKey;
    type ValueCommitment;
    type ExtractedCommitment;
    type ExtractedCommitmentBytes: Eq + for<'a> From<&'a Self::ExtractedCommitment>;
    type Memo;

    // Types for variable note size handling:
    type NotePlaintextBytes: NoteBytes;
    type NoteCiphertextBytes: NoteBytes;
    type CompactNotePlaintextBytes: NoteBytes;
    type CompactNoteCiphertextBytes: NoteBytes;

    // New parsing methods for safe conversions:
    fn parse_note_plaintext_bytes(plaintext: &[u8]) -> Option<Self::NotePlaintextBytes>;
    fn parse_note_ciphertext_bytes(output: &[u8], tag: [u8; AEAD_TAG_SIZE]) -> Option<Self::NoteCiphertextBytes>;
    fn parse_compact_note_plaintext_bytes(plaintext: &[u8]) -> Option<Self::CompactNotePlaintextBytes>;

Here, NoteBytes is a helper trait designed to simplify and unify the definition and implementation of these new associated types.

Additionally, constants will be removed from function signatures since they are not known at compilation time. For example:

pub fn try_note_decryption<D: Domain, Output: ShieldedOutput<D, ENC_CIPHERTEXT_SIZE>>(...)

will be replaced with:

pub fn try_note_decryption<D: Domain, Output: ShieldedOutput<D>>(...)

Implementation:

We have provided our initial implementation, complemented by the appropriate changes in the orchard and sapling-crypto crates. See the following modules for details:

https://github.com/QED-it/orchard/blob/zsa1/src/note_encryption/domain.rs
https://github.com/QED-it/sapling-crypto/blob/zsa1/src/note_encryption.rs

Additionally, we made several minor updates in the zcash_primitives crate of QED-it's fork of the librustzcash repository to align it with the described changes in zcash_note_encryption, orchard, and sapling-crypto crates:

https://github.com/QED-it/librustzcash/tree/zsa1/zcash_primitives

* Copy updated src folder from librustzcash/zsa1 * Cherry-pick the latest update of zcash_note_encryption from zcash/librustzcash (it was missed in QED-it/librustzcash) * Upgrade Rust version * Upgrade Rust version to 1.65 * Add --features=alloc to command line for build-nodefault jon in ci.yml * Downgrade Rust version back to 1.61.0 --------- Co-authored-by: Dmitry Demin <[email protected]>

Co-authored-by: Dmitry Demin <[email protected]>

PaulLaux · 2024-04-30T17:09:51Z

Regarding the compile time vs runtime discussion, we currently set the array size during compile time in the implementation of the trait: https://github.com/QED-it/orchard/blob/a9af64c146bef38d7ac957983bc78c8a3f54f76e/src/note_encryption/note_encryption_zsa.rs#L12

impl OrchardDomain for OrchardZSA {
    const COMPACT_NOTE_SIZE: usize = COMPACT_NOTE_SIZE_ZSA;

    type NotePlaintextBytes = NoteBytes<{ Self::NOTE_PLAINTEXT_SIZE }>;
    type NoteCiphertextBytes = NoteBytes<{ Self::ENC_CIPHERTEXT_SIZE }>;
    type CompactNotePlaintextBytes = NoteBytes<{ Self::COMPACT_NOTE_SIZE }>;
    type CompactNoteCiphertextBytes = NoteBytes<{ Self::COMPACT_NOTE_SIZE }>;
...

…on (#6) * Update CHANGELOG.md with changes for note plaintext size generalization * Improve the description in CHANGELOG.md --------- Co-authored-by: Dmitry Demin <[email protected]>

Co-authored-by: Dmitry Demin <[email protected]>

str4d · 2024-06-25T12:02:12Z

src/lib.rs

+    type NotePlaintextBytes: AsMut<[u8]> + for<'a> From<&'a [u8]>;
+    type NoteCiphertextBytes: AsMut<[u8]> + for<'a> From<(&'a [u8], &'a [u8])>;
+    type CompactNotePlaintextBytes: AsMut<[u8]> + for<'a> From<&'a [u8]>;
+    type CompactNoteCiphertextBytes: AsRef<[u8]>;


It is incorrect to have From bounds on these types, as the conversions are necessarily fallible (the slices can be any length, and the types cannot) and this would force panics in the public API (not just in the internal trait API that we control both sides of). Additionally, NoteCiphertextBytes should not take the tag as a slice (we know its length).

Replace all of these with trait methods that "parse" the slices to get these, e.g. fn parse_note_plaintext_bytes(plaintext: &[u8]) -> Option<Self::NotePlaintextBytes>.

Done: added three new methods to Domain trait to parse note bytes.

fn parse_note_plaintext_bytes(plaintext: &[u8]) -> Option<Self::NotePlaintextBytes> fn parse_note_ciphertext_bytes(output: &[u8], tag: [u8; AEAD_TAG_SIZE]) -> Option<Self::NoteCiphertextBytes> fn parse_compact_note_plaintext_bytes(plaintext: &[u8]) -> Option<Self::CompactNotePlaintextBytes>

str4d

Reviewed b8bd2a1.

str4d · 2024-06-25T12:42:34Z

src/lib.rs

+    /// Exposes the note ciphertext of the output. Returns `None` if the output is compact.
+    fn enc_ciphertext(&self) -> Option<D::NoteCiphertextBytes>;


This no longer "exposes" the output, and instead forcibly clones it, which is undesirable in usages of ShieldedOutput outside of zcash_note_encryption. We should instead return a reference that the caller can clone if desired (by adding a Clone bound on `Domain::NoteCiphertextBytes):

Suggested change

/// Exposes the note ciphertext of the output. Returns `None` if the output is compact.

fn enc_ciphertext(&self) -> Option<D::NoteCiphertextBytes>;

/// Exposes the note ciphertext of the output.

///

/// Returns `None` if the output is only compact.

fn enc_ciphertext(&self) -> Option<&D::NoteCiphertextBytes>;

Also, document this change to the trait in the changelog.

We tried to change the code to return references but encountered a few technical challenges due to how NoteCiphertextBytes and CompactNoteCiphertextBytes are implemented in both the orchard and sapling-crypto crates. These types are defined and implemented as newtypes over fixed-size byte arrays:

type NoteCiphertextBytes = NoteBytesData<{ ENC_CIPHERTEXT_SIZE }>; type CompactNoteCiphertextBytes = NoteBytesData<{ COMPACT_NOTE_SIZE }>; pub struct NoteBytesData<const N: usize>(pub [u8; N]);

The issue is that these newtypes wrap arrays that are not directly stored as such in the structs implementing ShieldedOutput. Therefore, we cannot return a reference to them directly because they are constructed on-the-fly from these arrays.

A possible workaround is to change NoteCiphertextBytes and CompactNoteCiphertextBytes type implementations in orchard and sapling-crypto to wrap references instead of arrays, but we would need to adjust the Domain trait to allow lifetimes and define associated types as generic with lifetimes using Rust's Generic Associated Types (GAT):

trait Domain { type NoteCiphertextBytes<'a>; type CompactNoteCiphertextBytes<'a>; }

This approach would involve additional changes, impacting all Domain and ShieldedOutput implementations and adding complexity to the codebase. If using references is crucial, we can proceed with this strategy though.

After discussion in the ZSA meeting, we will make my suggested change. If an action is non-compact, we know it is possible to get a reference to the appropriate type, and the data is large enough that we don't want to force clones.

Done: now enc_ciphertext returns Option of a reference.

src/lib.rs

str4d · 2024-06-25T12:48:12Z

src/lib.rs

-        output[NOTE_PLAINTEXT_SIZE..].copy_from_slice(&tag);
-
-        output
+        D::NoteCiphertextBytes::from((output, tag.as_ref()))


Once the Froms are replaced, this becomes

Suggested change

D::NoteCiphertextBytes::from((output, tag.as_ref()))

D::parse_note_ciphertext_bytes(output, tag).expect("output is correct length")

the unwrap here being fine because we know that output is derived from D::NotePlaintextBytes, which we can assume is the correct size relative to D::NoteCiphertextBytes.

Non-blocking: I have ideas on how to make this infallible in a type-safe way, but I'll look at that refactor after this PR lands.

Done (added and used parse.. method).

str4d · 2024-06-25T12:52:26Z

src/lib.rs

+    fn extract_memo(
+        &self,
+        plaintext: &Self::NotePlaintextBytes,
+    ) -> (Self::CompactNotePlaintextBytes, Self::Memo);


Hmm, I dislike this change, in part because we want to move away from the notion of compact memo representations in future NUs. But looking at how it is used in this crate, I think it's fine for now if we rename it:

Suggested change

fn extract_memo(

&self,

plaintext: &Self::NotePlaintextBytes,

) -> (Self::CompactNotePlaintextBytes, Self::Memo);

fn split_plaintext_at_memo(

&self,

plaintext: &Self::NotePlaintextBytes,

) -> (Self::CompactNotePlaintextBytes, Self::Memo);

Done: renamed to split_plaintext_at_memo.

str4d · 2024-06-25T12:55:05Z

src/lib.rs

    ) -> Option<(Self::Note, Self::Recipient)>;

-    /// Extracts the memo field from the given note plaintext.
+    /// Splits the memo field from the given note plaintext.


Paired with the method rename:

Suggested change

/// Splits the memo field from the given note plaintext.

/// Splits the given note plaintext into the compact part (containing the note) and

/// the memo field.

str4d · 2024-06-25T12:56:16Z

src/lib.rs

-/// The size of a compact note.
-pub const COMPACT_NOTE_SIZE: usize = 1 + // version
-    11 + // diversifier
-    8  + // value
-    32; // rseed (or rcm prior to ZIP 212)
-/// The size of [`NotePlaintextBytes`].
-pub const NOTE_PLAINTEXT_SIZE: usize = COMPACT_NOTE_SIZE + 512;


Either document these removals in the changelog, or (my preference) undo these removals (the constants are still relevant in downstream crates) and alter their docstrings to say that these are pre-ZSA.

Done: documented in the changelog.

These constants applicable to Sapling and Orchard but not OrchardZSA.
We believe keeping the constants here will create a confusion in the future and propose to remove them.
Each implementation should provide these.

src/lib.rs

…rom ShieldedOutput methods)

) * Attempt to resolve review issues for zcash/pull/2 * NoteBytes moved to zcash_note_encryption (copy of #8) (#9) * Add NoteBytes * Implement concat manually for wasm * Fix slice bounds in concat * Fmt * Split interface and implementation of NoteBytes * Add new method to NoteBytes --------- Co-authored-by: alexeykoren <[email protected]> * Resolve review issues for zcash/pull/2 (except returning references from ShieldedOutput methods) * Fix cargo doc issue * Fix based on feedback from PR #10 review * Make split_ciphertext_at_tag a method of ShieldedOutput with minor refactoring --------- Co-authored-by: Dmitry Demin <[email protected]> Co-authored-by: alexeykoren <[email protected]>

Co-authored-by: Dmitry Demin <[email protected]>

* Refactor enc_ciphertext to return reference instead of copy These changes were discussed and suggested in PR zcash_note_encryption#2 * Remove extra spaces in rust-toolchain.toml * Restore the original order of const definition to reduce PR diff * Fix the comment for split_plaintext_at_memo * Fix docstring for NOTE_PLAINTEXT_SIZE * Update CHANGELOG * Remove unused constants COMPACT_NOTE_SIZE, NOTE_PLAINTEXT_SIZE, ENC_CIPHERTEXT_SIZE, and update CHANGELOG accordingly * Minor improvement in CHANGELOG.md --------- Co-authored-by: Dmitry Demin <[email protected]>

Co-authored-by: Dmitry Demin <[email protected]>

…hertext (#112) This PR updates the `ShieldedOutput` implementation for the `Action`/`CompactAction` struct to align with the recent changes in the `zcash_note_encryption` crate. Specifically, the `enc_ciphertext` method now returns a reference instead of a copy. This change was discussed and suggested in PR zcash/zcash_note_encryption#2 review. --------- Co-authored-by: Dmitry Demin <[email protected]>

str4d

utACK 76745f0 with @daira, and comments to be addressed after merge.

str4d · 2025-01-21T21:31:52Z

src/lib.rs

+    type NotePlaintextBytes: NoteBytes;
+    type NoteCiphertextBytes: NoteBytes;
+    type CompactNotePlaintextBytes: NoteBytes;
+    type CompactNoteCiphertextBytes: NoteBytes;


Note to self: with Memo Bundles, we won't be doing compact note decryption anymore. It would be nice to not need these associated types, but we can also just set them to be uninhabitable, or identical to the non-compact types, for NU7.

str4d · 2025-01-21T21:37:03Z

src/lib.rs

+    fn split_plaintext_at_memo(
+        &self,
+        plaintext: &Self::NotePlaintextBytes,
+    ) -> Option<(Self::CompactNotePlaintextBytes, Self::Memo)>;


The API here is a bit weird in maybe not returning compact note bytes, given that we should always be able to produce them from non-compact note bytes. However, I need to see how this is used downstream before deciding what to do here, in particular with the context of Memo Bundles (which is what the comment on lines 263-264 is referring to). It might be that after the Memo Bundle changes, we can revert this change and go back to having extract_memo, but we can figure that out after this PR.

str4d · 2025-01-21T21:41:26Z

src/lib.rs

+    fn enc_ciphertext_compact(&self) -> D::CompactNoteCiphertextBytes;
+
+    //// Splits the AEAD tag from the ciphertext.
+    fn split_ciphertext_at_tag(&self) -> Option<(D::NotePlaintextBytes, [u8; AEAD_TAG_SIZE])> {


I'm not entirely happy about using an associated type named NotePlaintextBytes as a container for ciphertext bytes. If the intention is to use it for either, it should be named appropriately; otherwise we should have separate associated types that can happen to be identical in a concrete trait impl if it makes sense.

str4d · 2025-01-21T21:46:34Z

src/lib.rs

+        let (plaintext, tail) = enc_ciphertext_bytes
+            .len()
+            .checked_sub(AEAD_TAG_SIZE)
+            .map(|tag_loc| enc_ciphertext_bytes.split_at(tag_loc))?;


I think this would be a programming error if it returned None, because that would imply D::NoteCiphertextBytes was set to a type that can represent too short of a byte array. We should instead just panic here (as there's nothing the end user can do; it's the trait implementer's bug):

Suggested change

let (plaintext, tail) = enc_ciphertext_bytes

.len()

.checked_sub(AEAD_TAG_SIZE)

.map(|tag_loc| enc_ciphertext_bytes.split_at(tag_loc))?;

let tag_loc = enc_ciphertext_bytes

.len()

.checked_sub(AEAD_TAG_SIZE)

.expect("D::CompactNoteCiphertextBytes should be at least AEAD_TAG_SIZE bytes");

let (plaintext, tail) = enc_ciphertext_bytes.split_at(tag_loc);

str4d · 2025-01-21T21:50:39Z

src/lib.rs

+
+        let tag: [u8; AEAD_TAG_SIZE] = tail.try_into().expect("the length of the tag is correct");
+
+        D::parse_note_plaintext_bytes(plaintext).map(|plaintext| (plaintext, tag))


Here also I think a panic would be correct. Inside D::parse_note_plaintext_bytes we don't know if plaintext is the correct length, so it needs to be fallible. Here however, we know we obtained plaintext from a D::NoteCiphertextBytes, and it would therefore be a programming error if that type was inconsistent with D::NotePlaintextBytes. The only case fallibility would matter here is if D::parse_note_plaintext_bytes did some type-checking, but AFAIK it is only checking the length, and the actual parsing into protocol-specific types happens in a separate method.

After that change, None would now just mean "this type has no full note ciphertext".

Suggested change

D::parse_note_plaintext_bytes(plaintext).map(|plaintext| (plaintext, tag))

Some((

D::parse_note_plaintext_bytes(plaintext)

.expect("D::NoteCiphertextBytes and D::NotePlaintextBytes should be consistent"),

tag,

))

str4d · 2025-01-22T01:46:55Z

Replaced by #6.

dmidem and others added 2 commits December 19, 2023 20:08

Refactor/modify lib.rs to avoid Vec usage and fix CI build failure (#3)

8b6b31d

Co-authored-by: Dmitry Demin <[email protected]>

PaulLaux mentioned this pull request Apr 30, 2024

zcash_note_encryption generalization zcash/librustzcash#746

Closed

dmidem and others added 2 commits May 16, 2024 16:55

Update CHANGELOG.md with changes for note plaintext size generalizati…

1ca015a

…on (#6) * Update CHANGELOG.md with changes for note plaintext size generalization * Improve the description in CHANGELOG.md --------- Co-authored-by: Dmitry Demin <[email protected]>

Revert MSRV to 1.56.1 (#5)

b8bd2a1

Co-authored-by: Dmitry Demin <[email protected]>

str4d reviewed Jun 25, 2024

View reviewed changes

str4d requested changes Jun 25, 2024

View reviewed changes

dmidem added a commit to QED-it/zcash_note_encryption that referenced this pull request Jul 25, 2024

Attempt to resolve review issues for zcash/pull/2

d0c9073

dmidem added a commit to QED-it/zcash_note_encryption that referenced this pull request Jul 30, 2024

Resolve review issues for zcash/pull/2 (except returning references f…

5bf1634

…rom ShieldedOutput methods)

This was referenced Jul 30, 2024

Resolve Issues from PR #2 in zcash/zcash_note_encryption repository QED-it/zcash_note_encryption#10

Merged

adjust to zcash_note_encryption changes QED-it/sapling-crypto#2

Merged

dmidem mentioned this pull request Jul 30, 2024

Synchronize Orchard with updates from zcash_note_encryption for zcash PR #2 issues resolve QED-it/orchard#111

Merged

Implement new parse_... methods directly in the Domain trait (#12)

5838455

Co-authored-by: Dmitry Demin <[email protected]>

dmidem and others added 2 commits August 12, 2024 21:16

Restore visibility for AEAD_TAG_SIZE const (#15)

76745f0

Co-authored-by: Dmitry Demin <[email protected]>

str4d approved these changes Jan 21, 2025

View reviewed changes

str4d mentioned this pull request Jan 22, 2025

Add generalization to support OrchardZSA and AssetBase #6

Merged

str4d closed this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: note plaintext size generalization #2

Feature: note plaintext size generalization #2

PaulLaux commented Apr 30, 2024 •

edited

Loading

PaulLaux commented Apr 30, 2024

str4d Jun 25, 2024 •

edited

Loading

dmidem Aug 19, 2024 •

edited

Loading

str4d left a comment

str4d Jun 25, 2024

dmidem Aug 6, 2024 •

edited

Loading

str4d Aug 6, 2024

dmidem Aug 19, 2024

str4d Jun 25, 2024

dmidem Aug 19, 2024

str4d Jun 25, 2024

dmidem Aug 19, 2024

str4d Jun 25, 2024

dmidem Aug 19, 2024

str4d Jun 25, 2024

dmidem Aug 19, 2024

PaulLaux Aug 20, 2024

str4d left a comment

str4d Jan 21, 2025

str4d Jan 21, 2025

str4d Jan 21, 2025

str4d Jan 21, 2025

str4d Jan 21, 2025

str4d commented Jan 22, 2025

		/// Exposes the note ciphertext of the output. Returns `None` if the output is compact.
		fn enc_ciphertext(&self) -> Option<D::NoteCiphertextBytes>;

	D::NoteCiphertextBytes::from((output, tag.as_ref()))
	D::parse_note_ciphertext_bytes(output, tag).expect("output is correct length")

	/// Splits the memo field from the given note plaintext.
	/// Splits the given note plaintext into the compact part (containing the note) and
	/// the memo field.


		let tag: [u8; AEAD_TAG_SIZE] = tail.try_into().expect("the length of the tag is correct");

		D::parse_note_plaintext_bytes(plaintext).map(\|plaintext\| (plaintext, tag))

Feature: note plaintext size generalization #2

Feature: note plaintext size generalization #2

Conversation

PaulLaux commented Apr 30, 2024 • edited Loading

Current state in zcash_note_encryption:

Proposed changes:

Updated Domain trait:

Implementation:

PaulLaux commented Apr 30, 2024

str4d Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

dmidem Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmidem Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Jan 22, 2025

PaulLaux commented Apr 30, 2024 •

edited

Loading

Current state in `zcash_note_encryption`:

Updated `Domain` trait:

str4d Jun 25, 2024 •

edited

Loading

dmidem Aug 19, 2024 •

edited

Loading

dmidem Aug 6, 2024 •

edited

Loading