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(librustzcash): Upgrade zcash dependencies for NU6 #8746

Merged
merged 12 commits into from
Aug 12, 2024

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to use the current main version of librustzcash so we can remove unstable features.

We will get back to depend on a release after librustzcash components are released.

Close #8713

Solution

Upgrade, fix problems.

Draft until CI pass as there might be issues for example in the deny.toml.

Tests

No tests were added as our current suite should be enough coverage.

Follow-up Work

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.

@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Aug 6, 2024
@oxarbitrage oxarbitrage added A-dependencies Area: Dependency file updates NU-6 Network Upgrade: NU6 specific tasks labels Aug 6, 2024
@oxarbitrage oxarbitrage marked this pull request as ready for review August 8, 2024 17:31
@oxarbitrage oxarbitrage requested a review from a team as a code owner August 8, 2024 17:31
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team August 8, 2024 17:31
@oxarbitrage
Copy link
Contributor Author

The "Release crates / Check crate release dry run (pull_request)" check will not pass for this PR because we are relying on a GitHub repository for librustzcash instead of a version from crates.io. This is not allowed in our release process.

If this PR gets approved, we will need to perform an admin merge. As a side effect, all future pull requests will also require admin merges until we can revert to a proper release. An alternative is to make the "Release crates / Check crate release dry run (pull_request)" check not required. The test will fail, but we will still be able to merge. This should be a temporary measure, but it seems better than having to admin merge every subsequent pull request.

upbqdn
upbqdn previously approved these changes Aug 9, 2024
zebra-scan/src/service/scan_task/scan.rs Outdated Show resolved Hide resolved
deny.toml Outdated Show resolved Hide resolved
deny.toml Show resolved Hide resolved
Co-authored-by: Marek <mail@marek.onl>
@mpguerra
Copy link
Contributor

mpguerra commented Aug 9, 2024

An alternative is to make the "Release crates / Check crate release dry run (pull_request)" check not required. The test will fail, but we will still be able to merge. This should be a temporary measure, but it seems better than having to admin merge every subsequent pull request.

Let's go ahead and do this then. Does anyone know how/where to do this?

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

I think it's preferrable to point to a commit in the main branch, and not the main branch itself, e.g.

{ git = "https://github.com/zcash/librustzcash/", rev = "5a4a3e06dcd2cf5bdf79a7cd48709b58693c65f0" }

This will make it less likely to suddenly break if something incompatible ends up being merged into their main.

@oxarbitrage
Copy link
Contributor Author

I think it's preferrable to point to a commit in the main branch, and not the main branch itself, e.g.
...

I think we can do that for sure but by the talks we had with the ECC engineers in multiple meetings we decided to use the last main as all that it is and will be there is part of the next release, reducing the delta.

This will make it less likely to suddenly break if something incompatible ends up being merged into their main.

My understanding is that we actually want to break if something like that happens.

I don't mind changing this up to a commit but just wanted to point that out.

@conradoplg
Copy link
Collaborator

I think it's preferrable to point to a commit in the main branch, and not the main branch itself, e.g.
...

I think we can do that for sure but by the talks we had with the ECC engineers in multiple meetings we decided to use the last main as all that it is and will be there is part of the next release, reducing the delta.

This will make it less likely to suddenly break if something incompatible ends up being merged into their main.

My understanding is that we actually want to break if something like that happens.

I don't mind changing this up to a commit but just wanted to point that out.

I realized this is all moot since Cargo.lock already ensures the same commit will be used even if main is updated, so we're good. (I'm so used to working on a library that I forget about this 😆 )

@oxarbitrage
Copy link
Contributor Author

I realized this is all moot since Cargo.lock already ensures the same commit will be used even if main is updated, so we're good. (I'm so used to working on a library that I forget about this 😆 )

That's good to know, thanks for pointing it out.

@oxarbitrage
Copy link
Contributor Author

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Aug 12, 2024

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Aug 12, 2024
@mergify mergify bot merged commit a575576 into main Aug 12, 2024
173 of 179 checks passed
@mergify mergify bot deleted the upgrade-primitives-nu6 branch August 12, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG NU-6 Network Upgrade: NU6 specific tasks
Projects
None yet
4 participants