Remove multi-signature from multi-signer state

Author: jpraynaud

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<Option<ProtocolMultiSignature>, 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<DateTime<Utc>>,
 
-    /// Created multi signature for message signed
-    multi_signature: Option<ProtocolMultiSignature>,
-
     /// Verification key store
     verification_key_store: Arc<VerificationKeyStore>,
 
@@ -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::<String>());
 
-        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<Option<ProtocolMultiSignature>, 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"
         );

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<Option<CertificatePending>, Box<dyn StdError + Sync + Send>>;
 
-    /// Check if the multisigner has issued a multi-signature.
-    async fn is_multisig_created(&self) -> Result<bool, Box<dyn StdError + Sync + Send>>;
+    /// Create multi-signature.
+    async fn create_multi_signature(
+        &self,
+    ) -> Result<Option<ProtocolMultiSignature>, Box<dyn StdError + Sync + Send>>;
 
     /// 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<Certificate, Box<dyn StdError + Sync + Send>>;
 
     /// 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<bool, Box<dyn StdError + Sync + Send>> {
-        debug!("RUNNER: check if we can create a multi-signature");
-        let has_multisig = self
+    async fn create_multi_signature(
+        &self,
+    ) -> Result<Option<ProtocolMultiSignature>, Box<dyn StdError + Sync + Send>> {
+        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<Certificate, Box<dyn StdError + Sync + Send>> {
         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<PartyId> = self
             .dependencies
             .single_signature_store
@@ -645,12 +642,6 @@ impl AggregatorRunnerTrait for AggregatorRunner {
             .unwrap_or_default()
             .into_keys()
             .collect::<Vec<_>>();
-        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");
     }

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<IdleState, RuntimeError> {
         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()

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");

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();

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<DumbSnapshotUploader>,
     pub chain_observer: Arc<FakeObserver>,