Skip to content

Commit

Permalink
chore: refactor current network handling and add test (#6004)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
AaronFeickert authored Dec 8, 2023
1 parent 3f76739 commit b8f1e0a
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 20 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 10 additions & 10 deletions applications/minotari_app_utilities/src/network_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<MutexGuard<'static, Network>>),
#[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<NetworkCheckError> for ExitError {
Expand Down Expand Up @@ -71,11 +72,10 @@ pub fn is_network_choice_valid(network: Network) -> Result<Network, NetworkCheck

pub fn set_network_if_choice_valid(network: Network) -> 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),
}
}
35 changes: 31 additions & 4 deletions base_layer/core/src/consensus/consensus_encoding/hashing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -42,7 +42,10 @@ where D: Default
{
#[allow(clippy::new_ret_no_self)]
pub fn new(label: &'static str) -> ConsensusHasher<D> {
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<D> {
let mut digest = D::default();
M::add_domain_separation_tag(&mut digest, &format!("{}.n{}", label, network.as_byte()));
ConsensusHasher::from_digest(digest)
Expand Down Expand Up @@ -140,16 +143,39 @@ impl<D: Digest> Write for WriteHashWrapper<D> {
mod tests {
use blake2::Blake2b;
use digest::consts::U32;
use tari_common::configuration::Network;
use tari_crypto::hash_domain;
use tari_script::script;

use super::*;

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::<TestHashDomain, Blake2b<U32>>::new_with_network(label, Network::MainNet)
.chain(&input)
.finalize();

// Generate a stagenet hash
let hash_stagenet =
DomainSeparatedConsensusHasher::<TestHashDomain, Blake2b<U32>>::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::<U32>::default();
TestHashDomain::add_domain_separation_tag(&mut hasher, &format!("{}.n{}", "foo", network.as_byte()));
Expand All @@ -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::<U32>::default();
Expand Down
1 change: 0 additions & 1 deletion common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down
2 changes: 1 addition & 1 deletion common/src/configuration/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 13 additions & 3 deletions common/src/configuration/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Mutex<Network>> = Lazy::new(|| Mutex::new(Network::default()));
static CURRENT_NETWORK: OnceLock<Network> = 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.
Expand All @@ -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
}
Expand Down

0 comments on commit b8f1e0a

Please sign in to comment.