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

change(network): Add configurable network parameters to Network::Testnet #7924

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 17 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
8 changes: 4 additions & 4 deletions zebra-chain/src/block/tests/vectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ fn block_test_vectors_height_mainnet() {
fn block_test_vectors_height_testnet() {
let _init_guard = zebra_test::init();

block_test_vectors_height(Testnet);
block_test_vectors_height(Testnet(None.into()));
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
}

/// Test that the block test vector indexes match the heights in the block data,
Expand All @@ -213,7 +213,7 @@ fn block_test_vectors_height(network: Network) {
zebra_test::vectors::MAINNET_BLOCKS.iter(),
zebra_test::vectors::MAINNET_FINAL_SAPLING_ROOTS.clone(),
),
Testnet => (
Testnet(_) => (
arya2 marked this conversation as resolved.
Show resolved Hide resolved
zebra_test::vectors::TESTNET_BLOCKS.iter(),
zebra_test::vectors::TESTNET_FINAL_SAPLING_ROOTS.clone(),
),
Expand Down Expand Up @@ -254,7 +254,7 @@ fn block_commitment_mainnet() {
fn block_commitment_testnet() {
let _init_guard = zebra_test::init();

block_commitment(Testnet);
block_commitment(Testnet(None.into()));
}

/// Check that the block commitment field parses without errors.
Expand All @@ -267,7 +267,7 @@ fn block_commitment(network: Network) {
zebra_test::vectors::MAINNET_BLOCKS.iter(),
zebra_test::vectors::MAINNET_FINAL_SAPLING_ROOTS.clone(),
),
Testnet => (
Testnet(_) => (
zebra_test::vectors::TESTNET_BLOCKS.iter(),
zebra_test::vectors::TESTNET_FINAL_SAPLING_ROOTS.clone(),
),
Expand Down
10 changes: 5 additions & 5 deletions zebra-chain/src/history_tree/tests/vectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ use zebra_test::vectors::{
#[test]
fn push_and_prune() -> Result<()> {
push_and_prune_for_network_upgrade(Network::Mainnet, NetworkUpgrade::Heartwood)?;
push_and_prune_for_network_upgrade(Network::Testnet, NetworkUpgrade::Heartwood)?;
push_and_prune_for_network_upgrade(Network::Testnet(None.into()), NetworkUpgrade::Heartwood)?;
push_and_prune_for_network_upgrade(Network::Mainnet, NetworkUpgrade::Canopy)?;
push_and_prune_for_network_upgrade(Network::Testnet, NetworkUpgrade::Canopy)?;
push_and_prune_for_network_upgrade(Network::Testnet(None.into()), NetworkUpgrade::Canopy)?;
Ok(())
}

Expand All @@ -38,7 +38,7 @@ fn push_and_prune_for_network_upgrade(
) -> Result<()> {
let (blocks, sapling_roots) = match network {
Network::Mainnet => (&*MAINNET_BLOCKS, &*MAINNET_FINAL_SAPLING_ROOTS),
Network::Testnet => (&*TESTNET_BLOCKS, &*TESTNET_FINAL_SAPLING_ROOTS),
Network::Testnet(_) => (&*TESTNET_BLOCKS, &*TESTNET_FINAL_SAPLING_ROOTS),
};
let height = network_upgrade.activation_height(network).unwrap().0;

Expand Down Expand Up @@ -115,14 +115,14 @@ fn upgrade() -> Result<()> {
// The history tree only exists Hearwood-onward, and the only upgrade for which
// we have vectors since then is Canopy. Therefore, only test the Heartwood->Canopy upgrade.
upgrade_for_network_upgrade(Network::Mainnet, NetworkUpgrade::Canopy)?;
upgrade_for_network_upgrade(Network::Testnet, NetworkUpgrade::Canopy)?;
upgrade_for_network_upgrade(Network::Testnet(None.into()), NetworkUpgrade::Canopy)?;
Ok(())
}

fn upgrade_for_network_upgrade(network: Network, network_upgrade: NetworkUpgrade) -> Result<()> {
let (blocks, sapling_roots) = match network {
Network::Mainnet => (&*MAINNET_BLOCKS, &*MAINNET_FINAL_SAPLING_ROOTS),
Network::Testnet => (&*TESTNET_BLOCKS, &*TESTNET_FINAL_SAPLING_ROOTS),
Network::Testnet(_) => (&*TESTNET_BLOCKS, &*TESTNET_FINAL_SAPLING_ROOTS),
};
let height = network_upgrade.activation_height(network).unwrap().0;

Expand Down
13 changes: 12 additions & 1 deletion zebra-chain/src/parameters/arbitrary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use proptest::prelude::*;

use super::NetworkUpgrade;
use super::{Network, NetworkUpgrade};

impl NetworkUpgrade {
/// Generates network upgrades.
Expand Down Expand Up @@ -32,3 +32,14 @@ impl NetworkUpgrade {
.boxed()
}
}

#[cfg(any(test, feature = "proptest-impl"))]
impl Arbitrary for Network {
type Parameters = ();

fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
prop_oneof![Just(Self::Mainnet), Just(Self::new_testnet()),].boxed()
}

type Strategy = BoxedStrategy<Self>;
}
4 changes: 3 additions & 1 deletion zebra-chain/src/parameters/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ pub fn genesis_hash(network: Network) -> block::Hash {
// zcash-cli getblockhash 0
Network::Mainnet => "00040fe8ec8471911baa1db1266ea15dd06b4a8a5c453883c000b031973dce08",
// zcash-cli -testnet getblockhash 0
Network::Testnet => "05a60a92d99d85997cce3b87616c089f6124d7342af37106edc76126334a2c38",
Network::Testnet(params) => params
.genesis_hash()
.unwrap_or("05a60a92d99d85997cce3b87616c089f6124d7342af37106edc76126334a2c38"),
}
.parse()
.expect("hard-coded hash parses")
Expand Down
146 changes: 134 additions & 12 deletions zebra-chain/src/parameters/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ use thiserror::Error;

use crate::{
block::{Height, HeightDiff},
parameters::NetworkUpgrade::Canopy,
parameters::NetworkUpgrade::{self, Canopy},
};

#[cfg(any(test, feature = "proptest-impl"))]
use proptest_derive::Arbitrary;

#[cfg(test)]
mod tests;

Expand Down Expand Up @@ -53,21 +50,109 @@ const ZIP_212_GRACE_PERIOD_DURATION: HeightDiff = 32_256;

/// An enum describing the possible network choices.
#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash, Serialize, Deserialize)]
#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
pub enum Network {
/// The production mainnet.
#[default]
Mainnet,

/// The oldest public test network.
Testnet,
Testnet(#[serde(skip)] TestnetParameters),
}

/// Configures testnet network parameters to use instead of default values.
#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash, Serialize)]
pub struct TestnetParameters(Option<NetworkParameters>);
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
teor2345 marked this conversation as resolved.
Show resolved Hide resolved

impl std::ops::Deref for TestnetParameters {
type Target = Option<NetworkParameters>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl From<Option<NetworkParameters>> for TestnetParameters {
fn from(value: Option<NetworkParameters>) -> Self {
Self(value)
}
}

impl TestnetParameters {
/// Returns `network_id` in network parameters, if any.
pub fn network_id(self) -> Option<[u8; 4]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking self by value only works while this is a Copy type. Instead, take &self (by reference) to make a future transition to a non-Copy type easier.

self.0.and_then(NetworkParameters::network_id)
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
}

/// Returns `default_port` in network parameters, if any.
pub fn default_port(self) -> Option<u16> {
self.0.and_then(NetworkParameters::default_port)
}

/// Returns `cache_name` in network parameters, if any.
pub fn cache_name(self) -> Option<&'static str> {
self.0.and_then(NetworkParameters::cache_name)
}

/// Returns `genesis_hash` in network parameters, if any.
pub fn genesis_hash(self) -> Option<&'static str> {
self.0.and_then(NetworkParameters::genesis_hash)
}

/// Returns `activation_heights` in network parameters, if any.
pub fn activation_heights(self) -> Option<&'static [(Height, NetworkUpgrade)]> {
self.0.and_then(NetworkParameters::activation_heights)
}
}

/// Configures network parameters to use instead of default values.
#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash, Serialize)]
pub struct NetworkParameters {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the first useful parameter to add is activation_heights, let's open a ticket?
In another ticket it would be useful to add an NU6 variant and a random consensus branch ID.

/// Used as the network magic, Zebra will reject messages and connections from peers
/// with a different network magic.
network_id: Option<[u8; 4]>,
teor2345 marked this conversation as resolved.
Show resolved Hide resolved

/// Default port for the network
default_port: Option<u16>,

/// Network portion of zebra-state cache path
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
cache_name: Option<&'static str>,

/// Genesis hash for this testnet
genesis_hash: Option<&'static str>,
teor2345 marked this conversation as resolved.
Show resolved Hide resolved

/// Activation heights for this testnet
activation_heights: Option<&'static [(Height, NetworkUpgrade)]>,
Comment on lines +110 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest making all of these parameters required, and using the default for the public testnet if the parameter isn't supplied. Then we can delete TestnetParameters and just use NetworkParameters instead.

This avoids:

  • the optional-of-optionals design, which is difficult to understand
  • re-implementing each method twice for NetworkParameters and TestnetParameters
  • poor test coverage due to a large number of code paths

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually the unique type is listed first.

Suggested change
activation_heights: Option<&'static [(Height, NetworkUpgrade)]>,
activation_heights: Option<&'static [(NetworkUpgrade, Height)]>,

}

impl NetworkParameters {
fn network_id(self) -> Option<[u8; 4]> {
self.network_id
}
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
fn default_port(self) -> Option<u16> {
self.default_port
}
fn cache_name(self) -> Option<&'static str> {
self.cache_name
}
fn genesis_hash(self) -> Option<&'static str> {
self.genesis_hash
}
fn activation_heights(self) -> Option<&'static [(Height, NetworkUpgrade)]> {
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
self.activation_heights
}
}

/// The network id to expect in AddrV2 messages.
#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Hash, Serialize, Deserialize)]
pub struct NetworkId {
ipv4: u8,
ipv6: u8,
}
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
impl From<Network> for &'static str {
fn from(network: Network) -> &'static str {
match network {
Network::Mainnet => "Mainnet",
Network::Testnet => "Testnet",
Network::Testnet(_) => "Testnet",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sort of correct, but it replaces custom testnets with the default in the config. Let's document that behaviour.

}
}
}
Expand All @@ -85,11 +170,26 @@ impl fmt::Display for Network {
}

