OrchardZSA backward compatability 0.8.0 #104

dmidem · 2024-05-13T08:02:14Z

This pull request aims to generalize the implementation of the Orchard protocol, providing backward compatibility to support both the new (ZSA) and the original (non-ZSA - Vanilla) Orchard protocol variants. Key modifications and enhancements include:

1. Trait and Generic Structures for Note Encryption:

OrchardDomain Trait: A new OrchardDomain trait in note_encryption.rs differentiates between the original Orchard (Vanilla) and Orchard ZSA protocols, simplifying the implementation of the Domain trait through abstraction.
OrchardDomainBase Generic Struct: Introduced to contain data used for internal calculations in both Orchard variants.
TransmittedNoteCiphertext Modifications: This struct is now generic, supporting various lengths of encrypted note ciphertext to accommodate both Orchard variants.

2. Trait and Generic Structures for Circuit Generalization:

OrchardCircuit Trait: A new OrchardCircuit trait in circuit.rs provides an interface for different implementations of the PLONK circuit tailored to the specific requirements of the Orchard protocol's variants (Vanilla and ZSA).
OrchardCircuitBase Generic Struct: Contains data for internal calculations across both Orchard variants.

3. Module Organization:

Introduction of note_encryption_vanilla.rs, note_encryption_zsa.rs to support the different types of encrypted notes.
Introduction of circuit_vanilla.rs, and circuit_zsa.rs to support various circuit configurations.

4. Test Suite Updates:

Updates to unit tests include separate versions for Vanilla and ZSA variants, ensuring thorough validation of the modifications.

5. Dependency Adaptation:

The adoption of a modified version of the Halo2 Rust crate facilitates support for both Orchard protocol variants, guaranteeing that all tests, including those for non-ZSA functionality, pass successfully.

…onality - Introduced OrchardDomain trait in note_encryption.rs to abstract differences between Orchard and Orchard ZSA. - Implemented Domain for new generic struct OrchardType<D: OrchardDomain>. - Added Rust macro define_note_byte_types to simplify type definitions for OrchardDomain. - Modified TransmittedNoteCiphertext struct to be generic, accommodating different ciphertext lengths. - Reverted module names to note_encryption_v2.rs and note_encryption_v3.rs for clarity in representing Orchard versions. - Updated the rest of orchard_zsa codebase and unit tests to align with these generalizations.

… (currently supports ZSA only)

…, to use them from librustzcash and support tests of vanilla and zsa circuits

1. Separated action-related code from note_encryption.rs into a distinct module (sub-module within note_encryption.rs). 2. Integrated note_encryption_vanilla.rs and note_encryption_zsa.rs as sub-modules of note_encryption.rs instead of lib.rs. 3. Renamed OrchardType to OrchardDomainContext for clarity. 4. Eliminated the define_note_byte_types macro in note_encryption.rs, opting to extend OrchardDomain's generics instead. 5. Expanded OrchardDomain's generics to include NOTE_PLAINTEXT_SIZE and ENC_CIPHERTEXT_SIZE constants. 6. Introduced and utilized NoteBytes generics to abstract array wrapping and define domain-specific types like NotePlaintextBytes and NoteCiphertextBytes in note_encryption_vanilla.rs and note_encryption_zsa.rs.

…zsa to avoid duplication

…circuit::Circuit<D>: plonk::Circuit' clauses.

…ytes

…finitions into separate modules in note_encryption subfolder

…te zero-filled enc_ciphertext in proptests of action.rs

…InitChip types, use true/false configure arg instead)

…rcuits

…nilla and ZSA circuits

…tead of the associated function

PaulLaux

Added some relatively minor comments. In addition:

in benches/.. please parametrize every benchmark group label with Orchard or OrchardZSA according to the relevant case. For example in benched/note_encryption we need:

let mut group = c.benchmark_group("note-decryption");

to be replaced with Orchard note-decryption and OrchardZSA note-decryption according to the performed case.
This is needed to properly display and separate the results.
see results using cargo bench and make sure all benchmark tests are executed.
same for /circuit. My advice is to avoid new generics and use functions.

src/note_encryption/compact_action.rs

src/note_encryption/orchard_domain_vanilla.rs

PaulLaux · 2024-06-26T15:44:11Z

src/builder.rs

