Applies some suggestions from code review

zebra-chain/src/parameters/error.rs

@@ -1,15 +1,8 @@
 //! Error types for zebra-chain parameters
 
-use std::fmt;
+use thiserror::Error;
 
 /// An error indicating that Zebra network is not supported in type conversions.
-#[derive(Debug)]
+#[derive(Clone, Debug, Error)]
-pub struct UnsupportedNetwork;
+#[error("Unsupported Zcash network parameters: {0}")]
-
+pub struct UnsupportedNetwork(pub String);
-impl fmt::Display for UnsupportedNetwork {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "unsupported Zcash network parameters")
-    }
-}
-
-impl std::error::Error for UnsupportedNetwork {}

zebra-chain/src/parameters/network.rs

@@ -66,8 +66,10 @@ impl NetworkParameters {
 }
 
 #[derive(Copy, Clone, Default, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]
-#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 /// An enum describing the kind of network, whether it's the production mainnet or a testnet.
+// Note: The order of these variants is important for correct bincode (de)serialization
+//       of history trees in the db format.
+// TODO: Replace bincode (de)serialization of `HistoryTreeParts` in a db format upgrade?
 pub enum NetworkKind {
     /// The production mainnet.
     #[default]
@@ -75,6 +77,9 @@ pub enum NetworkKind {
 
     /// A test network.
     Testnet,
+
+    /// Regtest mode
+    Regtest,
 }
 
 impl From<Network> for NetworkKind {
@@ -92,7 +97,8 @@ pub enum Network {
     #[default]
     Mainnet,
 
-    /// The oldest public test network.
+    /// A test network such as the default public testnet,
+    /// a configured testnet, or Regtest.
     Testnet(Arc<NetworkParameters>),
 }
 
@@ -112,26 +118,21 @@ impl NetworkKind {
     /// Return the network name as defined in
     /// [BIP70](https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki#paymentdetailspaymentrequest)
     pub fn bip70_network_name(&self) -> String {
-        match self {
+        if *self == Self::Mainnet {
-            Self::Mainnet => "main".to_string(),
+            "main".to_string()
-            Self::Testnet => "test".to_string(),
+        } else {
-        }
+            "test".to_string()
-    }
-
-    /// Converts a [`zcash_address::Network`] to a [`NetworkKind`].
-    pub fn from_zcash_address(network: zcash_address::Network) -> Self {
-        match network {
-            zcash_address::Network::Main => NetworkKind::Mainnet,
-            zcash_address::Network::Test | zcash_address::Network::Regtest => NetworkKind::Testnet,
         }
     }
 }
 
 impl From<NetworkKind> for &'static str {
     fn from(network: NetworkKind) -> &'static str {
+        // These should be different from the `Display` impl for `Network`
         match network {
             NetworkKind::Mainnet => "MainnetKind",
             NetworkKind::Testnet => "TestnetKind",
+            NetworkKind::Regtest => "RegtestKind",
         }
     }
 }
@@ -148,8 +149,7 @@ impl From<&Network> for &'static str {
             Network::Mainnet => "Mainnet",
             // TODO:
             // - Add a `name` field to use here instead of checking `is_default_testnet()`
-            // - Find out what zcashd calls the regtest cache dir for the `Network::new_regtest()` method, or
+            // - zcashd calls the Regtest cache dir 'regtest' (#8327).
-            //   if it always uses an ephemeral db, and do the same for Regtest in Zebra (#8327).
             Network::Testnet(params) if params.is_default_testnet() => "Testnet",
             Network::Testnet(_params) => "UnknownTestnet",
         }
