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

change(nu5): update librustzcash #3832

Closed
wants to merge 2 commits into from
Closed

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Mar 10, 2022

Motivation

zcash_script has a new API due to ZIP-244 changes that we must use.
In order to use it, we also need the updated librustzcash which also had API changes.

Specifications

Designs

Solution

  • Change to work with new librustzcash API
  • Fix dependency versions

Split from #3799
Part of #3520
Closes #3056

Review

This was split from #3799 which was reviewed by @teor2345

This is blocked on ZcashFoundation/zcash_script#30 (which is also blocked) so it's a draft, but it can already be reviewed.

This blocks the testnet reactivation test, so it's important.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@conradoplg conradoplg requested review from a team as code owners March 10, 2022 01:39
@conradoplg conradoplg requested review from jvff and upbqdn and removed request for a team March 10, 2022 01:39
@conradoplg conradoplg changed the title change: update librustzcash change(nu5): update librustzcash Mar 10, 2022
@conradoplg conradoplg marked this pull request as draft March 10, 2022 01:48
@teor2345
Copy link
Contributor

I think some of the dependencies might be wrong?

The application panicked (crashed).
Message: zcash_script_new_precomputed_tx returned 4 (encountered unknown error kind from zcash_script: 4)

https://github.com/ZcashFoundation/zebra/runs/5489608526?check_suite_focus=true#step:13:1431

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'm happy with this, as soon as the tests pass.

@conradoplg
Copy link
Collaborator Author

I'm happy with this, as soon as the tests pass.

It turns out we couldn't split the PR since we feed V5 transactios to zebra_script when testing, and zcash_script won't accept them in the pre-V5 functions 😢 I'll close this and change #3799 to target main again

@conradoplg conradoplg closed this Mar 10, 2022
@teor2345 teor2345 deleted the librustzcash-new-api branch March 21, 2022 21:35
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.

Switch to zcash/orchard now that upstream changes are merged
2 participants