Merge pull request #2863 from input-output-hk/djo/2825/enhance-errors-context

enhance errors context

Author: Alenar

Cargo.lock

@@ -23,6 +23,49 @@ To complete
 To complete
 -->
 
+### 5. Guidelines for writing useful log messages, error context, or error structure
+
+**Date:** 2025-07-25
+**Status:** Accepted
+
+### Context
+
+Some errors and logs currently lack enough context to understand the cause of an issue.
+
+This is especially true for failures that occur during requests to or from external sources.
+
+At the same time, adding too much context can make logs noisy and hard to read, and can flood log storage with low-value
+or sensitive data.
+
+### Decision
+
+When writing log messages, adding error context, or designing error structures, follow these guidelines:
+
+- **Prefer structured logging for internal context.**
+  - Add identifiers as structured fields rather than embedding them in the message text (e.g., party id, signed entity
+    type, beacon, request id, entity id).
+  - Keep the human-readable message short, put “what happened” in the message and “what it relates to” in structured fields.
+
+- **Avoid unnecessary or sensitive context in logs by default.**
+  - Do not log secrets or high-risk material (e.g., cryptographic keys, seeds, tokens, credentials).
+  - Do not log large payloads unless they are required for troubleshooting (see “External sources” below).
+
+- **Handle large debug output explicitly.**
+  - If a type’s `Debug` output is too large or contains sensitive fields, implement `Debug` manually to provide a safe,
+    non-exhaustive representation by default.
+  - Optionally support an “alternate” representation (e.g., `{:#?}`) that includes additional detail when it is safe and useful.
+
+- **External sources: allow exceptions when needed to troubleshoot.**
+  - For interactions with external sources, it can be acceptable to include additional context such as request/response
+    payloads **only when necessary** to diagnose issues.
+  - When logging external payloads, prefer safeguards such as truncation/size limits and logging only at error/debug
+    level (and redaction when applicable).
+
+### Consequences
+
+- Logs are more readable and actionable.
+- Errors are easier to understand and troubleshoot without routinely leaking sensitive data or producing excessive log volume.
+
 ### 4. Guidelines for crate test utilities
 
 **Date:** 2025-07-25

DEV-ADR.md

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

mithril-aggregator/Cargo.toml

