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

deps(bump): zcash_primitives, zcash_note_encryption, zcash_encoding, orchard, and zcash_script #5505

Merged
merged 6 commits into from
Oct 31, 2022

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Oct 30, 2022

Motivation

We want to bump this 3 dependencies at the same time. This PR will replace the other 3 if we go this way:

It also works around the fact that address() is now private.

I am pushing this now as we might want to add it to the release candidate.

Close #3831 (removing all duplicate ECC dependencies, not just zcash_proofs)

Solution

  • Upgrade the 4 ECC dependencies.
  • Upgrade zcash_script.
  • Fix address issue.

Review

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
  • How do you know it works? Does it have tests?

@oxarbitrage oxarbitrage requested review from a team as code owners October 30, 2022 14:37
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team October 30, 2022 14:37
@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Merging #5505 (9a8e061) into main (161bb80) will decrease coverage by 0.03%.
The diff coverage is 78.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5505      +/-   ##
==========================================
- Coverage   79.25%   79.22%   -0.04%     
==========================================
  Files         305      305              
  Lines       37873    37885      +12     
==========================================
- Hits        30018    30016       -2     
- Misses       7855     7869      +14     

upbqdn
upbqdn previously approved these changes Oct 30, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

This PR looks great. I was working on a similar one, but my workaround for address() wasn't as smooth.

@oxarbitrage I have updates that make Zebra compatible with ZcashFoundation/zcash_script#41, should I push them as a PR into this one, or would it be better to make a standalone PR?

@oxarbitrage
Copy link
Contributor Author

This PR looks great. I was working on a similar one, but my workaround for address() wasn't as smooth.

Apologies for working on something you were already working on. I didn't saw #5500 (comment)

@oxarbitrage I have updates that make Zebra compatible with ZcashFoundation/zcash_script#41, should I push them as a PR into this one, or would it be better to make a standalone PR?

That is great, i think we might want to have all the ECC or our own dependencies up to date in the release candidate but i am not totally sure if it is that important.

If your changes are not too big i think we can try to push to this one and merge all the dependencies alltogether.

@upbqdn
Copy link
Member

upbqdn commented Oct 30, 2022

@oxarbitrage I have updates that make Zebra compatible with ZcashFoundation/zcash_script#41, should I push them as a PR into this one, or would it be better to make a standalone PR?

I just realized it would be best if we wait until we merge ZcashFoundation/zcash_script#41, and then do the rest of the updates in Zebra, so a standalone PR will be a better solution.

@upbqdn
Copy link
Member

upbqdn commented Oct 30, 2022

Apologies for working on something you were already working on.

No worries at all, the only blocker was the address() function being private.

@upbqdn
Copy link
Member

upbqdn commented Oct 30, 2022

I just realized it would be best if we wait until we merge ZcashFoundation/zcash_script#41, and then do the rest of the updates in Zebra, so a standalone PR will be a better solution.

I actually pushed a draft PR with the updates #5506 without ZcashFoundation/zcash_script#41 being merged so that we can see what changes are needed.

@teor2345 teor2345 requested a review from a team as a code owner October 30, 2022 23:16
@teor2345 teor2345 requested review from arya2 and removed request for a team October 30, 2022 23:16
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 30, 2022
@teor2345 teor2345 changed the title deps(bump): zcash_primitives, zcash_note_encryption and orchard deps at once deps(bump): zcash_primitives, zcash_note_encryption, orchard, and zcash_script Oct 30, 2022
@teor2345 teor2345 added A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement P-High 🔥 A-script Area: Script handling and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 30, 2022
@teor2345 teor2345 added the A-cryptography Area: Cryptography related label Oct 30, 2022
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.

Just want to tweak a version number

zebra-chain/Cargo.toml Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 30, 2022
@teor2345 teor2345 changed the title deps(bump): zcash_primitives, zcash_note_encryption, orchard, and zcash_script deps(bump): zcash_primitives, zcash_note_encryption, zcash_encoding, orchard, and zcash_script Oct 30, 2022
teor2345
teor2345 previously approved these changes Oct 31, 2022
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 have a suggestion, but it's not a blocker.

zebra-chain/src/primitives/zcash_primitives.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

I am pushing this now as we might want to add it to the release candidate.

Yes, we do, using outdated or duplicate cryptographic dependencies is a security vulnerability. (And it's a pain to audit.)

@teor2345
Copy link
Contributor

error: linking with cc failed: exit status: 1
https://github.com/ZcashFoundation/zebra/actions/runs/3357475081/jobs/5563308196#step:15:463

This is only happening in the GitHub Actions Linux build, so I cleared this PR's GitHub Actions caches, and the main branch GitHub Actions caches.

@mergify mergify bot merged commit 8441801 into main Oct 31, 2022
@mergify mergify bot deleted the bump-3-deps branch October 31, 2022 14:15
@teor2345 teor2345 added the C-security Category: Security issues label Nov 1, 2022
teor2345 added a commit that referenced this pull request Feb 6, 2023
…orchard, and zcash_script (#5505)

* Bump zcash_primitives, zcash_note_encryption and oechard deps at once

* Bump dependencies, and update `deny.toml`

* Upgrade to zcash_script 0.1.8

* Update Cargo.lock

* Use 3-part version numbers consistently

* Get address by serializing the Output, then using zcash_primitives to parse it (#5507)

Co-authored-by: Marek <mail@marek.onl>
Co-authored-by: teor <teor@riseup.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography related A-dependencies Area: Dependency file updates A-script Area: Script handling C-enhancement Category: This is an improvement C-security Category: Security issues C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicated zcash_proofs
4 participants