From 776f48a948df0af407c424bba65cbfaf2ace09ab Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Wed, 22 Mar 2023 12:37:14 +0100 Subject: [PATCH 1/7] Remove unused code from multi-signer --- mithril-aggregator/src/multi_signer.rs | 39 +------------------------- 1 file changed, 1 insertion(+), 38 deletions(-) diff --git a/mithril-aggregator/src/multi_signer.rs b/mithril-aggregator/src/multi_signer.rs index 9d37b0e60b9..1e07a40e16c 100644 --- a/mithril-aggregator/src/multi_signer.rs +++ b/mithril-aggregator/src/multi_signer.rs @@ -9,8 +9,7 @@ use mithril_common::{ crypto_helper::{ key_decode_hex, key_encode_hex, ProtocolAggregateVerificationKey, ProtocolAggregationError, ProtocolClerk, ProtocolKeyRegistration, ProtocolMultiSignature, ProtocolParameters, - ProtocolPartyId, ProtocolRegistrationError, ProtocolSignerVerificationKey, - ProtocolSingleSignature, ProtocolStakeDistribution, + ProtocolRegistrationError, ProtocolSingleSignature, ProtocolStakeDistribution, }, entities::{self, Epoch, SignerWithStake, StakeDistribution}, store::{StakeStorer, StoreError}, @@ -169,12 +168,6 @@ pub trait MultiSigner: Sync + Send { } } - /// Get signer - async fn get_signer_verification_key( - &self, - party_id: ProtocolPartyId, - ) -> Result, ProtocolError>; - /// Get signers async fn get_signers(&self) -> Result, ProtocolError> { debug!("Get signers"); @@ -224,9 +217,6 @@ pub struct MultiSignerImpl { /// Created multi signature for message signed multi_signature: Option, - /// Created aggregate verification key - avk: Option, - /// Verification key store verification_key_store: Arc, @@ -255,7 +245,6 @@ impl MultiSignerImpl { current_initiated_at: None, clerk: None, multi_signature: None, - avk: None, verification_key_store, stake_store, single_signature_store, @@ -513,31 +502,6 @@ impl MultiSigner for MultiSignerImpl { } } - /// Get signer verification key - async fn get_signer_verification_key( - &self, - party_id: ProtocolPartyId, - ) -> Result, ProtocolError> { - debug!("Get signer {}", party_id); - let epoch = self - .current_beacon - .as_ref() - .ok_or_else(ProtocolError::UnavailableBeacon)? - .epoch - .offset_to_signer_retrieval_epoch()?; - let signers = self - .verification_key_store - .get_verification_keys(epoch) - .await? - .unwrap_or_default(); - match signers.get(&party_id) { - Some(signer) => Ok(Some( - key_decode_hex(&signer.verification_key).map_err(ProtocolError::Codec)?, - )), - _ => Ok(None), - } - } - async fn get_signers_with_stake(&self) -> Result, ProtocolError> { debug!("Get signers with stake"); let epoch = self @@ -705,7 +669,6 @@ impl MultiSigner for MultiSignerImpl { .ok_or_else(ProtocolError::UnavailableClerk)?; match clerk.aggregate(&signatures, message.compute_hash().as_bytes()) { Ok(multi_signature) => { - self.avk = Some(clerk.compute_avk()); self.multi_signature = Some(multi_signature.clone()); Ok(Some(multi_signature)) } From 9086b7c96897a3d50bcc65cf0e17e70d99e756f3 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Wed, 22 Mar 2023 12:39:39 +0100 Subject: [PATCH 2/7] Remove clerk from multi-signer state --- mithril-aggregator/src/multi_signer.rs | 55 +++++++++----------------- 1 file changed, 18 insertions(+), 37 deletions(-) diff --git a/mithril-aggregator/src/multi_signer.rs b/mithril-aggregator/src/multi_signer.rs index 1e07a40e16c..221ac978e22 100644 --- a/mithril-aggregator/src/multi_signer.rs +++ b/mithril-aggregator/src/multi_signer.rs @@ -1,7 +1,7 @@ use async_trait::async_trait; use chrono::prelude::*; use hex::ToHex; -use slog_scope::{debug, trace, warn}; +use slog_scope::{debug, warn}; use std::sync::Arc; use thiserror::Error; @@ -211,9 +211,6 @@ pub struct MultiSignerImpl { /// Signing start datetime of current message current_initiated_at: Option>, - /// Clerk used for verifying the single signatures - clerk: Option, - /// Created multi signature for message signed multi_signature: Option, @@ -243,7 +240,6 @@ impl MultiSignerImpl { current_message: None, current_beacon: None, current_initiated_at: None, - clerk: None, multi_signature: None, verification_key_store, stake_store, @@ -354,31 +350,6 @@ impl MultiSigner for MultiSignerImpl { debug!("Update current_message"; "protocol_message" => #?message, "signed message" => message.compute_hash().encode_hex::()); self.multi_signature = None; - let signers_with_stake = self.get_signers_with_stake().await?; - let protocol_parameters = self - .get_protocol_parameters() - .await? - .ok_or_else(ProtocolError::UnavailableProtocolParameters)?; - match self - .create_clerk(&signers_with_stake, &protocol_parameters) - .await - { - Ok(Some(clerk)) => { - trace!("update_current_message::new_clerk_created"); - self.clerk = Some(clerk) - } - Ok(None) => { - warn!( - "update_current_message::no_clerk_created: probably not enough signers with valid verification keys"; - "signers" => ?signers_with_stake - ); - self.clerk = None - } - Err(ProtocolError::Beacon(err)) => { - warn!("update_current_message::error"; "error" => ?err); - } - Err(e) => return Err(e), - } self.current_initiated_at = Some(Utc::now()); self.current_message = Some(message); Ok(()) @@ -585,15 +556,17 @@ impl MultiSigner for MultiSignerImpl { .await? .ok_or_else(ProtocolError::UnavailableProtocolParameters)?; - let clerk = self - .clerk - .as_ref() - .ok_or_else(ProtocolError::UnavailableClerk)?; - let signature = signatures .to_protocol_signature() .map_err(ProtocolError::Codec)?; + let signers_with_stakes = self.get_signers_with_stake().await?; + + let clerk = self + .create_clerk(&signers_with_stakes, &protocol_parameters) + .await? + .ok_or_else(ProtocolError::UnavailableClerk)?; + let avk = clerk.compute_avk(); // If there is no reg_party, then we simply received a signature from a non-registered @@ -649,6 +622,11 @@ impl MultiSigner for MultiSignerImpl { .current_beacon .as_ref() .ok_or_else(ProtocolError::UnavailableBeacon)?; + let protocol_parameters = self + .get_protocol_parameters() + .await? + .ok_or_else(ProtocolError::UnavailableProtocolParameters)?; + let signatures: Vec = self .single_signature_store .get_single_signatures(beacon) @@ -663,10 +641,13 @@ impl MultiSigner for MultiSignerImpl { }) .collect::>(); + let signers_with_stakes = self.get_signers_with_stake().await?; + let clerk = self - .clerk - .as_ref() + .create_clerk(&signers_with_stakes, &protocol_parameters) + .await? .ok_or_else(ProtocolError::UnavailableClerk)?; + match clerk.aggregate(&signatures, message.compute_hash().as_bytes()) { Ok(multi_signature) => { self.multi_signature = Some(multi_signature.clone()); From fed370b00b08e736cf2bde6d3b82319378cfc115 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Thu, 23 Mar 2023 11:41:54 +0100 Subject: [PATCH 3/7] Remove multi-signature from multi-signer state --- mithril-aggregator/src/multi_signer.rs | 43 +++------------- mithril-aggregator/src/runtime/runner.rs | 47 +++++++----------- .../src/runtime/state_machine.rs | 49 +++++++++++++------ mithril-aggregator/tests/certificate_chain.rs | 2 +- .../tests/create_certificate.rs | 2 +- .../tests/test_extensions/runtime_tester.rs | 11 +++++ 6 files changed, 72 insertions(+), 82 deletions(-) diff --git a/mithril-aggregator/src/multi_signer.rs b/mithril-aggregator/src/multi_signer.rs index 221ac978e22..670859bb48b 100644 --- a/mithril-aggregator/src/multi_signer.rs +++ b/mithril-aggregator/src/multi_signer.rs @@ -191,9 +191,6 @@ pub trait MultiSigner: Sync + Send { signatures: &entities::SingleSignatures, ) -> Result<(), ProtocolError>; - /// Retrieves a multi signature from a message - async fn get_multi_signature(&self) -> Result, ProtocolError>; - /// Creates a multi signature from single signatures async fn create_multi_signature( &mut self, @@ -211,9 +208,6 @@ pub struct MultiSignerImpl { /// Signing start datetime of current message current_initiated_at: Option>, - /// Created multi signature for message signed - multi_signature: Option, - /// Verification key store verification_key_store: Arc, @@ -240,7 +234,6 @@ impl MultiSignerImpl { current_message: None, current_beacon: None, current_initiated_at: None, - multi_signature: None, verification_key_store, stake_store, single_signature_store, @@ -349,7 +342,6 @@ impl MultiSigner for MultiSignerImpl { ) -> Result<(), ProtocolError> { debug!("Update current_message"; "protocol_message" => #?message, "signed message" => message.compute_hash().encode_hex::()); - self.multi_signature = None; self.current_initiated_at = Some(Utc::now()); self.current_message = Some(message); Ok(()) @@ -602,12 +594,6 @@ impl MultiSigner for MultiSignerImpl { }; } - /// Retrieves a multi signature from a message - async fn get_multi_signature(&self) -> Result, ProtocolError> { - debug!("Get multi signature"); - Ok(self.multi_signature.to_owned()) - } - /// Creates a multi signature from single signatures async fn create_multi_signature( &mut self, @@ -649,10 +635,7 @@ impl MultiSigner for MultiSignerImpl { .ok_or_else(ProtocolError::UnavailableClerk)?; match clerk.aggregate(&signatures, message.compute_hash().as_bytes()) { - Ok(multi_signature) => { - self.multi_signature = Some(multi_signature.clone()); - Ok(Some(multi_signature)) - } + Ok(multi_signature) => Ok(Some(multi_signature)), Err(ProtocolAggregationError::NotEnoughSignatures(actual, expected)) => { warn!("Could not compute multi-signature: Not enough signatures. Got only {} out of {}.", actual, expected); Ok(None) @@ -916,14 +899,10 @@ mod tests { ); // No signatures registered: multi-signer can't create the multi-signature - multi_signer - .create_multi_signature() - .await - .expect("create multi signature should not fail"); assert!(multi_signer - .get_multi_signature() + .create_multi_signature() .await - .expect("get multi signature should not fail") + .expect("create multi signature should not fail") .is_none()); // Add some signatures but not enough to reach the quorum: multi-signer should not create the multi-signature @@ -933,14 +912,10 @@ mod tests { .await .expect("register single signature should not fail"); } - multi_signer - .create_multi_signature() - .await - .expect("create multi signature should not fail"); assert!(multi_signer - .get_multi_signature() + .create_multi_signature() .await - .expect("get multi signature should not fail") + .expect("create multi signature should not fail") .is_none()); // Add the remaining signatures to reach the quorum: multi-signer should create a multi-signature @@ -950,15 +925,11 @@ mod tests { .await .expect("register single signature should not fail"); } - multi_signer - .create_multi_signature() - .await - .expect("create multi signature should not fail"); assert!( multi_signer - .get_multi_signature() + .create_multi_signature() .await - .expect("get multi signature should not fail") + .expect("create multi signature should not fail") .is_some(), "no multi-signature were computed" ); diff --git a/mithril-aggregator/src/runtime/runner.rs b/mithril-aggregator/src/runtime/runner.rs index d229cb5178f..58cb0297f32 100644 --- a/mithril-aggregator/src/runtime/runner.rs +++ b/mithril-aggregator/src/runtime/runner.rs @@ -1,9 +1,10 @@ use async_trait::async_trait; use chrono::Utc; +use mithril_common::crypto_helper::ProtocolMultiSignature; use mithril_common::entities::Epoch; use mithril_common::entities::PartyId; use mithril_common::store::StakeStorer; -use slog_scope::{debug, info, warn}; +use slog_scope::{debug, warn}; use mithril_common::crypto_helper::ProtocolStakeDistribution; use mithril_common::entities::{ @@ -134,8 +135,10 @@ pub trait AggregatorRunnerTrait: Sync + Send { &self, ) -> Result, Box>; - /// Check if the multisigner has issued a multi-signature. - async fn is_multisig_created(&self) -> Result>; + /// Create multi-signature. + async fn create_multi_signature( + &self, + ) -> Result, Box>; /// Create an archive of the cardano node db directory naming it after the given beacon. /// @@ -157,6 +160,7 @@ pub trait AggregatorRunnerTrait: Sync + Send { async fn create_and_save_certificate( &self, working_certificate: &WorkingCertificate, + multi_signature: ProtocolMultiSignature, ) -> Result>; /// Create a snapshot and save it to the given locations. @@ -546,25 +550,18 @@ impl AggregatorRunnerTrait for AggregatorRunner { Ok(certificate_pending) } - /// Is a multi-signature ready? - /// Can we create a multi-signature. - async fn is_multisig_created(&self) -> Result> { - debug!("RUNNER: check if we can create a multi-signature"); - let has_multisig = self + async fn create_multi_signature( + &self, + ) -> Result, Box> { + debug!("RUNNER: create multi-signature"); + + Ok(self .dependencies .multi_signer .write() .await .create_multi_signature() - .await? - .is_some(); - - if has_multisig { - debug!(" > new multi-signature created"); - } else { - info!(" > no multi-signature created"); - } - Ok(has_multisig) + .await?) } async fn create_snapshot_archive( @@ -633,10 +630,10 @@ impl AggregatorRunnerTrait for AggregatorRunner { async fn create_and_save_certificate( &self, working_certificate: &WorkingCertificate, + multi_signature: ProtocolMultiSignature, ) -> Result> { debug!("RUNNER: create and save certificate"); let certificate_store = self.dependencies.certificate_store.clone(); - let multisigner = self.dependencies.multi_signer.read().await; let signatures_party_ids: Vec = self .dependencies .single_signature_store @@ -645,12 +642,6 @@ impl AggregatorRunnerTrait for AggregatorRunner { .unwrap_or_default() .into_keys() .collect::>(); - let multi_signature = multisigner.get_multi_signature().await?.ok_or_else(|| { - RunnerError::NoComputedMultiSignature(format!( - "no multi signature generated for beacon {:?}", - working_certificate.beacon - )) - })?; let certificate = MithrilCertificateCreator::create_certificate( working_certificate, @@ -1183,6 +1174,7 @@ pub mod tests { let first_certificate = certificate_chain[0].clone(); let multi_signature: ProtocolMultiSignature = key_decode_hex(&first_certificate.multi_signature as &HexEncodedKey).unwrap(); + let multi_signature_clone = multi_signature.clone(); let working_certificate = WorkingCertificate { beacon: first_certificate.beacon.clone(), signers: first_certificate.metadata.signers.clone(), @@ -1193,16 +1185,13 @@ pub mod tests { ..WorkingCertificate::fake() }; let (mut deps, config) = initialize_dependencies().await; - let mut mock_multi_signer = MockMultiSigner::new(); - mock_multi_signer - .expect_get_multi_signature() - .return_once(move || Ok(Some(multi_signature))); + let mock_multi_signer = MockMultiSigner::new(); deps.multi_signer = Arc::new(RwLock::new(mock_multi_signer)); deps.init_state_from_chain(&certificate_chain, vec![]).await; let runner = AggregatorRunner::new(config, Arc::new(deps)); let certificate = runner - .create_and_save_certificate(&working_certificate) + .create_and_save_certificate(&working_certificate, multi_signature_clone) .await; certificate.expect("a certificate should have been created and saved"); } diff --git a/mithril-aggregator/src/runtime/state_machine.rs b/mithril-aggregator/src/runtime/state_machine.rs index b2d8316aa03..d4295e4d6e3 100644 --- a/mithril-aggregator/src/runtime/state_machine.rs +++ b/mithril-aggregator/src/runtime/state_machine.rs @@ -215,14 +215,12 @@ impl AggregatorRuntime { .transition_from_signing_to_idle_new_beacon(state) .await?; self.state = AggregatorState::Idle(new_state); - } else if self.runner.is_multisig_created().await? { - info!("→ a multi-signature have been created, build a snapshot & a certificate and transitioning back to IDLE"); - let new_state = self - .transition_from_signing_to_idle_multisignature(state) - .await?; - self.state = AggregatorState::Idle(new_state); } else { - info!(" ⋅ not enough signature yet to aggregate a multi-signature, waiting…"); + let new_state = self + .transition_from_signing_to_idle_multisignature(state) + .await?; + info!("→ a multi-signature have been created, build a snapshot & a certificate and transitioning back to IDLE"); + self.state = AggregatorState::Idle(new_state); } } } @@ -270,6 +268,18 @@ impl AggregatorRuntime { state: SigningState, ) -> Result { trace!("launching transition from SIGNING to IDLE state"); + let multi_signature = self.runner.create_multi_signature().await?; + + let multi_signature = if multi_signature.is_none() { + return Err(RuntimeError::Critical { + message: "not enough signature yet to aggregate a multi-signature, waiting…" + .to_string(), + nested_error: None, + }); + } else { + multi_signature.unwrap() + }; + self.runner.drop_pending_certificate().await?; let ongoing_snapshot = self .runner @@ -281,7 +291,7 @@ impl AggregatorRuntime { .await?; let certificate = self .runner - .create_and_save_certificate(&state.working_certificate) + .create_and_save_certificate(&state.working_certificate, multi_signature) .await?; let _ = self .runner @@ -349,6 +359,9 @@ mod tests { use super::super::runner::MockAggregatorRunner; use super::*; + use mithril_common::crypto_helper::tests_setup::setup_certificate_chain; + use mithril_common::crypto_helper::{key_decode_hex, ProtocolMultiSignature}; + use mithril_common::entities::HexEncodedKey; use mithril_common::era::UnsupportedEraError; use mithril_common::test_utils::fake_data; use mockall::predicate; @@ -632,30 +645,36 @@ mod tests { .once() .returning(|| Ok(fake_data::beacon())); runner - .expect_is_multisig_created() + .expect_create_multi_signature() .once() - .returning(|| Ok(false)); + .returning(|| Ok(None)); let state = SigningState { current_beacon: fake_data::beacon(), working_certificate: WorkingCertificate::fake(), }; let mut runtime = init_runtime(Some(AggregatorState::Signing(state)), runner).await; - runtime.cycle().await.unwrap(); + runtime + .cycle() + .await + .expect_err("cycle should have returned an error"); assert_eq!("signing".to_string(), runtime.get_state()); } #[tokio::test] async fn signing_multisig_is_created() { + let (certificate_chain, _) = setup_certificate_chain(5, 1); + let first_certificate = certificate_chain[0].clone(); + let multi_signature: ProtocolMultiSignature = + key_decode_hex(&first_certificate.multi_signature as &HexEncodedKey).unwrap(); let mut runner = MockAggregatorRunner::new(); runner .expect_get_beacon_from_chain() .once() .returning(|| Ok(fake_data::beacon())); runner - .expect_is_multisig_created() - .once() - .returning(|| Ok(true)); + .expect_create_multi_signature() + .return_once(move || Ok(Some(multi_signature))); runner .expect_drop_pending_certificate() .once() @@ -676,7 +695,7 @@ mod tests { runner .expect_create_and_save_certificate() .once() - .returning(|_| Ok(fake_data::certificate("whatever".to_string()))); + .returning(|_, _| Ok(fake_data::certificate("whatever".to_string()))); runner .expect_create_and_save_snapshot() .once() diff --git a/mithril-aggregator/tests/certificate_chain.rs b/mithril-aggregator/tests/certificate_chain.rs index b752c6316a2..fe4c1ddf070 100644 --- a/mithril-aggregator/tests/certificate_chain.rs +++ b/mithril-aggregator/tests/certificate_chain.rs @@ -51,7 +51,7 @@ async fn certificate_chain() { cycle!(tester, "ready"); cycle!(tester, "signing"); tester.register_signers(&signers).await.unwrap(); - cycle!(tester, "signing"); + cycle_err!(tester, "signing"); tester.send_single_signatures(&signers).await.unwrap(); comment!("The state machine should have issued a multisignature"); diff --git a/mithril-aggregator/tests/create_certificate.rs b/mithril-aggregator/tests/create_certificate.rs index 240df2b9674..854c3e53428 100644 --- a/mithril-aggregator/tests/create_certificate.rs +++ b/mithril-aggregator/tests/create_certificate.rs @@ -49,7 +49,7 @@ async fn create_certificate() { comment!("register signers"); tester.register_signers(&signers).await.unwrap(); - cycle!(tester, "signing"); + cycle_err!(tester, "signing"); comment!("change the immutable number to alter the beacon"); tester.increase_immutable_number().await.unwrap(); diff --git a/mithril-aggregator/tests/test_extensions/runtime_tester.rs b/mithril-aggregator/tests/test_extensions/runtime_tester.rs index c27fcaee215..5fa19a7e795 100644 --- a/mithril-aggregator/tests/test_extensions/runtime_tester.rs +++ b/mithril-aggregator/tests/test_extensions/runtime_tester.rs @@ -31,6 +31,17 @@ macro_rules! cycle { }}; } +#[macro_export] +macro_rules! cycle_err { + ( $tester:expr, $expected_state:expr ) => {{ + $tester + .cycle() + .await + .expect_err("cycle tick shoudl have returned an error"); + assert_eq!($expected_state, $tester.runtime.get_state()); + }}; +} + pub struct RuntimeTester { pub snapshot_uploader: Arc, pub chain_observer: Arc, From 080c166465eb1b724bf3046da6b48d72dc910f55 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Thu, 23 Mar 2023 11:46:07 +0100 Subject: [PATCH 4/7] Remove initiated date from multi-signer state --- mithril-aggregator/src/multi_signer.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/mithril-aggregator/src/multi_signer.rs b/mithril-aggregator/src/multi_signer.rs index 670859bb48b..2a31b5f58c0 100644 --- a/mithril-aggregator/src/multi_signer.rs +++ b/mithril-aggregator/src/multi_signer.rs @@ -1,5 +1,4 @@ use async_trait::async_trait; -use chrono::prelude::*; use hex::ToHex; use slog_scope::{debug, warn}; use std::sync::Arc; @@ -205,9 +204,6 @@ pub struct MultiSignerImpl { /// Beacon that is currently used current_beacon: Option, - /// Signing start datetime of current message - current_initiated_at: Option>, - /// Verification key store verification_key_store: Arc, @@ -233,7 +229,6 @@ impl MultiSignerImpl { Self { current_message: None, current_beacon: None, - current_initiated_at: None, verification_key_store, stake_store, single_signature_store, @@ -342,7 +337,6 @@ impl MultiSigner for MultiSignerImpl { ) -> Result<(), ProtocolError> { debug!("Update current_message"; "protocol_message" => #?message, "signed message" => message.compute_hash().encode_hex::()); - self.current_initiated_at = Some(Utc::now()); self.current_message = Some(message); Ok(()) } From 0b32e3f93f6cef164cb4b5f778abad0d6af341be Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Fri, 24 Mar 2023 11:04:28 +0100 Subject: [PATCH 5/7] Refactor register single signature And make it use a message argument. --- .../http_server/routes/signatures_routes.rs | 44 +++++++++++++------ mithril-aggregator/src/multi_signer.rs | 18 ++++---- .../tests/test_extensions/runtime_tester.rs | 6 +-- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/mithril-aggregator/src/http_server/routes/signatures_routes.rs b/mithril-aggregator/src/http_server/routes/signatures_routes.rs index 58e61f5025c..718c52c3fe3 100644 --- a/mithril-aggregator/src/http_server/routes/signatures_routes.rs +++ b/mithril-aggregator/src/http_server/routes/signatures_routes.rs @@ -26,6 +26,7 @@ mod handlers { use crate::{ dependency::MultiSignerWrapper, message_adapters::FromRegisterSingleSignatureAdapter, }; + use mithril_common::entities::SingleSignatures; use mithril_common::messages::RegisterSignatureMessage; use slog_scope::{debug, warn}; use std::convert::Infallible; @@ -37,10 +38,23 @@ mod handlers { multi_signer: MultiSignerWrapper, ) -> Result { debug!("⇄ HTTP SERVER: register_signatures/{:?}", message); - let signature = FromRegisterSingleSignatureAdapter::adapt(message); - let mut multi_signer = multi_signer.write().await; - match multi_signer.register_single_signature(&signature).await { + async fn register_single_signature( + multi_signer: MultiSignerWrapper, + signature: SingleSignatures, + ) -> Result<(), ProtocolError> { + let multi_signer = multi_signer.write().await; + let message = multi_signer + .get_current_message() + .await + .ok_or_else(ProtocolError::UnavailableMessage)?; + multi_signer + .register_single_signature(&message, &signature) + .await + } + + let signature = FromRegisterSingleSignatureAdapter::adapt(message); + match register_single_signature(multi_signer, signature).await { Err(ProtocolError::ExistingSingleSignature(party_id)) => { debug!("register_signatures::already_exist"; "party_id" => ?party_id); Ok(reply::empty(StatusCode::CONFLICT)) @@ -58,6 +72,7 @@ mod handlers { mod tests { use crate::http_server::SERVER_BASE_PATH; + use mithril_common::entities::ProtocolMessage; use mithril_common::messages::RegisterSignatureMessage; use mithril_common::test_utils::apispec::APISpec; use tokio::sync::RwLock; @@ -85,11 +100,11 @@ mod tests { async fn test_register_signatures_post_ok() { let mut mock_multi_signer = MockMultiSigner::new(); mock_multi_signer - .expect_update_current_message() - .return_once(|_| Ok(())); + .expect_get_current_message() + .return_once(|| Some(ProtocolMessage::new())); mock_multi_signer .expect_register_single_signature() - .return_once(|_| Ok(())); + .return_once(|_, _| Ok(())); let (mut dependency_manager, _) = initialize_dependencies().await; dependency_manager.multi_signer = Arc::new(RwLock::new(mock_multi_signer)); @@ -118,9 +133,12 @@ mod tests { #[tokio::test] async fn test_register_signatures_post_ko_400() { let mut mock_multi_signer = MockMultiSigner::new(); + mock_multi_signer + .expect_get_current_message() + .return_once(|| Some(ProtocolMessage::new())); mock_multi_signer .expect_register_single_signature() - .return_once(|_| Ok(())); + .return_once(|_, _| Ok(())); let (mut dependency_manager, _) = initialize_dependencies().await; dependency_manager.multi_signer = Arc::new(RwLock::new(mock_multi_signer)); @@ -153,11 +171,11 @@ mod tests { let party_id = message.party_id.clone(); let mut mock_multi_signer = MockMultiSigner::new(); mock_multi_signer - .expect_update_current_message() - .return_once(|_| Ok(())); + .expect_get_current_message() + .return_once(|| Some(ProtocolMessage::new())); mock_multi_signer .expect_register_single_signature() - .return_once(move |_| Err(ProtocolError::ExistingSingleSignature(party_id))); + .return_once(move |_, _| Err(ProtocolError::ExistingSingleSignature(party_id))); let (mut dependency_manager, _) = initialize_dependencies().await; dependency_manager.multi_signer = Arc::new(RwLock::new(mock_multi_signer)); @@ -185,11 +203,11 @@ mod tests { async fn test_register_signatures_post_ko_500() { let mut mock_multi_signer = MockMultiSigner::new(); mock_multi_signer - .expect_update_current_message() - .return_once(|_| Ok(())); + .expect_get_current_message() + .return_once(|| Some(ProtocolMessage::new())); mock_multi_signer .expect_register_single_signature() - .return_once(|_| Err(ProtocolError::Core("an error occurred".to_string()))); + .return_once(|_, _| Err(ProtocolError::Core("an error occurred".to_string()))); let (mut dependency_manager, _) = initialize_dependencies().await; dependency_manager.multi_signer = Arc::new(RwLock::new(mock_multi_signer)); diff --git a/mithril-aggregator/src/multi_signer.rs b/mithril-aggregator/src/multi_signer.rs index 2a31b5f58c0..07dbe259cbd 100644 --- a/mithril-aggregator/src/multi_signer.rs +++ b/mithril-aggregator/src/multi_signer.rs @@ -186,7 +186,8 @@ pub trait MultiSigner: Sync + Send { /// Registers a single signature async fn register_single_signature( - &mut self, + &self, + message: &entities::ProtocolMessage, signatures: &entities::SingleSignatures, ) -> Result<(), ProtocolError>; @@ -525,18 +526,15 @@ impl MultiSigner for MultiSignerImpl { /// Registers a single signature async fn register_single_signature( - &mut self, + &self, + message: &entities::ProtocolMessage, signatures: &entities::SingleSignatures, ) -> Result<(), ProtocolError> { debug!( - "Register single signature from {} at indexes {:?}", - signatures.party_id, signatures.won_indexes + "Register single signature from {} at indexes {:?} for message {:?}", + signatures.party_id, signatures.won_indexes, message ); - let message = &self - .get_current_message() - .await - .ok_or_else(ProtocolError::UnavailableMessage)?; let protocol_parameters = self .get_protocol_parameters() .await? @@ -902,7 +900,7 @@ mod tests { // Add some signatures but not enough to reach the quorum: multi-signer should not create the multi-signature for signature in signatures_to_almost_reach_quorum { multi_signer - .register_single_signature(&signature) + .register_single_signature(&message, &signature) .await .expect("register single signature should not fail"); } @@ -915,7 +913,7 @@ mod tests { // Add the remaining signatures to reach the quorum: multi-signer should create a multi-signature for signature in &signatures { multi_signer - .register_single_signature(signature) + .register_single_signature(&message, signature) .await .expect("register single signature should not fail"); } diff --git a/mithril-aggregator/tests/test_extensions/runtime_tester.rs b/mithril-aggregator/tests/test_extensions/runtime_tester.rs index 5fa19a7e795..f8ddc805718 100644 --- a/mithril-aggregator/tests/test_extensions/runtime_tester.rs +++ b/mithril-aggregator/tests/test_extensions/runtime_tester.rs @@ -37,7 +37,7 @@ macro_rules! cycle_err { $tester .cycle() .await - .expect_err("cycle tick shoudl have returned an error"); + .expect_err("cycle tick should have returned an error"); assert_eq!($expected_state, $tester.runtime.get_state()); }}; } @@ -240,7 +240,7 @@ impl RuntimeTester { /// "Send", actually register, the given single signatures in the multi-signers pub async fn send_single_signatures(&self, signers: &[SignerFixture]) -> Result<(), String> { - let mut multisigner = self.deps.multi_signer.write().await; + let multisigner = self.deps.multi_signer.read().await; let message = multisigner .get_current_message() .await @@ -258,7 +258,7 @@ impl RuntimeTester { ); multisigner - .register_single_signature(&single_signatures) + .register_single_signature(&message, &single_signatures) .await .map_err(|e| { format!("registering a winning lottery signature should not fail: {e:?}") From d83131da1d29489ab6c4503aa3be069b8fe129d1 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Fri, 24 Mar 2023 11:50:19 +0100 Subject: [PATCH 6/7] Refactor compute multi signature And make it use a message argument. --- mithril-aggregator/src/multi_signer.rs | 17 +++++++---------- mithril-aggregator/src/runtime/runner.rs | 12 ++++++------ 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/mithril-aggregator/src/multi_signer.rs b/mithril-aggregator/src/multi_signer.rs index 07dbe259cbd..749bf93549b 100644 --- a/mithril-aggregator/src/multi_signer.rs +++ b/mithril-aggregator/src/multi_signer.rs @@ -193,7 +193,8 @@ pub trait MultiSigner: Sync + Send { /// Creates a multi signature from single signatures async fn create_multi_signature( - &mut self, + &self, + message: &entities::ProtocolMessage, ) -> Result, ProtocolError>; } @@ -588,14 +589,10 @@ impl MultiSigner for MultiSignerImpl { /// Creates a multi signature from single signatures async fn create_multi_signature( - &mut self, + &self, + message: &entities::ProtocolMessage, ) -> Result, ProtocolError> { debug!("Create multi signature"); - let message = &self - .get_current_message() - .await - .ok_or_else(ProtocolError::UnavailableMessage)?; - let beacon = self .current_beacon .as_ref() @@ -892,7 +889,7 @@ mod tests { // No signatures registered: multi-signer can't create the multi-signature assert!(multi_signer - .create_multi_signature() + .create_multi_signature(&message) .await .expect("create multi signature should not fail") .is_none()); @@ -905,7 +902,7 @@ mod tests { .expect("register single signature should not fail"); } assert!(multi_signer - .create_multi_signature() + .create_multi_signature(&message) .await .expect("create multi signature should not fail") .is_none()); @@ -919,7 +916,7 @@ mod tests { } assert!( multi_signer - .create_multi_signature() + .create_multi_signature(&message) .await .expect("create multi signature should not fail") .is_some(), diff --git a/mithril-aggregator/src/runtime/runner.rs b/mithril-aggregator/src/runtime/runner.rs index 58cb0297f32..30bc9c8769c 100644 --- a/mithril-aggregator/src/runtime/runner.rs +++ b/mithril-aggregator/src/runtime/runner.rs @@ -555,13 +555,13 @@ impl AggregatorRunnerTrait for AggregatorRunner { ) -> Result, Box> { debug!("RUNNER: create multi-signature"); - Ok(self - .dependencies - .multi_signer - .write() + let multi_signer = self.dependencies.multi_signer.read().await; + let message = multi_signer + .get_current_message() .await - .create_multi_signature() - .await?) + .ok_or_else(ProtocolError::UnavailableMessage)?; + + Ok(multi_signer.create_multi_signature(&message).await?) } async fn create_snapshot_archive( From 603ec16a5f9a73e3424b82d4edd6a1c8b533d229 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Raynaud Date: Mon, 27 Mar 2023 10:18:59 +0200 Subject: [PATCH 7/7] Bump crates versions --- Cargo.lock | 4 ++-- mithril-aggregator/Cargo.toml | 2 +- mithril-common/Cargo.toml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 15ad5667623..8ed6e52e8be 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2027,7 +2027,7 @@ dependencies = [ [[package]] name = "mithril-aggregator" -version = "0.2.36" +version = "0.2.37" dependencies = [ "async-trait", "chrono", @@ -2089,7 +2089,7 @@ dependencies = [ [[package]] name = "mithril-common" -version = "0.2.32" +version = "0.2.33" dependencies = [ "async-trait", "bech32", diff --git a/mithril-aggregator/Cargo.toml b/mithril-aggregator/Cargo.toml index 545c6f40a72..485a5f20aa1 100644 --- a/mithril-aggregator/Cargo.toml +++ b/mithril-aggregator/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-aggregator" -version = "0.2.36" +version = "0.2.37" description = "A Mithril Aggregator server" authors = { workspace = true } edition = { workspace = true } diff --git a/mithril-common/Cargo.toml b/mithril-common/Cargo.toml index 8f44d6b6924..730be0ae454 100644 --- a/mithril-common/Cargo.toml +++ b/mithril-common/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mithril-common" -version = "0.2.32" +version = "0.2.33" authors = { workspace = true } edition = { workspace = true } documentation = { workspace = true }