@@ -186,18 +186,11 @@ impl Network {
     pub fn kind(&self) -> NetworkKind {
         match self {
             Network::Mainnet => NetworkKind::Mainnet,
+            // TODO: Return `NetworkKind::Regtest` if the parameters match the default Regtest params
             Network::Testnet(_) => NetworkKind::Testnet,
         }
     }
 
-    /// Returns the default [`Network`] for a [`NetworkKind`].
-    pub fn from_kind(kind: NetworkKind) -> Self {
-        match kind {
-            NetworkKind::Mainnet => Self::Mainnet,
-            NetworkKind::Testnet => Self::new_default_testnet(),
-        }
-    }
-
     /// Returns an iterator over [`Network`] variants.
     pub fn iter() -> impl Iterator<Item = Self> {
         // TODO: Use default values of `Testnet` variant when adding fields for #7845.

zebra-chain/src/primitives/address.rs

@@ -68,16 +68,9 @@ impl TryFrom<&Network> for zcash_address::Network {
             // TODO: If the network parameters match `Regtest` parameters, convert to
             //       `zcash_address::Network::Regtest instead of returning `UnsupportedAddress` error.
             //       (#7119, #7839)
-            Network::Testnet(_params) => Err(UnsupportedNetwork),
+            Network::Testnet(_params) => Err(UnsupportedNetwork(
-        }
+                "could not convert configured testnet to zcash_address::Network".to_string(),
-    }
+            )),
-}
-
-impl From<NetworkKind> for zcash_address::Network {
-    fn from(network: NetworkKind) -> Self {
-        match network {
-            NetworkKind::Mainnet => zcash_address::Network::Main,
-            NetworkKind::Testnet => zcash_address::Network::Test,
         }
     }
 }
@@ -205,10 +198,34 @@ impl Address {
             Self::Transparent(address) => Some(address.to_string()),
             Self::Sapling { address, network } => {
                 let data = address.to_bytes();
-                let address = ZcashAddress::from_sapling((*network).into(), data);
+                let address = ZcashAddress::from_sapling(network.to_zcash_address(), data);
                 Some(address.encode())
             }
             Self::Unified { .. } => None,
         }
     }
 }
+
+impl NetworkKind {
+    /// Converts a [`zcash_address::Network`] to a [`NetworkKind`].
+    ///
+    /// This method is meant to be used for decoding Zcash addresses in zebra-rpc methods.
+    fn from_zcash_address(network: zcash_address::Network) -> Self {
+        match network {
+            zcash_address::Network::Main => NetworkKind::Mainnet,
+            zcash_address::Network::Test => NetworkKind::Testnet,
+            zcash_address::Network::Regtest => NetworkKind::Regtest,
+        }
+    }
+
+    /// Converts a [`zcash_address::Network`] to a [`NetworkKind`].
+    ///
+    /// This method is meant to be used for encoding Zcash addresses in zebra-rpc methods.
+    fn to_zcash_address(self) -> zcash_address::Network {
+        match self {
+            NetworkKind::Mainnet => zcash_address::Network::Main,
+            NetworkKind::Testnet => zcash_address::Network::Test,
+            NetworkKind::Regtest => zcash_address::Network::Regtest,
+        }
+    }
+}

zebra-chain/src/primitives/zcash_primitives.rs

@@ -351,7 +351,10 @@ impl TryFrom<&Network> for zcash_primitives::consensus::Network {
             Network::Testnet(_params) if network.is_default_testnet() => {
                 Ok(zcash_primitives::consensus::Network::TestNetwork)
             }
-            Network::Testnet(_params) => Err(UnsupportedNetwork),
+            Network::Testnet(_params) => Err(UnsupportedNetwork(
+                "could not convert configured testnet to zcash_primitives::consensus::Network"
+                    .to_string(),
+            )),
         }
     }
 }
@@ -360,7 +363,9 @@ impl From<NetworkKind> for zcash_primitives::consensus::Network {
     fn from(network: NetworkKind) -> Self {
         match network {
             NetworkKind::Mainnet => zcash_primitives::consensus::Network::MainNetwork,
-            NetworkKind::Testnet => zcash_primitives::consensus::Network::TestNetwork,
+            NetworkKind::Testnet | NetworkKind::Regtest => {
+                zcash_primitives::consensus::Network::TestNetwork
+            }
         }
     }
 }

zebra-chain/src/transparent/address.rs

