Nu7 implementation #65

alexeykoren · 2024-06-26T16:08:16Z

ZSA is implemented across all the codebase, including:

TransactionData now contains 2 extra components - OrchardZsaBundle and IssueBundle
Serialization/deserialization of the new components
Digests of the new components are taken into account during calculation of sighash/txid.
TODO: we are waiting for test vectors and small ordering fixes to be merged to be able to check txid calculation
Nu7 network upgrade is introduced, although currently "nu6" zcash_unstable flag is used to separate ZSA from regular build. All nu7-related "nu6" flags are annotated with /* TODO nu7 */ comment
Builder methods for ZSAs
Wallet-related functionality (zcash_client_backend, zcash_client_sqlite) are disabled since they do not support ZSAs

PaulLaux

added some minor comments but I have the felling the the diff is incorrect, maybe because all the upstream commits that should not be there.

PaulLaux · 2024-07-17T08:41:27Z

.github/workflows/ci.yml

+#        run: rustup target add ${{ matrix.target }}
+#      - name: Build for target
+#        working-directory: ./ci-build
+#        run: cargo build --verbose --target ${{ matrix.target }}


why comment out?

We decided not to touch zcah_client_xxx crates for now and this target requires them

so let's remove just the relevant lines:

name: Add zcash_client_backend as a dependency of the synthetic crate working-directory: ./ci-build run: cargo add --features lightwalletd-tonic --path ../crates/zcash_client_backend

Seems that it can work

Cargo.toml

components/zcash_protocol/src/consensus.rs

supply-chain/config.toml

zcash_client_backend/src/proto/compact_formats.rs

zcash_primitives/proptest-regressions/transaction/tests.txt

zcash_client_sqlite/src/testing.rs

zcash_primitives/src/transaction/builder.rs

…te (which supports Vanilla and ZSA) - but for now zcash_primitives is not generalised itself and pinned to Vanilla only

This PR updates `librustzcash` to accommodate the recent refactoring changes in the `orchard` crate. The specific changes include: - Renamed all references from `orchard_flavors` to `orchard_flavor` to match the new module name. - Updated the `OrchardDomain` trait references to `OrchardDomainCommon` to reflect the trait renaming. - Adjusted the `Bundle` structure to use the fixed `NoteValue` type for the `burn` field, replacing the previous generic argument. - Refactored associated code to align with the new structure and naming conventions in the `orchard` crate. - Switched from the `orchardzsa-backward-compatibility-0.8.0-ak` to `orchardzsa-backward-compatibility-0.8.0` branch in `Cargo.toml` to use the latest version of the `orchard` crate. --------- Co-authored-by: Dmitry Demin <[email protected]>

) This PR makes the following changes: - reordering of the component fields in `read_action` and `write_action` in issuance in order to make them spec compliant. - It sets some of the constant values for NU7 in a way that is consistent with the generated test vectors from the Python reference implementation (See QED-it/zcash-test-vectors#22 for the changes to the Python reference implementation). - Some changes are made to function names to reflect the shift from preparing ZSAs for V7 rather than V6. - There are changes to the tests to use the `zcash_unstable` flag in order to cover both ZSA and vanilla Orchard behavior. - The test vectors are also expanded to include both the V5 vectors, and the V7 vectors (behind a `zcash_unstable` flag).

PaulLaux

good start, added some comments, mostly around builder.
In addition:

fix diff and redundant commits in the list
rename txv7 and v7 to txv6 and v6 respectively.

components/zcash_protocol/src/consensus.rs

components/zcash_protocol/src/local_consensus.rs

zcash_client_backend/src/scanning.rs

zcash_primitives/src/transaction/builder.rs

zcash_primitives/src/transaction/mod.rs

PaulLaux · 2024-07-24T20:14:42Z

zcash_primitives/src/transaction/mod.rs

+                "Sprout components cannot be present when serializing to the V7 transaction format.",
+            ));
+        }
+        self.write_v5_header(&mut writer)?;


