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

Use zcash_script’s new Script trait #8751

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sellout
Copy link

@sellout sellout commented Aug 8, 2024

Motivation

This is a precursor to testing the Rust implementation of Zcash Script.

Blocked-On: ZcashFoundation/zcash_script#171 and publishing a new version of zcash_script with those changes.

Solution

This uses a trait that wraps the C++ Zcash Script implementation. As we progress toward cutting over to a Rust implementation, this trait will additionally have impls for the Rust version and a variant that runs both C++ & Rust, comparing the results.

Additionally, this eliminates a few cases from zebra_script::Error that can never be produced.

Tests

The tests are the same as the previous zebra_script tests – there should be no change in behavior.

Follow-up Work

This is blocked on a release of zcash_script containing ZcashFoundation/zcash_script#171.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@sellout
Copy link
Author

sellout commented Aug 8, 2024

This depends on ZcashFoundation/zcash_script#171.

zebra-script/src/lib.rs Outdated Show resolved Hide resolved
@sellout sellout force-pushed the rusty-zcash_script branch 2 times, most recently from 38617ec to f80b392 Compare August 9, 2024 06:01
@mpguerra
Copy link
Contributor

mpguerra commented Aug 9, 2024

Thank you for your PR! We're focusing on changes for NU6 testnet activation at the moment so we won't be able to prioritise this review for another week or two.

zebra-script/src/lib.rs Show resolved Hide resolved
zebra-script/src/lib.rs Outdated Show resolved Hide resolved
zebra-script/src/lib.rs Outdated Show resolved Hide resolved
@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Aug 27, 2024
This is a precursor to testing the Rust implementation of Zcash Script.
@sellout sellout marked this pull request as ready for review August 30, 2024 20:19
@sellout sellout requested a review from a team as a code owner August 30, 2024 20:19
@sellout sellout requested review from arya2 and upbqdn and removed request for a team August 30, 2024 20:19
@sellout
Copy link
Author

sellout commented Aug 30, 2024

Re: the unchecked checkboxes

  • there are no visible changes to add to the CHANGELOG and
  • I don’t have permissions to add labels.

@arya2 arya2 added C-enhancement Category: This is an improvement A-script Area: Script handling P-Medium ⚡ rust Pull requests that update Rust code do-not-merge Tells Mergify not to merge this PR labels Aug 30, 2024
@arya2
Copy link
Contributor

arya2 commented Aug 30, 2024

Re: the unchecked checkboxes

  • there are no visible changes to add to the CHANGELOG and

We usually check boxes if they don't apply to the PR.

  • I don’t have permissions to add labels.

Added a priority label.

Added a do-not-merge label as well, we should wait until the changes in zcash_script have been published before merging this PR.

@arya2 arya2 added the S-blocked Status: Blocked on other tasks label Aug 30, 2024
@mpguerra
Copy link
Contributor

Added a do-not-merge label as well, we should wait until the changes in zcash_script have been published before merging this PR.

What's next here? I see ZcashFoundation/zcash_script#171 has merged

@conradoplg
Copy link
Contributor

What's next here? I see ZcashFoundation/zcash_script#171 has merged

We should double check this PR works with zcash_script main branch, and then I can make a zcash_script release and we point to that instead.

@sellout
Copy link
Author

sellout commented Sep 23, 2024

We should double check this PR works with zcash_script main branch, and then I can make a zcash_script release and we point to that instead.

I can do this, but wondering how to show that it’s successful – should I put up a temporary PR that just fetches zcash_script from GH instead of the crate registry and shows that everything still passes? And we’ll just close it afterward?

Also, I’m happy for there to be a zcash_script release now, but I figured it would be fine to leave it until the next PR or two land on zcash_script, which contain the actual Rust implementation.

@conradoplg
Copy link
Contributor

I can do this, but wondering how to show that it’s successful – should I put up a temporary PR that just fetches zcash_script from GH instead of the crate registry and shows that everything still passes? And we’ll just close it afterward?

Also, I’m happy for there to be a zcash_script release now, but I figured it would be fine to leave it until the next PR or two land on zcash_script, which contain the actual Rust implementation.

You can simply change Cargo.toml in this PR to point zcash_script to a specific commit in its repo (i.e. the one corresponding to its current main), which I think would also make CI pass here. (I can make the change if you prefer, let me know)

This issue with delaying the zcash_script release is that if this PR merges before the next Zebra release, this would prevent us from publishing the zebra-script crate (which we usually do at the same time) since it would depend on a git repo and crates.io doesn't allow that. We have several options though: don't merge this PR until all changes in zcash_script are done, or just make a zcash_script release with what it currently has and point to that. We'll figure it out, I think the important thing now is to just make sure this PR works with the current zcash_script.

sellout added a commit to sellout/zcash_script that referenced this pull request Sep 27, 2024
Notably, `HashType` has changed incompatibly, so
ZcashFoundation/zebra#8751 will need to be updated.
sellout added a commit to sellout/zcash_script that referenced this pull request Sep 27, 2024
Notably, `HashType` has changed incompatibly, so
ZcashFoundation/zebra#8751 will need to be updated.
sellout added a commit to sellout/zebra that referenced this pull request Sep 29, 2024
Show that ZcashFoundation#8751 would work with a new zcash_script release.
Show that ZcashFoundation#8751 would work with a new zcash_script release.
@sellout
Copy link
Author

sellout commented Sep 30, 2024

You can simply change Cargo.toml in this PR to point zcash_script to a specific commit in its repo (i.e. the one corresponding to its current main), which I think would also make CI pass here.

Ok, this now points at the zcash_script master branch. I also made a PR (#8896, to be discarded) that shows that the current Zebra main branch (without this PR) will also work with the new zcash_script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-script Area: Script handling C-enhancement Category: This is an improvement do-not-merge Tells Mergify not to merge this PR P-Medium ⚡ rust Pull requests that update Rust code S-blocked Status: Blocked on other tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants