From be89d59be12af5a2615c7574c7d90036852fd950 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:46:57 +0100 Subject: [PATCH 01/15] refactor: improve `CardanoImmutableDigester` maintainability and modularity by isolating responsibilities into smaller scopes and functions Co-authored-by: DJO --- .../digesters/cardano_immutable_digester.rs | 175 ++++++++++-------- 1 file changed, 100 insertions(+), 75 deletions(-) diff --git a/mithril-common/src/digesters/cardano_immutable_digester.rs b/mithril-common/src/digesters/cardano_immutable_digester.rs index 2c33d2a33ad..4b927c98258 100644 --- a/mithril-common/src/digesters/cardano_immutable_digester.rs +++ b/mithril-common/src/digesters/cardano_immutable_digester.rs @@ -3,7 +3,7 @@ use crate::{ cache::ImmutableFileDigestCacheProvider, ImmutableDigester, ImmutableDigesterError, ImmutableFile, }, - entities::{CardanoDbBeacon, HexEncodedDigest, ImmutableFileName}, + entities::{CardanoDbBeacon, HexEncodedDigest, ImmutableFileName, ImmutableFileNumber}, logging::LoggerExtensions, }; use async_trait::async_trait; @@ -11,10 +11,14 @@ use sha2::{Digest, Sha256}; use slog::{debug, info, warn, Logger}; use std::{collections::BTreeMap, io, path::Path, sync::Arc}; -/// Result of a cache computation, contains the digest and the list of new entries to add +/// Result of a cache computation, contains the list of immutable digests and the list of new entries to add /// to the [ImmutableFileDigestCacheProvider]. -type CacheComputationResult = - Result<([u8; 32], Vec<(ImmutableFileName, HexEncodedDigest)>), io::Error>; +type ComputedImmutablesDigestsResult = Result; + +struct ComputedImmutablesDigests { + digests: Vec, + new_cached_entries: Vec<(ImmutableFileName, HexEncodedDigest)>, +} /// A digester working directly on a Cardano DB immutables files pub struct CardanoImmutableDigester { @@ -40,6 +44,36 @@ impl CardanoImmutableDigester { logger: logger.new_with_component_name::(), } } + + async fn process_immutables( + &self, + immutables: Vec, + ) -> Result { + let cached_values = match self.cache_provider.as_ref() { + None => BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))), + Some(cache_provider) => match cache_provider.get(immutables.clone()).await { + Ok(values) => values, + Err(error) => { + warn!( + self.logger, "Error while getting cached immutable files digests"; + "error" => ?error + ); + BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))) + } + }, + }; + + // The computation of immutable files digests is done in a separate thread because it is blocking the whole task + let logger = self.logger.clone(); + let computed_digests = + tokio::task::spawn_blocking(move || -> ComputedImmutablesDigestsResult { + compute_immutables_digests(logger, cached_values) + }) + .await + .map_err(|e| ImmutableDigesterError::DigestComputationError(e.into()))??; + + Ok(computed_digests) + } } #[async_trait] @@ -49,99 +83,87 @@ impl ImmutableDigester for CardanoImmutableDigester { dirpath: &Path, beacon: &CardanoDbBeacon, ) -> Result { - let up_to_file_number = beacon.immutable_file_number; - let immutables = ImmutableFile::list_completed_in_dir(dirpath)? - .into_iter() - .filter(|f| f.number <= up_to_file_number) - .collect::>(); - info!(self.logger, ">> compute_digest"; "beacon" => #?beacon, "nb_of_immutables" => immutables.len()); - - match immutables.last() { - None => Err(ImmutableDigesterError::NotEnoughImmutable { - expected_number: up_to_file_number, - found_number: None, - db_dir: dirpath.to_owned(), - }), - Some(last_immutable_file) if last_immutable_file.number < up_to_file_number => { - Err(ImmutableDigesterError::NotEnoughImmutable { - expected_number: up_to_file_number, - found_number: Some(last_immutable_file.number), - db_dir: dirpath.to_owned(), - }) + let immutables_to_process = + list_immutable_files_to_process(dirpath, beacon.immutable_file_number)?; + info!(self.logger, ">> compute_digest"; "beacon" => #?beacon, "nb_of_immutables" => immutables_to_process.len()); + let computed_immutables_digests = self.process_immutables(immutables_to_process).await?; + + let digest = { + let mut hasher = Sha256::new(); + hasher.update(compute_beacon_hash(&self.cardano_network, beacon).as_bytes()); + for digest in computed_immutables_digests.digests { + hasher.update(digest); } - Some(_) => { - let cached_values = match self.cache_provider.as_ref() { - None => BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))), - Some(cache_provider) => match cache_provider.get(immutables.clone()).await { - Ok(values) => values, - Err(error) => { - warn!( - self.logger, "Error while getting cached immutable files digests"; - "error" => ?error - ); - BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))) - } - }, - }; - - // digest is done in a separate thread because it is blocking the whole task - let logger = self.logger.clone(); - let thread_cardano_network = self.cardano_network.clone(); - let thread_beacon = beacon.clone(); - let (hash, new_cache_entries) = - tokio::task::spawn_blocking(move || -> CacheComputationResult { - compute_hash( - logger, - thread_cardano_network, - &thread_beacon, - cached_values, - ) - }) - .await - .map_err(|e| ImmutableDigesterError::DigestComputationError(e.into()))??; - let digest = hex::encode(hash); - - debug!(self.logger, "Computed digest: {digest:?}"); - - if let Some(cache_provider) = self.cache_provider.as_ref() { - if let Err(error) = cache_provider.store(new_cache_entries).await { - warn!( - self.logger, "Error while storing new immutable files digests to cache"; - "error" => ?error - ); - } - } + let hash: [u8; 32] = hasher.finalize().into(); + + hex::encode(hash) + }; - Ok(digest) + debug!(self.logger, "Computed digest: {digest:?}"); + + if let Some(cache_provider) = self.cache_provider.as_ref() { + if let Err(error) = cache_provider + .store(computed_immutables_digests.new_cached_entries) + .await + { + warn!( + self.logger, "Error while storing new immutable files digests to cache"; + "error" => ?error + ); } } + + Ok(digest) } } -fn compute_hash( +fn list_immutable_files_to_process( + dirpath: &Path, + up_to_file_number: ImmutableFileNumber, +) -> Result, ImmutableDigesterError> { + let immutables: Vec = ImmutableFile::list_completed_in_dir(dirpath)? + .into_iter() + .filter(|f| f.number <= up_to_file_number) + .collect(); + + match immutables.last() { + None => Err(ImmutableDigesterError::NotEnoughImmutable { + expected_number: up_to_file_number, + found_number: None, + db_dir: dirpath.to_owned(), + }), + Some(last_immutable_file) if last_immutable_file.number < up_to_file_number => { + Err(ImmutableDigesterError::NotEnoughImmutable { + expected_number: up_to_file_number, + found_number: Some(last_immutable_file.number), + db_dir: dirpath.to_owned(), + }) + } + Some(_) => Ok(immutables), + } +} + +fn compute_immutables_digests( logger: Logger, - cardano_network: String, - beacon: &CardanoDbBeacon, entries: BTreeMap>, -) -> CacheComputationResult { - let mut hasher = Sha256::new(); +) -> ComputedImmutablesDigestsResult { let mut new_cached_entries = Vec::new(); let mut progress = Progress { index: 0, total: entries.len(), }; - hasher.update(compute_beacon_hash(&cardano_network, beacon).as_bytes()); + let mut digests = Vec::with_capacity(entries.len()); for (ix, (entry, cache)) in entries.iter().enumerate() { match cache { None => { let data = hex::encode(entry.compute_raw_hash::()?); - hasher.update(&data); + digests.push(data.clone()); new_cached_entries.push((entry.filename.clone(), data)); } Some(digest) => { - hasher.update(digest); + digests.push(digest.to_string()); } }; @@ -150,7 +172,10 @@ fn compute_hash( } } - Ok((hasher.finalize().into(), new_cached_entries)) + Ok(ComputedImmutablesDigests { + digests, + new_cached_entries, + }) } fn compute_beacon_hash(network: &str, cardano_db_beacon: &CardanoDbBeacon) -> String { From 07fba554afd603e1397827f842b1e024861f4bf4 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Wed, 27 Nov 2024 16:04:58 +0100 Subject: [PATCH 02/15] refactor: adapt `cardano_immutable_digester` tests to test `list_immutable_files_to_process` function directly --- .../digesters/cardano_immutable_digester.rs | 41 +++++-------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/mithril-common/src/digesters/cardano_immutable_digester.rs b/mithril-common/src/digesters/cardano_immutable_digester.rs index 4b927c98258..8bf84539895 100644 --- a/mithril-common/src/digesters/cardano_immutable_digester.rs +++ b/mithril-common/src/digesters/cardano_immutable_digester.rs @@ -288,20 +288,15 @@ mod tests { #[tokio::test] async fn fail_if_no_file_in_folder() { let immutable_db = db_builder("fail_if_no_file_in_folder").build(); - let digester = - CardanoImmutableDigester::new("devnet".to_string(), None, TestLogger::stdout()); - let beacon = CardanoDbBeacon::new(1, 1); - let result = digester - .compute_digest(&immutable_db.dir, &beacon) - .await - .expect_err("compute_digest should have failed"); + let result = list_immutable_files_to_process(&immutable_db.dir, 1) + .expect_err("list_immutable_files_to_process should have failed"); assert_eq!( format!( "{:?}", ImmutableDigesterError::NotEnoughImmutable { - expected_number: beacon.immutable_file_number, + expected_number: 1, found_number: None, db_dir: immutable_db.dir, } @@ -315,14 +310,8 @@ mod tests { let immutable_db = db_builder("fail_if_no_immutable_exist") .with_non_immutables(&["not_immutable"]) .build(); - let digester = - CardanoImmutableDigester::new("devnet".to_string(), None, TestLogger::stdout()); - let beacon = CardanoDbBeacon::new(1, 1); - assert!(digester - .compute_digest(&immutable_db.dir, &beacon) - .await - .is_err()); + assert!(list_immutable_files_to_process(&immutable_db.dir, 1).is_err()); } #[tokio::test] @@ -330,20 +319,15 @@ mod tests { let immutable_db = db_builder("fail_if_theres_only_the_uncompleted_immutable_trio") .append_immutable_trio() .build(); - let digester = - CardanoImmutableDigester::new("devnet".to_string(), None, TestLogger::stdout()); - let beacon = CardanoDbBeacon::new(1, 1); - let result = digester - .compute_digest(&immutable_db.dir, &beacon) - .await - .expect_err("compute_digest should've failed"); + let result = list_immutable_files_to_process(&immutable_db.dir, 1) + .expect_err("list_immutable_files_to_process should've failed"); assert_eq!( format!( "{:?}", ImmutableDigesterError::NotEnoughImmutable { - expected_number: beacon.immutable_file_number, + expected_number: 1, found_number: None, db_dir: immutable_db.dir, } @@ -358,20 +342,15 @@ mod tests { .with_immutables(&[1, 2, 3, 4, 5]) .append_immutable_trio() .build(); - let digester = - CardanoImmutableDigester::new("devnet".to_string(), None, TestLogger::stdout()); - let beacon = CardanoDbBeacon::new(1, 10); - let result = digester - .compute_digest(&immutable_db.dir, &beacon) - .await - .expect_err("compute_digest should've failed"); + let result = list_immutable_files_to_process(&immutable_db.dir, 10) + .expect_err("list_immutable_files_to_process should've failed"); assert_eq!( format!( "{:?}", ImmutableDigesterError::NotEnoughImmutable { - expected_number: beacon.immutable_file_number, + expected_number: 10, found_number: Some(5), db_dir: immutable_db.dir, } From 0b08c6f6b8e93f4e16e4226932456b3e23f08055 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Wed, 27 Nov 2024 15:47:42 +0100 Subject: [PATCH 03/15] feat: implement `compute_merkle_tree` on `CardanoImmutableDigester` --- .../digesters/cardano_immutable_digester.rs | 173 +++++++++++++++++- .../src/digesters/dumb_immutable_observer.rs | 9 + .../src/digesters/immutable_digester.rs | 12 ++ ...cardano_immutable_full_signable_builder.rs | 9 + 4 files changed, 194 insertions(+), 9 deletions(-) diff --git a/mithril-common/src/digesters/cardano_immutable_digester.rs b/mithril-common/src/digesters/cardano_immutable_digester.rs index 8bf84539895..f66f0109ccc 100644 --- a/mithril-common/src/digesters/cardano_immutable_digester.rs +++ b/mithril-common/src/digesters/cardano_immutable_digester.rs @@ -1,4 +1,5 @@ use crate::{ + crypto_helper::{MKTree, MKTreeStoreInMemory}, digesters::{ cache::ImmutableFileDigestCacheProvider, ImmutableDigester, ImmutableDigesterError, ImmutableFile, @@ -16,7 +17,7 @@ use std::{collections::BTreeMap, io, path::Path, sync::Arc}; type ComputedImmutablesDigestsResult = Result; struct ComputedImmutablesDigests { - digests: Vec, + entries: BTreeMap, new_cached_entries: Vec<(ImmutableFileName, HexEncodedDigest)>, } @@ -91,7 +92,7 @@ impl ImmutableDigester for CardanoImmutableDigester { let digest = { let mut hasher = Sha256::new(); hasher.update(compute_beacon_hash(&self.cardano_network, beacon).as_bytes()); - for digest in computed_immutables_digests.digests { + for (_, digest) in computed_immutables_digests.entries { hasher.update(digest); } let hash: [u8; 32] = hasher.finalize().into(); @@ -115,6 +116,40 @@ impl ImmutableDigester for CardanoImmutableDigester { Ok(digest) } + + async fn compute_merkle_tree( + &self, + dirpath: &Path, + beacon: &CardanoDbBeacon, + ) -> Result, ImmutableDigesterError> { + let immutables_to_process = + list_immutable_files_to_process(dirpath, beacon.immutable_file_number)?; + info!(self.logger, ">> compute_merkle_tree"; "beacon" => #?beacon, "nb_of_immutables" => immutables_to_process.len()); + let computed_immutables_digests = self.process_immutables(immutables_to_process).await?; + + let digests: Vec = + computed_immutables_digests.entries.into_values().collect(); + let mktree = + MKTree::new(&digests).map_err(ImmutableDigesterError::MerkleTreeComputationError)?; + + debug!( + self.logger, + "Successfully computed Merkle tree for Cardano database"; "beacon" => #?beacon); + + if let Some(cache_provider) = self.cache_provider.as_ref() { + if let Err(error) = cache_provider + .store(computed_immutables_digests.new_cached_entries) + .await + { + warn!( + self.logger, "Error while storing new immutable files digests to cache"; + "error" => ?error + ); + } + } + + Ok(mktree) + } } fn list_immutable_files_to_process( @@ -153,17 +188,17 @@ fn compute_immutables_digests( total: entries.len(), }; - let mut digests = Vec::with_capacity(entries.len()); + let mut digests = BTreeMap::new(); for (ix, (entry, cache)) in entries.iter().enumerate() { match cache { None => { let data = hex::encode(entry.compute_raw_hash::()?); - digests.push(data.clone()); + digests.insert(entry.clone(), data.clone()); new_cached_entries.push((entry.filename.clone(), data)); } Some(digest) => { - digests.push(digest.to_string()); + digests.insert(entry.clone(), digest.to_string()); } }; @@ -173,7 +208,7 @@ fn compute_immutables_digests( } Ok(ComputedImmutablesDigests { - digests, + entries: digests, new_cached_entries, }) } @@ -359,6 +394,33 @@ mod tests { ); } + #[tokio::test] + async fn can_compute_merkle_tree_of_a_hundred_immutable_file_trio() { + let immutable_db = db_builder("can_compute_merkle_tree_of_a_hundred_immutable_file_trio") + .with_immutables(&(1..=100).collect::>()) + .append_immutable_trio() + .build(); + let logger = TestLogger::stdout(); + let digester = CardanoImmutableDigester::new( + "devnet".to_string(), + Some(Arc::new(MemoryImmutableFileDigestCacheProvider::default())), + logger.clone(), + ); + let beacon = CardanoDbBeacon::new(1, 100); + + let result = digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_merkle_tree must not fail"); + + let expected_merkle_root = result.compute_root().unwrap().to_hex(); + + assert_eq!( + "8552f75838176c967a33eb6da1fe5f3c9940b706d75a9c2352c0acd8439f3d84".to_string(), + expected_merkle_root + ) + } + #[tokio::test] async fn can_compute_hash_of_a_hundred_immutable_file_trio() { let immutable_db = db_builder("can_compute_hash_of_a_hundred_immutable_file_trio") @@ -385,8 +447,8 @@ mod tests { } #[tokio::test] - async fn digests_are_stored_into_cache_provider() { - let immutable_db = db_builder("digests_are_stored_into_cache_provider") + async fn compute_digest_store_digests_into_cache_provider() { + let immutable_db = db_builder("compute_digest_store_digests_into_cache_provider") .with_immutables(&[1, 2]) .append_immutable_trio() .build(); @@ -420,6 +482,42 @@ mod tests { assert_eq!(expected, cached_entries); } + #[tokio::test] + async fn compute_merkle_tree_store_digests_into_cache_provider() { + let immutable_db = db_builder("compute_merkle_tree_store_digests_into_cache_provider") + .with_immutables(&[1, 2]) + .append_immutable_trio() + .build(); + let immutables = immutable_db.immutables_files; + let cache = Arc::new(MemoryImmutableFileDigestCacheProvider::default()); + let logger = TestLogger::stdout(); + let digester = CardanoImmutableDigester::new( + "devnet".to_string(), + Some(cache.clone()), + logger.clone(), + ); + let beacon = CardanoDbBeacon::new(1, 2); + + digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_digest must not fail"); + + let cached_entries = cache + .get(immutables.clone()) + .await + .expect("Cache read should not fail"); + let expected: BTreeMap<_, _> = immutables + .into_iter() + .map(|i| { + let digest = hex::encode(i.compute_raw_hash::().unwrap()); + (i, Some(digest)) + }) + .collect(); + + assert_eq!(expected, cached_entries); + } + #[tokio::test] async fn computed_digest_with_cold_or_hot_or_without_any_cache_are_equals() { let immutable_db = DummyImmutablesDbBuilder::new( @@ -464,6 +562,53 @@ mod tests { ); } + #[tokio::test] + async fn computed_merkle_tree_with_cold_or_hot_or_without_any_cache_are_equals() { + let immutable_db = DummyImmutablesDbBuilder::new( + "computed_merkle_tree_with_cold_or_hot_or_without_any_cache_are_equals", + ) + .with_immutables(&[1, 2, 3]) + .append_immutable_trio() + .build(); + let logger = TestLogger::stdout(); + let no_cache_digester = + CardanoImmutableDigester::new("devnet".to_string(), None, logger.clone()); + let cache_digester = CardanoImmutableDigester::new( + "devnet".to_string(), + Some(Arc::new(MemoryImmutableFileDigestCacheProvider::default())), + logger.clone(), + ); + let beacon = CardanoDbBeacon::new(1, 3); + + let without_cache_digest = no_cache_digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_merkle_tree must not fail"); + + let cold_cache_digest = cache_digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_merkle_tree must not fail"); + + let full_cache_digest = cache_digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_merkle_tree must not fail"); + + let without_cache_merkle_root = without_cache_digest.compute_root().unwrap(); + let cold_cache_merkle_root = cold_cache_digest.compute_root().unwrap(); + let full_cache_merkle_root = full_cache_digest.compute_root().unwrap(); + assert_eq!( + without_cache_merkle_root, full_cache_merkle_root, + "Merkle roots with or without cache should be the same" + ); + + assert_eq!( + cold_cache_merkle_root, full_cache_merkle_root, + "Merkle roots with cold or with hot cache should be the same" + ); + } + #[tokio::test] async fn hash_computation_is_quicker_with_a_full_cache() { let immutable_db = db_builder("hash_computation_is_quicker_with_a_full_cache") @@ -506,7 +651,7 @@ mod tests { } #[tokio::test] - async fn cache_read_failure_dont_block_computation() { + async fn cache_read_failure_dont_block_computations() { let immutable_db = db_builder("cache_read_failure_dont_block_computation") .with_immutables(&[1, 2, 3]) .append_immutable_trio() @@ -530,6 +675,11 @@ mod tests { .compute_digest(&immutable_db.dir, &beacon) .await .expect("compute_digest must not fail even with cache write failure"); + + digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_merkle_tree must not fail even with cache write failure"); } #[tokio::test] @@ -557,5 +707,10 @@ mod tests { .compute_digest(&immutable_db.dir, &beacon) .await .expect("compute_digest must not fail even with cache read failure"); + + digester + .compute_merkle_tree(&immutable_db.dir, &beacon) + .await + .expect("compute_merkle_tree must not fail even with cache read failure"); } } diff --git a/mithril-common/src/digesters/dumb_immutable_observer.rs b/mithril-common/src/digesters/dumb_immutable_observer.rs index b500b6b4957..d3d8e4dd545 100644 --- a/mithril-common/src/digesters/dumb_immutable_observer.rs +++ b/mithril-common/src/digesters/dumb_immutable_observer.rs @@ -1,6 +1,7 @@ use std::path::Path; use crate::{ + crypto_helper::{MKTree, MKTreeStoreInMemory}, digesters::{ImmutableDigester, ImmutableDigesterError}, entities::CardanoDbBeacon, }; @@ -51,4 +52,12 @@ impl ImmutableDigester for DumbImmutableDigester { }) } } + + async fn compute_merkle_tree( + &self, + _dirpath: &Path, + _beacon: &CardanoDbBeacon, + ) -> Result, ImmutableDigesterError> { + unimplemented!() + } } diff --git a/mithril-common/src/digesters/immutable_digester.rs b/mithril-common/src/digesters/immutable_digester.rs index 2e4cd7252ef..04ebc5925f9 100644 --- a/mithril-common/src/digesters/immutable_digester.rs +++ b/mithril-common/src/digesters/immutable_digester.rs @@ -1,4 +1,5 @@ use crate::{ + crypto_helper::{MKTree, MKTreeStoreInMemory}, digesters::ImmutableFileListingError, entities::{CardanoDbBeacon, ImmutableFileNumber}, }; @@ -54,6 +55,13 @@ pub trait ImmutableDigester: Sync + Send { dirpath: &Path, beacon: &CardanoDbBeacon, ) -> Result; + + /// Compute the digests merkle tree + async fn compute_merkle_tree( + &self, + dirpath: &Path, + beacon: &CardanoDbBeacon, + ) -> Result, ImmutableDigesterError>; } /// [ImmutableDigester] related Errors. @@ -78,4 +86,8 @@ pub enum ImmutableDigesterError { /// Error raised when the digest computation failed. #[error("Digest computation failed")] DigestComputationError(#[from] io::Error), + + /// Error raised when the Merkle tree computation failed. + #[error("Merkle tree computation failed")] + MerkleTreeComputationError(anyhow::Error), } diff --git a/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs b/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs index d62efd07eb0..26f048c29f6 100644 --- a/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs +++ b/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs @@ -65,6 +65,7 @@ mod tests { use async_trait::async_trait; use std::path::Path; + use crate::crypto_helper::{MKTree, MKTreeStoreInMemory}; use crate::digesters::{ImmutableDigester, ImmutableDigesterError}; use crate::entities::CardanoDbBeacon; use crate::test_utils::TestLogger; @@ -83,6 +84,14 @@ mod tests { ) -> Result { Ok(format!("immutable {}", beacon.immutable_file_number)) } + + async fn compute_merkle_tree( + &self, + _dirpath: &Path, + _beacon: &CardanoDbBeacon, + ) -> Result, ImmutableDigesterError> { + unimplemented!() + } } #[tokio::test] From c0cd4c37d21ca841b9cb7a0b0df1379a760ab259 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Thu, 28 Nov 2024 19:05:49 +0100 Subject: [PATCH 04/15] refactor: enhance `ComputedImmutablesDigests` struct and isolate `update_cache` responsibility to reduce code duplication Co-authored-by: DJO --- .../digesters/cardano_immutable_digester.rs | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/mithril-common/src/digesters/cardano_immutable_digester.rs b/mithril-common/src/digesters/cardano_immutable_digester.rs index f66f0109ccc..425d02026d2 100644 --- a/mithril-common/src/digesters/cardano_immutable_digester.rs +++ b/mithril-common/src/digesters/cardano_immutable_digester.rs @@ -18,7 +18,7 @@ type ComputedImmutablesDigestsResult = Result, - new_cached_entries: Vec<(ImmutableFileName, HexEncodedDigest)>, + new_cached_entries: Vec, } /// A digester working directly on a Cardano DB immutables files @@ -75,6 +75,28 @@ impl CardanoImmutableDigester { Ok(computed_digests) } + + async fn update_cache(&self, computed_immutables_digests: &ComputedImmutablesDigests) { + if let Some(cache_provider) = self.cache_provider.as_ref() { + let new_cached_entries = computed_immutables_digests + .entries + .iter() + .filter(|(file, _hash)| { + computed_immutables_digests + .new_cached_entries + .contains(&file.filename) + }) + .map(|(file, hash)| (file.filename.clone(), hash.clone())) + .collect(); + + if let Err(error) = cache_provider.store(new_cached_entries).await { + warn!( + self.logger, "Error while storing new immutable files digests to cache"; + "error" => ?error + ); + } + } + } } #[async_trait] @@ -89,6 +111,8 @@ impl ImmutableDigester for CardanoImmutableDigester { info!(self.logger, ">> compute_digest"; "beacon" => #?beacon, "nb_of_immutables" => immutables_to_process.len()); let computed_immutables_digests = self.process_immutables(immutables_to_process).await?; + self.update_cache(&computed_immutables_digests).await; + let digest = { let mut hasher = Sha256::new(); hasher.update(compute_beacon_hash(&self.cardano_network, beacon).as_bytes()); @@ -102,18 +126,6 @@ impl ImmutableDigester for CardanoImmutableDigester { debug!(self.logger, "Computed digest: {digest:?}"); - if let Some(cache_provider) = self.cache_provider.as_ref() { - if let Err(error) = cache_provider - .store(computed_immutables_digests.new_cached_entries) - .await - { - warn!( - self.logger, "Error while storing new immutable files digests to cache"; - "error" => ?error - ); - } - } - Ok(digest) } @@ -127,6 +139,8 @@ impl ImmutableDigester for CardanoImmutableDigester { info!(self.logger, ">> compute_merkle_tree"; "beacon" => #?beacon, "nb_of_immutables" => immutables_to_process.len()); let computed_immutables_digests = self.process_immutables(immutables_to_process).await?; + self.update_cache(&computed_immutables_digests).await; + let digests: Vec = computed_immutables_digests.entries.into_values().collect(); let mktree = @@ -136,18 +150,6 @@ impl ImmutableDigester for CardanoImmutableDigester { self.logger, "Successfully computed Merkle tree for Cardano database"; "beacon" => #?beacon); - if let Some(cache_provider) = self.cache_provider.as_ref() { - if let Err(error) = cache_provider - .store(computed_immutables_digests.new_cached_entries) - .await - { - warn!( - self.logger, "Error while storing new immutable files digests to cache"; - "error" => ?error - ); - } - } - Ok(mktree) } } @@ -190,17 +192,15 @@ fn compute_immutables_digests( let mut digests = BTreeMap::new(); - for (ix, (entry, cache)) in entries.iter().enumerate() { - match cache { + for (ix, (entry, cache)) in entries.into_iter().enumerate() { + let hash = match cache { None => { - let data = hex::encode(entry.compute_raw_hash::()?); - digests.insert(entry.clone(), data.clone()); - new_cached_entries.push((entry.filename.clone(), data)); - } - Some(digest) => { - digests.insert(entry.clone(), digest.to_string()); + new_cached_entries.push(entry.filename.clone()); + hex::encode(entry.compute_raw_hash::()?) } + Some(digest) => digest, }; + digests.insert(entry, hash); if progress.report(ix) { info!(logger, "Hashing: {progress}"); From e2e2b0e61a1f492603eb98468062978371278fb0 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Fri, 29 Nov 2024 10:28:26 +0100 Subject: [PATCH 05/15] refactor: enhance use of aliases in `ImmutableDigester` - use `StdError` alias instead of `anyhow::Error` - remove unnecessary `ComputedImmutablesDigestsResult` alias --- .../src/digesters/cardano_immutable_digester.rs | 8 ++------ mithril-common/src/digesters/immutable_digester.rs | 3 ++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/mithril-common/src/digesters/cardano_immutable_digester.rs b/mithril-common/src/digesters/cardano_immutable_digester.rs index 425d02026d2..85a4a6dc222 100644 --- a/mithril-common/src/digesters/cardano_immutable_digester.rs +++ b/mithril-common/src/digesters/cardano_immutable_digester.rs @@ -12,10 +12,6 @@ use sha2::{Digest, Sha256}; use slog::{debug, info, warn, Logger}; use std::{collections::BTreeMap, io, path::Path, sync::Arc}; -/// Result of a cache computation, contains the list of immutable digests and the list of new entries to add -/// to the [ImmutableFileDigestCacheProvider]. -type ComputedImmutablesDigestsResult = Result; - struct ComputedImmutablesDigests { entries: BTreeMap, new_cached_entries: Vec, @@ -67,7 +63,7 @@ impl CardanoImmutableDigester { // The computation of immutable files digests is done in a separate thread because it is blocking the whole task let logger = self.logger.clone(); let computed_digests = - tokio::task::spawn_blocking(move || -> ComputedImmutablesDigestsResult { + tokio::task::spawn_blocking(move || -> Result { compute_immutables_digests(logger, cached_values) }) .await @@ -183,7 +179,7 @@ fn list_immutable_files_to_process( fn compute_immutables_digests( logger: Logger, entries: BTreeMap>, -) -> ComputedImmutablesDigestsResult { +) -> Result { let mut new_cached_entries = Vec::new(); let mut progress = Progress { index: 0, diff --git a/mithril-common/src/digesters/immutable_digester.rs b/mithril-common/src/digesters/immutable_digester.rs index 04ebc5925f9..908c7743926 100644 --- a/mithril-common/src/digesters/immutable_digester.rs +++ b/mithril-common/src/digesters/immutable_digester.rs @@ -2,6 +2,7 @@ use crate::{ crypto_helper::{MKTree, MKTreeStoreInMemory}, digesters::ImmutableFileListingError, entities::{CardanoDbBeacon, ImmutableFileNumber}, + StdError, }; use async_trait::async_trait; use std::{ @@ -89,5 +90,5 @@ pub enum ImmutableDigesterError { /// Error raised when the Merkle tree computation failed. #[error("Merkle tree computation failed")] - MerkleTreeComputationError(anyhow::Error), + MerkleTreeComputationError(StdError), } From acf7e527b6276d24875f54ef10f8acc8f3f7e1ab Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Tue, 26 Nov 2024 11:10:44 +0100 Subject: [PATCH 06/15] feat: extend ProtocolMessagePartKey with `CardanoDatabaseMerkleRoot` --- .../src/entities/protocol_message.rs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/mithril-common/src/entities/protocol_message.rs b/mithril-common/src/entities/protocol_message.rs index 2aa0fdf53e8..fa61b879357 100644 --- a/mithril-common/src/entities/protocol_message.rs +++ b/mithril-common/src/entities/protocol_message.rs @@ -46,6 +46,10 @@ pub enum ProtocolMessagePartKey { /// The ProtocolMessage part key associated to the Cardano stake distribution Merkle root #[serde(rename = "cardano_stake_distribution_merkle_root")] CardanoStakeDistributionMerkleRoot, + + /// The ProtocolMessage part key associated to the Cardano database Merkle root + #[serde(rename = "cardano_database_merkle_root")] + CardanoDatabaseMerkleRoot, } impl Display for ProtocolMessagePartKey { @@ -61,6 +65,7 @@ impl Display for ProtocolMessagePartKey { Self::CardanoStakeDistributionMerkleRoot => { write!(f, "cardano_stake_distribution_merkle_root") } + Self::CardanoDatabaseMerkleRoot => write!(f, "cardano_database_merkle_root"), } } } @@ -207,6 +212,20 @@ mod tests { assert_ne!(hash_expected, protocol_message_modified.compute_hash()); } + #[test] + fn test_protocol_message_compute_hash_include_cardano_database_merkle_root() { + let protocol_message = build_protocol_message_reference(); + let hash_expected = protocol_message.compute_hash(); + + let mut protocol_message_modified = protocol_message.clone(); + protocol_message_modified.set_message_part( + ProtocolMessagePartKey::CardanoDatabaseMerkleRoot, + "cardano-database-merkle-root-456".to_string(), + ); + + assert_ne!(hash_expected, protocol_message_modified.compute_hash()); + } + #[test] fn test_protocol_message_compute_hash_include_next_protocol_parameters() { let protocol_message = build_protocol_message_reference(); @@ -259,6 +278,10 @@ mod tests { ProtocolMessagePartKey::CardanoStakeDistributionMerkleRoot, "cardano-stake-distribution-merkle-root-123".to_string(), ); + protocol_message.set_message_part( + ProtocolMessagePartKey::CardanoDatabaseMerkleRoot, + "cardano-database-merkle-root-123".to_string(), + ); protocol_message } From d2e12f4a83dd4627384a757eeea5378186f04ffe Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Tue, 26 Nov 2024 17:18:14 +0100 Subject: [PATCH 07/15] feat: implement `CardanoDatabaseSignableBuilder` --- .../src/signable_builder/cardano_database.rs | 135 ++++++++++++++++++ mithril-common/src/signable_builder/mod.rs | 2 + 2 files changed, 137 insertions(+) create mode 100644 mithril-common/src/signable_builder/cardano_database.rs diff --git a/mithril-common/src/signable_builder/cardano_database.rs b/mithril-common/src/signable_builder/cardano_database.rs new file mode 100644 index 00000000000..bb36a005f1e --- /dev/null +++ b/mithril-common/src/signable_builder/cardano_database.rs @@ -0,0 +1,135 @@ +use std::{ + path::{Path, PathBuf}, + sync::Arc, +}; + +use anyhow::Context; +use async_trait::async_trait; +use slog::{info, Logger}; + +use crate::{ + digesters::ImmutableDigester, + entities::{CardanoDbBeacon, ProtocolMessage, ProtocolMessagePartKey}, + logging::LoggerExtensions, + signable_builder::SignableBuilder, + StdResult, +}; + +/// This structure is responsible for calculating the message for incremental Cardano database. +pub struct CardanoDatabaseSignableBuilder { + digester: Arc, + logger: Logger, + dirpath: PathBuf, +} + +impl CardanoDatabaseSignableBuilder { + /// Constructor + pub fn new(digester: Arc, dirpath: &Path, logger: Logger) -> Self { + Self { + digester, + logger: logger.new_with_component_name::(), + dirpath: dirpath.to_owned(), + } + } +} + +#[async_trait] +impl SignableBuilder for CardanoDatabaseSignableBuilder { + async fn compute_protocol_message( + &self, + beacon: CardanoDbBeacon, + ) -> StdResult { + let merkle_tree = self + .digester + .compute_merkle_tree(&self.dirpath, &beacon) + .await + .with_context(|| { + format!( + "Cardano Database Signable Builder can not compute merkle tree of '{}'", + &self.dirpath.display() + ) + })?; + + let merkle_root = merkle_tree.compute_root()?.to_hex(); + info!( + self.logger, + "Computed Cardano database Merkle root = '{merkle_root}'" + ); + + let mut protocol_message = ProtocolMessage::new(); + protocol_message.set_message_part( + ProtocolMessagePartKey::CardanoDatabaseMerkleRoot, + merkle_root, + ); + + Ok(protocol_message) + } +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use crate::{ + crypto_helper::{MKTree, MKTreeStoreInMemory}, + digesters::ImmutableDigesterError, + entities::{CardanoDbBeacon, ProtocolMessagePartKey}, + test_utils::TestLogger, + }; + + use super::*; + + #[derive(Default)] + pub struct ImmutableDigesterImpl { + digests: Vec, + } + + impl ImmutableDigesterImpl { + pub fn new(digests: Vec) -> Self { + Self { digests } + } + } + + #[async_trait] + impl ImmutableDigester for ImmutableDigesterImpl { + async fn compute_digest( + &self, + _dirpath: &Path, + _beacon: &CardanoDbBeacon, + ) -> Result { + Ok("whatever".to_string()) + } + + async fn compute_merkle_tree( + &self, + _dirpath: &Path, + _beacon: &CardanoDbBeacon, + ) -> Result, ImmutableDigesterError> { + Ok(MKTree::new(&self.digests).unwrap()) + } + } + + #[tokio::test] + async fn compute_signable() { + let digests = vec!["digest-1".to_string(), "digest-2".to_string()]; + let digester = ImmutableDigesterImpl::new(digests.clone()); + let signable_builder = CardanoDatabaseSignableBuilder::new( + Arc::new(digester), + Path::new(""), + TestLogger::stdout(), + ); + + let protocol_message = signable_builder + .compute_protocol_message(CardanoDbBeacon::default()) + .await + .unwrap(); + + let expected_mktree: MKTree = MKTree::new(&digests).unwrap(); + let mut expected_message = ProtocolMessage::new(); + expected_message.set_message_part( + ProtocolMessagePartKey::CardanoDatabaseMerkleRoot, + expected_mktree.compute_root().unwrap().to_hex(), + ); + assert_eq!(expected_message, protocol_message); + } +} diff --git a/mithril-common/src/signable_builder/mod.rs b/mithril-common/src/signable_builder/mod.rs index 495afcf35ff..84bd6cda73d 100644 --- a/mithril-common/src/signable_builder/mod.rs +++ b/mithril-common/src/signable_builder/mod.rs @@ -11,9 +11,11 @@ pub use mithril_stake_distribution::*; pub use signable_builder_service::*; cfg_fs! { + mod cardano_database; mod cardano_immutable_full_signable_builder; mod cardano_transactions; + pub use cardano_database::*; pub use cardano_immutable_full_signable_builder::*; pub use cardano_transactions::*; } From e00ca1e7d0671b67329f33bee48fb9b7ed775284 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Wed, 27 Nov 2024 17:53:40 +0100 Subject: [PATCH 08/15] refactor: introduce `SignableBuilderServiceDependencies` to simplify `MithrilSignableBuilderService` constructor It reduces the number of parameters in the constructor by encapsulating dependencies into a single struct. --- .../src/dependency_injection/builder.rs | 13 +++-- .../signable_builder_service.rs | 47 +++++++++++++++---- .../src/dependency_injection/builder.rs | 11 +++-- mithril-signer/src/runtime/runner.rs | 10 ++-- .../test_extensions/state_machine_tester.rs | 11 +++-- 5 files changed, 66 insertions(+), 26 deletions(-) diff --git a/mithril-aggregator/src/dependency_injection/builder.rs b/mithril-aggregator/src/dependency_injection/builder.rs index 3576fdafdf4..9af3329f2ba 100644 --- a/mithril-aggregator/src/dependency_injection/builder.rs +++ b/mithril-aggregator/src/dependency_injection/builder.rs @@ -37,8 +37,8 @@ use mithril_common::{ signable_builder::{ CardanoImmutableFilesFullSignableBuilder, CardanoStakeDistributionSignableBuilder, CardanoTransactionsSignableBuilder, MithrilSignableBuilderService, - MithrilStakeDistributionSignableBuilder, SignableBuilderService, SignableSeedBuilder, - TransactionsImporter, + MithrilStakeDistributionSignableBuilder, SignableBuilderService, + SignableBuilderServiceDependencies, SignableSeedBuilder, TransactionsImporter, }, signed_entity_type_lock::SignedEntityTypeLock, MithrilTickerService, TickerService, @@ -1129,13 +1129,16 @@ impl DependenciesBuilder { CardanoStakeDistributionSignableBuilder::new(self.get_stake_store().await?), ); let era_checker = self.get_era_checker().await?; - let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( - era_checker, - seed_signable_builder, + let signable_builders_dependencies = SignableBuilderServiceDependencies::new( mithril_stake_distribution_builder, immutable_signable_builder, cardano_transactions_builder, cardano_stake_distribution_builder, + ); + let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( + era_checker, + seed_signable_builder, + signable_builders_dependencies, self.root_logger(), )); diff --git a/mithril-common/src/signable_builder/signable_builder_service.rs b/mithril-common/src/signable_builder/signable_builder_service.rs index 9c15044c9ba..21b9ee02c35 100644 --- a/mithril-common/src/signable_builder/signable_builder_service.rs +++ b/mithril-common/src/signable_builder/signable_builder_service.rs @@ -36,24 +36,47 @@ pub struct MithrilSignableBuilderService { logger: Logger, } -impl MithrilSignableBuilderService { - /// MithrilSignableBuilderService factory +/// SignableBuilders dependencies required by the [MithrilSignableBuilderService]. +pub struct SignableBuilderServiceDependencies { + mithril_stake_distribution_builder: Arc>, + immutable_signable_builder: Arc>, + cardano_transactions_signable_builder: Arc>, + cardano_stake_distribution_builder: Arc>, +} + +impl SignableBuilderServiceDependencies { + /// Create a new instance of [SignableBuilderServiceDependencies]. pub fn new( - era_checker: Arc, - seed_signable_builder: Arc, mithril_stake_distribution_builder: Arc>, immutable_signable_builder: Arc>, cardano_transactions_signable_builder: Arc>, cardano_stake_distribution_builder: Arc>, - logger: Logger, ) -> Self { Self { - era_checker, - seed_signable_builder, mithril_stake_distribution_builder, immutable_signable_builder, cardano_transactions_signable_builder, cardano_stake_distribution_builder, + } + } +} + +impl MithrilSignableBuilderService { + /// MithrilSignableBuilderService factory + pub fn new( + era_checker: Arc, + seed_signable_builder: Arc, + dependencies: SignableBuilderServiceDependencies, + logger: Logger, + ) -> Self { + Self { + era_checker, + seed_signable_builder, + mithril_stake_distribution_builder: dependencies.mithril_stake_distribution_builder, + immutable_signable_builder: dependencies.immutable_signable_builder, + cardano_transactions_signable_builder: dependencies + .cardano_transactions_signable_builder, + cardano_stake_distribution_builder: dependencies.cardano_stake_distribution_builder, logger: logger.new_with_component_name::(), } } @@ -200,13 +223,17 @@ mod tests { } fn build_signable_builder_service(self) -> MithrilSignableBuilderService { - MithrilSignableBuilderService::new( - Arc::new(self.era_checker), - Arc::new(self.mock_signable_seed_builder), + let dependencies = SignableBuilderServiceDependencies::new( Arc::new(self.mock_mithril_stake_distribution_signable_builder), Arc::new(self.mock_cardano_immutable_files_full_signable_builder), Arc::new(self.mock_cardano_transactions_signable_builder), Arc::new(self.mock_cardano_stake_distribution_signable_builder), + ); + + MithrilSignableBuilderService::new( + Arc::new(self.era_checker), + Arc::new(self.mock_signable_seed_builder), + dependencies, TestLogger::stdout(), ) } diff --git a/mithril-signer/src/dependency_injection/builder.rs b/mithril-signer/src/dependency_injection/builder.rs index 746d687e018..c84b50a0539 100644 --- a/mithril-signer/src/dependency_injection/builder.rs +++ b/mithril-signer/src/dependency_injection/builder.rs @@ -24,7 +24,7 @@ use mithril_common::era::{EraChecker, EraReader}; use mithril_common::signable_builder::{ CardanoImmutableFilesFullSignableBuilder, CardanoStakeDistributionSignableBuilder, CardanoTransactionsSignableBuilder, MithrilSignableBuilderService, - MithrilStakeDistributionSignableBuilder, + MithrilStakeDistributionSignableBuilder, SignableBuilderServiceDependencies, }; use mithril_common::signed_entity_type_lock::SignedEntityTypeLock; use mithril_common::{MithrilTickerService, StdResult, TickerService}; @@ -342,13 +342,16 @@ impl<'a> DependenciesBuilder<'a> { epoch_service.clone(), protocol_initializer_store.clone(), )); - let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( - era_checker.clone(), - signable_seed_builder_service, + let signable_builders_dependencies = SignableBuilderServiceDependencies::new( mithril_stake_distribution_signable_builder, cardano_immutable_snapshot_builder, cardano_transactions_builder, cardano_stake_distribution_signable_builder, + ); + let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( + era_checker.clone(), + signable_seed_builder_service, + signable_builders_dependencies, self.root_logger(), )); let metrics_service = Arc::new(MetricsService::new(self.root_logger())?); diff --git a/mithril-signer/src/runtime/runner.rs b/mithril-signer/src/runtime/runner.rs index 14882391992..5addf25ab2e 100644 --- a/mithril-signer/src/runtime/runner.rs +++ b/mithril-signer/src/runtime/runner.rs @@ -327,6 +327,7 @@ impl Runner for SignerRunner { #[cfg(test)] mod tests { + use mithril_common::signable_builder::SignableBuilderServiceDependencies; use mockall::mock; use mockall::predicate::eq; use std::collections::BTreeSet; @@ -464,13 +465,16 @@ mod tests { epoch_service.clone(), protocol_initializer_store.clone(), )); - let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( - era_checker.clone(), - signable_seed_builder_service, + let signable_builders_dependencies = SignableBuilderServiceDependencies::new( mithril_stake_distribution_signable_builder, cardano_immutable_signable_builder, cardano_transactions_builder, cardano_stake_distribution_builder, + ); + let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( + era_checker.clone(), + signable_seed_builder_service, + signable_builders_dependencies, logger.clone(), )); let metrics_service = Arc::new(MetricsService::new(logger.clone()).unwrap()); diff --git a/mithril-signer/tests/test_extensions/state_machine_tester.rs b/mithril-signer/tests/test_extensions/state_machine_tester.rs index 812e1c36d36..26ce9e14ba8 100644 --- a/mithril-signer/tests/test_extensions/state_machine_tester.rs +++ b/mithril-signer/tests/test_extensions/state_machine_tester.rs @@ -30,7 +30,7 @@ use mithril_common::{ signable_builder::{ CardanoImmutableFilesFullSignableBuilder, CardanoStakeDistributionSignableBuilder, CardanoTransactionsSignableBuilder, MithrilSignableBuilderService, - MithrilStakeDistributionSignableBuilder, + MithrilStakeDistributionSignableBuilder, SignableBuilderServiceDependencies, }, signed_entity_type_lock::SignedEntityTypeLock, MithrilTickerService, StdError, TickerService, @@ -225,13 +225,16 @@ impl StateMachineTester { epoch_service.clone(), protocol_initializer_store.clone(), )); - let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( - era_checker.clone(), - signable_seed_builder_service, + let signable_builders_dependencies = SignableBuilderServiceDependencies::new( mithril_stake_distribution_signable_builder, cardano_immutable_snapshot_builder, cardano_transactions_builder, cardano_stake_distribution_builder, + ); + let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( + era_checker.clone(), + signable_seed_builder_service, + signable_builders_dependencies, logger.clone(), )); let metrics_service = Arc::new(MetricsService::new(logger.clone()).unwrap()); From 9390fe6cd3bc0b8c28fc45b69e095654c554a396 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Wed, 27 Nov 2024 18:45:54 +0100 Subject: [PATCH 09/15] feat: wire the `CardanoDatabaseSignableBuilder` in the `MithrilSignableBuilderService` --- .../src/dependency_injection/builder.rs | 15 ++++-- .../signable_builder_service.rs | 51 +++++++++++++++---- .../src/dependency_injection/builder.rs | 13 +++-- mithril-signer/src/runtime/runner.rs | 10 +++- .../test_extensions/state_machine_tester.rs | 13 +++-- 5 files changed, 82 insertions(+), 20 deletions(-) diff --git a/mithril-aggregator/src/dependency_injection/builder.rs b/mithril-aggregator/src/dependency_injection/builder.rs index 9af3329f2ba..73c2786f1db 100644 --- a/mithril-aggregator/src/dependency_injection/builder.rs +++ b/mithril-aggregator/src/dependency_injection/builder.rs @@ -35,10 +35,11 @@ use mithril_common::{ EraChecker, EraMarker, EraReader, EraReaderAdapter, SupportedEra, }, signable_builder::{ - CardanoImmutableFilesFullSignableBuilder, CardanoStakeDistributionSignableBuilder, - CardanoTransactionsSignableBuilder, MithrilSignableBuilderService, - MithrilStakeDistributionSignableBuilder, SignableBuilderService, - SignableBuilderServiceDependencies, SignableSeedBuilder, TransactionsImporter, + CardanoDatabaseSignableBuilder, CardanoImmutableFilesFullSignableBuilder, + CardanoStakeDistributionSignableBuilder, CardanoTransactionsSignableBuilder, + MithrilSignableBuilderService, MithrilStakeDistributionSignableBuilder, + SignableBuilderService, SignableBuilderServiceDependencies, SignableSeedBuilder, + TransactionsImporter, }, signed_entity_type_lock::SignedEntityTypeLock, MithrilTickerService, TickerService, @@ -1128,12 +1129,18 @@ impl DependenciesBuilder { let cardano_stake_distribution_builder = Arc::new( CardanoStakeDistributionSignableBuilder::new(self.get_stake_store().await?), ); + let cardano_database_signable_builder = Arc::new(CardanoDatabaseSignableBuilder::new( + self.get_immutable_digester().await?, + &self.configuration.db_directory, + self.root_logger(), + )); let era_checker = self.get_era_checker().await?; let signable_builders_dependencies = SignableBuilderServiceDependencies::new( mithril_stake_distribution_builder, immutable_signable_builder, cardano_transactions_builder, cardano_stake_distribution_builder, + cardano_database_signable_builder, ); let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( era_checker, diff --git a/mithril-common/src/signable_builder/signable_builder_service.rs b/mithril-common/src/signable_builder/signable_builder_service.rs index 21b9ee02c35..92e030d69b0 100644 --- a/mithril-common/src/signable_builder/signable_builder_service.rs +++ b/mithril-common/src/signable_builder/signable_builder_service.rs @@ -33,6 +33,7 @@ pub struct MithrilSignableBuilderService { immutable_signable_builder: Arc>, cardano_transactions_signable_builder: Arc>, cardano_stake_distribution_builder: Arc>, + cardano_database_signable_builder: Arc>, logger: Logger, } @@ -42,6 +43,7 @@ pub struct SignableBuilderServiceDependencies { immutable_signable_builder: Arc>, cardano_transactions_signable_builder: Arc>, cardano_stake_distribution_builder: Arc>, + cardano_database_signable_builder: Arc>, } impl SignableBuilderServiceDependencies { @@ -51,12 +53,14 @@ impl SignableBuilderServiceDependencies { immutable_signable_builder: Arc>, cardano_transactions_signable_builder: Arc>, cardano_stake_distribution_builder: Arc>, + cardano_database_signable_builder: Arc>, ) -> Self { Self { mithril_stake_distribution_builder, immutable_signable_builder, cardano_transactions_signable_builder, cardano_stake_distribution_builder, + cardano_database_signable_builder, } } } @@ -77,6 +81,7 @@ impl MithrilSignableBuilderService { cardano_transactions_signable_builder: dependencies .cardano_transactions_signable_builder, cardano_stake_distribution_builder: dependencies.cardano_stake_distribution_builder, + cardano_database_signable_builder: dependencies.cardano_database_signable_builder, logger: logger.new_with_component_name::(), } } @@ -117,11 +122,14 @@ impl MithrilSignableBuilderService { .with_context(|| format!( "Signable builder service can not compute protocol message with block_number: '{block_number}'" ))?, - SignedEntityType::CardanoDatabase(_) => { - return Err(anyhow::anyhow!( - "Signable builder service can not compute protocol message for Cardano database because it is not yet implemented." - )); - } + SignedEntityType::CardanoDatabase(beacon) => + self + .cardano_database_signable_builder + .compute_protocol_message(beacon.clone()) + .await + .with_context(|| format!( + "Signable builder service can not compute protocol message for Cardano database with beacon: '{beacon}'" + ))?, }; Ok(protocol_message) @@ -208,6 +216,7 @@ mod tests { MockSignableBuilderImpl, mock_cardano_transactions_signable_builder: MockSignableBuilderImpl, mock_cardano_stake_distribution_signable_builder: MockSignableBuilderImpl, + mock_cardano_database_signable_builder: MockSignableBuilderImpl, } impl MockDependencyInjector { @@ -219,6 +228,7 @@ mod tests { mock_cardano_immutable_files_full_signable_builder: MockSignableBuilderImpl::new(), mock_cardano_stake_distribution_signable_builder: MockSignableBuilderImpl::new(), mock_cardano_transactions_signable_builder: MockSignableBuilderImpl::new(), + mock_cardano_database_signable_builder: MockSignableBuilderImpl::new(), } } @@ -228,6 +238,7 @@ mod tests { Arc::new(self.mock_cardano_immutable_files_full_signable_builder), Arc::new(self.mock_cardano_transactions_signable_builder), Arc::new(self.mock_cardano_stake_distribution_signable_builder), + Arc::new(self.mock_cardano_database_signable_builder), ); MithrilSignableBuilderService::new( @@ -340,17 +351,21 @@ mod tests { } #[tokio::test] - async fn build_cardano_database_signable_when_given_cardano_database_entity_type_return_error( - ) { + async fn build_cardano_database_signable_when_given_cardano_database_entity_type() { let current_era = SupportedEra::Pythagoras; - let mock_container = MockDependencyInjector::new(current_era); + let mut mock_container = build_mock_container(current_era); + mock_container + .mock_cardano_database_signable_builder + .expect_compute_protocol_message() + .once() + .return_once(|_| Ok(ProtocolMessage::new())); let signable_builder_service = mock_container.build_signable_builder_service(); let signed_entity_type = SignedEntityType::CardanoDatabase(CardanoDbBeacon::default()); signable_builder_service .compute_protocol_message(signed_entity_type) .await - .expect_err("Should return error because CardanoDatabase is not implemented yet."); + .unwrap(); } } @@ -443,5 +458,23 @@ mod tests { .await .unwrap(); } + + #[tokio::test] + async fn build_cardano_database_signable_when_given_cardano_database_entity_type() { + let current_era = SupportedEra::Thales; + let mut mock_container = build_mock_container(current_era); + mock_container + .mock_cardano_database_signable_builder + .expect_compute_protocol_message() + .once() + .return_once(|_| Ok(ProtocolMessage::new())); + let signable_builder_service = mock_container.build_signable_builder_service(); + let signed_entity_type = SignedEntityType::CardanoDatabase(CardanoDbBeacon::default()); + + signable_builder_service + .compute_protocol_message(signed_entity_type) + .await + .unwrap(); + } } } diff --git a/mithril-signer/src/dependency_injection/builder.rs b/mithril-signer/src/dependency_injection/builder.rs index c84b50a0539..e7ad5e56392 100644 --- a/mithril-signer/src/dependency_injection/builder.rs +++ b/mithril-signer/src/dependency_injection/builder.rs @@ -22,9 +22,10 @@ use mithril_common::digesters::{ }; use mithril_common::era::{EraChecker, EraReader}; use mithril_common::signable_builder::{ - CardanoImmutableFilesFullSignableBuilder, CardanoStakeDistributionSignableBuilder, - CardanoTransactionsSignableBuilder, MithrilSignableBuilderService, - MithrilStakeDistributionSignableBuilder, SignableBuilderServiceDependencies, + CardanoDatabaseSignableBuilder, CardanoImmutableFilesFullSignableBuilder, + CardanoStakeDistributionSignableBuilder, CardanoTransactionsSignableBuilder, + MithrilSignableBuilderService, MithrilStakeDistributionSignableBuilder, + SignableBuilderServiceDependencies, }; use mithril_common::signed_entity_type_lock::SignedEntityTypeLock; use mithril_common::{MithrilTickerService, StdResult, TickerService}; @@ -328,6 +329,11 @@ impl<'a> DependenciesBuilder<'a> { let cardano_stake_distribution_signable_builder = Arc::new( CardanoStakeDistributionSignableBuilder::new(stake_store.clone()), ); + let cardano_database_signable_builder = Arc::new(CardanoDatabaseSignableBuilder::new( + digester.clone(), + &self.config.db_directory, + self.root_logger(), + )); let epoch_service = Arc::new(RwLock::new(MithrilEpochService::new( stake_store.clone(), protocol_initializer_store.clone(), @@ -347,6 +353,7 @@ impl<'a> DependenciesBuilder<'a> { cardano_immutable_snapshot_builder, cardano_transactions_builder, cardano_stake_distribution_signable_builder, + cardano_database_signable_builder, ); let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( era_checker.clone(), diff --git a/mithril-signer/src/runtime/runner.rs b/mithril-signer/src/runtime/runner.rs index 5addf25ab2e..da21bbe71a4 100644 --- a/mithril-signer/src/runtime/runner.rs +++ b/mithril-signer/src/runtime/runner.rs @@ -327,7 +327,9 @@ impl Runner for SignerRunner { #[cfg(test)] mod tests { - use mithril_common::signable_builder::SignableBuilderServiceDependencies; + use mithril_common::signable_builder::{ + CardanoDatabaseSignableBuilder, SignableBuilderServiceDependencies, + }; use mockall::mock; use mockall::predicate::eq; use std::collections::BTreeSet; @@ -447,6 +449,11 @@ mod tests { let cardano_stake_distribution_builder = Arc::new( CardanoStakeDistributionSignableBuilder::new(stake_store.clone()), ); + let cardano_database_signable_builder = Arc::new(CardanoDatabaseSignableBuilder::new( + digester.clone(), + Path::new(""), + logger.clone(), + )); let protocol_initializer_store = Arc::new(ProtocolInitializerRepository::new( sqlite_connection.clone(), None, @@ -470,6 +477,7 @@ mod tests { cardano_immutable_signable_builder, cardano_transactions_builder, cardano_stake_distribution_builder, + cardano_database_signable_builder, ); let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( era_checker.clone(), diff --git a/mithril-signer/tests/test_extensions/state_machine_tester.rs b/mithril-signer/tests/test_extensions/state_machine_tester.rs index 26ce9e14ba8..990d6f82dce 100644 --- a/mithril-signer/tests/test_extensions/state_machine_tester.rs +++ b/mithril-signer/tests/test_extensions/state_machine_tester.rs @@ -28,9 +28,10 @@ use mithril_common::{ }, era::{adapters::EraReaderDummyAdapter, EraChecker, EraMarker, EraReader, SupportedEra}, signable_builder::{ - CardanoImmutableFilesFullSignableBuilder, CardanoStakeDistributionSignableBuilder, - CardanoTransactionsSignableBuilder, MithrilSignableBuilderService, - MithrilStakeDistributionSignableBuilder, SignableBuilderServiceDependencies, + CardanoDatabaseSignableBuilder, CardanoImmutableFilesFullSignableBuilder, + CardanoStakeDistributionSignableBuilder, CardanoTransactionsSignableBuilder, + MithrilSignableBuilderService, MithrilStakeDistributionSignableBuilder, + SignableBuilderServiceDependencies, }, signed_entity_type_lock::SignedEntityTypeLock, MithrilTickerService, StdError, TickerService, @@ -211,6 +212,11 @@ impl StateMachineTester { let cardano_stake_distribution_builder = Arc::new( CardanoStakeDistributionSignableBuilder::new(stake_store.clone()), ); + let cardano_database_signable_builder = Arc::new(CardanoDatabaseSignableBuilder::new( + digester.clone(), + Path::new(""), + logger.clone(), + )); let epoch_service = Arc::new(RwLock::new(MithrilEpochService::new( stake_store.clone(), protocol_initializer_store.clone(), @@ -230,6 +236,7 @@ impl StateMachineTester { cardano_immutable_snapshot_builder, cardano_transactions_builder, cardano_stake_distribution_builder, + cardano_database_signable_builder, ); let signable_builder_service = Arc::new(MithrilSignableBuilderService::new( era_checker.clone(), From a5ffcf388f4e707a17b73396ef277cd04bf7d870 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Thu, 28 Nov 2024 12:25:25 +0100 Subject: [PATCH 10/15] ci: activate `CardanoDatabase` signed entity type in the e2e test --- mithril-common/src/digesters/immutable_digester.rs | 11 +++++++++++ mithril-test-lab/mithril-end-to-end/src/main.rs | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/mithril-common/src/digesters/immutable_digester.rs b/mithril-common/src/digesters/immutable_digester.rs index 908c7743926..299d0011749 100644 --- a/mithril-common/src/digesters/immutable_digester.rs +++ b/mithril-common/src/digesters/immutable_digester.rs @@ -19,6 +19,8 @@ use thiserror::Error; /// use async_trait::async_trait; /// use mithril_common::digesters::{ImmutableDigester, ImmutableDigesterError}; /// use mithril_common::entities::CardanoDbBeacon; +/// use mithril_common::crypto_helper::{MKTree, MKTreeStoreInMemory}; +/// use anyhow::anyhow; /// use mockall::mock; /// use std::path::Path; /// @@ -32,6 +34,12 @@ use thiserror::Error; /// dirpath: &Path, /// beacon: &CardanoDbBeacon, /// ) -> Result; +/// +/// async fn compute_merkle_tree( +/// &self, +/// dirpath: &Path, +/// beacon: &CardanoDbBeacon, +/// ) -> Result, ImmutableDigesterError>; /// } /// } /// @@ -45,6 +53,9 @@ use thiserror::Error; /// db_dir: PathBuff::new(), /// }) /// }); +/// mock.expect_compute_merkle_tree().return_once(|_, _| { +/// Err(ImmutableDigesterError::MerkleTreeComputationError(anyhow!("Error"))) +/// }); /// } /// } /// ``` diff --git a/mithril-test-lab/mithril-end-to-end/src/main.rs b/mithril-test-lab/mithril-end-to-end/src/main.rs index 0a6e6e69228..7edec220605 100644 --- a/mithril-test-lab/mithril-end-to-end/src/main.rs +++ b/mithril-test-lab/mithril-end-to-end/src/main.rs @@ -95,7 +95,7 @@ pub struct Args { #[clap( long, value_delimiter = ',', - default_value = "CardanoTransactions,CardanoStakeDistribution" + default_value = "CardanoTransactions,CardanoStakeDistribution,CardanoDatabase" )] signed_entity_types: Vec, From 2209f530546af06ecf506f7bde83594d0dbdb735 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Fri, 29 Nov 2024 11:29:34 +0100 Subject: [PATCH 11/15] refactor: extract cache retrieval logic from `process_immutables` into a dedicated function --- .../digesters/cardano_immutable_digester.rs | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/mithril-common/src/digesters/cardano_immutable_digester.rs b/mithril-common/src/digesters/cardano_immutable_digester.rs index 85a4a6dc222..4fe2f417b83 100644 --- a/mithril-common/src/digesters/cardano_immutable_digester.rs +++ b/mithril-common/src/digesters/cardano_immutable_digester.rs @@ -46,19 +46,7 @@ impl CardanoImmutableDigester { &self, immutables: Vec, ) -> Result { - let cached_values = match self.cache_provider.as_ref() { - None => BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))), - Some(cache_provider) => match cache_provider.get(immutables.clone()).await { - Ok(values) => values, - Err(error) => { - warn!( - self.logger, "Error while getting cached immutable files digests"; - "error" => ?error - ); - BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))) - } - }, - }; + let cached_values = self.fetch_immutables_cached(immutables).await; // The computation of immutable files digests is done in a separate thread because it is blocking the whole task let logger = self.logger.clone(); @@ -72,6 +60,25 @@ impl CardanoImmutableDigester { Ok(computed_digests) } + async fn fetch_immutables_cached( + &self, + immutables: Vec, + ) -> BTreeMap> { + match self.cache_provider.as_ref() { + None => BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))), + Some(cache_provider) => match cache_provider.get(immutables.clone()).await { + Ok(values) => values, + Err(error) => { + warn!( + self.logger, "Error while getting cached immutable files digests"; + "error" => ?error + ); + BTreeMap::from_iter(immutables.into_iter().map(|i| (i, None))) + } + }, + } + } + async fn update_cache(&self, computed_immutables_digests: &ComputedImmutablesDigests) { if let Some(cache_provider) = self.cache_provider.as_ref() { let new_cached_entries = computed_immutables_digests From ac1e197213d00f04b3a5f448e10bbe5075b55f7b Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Fri, 29 Nov 2024 12:10:27 +0100 Subject: [PATCH 12/15] refactor: enhance test tooling for `ImmutableDigester` Co-authored-by: DJO --- .../src/digesters/dumb_immutable_digester.rs | 88 +++++++++++++++++++ .../src/digesters/dumb_immutable_observer.rs | 63 ------------- mithril-common/src/digesters/mod.rs | 4 +- .../src/signable_builder/cardano_database.rs | 34 +------ ...cardano_immutable_full_signable_builder.rs | 28 +----- mithril-signer/src/runtime/runner.rs | 2 +- .../test_extensions/state_machine_tester.rs | 2 +- 7 files changed, 96 insertions(+), 125 deletions(-) create mode 100644 mithril-common/src/digesters/dumb_immutable_digester.rs delete mode 100644 mithril-common/src/digesters/dumb_immutable_observer.rs diff --git a/mithril-common/src/digesters/dumb_immutable_digester.rs b/mithril-common/src/digesters/dumb_immutable_digester.rs new file mode 100644 index 00000000000..82b4320d7b4 --- /dev/null +++ b/mithril-common/src/digesters/dumb_immutable_digester.rs @@ -0,0 +1,88 @@ +use std::path::Path; + +use crate::{ + crypto_helper::{MKTree, MKTreeStoreInMemory}, + digesters::{ImmutableDigester, ImmutableDigesterError}, + entities::CardanoDbBeacon, +}; +use async_trait::async_trait; +use tokio::sync::RwLock; + +/// A [ImmutableDigester] returning configurable result for testing purpose. +pub struct DumbImmutableDigester { + digest: RwLock, + mktree_leaves: RwLock>, + is_success: bool, +} + +impl DumbImmutableDigester { + /// Set the digest returned by [compute_digest][DumbImmutableDigester::compute_digest] + pub fn with_digest(mut self, new_digest: &str) -> Self { + self.digest = RwLock::new(new_digest.to_string()); + self + } + + /// Set the leaves used to construct the merkle tree returned by [compute_merkle_tree][DumbImmutableDigester::compute_merkle_tree] + pub fn with_merkle_tree(mut self, leaves: Vec) -> Self { + self.mktree_leaves = RwLock::new(leaves); + self + } + + /// Update digest returned by [compute_digest][DumbImmutableDigester::compute_digest] + pub async fn update_digest(&self, new_digest: String) { + let mut digest = self.digest.write().await; + *digest = new_digest; + } + + /// Update the leaves used to construct the merkle tree returned by [compute_merkle_tree][DumbImmutableDigester::compute_merkle_tree] + pub async fn update_merkle_tree(&self, leaves: Vec) { + let mut mktree_leaves = self.mktree_leaves.write().await; + *mktree_leaves = leaves; + } +} + +impl Default for DumbImmutableDigester { + fn default() -> Self { + Self { + digest: RwLock::new(String::from("1234")), + mktree_leaves: RwLock::new(vec!["1".to_string(), "2".to_string(), "3".to_string()]), + is_success: true, + } + } +} + +#[async_trait] +impl ImmutableDigester for DumbImmutableDigester { + async fn compute_digest( + &self, + dirpath: &Path, + beacon: &CardanoDbBeacon, + ) -> Result { + if self.is_success { + Ok(self.digest.read().await.clone()) + } else { + Err(ImmutableDigesterError::NotEnoughImmutable { + expected_number: beacon.immutable_file_number, + found_number: None, + db_dir: dirpath.to_owned(), + }) + } + } + + async fn compute_merkle_tree( + &self, + dirpath: &Path, + beacon: &CardanoDbBeacon, + ) -> Result, ImmutableDigesterError> { + if self.is_success { + let leaves = self.mktree_leaves.read().await; + Ok(MKTree::new(&leaves).unwrap()) + } else { + Err(ImmutableDigesterError::NotEnoughImmutable { + expected_number: beacon.immutable_file_number, + found_number: None, + db_dir: dirpath.to_owned(), + }) + } + } +} diff --git a/mithril-common/src/digesters/dumb_immutable_observer.rs b/mithril-common/src/digesters/dumb_immutable_observer.rs deleted file mode 100644 index d3d8e4dd545..00000000000 --- a/mithril-common/src/digesters/dumb_immutable_observer.rs +++ /dev/null @@ -1,63 +0,0 @@ -use std::path::Path; - -use crate::{ - crypto_helper::{MKTree, MKTreeStoreInMemory}, - digesters::{ImmutableDigester, ImmutableDigesterError}, - entities::CardanoDbBeacon, -}; -use async_trait::async_trait; -use tokio::sync::RwLock; - -/// A [ImmutableDigester] returning configurable result for testing purpose. -pub struct DumbImmutableDigester { - digest: RwLock, - is_success: bool, -} - -impl DumbImmutableDigester { - /// DumbDigester factory - pub fn new(digest: &str, is_success: bool) -> Self { - let digest = RwLock::new(String::from(digest)); - - Self { digest, is_success } - } - - /// Update digest returned by [compute_digest][DumbImmutableDigester::compute_digest] - pub async fn update_digest(&self, new_digest: String) { - let mut digest = self.digest.write().await; - *digest = new_digest; - } -} - -impl Default for DumbImmutableDigester { - fn default() -> Self { - Self::new("1234", true) - } -} - -#[async_trait] -impl ImmutableDigester for DumbImmutableDigester { - async fn compute_digest( - &self, - dirpath: &Path, - beacon: &CardanoDbBeacon, - ) -> Result { - if self.is_success { - Ok(self.digest.read().await.clone()) - } else { - Err(ImmutableDigesterError::NotEnoughImmutable { - expected_number: beacon.immutable_file_number, - found_number: None, - db_dir: dirpath.to_owned(), - }) - } - } - - async fn compute_merkle_tree( - &self, - _dirpath: &Path, - _beacon: &CardanoDbBeacon, - ) -> Result, ImmutableDigesterError> { - unimplemented!() - } -} diff --git a/mithril-common/src/digesters/mod.rs b/mithril-common/src/digesters/mod.rs index c247ee37e76..415663341a0 100644 --- a/mithril-common/src/digesters/mod.rs +++ b/mithril-common/src/digesters/mod.rs @@ -2,7 +2,7 @@ pub mod cache; mod cardano_immutable_digester; -mod dumb_immutable_observer; +mod dumb_immutable_digester; mod immutable_digester; mod immutable_file; mod immutable_file_observer; @@ -15,7 +15,7 @@ pub use immutable_file_observer::{ ImmutableFileSystemObserver, }; -pub use dumb_immutable_observer::DumbImmutableDigester; +pub use dumb_immutable_digester::DumbImmutableDigester; cfg_test_tools! { mod dummy_immutable_db_builder; diff --git a/mithril-common/src/signable_builder/cardano_database.rs b/mithril-common/src/signable_builder/cardano_database.rs index bb36a005f1e..0b476c60f29 100644 --- a/mithril-common/src/signable_builder/cardano_database.rs +++ b/mithril-common/src/signable_builder/cardano_database.rs @@ -72,47 +72,17 @@ mod tests { use crate::{ crypto_helper::{MKTree, MKTreeStoreInMemory}, - digesters::ImmutableDigesterError, + digesters::DumbImmutableDigester, entities::{CardanoDbBeacon, ProtocolMessagePartKey}, test_utils::TestLogger, }; use super::*; - #[derive(Default)] - pub struct ImmutableDigesterImpl { - digests: Vec, - } - - impl ImmutableDigesterImpl { - pub fn new(digests: Vec) -> Self { - Self { digests } - } - } - - #[async_trait] - impl ImmutableDigester for ImmutableDigesterImpl { - async fn compute_digest( - &self, - _dirpath: &Path, - _beacon: &CardanoDbBeacon, - ) -> Result { - Ok("whatever".to_string()) - } - - async fn compute_merkle_tree( - &self, - _dirpath: &Path, - _beacon: &CardanoDbBeacon, - ) -> Result, ImmutableDigesterError> { - Ok(MKTree::new(&self.digests).unwrap()) - } - } - #[tokio::test] async fn compute_signable() { let digests = vec!["digest-1".to_string(), "digest-2".to_string()]; - let digester = ImmutableDigesterImpl::new(digests.clone()); + let digester = DumbImmutableDigester::default().with_merkle_tree(digests.clone()); let signable_builder = CardanoDatabaseSignableBuilder::new( Arc::new(digester), Path::new(""), diff --git a/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs b/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs index 26f048c29f6..b9f8a0f34f1 100644 --- a/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs +++ b/mithril-common/src/signable_builder/cardano_immutable_full_signable_builder.rs @@ -62,41 +62,17 @@ impl SignableBuilder for CardanoImmutableFilesFullSignableBuild #[cfg(test)] mod tests { - use async_trait::async_trait; use std::path::Path; - use crate::crypto_helper::{MKTree, MKTreeStoreInMemory}; - use crate::digesters::{ImmutableDigester, ImmutableDigesterError}; + use crate::digesters::DumbImmutableDigester; use crate::entities::CardanoDbBeacon; use crate::test_utils::TestLogger; use super::*; - #[derive(Default)] - pub struct ImmutableDigesterImpl; - - #[async_trait] - impl ImmutableDigester for ImmutableDigesterImpl { - async fn compute_digest( - &self, - _dirpath: &Path, - beacon: &CardanoDbBeacon, - ) -> Result { - Ok(format!("immutable {}", beacon.immutable_file_number)) - } - - async fn compute_merkle_tree( - &self, - _dirpath: &Path, - _beacon: &CardanoDbBeacon, - ) -> Result, ImmutableDigesterError> { - unimplemented!() - } - } - #[tokio::test] async fn compute_signable() { - let digester = ImmutableDigesterImpl; + let digester = DumbImmutableDigester::default().with_digest("immutable 0"); let signable_builder = CardanoImmutableFilesFullSignableBuilder::new( Arc::new(digester), Path::new(""), diff --git a/mithril-signer/src/runtime/runner.rs b/mithril-signer/src/runtime/runner.rs index da21bbe71a4..360e7bb8ff2 100644 --- a/mithril-signer/src/runtime/runner.rs +++ b/mithril-signer/src/runtime/runner.rs @@ -423,7 +423,7 @@ mod tests { )); let api_version_provider = Arc::new(APIVersionProvider::new(era_checker.clone())); - let digester = Arc::new(DumbImmutableDigester::new(DIGESTER_RESULT, true)); + let digester = Arc::new(DumbImmutableDigester::default().with_digest(DIGESTER_RESULT)); let cardano_immutable_signable_builder = Arc::new(CardanoImmutableFilesFullSignableBuilder::new( digester.clone(), diff --git a/mithril-signer/tests/test_extensions/state_machine_tester.rs b/mithril-signer/tests/test_extensions/state_machine_tester.rs index 990d6f82dce..299126ec621 100644 --- a/mithril-signer/tests/test_extensions/state_machine_tester.rs +++ b/mithril-signer/tests/test_extensions/state_machine_tester.rs @@ -158,7 +158,7 @@ impl StateMachineTester { }, ticker_service.clone(), )); - let digester = Arc::new(DumbImmutableDigester::new("DIGEST", true)); + let digester = Arc::new(DumbImmutableDigester::default().with_digest("DIGEST")); let protocol_initializer_store = Arc::new(ProtocolInitializerRepository::new( sqlite_connection.clone(), config.store_retention_limit.map(|limit| limit as u64), From c27ba75ec298d4f64cb36c015a3d7d13d4484f98 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Fri, 29 Nov 2024 17:55:00 +0100 Subject: [PATCH 13/15] fix: prevent metrics increment on errors in `create_artifact` and log warnings Also introduce `SignedEntityServiceArtifactsDependencies` to reduce the number of parameters in `MithrilSignedEntityService` constructor. --- .../src/dependency_injection/builder.rs | 13 +- mithril-aggregator/src/runtime/runner.rs | 28 --- .../src/services/signed_entity.rs | 212 ++++++++++++++++-- 3 files changed, 205 insertions(+), 48 deletions(-) diff --git a/mithril-aggregator/src/dependency_injection/builder.rs b/mithril-aggregator/src/dependency_injection/builder.rs index 73c2786f1db..d95ecc8f5c0 100644 --- a/mithril-aggregator/src/dependency_injection/builder.rs +++ b/mithril-aggregator/src/dependency_injection/builder.rs @@ -69,7 +69,8 @@ use crate::{ CardanoTransactionsImporter, CertifierService, EpochServiceDependencies, MessageService, MithrilCertifierService, MithrilEpochService, MithrilMessageService, MithrilProverService, MithrilSignedEntityService, MithrilStakeDistributionService, ProverService, - SignedEntityService, StakeDistributionService, UpkeepService, UsageReporter, + SignedEntityService, SignedEntityServiceArtifactsDependencies, StakeDistributionService, + UpkeepService, UsageReporter, }, store::CertificatePendingStorer, tools::{CExplorerSignerRetriever, GcpFileUploader, GenesisToolsDependency, SignersImporter}, @@ -1207,13 +1208,17 @@ impl DependenciesBuilder { let stake_store = self.get_stake_store().await?; let cardano_stake_distribution_artifact_builder = Arc::new(CardanoStakeDistributionArtifactBuilder::new(stake_store)); - let signed_entity_service = Arc::new(MithrilSignedEntityService::new( - signed_entity_storer, + let dependencies = SignedEntityServiceArtifactsDependencies::new( mithril_stake_distribution_artifact_builder, cardano_immutable_files_full_artifact_builder, cardano_transactions_artifact_builder, - self.get_signed_entity_lock().await?, cardano_stake_distribution_artifact_builder, + ); + let signed_entity_service = Arc::new(MithrilSignedEntityService::new( + signed_entity_storer, + dependencies, + self.get_signed_entity_lock().await?, + self.get_metrics_service().await?, logger, )); diff --git a/mithril-aggregator/src/runtime/runner.rs b/mithril-aggregator/src/runtime/runner.rs index a45215d8abb..4092f05c27f 100644 --- a/mithril-aggregator/src/runtime/runner.rs +++ b/mithril-aggregator/src/runtime/runner.rs @@ -172,32 +172,6 @@ impl AggregatorRunner { Ok(unlocked_signed_entities) } - - fn increment_artifact_total_produced_metric_since_startup( - &self, - signed_entity_type: &SignedEntityType, - ) { - let metrics = self.dependencies.metrics_service.clone(); - let metric_counter = match signed_entity_type { - SignedEntityType::MithrilStakeDistribution(_) => { - metrics.get_artifact_mithril_stake_distribution_total_produced_since_startup() - } - SignedEntityType::CardanoImmutableFilesFull(_) => { - metrics.get_artifact_cardano_db_total_produced_since_startup() - } - SignedEntityType::CardanoStakeDistribution(_) => { - metrics.get_artifact_cardano_stake_distribution_total_produced_since_startup() - } - SignedEntityType::CardanoTransactions(_, _) => { - metrics.get_artifact_cardano_transaction_total_produced_since_startup() - } - SignedEntityType::CardanoDatabase(_) => { - metrics.get_artifact_cardano_database_total_produced_since_startup() - } - }; - - metric_counter.increment(); - } } #[cfg_attr(test, mockall::automock)] @@ -457,8 +431,6 @@ impl AggregatorRunnerTrait for AggregatorRunner { ) })?; - self.increment_artifact_total_produced_metric_since_startup(signed_entity_type); - Ok(()) } diff --git a/mithril-aggregator/src/services/signed_entity.rs b/mithril-aggregator/src/services/signed_entity.rs index 5f5232fca54..57b41565f05 100644 --- a/mithril-aggregator/src/services/signed_entity.rs +++ b/mithril-aggregator/src/services/signed_entity.rs @@ -5,7 +5,7 @@ use anyhow::{anyhow, Context}; use async_trait::async_trait; use chrono::Utc; -use slog::{info, Logger}; +use slog::{info, warn, Logger}; use std::sync::Arc; use tokio::task::JoinHandle; @@ -24,6 +24,7 @@ use mithril_common::{ use crate::{ artifact_builder::ArtifactBuilder, database::{record::SignedEntityRecord, repository::SignedEntityStorer}, + MetricsService, }; /// ArtifactBuilder Service trait @@ -88,13 +89,25 @@ pub struct MithrilSignedEntityService { signed_entity_type_lock: Arc, cardano_stake_distribution_artifact_builder: Arc>, + metrics_service: Arc, logger: Logger, } -impl MithrilSignedEntityService { - /// MithrilSignedEntityService factory +/// ArtifactsBuilder dependencies required by the [MithrilSignedEntityService]. +pub struct SignedEntityServiceArtifactsDependencies { + mithril_stake_distribution_artifact_builder: + Arc>, + cardano_immutable_files_full_artifact_builder: + Arc>, + cardano_transactions_artifact_builder: + Arc>, + cardano_stake_distribution_artifact_builder: + Arc>, +} + +impl SignedEntityServiceArtifactsDependencies { + /// Create a new instance of [SignedEntityServiceArtifactsDependencies]. pub fn new( - signed_entity_storer: Arc, mithril_stake_distribution_artifact_builder: Arc< dyn ArtifactBuilder, >, @@ -104,19 +117,40 @@ impl MithrilSignedEntityService { cardano_transactions_artifact_builder: Arc< dyn ArtifactBuilder, >, - signed_entity_type_lock: Arc, cardano_stake_distribution_artifact_builder: Arc< dyn ArtifactBuilder, >, - logger: Logger, ) -> Self { Self { - signed_entity_storer, mithril_stake_distribution_artifact_builder, cardano_immutable_files_full_artifact_builder, cardano_transactions_artifact_builder, - signed_entity_type_lock, cardano_stake_distribution_artifact_builder, + } + } +} + +impl MithrilSignedEntityService { + /// MithrilSignedEntityService factory + pub fn new( + signed_entity_storer: Arc, + dependencies: SignedEntityServiceArtifactsDependencies, + signed_entity_type_lock: Arc, + metrics_service: Arc, + logger: Logger, + ) -> Self { + Self { + signed_entity_storer, + mithril_stake_distribution_artifact_builder: dependencies + .mithril_stake_distribution_artifact_builder, + cardano_immutable_files_full_artifact_builder: dependencies + .cardano_immutable_files_full_artifact_builder, + cardano_transactions_artifact_builder: dependencies + .cardano_transactions_artifact_builder, + cardano_stake_distribution_artifact_builder: dependencies + .cardano_stake_distribution_artifact_builder, + signed_entity_type_lock, + metrics_service, logger: logger.new_with_component_name::(), } } @@ -161,6 +195,9 @@ impl MithrilSignedEntityService { "Signed Entity Service can not store signed entity with type: '{signed_entity_type}'" ) })?; + + self.increment_artifact_total_produced_metric_since_startup(signed_entity_type); + Ok(()) } @@ -233,6 +270,32 @@ impl MithrilSignedEntityService { ) }) } + + fn increment_artifact_total_produced_metric_since_startup( + &self, + signed_entity_type: SignedEntityType, + ) { + let metrics = self.metrics_service.clone(); + let metric_counter = match signed_entity_type { + SignedEntityType::MithrilStakeDistribution(_) => { + metrics.get_artifact_mithril_stake_distribution_total_produced_since_startup() + } + SignedEntityType::CardanoImmutableFilesFull(_) => { + metrics.get_artifact_cardano_db_total_produced_since_startup() + } + SignedEntityType::CardanoStakeDistribution(_) => { + metrics.get_artifact_cardano_stake_distribution_total_produced_since_startup() + } + SignedEntityType::CardanoTransactions(_, _) => { + metrics.get_artifact_cardano_transaction_total_produced_since_startup() + } + SignedEntityType::CardanoDatabase(_) => { + metrics.get_artifact_cardano_database_total_produced_since_startup() + } + }; + + metric_counter.increment(); + } } #[async_trait] @@ -276,7 +339,7 @@ impl SignedEntityService for MithrilSignedEntityService { result.with_context(|| format!( "Signed Entity Service can not store signed entity with type: '{signed_entity_type}'" - ))? + ))?.inspect_err(|e| warn!(service.logger, "Error while creating artifact"; "error" => ?e)) })) } @@ -400,6 +463,7 @@ mod tests { signable_builder, test_utils::fake_data, }; + use mithril_metric::CounterValue; use serde::{de::DeserializeOwned, Serialize}; use std::sync::atomic::AtomicBool; @@ -472,13 +536,17 @@ mod tests { } fn build_artifact_builder_service(self) -> MithrilSignedEntityService { - MithrilSignedEntityService::new( - Arc::new(self.mock_signed_entity_storer), + let dependencies = SignedEntityServiceArtifactsDependencies::new( Arc::new(self.mock_mithril_stake_distribution_artifact_builder), Arc::new(self.mock_cardano_immutable_files_full_artifact_builder), Arc::new(self.mock_cardano_transactions_artifact_builder), - Arc::new(SignedEntityTypeLock::default()), Arc::new(self.mock_cardano_stake_distribution_artifact_builder), + ); + MithrilSignedEntityService::new( + Arc::new(self.mock_signed_entity_storer), + dependencies, + Arc::new(SignedEntityTypeLock::default()), + Arc::new(MetricsService::new(TestLogger::stdout()).unwrap()), TestLogger::stdout(), ) } @@ -524,13 +592,17 @@ mod tests { .withf(move |signed_entity| signed_entity.artifact == signed_entity_artifact) .return_once(|_| Ok(())); - MithrilSignedEntityService::new( - Arc::new(self.mock_signed_entity_storer), + let dependencies = SignedEntityServiceArtifactsDependencies::new( Arc::new(self.mock_mithril_stake_distribution_artifact_builder), Arc::new(cardano_immutable_files_full_long_artifact_builder), Arc::new(self.mock_cardano_transactions_artifact_builder), - Arc::new(SignedEntityTypeLock::default()), Arc::new(self.mock_cardano_stake_distribution_artifact_builder), + ); + MithrilSignedEntityService::new( + Arc::new(self.mock_signed_entity_storer), + dependencies, + Arc::new(SignedEntityTypeLock::default()), + Arc::new(MetricsService::new(TestLogger::stdout()).unwrap()), TestLogger::stdout(), ) } @@ -569,6 +641,29 @@ mod tests { } } + fn get_artifact_total_produced_metric_since_startup_counter_value( + metrics_service: Arc, + signed_entity_type: &SignedEntityType, + ) -> CounterValue { + match signed_entity_type { + SignedEntityType::MithrilStakeDistribution(_) => metrics_service + .get_artifact_mithril_stake_distribution_total_produced_since_startup() + .get(), + SignedEntityType::CardanoImmutableFilesFull(_) => metrics_service + .get_artifact_cardano_db_total_produced_since_startup() + .get(), + SignedEntityType::CardanoStakeDistribution(_) => metrics_service + .get_artifact_cardano_stake_distribution_total_produced_since_startup() + .get(), + SignedEntityType::CardanoTransactions(_, _) => metrics_service + .get_artifact_cardano_transaction_total_produced_since_startup() + .get(), + SignedEntityType::CardanoDatabase(_) => metrics_service + .get_artifact_cardano_database_total_produced_since_startup() + .get(), + } + } + #[tokio::test] async fn build_mithril_stake_distribution_artifact_when_given_mithril_stake_distribution_entity_type( ) { @@ -776,10 +871,23 @@ mod tests { ); let error_message_str = error_message.as_str(); + let initial_counter_value = get_artifact_total_produced_metric_since_startup_counter_value( + artifact_builder_service.metrics_service.clone(), + &signed_entity_type, + ); + artifact_builder_service - .create_artifact_task(signed_entity_type, &certificate) + .create_artifact_task(signed_entity_type.clone(), &certificate) .await .expect(error_message_str); + + assert_eq!( + initial_counter_value + 1, + get_artifact_total_produced_metric_since_startup_counter_value( + artifact_builder_service.metrics_service.clone(), + &signed_entity_type, + ) + ) } #[tokio::test] @@ -943,4 +1051,76 @@ mod tests { atomic_stop.swap(true, Ordering::Relaxed); } + + #[tokio::test] + async fn metrics_counter_value_is_not_incremented_when_compute_artifact_error() { + let signed_entity_service = { + let mut mock_container = MockDependencyInjector::new(); + mock_container + .mock_cardano_immutable_files_full_artifact_builder + .expect_compute_artifact() + .returning(|_, _| Err(anyhow!("Error while computing artifact"))); + + mock_container.build_artifact_builder_service() + }; + + let signed_entity_type = SignedEntityType::MithrilStakeDistribution(Epoch(7)); + + let initial_counter_value = get_artifact_total_produced_metric_since_startup_counter_value( + signed_entity_service.metrics_service.clone(), + &signed_entity_type, + ); + + signed_entity_service + .create_artifact( + signed_entity_type.clone(), + &fake_data::certificate("hash".to_string()), + ) + .await + .unwrap(); + + assert_eq!( + initial_counter_value, + get_artifact_total_produced_metric_since_startup_counter_value( + signed_entity_service.metrics_service.clone(), + &signed_entity_type, + ) + ); + } + + #[tokio::test] + async fn metrics_counter_value_is_not_incremented_when_store_signed_entity_error() { + let signed_entity_service = { + let mut mock_container = MockDependencyInjector::new(); + mock_container + .mock_signed_entity_storer + .expect_store_signed_entity() + .returning(|_| Err(anyhow!("Error while storing signed entity"))); + + mock_container.build_artifact_builder_service() + }; + + let signed_entity_type = SignedEntityType::MithrilStakeDistribution(Epoch(7)); + + let initial_counter_value = get_artifact_total_produced_metric_since_startup_counter_value( + signed_entity_service.metrics_service.clone(), + &signed_entity_type, + ); + + signed_entity_service + .create_artifact( + signed_entity_type.clone(), + &fake_data::certificate("hash".to_string()), + ) + .await + .unwrap(); + + assert_eq!( + initial_counter_value, + get_artifact_total_produced_metric_since_startup_counter_value( + signed_entity_service.metrics_service.clone(), + &signed_entity_type, + ) + ); + } } From 93d3d36196e0de909b84bf7e161ae08d0cfcc325 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Fri, 29 Nov 2024 18:10:40 +0100 Subject: [PATCH 14/15] chore: reference the feature in the CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae5f5516b0b..c7a20a84a0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ As a minor extension, we have adopted a slightly different versioning convention - **UNSTABLE** Cardano database incremental certification: - Implement the new signed entity type `CardanoDatabase`. + - Implement the signable builder for the signed entity type `CardanoDatabase`. - Crates versions: From f9b6de06a8d6a45b992cb70b4da2b7a24e6bc205 Mon Sep 17 00:00:00 2001 From: Damien Lachaume <135982616+dlachaume@users.noreply.github.com> Date: Fri, 29 Nov 2024 18:11:24 +0100 Subject: [PATCH 15/15] chore: upgrade crate versions * mithril-aggregator from `0.5.117` to `0.5.118` * mithril-common from `0.4.91` to `0.4.92` * mithril-signer from `0.2.217` to `0.2.218` * mithril-end-to-end from `0.4.50` to `0.4.51` --- Cargo.lock | 8 ++++---- mithril-aggregator/Cargo.toml | 2 +- mithril-common/Cargo.toml | 2 +- mithril-signer/Cargo.toml | 2 +- mithril-test-lab/mithril-end-to-end/Cargo.toml | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 23a1f6a606e..1b71789df46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3588,7 +3588,7 @@ dependencies = [ [[package]] name = "mithril-aggregator" -version = "0.5.117" +version = "0.5.118" dependencies = [ "anyhow", "async-trait", @@ -3745,7 +3745,7 @@ dependencies = [ [[package]] name = "mithril-common" -version = "0.4.91" +version = "0.4.92" dependencies = [ "anyhow", "async-trait", @@ -3816,7 +3816,7 @@ dependencies = [ [[package]] name = "mithril-end-to-end" -version = "0.4.50" +version = "0.4.51" dependencies = [ "anyhow", "async-recursion", @@ -3903,7 +3903,7 @@ dependencies = [ [[package]] name = "mithril-signer" -version = "0.2.217" +version = "0.2.218" dependencies = [ "anyhow", "async-trait", diff --git a/mithril-aggregator/Cargo.toml b/mithril-aggregator/Cargo.toml index d0ba4bd719e..555e0f9b84f 100644 --- a/mithril-aggregator/Cargo.toml +++ b/mithril-aggregator/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-aggregator" -version = "0.5.117" +version = "0.5.118" description = "A Mithril Aggregator server" authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-common/Cargo.toml b/mithril-common/Cargo.toml index a7ef80fff1a..c74863fa44d 100644 --- a/mithril-common/Cargo.toml +++ b/mithril-common/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-common" -version = "0.4.91" +version = "0.4.92" description = "Common types, interfaces, and utilities for Mithril nodes." authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-signer/Cargo.toml b/mithril-signer/Cargo.toml index e1a34aaf81c..7e87033eabf 100644 --- a/mithril-signer/Cargo.toml +++ b/mithril-signer/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-signer" -version = "0.2.217" +version = "0.2.218" description = "A Mithril Signer" authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-test-lab/mithril-end-to-end/Cargo.toml b/mithril-test-lab/mithril-end-to-end/Cargo.toml index ec16dab542a..4de2e661384 100644 --- a/mithril-test-lab/mithril-end-to-end/Cargo.toml +++ b/mithril-test-lab/mithril-end-to-end/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-end-to-end" -version = "0.4.50" +version = "0.4.51" authors = { workspace = true } edition = { workspace = true } documentation = { workspace = true }