From b8f1e0a1a41ea0a9556a3a47791a2f1824206609 Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Fri, 8 Dec 2023 05:17:55 -0600 Subject: [PATCH] chore: refactor current network handling and add test (#6004) Description --- Refactors the handling of network-based hashing operations introduced in #5980 to better handle read operations. Adds a sanity test for hash independence. Supersedes #6014. Closes #6003. Motivation and Context --- Recent work in #5980 binds the current network into consensus hashing. It uses a `Mutex`, which has two subtle issues. First, it allows the network to be set multiple times, which should not occur. Second, it locks on reads, which is unnecessary and inefficient. This PR updates how the current network is handled. It adds `Network::get_current_or_default` that will return either the current network (if it has been set) or the default network (if it has not). It adds `Network:set_current` that attempts to set the network; if it has been set before, this operation will fail. Note that calling `Network::get_current_or_default` does _not_ set the network for this purpose. It modifies the API for consensus hashing to add a wrapper that uses the current network. This wraps functionality allowing for specification of a network, which is useful for testing. On top of this new API, a new test is added that checks for distinct hashes using the same input but different networks. This is not comprehensive, but will detect obvious issues. How Has This Been Tested? --- Existing tests pass. A new test passes. What process can a PR reviewer use to test or verify this change? --- Check that the tests do what they claim. Check that the updates to consensus hashing properly introduce the expected wrapping functionality. Check that the updated network API does what it is supposed to. --- Cargo.lock | 1 - .../src/network_check.rs | 20 +++++------ .../consensus/consensus_encoding/hashing.rs | 35 ++++++++++++++++--- common/Cargo.toml | 1 - common/src/configuration/mod.rs | 2 +- common/src/configuration/network.rs | 16 +++++++-- 6 files changed, 55 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7048a0183c..903098a434 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5615,7 +5615,6 @@ dependencies = [ "log", "log4rs", "multiaddr", - "once_cell", "path-clean", "prost-build", "serde", diff --git a/applications/minotari_app_utilities/src/network_check.rs b/applications/minotari_app_utilities/src/network_check.rs index 65c8e82104..d957a116d6 100644 --- a/applications/minotari_app_utilities/src/network_check.rs +++ b/applications/minotari_app_utilities/src/network_check.rs @@ -20,10 +20,8 @@ // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -use std::sync::{MutexGuard, PoisonError}; - use tari_common::{ - configuration::{Network, CURRENT_NETWORK}, + configuration::Network, exit_codes::{ExitCode, ExitError}, }; use tari_features::resolver::Target; @@ -37,8 +35,11 @@ pub enum NetworkCheckError { NextNetBinary(Network), #[error("The network {0} is invalid for this binary built for TestNet")] TestNetBinary(Network), - #[error("Had a problem with the CURRENT_NETWORK guard: {0}")] - CurrentNetworkGuard(#[from] PoisonError>), + #[error("Could not set the network, tried to set to {attempted} but the current network is {current_network}")] + CouldNotSetNetwork { + attempted: Network, + current_network: Network, + }, } impl From for ExitError { @@ -71,11 +72,10 @@ pub fn is_network_choice_valid(network: Network) -> Result Result<(), NetworkCheckError> { match is_network_choice_valid(network) { - Ok(network) => { - let mut current_network = CURRENT_NETWORK.lock()?; - *current_network = network; - Ok(()) - }, + Ok(network) => Network::set_current(network).map_err(|instead_network| NetworkCheckError::CouldNotSetNetwork { + attempted: network, + current_network: instead_network, + }), Err(e) => Err(e), } } diff --git a/base_layer/core/src/consensus/consensus_encoding/hashing.rs b/base_layer/core/src/consensus/consensus_encoding/hashing.rs index 9889ac9665..6418f280b3 100644 --- a/base_layer/core/src/consensus/consensus_encoding/hashing.rs +++ b/base_layer/core/src/consensus/consensus_encoding/hashing.rs @@ -28,7 +28,7 @@ use digest::{ consts::{U32, U64}, Digest, }; -use tari_common::configuration::CURRENT_NETWORK; +use tari_common::configuration::Network; use tari_crypto::{hash_domain, hashing::DomainSeparation}; /// Domain separated consensus encoding hasher. @@ -42,7 +42,10 @@ where D: Default { #[allow(clippy::new_ret_no_self)] pub fn new(label: &'static str) -> ConsensusHasher { - let network = *CURRENT_NETWORK.lock().unwrap(); + Self::new_with_network(label, Network::get_current_or_default()) + } + + pub fn new_with_network(label: &'static str, network: Network) -> ConsensusHasher { let mut digest = D::default(); M::add_domain_separation_tag(&mut digest, &format!("{}.n{}", label, network.as_byte())); ConsensusHasher::from_digest(digest) @@ -140,6 +143,7 @@ impl Write for WriteHashWrapper { mod tests { use blake2::Blake2b; use digest::consts::U32; + use tari_common::configuration::Network; use tari_crypto::hash_domain; use tari_script::script; @@ -147,9 +151,31 @@ mod tests { hash_domain!(TestHashDomain, "com.tari.test.test_hash", 0); + #[test] + fn network_yields_distinct_hash() { + let label = "test"; + let input = [1u8; 32]; + + // Generate a mainnet hash + let hash_mainnet = + DomainSeparatedConsensusHasher::>::new_with_network(label, Network::MainNet) + .chain(&input) + .finalize(); + + // Generate a stagenet hash + let hash_stagenet = + DomainSeparatedConsensusHasher::>::new_with_network(label, Network::StageNet) + .chain(&input) + .finalize(); + + // They should be distinct + assert_ne!(hash_mainnet, hash_stagenet); + } + #[test] fn it_hashes_using_the_domain_hasher() { - let network = *CURRENT_NETWORK.lock().unwrap(); + let network = Network::get_current_or_default(); + // Script is chosen because the consensus encoding impl for TariScript has 2 writes let mut hasher = Blake2b::::default(); TestHashDomain::add_domain_separation_tag(&mut hasher, &format!("{}.n{}", "foo", network.as_byte())); @@ -164,7 +190,8 @@ mod tests { #[test] fn it_adds_to_hash_challenge_in_complete_chunks() { - let network = *CURRENT_NETWORK.lock().unwrap(); + let network = Network::get_current_or_default(); + // Script is chosen because the consensus encoding impl for TariScript has 2 writes let test_subject = script!(Nop); let mut hasher = Blake2b::::default(); diff --git a/common/Cargo.toml b/common/Cargo.toml index 9c7592b832..1b2fd86865 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -34,7 +34,6 @@ structopt = { version = "0.3.13", default_features = false } tempfile = "3.1.0" thiserror = "1.0.29" toml = { version = "0.5", optional = true } -once_cell = "1.18.0" [dev-dependencies] tari_test_utils = { path = "../infrastructure/test_utils"} diff --git a/common/src/configuration/mod.rs b/common/src/configuration/mod.rs index 7e754accf4..44f30f90c8 100644 --- a/common/src/configuration/mod.rs +++ b/common/src/configuration/mod.rs @@ -41,7 +41,7 @@ pub mod bootstrap; pub mod error; pub mod loader; mod network; -pub use network::{Network, CURRENT_NETWORK}; +pub use network::Network; mod common_config; mod multiaddr_list; pub mod name_server; diff --git a/common/src/configuration/network.rs b/common/src/configuration/network.rs index faeaee5215..a78aa7d4a1 100644 --- a/common/src/configuration/network.rs +++ b/common/src/configuration/network.rs @@ -25,15 +25,14 @@ use std::{ fmt, fmt::{Display, Formatter}, str::FromStr, - sync::Mutex, + sync::OnceLock, }; -use once_cell::sync::Lazy; use serde::{Deserialize, Serialize}; use crate::ConfigurationError; -pub static CURRENT_NETWORK: Lazy> = Lazy::new(|| Mutex::new(Network::default())); +static CURRENT_NETWORK: OnceLock = OnceLock::new(); /// Represents the available Tari p2p networks. Only nodes with matching byte values will be able to connect, so these /// should never be changed once released. @@ -50,6 +49,17 @@ pub enum Network { } impl Network { + pub fn get_current_or_default() -> Self { + match CURRENT_NETWORK.get() { + Some(&network) => network, + None => Network::default(), + } + } + + pub fn set_current(network: Network) -> Result<(), Network> { + CURRENT_NETWORK.set(network) + } + pub fn as_byte(self) -> u8 { self as u8 }