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

zebra_script: update to new zcash_script callback API #8566

Merged
merged 10 commits into from
Jun 19, 2024

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented May 23, 2024

Motivation

See ZcashFoundation/zcash_script#157 for context.

Currently no matching issue.

Closes #3436 because makes it moot.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Specifications

Complex Code or Requirements

Solution

This changes zebra_script to use the new zcash_script API.

This is a bit bigger than expected because I did a small refactoring that made the implementation easier: passing a BranchID instead of a NetworkUpgrade to the sighasher (which is simpler since all callers converted a BranchID to a NetworkUpgrade before calling it)

This also changes the SigHasher to allow precomputation, which is used with the new API. This way we keep the same performance since the old API also supported precomputation. But this will also allow us to share the precomputation with the shielded sighash implementation, but I'll do that in a follow up PR.

Testing

I just updated the tests for the new SigHasher API where applicable.

Review

This depends on #8568 since I wanted to make sure I was using up-to-date APIs.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

We can now use the new SigHasher to share precomputation with the shielded sighash, but that's optional.

@conradoplg conradoplg force-pushed the simplify-zcash-script-api branch 2 times, most recently from d481230 to 3131e2f Compare May 29, 2024 21:56
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label May 29, 2024
@conradoplg conradoplg changed the base branch from main to upgrade-zcb-zebra May 29, 2024 22:45
@conradoplg conradoplg changed the title WIP: use simplified zcash_script API zebra_script: update to new zcash_script callback API May 29, 2024
Base automatically changed from upgrade-zcb-zebra to main June 4, 2024 21:04
@conradoplg conradoplg marked this pull request as ready for review June 13, 2024 22:50
@conradoplg conradoplg requested review from a team as code owners June 13, 2024 22:50
@conradoplg conradoplg requested review from arya2 and upbqdn and removed request for a team June 13, 2024 22:50
@mpguerra mpguerra linked an issue Jun 17, 2024 that may be closed by this pull request
3 tasks
upbqdn
upbqdn previously approved these changes Jun 17, 2024
zebra-chain/src/primitives/zcash_primitives.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/sighash.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/sighash.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/sighash.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/sighash.rs Outdated Show resolved Hide resolved
zebra-chain/src/primitives/zcash_primitives.rs Outdated Show resolved Hide resolved
@upbqdn upbqdn added A-script Area: Script handling rust Pull requests that update Rust code and removed P-Low ❄️ C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jun 19, 2024
mergify bot added a commit that referenced this pull request Jun 19, 2024
@mergify mergify bot merged commit 3650442 into main Jun 19, 2024
83 checks passed
@mergify mergify bot deleted the simplify-zcash-script-api branch June 19, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-script Area: Script handling P-Medium ⚡ rust Pull requests that update Rust code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Create wrapper for precomputed in CachedFfiTransaction
2 participants