Skip to content

Separate OrchardZSA from Orchard#13

Merged
vivek-arte merged 32 commits into
zsa1from
separate_zsa
Apr 30, 2024
Merged

Separate OrchardZSA from Orchard#13
vivek-arte merged 32 commits into
zsa1from
separate_zsa

Conversation

@vivek-arte
Copy link
Copy Markdown

@vivek-arte vivek-arte commented Mar 30, 2024

This PR 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.

…s some redundant material that can be removed.
…efactoring) -- now all the ZSA additions are in the orchardzsa folder only, albeit with redundancies that are to be removed
…we might later want to change. Also removing older generated test vectors.
@PaulLaux PaulLaux changed the title Refactoring test vectors to separate OrchardZSA additions from Orchard work (and allowing for generation of both V5 and V6 test vectors) Separate OrchardZSA from Orchard work Apr 16, 2024
@PaulLaux PaulLaux changed the title Separate OrchardZSA from Orchard work Separate OrchardZSA from Orchard Apr 16, 2024
Copy link
Copy Markdown

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, added comments and questions.

TxV6 require some more work: extracting mutual code with v5 and polish.
To be done as a separate task.

Comment thread .github/workflows/test_vectors.yml
Comment thread zcash_test_vectors/orchard/key_components.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/commitments.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/commitments.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/key_components.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/key_components.py Outdated
Comment thread zcash_test_vectors/transaction.py Outdated
Comment thread zcash_test_vectors/transaction.py Outdated
Comment thread zcash_test_vectors/transaction.py Outdated
Comment thread zcash_test_vectors/zip_0244.py Outdated
@vivek-arte
Copy link
Copy Markdown
Author

Note: The NU6 specific updates to transaction.py and zip-0244.py will be done in a subsequent, follow-on PR.

PaulLaux
PaulLaux previously approved these changes Apr 24, 2024
Copy link
Copy Markdown

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved pending small fixes.
Also, let's make sure all comments are correct and the links are valid.

Comment thread zcash_test_vectors/orchard_zsa/note.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/note.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/note_encryption.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/note_encryption.py Outdated
Comment thread zcash_test_vectors/orchard_zsa/note_encryption.py Outdated
super().__init__(rand)

def encrypt(self, note: OrchardZSANote, memo, pk_d_new, g_d_new, cv_new, cm_new, ovk=None):
tc = super().encrypt(note, memo, pk_d_new, g_d_new, cv_new, cm_new, ovk)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice generalization.
are we confident that we get the right ciphertext?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although super.encrypt(...) takes note: OrchardNote, since OrchardZSANote inherits OrchardNote, if I pass it an OrchardZSANote instead, it will use the OrchardZSANote version of the functions wherever they differ. I had seen simpler examples online displaying that.

I first confirmed we get the longer length ciphertexts, which is an initial sanity check.
Also then, I used these test vectors in the orchard crate with Dmitry's backward compatibility work, and it passed the test for note_encryption_zsa so it should be fine.

dmidem pushed a commit to QED-it/orchard that referenced this pull request Apr 30, 2024
…est-vectors repo (#102)

This PR simply changes the test vectors used, replacing them with those
generated by the current version of the zcash-test-vectors repo (i.e.
changes made in [this
PR](QED-it/zcash-test-vectors#13)).
@vivek-arte vivek-arte merged commit e2f96cf into zsa1 Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants