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

zcash_protocol: Add constructors to LocalNetwork #1281

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions components/zcash_protocol/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ and this library adheres to Rust's notion of
### Added
- `zcash_protocol::memo`:
- `impl TryFrom<&MemoBytes> for Memo`
- `zcash_protocol::local_consensus`:
- `new`, `all_upgrades_active` and `canopy_active` constructors to `LocalNetwork`
Copy link
Contributor

Choose a reason for hiding this comment

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

PR needs a rebase because this now merge-conflicts with the 0.1.1 release (in that it will end up editing the 0.1.1 changelog).


### Removed
- `unstable-nu6` and `zfuture` feature flags (use `--cfg zcash_unstable=\"nu6\"`
Expand Down
48 changes: 48 additions & 0 deletions components/zcash_protocol/src/local_consensus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,54 @@ pub struct LocalNetwork {
pub z_future: Option<BlockHeight>,
}

impl LocalNetwork {
/// Construct a new `LocalNetwork` where all network upgrade activation heights are specified.
///
/// Sequential network upgrades may share the same activation height but this constructor will
/// panic if the order in which network upgrades activate is changed.
pub fn new(
Oscar-Pepper marked this conversation as resolved.
Show resolved Hide resolved
overwinter: u64,
sapling: u64,
blossom: u64,
heartwood: u64,
canopy: u64,
nu5: u64,
Comment on lines +52 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realised that all of these arguments are u64. Two questions:

  • Why not BlockHeight?
  • If not BlockHeight, why u64? The internal casts to u32 will silently truncate the heights, which is undocumented and confusing. Additionally, these truncations occur after the assertion, which means that a caller can bypass the ordering requirement by setting the high bits of the heights.

The latter question is blocking; these should be u32 at a minimum, and I don't see a good reason for these to not be BlockHeight.

) -> Self {
assert!(
overwinter <= sapling
&& sapling <= blossom
&& blossom <= heartwood
&& heartwood <= canopy
&& canopy <= nu5,
"Network upgrade activation heights must be in ascending order"
);

LocalNetwork {
overwinter: Some(BlockHeight::from_u32(overwinter as u32)),
sapling: Some(BlockHeight::from_u32(sapling as u32)),
blossom: Some(BlockHeight::from_u32(blossom as u32)),
heartwood: Some(BlockHeight::from_u32(heartwood as u32)),
canopy: Some(BlockHeight::from_u32(canopy as u32)),
nu5: Some(BlockHeight::from_u32(nu5 as u32)),
Oscar-Pepper marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(zcash_unstable = "nu6")]
nu6: None,
#[cfg(zcash_unstable = "zfuture")]
z_future: None,
}
}

/// Construct a new `LocalNetwork` with all network upgrades initially active.
pub fn all_upgrades_active() -> Self {
Self::new(1, 1, 1, 1, 1, 1)
}

/// Construct a new `LocalNetwork` with all network upgrades up to and including canopy
/// initally active.
pub fn canopy_active(nu5_activation_height: u64) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto: this should at least be u32, and probably a BlockHeight.

Self::new(1, 1, 1, 1, 1, nu5_activation_height)
}
}

/// Parameters implementation for `LocalNetwork`
impl Parameters for LocalNetwork {
fn network_type(&self) -> NetworkType {
Expand Down