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

build(deps): Upgrade ECC crates for Zebra v2.0.0-rc.0 release candidate #8918

Merged
merged 15 commits into from
Oct 10, 2024

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Oct 8, 2024

Motivation

We need to upgrade the ECC dependencies for the next release.

This PR will also close #8809

Solution

Tests

All CI tests should pass.

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.

@oxarbitrage oxarbitrage added A-dependencies Area: Dependency file updates NU-6 Network Upgrade: NU6 specific tasks A-compatibility Area: Compatibility with other nodes or wallets, or standard rules P-High 🔥 labels Oct 8, 2024
@oxarbitrage oxarbitrage requested a review from a team as a code owner October 8, 2024 14:33
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team October 8, 2024 14:33
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 8, 2024
@str4d
Copy link
Contributor

str4d commented Oct 9, 2024

Note that by upgrading to those dependencies without setting the mainnet activation height, you are creating a mixed-consensus node state. I don't know how you manage that in Zebra; in zcashd we upgraded to these in the 6.0.0 release PR itself (not the RC), in the same commit as the mainnet activation height. To split up the upgrade, we upgraded first (using a [patch.crates-io] directive) to the librustzcash commit right before setting the mainnet activation height.

@oxarbitrage
Copy link
Contributor Author

Ok, thanks for the head-up @str4d !

However, when i tried to do it as that, it seems to be an inconsistency problem with the revisions for zcash_client_backend that zcashd does not has to build. Can you take a look?

@oxarbitrage oxarbitrage marked this pull request as draft October 9, 2024 12:49
@oxarbitrage oxarbitrage removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 9, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 9, 2024
@oxarbitrage
Copy link
Contributor Author

@Pili FYI: We decided with @str4d and other ECC folks in a meeting today that we want to do the Zebra v2.0.0-rc.0 with the patches pointing to commits just before the mainnet activation heights, this is the same zcashd did and should be the less risky process to follow.

To go over #8918 (comment) other patches were also needed. I am updating the PR with all that but i think we are back in track again.

@oxarbitrage oxarbitrage marked this pull request as ready for review October 9, 2024 18:51
@oxarbitrage oxarbitrage changed the title build(deps): Upgrade ECC crates for 2.0 release candidate build(deps): Upgrade ECC crates for Zebra v2.0.0-rc.0 release candidate Oct 9, 2024
@oxarbitrage oxarbitrage self-assigned this Oct 9, 2024
arya2
arya2 previously approved these changes Oct 9, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Looks good, I left a couple suggestions for cleanup and it looks like the entry for elasticsearch in the deny.toml is outdated, but there are no blockers.

Cargo.toml Outdated Show resolved Hide resolved
deny.toml Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

I tried to remove elastic in 6c21ecb but had to revert in a568cb8

Co-authored-by: Arya <aryasolhi@gmail.com>
@mpguerra mpguerra linked an issue Oct 10, 2024 that may be closed by this pull request
17 tasks
mergify bot added a commit that referenced this pull request Oct 10, 2024
@mergify mergify bot merged commit 8cd4d96 into main Oct 10, 2024
205 checks passed
@mergify mergify bot deleted the upgrade-ecc-deps--for-2.0 branch October 10, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules 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 P-High 🔥
Projects
None yet
3 participants