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

ZSA integration (step 2): Add Network Upgrade 7 (Nu7) support to Zebra #15

Open
wants to merge 33 commits into
base: zsa-integration-zsadeps
Choose a base branch
from

Conversation

dmidem
Copy link

@dmidem dmidem commented Oct 17, 2024

This PR introduces support for Network Upgrade 7 (Nu7) in the Zebra codebase. This update is one of the steps towards enabling ZSA (Zcash Shielded Assets) support in Zebra.

Changes

  • Implemented Network Upgrade 7 (Nu7) support across the relevant crates.
  • Marked several areas with FIXME comments where further attention is needed, such as determining the proper activation height and other related details.

@dmidem dmidem changed the title Step2: Add Network Upgrade 7 (Nu7) support to Zebra Step 2: Add Network Upgrade 7 (Nu7) support to Zebra Oct 17, 2024
@dmidem dmidem requested a review from PaulLaux October 17, 2024 09:10
@dmidem dmidem changed the title Step 2: Add Network Upgrade 7 (Nu7) support to Zebra ZSA integration, step 2: Add Network Upgrade 7 (Nu7) support to Zebra Oct 18, 2024
@dmidem dmidem changed the title ZSA integration, step 2: Add Network Upgrade 7 (Nu7) support to Zebra ZSA integration (step 2): Add Network Upgrade 7 (Nu7) support to Zebra Oct 18, 2024
@dmidem dmidem deleted the branch zsa-integration-zsadeps October 29, 2024 12:28
@dmidem dmidem closed this Oct 29, 2024
@dmidem dmidem reopened this Oct 29, 2024
@PaulLaux PaulLaux requested a review from arya2 October 30, 2024 13:42
@@ -126,6 +133,8 @@ pub(super) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
(block::Height(1_028_500), Canopy),
(block::Height(1_842_420), Nu5),
(block::Height(2_976_000), Nu6),
// FIXME: TODO: Set a correct value for NU7
(block::Height(2_942_001), Nu7),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to defer setting this at all until the release PR for a version that deploys NU7 on Testnet and to set this in the configuration until then.

NetworkUpgrade::current() will return NU6 as the current network upgrade if the activation height is higher than NU7, so this should be set above the NU6 activation height.

Suggested change
(block::Height(2_942_001), Nu7),
// (block::Height(2_976_001), Nu7),

Choose a reason for hiding this comment

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

or consider putting this under the nu7 flag :#[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )]

Copy link
Author

Choose a reason for hiding this comment

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

Link to this comment copied to #37

@@ -216,6 +226,8 @@ pub(crate) const CONSENSUS_BRANCH_IDS: &[(NetworkUpgrade, ConsensusBranchId)] =
(Canopy, ConsensusBranchId(0xe9ff75a6)),
(Nu5, ConsensusBranchId(0xc2d6d0b4)),
(Nu6, ConsensusBranchId(0xc8e71055)),
// FIXME: use a proper value below
(Nu7, ConsensusBranchId(0xc8e71056)),
Copy link
Collaborator

@arya2 arya2 Oct 30, 2024

Choose a reason for hiding this comment

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

Zebra will panic if this doesn't match the one in zcash_protocol, so it would be good to generate a random one now, get started on the NU7 deployment ZIP, and make the change there as well (behind the compilation cfg flag).

Choose a reason for hiding this comment

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

yes, let's add a random number and the flag

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened a PR adding a draft NU7 deployment ZIP with 0x77190AD8 as the consensus branch id, it could still change, but we may as well use it here as it's much less likely to need an update later than a random number.

Copy link
Author

Choose a reason for hiding this comment

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

Link to this comment copied to #37

Comment on lines +110 to +111
(Testnet(params), Nu7) if params.is_default_testnet() => 170_111,
(Mainnet, Nu7) => 170_121,
Copy link
Collaborator

@arya2 arya2 Oct 30, 2024

Choose a reason for hiding this comment

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

The only requirement here is that there should be some versions between the prior min specified protocol versions and new ones in case something goes wrong with the prior NU deployment.

This PR could also update the CURRENT_NETWORK_PROTOCOL_VERSION (and should if it includes an NU7 activation height on Testnet, though it would be better to remove the Testnet activation height).

Update: 170_130 & 170_140 are the currently the values prescribed in the draft ZIP for NU7 deployment, though they may still change before it's out of draft status.

Suggested change
(Testnet(params), Nu7) if params.is_default_testnet() => 170_111,
(Mainnet, Nu7) => 170_121,
(Testnet(params), Nu7) if params.is_default_testnet() => 170_130,
(Mainnet, Nu7) => 170_140,

Copy link
Author

Choose a reason for hiding this comment

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

Link to this comment copied to #37

@@ -73,6 +73,8 @@ Heartwood = 903_800
Canopy = 1_028_500
NU5 = 1_842_420
NU6 = 2_000_000
# FIXME: Use a proper value for NU7.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This value is already okay. These test configs are just meant to ensure that valid past configs can still be parsed whenever the config is changed and, secondarily, to provide examples.

Suggested change
# FIXME: Use a proper value for NU7.

Copy link
Author

Choose a reason for hiding this comment

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

Link to this comment copied to #37

Copy link

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

added few minor comments

@@ -90,6 +94,8 @@ pub(super) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
(block::Height(1_046_400), Canopy),
(block::Height(1_687_104), Nu5),
(block::Height(2_726_400), Nu6),
// FIXME: TODO: Add NU7 with a correct value
// (block::Height(2_726_401), Nu7),

Choose a reason for hiding this comment

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

Let's add an estimated value under the flag #[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )]

Copy link
Author

Choose a reason for hiding this comment

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

Link to this comment copied to #37

@@ -126,6 +133,8 @@ pub(super) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
(block::Height(1_028_500), Canopy),
(block::Height(1_842_420), Nu5),
(block::Height(2_976_000), Nu6),
// FIXME: TODO: Set a correct value for NU7
(block::Height(2_942_001), Nu7),

Choose a reason for hiding this comment

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

or consider putting this under the nu7 flag :#[cfg(zcash_unstable = "nu6" /* TODO nu7 */ )]

@@ -216,6 +226,8 @@ pub(crate) const CONSENSUS_BRANCH_IDS: &[(NetworkUpgrade, ConsensusBranchId)] =
(Canopy, ConsensusBranchId(0xe9ff75a6)),
(Nu5, ConsensusBranchId(0xc2d6d0b4)),
(Nu6, ConsensusBranchId(0xc8e71055)),
// FIXME: use a proper value below
(Nu7, ConsensusBranchId(0xc8e71056)),

Choose a reason for hiding this comment

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

yes, let's add a random number and the flag

@@ -69,6 +70,11 @@ expression: info
"name": "NU6",
"activationheight": 2976000,
"status": "pending"
},
"c8e71056": {

Choose a reason for hiding this comment

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

same for a new random string

Copy link
Author

Choose a reason for hiding this comment

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

Link to this comment copied to #37

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.

3 participants