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.

Sign up for GitHub

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

Jump to bottom

change: set NU5 mainnet activation height and current network protocol version #4390

Merged
merged 5 commits into from
May 19, 2022
Merged

change: set NU5 mainnet activation height and current network protocol version #4390

merged 5 commits into from
May 19, 2022

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented May 12, 2022 •
edited by teor2345
Loading

Motivation

We need to set height and protocol for NU5 mainnet activation.

Specifications

https://zips.z.cash/zip-0252

image

Designs

Solution

Closes #4115

Review

Anyone can review, must be merged before the next release.

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 May 12, 2022 16:45
@conradoplg conradoplg requested review from oxarbitrage and removed request for a team May 12, 2022 16:45
@dconnolly dconnolly added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks P-Medium ⚡ A-network Area: Network protocol updates or fixes labels May 12, 2022
@conradoplg conradoplg marked this pull request as draft May 12, 2022 17:24
@conradoplg
Copy link
Collaborator Author

Moved to draft, need to fix some tests

@conradoplg conradoplg marked this pull request as ready for review May 12, 2022 23:06
@teor2345 teor2345 changed the title change: set NU5 mainnet activation height and minimum protocol version change: set NU5 mainnet activation height and current network protocol version May 13, 2022
teor2345
teor2345 reviewed May 13, 2022
View reviewed changes
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.

Looks good to me, but this is consensus-critical, so I'd like someone else to double-check.

teor2345
teor2345 previously requested changes May 16, 2022
View reviewed changes
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 think this PR has to merge after:

Otherwise, we'll be declaring support for NU5 mainnet without any of the required changes.
(I'm not sure there are any, but it's good practice anyway.)

@teor2345
Copy link
Contributor

I opened PR #4398 to fix the lightwalletd test failure.

@teor2345
Copy link
Contributor

@Mergifyio update

@teor2345
Copy link
Contributor

I'm updating this PR to clear most of the test failures, the remaining ones are:

@mergify
Copy link
Contributor

mergify bot commented May 18, 2022

update

✅ Branch has been successfully updated

@teor2345 teor2345 dismissed their stale review May 18, 2022 20:03

I turned this dependency into a Depends-On line

teor2345
teor2345 approved these changes May 18, 2022
View reviewed changes
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.

This should automatically merge after PR #4405 merges

teor2345
teor2345 suggested changes May 18, 2022
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

NU5 is now included in the getblockchaininfo RPC snapshot:
https://github.com/ZcashFoundation/zebra/runs/6486090115?check_suite_focus=true#step:13:4694

Looks like we need to run:

cargo insta test --review --delete-unreferenced-snapshots

@conradoplg
Copy link
Collaborator Author

Thanks, done!

teor2345
teor2345 approved these changes May 18, 2022
View reviewed changes
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.

Looks good, let's go!

@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented May 18, 2022

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request May 18, 2022
@mergify
Merge of #4390
84cb89c
@mergify mergify bot mentioned this pull request May 18, 2022
Closed
51 tasks
@mergify mergify bot merged commit 9aaf0ed into main May 19, 2022
@mergify mergify bot deleted the nu5-mainnet-height-protocol branch May 19, 2022 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dconnolly dconnolly dconnolly left review comments

@teor2345 teor2345 teor2345 approved these changes

@oxarbitrage oxarbitrage Awaiting requested review from oxarbitrage oxarbitrage is a code owner automatically assigned from ZcashFoundation/network-reviewers

Assignees
No one assigned
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set the NU5 mainnet activation height and network protocol version
3 participants
@conradoplg @teor2345 @dconnolly