Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to TXID generation to account for ZSA additions #22

Merged
merged 36 commits into from
Sep 19, 2024

Conversation

vivek-arte
Copy link

@vivek-arte vivek-arte commented Jun 17, 2024

This PR makes the following changes:

  • It allows for the generation of test vectors for the V7 transaction format including ZSAs, in addition to the original Orchard vectors. The test vectors are generated independently for both cases, and pasted together in Corrections to V7 transaction format and using updated test vectors librustzcash#66.
  • This is done efficiently via the creation of separate data structures for V5 and V7, using inheritance to capture the common portions without code duplication.
  • There is also a supporting change made to the generation of random unicode strings for the asset description string, to allow for a wider range of random values to be generated.

@vivek-arte vivek-arte changed the base branch from zsa1 to orchard_zsa_20240610 June 17, 2024 06:25
@vivek-arte vivek-arte changed the base branch from orchard_zsa_20240610 to zsa1 June 18, 2024 10:19
vivek-arte added a commit to QED-it/librustzcash that referenced this pull request Jul 24, 2024
)

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).
alexeykoren pushed a commit to QED-it/librustzcash that referenced this pull request Jul 24, 2024
)

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).
Copy link

@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.

Good overall, added some comments about code duplication.

zcash_test_vectors/orchard_zsa/asset_base.py Show resolved Hide resolved
zcash_test_vectors/orchard_zsa/asset_base.py Show resolved Hide resolved
zcash_test_vectors/transaction.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
Copy link

@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.

Good overall, some minor tweaks are required.

zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244_zsa.py Outdated Show resolved Hide resolved
PaulLaux
PaulLaux previously approved these changes Sep 4, 2024
Copy link

@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.

Good overall, some minor comments.

zcash_test_vectors/transaction_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction_zsa.py Outdated Show resolved Hide resolved
zcash_test_vectors/transaction_zsa.py Show resolved Hide resolved
PaulLaux

This comment was marked as outdated.

@PaulLaux PaulLaux self-requested a review September 18, 2024 08:56
@PaulLaux PaulLaux dismissed their stale review September 18, 2024 08:57

review again

Copy link

@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.

added one comment to clarify intent.

zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
Copy link

@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.

some small fixes

zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
zcash_test_vectors/zip_0244.py Outdated Show resolved Hide resolved
Copy link

@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.
Please make sure that this PR does not change the QED-it/orchard#114 vectors. Update if needed.

@vivek-arte
Copy link
Author

Confirmed that QED-it/orchard#114 captures the changes made in this PR.

@vivek-arte vivek-arte merged commit a482147 into zsa1 Sep 19, 2024
3 checks passed
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