impl Network {
/// Creates a new `Testnet` with no parameters.
pub fn new_testnet() -> Self {
Copy link
Contributor

@teor2345 teor2345 Nov 8, 2023

Choose a reason for hiding this comment

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

It looks like we're using this method for both:

  • the canonical public testnet used in tests
  • the custom testnet parameters used by zebrad when configured with Testnet

They can be the same, but we also want to make it easy for them to be different. And make the code easy to maintain.

So here's what I suggest to make the code clearer and easier to maintain:

  • use a PUBLIC_TESTNET constant in tests, and make it test-only/proptest-impl using #cfg
  • use a new_configured_testnet() method in production code, it should only be called once when the network config is loaded, so it can be pub(crate)
  • make all the other methods that can create a testnet into test-only/proptest-impl using #cfg

After that change, anything that doesn't compile is an incorrect use of the public testnet in production code, or an incorrect use of the configured testnet in test code.

We'll also need tests for custom parameter changes, so we can't make those methods private, they need to be called from tests.

Suggested change
pub fn new_testnet() -> Self {
#[cfg(any(test, feature = "proptest-impl"))]
pub(crate) fn new_public_testnet() -> Self {

Self::new_testnet_with_parameters(None)
}

/// Creates a new `Testnet` with the provided parameters.
pub fn new_testnet_with_parameters(parameters: impl Into<TestnetParameters>) -> Self {
Copy link
Contributor

@teor2345 teor2345 Nov 8, 2023

Choose a reason for hiding this comment

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

Except I think this one needs to be private so it can be called from both new_public_testnet() and new_configured_testnet(). (But not anything else.)

Suggested change
pub fn new_testnet_with_parameters(parameters: impl Into<TestnetParameters>) -> Self {
fn new_testnet_with_parameters(parameters: impl Into<TestnetParameters>) -> Self {

Self::Testnet(parameters.into())
}

/// Creates a new `Mainnet`.
pub fn new_mainnet() -> Self {
Self::Mainnet
}

/// Get the default port associated to this network.
pub fn default_port(&self) -> u16 {
match self {
Network::Mainnet => 8233,
Network::Testnet => 18233,
Network::Testnet(params) => params.default_port().unwrap_or(18233),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

mandatory_checkpoint_height should be modified to avoid a panic.


Expand All @@ -116,7 +216,8 @@ impl Network {
pub fn bip70_network_name(&self) -> String {
match self {
Network::Mainnet => "main".to_string(),
Network::Testnet => "test".to_string(),
// TODO: add network name field?
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
Network::Testnet { .. } => "test".to_string(),
}
}

Expand All @@ -125,9 +226,30 @@ impl Network {
self.to_string().to_ascii_lowercase()
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
}

/// Return the network cache name.
pub fn cache_name(&self) -> String {
self.testnet_parameters()
.cache_name()
.map_or_else(|| self.lowercase_name(), ToString::to_string)
}

/// Returns `true` if this network is `Mainnet`.
pub fn is_mainnet(&self) -> bool {
*self == Network::Mainnet
}

/// Returns `true` if this network is a testing network.
pub fn is_a_test_network(&self) -> bool {
*self != Network::Mainnet
pub fn is_testnet(&self) -> bool {
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
matches!(*self, Network::Testnet(_))
}

/// Returns testnet parameters, if any.
pub fn testnet_parameters(&self) -> TestnetParameters {
if let Network::Testnet(testnet_parameters) = *self {
testnet_parameters
} else {
None.into()
}
}
}

Expand All @@ -137,7 +259,7 @@ impl FromStr for Network {
fn from_str(string: &str) -> Result<Self, Self::Err> {
match string.to_lowercase().as_str() {
"mainnet" => Ok(Network::Mainnet),
"testnet" => Ok(Network::Testnet),
"testnet" => Ok(Network::new_testnet()),
_ => Err(InvalidNetworkError(string.to_owned())),
}
}
Expand Down
12 changes: 8 additions & 4 deletions zebra-chain/src/parameters/network_upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,9 @@ impl NetworkUpgrade {
};
match network {
Mainnet => mainnet_heights,
Testnet => testnet_heights,

// TODO: use params if available
Testnet(params) => params.activation_heights().unwrap_or(testnet_heights),
Comment on lines +275 to +276
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this TODO is implemented?

}
.iter()
.cloned()
Expand Down Expand Up @@ -388,9 +390,11 @@ impl NetworkUpgrade {
height: block::Height,
) -> Option<Duration> {
match (network, height) {
(Network::Testnet, height) if height < TESTNET_MINIMUM_DIFFICULTY_START_HEIGHT => None,
(Network::Testnet(_), height) if height < TESTNET_MINIMUM_DIFFICULTY_START_HEIGHT => {
teor2345 marked this conversation as resolved.
Show resolved Hide resolved
None
}
(Network::Mainnet, _) => None,
(Network::Testnet, _) => {
(Network::Testnet(_), _) => {
let network_upgrade = NetworkUpgrade::current(network, height);
Some(network_upgrade.target_spacing() * TESTNET_MINIMUM_DIFFICULTY_GAP_MULTIPLIER)
}
Expand Down Expand Up @@ -456,7 +460,7 @@ impl NetworkUpgrade {
pub fn is_max_block_time_enforced(network: Network, height: block::Height) -> bool {
match network {
Network::Mainnet => true,
Network::Testnet => height >= TESTNET_MAX_TIME_START_HEIGHT,
Network::Testnet(_) => height >= TESTNET_MAX_TIME_START_HEIGHT,
}
}
/// Returns the NetworkUpgrade given an u32 as ConsensusBranchId
Expand Down
10 changes: 5 additions & 5 deletions zebra-chain/src/parameters/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ fn activation_bijective() {
let mainnet_nus: HashSet<&NetworkUpgrade> = mainnet_activations.values().collect();
assert_eq!(MAINNET_ACTIVATION_HEIGHTS.len(), mainnet_nus.len());

let testnet_activations = NetworkUpgrade::activation_list(Testnet);
let testnet_activations = NetworkUpgrade::activation_list(Testnet(None.into()));
let testnet_heights: HashSet<&block::Height> = testnet_activations.keys().collect();
assert_eq!(TESTNET_ACTIVATION_HEIGHTS.len(), testnet_heights.len());

Expand All @@ -38,7 +38,7 @@ fn activation_extremes_mainnet() {
#[test]
fn activation_extremes_testnet() {
let _init_guard = zebra_test::init();
activation_extremes(Testnet)
activation_extremes(Testnet(None.into()))
}

/// Test the activation_list, activation_height, current, and next functions
Expand Down Expand Up @@ -115,7 +115,7 @@ fn activation_consistent_mainnet() {
#[test]
fn activation_consistent_testnet() {
let _init_guard = zebra_test::init();
activation_consistent(Testnet)
activation_consistent(Testnet(None.into()))
}

/// Check that the `activation_height`, `is_activation_height`,
Expand Down Expand Up @@ -175,7 +175,7 @@ fn branch_id_extremes_mainnet() {
#[test]
fn branch_id_extremes_testnet() {
let _init_guard = zebra_test::init();
branch_id_extremes(Testnet)
branch_id_extremes(Testnet(None.into()))
}

/// Test the branch_id_list, branch_id, and current functions for `network` with
Expand Down Expand Up @@ -213,7 +213,7 @@ fn branch_id_consistent_mainnet() {
#[test]
fn branch_id_consistent_testnet() {
let _init_guard = zebra_test::init();
branch_id_consistent(Testnet)
branch_id_consistent(Testnet(None.into()))
}

/// Check that the branch_id and current functions are consistent for `network`.
Expand Down
Loading
Loading