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 1 commit
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
31 changes: 31 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,37 @@
pub z_future: Option<BlockHeight>,
}

impl LocalNetwork {
pub fn new(

Check warning on line 47 in components/zcash_protocol/src/local_consensus.rs

View check run for this annotation

Codecov / codecov/patch

components/zcash_protocol/src/local_consensus.rs#L47

Added line #L47 was not covered by tests
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 {
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)),

Check warning on line 61 in components/zcash_protocol/src/local_consensus.rs

View check run for this annotation

Codecov / codecov/patch

components/zcash_protocol/src/local_consensus.rs#L56-L61

Added lines #L56 - L61 were not covered by tests
Oscar-Pepper marked this conversation as resolved.
Show resolved Hide resolved
}
}

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

Check warning on line 67 in components/zcash_protocol/src/local_consensus.rs

View check run for this annotation

Codecov / codecov/patch

components/zcash_protocol/src/local_consensus.rs#L66-L67

Added lines #L66 - L67 were not covered by tests
}

/// Creates a `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)

Check warning on line 73 in components/zcash_protocol/src/local_consensus.rs

View check run for this annotation

Codecov / codecov/patch

components/zcash_protocol/src/local_consensus.rs#L72-L73

Added lines #L72 - L73 were not covered by tests
}
}

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