Merge pull request #2381 from input-output-hk/sfa/2363/error_messages_as_warning_in_signer

Error messages as warning in signer

Author: sfauvel

Cargo.lock

@@ -2,6 +2,27 @@
 
 ---
 
+<!--
+Template of ADR
+
+## ID. TITLE
+
+date: 2025-XX-XX
+status: Accepted
+
+### Context
+
+To complete
+
+### Decision
+
+To complete
+
+### Consequences
+
+To complete
+-->
+
 ## 2. Remove Artifacts serialization support when compiling to WebAssembly
 
 date: 2025-02-26

DEV-ADR.md

@@ -0,0 +1,29 @@
+---
+slug: 10
+title: |
+  10. Specific Mithril Http status code
+authors:
+  - name: Mithril Team
+tags: [Accepted]
+date: 2025-03-21
+---
+
+## Status
+
+Accepted
+
+## Context
+
+In exchanges between the signer and the aggregator, we need to retrieve the reason why a request was unsuccessful.
+Error handling will depend on the specific functional case of Mithril.
+We could have reused existing HTTP codes, but they are too general and could be returned for cases other than the one we wish to isolate.
+
+## Decision
+
+We therefore decided to return specific error codes when we need to identify the functional case.
+We start at 450 for client error codes and at 550 for server error codes.
+
+## Consequences
+
+Specific Mithril HTTP status code on server error should be between 550 and 599.
+For client error the HTTP status code should be between 450 and 499.

docs/website/adr/010-http-status-code.md

@@ -1,6 +1,6 @@
 [package]
 name = "mithril-aggregator"
-version = "0.7.19"
+version = "0.7.20"
 description = "A Mithril Aggregator server"
 authors = { workspace = true }
 edition = { workspace = true }

mithril-aggregator/Cargo.toml

@@ -10,6 +10,15 @@ use mithril_persistence::sqlite::error::{SqliteError, SQLITE_BUSY};
 use crate::tools::downcast_check;
 use crate::SignerRegistrationError;
 
+pub struct MithrilStatusCode();
+
+impl MithrilStatusCode {
+    pub fn registration_round_not_yet_opened() -> StatusCode {
+        // The unwrap is safe here because `from_16` function return error only for values outside of the range 100-999,
+        StatusCode::from_u16(550).unwrap()
+    }
+}
+
 pub fn json<T>(value: &T, status_code: StatusCode) -> Box<dyn warp::Reply>
 where
     T: Serialize,
@@ -46,7 +55,7 @@ pub fn server_error<E: Into<StdError>>(error: E) -> Box<dyn warp::Reply> {
         if downcast_check::<SignerRegistrationError>(&std_error, |e| {
             matches!(e, SignerRegistrationError::RegistrationRoundNotYetOpened)
         }) {
-            code = StatusCode::SERVICE_UNAVAILABLE;
+            code = MithrilStatusCode::registration_round_not_yet_opened();
         }
 
         code
@@ -59,10 +68,6 @@ pub fn internal_server_error<T: Into<ServerError>>(message: T) -> Box<dyn warp::
     json(&message.into(), StatusCode::INTERNAL_SERVER_ERROR)
 }
 