@@ -56,13 +56,14 @@ mod handlers {
             .get_signature_registration_total_received_since_startup()
             .increment(&[METRICS_HTTP_ORIGIN]);
 
+        let payload = message.clone();
         let signed_entity_type = message.signed_entity_type.clone();
         let signed_message = message.signed_message.clone();
 
         let mut single_signature = match FromRegisterSingleSignatureAdapter::try_adapt(message) {
             Ok(signature) => signature,
             Err(err) => {
-                warn!(logger,"register_signatures::payload decoding error"; "error" => ?err);
+                warn!(logger,"register_signatures::payload decoding error"; "full_payload" => #?payload,"error" => ?err);
 
                 return Ok(reply::bad_request(
                     "Could not decode signature payload".to_string(),
@@ -92,22 +93,22 @@ mod handlers {
         {
             Err(err) => match err.downcast_ref::<CertifierServiceError>() {
                 Some(CertifierServiceError::AlreadyCertified(signed_entity_type)) => {
-                    debug!(logger,"register_signatures::open_message_already_certified"; "signed_entity_type" => ?signed_entity_type);
+                    debug!(logger, "register_signatures::open_message_already_certified"; "signed_entity_type" => ?signed_entity_type, "party_id" => &single_signature.party_id);
                     Ok(reply::gone(
                         "already_certified".to_string(),
                         err.to_string(),
                     ))
                 }
                 Some(CertifierServiceError::Expired(signed_entity_type)) => {
-                    debug!(logger,"register_signatures::open_message_expired"; "signed_entity_type" => ?signed_entity_type);
+                    debug!(logger, "register_signatures::open_message_expired"; "signed_entity_type" => ?signed_entity_type, "party_id" => &single_signature.party_id);
                     Ok(reply::gone("expired".to_string(), err.to_string()))
                 }
                 Some(CertifierServiceError::NotFound(signed_entity_type)) => {
-                    debug!(logger,"register_signatures::not_found"; "signed_entity_type" => ?signed_entity_type);
+                    debug!(logger, "register_signatures::not_found"; "signed_entity_type" => ?signed_entity_type, "party_id" => &single_signature.party_id);
                     Ok(reply::empty(StatusCode::NOT_FOUND))
                 }
                 Some(_) | None => {
-                    warn!(logger,"register_signatures::error"; "error" => ?err);
+                    warn!(logger,"register_signatures::error"; "full_payload" => #?payload, "error" => ?err);
                     Ok(reply::server_error(err))
                 }
             },

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

@@ -114,12 +114,13 @@ mod handlers {
             .get_signer_registration_total_received_since_startup()
             .increment(&[origin_tag.as_deref().unwrap_or_default()]);
 
+        let payload = register_signer_message.clone();
         let registration_epoch = register_signer_message.epoch;
 
         let signer = match FromRegisterSignerAdapter::try_adapt(register_signer_message) {
             Ok(signer) => signer,
             Err(err) => {
-                warn!(logger,"register_signer::payload decoding error"; "error" => ?err);
+                warn!(logger,"register_signer::payload decoding error"; "full_payload" => #?payload, "error" => ?err);
                 return Ok(reply::bad_request(
                     "Could not decode signer payload".to_string(),
                     err.to_string(),
@@ -141,32 +142,42 @@ mod handlers {
                 Ok(reply::empty(StatusCode::CREATED))
             }
             Err(SignerRegistrationError::ExistingSigner(signer_with_stake)) => {
-                debug!(logger, "register_signer::already_registered");
+                debug!(
+                    logger, "register_signer::already_registered";
+                    "registration_epoch" => ?registration_epoch, "party_id" => &signer.party_id,
+                );
 
                 event_transmitter.send(EventMessage::signer_registration(
                     "HTTP::signer_register",
                     &signer_with_stake,
                     signer_node_version,
-                    epoch_str.as_str(),
+                    &epoch_str,
                 ));
 
                 Ok(reply::empty(StatusCode::CREATED))
             }
-            Err(SignerRegistrationError::FailedSignerRegistration(err)) => {
-                warn!(logger,"register_signer::failed_signer_registration"; "error" => ?err);
+            Err(SignerRegistrationError::InvalidSignerRegistration(
+                _party_id,
+                _registration_epoch,
+                err,
+            )) => {
+                warn!(logger,"register_signer::failed_signer_registration"; "full_payload" => #?payload, "error" => ?err);
                 Ok(reply::bad_request(
                     "failed_signer_registration".to_string(),
                     err.to_string(),
                 ))
             }
             Err(SignerRegistrationError::RegistrationRoundNotYetOpened) => {
-                warn!(logger, "register_signer::registration_round_not_yed_opened");
+                warn!(
+                    logger, "register_signer::registration_round_not_yed_opened";
+                    "registration_epoch" => ?registration_epoch, "party_id" => &signer.party_id,
+                );
                 Ok(reply::server_error(
                     SignerRegistrationError::RegistrationRoundNotYetOpened,
                 ))
             }
             Err(err) => {
-                warn!(logger,"register_signer::error"; "error" => ?err);
+                warn!(logger,"register_signer::error"; "full_payload" => #?payload, "error" => ?err);
                 Ok(reply::server_error(err))
             }
         }
@@ -395,9 +406,11 @@ mod tests {
     async fn test_register_signer_post_ko_400() {
         let mut mock_signer_registerer = MockSignerRegisterer::new();
         mock_signer_registerer.expect_register_signer().return_once(|_, _| {
-            Err(SignerRegistrationError::FailedSignerRegistration(anyhow!(
-                ProtocolRegistrationError::OpCertInvalid
-            )))
+            Err(SignerRegistrationError::InvalidSignerRegistration(
+                "party_2".to_string(),
+                Epoch(4),
+                anyhow!(ProtocolRegistrationError::OpCertInvalid),
+            ))
         });
         let mut dependency_manager = initialize_dependencies!().await;
         dependency_manager.signer_registerer = Arc::new(mock_signer_registerer);
@@ -433,7 +446,9 @@ mod tests {
         let mut mock_signer_registerer = MockSignerRegisterer::new();
         mock_signer_registerer.expect_register_signer().return_once(|_, _| {
             Err(SignerRegistrationError::FailedSignerRecorder(
-                "an error occurred".to_string(),
+                "party_1".to_string(),
+                Epoch(4),
+                anyhow!("an error occurred"),
             ))
         });
         let mut dependency_manager = initialize_dependencies!().await;

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

@@ -427,6 +427,7 @@ mod tests {
                         .returning(|_, _| {
                             Err(CertifierServiceError::InvalidSingleSignature(
                                 OpenMessage::dummy().signed_entity_type,
+                                "party_1".to_string(),
                                 anyhow!("Invalid signature"),
                             )
                             .into())

mithril-aggregator/src/services/certifier/buffered_certifier.rs

@@ -108,22 +108,27 @@ impl CertifierService for MithrilCertifierService {
     ) -> StdResult<SignatureRegistrationStatus> {
         debug!(
             self.logger,
-            ">> register_single_signature(signed_entity_type: {signed_entity_type:?}, single_signatures: {signature:?}"
+            ">> register_single_signature(signed_entity_type: {signed_entity_type:?}, single_signature: {signature:?}"
         );
-        trace!(self.logger, ">> register_single_signature"; "complete_single_signatures" => #?signature);
+        trace!(self.logger, ">> register_single_signature"; "complete_single_signature" => #?signature);
 
         let open_message = self
             .get_open_message_record(signed_entity_type)
-            .await.with_context(|| format!("CertifierService can not get open message record for signed_entity_type: '{signed_entity_type}'"))?
+            .await
+            .with_context(|| format!("CertifierService can not get open message record for signed_entity_type: '{signed_entity_type}'"))?
             .ok_or_else(|| {
-                warn!(self.logger, "register_single_signature: OpenMessage not found for type {signed_entity_type:?}.");
+                warn!(
+                    self.logger, "register_single_signature: OpenMessage not found for type {signed_entity_type:?}";
+                    "party_id" => &signature.party_id
+                );
                 CertifierServiceError::NotFound(signed_entity_type.clone())
             })?;
 
         if open_message.is_certified {
             warn!(
                 self.logger,
-                "register_single_signature: open message {signed_entity_type:?} is already certified, cannot register single signature."
+                "register_single_signature: open message {signed_entity_type:?} is already certified, cannot register single signature.";
+                "party_id" => &signature.party_id
             );
 
             return Err(CertifierServiceError::AlreadyCertified(signed_entity_type.clone()).into());
@@ -132,7 +137,8 @@ impl CertifierService for MithrilCertifierService {
         if open_message.is_expired {
             warn!(
                 self.logger,
-                "register_single_signature: open message {signed_entity_type:?} has expired, cannot register single signature."
+                "register_single_signature: open message {signed_entity_type:?} has expired, cannot register single signature.";
+                "party_id" => &signature.party_id
             );
 
             return Err(CertifierServiceError::Expired(signed_entity_type.clone()).into());
@@ -142,13 +148,18 @@ impl CertifierService for MithrilCertifierService {
             .verify_single_signature(&open_message.protocol_message.to_message(), signature)
             .await
             .map_err(|err| {
-                CertifierServiceError::InvalidSingleSignature(signed_entity_type.clone(), err)
+                CertifierServiceError::InvalidSingleSignature(
+                    signed_entity_type.clone(),
+                    signature.party_id.clone(),
+                    err,
+                )
             })?;
 
         let single_signature = self
             .single_signature_repository
             .create_single_signature(signature, &open_message.clone().into())
-            .await.with_context(|| format!("Certifier can not create the single signature from single_signature: '{signature:?}', open_message: '{open_message:?}'"))?;
+            .await
+            .with_context(|| format!("Certifier can not create the single signature from single_signature: '{signature:?}', open_message: '{open_message:?}'"))?;
         info!(
             self.logger,
             "register_single_signature: created pool '{}' single signature for {signed_entity_type:?}.",
@@ -826,7 +837,7 @@ mod tests {
         if let Some(err) = error.downcast_ref::<CertifierServiceError>() {
             assert!(matches!(
                 err,
-                CertifierServiceError::AlreadyCertified(signed_entity) if signed_entity == &signed_entity_type
+                CertifierServiceError::AlreadyCertified(signed_entity, ..) if signed_entity == &signed_entity_type
             ),);
         } else {
             panic!("Unexpected error {error:?}");

mithril-aggregator/src/services/certifier/certifier_service.rs

@@ -2,7 +2,7 @@ use async_trait::async_trait;
 use thiserror::Error;
 
 use mithril_common::entities::{
-    Certificate, Epoch, ProtocolMessage, SignedEntityType, SignedEntityTypeDiscriminants,
+    Certificate, Epoch, PartyId, ProtocolMessage, SignedEntityType, SignedEntityTypeDiscriminants,
     SingleSignature,
 };
 use mithril_common::{StdError, StdResult};
@@ -27,8 +27,8 @@ pub enum CertifierServiceError {
     Expired(SignedEntityType),
 
     /// An invalid signature was provided.
-    #[error("Invalid single signature for {0:?}.")]
-    InvalidSingleSignature(SignedEntityType, #[source] StdError),
+    #[error("Invalid single signature for {0:?} from party '{1}'.")]
+    InvalidSingleSignature(SignedEntityType, PartyId, #[source] StdError),
 
     /// No parent certificate could be found, this certifier cannot create genesis certificates.
     #[error(

mithril-aggregator/src/services/certifier/interface.rs

@@ -1,9 +1,7 @@
 use thiserror::Error;
 
-use mithril_common::{
-    StdError,
-    entities::{Epoch, SignerWithStake},
-};
+use mithril_common::StdError;
+use mithril_common::entities::{Epoch, PartyId, SignerWithStake};
 
 use mithril_cardano_node_chain::chain_observer::ChainObserverError;
 
@@ -42,12 +40,12 @@ pub enum SignerRegistrationError {
     EpochService(#[source] StdError),
 
     /// Signer registration failed.
-    #[error("signer registration failed")]
-    FailedSignerRegistration(#[source] StdError),
+    #[error("Signer registration is invalid for party '{0}' and epoch {1}")]
+    InvalidSignerRegistration(PartyId, Epoch, #[source] StdError),
 
     /// Signer recorder failed.
-    #[error("signer recorder failed: '{0}'")]
-    FailedSignerRecorder(String),
+    #[error("Recording signer registration failed for party '{0}' and epoch {1}")]
+    FailedSignerRecorder(PartyId, Epoch, #[source] StdError),
 
     /// Signer registration is always closed on a follower aggregator.
     #[error("signer registration is always closed on a follower aggregator")]

mithril-aggregator/src/services/signer_registration/error.rs

@@ -70,12 +70,24 @@ impl MithrilSignerRegistrationFollower {
                 .signer_registration_verifier
                 .verify(signer, stake_distribution)
                 .await
-                .map_err(SignerRegistrationError::FailedSignerRegistration)?;
+                .map_err(|err| {
+                    SignerRegistrationError::InvalidSignerRegistration(
+                        signer.party_id.clone(),
+                        epoch,
+                        err,
+                    )
+                })?;
 
             self.signer_recorder
                 .record_signer_registration(signer_with_stake.party_id.clone())
                 .await
-                .map_err(|e| SignerRegistrationError::FailedSignerRecorder(e.to_string()))?;
+                .map_err(|err| {
+                    SignerRegistrationError::FailedSignerRecorder(
+                        signer_with_stake.party_id.clone(),
+                        epoch,
+                        err,
+                    )
+                })?;
 
             self
                 .verification_key_store