rename to write_header()

It's their naming, shall we change it anyway?

well yes, the functionality is now extended to v6

PaulLaux · 2024-07-24T20:14:58Z

zcash_primitives/src/transaction/mod.rs

+        }
+        self.write_v5_header(&mut writer)?;
+        self.write_transparent(&mut writer)?;
+        self.write_v5_sapling(&mut writer)?;


rename to write_sapling()

well yes, the functionality is now extended to v6

PaulLaux · 2024-07-24T20:15:55Z

zcash_primitives/src/transaction/mod.rs

+            _orchard_zsa_bundle in orchard_testing::arb_zsa_bundle_for_version(version),
+            _issue_bundle in issuance::testing::arb_bundle_for_version(version),


why the _?

in these proptests I'm not sure its possible to hide it behind feature flag, so I want avoid clippy bitching at unused arguments for non-nu6 builds

zcash_primitives/src/transaction/mod.rs

PaulLaux

added some comments. Most impotently try to stick to orchard::issuance api

PaulLaux · 2024-08-12T16:48:47Z

components/zcash_protocol/src/consensus.rs

+    #[allow(unused_mut)]
+    let mut upper = None;
+    #[cfg(zcash_unstable = "nu6")]
+    {


sure we need the scope? (x3)

Yeah, looks ugly af, but I'm not sure how to do it better. Open to ideas...

PaulLaux · 2024-08-12T16:51:24Z

zcash_primitives/src/transaction/builder.rs

-use super::components::amount::NonNegativeAmount;
-use super::components::sapling::zip212_enforcement;


Let's try to avoid moving (to line 41) if not needed.

Found them a better place

PaulLaux · 2024-08-12T18:08:53Z

zcash_primitives/src/transaction/mod.rs

@@ -345,7 +403,13 @@ impl<A: Authorization> TransactionData<A> {
        transparent_bundle: Option<transparent::Bundle<A::TransparentAuth>>,
        sprout_bundle: Option<sprout::Bundle>,
        sapling_bundle: Option<sapling::Bundle<A::SaplingAuth, Amount>>,
-        orchard_bundle: Option<orchard::Bundle<A::OrchardAuth, Amount>>,
+        orchard_bundle: Option<orchard::Bundle<A::OrchardAuth, Amount, OrchardVanilla>>,
+        #[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )] orchard_zsa_bundle: Option<


please make sure a new line after the flag (all occurrences )

I'd be happy but cargo fmt disagrees

PaulLaux · 2024-08-12T18:10:41Z

zcash_primitives/src/transaction/mod.rs

+        #[cfg(zcash_unstable = "nu6")]
+        /* TODO nu7 */


please be consistent about the flag syntax

yup, weird, fixed

PaulLaux · 2024-08-12T18:11:02Z

zcash_primitives/src/transaction/mod.rs

+            A::OrchardZsaAuth,
+            B::OrchardZsaAuth,
+        >,
+        #[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )] f_issue: impl issuance::MapIssueAuth<


PaulLaux · 2024-08-12T18:47:48Z

zcash_primitives/src/transaction/builder.rs

+                    let (bundle, meta) = builder
+                        .build(&mut rng)
+                        .map_err(Error::OrchardBuild)?
+                        .unwrap();


should be outside of if (before)

If we do that Rust will infer OrchardVanilla for the bundle

zcash_primitives/src/transaction/components/issuance.rs

PaulLaux · 2024-08-12T18:53:01Z

zcash_primitives/src/transaction/components/orchard.rs

+        let flags = read_flags(&mut reader)?;
+        let value_balance = Transaction::read_amount(&mut reader)?;
+        let anchor = read_anchor(&mut reader)?;
+        let proof_bytes = Vector::read(&mut reader, |r| r.read_u8())?;
+        let actions = NonEmpty::from_vec(
+            actions_without_auth
+                .into_iter()
+                .map(|act| act.try_map(|_| read_signature::<_, redpallas::SpendAuth>(&mut reader)))
+                .collect::<Result<Vec<_>, _>>()?,
+        )
+        .expect("A nonzero number of actions was read from the transaction data.");


not sure I understand the question (replay to previous comment)

PaulLaux · 2024-08-12T18:55:03Z

zcash_primitives/src/transaction/components/orchard.rs

@@ -134,21 +181,43 @@ pub fn read_cmx<R: Read>(mut reader: R) -> io::Result<ExtractedNoteCommitment> {
    })
 }