@@ -29,10 +29,6 @@ use proptest::prelude::*;
 #[derive(
     Clone, Eq, PartialEq, Hash, serde_with::SerializeDisplay, serde_with::DeserializeFromStr,
 )]
-#[cfg_attr(
-    any(test, feature = "proptest-impl"),
-    derive(proptest_derive::Arbitrary)
-)]
 pub enum Address {
     /// P2SH (Pay to Script Hash) addresses
     PayToScriptHash {

zebra-chain/src/transparent/arbitrary.rs

@@ -1,8 +1,8 @@
 use proptest::{arbitrary::any, collection::vec, prelude::*};
 
-use crate::{block, LedgerState};
+use crate::{block, parameters::NetworkKind, LedgerState};
 
-use super::{CoinbaseData, Input, OutPoint, Script, GENESIS_COINBASE_DATA};
+use super::{Address, CoinbaseData, Input, OutPoint, Script, GENESIS_COINBASE_DATA};
 
 impl Input {
     /// Construct a strategy for creating valid-ish vecs of Inputs.
@@ -46,3 +46,27 @@ impl Arbitrary for Input {
 
     type Strategy = BoxedStrategy<Self>;
 }
+
+impl Arbitrary for Address {
+    type Parameters = ();
+
+    fn arbitrary_with(_args: ()) -> Self::Strategy {
+        any::<(bool, bool, [u8; 20])>()
+            .prop_map(|(is_mainnet, is_p2pkh, hash_bytes)| {
+                let network = if is_mainnet {
+                    NetworkKind::Mainnet
+                } else {
+                    NetworkKind::Testnet
+                };
+
+                if is_p2pkh {
+                    Address::from_pub_key_hash(network, hash_bytes)
+                } else {
+                    Address::from_script_hash(network, hash_bytes)
+                }
+            })
+            .boxed()
+    }
+
+    type Strategy = BoxedStrategy<Self>;
+}

zebra-network/src/config.rs

@@ -682,7 +682,12 @@ impl<'de> Deserialize<'de> for Config {
 
             Network::new_configured_testnet(network_params)
         } else {
-            Network::from_kind(network_kind)
+            // Convert to default `Network` for a `NetworkKind` if there are no testnet parameters.
+            match network_kind {
+                NetworkKind::Mainnet => Network::Mainnet,
+                NetworkKind::Testnet => Network::new_default_testnet(),
+                NetworkKind::Regtest => unimplemented!("Regtest is not yet implemented in Zebra"),
+            }
         };
 
         let listen_addr = match listen_addr.parse::<SocketAddr>() {

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

@@ -498,14 +498,17 @@ impl AddressTransaction {
 
 /// Returns a byte representing the [`transparent::Address`] variant.
 fn address_variant(address: &transparent::Address) -> u8 {
+    use NetworkKind::*;
     // Return smaller values for more common variants.
     //
     // (This probably doesn't matter, but it might help slightly with data compression.)
     match (address.network(), address) {
-        (NetworkKind::Mainnet, PayToPublicKeyHash { .. }) => 0,
+        (Mainnet, PayToPublicKeyHash { .. }) => 0,
-        (NetworkKind::Mainnet, PayToScriptHash { .. }) => 1,
+        (Mainnet, PayToScriptHash { .. }) => 1,
-        (NetworkKind::Testnet, PayToPublicKeyHash { .. }) => 2,
+        // There's no way to distinguish between Regtest and Testnet for encoded transparent addresses,
-        (NetworkKind::Testnet, PayToScriptHash { .. }) => 3,
+        // so the network kind should always be `Mainnet` or `Testnet`.
+        (Testnet | Regtest, PayToPublicKeyHash { .. }) => 2,
+        (Testnet | Regtest, PayToScriptHash { .. }) => 3,
     }
 }
 
@@ -531,7 +534,6 @@ impl FromDisk for transparent::Address {
         let network = if address_variant < 2 {
             NetworkKind::Mainnet
         } else {
-            // TODO: Replace `network` field on `Address` with the prefix and a method for checking if it's the mainnet prefix.
             NetworkKind::Testnet
         };