Upstream PR 471 (OrchardZSA) – internal review and fixes#201
Closed
dmidem wants to merge 153 commits into
Closed
Conversation
* Added .circleci/config.yml
Implements the issuer keys as
IssuerAuthorizingKey -> isk
IssuerVerifyingKey -> ik
Test vectors generated with zcash_test_vectors repo
* Added NoteType to Notes * Added NoteType to value commitment derivation
* Circleci project setup (#1) * Added .circleci/config.yml * Added NoteType to Notes * reformated file * updated `derive` for NoteType * added note_type to value commit derivation * rustfmt * updated ci config * updated ci config * updated ci config * updated derive for note_type * added test for arb note_type * added test for `native` note type * zsa-note-encryption: introduce AssetType and encode and decode it in note plaintexts * zsa-note-encryption: extend the size of compact notes to include asset_type * fixed clippy warrnings * rustfmt * zsa-note-encryption: document parsing requirement * zsa-note-encryption: revert support of ZSA compact action * zsa_value: add NoteType method is_native * zsa-note-encryption: remove dependency on changes in the other crate * zsa-note-encryption: extract memo of ZSA notes * zsa-note-encryption: tests (zcash_test_vectors 77c73492) * zsa-note-encryption: simplify roundtrip test * zsa-note-encryption: more test vectors (zcash_test_vectors c10da464) * Circleci project setup (#1) * Added .circleci/config.yml * issuer keys implementation (#5) Implements the issuer keys as IssuerAuthorizingKey -> isk IssuerVerifyingKey -> ik Test vectors generated with zcash_test_vectors repo * Added NoteType to Notes (#2) * Added NoteType to Notes * Added NoteType to value commitment derivation * zsa-note-encryption: use both native and ZSA in proptests * zsa-note-encryption: test vector commit 51398c93 * zsa-note-encryption: fix after merge Co-authored-by: Paul <3682187+PaulLaux@users.noreply.github.com> Co-authored-by: Paul <lauxpaul@protonmail.com> Co-authored-by: Aurélien Nicolas <info@nau.re> Co-authored-by: Daniel Benarroch <danielbenarroch92@gmail.com>
+ Updated test bsk_consistent_with_bvk to verify mixed note types. + Added NoteType support to the builder and the bundle. + added split_flag to SpentInfo and as input to the Circuit (currently commented out) + added conditional cv_sum calculation (currently commented out) + added padding to actions
- added IssueBundle and IssueAction - added a builder for IssueBundle - added verify_issue_bundle() for consensus verification. - unit tests.
added tests in `tests/zsa.rs`
* disabled split notes and proof check for zsa transfer
* fixes and suggestions * changed "issuer" to "issuance" as per zcash#356 (comment) * terminology fixes * updated naming
* rename 2 note_type -> asset as per zcash#356 (comment) * added a dedicated type for "IssuanceAuth" * disabled codecov github action due to bad behavior. * extracted "is_asset_desc_of_valid_size()" into asset_id.rs
* improved `verify_issue_bundle()`
Added a method to add assets to burn to the Builder bvk computation now includes the burnt assets Added Tests for bsk/bvk consistency for burning Added E2E tests for assets burning
Added CI badge to README
Added `OrchardDomainV3` on top of the encryption generalization (QED-it/librustzcash#18). not for review: note_encryption.rs, note_encryptionv2v3.rs and src/test_vectors/note_encryption.rs. These files represent two possible approaches for backward compatibility and will be finalized down the road. (the files were excluded from the build).
Make IVK::from_bytes public
When split_flag is set, the following values are modified * v_net is equal to -v_new instead of v_old - v_new * cv_net is evaluated with this new value of v_net The following constraints are modified * (v_old - v_new = magnitude * sign) becomes (v_old * (1-split_flag) - v_new = magnitude * sign) to take into account the new value of v_net * nf_old = nf_old_pub is only checked when split_flag=0 * the new constraint asset_old = asset_new is always checked regardless of the value of split_flag
- Renamed AssetId to AssetBase - Changed the AssetBase implementation to support the zip update. - Updated visibility for various members of issuance.rs
…#49) This PR updates the test-vectors from the updates to the zcash-test-vectors repository (see here). The keys test is also updated to now use the asset base from the test vectors instead of just using the native asset.
In the circuit, we update value_commit_orchard to take into account asset.
Previously, value_commit_orchard returns cv_net = [v_net] ValueCommitV + [rcv] ValueCommitR..
Now, value_commit_orchard returns cv_net = [v_net] asset + [rcv] ValueCommitR.
ValueCommitV and ValueCommitR are constants
v_net is equal to sign * magnitude where sign is in {-1, 1} and magnitude is an unsigned integer on 64 bits.
To evaluate [v_net] asset where v_net = sign * magnitude, we perform the following steps
1. verify that magnitude is on 64 bits
2. evaluate commitment=[magnitude]asset with the variable-base long-scalar multiplication
3. evaluate result=[sign]commitment with the new mul_sign gate
We would like to have a constant-time evaluation of the note commitment for both ZEC and ZSA. ZEC_note_commitment=Extract_P(SinsemillaHashToPoint(zec_personalization, common_bits) + [rcm]R) ZSA_note_commitment=Extract_P(SinsemillaHashToPoint(zsa_personalization, common_bits || asset) + [rcm]R) R is the same constant for ZEC and ZSA note commitments.
1. Added a new error, `ValueSumOverflow`, that occurs if the sum value overflows when adding new supply amounts. 2. Created a new `supply_info` module containing `SupplyInfo` and `AssetSupply` structures, with `add_supply` function and unit tests for it. 3. Renamed the `are_note_asset_ids_derived_correctly` function to `verify_supply`, changed its behavior to verify and compute asset supply, added unit tests for it. 4. Updated the `verify_issue_bundle` function to use the changes mentioned above, updated its description, and added new unit tests. 5. Renamed errors with `...NoteType` suffix in the name to `...AssetBase`. 6. Added `update_finalization_set` method to `SupplyInfo` and use after the calls of `verify_issue_bundle function` (if needed), instead of mutating the finalization set inside `verify_issue_bundle`.
- Add getter method for Bundle.burn field
For zcash_note_encryption, we have to use version 0.2 with QEDIT patch.
In the circuit, we update note_commit to take into account asset.
Previously, note_commit returns cm = hash(Q_ZEC, msg) + [rcm]R.
Now, note_commit returns
- cm = hash(Q_ZEC, msg) + [rcm]R for ZEC note
- cm = hash(Q_ZSA, msg || asset) + [rcm]R for ZSA note
We now evaluate note_commit with the following steps
1. evaluate **hash_zec = hash(Q_ZEC, msg)**
2. evaluate **hash_zsa = hash(Q_ZSA, msg || asset)**
3. select **hash = hash_zec if is_native_asset**
**= hash_zsa otherwise**
4. evaluate **cm = hash + [rcm]R**
5. check some constraints on msg and asset and their decompositions
6. return **cm**
The following modifications are required to update note_commit:
- add a is_native_asset witness (and check that it is a boolean and its
value is correct according to asset)
- add a MUX chip to evaluate a multiplexer on Pallas points
Warning: we increased the size of the Orchard circuit !
… = nf_old_pub) (#62) Currently, every new note commitment is calculated using rho_new = nf_old = DeriveNullifier_nk(rho_old, psi_old, cm_old). For split notes, we would like to evaluate the new note commitment with rho_new = nf_old_pub (a random nullifier which is stored in the instance nf_old_pub). For all remaining notes, nf_old = nf_old_pub. As such, implementing rho_new = nf_old_pub for all notes will not affect those remaining notes (and only affect split notes).
dmidem
force-pushed
the
zcash-pr471-review-fixes-1
branch
6 times, most recently
from
cb4aae6 to
0ed3034
Compare
dmidem
force-pushed
the
zcash-pr471-review-fixes-1
branch
6 times, most recently
from
88b7398 to
02b5949
Compare
1. Disable LTO for cargo test --release in CI to avoid very slow / stuck macOS builds (rustc/LLVM during release test compilation). 2. Run apt-get update before installing libfontconfig1-dev on Ubuntu to prevent intermittent 404s from stale package indexes.
…for list of fixed comments) r2487851383 r2487857002 r2487858123 r2487968253 r2487968899 r2542305679 r2542313926 r2542359143 r2542360991 r2542333126 r2542371427 (see note) r2542392826 (see note) r2542406328 r2542447228 r2542428312 r2542599948 r2542671834 r2542817988 r2542846302 r2542756837 r2543147318 r2543502871 r2543506591 r2543527521 r2543542313 (see note) r2543565714 (see note) r2549938764 r2542276487 r2542288774 r2550151848 r2550187398 r2550276814 r2550352041 r2550380193 r2550407188 r2550616659 Note: r2542371427 partially addressed - modules moved into issuance module, zsa-issuance feature flag added to Cargo.toml. The issuance module is not preceded with the flag yet as there are many places in the rest of the code that need to be preceded with it. This needs to be discussed first, so the flag is unused for now. Note: r2542392826 addressed except orchard_flavor changes. Note: r2543542313 - changed pub to pub(crate) but needs to be discussed. Note: r2543565714 - It's not completely clear from the comment, but I removed the doc comment for rseed_split_note and updated it for create_split_note - is that correct? Also kept create_split_note pub, not pub(crate) - needs to be discussed.
dmidem
force-pushed
the
zcash-pr471-review-fixes-1
branch
from
30aa91d to
8750277
Compare
…for list of fixed comments) r2585521988 r2585536670 r2585544143 r2585546655 r2585548116 r2585631816 r2585636883 r2599516192 r2599562825 r2599565711 r2599584010 r2599589396 r2599594554 r2599594936 r2599745324 r2602373392 r2602378140 r2602410770 r2602415043 r2602416292 r2602418341 r2602421894 r2602426619 r2602532807 r2602541601 r2602556915 r2602570477 (see note r2602938883 (see note) Note: r2602570477 and r2602938883 — we renamed the generic parameter P to Pr only where it conflicts with P for Proof. Should we use Pr for OrchardPrimitives everywhere else for consistency?
dmidem
force-pushed
the
zcash-pr471-review-fixes-1
branch
from
8750277 to
cd41b4f
Compare
dmidem
changed the title
This was referenced Jan 5, 2026
Collaborator
|
continued in #239 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR mirrors zcash#471 (QED-it:zsa1 → zcash:ltf/zsa) on our fork for internal review and changes.
We’ll address upstream review comments here, then propagate the final commits and responses back to zcash#471.