-pub fn read_note_ciphertext<R: Read>(mut reader: R) -> io::Result<TransmittedNoteCiphertext> {
-    let mut tnc = TransmittedNoteCiphertext {
+pub fn read_note_ciphertext<R: Read>(


pub fn read_note_ciphertext<R: Read, D: OrchardDomainCommon>( mut reader: R, ) -> io::Result<TransmittedNoteCiphertext<D>> { let mut epk = [0; 32]; let mut enc = vec![0u8; D::ENC_CIPHERTEXT_SIZE]; let mut out = [0;80]; reader.read_exact(&mut epk)?; reader.read_exact(&mut enc)?; reader.read_exact(&mut out)?; Ok(TransmittedNoteCiphertext::<D> { epk_bytes: epk, enc_ciphertext: <D>::NoteCiphertextBytes::from_slice(&enc).unwrap(), out_ciphertext: out, }) }

PaulLaux · 2024-08-12T19:25:23Z

zcash_primitives/src/transaction/components/orchard.rs

+    Ok(tnc)
+}
+
+pub fn read_action_without_auth<R: Read>(mut reader: R) -> io::Result<Action<(), OrchardVanilla>> {


pub fn read_action<R: Read, D: OrchardDomainCommon>(mut reader: R) -> io::Result<Action<(), D>> { let cv_net = read_value_commitment(&mut reader)?; let nf_old = read_nullifier(&mut reader)?; let rk = read_verification_key(&mut reader)?; let cmx = read_cmx(&mut reader)?; let encrypted_note = read_note_ciphertext(&mut reader)?; // the generic function Ok(...) }

This PR updates the `zcash_primitives` crate to align with the recent changes in the `sapling-crypto` crate, which was updated to sync with the latest changes in `zcash_note_encryption`. ### Changes: - Updated the usage of `enc_ciphertext` to call `.as_ref()` to match the new return type. - Adjusted the import of the `ENC_CIPHERTEXT_SIZE` constant to use the definition from `sapling-crypto` instead of `zcash_note_encryption`, as the constant has been moved to `sapling-crypto`. Co-authored-by: Dmitry Demin <[email protected]>

This updates the test vectors to be of version 6 (as opposed to version 7). It also provides a new file for the V6 test vectors, an improvement to having them in the same file but behind feature flags. It provides a error for when the asset description string is not a UTF8 encoding.

alexeykoren · 2024-09-22T09:38:50Z

Closed in favor of #73

QED-it deleted a comment from what-the-diff bot Jun 26, 2024

alexeykoren requested review from PaulLaux, dmidem and vivek-arte June 27, 2024 15:33

PaulLaux marked this pull request as ready for review July 1, 2024 17:18

PaulLaux reviewed Jul 18, 2024

View reviewed changes

PaulLaux and others added 24 commits July 24, 2024 11:45

minor update

d58ac5b

updated version

f34dd02

commented out test code

b890183

updated test and cleanup

f62a230

New orchard version

a56c5da

Add TODO for burn reading/writing

d670033

fixed zcash_client_backend and simplified cargo.toml

c6175b8

updated Cargo.toml

af4d4bb

Fix post-rebase problems

fbbd655

Make BlockHeader from_data public

424bf1a

Make txid bytes public

b0b0f65

Note serialization

dac8d1f

Update zcash_primitives to use the generalised version of orchard cra…

cd8524b

…te (which supports Vanilla and ZSA) - but for now zcash_primitives is not generalised itself and pinned to Vanilla only

Fix tests accordingly to modifications in orchard

3c83b72

Minor fix or Rust code format (because of cargo fmt error)

aac4840

Fix post-rebase problems

6da0474

Note serialization

0049a3b

Update zcash_primitives to use the generalised version of orchard cra…

98f4747

…te (which supports Vanilla and ZSA) - but for now zcash_primitives is not generalised itself and pinned to Vanilla only

Introduce issue bundle

2365c2b

Orchard domain interface update

7e87fa5

Work in progress

2bd5e45

Add ZSA

2704c8a

Add issuance

5eec964

Fix fmt

3a18a4d

dmidem and others added 3 commits July 24, 2024 12:01

Remove duplicate import

f160f25

alexeykoren force-pushed the txv6-separate-bundles-rebased branch from 04ecb20 to f160f25 Compare July 24, 2024 16:25

PaulLaux reviewed Jul 24, 2024

View reviewed changes

alexeykoren added 15 commits July 25, 2024 19:28

Reformat feature flag

4dbee84

Address PR comments

8b9672d

Address PR comments

1860f1d

Fmt

f4cf872

Clippy

aec1879

Use ZSA BundleType

830260f

Use bundle check instead of has_orchard_zsa

acb8f97

Fix upper bound calculation

4f6ec46

Fmt

3d20879

Add finalization

7f045e7

Add issuance builder tests

f411948

Fmt

3ac2618

Fix double import

ea1770e

Use updated zsa1

fe88163

Fmt

fc38730

PaulLaux reviewed Aug 12, 2024

View reviewed changes

dmidem and others added 7 commits August 20, 2024 10:26

Minor changes

dc0a815

Change V7 to v6

a434849

Make ciphertext reading generic

9f52b1b

PR fixes

f79962b

PR fixes

738ed17

alexeykoren closed this Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nu7 implementation #65

Nu7 implementation #65

alexeykoren commented Jun 26, 2024 •

edited by PaulLaux

Loading

PaulLaux left a comment

PaulLaux Jul 17, 2024

alexeykoren Jul 25, 2024

PaulLaux Aug 11, 2024

PaulLaux left a comment

PaulLaux Jul 24, 2024

alexeykoren Jul 25, 2024

PaulLaux Aug 11, 2024

PaulLaux Jul 24, 2024

alexeykoren Jul 25, 2024

PaulLaux Aug 11, 2024

PaulLaux Jul 24, 2024

alexeykoren Jul 25, 2024

PaulLaux left a comment

PaulLaux Aug 12, 2024

alexeykoren Sep 4, 2024

PaulLaux Aug 12, 2024

alexeykoren Sep 4, 2024

PaulLaux Aug 12, 2024

alexeykoren Sep 4, 2024

PaulLaux Aug 12, 2024

alexeykoren Sep 4, 2024

PaulLaux Aug 12, 2024

alexeykoren Sep 4, 2024

PaulLaux Aug 12, 2024

alexeykoren Sep 4, 2024

PaulLaux Aug 12, 2024 •

edited

Loading

PaulLaux Aug 12, 2024

PaulLaux Aug 12, 2024

alexeykoren commented Sep 22, 2024

		_orchard_zsa_bundle in orchard_testing::arb_zsa_bundle_for_version(version),
		_issue_bundle in issuance::testing::arb_bundle_for_version(version),

		use super::components::amount::NonNegativeAmount;
		use super::components::sapling::zip212_enforcement;

Nu7 implementation #65

Nu7 implementation #65

Conversation

alexeykoren commented Jun 26, 2024 • edited by PaulLaux Loading

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

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

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

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

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

PaulLaux Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykoren commented Sep 22, 2024

alexeykoren commented Jun 26, 2024 •

edited by PaulLaux

Loading

PaulLaux Aug 12, 2024 •

edited

Loading