@@ -620,10 +633,12 @@ impl Builder {
    ///
    /// The returned bundle will have no proof or signatures; these can be applied with
    /// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively.
-    pub fn build<V: TryFrom<i64>>(
+    // FIXME: Consider factoring parts of the return type into `type` definitions


it it still here ?

benches/note_decryption.rs

PaulLaux · 2024-07-03T11:46:28Z

src/action.rs

+    /// `ActionArb` adapts `arb_...` functions for both Vanilla and ZSA Orchard protocol variations
+    /// in property-based testing, addressing proptest crate limitations.


Remove trivial comment

The comment is required by cargo doc here because ActionArb is a public struct.

src/orchard_flavors.rs

PaulLaux · 2024-07-04T08:14:36Z

src/orchard_flavors.rs

+#[derive(Debug, Clone, Default)]
+pub struct OrchardZSA;
+
+/// A trait binding the common functionality between different Orchard protocol variations


A trait binding the common functionality between different Orchard protocol flavors.

tests/builder.rs

PaulLaux · 2024-07-04T08:18:13Z

tests/builder.rs

+
+        let mut builder = Builder::new(
+            BundleType::Transactional {
+                // FIXME: Should we use SPENDS_DISABLED_WITH_ZSA for FL=OrchardZSA case?


Let's keep SPENDS_DISABLED_WITHOUT_ZSA here since we use SPENDS_DISABLED_WITH_ZSA above.

Got it, removed the FIXME.

tests/builder.rs

…rning

…rt in constants.rs

…ts instead of adding them

… without converting burn values

…tion and circuit benchmarks

PaulLaux

Good, except benchmark for decryption does not run

PaulLaux · 2024-07-09T09:55:38Z

src/builder.rs

@@ -900,6 +903,7 @@ pub struct InProgress<P, S: InProgressSignatures> {

 impl<P, S: InProgressSignatures> InProgress<P, S> {
    /// Changes this authorization from one proof type to another.
+    // FIXME: Change comment to "mutate the proof using the provided function"


PaulLaux · 2024-07-09T10:02:37Z

benches/note_decryption.rs

@@ -15,7 +15,11 @@ use zcash_note_encryption_zsa::{batch, try_compact_note_decryption, try_note_dec
 #[cfg(unix)]
 use pprof::criterion::{Output, PProfProfiler};

-fn bench_note_decryption<FL: OrchardFlavor>(c: &mut Criterion) {
+mod utils;


When running cargo bench, I see tests for proving and verifying for Orchard and OrchardZSA.
I don't see the decryption tests. Are you sure it is activated?

…lla branches - Added `extract_asset` method to `OrchardDomainCommon` trait to get the asset base from the note plaintext. - For `OrchardVanilla`, the method extracts native assets. - For `OrchardZSA`, the method extracts ZSA assets. - Transformed `parse_note_version` to `validate_note_version` function that checks if the version matches the expected version for the domain and returns a boolean. - Updated `parse_note_plaintext_without_memo` to validate the note version using `validate_note_version` before processing the plaintext.

…zcash)

PaulLaux

One more fix is required, not "baking" the version byte into the domain

PaulLaux · 2024-07-15T11:06:33Z

benches/note_decryption.rs

@@ -28,6 +28,8 @@ fn bench_note_decryption<FL: OrchardFlavorBench>(c: &mut Criterion) {
    let recipient = valid_ivk.address_at(0u32);
    let valid_ivk = PreparedIncomingViewingKey::new(&valid_ivk);

+    // FIXME: should we add to the following comment that now for ZSA flavor the early
+    // rejection also happens when the asset is invalid?


Got it. Removed the FIXME comment.

PaulLaux · 2024-07-15T11:06:52Z

src/builder.rs

@@ -584,7 +584,7 @@ impl Builder {
    }

    /// Add an instruction to burn a given amount of a specific asset.
-    // FIXME: Should we add `add_burn` into `testing` (into `arb_bundle` function)
+    // FIXME: Should we add `add_burn` into the `into_bundle` function in this testing module


no, I opened a new task for later

Got it. Removed the FIXME comment.

PaulLaux · 2024-07-15T11:19:45Z

src/note_encryption/orchard_domain.rs

+    /// The note version byte.
+    const NOTE_VERSION_BYTE: u8;
+


It is not working. we should not bake the version byte as part of the domain.

The version is a property of the note, not the encryption.

PaulLaux · 2024-07-15T11:21:41Z

src/note_encryption/domain.rs

+/// Checks if the version of the note plaintext matches the expected version for the domain.
+pub(super) fn validate_note_version<D: OrchardDomainCommon>(
+    plaintext: &D::CompactNotePlaintextBytes,
+) -> bool {
+    plaintext
+        .as_ref()
+        .first()
+        .map_or(false, |version| *version == D::NOTE_VERSION_BYTE)


We need to restore parse_note_version

Even if now we don't support other versions today, the encryption/decryption need to be version agnostic.

@PaulLaux , I still have a question.

Inside the parse_note_plaintext_without_memo function, we need to extract the asset from the plaintext. We know three facts: the Orchard flavor (Vanilla or ZSA), the compact note size, and the note version byte. Which criteria should we use to decide how to extract the asset: either use the native asset or extract it from the plaintext? We can't rely on the note version byte only and ignore the size of the plaintext, like we did before, otherwise we get out-of-bounds panics.

On the other hand, to extract the asset, we can ignore the version byte value and use the Orchard flavor or compact note size alone to decide how we extract the asset. This is the way I implemented it now (with additional note version validation, which has nothing to do with asset extraction).

Do you mean we need to use a combination of the note version byte and the flavor/plaitext size? Like this:

let version = parse_note_version(plaintext.as_ref())?; let asset = match version { NOTE_VERSION_BYTE_V2 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_VANILLA => AssetBase::native(), NOTE_VERSION_BYTE_V3 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_ZSA => { let bytes = plaintext.as_ref()[COMPACT_NOTE_SIZE_VANILLA..COMPACT_NOTE_SIZE_ZSA] .try_into() .unwrap(); AssetBase::from_bytes(bytes).into() }, _ => return None, }

In this case, we don't need the new extract_asset methods of the OrchardDomainCommon I added. Or we can use extract_asset this way, for example:

let version = parse_note_version(plaintext.as_ref())?; let asset = match version { NOTE_VERSION_BYTE_V2 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_VANILLA => D::extract_asset(plaintext)?, NOTE_VERSION_BYTE_V3 if D::COMPACT_NOTE_SIZE == COMPACT_NOTE_SIZE_ZSA => D::extract_asset(plaintext)?, _ => return None, }

when activating parse_note_plain_text_without_memo we already know what to expect so just:

let asset = D::extract_asset(plaintext)?;

We already chosen the D::.

Except regular error handling / propagation inside the function, nothing else is needed.

…ciated constant

PaulLaux

Very nice

PaulLaux and others added 30 commits April 9, 2024 12:44

draft2

8bee528

draft3

5f78e72

draft4

5221c29

draft5

e07fab9

draft6

ee0a07d

Minor changes in the doc comments

b177325

Add tests for note_encryption_v2

4900519

Rename V2 name suffixes to Vanilla, and V3 - to ZSA

0a3d231

Make the Circuit struct generic to support different Orchard variants…

f3ae948

… (currently supports ZSA only)

Continue Circuit generalization

fc64461

Split circuit implementation into circuit_vanilla and circuit_zsa

c6f8936

Fix to support modified halo2

ce46de0

Add missed fields to fn print_action_circuit

e5ab5d0

Pin half crate to 1.8.2 (to resolve MSRV conflict)

a8dadc9

Pin half crate to 1.8.2 for dev deps too (to resolve MSRV conflict)

a8a72c2

Pin half crate to 2.2.1

120b090

Fix cargo clippy errors

5d5bbf1

Convert arb_... testing functions to methods of dummy generict struct…

97f55f9

…, to use them from librustzcash and support tests of vanilla and zsa circuits

Make hash_bundle_txid_data function backwards compatible

90512c8

Fix cargo clippy errors

a1e6cd6

Minor fix (make action module pub)

8b28a0e

Make add_chip.rs a shared module between circuit_vanilla and circuit_…

fc20a5a

…zsa to avoid duplication

Introduce OrcharcCircuit trait to eliminate repetitive 'where crate::…

1d68e0b

…circuit::Circuit<D>: plonk::Circuit' clauses.

Introdice NoteByteReader and NoteByteWriter traiots to use with NoteB…

c820dae

…ytes

note_encryption: extract Domain impl, OrchardDoimain and NoteBytes de…

088494a

…finitions into separate modules in note_encryption subfolder

Use vec with a proper length for the concrete OrchardDomain to genera…

46a222e

…te zero-filled enc_ciphertext in proptests of action.rs

Use try_fold instead of fold when cargo clippy suggests it

367c701

Rename domain_impl.rs in note_encryption to domain.rs

dc97b7a

dmidem added 9 commits July 1, 2024 10:12

Fox cargo fmt issues

1363d47

Fix generic arg name compilation error in benches

c05d06e

Fix halo2 Sinsemilla amd Merke type chip names (remove ...WithPrivate…

1de980f

…InitChip types, use true/false configure arg instead)

Generalize test in commit_ivk to test against both Vanilla and ZSA ci…

4155972

…rcuits

Generalize builder::tests::shielding_bundle to run it against both Va…

060bf95

…nilla and ZSA circuits

Remove unnecessary FIXMEs

562dcf0

Refactor burn items to use NoteValue

27c03b4

Use associated boolean const in MyLookup trait of commit_ivk test ins…

aa46d52

…tead of the associated function

Add test case to ensure spend adding fails when spends are disabled

1bffe59

PaulLaux reviewed Jul 4, 2024

View reviewed changes

dmidem added 8 commits July 5, 2024 16:49

Address the latest PR #104 review comments

752feb1

Add UnauthorizedBundleWithMetadata type alias to get rid of clippy wa…

f45db57

…rning

Return doc comment for ActionArb in action.rs and removed unused impo…

b7b780c

…rt in constants.rs

Lock halo2 dependencies to specific commit hash

82e19da

Fix issues in bundle auth data commitment and note digest generation

1e54e19

Return BuildError (BurnDuplicateAsset) in add_burn for duplicate asse…

3efb12c

…ts instead of adding them

Modify bundle::try_map_value_balance fn to convert only value_balance…

80c7574

… without converting burn values

Parameterized benchmark group labels with flavor names in note_encryp…

b72152a

…tion and circuit benchmarks

PaulLaux reviewed Jul 9, 2024

View reviewed changes

dmidem added 6 commits July 11, 2024 11:38

Fix 'cargo clippy --tests' warnings

0c3742a

Make OrchardDomainCommon pub

f0c40f2

Make from_bytes/to_bytes methods of NoteValue pub (needed for librust…

c4baaa7

…zcash)

Minor fix of comments

2d7e2a3

Minor update in FIXME

de85554

PaulLaux reviewed Jul 15, 2024

View reviewed changes

dmidem added 2 commits July 15, 2024 13:25

Removed extra FIXMEs

b20ff2f

Restore parse_note_version function and remove NOTE_VERSION_BYTE asso…

4aff14d

…ciated constant

PaulLaux approved these changes Jul 16, 2024

View reviewed changes

dmidem merged commit 39b479e into zsa1 Jul 16, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OrchardZSA backward compatability 0.8.0 #104

OrchardZSA backward compatability 0.8.0 #104

dmidem commented May 13, 2024 •

edited by PaulLaux

Loading

PaulLaux left a comment

PaulLaux Jun 26, 2024

PaulLaux Jul 3, 2024

dmidem Jul 4, 2024 •

edited

Loading

PaulLaux Jul 4, 2024

dmidem Jul 5, 2024

PaulLaux Jul 4, 2024

dmidem Jul 5, 2024

PaulLaux left a comment

PaulLaux Jul 9, 2024

PaulLaux Jul 9, 2024

PaulLaux left a comment

PaulLaux Jul 15, 2024

dmidem Jul 15, 2024 •

edited

Loading

PaulLaux Jul 15, 2024

dmidem Jul 15, 2024 •

edited

Loading

PaulLaux Jul 15, 2024

PaulLaux Jul 15, 2024

PaulLaux Jul 15, 2024

dmidem Jul 15, 2024 •

edited

Loading

PaulLaux Jul 15, 2024 •

edited

Loading

PaulLaux left a comment

		/// `ActionArb` adapts `arb_...` functions for both Vanilla and ZSA Orchard protocol variations
		/// in property-based testing, addressing proptest crate limitations.

OrchardZSA backward compatability 0.8.0 #104

OrchardZSA backward compatability 0.8.0 #104

Conversation

dmidem commented May 13, 2024 • edited by PaulLaux Loading

1. Trait and Generic Structures for Note Encryption:

2. Trait and Generic Structures for Circuit Generalization:

3. Module Organization:

4. Test Suite Updates:

5. Dependency Adaptation:

PaulLaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmidem Jul 4, 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

PaulLaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulLaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmidem Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmidem Jul 15, 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

dmidem Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

PaulLaux Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

PaulLaux left a comment

Choose a reason for hiding this comment

dmidem commented May 13, 2024 •

edited by PaulLaux

Loading

dmidem Jul 4, 2024 •

edited

Loading

dmidem Jul 15, 2024 •

edited

Loading

dmidem Jul 15, 2024 •

edited

Loading

dmidem Jul 15, 2024 •

edited

Loading

PaulLaux Jul 15, 2024 •

edited

Loading