Skip to content

Use zcash_script in PCZTs #1721

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Use zcash_script in PCZTs #1721

wants to merge 2 commits into from

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Feb 27, 2025

This replaces the local script implementation with the one from the zcash_script crate.

The Script type that was here is replaced by a family of three types

  • script::PubKey;
  • script::Sig<PushValue> – a v5+ script_sig, used for new sigs;
  • script::Sig<Opcode> – a pre-v5 script_sig, used for sigs read off the chain, which may predate NU5; and
  • an unwrapped Vec<u8> for script code.

This uses a number of zcash_script features that haven’t yet made it into the master branch, let alone a release, so this should not be merged.

This replaces the local script implementation with the one from the
zcash_script crate.

The `Script` type that was here is replaced by a family of three types
- `script::PubKey`;
- `script::Sig<PushValue>` – a v5+ script_sig, used for new sigs;
- `script::Sig<Opcode>` – a pre-v5 script\_sig, used for sigs read off
  the chain, which may predate NU5; and
- an unwrapped `Vec<u8>` for script code.

This uses a number of zcash_script features that haven’t yet made it
into the master branch, let alone a release, so this should not be
merged.
Convert to script code in the test runner, rather than from it.

This makes it easier to read/write the scripts (although they’re
currently nonsensical) and also the conversion from `script::PubKey` to
script code is total, while the reverse isn’t (since the code is
arbitrary bytes).
@daira
Copy link
Contributor

daira commented Mar 12, 2025

The advantages of this appear to be:

  • more precise typing
  • avoiding some duplication in zcash_transparent
  • being able to check that scripts are valid.

The downside is that pzct acquires a dependency on zcash_script, which makes it heavier to use in hardware wallets, one of its most important use cases. zcash_script's build process currently requires compiling C++. Given that Keystone for example is already struggling with library sizes and that they would need to audit that the C++ code is not used, do the advantages outweigh this downside?

@nuttycom
Copy link
Contributor

The downside is that pzct acquires a dependency on zcash_script, which makes it heavier to use in hardware wallets, one of its most important use cases. zcash_script's build process currently requires compiling C++. Given that Keystone for example is already struggling with library sizes and that they would need to audit that the C++ code is not used, do the advantages outweigh this downside?

It makes sense to me for that dependency to be behind a feature flag, but beyond that, I don't think that zcash_script is a particularly heavy dependency anyway? The heavy-weight crates in the graph (like secp256k1) are going to be required anyway.

@sellout
Copy link
Contributor Author

sellout commented Mar 12, 2025

@daira

zcash_script's build process currently requires compiling C++. Given that Keystone for example is already struggling with library sizes and that they would need to audit that the C++ code is not used, do the advantages outweigh this downside?

Would it be worthwhile to split the zcash_script crate into two? One that only has the Rust code, and a second that includes the C++ & comparison bits. So then PCZT and other new consumers could be unburdened by the C++ while Zebra still gets the current situation.

Or, to avoid the auditing issue, would it need to also be split into two repositories?

@nuttycom
Copy link
Contributor

nuttycom commented Mar 12, 2025

Would it be worthwhile to split the zcash_script crate into two? One that only has the Rust code, and a second that includes the C++ & comparison bits. So then PCZT and other new consumers could be unburdened by the C++ while Zebra still gets the current situation.

What I would do here is actually flip the dependencies: move the Rust implementation into the zcash_transparent crate, and then have zcash_script depend on that crate.

We would also likely move zcash_transparent into its own repository if we were to do that.

@sellout
Copy link
Contributor Author

sellout commented Mar 12, 2025

@nuttycom

What I would do here is actually flip the dependencies: move the Rust implementation into the zcash_transparent crate, and then have zcash_script depend on that crate.

We would also likely move zcash_transparent into its own repository if we were to do that.

That sounds reasonable to me. I would do that after all the in-progress changes on zcash_script are merged1. They’re all prerequisites for at least the current form of this PR, so I don’t think there are any downsides to waiting.

Footnotes

  1. zcash_transparent could be moved into a separate repo before then, of course. It’s only the move of zcash_script into zcash_transparent that should wait.

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.

3 participants