From 736c87710365f0c825c91175d1a646d0592a4c67 Mon Sep 17 00:00:00 2001 From: Simon Paitrault Date: Mon, 18 Mar 2024 08:43:49 +0100 Subject: [PATCH] fix: fixing review comments Signed-off-by: Simon Paitrault --- crates/topos-p2p/src/behaviour/gossip.rs | 4 ++-- crates/topos-p2p/src/runtime/handle_event.rs | 1 + crates/topos-tce-api/src/graphql/builder.rs | 2 +- crates/topos-tce-broadcast/src/tests/task.rs | 8 +++----- .../topos-tce-broadcast/src/tests/task_manager.rs | 9 +++------ crates/topos-tce-storage/src/fullnode/mod.rs | 8 ++++---- .../src/tests/pending_certificates.rs | 8 ++++---- crates/topos-tce-storage/src/validator/mod.rs | 14 +++++++------- crates/topos-tce/src/lib.rs | 2 +- crates/topos-test-sdk/src/crypto.rs | 9 +++++++++ crates/topos-test-sdk/src/lib.rs | 1 + crates/topos-test-sdk/src/tce/mod.rs | 10 +++------- 12 files changed, 39 insertions(+), 37 deletions(-) create mode 100644 crates/topos-test-sdk/src/crypto.rs diff --git a/crates/topos-p2p/src/behaviour/gossip.rs b/crates/topos-p2p/src/behaviour/gossip.rs index 35716fe4a..eaf6991ef 100644 --- a/crates/topos-p2p/src/behaviour/gossip.rs +++ b/crates/topos-p2p/src/behaviour/gossip.rs @@ -255,7 +255,7 @@ impl NetworkBehaviour for Behaviour { _ => {} }, gossipsub::Event::Subscribed { peer_id, topic } => { - debug!("Subscribed to {:?} with {peer_id}", topic); + debug!("{peer_id} subscribed to {:?}", topic); // If the behaviour isn't already healthy we check if this event // triggers a switch to healthy @@ -268,7 +268,7 @@ impl NetworkBehaviour for Behaviour { } } gossipsub::Event::Unsubscribed { peer_id, topic } => { - debug!("Unsubscribed from {:?} with {peer_id}", topic); + debug!("{peer_id} unsubscribed from {:?}", topic); } gossipsub::Event::GossipsubNotSupported { peer_id } => { debug!("Gossipsub not supported by {:?}", peer_id); diff --git a/crates/topos-p2p/src/runtime/handle_event.rs b/crates/topos-p2p/src/runtime/handle_event.rs index 5ccd60523..0f5dff1f8 100644 --- a/crates/topos-p2p/src/runtime/handle_event.rs +++ b/crates/topos-p2p/src/runtime/handle_event.rs @@ -193,6 +193,7 @@ impl EventHandler> for Runtime { let behaviour = self.swarm.behaviour(); if let Some(event) = self.healthy_status_changed() { + debug!("Healthy status changed: {:?}", event); _ = self.event_sender.send(event).await; } diff --git a/crates/topos-tce-api/src/graphql/builder.rs b/crates/topos-tce-api/src/graphql/builder.rs index 061dfb779..b31eeb058 100644 --- a/crates/topos-tce-api/src/graphql/builder.rs +++ b/crates/topos-tce-api/src/graphql/builder.rs @@ -62,7 +62,7 @@ impl ServerBuilder { .take() .expect("Cannot build GraphQL server without a FullNode store"); - let fullnode_store = store.get_fullnode_store(); + let fullnode_store = store.fullnode_store(); let runtime = self .runtime .take() diff --git a/crates/topos-tce-broadcast/src/tests/task.rs b/crates/topos-tce-broadcast/src/tests/task.rs index c4f13f5cc..eca0102a6 100644 --- a/crates/topos-tce-broadcast/src/tests/task.rs +++ b/crates/topos-tce-broadcast/src/tests/task.rs @@ -1,4 +1,4 @@ -use std::{future::IntoFuture, str::FromStr, sync::Arc, time::Duration}; +use std::{future::IntoFuture, sync::Arc, time::Duration}; use rstest::rstest; use tokio::{ @@ -11,6 +11,7 @@ use topos_tce_storage::validator::ValidatorStore; use topos_test_sdk::{ certificates::create_certificate_chain, constants::{SOURCE_SUBNET_ID_1, TARGET_SUBNET_ID_1}, + crypto::message_signer, storage::create_validator_store, }; @@ -26,6 +27,7 @@ async fn start_with_ungossiped_cert( #[future(awt)] #[from(create_validator_store)] validatore_store: Arc, + message_signer: Arc, ) { let certificate = create_certificate_chain(SOURCE_SUBNET_ID_1, &[TARGET_SUBNET_ID_1], 1) .pop() @@ -40,10 +42,6 @@ async fn start_with_ungossiped_cert( }; let (event_sender, mut event_receiver) = mpsc::channel(2); let (broadcast_sender, _) = broadcast::channel(1); - let message_signer = Arc::new( - MessageSigner::from_str("122f3ae6ade1fd136b292cea4f6243c7811160352c8821528547a1fe7c459daf") - .unwrap(), - ); let need_gossip = true; let subscriptions = SubscriptionsView::default(); diff --git a/crates/topos-tce-broadcast/src/tests/task_manager.rs b/crates/topos-tce-broadcast/src/tests/task_manager.rs index 6a4315636..672da59f7 100644 --- a/crates/topos-tce-broadcast/src/tests/task_manager.rs +++ b/crates/topos-tce-broadcast/src/tests/task_manager.rs @@ -1,4 +1,4 @@ -use std::{str::FromStr, sync::Arc}; +use std::sync::Arc; use rstest::rstest; use tokio::{ @@ -12,6 +12,7 @@ use topos_tce_storage::validator::ValidatorStore; use topos_test_sdk::{ certificates::create_certificate_chain, constants::{SOURCE_SUBNET_ID_1, TARGET_SUBNET_ID_1}, + crypto::message_signer, storage::create_validator_store, }; @@ -23,6 +24,7 @@ async fn can_start( #[future(awt)] #[from(create_validator_store)] validator_store: Arc, + message_signer: Arc, ) { let (message_sender, message_receiver) = mpsc::channel(1); let (event_sender, _) = mpsc::channel(1); @@ -35,11 +37,6 @@ async fn can_start( delivery_threshold: 1, }; - let message_signer = Arc::new( - MessageSigner::from_str("122f3ae6ade1fd136b292cea4f6243c7811160352c8821528547a1fe7c459daf") - .unwrap(), - ); - let manager = TaskManager::new( message_receiver, SubscriptionsView::default(), diff --git a/crates/topos-tce-storage/src/fullnode/mod.rs b/crates/topos-tce-storage/src/fullnode/mod.rs index b50900d4e..dccd58223 100644 --- a/crates/topos-tce-storage/src/fullnode/mod.rs +++ b/crates/topos-tce-storage/src/fullnode/mod.rs @@ -85,7 +85,7 @@ impl FullNodeStore { } /// Await for a [`LockGuards`] for the given certificate id - pub(crate) async fn get_certificate_lock_guard( + pub(crate) async fn certificate_lock_guard( &self, certificate_id: CertificateId, ) -> OwnedMutexGuard<()> { @@ -97,7 +97,7 @@ impl FullNodeStore { } /// Await for a [`LockGuards`] for the given subnet id - pub(crate) async fn get_subnet_lock_guard(&self, subnet_id: SubnetId) -> OwnedMutexGuard<()> { + pub(crate) async fn subnet_lock_guard(&self, subnet_id: SubnetId) -> OwnedMutexGuard<()> { self.subnet_lock_guards .get_lock(subnet_id) .await @@ -114,11 +114,11 @@ impl WriteStore for FullNodeStore { ) -> Result { // Lock resources for concurrency issues let _cert_guard = self - .get_certificate_lock_guard(certificate.certificate.id) + .certificate_lock_guard(certificate.certificate.id) .await; let _subnet_guard = self - .get_subnet_lock_guard(certificate.certificate.source_subnet_id) + .subnet_lock_guard(certificate.certificate.source_subnet_id) .await; let subnet_id = certificate.certificate.source_subnet_id; diff --git a/crates/topos-tce-storage/src/tests/pending_certificates.rs b/crates/topos-tce-storage/src/tests/pending_certificates.rs index 1d0a1fbb4..714ef59b7 100644 --- a/crates/topos-tce-storage/src/tests/pending_certificates.rs +++ b/crates/topos-tce-storage/src/tests/pending_certificates.rs @@ -91,7 +91,7 @@ async fn adding_pending_certificate_already_delivered(store: Arc .is_err()); } -/// This test is covering a corner case which involve the delivery of a prev certificate +/// This test is covering a corner case which involves the delivery of a prev certificate /// and a child certificate. /// /// The scenario is this one: @@ -123,7 +123,7 @@ mod concurrency { // The lock guard simulate the start of the certificate insertion in the table. let lock_guard_certificate = store .fullnode_store - .get_certificate_lock_guard(parent.certificate.id) + .certificate_lock_guard(parent.certificate.id) .await; tokio::spawn(async move { @@ -149,7 +149,7 @@ mod concurrency { // The lock guard simulate the start of the certificate insertion in the table. let lock_guard_subnet = store .fullnode_store - .get_subnet_lock_guard(cert.certificate.source_subnet_id) + .subnet_lock_guard(cert.certificate.source_subnet_id) .await; tokio::spawn(async move { @@ -188,7 +188,7 @@ mod concurrency { // The lock guard simulate the start of the certificate insertion in the table. let lock_guard_subnet = store .fullnode_store - .get_subnet_lock_guard(cert.certificate.source_subnet_id) + .subnet_lock_guard(cert.certificate.source_subnet_id) .await; tokio::spawn(async move { diff --git a/crates/topos-tce-storage/src/validator/mod.rs b/crates/topos-tce-storage/src/validator/mod.rs index 55768ff27..dd6f994b1 100644 --- a/crates/topos-tce-storage/src/validator/mod.rs +++ b/crates/topos-tce-storage/src/validator/mod.rs @@ -119,7 +119,7 @@ impl ValidatorStore { } /// Returns the [`FullNodeStore`] used by the [`ValidatorStore`] - pub fn get_fullnode_store(&self) -> Arc { + pub fn fullnode_store(&self) -> Arc { self.fullnode_store.clone() } @@ -290,12 +290,12 @@ impl ValidatorStore { &self, certificate: &Certificate, ) -> Result, StorageError> { - // A lock guard is asked during the insertion of a pending certificate - // to avoid race condition when a certificate is being inserted and added - // to the pending pool at the same time + // A lock guard is taken during the insertion of a pending certificate (C1) + // to avoid race condition when this certificate C1 is delivered by the network + // and in the process of being inserted into the precedence tables. let _certificate_guard = self .fullnode_store - .get_certificate_lock_guard(certificate.id) + .certificate_lock_guard(certificate.id) .await; if self.get_certificate(&certificate.id)?.is_some() { @@ -320,12 +320,12 @@ impl ValidatorStore { )); } - // A lock guard is asked during the insertion of a pending certificate + // A lock guard is taken during the insertion of a pending certificate // to avoid race condition when a certificate is being added to the // pending pool while its parent is currently being inserted as delivered let _prev_certificate_guard = self .fullnode_store - .get_certificate_lock_guard(certificate.prev_id) + .certificate_lock_guard(certificate.prev_id) .await; let prev_delivered = certificate.prev_id == INITIAL_CERTIFICATE_ID diff --git a/crates/topos-tce/src/lib.rs b/crates/topos-tce/src/lib.rs index 89ae8e4f1..42f654dc7 100644 --- a/crates/topos-tce/src/lib.rs +++ b/crates/topos-tce/src/lib.rs @@ -107,7 +107,7 @@ pub async fn run( let validator_store = ValidatorStore::new(path) .map_err(|error| format!("Unable to create validator store: {error}"))?; - let fullnode_store = validator_store.get_fullnode_store(); + let fullnode_store = validator_store.fullnode_store(); let storage_client = StorageClient::new(validator_store.clone()); diff --git a/crates/topos-test-sdk/src/crypto.rs b/crates/topos-test-sdk/src/crypto.rs new file mode 100644 index 000000000..186c7146f --- /dev/null +++ b/crates/topos-test-sdk/src/crypto.rs @@ -0,0 +1,9 @@ +use std::{str::FromStr, sync::Arc}; + +use rstest::fixture; +use topos_crypto::messages::MessageSigner; + +#[fixture(key = "122f3ae6ade1fd136b292cea4f6243c7811160352c8821528547a1fe7c459daf")] +pub fn message_signer(key: &str) -> Arc { + Arc::new(MessageSigner::from_str(key).unwrap()) +} diff --git a/crates/topos-test-sdk/src/lib.rs b/crates/topos-test-sdk/src/lib.rs index 8c68c678d..b4e52d3ef 100644 --- a/crates/topos-test-sdk/src/lib.rs +++ b/crates/topos-test-sdk/src/lib.rs @@ -1,5 +1,6 @@ pub mod certificates; +pub mod crypto; pub mod networking; pub mod p2p; pub mod sequencer; diff --git a/crates/topos-test-sdk/src/tce/mod.rs b/crates/topos-test-sdk/src/tce/mod.rs index 6f2425f0d..432fd140a 100644 --- a/crates/topos-test-sdk/src/tce/mod.rs +++ b/crates/topos-test-sdk/src/tce/mod.rs @@ -44,6 +44,7 @@ use self::p2p::{bootstrap_network, create_network_worker}; use self::protocol::{create_reliable_broadcast_client, create_reliable_broadcast_params}; use self::public_api::create_public_api; use self::synchronizer::create_synchronizer; +use crate::crypto::message_signer; use crate::p2p::local_peer; use crate::storage::create_fullnode_store; use crate::storage::create_validator_store; @@ -174,10 +175,6 @@ impl NodeConfig { } } -fn default_message_signer() -> Arc { - Arc::new(MessageSigner::new(&[5u8; 32]).unwrap()) -} - #[derive(Clone)] struct DummyService {} @@ -207,9 +204,8 @@ pub fn create_dummy_router() -> Router { peers = &[], certificates = &[], validator_id = ValidatorId::default(), - validators = HashSet::default(), - message_signer = default_message_signer()) -] + validators = HashSet::default() +)] pub async fn start_node( certificates: &[CertificateDelivered], config: NodeConfig,