-pub fn service_unavailable<T: Into<ServerError>>(message: T) -> Box<dyn warp::Reply> {
-    json(&message.into(), StatusCode::SERVICE_UNAVAILABLE)
-}
-
 pub fn add_content_disposition_header(
     reply: warp::fs::File,
     filepath: &Path,
@@ -113,16 +118,22 @@ mod tests {
     }
 
     #[test]
-    fn test_server_error_convert_signer_registration_round_not_yet_opened_to_503() {
+    fn test_server_error_convert_signer_registration_round_not_yet_opened_to_550() {
         let err = SignerRegistrationError::RegistrationRoundNotYetOpened;
         let response = server_error(err).into_response();
 
-        assert_eq!(StatusCode::SERVICE_UNAVAILABLE, response.status());
+        assert_eq!(
+            MithrilStatusCode::registration_round_not_yet_opened(),
+            response.status()
+        );
 
         // Wrapping the error in a StdError should also work
         let err = anyhow!(SignerRegistrationError::RegistrationRoundNotYetOpened);
         let response = server_error(err).into_response();
 
-        assert_eq!(StatusCode::SERVICE_UNAVAILABLE, response.status());
+        assert_eq!(
+            MithrilStatusCode::registration_round_not_yet_opened(),
+            response.status()
+        );
     }
 }

mithril-aggregator/src/http_server/routes/reply.rs

@@ -174,8 +174,8 @@ mod handlers {
             }
             Err(SignerRegistrationError::RegistrationRoundNotYetOpened) => {
                 warn!(logger, "register_signer::registration_round_not_yed_opened");
-                Ok(reply::service_unavailable(
-                    SignerRegistrationError::RegistrationRoundNotYetOpened.to_string(),
+                Ok(reply::server_error(
+                    SignerRegistrationError::RegistrationRoundNotYetOpened,
                 ))
             }
             Err(err) => {
@@ -275,6 +275,7 @@ mod tests {
 
     use crate::{
         database::{record::SignerRecord, repository::MockSignerGetter},
+        http_server::routes::reply::MithrilStatusCode,
         initialize_dependencies,
         services::{FakeEpochService, MockSignerRegisterer},
         store::MockVerificationKeyStorer,
@@ -478,7 +479,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn test_register_signer_post_ko_503() {
+    async fn test_register_signer_post_ko_550() {
         let mut mock_signer_registerer = MockSignerRegisterer::new();
         mock_signer_registerer
             .expect_register_signer()
@@ -506,7 +507,7 @@ mod tests {
             "application/json",
             &signer,
             &response,
-            &StatusCode::SERVICE_UNAVAILABLE,
+            &MithrilStatusCode::registration_round_not_yet_opened(),
         )
         .unwrap();
     }

mithril-aggregator/src/http_server/routes/signer_routes.rs

@@ -1,6 +1,6 @@
 [package]
 name = "mithril-signer"
-version = "0.2.235"
+version = "0.2.236"
 description = "A Mithril Signer"
 authors = { workspace = true }
 edition = { workspace = true }

mithril-signer/Cargo.toml

@@ -1,3 +1,4 @@
+use anyhow::Error;
 use slog::{debug, info, Logger};
 use std::{fmt::Display, ops::Deref, sync::Arc, time::Duration};
 use tokio::{sync::Mutex, time::sleep};
@@ -8,8 +9,11 @@ use mithril_common::{
     logging::LoggerExtensions,
 };
 
-use crate::entities::{BeaconToSign, SignerEpochSettings};
 use crate::MetricsService;
+use crate::{
+    entities::{BeaconToSign, SignerEpochSettings},
+    services::AggregatorClientError,
+};
 
 use super::{Runner, RuntimeError};
 
@@ -314,19 +318,36 @@ impl StateMachine {
                 nested_error: Some(e),
             })?;
 
-        self.runner.register_signer_to_aggregator()
-            .await.map_err(|e| {
-            if e.downcast_ref::<ProtocolInitializerError>().is_some() {
-                RuntimeError::Critical { message: format!("Could not register to aggregator in 'unregistered → registered' phase for epoch {:?}.", epoch), nested_error: Some(e) }
+        fn handle_registration_result(
+            register_result: Result<(), Error>,
+            epoch: Epoch,
+        ) -> Result<Option<SignerState>, RuntimeError> {
+            if let Err(e) = register_result {
+                if let Some(AggregatorClientError::RegistrationRoundNotYetOpened(_)) =
+                    e.downcast_ref::<AggregatorClientError>()
+                {
+                    Ok(Some(SignerState::Unregistered { epoch }))
+                } else if e.downcast_ref::<ProtocolInitializerError>().is_some() {
+                    Err(RuntimeError::Critical { message: format!("Could not register to aggregator in 'unregistered → registered' phase for epoch {:?}.", epoch), nested_error: Some(e) })
+                } else {
+                    Err(RuntimeError::KeepState { message: format!("Could not register to aggregator in 'unregistered → registered' phase for epoch {:?}.", epoch), nested_error: Some(e) })
+                }
             } else {
-                RuntimeError::KeepState { message: format!("Could not register to aggregator in 'unregistered → registered' phase for epoch {:?}.", epoch), nested_error: Some(e) }
+                Ok(None)
             }
-        })?;
+        }
+
+        let register_result = self.runner.register_signer_to_aggregator().await;
+        let next_state_found = handle_registration_result(register_result, epoch)?;
 
         self.metrics_service
             .get_signer_registration_success_since_startup_counter()
             .increment();
 
+        if let Some(state) = next_state_found {
+            return Ok(state);
+        }
+
         self.metrics_service
             .get_signer_registration_success_last_epoch_gauge()
             .record(epoch);
@@ -452,13 +473,15 @@ impl StateMachine {
 
 #[cfg(test)]
 mod tests {
+    use anyhow::anyhow;
     use chrono::DateTime;
     use mockall::predicate;
 
     use mithril_common::entities::{ChainPoint, Epoch, ProtocolMessage, SignedEntityType};
     use mithril_common::test_utils::fake_data;
 
     use crate::runtime::runner::MockSignerRunner;
+    use crate::services::AggregatorClientError;
     use crate::test_tools::TestLogger;
 
     use super::*;
@@ -645,6 +668,88 @@ mod tests {
             },
             state_machine.get_state().await
         );
+
+        let metrics_service = state_machine.metrics_service;
+        let success_since_startup =
+            metrics_service.get_runtime_cycle_success_since_startup_counter();
+        assert_eq!(1, success_since_startup.get());
+    }
+
+    #[tokio::test]
+    async fn unregistered_to_ready_to_sign_counter() {
+        let mut runner = MockSignerRunner::new();
+
+        runner
+            .expect_get_epoch_settings()
+            .once()
+            .returning(|| Ok(Some(SignerEpochSettings::dummy())));
+
+        runner
+            .expect_inform_epoch_settings()
+            .with(predicate::eq(SignerEpochSettings::dummy()))
+            .once()
+            .returning(|_| Ok(()));
+
+        runner
+            .expect_get_current_time_point()
+            .times(2)
+            .returning(|| Ok(TimePoint::dummy()));
+        runner
+            .expect_update_stake_distribution()
+            .once()
+            .returning(|_| Ok(()));
+        runner
+            .expect_register_signer_to_aggregator()
+            .once()
+            .returning(|| {
+                Err(AggregatorClientError::RegistrationRoundNotYetOpened(
+                    anyhow!("Not yet opened"),
+                ))?
+            });
+
+        runner.expect_upkeep().never();
+        runner.expect_can_sign_current_epoch().never();
+
+        let state_machine = init_state_machine(
+            SignerState::Unregistered {
+                epoch: TimePoint::dummy().epoch,
+            },
+            runner,
+        );
+
+        state_machine
+            .cycle()
+            .await
+            .expect("Cycling the state machine should not fail");
+
+        assert_eq!(
+            SignerState::Unregistered {
+                epoch: TimePoint::dummy().epoch,
+            },
+            state_machine.get_state().await
+        );
+
+        let metrics_service = state_machine.metrics_service;
+        assert_eq!(
+            1,
+            metrics_service
+                .get_signer_registration_success_since_startup_counter()
+                .get()
+        );
+
+        assert_eq!(
+            0 as f64,
+            metrics_service
+                .get_signer_registration_success_last_epoch_gauge()
+                .get()
+        );
+
+        assert_eq!(
+            1,
+            metrics_service
+                .get_runtime_cycle_total_since_startup_counter()
+                .get()
+        );
     }
 
     #[tokio::test]