ZSA test vectors modifications#108
Conversation
Issuer keys test vectors implementation
* zsa-note-type: test vectors for note commitment and encryption * zsa-note-type: explicit native then ZSA notes * zsa-note-type: consistent order of arguments * zsa-note-type: revert output format * zsa-note-type: commit note encryption with note type test vectors Co-authored-by: Aurélien Nicolas <info@nau.re>
* zsa-sync-test-vectors: output directly the needed test code * zsa-sync-test-vectors: regenerate rust code with the same content but final format Co-authored-by: Aurélien Nicolas <info@nau.re>
- Updated to support encryption_v3 as given in QED-it/orchard#38 and QED-it/librustzcash#18. - This PR breaks compatibility with OrchardDomainV2
This adds the changes to the zcash test vectors to update the calculation of the Asset Base from the Asset Identifier via the Asset Digest, as detailed in ZIP 227.
We would like to have the same R constant for ZEC and ZSA note commitments.
…orr scheme, and rename `asset_id` to `asset_base` (#12) This updates the ik derivation to be via the Schnorr scheme, and refactors asset_id.py to asset_base.py. It also adds test vector generation code for issuance authorization signature, adds a check to ensure the `isk` is non zero, and updates the generated Rust files.
This merges upstream changes into the `zsa1` branch of our fork.
This separates the additions of the ZSA work into a separate file, creating separate test vector files for them. It also adds in upstream changes to the test vectors crate.
* changing documentation of the BIP 340 code to reflect the specific commit it has been taken from * re-adding the main function to key_components so that orchard vectors can be generated independently * reducing code duplication between orchard and orchard_zsa key_components * cosmetic changes to reduce unnecessary divergences from upstream, addition of blank line at end of some files * making the key component random initialization cleaner * adding suggestions from review
This is to add upstream changes for ZIP 320 test vectors into the repository.
This PR adds in the changes for ZIP 320 vectors from upstream. It also edits the output file in order to reduce the diff between the generated test vectors upstream.
* updating zip_0244 functions to support generating both V5 and V6 digests, and adding NU6 constants to transaction.py * adding V6 transaction class to transaction.py -- code reuse needs to be improved * changes to transaction.py and zip_0244.py * minor update * initial attempt at inheritance, to be improved * changing inheritance structure of TransactionV5 and TransactionV6, and moving TransactionV6 to transaction_zsa.py * reverting zip_0244.py to only include V5 details * separate txid vectors for zsa into separate files * adding ZSA version of zip 244 vectors * renaming to NU7 and so on * still WIP, updating to work with librustzcash code * updating get_random_unicode_bytes to give a slightly wider range of values * changes to the transaction format serialization and the txid generation to make it spec compliant * updating the generated test vectors * some cleanup changes * removing unichr option from get_random_unicode_bytes * cleaner import * simple changes based on review comments * moving common transaction fields in a way that reduces code duplication * reducing code duplication by creating an OrchardActionBase class which is inherited by OrchardActionDEscription and OrchardZSAActionDescription * reducing code duplication in transaction, transaction_zsa, zip_0244 and orchard_zsa/digests files * reducing code duplication inside the main function of zip_0244 and orchard_zsa/digests * changing txn version back to V6 * renaming rho to nf_old in note_encryption files
This renames the TransactionZSA class to TransactionV6 to better reflect its purpose as a V6 transaction format reference implementation
For more details, see ZIP227
* Moving the burn fields into the action group * fixing small issue with switch to asset_desc_hash * regenerating test vector files
Main modifications - Generate test vectors for ZIP230 adresses - Generate test vectors for ZIP32 - Update regenerate script - Update rust output by replacing vectors by slices to support no_std
Add `SighashInfo` (version and associated data) to - Transparent authorizing signatures. Update `auth_digest` by adding `SighashInfo` as defined in ZIP246 Add `SighashInfo` for Sapling/Orchard binding signatures only when the signatures exist
|
|
||
| if [ "$gen_type" = "rust" ]; then | ||
| echo "Running rustfmt on generated Rust files..." | ||
| rustfmt --edition 2021 test-vectors/rust/*.rs |
There was a problem hiding this comment.
This change should be extracted into a standalone PR, not proposed as part of the ZSA test vector changes. Changes to the entire output format (and thus the entire test vector set) need to be separately considered.
There was a problem hiding this comment.
We have opened #112 for the consideration of these changes. These changes have been rolled back in this PR.
| filename, | ||
| )) | ||
| print() | ||
| visibility = 'pub(crate) ' |
There was a problem hiding this comment.
Likewise, these changes should be extracted from this PR. If it were only the pub(crate) change then it might be fine, but proposing changes to the default indentation makes the PR way too invasive to be done as a side-effect of the ZSA test vector changes.
There was a problem hiding this comment.
This has also been opened as #114, which can be reviewed independently. These changes have been rolled back from this PR
oxarbitrage
left a comment
There was a problem hiding this comment.
I left several comments related to cleanup, readability, and adding documentation. Applying some of these suggestions (and moving a few of the unrelated changes into a separate PR) would make the diff smaller and cleaner, which in turn should help us review the critical parts more carefully.
That said, these are all optional suggestions and reflect personal preference; I didn’t find any critical issues in this initial review.
If you prefer, I can also push the suggested changes directly to your branch.
|
|
||
| test_vectors = [] | ||
|
|
||
| # Start with the test vector from the BIP 340 repository. Specifically, the index 0 from https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.csv |
There was a problem hiding this comment.
| # Start with the test vector from the BIP 340 repository. Specifically, the index 0 from https://github.com/bitcoin/bips/blob/master/bip-0340/test-vectors.csv | |
| # Start with the test vector from the BIP 340 repository. Specifically, the index 0 from https://github.com/bitcoin/bips/blob/445e445144afa55cbd09957919ddda92c579f8d8/bip-0340/test-vectors.csv |
Maybe use a specific commit here to avoid a potentially misleading reference if something changes upstream.
| orchard_group_hash = "zcash_test_vectors.orchard.group_hash:main" | ||
| orchard_map_to_curve = "zcash_test_vectors.orchard.group_hash:map_to_curve_test_vectors" | ||
| orchard_key_components = "zcash_test_vectors.orchard.key_components:main" | ||
| orchard_map_to_curve = "zcash_test_vectors.orchard.group_hash:map_to_curve_test_vectors" |
There was a problem hiding this comment.
This change appears to be only for alphabetical ordering. Maybe consider moving it to a separate PR to keep this diff smaller.
| if [ "$gen_type" = "rust" ]; then | ||
| echo "Running rustfmt on generated Rust files..." | ||
| rustfmt --edition 2021 test-vectors/rust/*.rs | ||
| fi |
There was a problem hiding this comment.
Can be moved to a different PR.
| @@ -261,7 +262,7 @@ def randbytes(l): | |||
| 'rseed': note.rseed, | |||
| 'memo': np.memo, | |||
| 'cv_net': bytes(cv), | |||
| 'rho': bytes(rho), | |||
| 'nf_old': bytes(rho), | |||
There was a problem hiding this comment.
Could you add a reference to justify this change?
There was a problem hiding this comment.
The test vectors were changed to use nf_old instead of rho in the orchard repository in QED-it/orchard@1a8ded0, but the zcash-test-vectors repository seems not to have been updated to generate the test vectors with nf_old instead of rho. We just fixed that.
There was a problem hiding this comment.
The changes in this file to define and use construct_note don’t seem strictly necessary. They appear to have been added to match the implementation in zcash_test_vectors/orchard_zsa/note_encryption.py. I’m fine with keeping them, but they could be moved to a separate PR.
There was a problem hiding this comment.
We updated this structure to minimize having to rewrite code between the Vanilla Orchard and OrchardZSA parts (for example, the assert leadByte == 0x02 code needs to change to assert leadByte == 0x03 in OrchardZSA but we do not want to rewrite the entire decrypt_using_ovk function).
I prefer keeping this change in here rather than a separate PR, because the reason why the change is made is clearer when in the context of how it is reused for OrchardZSA, and in a standalone PR it would seem not strictly necessary, as you mentioned.
| # The algorithm byte prefix for the encoding of the BIP340 Schnorr signature in ZIP227 is 0x00. | ||
| ZSA_BIP340_SIG_SCHEME = b'\0' | ||
|
|
||
| #This function provides the encoding of the issuance key, with the algorithm byte prefix. |
There was a problem hiding this comment.
| #This function provides the encoding of the issuance key, with the algorithm byte prefix. | |
| # This function provides the encoding of the issuance key, with the algorithm byte prefix. |
| @@ -539,7 +574,7 @@ def __bytes__(self): | |||
| for desc in self.vActionsOrchard: | |||
| ret += bytes(desc) # Excludes spendAuthSig | |||
| ret += struct.pack('B', self.flagsOrchard) | |||
| ret += struct.pack('<Q', self.valueBalanceOrchard) | |||
| ret += struct.pack('<q', self.valueBalanceOrchard) | |||
There was a problem hiding this comment.
Can we justify this change ?
There was a problem hiding this comment.
The change was actually unnecessary, thanks for pointing it out! Reverted back to the original in QED-it#44
| Script, | ||
| TransactionV5, | ||
| NU5_TX_VERSION_BYTES, |
There was a problem hiding this comment.
Unused import
| NU5_TX_VERSION_BYTES, |
| @@ -35,12 +41,22 @@ def transparent_digest(tx): | |||
|
|
|||
| return digest.digest() | |||
|
|
|||
| TRANSPARENT_AUTH_DIGEST_PERSONALIZAION = b'ZTxAuthTransHash' | |||
There was a problem hiding this comment.
Maybe add a reference on top of personalization constants.
| ret += bytes(self.bindingSigSapling) | ||
|
|
||
| return ret | ||
|
|
||
| class TransactionV5(TransactionBase): |
There was a problem hiding this comment.
There was a problem hiding this comment.
We merged the upstream work, and incorporated the ZIP 233 additions
…g in TransactionBase
This performs some code refactoring to make the upstream merge in the next step less complicated. The refactoring is cosmetic in that it doesn't affect the generated test vectors. It mainly splits out the initialization and byte serialization of the transaction (in TransactionBase) to differing functions for header, transparent and sapling parts of the transaction. The Orchard parts are in the child classes (TransactionV5 and TransactionV6) and are not separated out into functions.
This merges in the updates from `upstream/master` to the target branch.
This makes the spacing, comment and minor rearrangements suggested in zcash#108 (review). Specifically, this PR makes the changes that are touching the ZSA portions of the repository.
This refactors the portions of the `TransactionBase` class so that it does not need to use the `version_group_id` to decide behaviour - the way it behaves is decided by redefinition of the added functions in the child classes (aka `TransactionV5` and `TransactionV6`). This refactor does not affect the generated test vectors.
This removes the formatting updates we made (namely the addition of `rustfmt`, and the addition of `pub(crate)` visibility and indentation changes). This reduces the difference between `upstream:master` and our branch significantly, from 69 files to 40 files, and would help speed up the review of the ZSA test vectors.
This expands the sampled code points for the get_random_unicode_bytes function to include all allowed Unicode code points. This changes the random values of the description of the Assets, and therefore changes the orchard_zsa_digests.rs and orchard_zsa_asset_base.rs files, which would need to be copied into the librustzcash and orchard repositories respectively.
This adds comments referencing the relevant sections of the ZIPs, as requested in zcash#108 (comment). We also update some constant names to fix a typo.
- Update nu7 branch id in ZIP233 - Add ZIP233 into `regenerate.sh` script - Update ZIP233 test vectors
Updated reference implementation for the TXV6 and OrchardZSA components.