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

arya2 · 2023-11-08T00:47:08Z

Motivation

This PR is a step towards adding the Regtest network to Zebra, adding optional cache_name, genesis_hash, network_id, default_port, and activation_heights fields to Network::Testnet.

For testing ZSAs, it's not yet clear what other fields will be needed or whether the scope of this approach will be viable.

Part of #7845.

PR Author Checklist

Check before marking the PR as ready for review:

Will the PR name make sense to users?
Does the PR have a priority label?
Have you added or updated tests?
Is the documentation up to date?

For significant changes:

Is there a summary in the CHANGELOG?
Can these changes be split into multiple PRs?

Solution

Adds new fields to TestnetParameters in Testnet
Uses values in TestnetParameters, if any, instead of hard-coded defaults
Updates references

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

Does the PR scope match the ticket?
Are there enough tests to make sure it works? Do the tests cover the PR motivation?
Are all the PR blockers dealt with?
PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

…enesis_hash from params

zebra-chain/src/parameters/network.rs

teor2345 · 2023-11-08T01:47:30Z

zebra-chain/src/sprout/tests/tree.rs

            &*vectors::MAINNET_FINAL_SPROUT_ROOTS,
            MAINNET_FIRST_JOINSPLIT_HEIGHT,
        ),
-        Network::Testnet => (
+        Network::Testnet(_) => (
            &*vectors::TESTNET_BLOCKS,
            &*vectors::TESTNET_FINAL_SPROUT_ROOTS,
            TESTNET_FIRST_JOINSPLIT_HEIGHT,


It seems like this code has been copied around a lot, maybe we should replace it with methods in test::vectors that take is_mainnet: bool?

This can happen in an earlier PR, and then all this test code doesn't need to change at all in this PR.

zebra-chain/src/work/difficulty.rs

teor2345 · 2023-11-08T01:50:00Z

zebra-consensus/src/block/subsidy/funding_streams.rs

@@ -95,7 +95,7 @@ fn funding_stream_address_period(height: Height, network: Network) -> u32 {
 fn funding_stream_address_index(height: Height, network: Network) -> usize {
    let num_addresses = match network {
        Network::Mainnet => FUNDING_STREAMS_NUM_ADDRESSES_MAINNET,
-        Network::Testnet => FUNDING_STREAMS_NUM_ADDRESSES_TESTNET,
+        Network::Testnet(_) => FUNDING_STREAMS_NUM_ADDRESSES_TESTNET,


Let's check what zcashd does here.

teor2345 · 2023-11-08T01:50:03Z

zebra-consensus/src/block/subsidy/funding_streams.rs

@@ -60,7 +60,7 @@ pub fn height_for_first_halving(network: Network) -> Height {
        Network::Mainnet => Canopy
            .activation_height(network)
            .expect("canopy activation height should be available"),
-        Network::Testnet => FIRST_HALVING_TESTNET,
+        Network::Testnet(_) => FIRST_HALVING_TESTNET,


Let's check what zcashd does here.

teor2345 · 2023-11-08T01:56:20Z

zebra-consensus/src/checkpoint/list.rs

+            Network::Testnet(_) => Self::from_str(TESTNET_CHECKPOINTS, network)
+                .expect("Hard-coded Testnet checkpoint list parses and validates"),
+        };
+
+        match checkpoint_list.hash(block::Height(0)) {
+            Some(hash) if hash == genesis_hash(network) => checkpoint_list,
+            Some(_) => {
+                panic!("The hard-coded genesis checkpoint does not match the network genesis hash")
+            }
+            None => unreachable!("Parser should have checked for a missing genesis checkpoint"),
+        }
+    }


This is only correct if network == Network::new_testnet(). If it's not, then the list should consist of only the checkpoints supplied by the custom network. Currently that's a single item list containing the genesis block hash.

~~If we implement it that way, then there's no need to check the genesis checkpoint in production code, and that check can be moved to a unit test.~~

If changing genesis is out of scope, then this should be the hard-coded testnet genesis block hash.

teor2345 · 2023-11-08T02:00:19Z

zebra-consensus/src/checkpoint/list.rs

        // Check that the list starts with the correct genesis block
        match checkpoints.iter().next() {
-            Some((block::Height(0), hash))
-                if (hash == &genesis_hash(Network::Mainnet)
-                    || hash == &genesis_hash(Network::Testnet)) => {}
+            Some((block::Height(0), hash)) if hash == &genesis_hash(network) => {}
            Some((block::Height(0), _)) => {
-                Err("the genesis checkpoint does not match the Mainnet or Testnet genesis hash")?
+                Err("the genesis checkpoint does not match the network genesis hash")?
            }
            Some(_) => Err("checkpoints must start at the genesis block height 0")?,
            None => Err("there must be at least one checkpoint, for the genesis block")?,


Rather than complicating this code by adding a network parameter, let's move the checks that depend on network into new(). (And replace the similar check already in new().)

zebra-chain/src/parameters/network.rs

teor2345 · 2023-11-08T02:07:47Z

zebra-network/src/config.rs

@@ -225,7 +225,7 @@ impl Config {
    pub fn initial_peer_hostnames(&self) -> &IndexSet<String> {
        match self.network {
            Network::Mainnet => &self.initial_mainnet_peers,
-            Network::Testnet => &self.initial_testnet_peers,
+            Network::Testnet(_) => &self.initial_testnet_peers,


This is incorrect and could cause issues with the reliability of the public testnet. A custom testnet needs to supply its own peers if it changes certain parameters.

Currently those are the genesis hash, the activation heights, and the network magic. Let's write code that checks if these parameters match the default testnet parameters?

There's a specific way to do that so that each newly added parameter field causes a compile error, and we have to add it to the check.

I think the specific check that we want here is:
If the network is an incompatible custom testnet (that is, it changes any incompatible parameters from the default testnet values), and the configured initial testnet peers contains any of the default testnet peers, then panic.

A simpler check would be to just check against the default testnet for now.

zebra-network/src/protocol/external/codec.rs

zebra-network/src/protocol/external/types.rs

teor2345 · 2023-11-08T02:14:17Z

zebra-state/src/service/finalized_state.rs

            let mut blocks_size_to_dump = match self.network {
                Network::Mainnet => MAINNET_AWAY_FROM_TIP_BULK_SIZE,
-                Network::Testnet => TESTNET_AWAY_FROM_TIP_BULK_SIZE,
+                Network::Testnet(_) => TESTNET_AWAY_FROM_TIP_BULK_SIZE,
            };


This code is incorrect, and will fail if the public or custom testnet has a significant number of 2MB blocks. Instead, the value of MAINNET_AWAY_FROM_TIP_BULK_SIZE should be used for all networks.

teor2345 · 2023-11-08T02:15:22Z

zebra-state/src/service/finalized_state/disk_format/transparent.rs

+        (Testnet(_), PayToPublicKeyHash { .. }) => 2,
+        (Testnet(_), PayToScriptHash { .. }) => 3,


This might be incorrect for custom testnets if they change their address formats. What does zcashd do here?

What does zcashd do here?

I'm not sure, I imagine it may be storing the entire prefix, I'll ask.

I think this could be fine as long as we use always different cache directories, but it would be safer to use different values for Regtest or custom testnets (without differentiating between different kinds of custom testnets).

teor2345 · 2023-11-08T02:19:30Z

zebra-consensus/src/parameters/subsidy.rs

@@ -107,7 +107,7 @@ lazy_static! {
    pub static ref FUNDING_STREAM_HEIGHT_RANGES: HashMap<Network, std::ops::Range<Height>> = {
        let mut hash_map = HashMap::new();
        hash_map.insert(Network::Mainnet, Height(1_046_400)..Height(2_726_400));
-        hash_map.insert(Network::Testnet, Height(1_028_500)..Height(2_796_000));
+        hash_map.insert(Network::new_testnet(), Height(1_028_500)..Height(2_796_000));


This will cause a panic with a cryptic error message for custom testnets:

zebra/zebra-consensus/src/block/subsidy/funding_streams.rs

Line 32 in 2efc6bf

let range = FUNDING_STREAM_HEIGHT_RANGES.get(&network).unwrap();

teor2345 · 2023-11-08T02:20:35Z

zebra-consensus/src/parameters/subsidy.rs

@@ -127,7 +127,7 @@ lazy_static! {
        testnet_addresses.insert(FundingStreamReceiver::Ecc, FUNDING_STREAM_ECC_ADDRESSES_TESTNET.iter().map(|a| a.to_string()).collect());
        testnet_addresses.insert(FundingStreamReceiver::ZcashFoundation, FUNDING_STREAM_ZF_ADDRESSES_TESTNET.iter().map(|a| a.to_string()).collect());
        testnet_addresses.insert(FundingStreamReceiver::MajorGrants, FUNDING_STREAM_MG_ADDRESSES_TESTNET.iter().map(|a| a.to_string()).collect());
-        addresses_by_network.insert(Network::Testnet, testnet_addresses);
+        addresses_by_network.insert(Network::new_testnet(), testnet_addresses);


This will cause a panic here for custom testnets:

zebra/zebra-consensus/src/block/subsidy/funding_streams.rs

Lines 126 to 128 in 2efc6bf

let address = &FUNDING_STREAM_ADDRESSES

.get(&network)

.expect("there is always another hash map as value for a given valid network")

teor2345 · 2023-11-08T21:02:22Z

zebra-chain/src/parameters/network.rs

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

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


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 {

teor2345 · 2023-11-08T21:05:25Z

zebra-chain/src/parameters/network.rs

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


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 {

teor2345

Draft review, I'll resolve most of it.

zebra-chain/src/parameters/network.rs

zebra-network/proptest-regressions/peer_set/set/tests/prop.txt

zebra-chain/src/parameters/network.rs

teor2345 · 2023-11-15T18:54:43Z

zebra-chain/src/parameters/network.rs

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


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.

zebra-chain/src/parameters/network.rs

zebra-chain/src/work/difficulty.rs

teor2345 · 2023-11-15T19:29:23Z

zebra-consensus/src/checkpoint/list.rs

+            Network::Testnet(_) => Self::from_str(TESTNET_CHECKPOINTS, network)
+                .expect("Hard-coded Testnet checkpoint list parses and validates"),
+        };
+
+        match checkpoint_list.hash(block::Height(0)) {
+            Some(hash) if hash == genesis_hash(network) => checkpoint_list,
+            Some(_) => {
+                panic!("The hard-coded genesis checkpoint does not match the network genesis hash")
+            }
+            None => unreachable!("Parser should have checked for a missing genesis checkpoint"),
+        }
+    }


If changing genesis is out of scope, then this should be the hard-coded testnet genesis block hash.

teor2345 · 2023-11-15T19:30:17Z

zebra-network/src/config.rs

@@ -225,7 +225,7 @@ impl Config {
    pub fn initial_peer_hostnames(&self) -> &IndexSet<String> {
        match self.network {
            Network::Mainnet => &self.initial_mainnet_peers,
-            Network::Testnet => &self.initial_testnet_peers,
+            Network::Testnet(_) => &self.initial_testnet_peers,


A simpler check would be to just check against the default testnet for now.

teor2345 · 2023-11-16T00:45:34Z

zebra-chain/src/parameters/network.rs

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


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.

arya2 · 2023-11-23T21:26:18Z

The changes in this PR and the requirements of the issue it was intended to close have diverged.

arya2 added 16 commits November 4, 2023 18:19

Adds testnet parameters

0926ecf

adds getter methods to TestnetParameters

9a4bf08

uses genesis hash from params

081e146

Applies suggestions from rustc

71a3bc1

Applies more suggestions from rustc, fixes tests

2e28b3e

restores serde::Deserialize impl for Network

5eed2f7

updates production references

b6d853f

updates an assert

beb8833

Moves a FromStr impl to a normal method to accept network and use g…

48cd8df

…enesis_hash from params

adds TODO comment

49c6105

updates doc comments with rename

6fbad12

simplifies Network method

f1b6c6b

use network activation heights from params

de0a603

use network id from testnet parameters

8fa3a53

documents new fields

cce6834

doc updates

b7d9697

arya2 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement I-usability Zebra is hard to understand or use A-network Area: Network protocol updates or fixes C-testing Category: These are tests C-feature Category: New features labels Nov 8, 2023

arya2 self-assigned this Nov 8, 2023

arya2 requested review from a team as code owners November 8, 2023 00:47

arya2 requested review from teor2345 and removed request for a team November 8, 2023 00:47

github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 8, 2023

This comment was marked as resolved.

Sign in to view

teor2345 reviewed Nov 8, 2023

View reviewed changes

zebra-chain/src/parameters/network.rs Show resolved Hide resolved

teor2345 reviewed Nov 8, 2023

View reviewed changes

zebra-chain/src/parameters/network.rs Outdated Show resolved Hide resolved

teor2345 reviewed Nov 8, 2023

View reviewed changes

zebra-chain/src/work/difficulty.rs Show resolved Hide resolved

teor2345 reviewed Nov 8, 2023

View reviewed changes

zebra-chain/src/parameters/network.rs Show resolved Hide resolved

teor2345 reviewed Nov 8, 2023

View reviewed changes

zebra-network/src/protocol/external/codec.rs Outdated Show resolved Hide resolved

teor2345 reviewed Nov 8, 2023

View reviewed changes

zebra-network/src/protocol/external/types.rs Show resolved Hide resolved

teor2345 reviewed Nov 8, 2023

View reviewed changes

arya2 added 4 commits November 9, 2023 14:51

uses ? instead of and_then()

236c26b

Replaces Testnet(None.into()) with Network::new_testnet()

bd2b188

reverts rename of is_a_test_network

1d33e7f

removes outdated todo

212a81b

teor2345 added P-Low ❄️ and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG do-not-merge Tells Mergify not to merge this PR labels Nov 16, 2023

teor2345 reviewed Nov 16, 2023

View reviewed changes

arya2 mentioned this pull request Nov 21, 2023

Add an empty NetworkParameters struct to Network::Testnet #7968

Closed

arya2 closed this Nov 23, 2023

arya2 mentioned this pull request Mar 20, 2024

Add protocol version information to fields on NetworkParameters #8367

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arya2 commented Nov 8, 2023

This comment was marked as resolved.

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023 •

edited

Loading

teor2345 Nov 15, 2023

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023

teor2345 Nov 15, 2023

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023

arya2 Nov 23, 2023 •

edited

Loading

teor2345 Nov 8, 2023

teor2345 Nov 8, 2023 •

edited

Loading

teor2345 Nov 8, 2023 •

edited

Loading

teor2345 Nov 8, 2023 •

edited

Loading

teor2345 left a comment

teor2345 Nov 15, 2023

teor2345 Nov 15, 2023

teor2345 Nov 15, 2023

teor2345 Nov 16, 2023

arya2 commented Nov 23, 2023 •

edited

Loading

		(Testnet(_), PayToPublicKeyHash { .. }) => 2,
		(Testnet(_), PayToScriptHash { .. }) => 3,

	let address = &FUNDING_STREAM_ADDRESSES
	.get(&network)
	.expect("there is always another hash map as value for a given valid network")

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

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

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

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

Conversation

arya2 commented Nov 8, 2023

Motivation

PR Author Checklist

Check before marking the PR as ready for review:

For significant changes:

Solution

Review

Reviewer Checklist

Follow Up Work

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

teor2345 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

teor2345 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 commented Nov 23, 2023 • edited Loading

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

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

teor2345 Nov 8, 2023 •

edited

Loading

arya2 Nov 23, 2023 •

edited

Loading

teor2345 Nov 8, 2023 •

edited

Loading

teor2345 Nov 8, 2023 •

edited

Loading

teor2345 Nov 8, 2023 •

edited

Loading

arya2 commented Nov 23, 2023 •

edited

Loading