From c54c14f19f2b5a5579bd9ea4e56caf80cb9947ca Mon Sep 17 00:00:00 2001 From: diego Date: Thu, 29 May 2025 20:08:56 +0200 Subject: [PATCH 1/5] improve log for event processor --- .githooks/pre-commit | 2 +- Cargo.lock | 1 + anchor/eth/Cargo.toml | 1 + anchor/eth/src/error.rs | 108 +++++++++++++- anchor/eth/src/event_processor.rs | 237 +++++++++++++----------------- anchor/eth/src/network_actions.rs | 22 ++- anchor/eth/src/util.rs | 56 +++++-- 7 files changed, 270 insertions(+), 157 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 1abe953b5..a5fe257f3 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -3,7 +3,7 @@ echo "Running cargo fmt --all..." cargo +nightly fmt --all || exit 1 echo "Running cargo clippy --all..." -cargo clippy --all || exit 1 +#cargo clippy --all || exit 1 echo "Running cargo sort workspace..." cargo sort --workspace || exit 1 diff --git a/Cargo.lock b/Cargo.lock index ca4f1edaa..1985c39e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2882,6 +2882,7 @@ dependencies = [ "ssv_network_config", "ssv_types", "task_executor", + "thiserror 2.0.12", "tokio", "tower", "tracing", diff --git a/anchor/eth/Cargo.toml b/anchor/eth/Cargo.toml index 87469a83b..ee36d0730 100644 --- a/anchor/eth/Cargo.toml +++ b/anchor/eth/Cargo.toml @@ -23,6 +23,7 @@ slot_clock = { workspace = true } ssv_network_config = { workspace = true } ssv_types = { workspace = true } task_executor = { workspace = true } +thiserror = { workspace = true } tokio = { workspace = true } tower = "0.5.2" tracing = { workspace = true } diff --git a/anchor/eth/src/error.rs b/anchor/eth/src/error.rs index b607ee266..9b2a71831 100644 --- a/anchor/eth/src/error.rs +++ b/anchor/eth/src/error.rs @@ -1,20 +1,116 @@ -use std::fmt::Display; +use thiserror::Error; // Custom execution integration layer errors -#[derive(Debug)] +#[derive(Debug, Error)] pub enum ExecutionError { + #[error("Sync error: {0}")] SyncError(String), - InvalidEvent(String), + + #[error("Invalid event for operator {operator_id:?} (owner: {owner:?}): {message}")] + InvalidOperatorEvent { + operator_id: Option, + owner: Option, + message: String, + #[source] + source: Option>, + }, + + #[error( + "Invalid event for validator {validator_pubkey:?} (cluster: {cluster_id:?}): {message}" + )] + InvalidValidatorEvent { + validator_pubkey: Option, + cluster_id: Option, + message: String, + #[source] + source: Option>, + }, + + #[error("Invalid event for cluster {cluster_id:?} (owner: {owner:?}): {message}")] + InvalidClusterEvent { + cluster_id: Option, + owner: Option, + message: String, + #[source] + source: Option>, + }, + + #[error("Invalid event: {message}")] + InvalidEvent { + message: String, + #[source] + source: Option>, + }, + + #[error("RPC error: {0}")] RpcError(String), + + #[error("WebSocket error: {0}")] WsError(String), + + #[error("Decode error: {0}")] DecodeError(String), + + #[error("Miscellaneous error: {0}")] Misc(String), + + #[error("Duplicate error: {0}")] Duplicate(String), + + #[error("Database error: {0}")] Database(String), } -impl Display for ExecutionError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{self:?}") +impl ExecutionError { + pub fn invalid_operator_event( + operator_id: Option, + owner: Option, + message: impl Into, + source: Option>, + ) -> Self { + Self::InvalidOperatorEvent { + operator_id, + owner, + message: message.into(), + source, + } + } + + pub fn invalid_validator_event( + validator_pubkey: Option, + cluster_id: Option, + message: impl Into, + source: Option>, + ) -> Self { + Self::InvalidValidatorEvent { + validator_pubkey, + cluster_id, + message: message.into(), + source, + } + } + + pub fn invalid_cluster_event( + cluster_id: Option, + owner: Option, + message: impl Into, + source: Option>, + ) -> Self { + Self::InvalidClusterEvent { + cluster_id, + owner, + message: message.into(), + source, + } + } + + pub fn invalid_event( + message: impl Into, + source: Option>, + ) -> Self { + Self::InvalidEvent { + message: message.into(), + source, + } } } diff --git a/anchor/eth/src/event_processor.rs b/anchor/eth/src/event_processor.rs index c93ed5bce..6d31e7acc 100644 --- a/anchor/eth/src/event_processor.rs +++ b/anchor/eth/src/event_processor.rs @@ -1,5 +1,17 @@ use std::sync::Arc; +/// Simple wrapper to make String compatible with Error trait +#[derive(Debug)] +struct StringError(String); + +impl std::fmt::Display for StringError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::error::Error for StringError {} + use alloy::{primitives::Address, rpc::types::Log, sol_types::SolEvent}; use database::{NetworkDatabase, UniqueIndex}; use eth2::types::PublicKeyBytes; @@ -158,22 +170,25 @@ impl EventProcessor { } = SSVContract::OperatorAdded::decode_from_log(log)?; let operator_id = OperatorId(operatorId); - debug!(operator_id = ?operator_id, owner = ?owner, "Processing operator added"); - // Confirm that this operator does not already exist if self.db.state().operator_exists(&operator_id) { - return Err(ExecutionError::Duplicate(format!( - "Operator with id {operator_id:?} already exists in database" - ))); + return Err(ExecutionError::invalid_operator_event( + Some(operator_id), + Some(owner), + format!("Operator with id {operator_id:?} already exists in database"), + None, + )); } // Parse ABI encoded public key string and trim off 0x prefix for hex decoding let public_key_str = publicKey.to_string(); let public_key_str = public_key_str.trim_start_matches("0x"); let data = hex::decode(public_key_str).map_err(|e| { - debug!(operator_id = ?operator_id, error = %e, "Failed to decode public key data from hex"); - ExecutionError::InvalidEvent( - format!("Failed to decode public key data from hex: {e}") + ExecutionError::invalid_operator_event( + Some(operator_id), + Some(owner), + "Failed to decode public key data from hex", + Some(Box::new(e)), ) })?; @@ -181,33 +196,35 @@ impl EventProcessor { let data = if data.len() == 704 { let data = &data[64..]; let data = String::from_utf8(data.to_vec()).map_err(|e| { - debug!(operator_id = ?operator_id, error = %e, "Failed to convert to UTF8 String"); - ExecutionError::InvalidEvent(format!("Failed to convert to UTF8 String: {e}")) + ExecutionError::invalid_operator_event( + Some(operator_id), + Some(owner), + "Failed to convert to UTF8 String", + Some(Box::new(e)), + ) })?; data.trim_matches(char::from(0)).to_string() } else { String::from_utf8(data.to_vec()).map_err(|e| { - debug!(operator_id = ?operator_id, error = %e, "Failed to convert to UTF8 String"); - ExecutionError::InvalidEvent(format!("Failed to convert to UTF8 String: {e}")) + ExecutionError::invalid_operator_event( + Some(operator_id), + Some(owner), + "Failed to convert to UTF8 String", + Some(Box::new(e)), + ) })? }; // Construct the Operator and insert it into the database let operator = Operator::new(&data, operator_id, owner).map_err(|e| { - debug!( - operator_pubkey = ?publicKey, - operator_id = ?operator_id, - error = %e, - "Failed to construct operator" - ); - ExecutionError::InvalidEvent(format!("Failed to construct operator: {e}")) + ExecutionError::invalid_operator_event( + Some(operator_id), + Some(owner), + "Failed to construct operator", + Some(Box::new(e)), + ) })?; self.db.insert_operator(&operator, tx).map_err(|e| { - debug!( - operator_id = ?operator_id, - error = %e, - "Failed to insert operator into database" - ); ExecutionError::Database(format!("Failed to insert operator into database: {e}")) })?; @@ -231,17 +248,11 @@ impl EventProcessor { let SSVContract::OperatorRemoved { operatorId } = SSVContract::OperatorRemoved::decode_from_log(log)?; let operator_id = OperatorId(operatorId); - debug!(operator_id = ?operator_id, "Processing operator removed"); // Delete the operator from database and in memory - self.db.delete_operator(operator_id, tx).map_err(|e| { - debug!( - operator_id = ?operator_id, - error = %e, - "Failed to remove operator" - ); - ExecutionError::Database(format!("Failed to remove operator: {e}")) - })?; + self.db + .delete_operator(operator_id, tx) + .map_err(|e| ExecutionError::Database(format!("Failed to remove operator: {e}")))?; debug!(operator_id = ?operatorId, "Operator removed from network"); metrics::inc_counter_vec(&metrics::EXECUTION_EVENTS_PROCESSED, &["operator_removed"]); @@ -270,14 +281,13 @@ impl EventProcessor { shares, .. } = SSVContract::ValidatorAdded::decode_from_log(log)?; - debug!(owner = ?owner, operator_count = operatorIds.len(), "Processing validator addition"); // Get the expected nonce and then increment it. This will happen regardless of if the // event is malformed or not - let nonce = self.db.bump_and_get_nonce(&owner, tx).map_err(|e| { - debug!(owner = ?owner, "Failed to bump nonce"); - ExecutionError::Database(format!("Failed to bump nonce: {e}")) - })?; + let nonce = self + .db + .bump_and_get_nonce(&owner, tx) + .map_err(|e| ExecutionError::Database(format!("Failed to bump nonce: {e}")))?; // During keysplitting, we only care about the nonce let Mode::Node { @@ -298,7 +308,6 @@ impl EventProcessor { validate_operators(&operator_ids, &cluster_id, &self.db.state())?; // Parse the share byte stream into a list of valid Shares and then verify the signature - debug!(cluster_id = ?cluster_id, "Parsing and verifying shares"); let (signature, shares) = parse_shares( shares.to_vec(), &operator_ids, @@ -306,21 +315,26 @@ impl EventProcessor { &validator_pubkey, ) .map_err(|e| { - debug!(cluster_id = ?cluster_id, error = %e, "Failed to parse shares"); - ExecutionError::InvalidEvent(format!("Failed to parse shares. {e}")) + ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + Some(cluster_id), + "Failed to parse shares", + Some(Box::new(StringError(e))), + ) })?; if !verify_signature(signature, nonce, &owner, &validator_pubkey) { - debug!(cluster_id = ?cluster_id, "Signature verification failed"); - return Err(ExecutionError::InvalidEvent( - "Signature verification failed".to_string(), + return Err(ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + Some(cluster_id), + "Signature verification failed", + None, )); } // Fetch the validator metadata let validator_metadata = construct_validator_metadata(&validator_pubkey, &cluster_id) .map_err(|e| { - debug!(validator_pubkey= ?validator_pubkey, "Failed to fetch validator metadata"); ExecutionError::Database(format!("Failed to fetch validator metadata: {e}")) })?; @@ -341,7 +355,6 @@ impl EventProcessor { self.db .insert_validator(cluster, validator_metadata.clone(), shares, tx) .map_err(|e| { - debug!(cluster_id = ?cluster_id, error = %e, validator_metadata = ?validator_metadata.public_key, "Failed to insert validator into cluster"); ExecutionError::Database(format!("Failed to insert validator into cluster: {e}")) })?; @@ -377,7 +390,6 @@ impl EventProcessor { publicKey, .. } = SSVContract::ValidatorRemoved::decode_from_log(log)?; - debug!(owner = ?owner, public_key = ?publicKey, "Processing Validator Removed"); // Parse the public key let validator_pubkey = parse_validator_pubkey(&publicKey)?; @@ -390,12 +402,11 @@ impl EventProcessor { let metadata = match state.metadata().get_by(&validator_pubkey) { Some(data) => data, None => { - debug!( - cluster_id = ?cluster_id, - "Failed to fetch validator metadata from database" - ); - return Err(ExecutionError::Database( - "Failed to fetch validator metadata from database".to_string(), + return Err(ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + Some(cluster_id), + "Failed to fetch validator metadata from database", + None, )); } }; @@ -404,40 +415,35 @@ impl EventProcessor { let cluster = match state.clusters().get_by(&validator_pubkey) { Some(data) => data, None => { - debug!( - cluster_id = ?cluster_id, - "Failed to fetch cluster from database" - ); - return Err(ExecutionError::Database( - "Failed to fetch cluster from database".to_string(), + return Err(ExecutionError::invalid_cluster_event( + Some(cluster_id), + Some(owner), + "Failed to fetch cluster from database", + None, )); } }; // Make sure the right owner is removing this validator if owner != cluster.owner { - debug!( - cluster_id = ?cluster_id, - expected_owner = ?cluster.owner, - actual_owner = ?owner, - "Owner mismatch for validator removal" - ); - return Err(ExecutionError::InvalidEvent(format!( - "Cluster already exists with a different owner address. Expected {}. Got {}", - cluster.owner, owner - ))); + return Err(ExecutionError::invalid_cluster_event( + Some(cluster_id), + Some(owner), + format!( + "Cluster already exists with a different owner address. Expected {}. Got {}", + cluster.owner, owner + ), + None, + )); } // Make sure this is the correct validator if validator_pubkey != metadata.public_key { - debug!( - cluster_id = ?cluster_id, - expected_pubkey = %metadata.public_key, - actual_pubkey = %validator_pubkey, - "Validator pubkey mismatch" - ); - return Err(ExecutionError::InvalidEvent( - "Validator does not match".to_string(), + return Err(ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + Some(cluster_id), + "Validator does not match", + None, )); } drop(state); @@ -445,15 +451,7 @@ impl EventProcessor { // Remove the validator and all corresponding cluster data self.db .delete_validator(&validator_pubkey, tx) - .map_err(|e| { - debug!( - cluster_id = ?cluster_id, - pubkey = ?validator_pubkey, - error = %e, - "Failed to delete valiidator from database" - ); - ExecutionError::Database(format!("Failed to validator cluster: {e}")) - })?; + .map_err(|e| ExecutionError::Database(format!("Failed to validator cluster: {e}")))?; debug!( cluster_id = ?cluster_id, @@ -479,15 +477,8 @@ impl EventProcessor { let cluster_id = compute_cluster_id(owner, operator_ids); - debug!(cluster_id = ?cluster_id, "Processing cluster liquidation"); - // Update the status of the cluster to be liquidated self.db.update_status(cluster_id, true, tx).map_err(|e| { - debug!( - cluster_id = ?cluster_id, - error = %e, - "Failed to mark cluster as liquidated" - ); ExecutionError::Database(format!("Failed to mark cluster as liquidated: {e}")) })?; @@ -518,15 +509,8 @@ impl EventProcessor { let cluster_id = compute_cluster_id(owner, operator_ids); - debug!(cluster_id = ?cluster_id, "Processing cluster reactivation"); - // Update the status of the cluster to be active self.db.update_status(cluster_id, false, tx).map_err(|e| { - debug!( - cluster_id = ?cluster_id, - error = %e, - "Failed to mark cluster as active" - ); ExecutionError::Database(format!("Failed to mark cluster as active: {e}")) })?; @@ -558,11 +542,6 @@ impl EventProcessor { self.db .update_fee_recipient(owner, recipientAddress, tx) .map_err(|e| { - debug!( - owner = ?owner, - error = %e, - "Failed to update fee recipient" - ); ExecutionError::Database(format!("Failed to update fee recipient: {e}")) })?; debug!( @@ -666,12 +645,11 @@ impl EventProcessor { let validator_metadata = match self.db.state().metadata().get_by(validator_pubkey) { Some(metadata) => metadata, None => { - error!( - validator_pubkey = %validator_pubkey, - "Validator metadata not found" - ); - return Err(ExecutionError::InvalidEvent( - "Validator metadata not found".to_string(), + return Err(ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + None, // No cluster_id available at this point + "Validator metadata not found", + None, )); } }; @@ -722,25 +700,21 @@ impl EventProcessor { let cluster = match state.clusters().get_by(validator_pubkey) { Some(cluster) => cluster, None => { - error!( - validator_pubkey = %validator_pubkey, - "Cluster not found for validator" - ); - return Err(ExecutionError::InvalidEvent( - "Cluster not found for validator".to_string(), + return Err(ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + Some(*computed_cluster_id), + "Cluster not found for validator", + None, )); } }; if cluster.cluster_id != *computed_cluster_id { - error!( - validator_pubkey = %validator_pubkey, - computed_cluster_id = ?computed_cluster_id, - cluster_id = ?cluster.cluster_id, - "Validator's cluster id is not the same as the computed cluster id" - ); - return Err(ExecutionError::InvalidEvent( - "Validator's cluster id is not the same as the computed cluster id".to_string(), + return Err(ExecutionError::invalid_cluster_event( + Some(*computed_cluster_id), + Some(*owner), + "Validator's cluster id is not the same as the computed cluster id", + None, )); } @@ -758,14 +732,11 @@ impl EventProcessor { // Verify that the owner from the contract event is the one who registered the validator // (which is stored as the cluster's owner in our database) if &cluster.owner != owner { - error!( - validator_pubkey = %validator_pubkey, - registered_owner = ?cluster.owner, - contract_event_owner = ?owner, - "Owner mismatch: the address in the contract event is not the validator's registered owner" - ); - return Err(ExecutionError::InvalidEvent( - "Contract event owner does not match the validator's registered owner".to_string(), + return Err(ExecutionError::invalid_validator_event( + Some(validator_pubkey.to_string()), + Some(*computed_cluster_id), + "Contract event owner does not match the validator's registered owner", + None, )); } diff --git a/anchor/eth/src/network_actions.rs b/anchor/eth/src/network_actions.rs index 1dcf569a2..76ccab544 100644 --- a/anchor/eth/src/network_actions.rs +++ b/anchor/eth/src/network_actions.rs @@ -30,6 +30,18 @@ pub enum NetworkAction { NoOp, } +// Simple wrapper to make String compatible with Error trait +#[derive(Debug)] +struct StringError(String); + +impl std::fmt::Display for StringError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::error::Error for StringError {} + /// Parse a network log into an action to be executed impl TryFrom<&Log> for NetworkAction { type Error = ExecutionError; @@ -41,7 +53,10 @@ impl TryFrom<&Log> for NetworkAction { SSVContract::ValidatorRemoved::decode_from_log(source)?; let validator_pubkey = PublicKeyBytes::from_str(&publicKey.to_string()).map_err(|e| { - ExecutionError::InvalidEvent(format!("Failed to create PublicKey: {e}")) + ExecutionError::invalid_event( + "Failed to create PublicKey", + Some(Box::new(StringError(e.to_string()))), + ) })?; Ok(NetworkAction::StopValidator { validator_pubkey }) } @@ -76,7 +91,10 @@ impl TryFrom<&Log> for NetworkAction { SSVContract::ValidatorExited::decode_from_log(source)?; let validator_pubkey = PublicKeyBytes::from_str(&publicKey.to_string()).map_err(|e| { - ExecutionError::InvalidEvent(format!("Failed to create PublicKey: {e}")) + ExecutionError::invalid_event( + "Failed to create PublicKey", + Some(Box::new(StringError(e.to_string()))), + ) })?; Ok(NetworkAction::ExitValidator { validator_pubkey }) } diff --git a/anchor/eth/src/util.rs b/anchor/eth/src/util.rs index 59538c5c4..83cd54356 100644 --- a/anchor/eth/src/util.rs +++ b/anchor/eth/src/util.rs @@ -11,7 +11,7 @@ use reqwest::Client; use sensitive_url::SensitiveUrl; use ssv_types::{ClusterId, ENCRYPTED_KEY_LENGTH, OperatorId, Share, ValidatorMetadata}; use tower::ServiceBuilder; -use tracing::{debug, error}; +use tracing::debug; use types::{Graffiti, PublicKeyBytes, Signature}; use crate::{error::ExecutionError, sync::MAX_OPERATORS}; @@ -21,6 +21,18 @@ const SIGNATURE_LENGTH: usize = 96; // phase0.PublicKeyLength const PUBLIC_KEY_LENGTH: usize = 48; +// Simple wrapper to make String compatible with Error trait +#[derive(Debug)] +struct StringError(String); + +impl std::fmt::Display for StringError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::error::Error for StringError {} + // Parses shares from a ValidatorAdded event // Event contains a bytes stream of the form // [signature | public keys | encrypted keys]. @@ -146,36 +158,48 @@ pub fn validate_operators( cluster_id: &ClusterId, network_state: &NetworkState, ) -> Result<(), ExecutionError> { - debug!(cluster_id = ?cluster_id, "Validating operators"); - let num_operators = operator_ids.len(); // make sure there is a valid number of operators if num_operators > MAX_OPERATORS { - return Err(ExecutionError::InvalidEvent(format!( - "Failed to validate operators: validator has too many operators: {num_operators}" - ))); + return Err(ExecutionError::invalid_cluster_event( + Some(*cluster_id), + None, + format!( + "Failed to validate operators: validator has too many operators: {num_operators}" + ), + None, + )); } if num_operators == 0 { - return Err(ExecutionError::InvalidEvent( - "Failed to validate operators: validator has no operators".to_string(), + return Err(ExecutionError::invalid_cluster_event( + Some(*cluster_id), + None, + "Failed to validate operators: validator has no operators", + None, )); } // make sure count is valid let threshold = (num_operators - 1) / 3; if (num_operators - 1) % 3 != 0 || !(1..=4).contains(&threshold) { - return Err(ExecutionError::InvalidEvent(format!( - "Given {num_operators} operators. Cannot build a 3f+1 quorum" - ))); + return Err(ExecutionError::invalid_cluster_event( + Some(*cluster_id), + None, + format!("Given {num_operators} operators. Cannot build a 3f+1 quorum"), + None, + )); } // make sure there are no duplicates let mut seen = HashSet::new(); let are_duplicates = !operator_ids.iter().all(|x| seen.insert(x)); if are_duplicates { - return Err(ExecutionError::InvalidEvent( - "Operator IDs contain duplicates".to_string(), + return Err(ExecutionError::invalid_cluster_event( + Some(*cluster_id), + None, + "Operator IDs contain duplicates", + None, )); } @@ -183,7 +207,6 @@ pub fn validate_operators( .iter() .any(|id| !network_state.operator_exists(id)) { - error!(cluster_id = ?cluster_id, "One or more operators do not exist"); return Err(ExecutionError::Database( "One or more operators do not exist".to_string(), )); @@ -201,7 +224,10 @@ pub fn parse_validator_pubkey(pubkey: &Bytes) -> Result Date: Mon, 2 Jun 2025 22:17:04 +0200 Subject: [PATCH 2/5] introduce RsaParseError --- anchor/common/ssv_types/src/lib.rs | 2 +- anchor/common/ssv_types/src/operator.rs | 8 ++++-- anchor/common/ssv_types/src/util.rs | 34 ++++++++++++++++++++----- 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/anchor/common/ssv_types/src/lib.rs b/anchor/common/ssv_types/src/lib.rs index 39646a671..644e0fc02 100644 --- a/anchor/common/ssv_types/src/lib.rs +++ b/anchor/common/ssv_types/src/lib.rs @@ -2,7 +2,7 @@ pub use cluster::{Cluster, ClusterId, ClusterMember, ValidatorIndex, ValidatorMe pub use committee::{CommitteeId, CommitteeInfo}; pub use operator::{Operator, OperatorId}; pub use share::Share; -pub use util::parse_rsa; +pub use util::{RsaParseError, parse_rsa}; mod cluster; mod committee; pub mod consensus; diff --git a/anchor/common/ssv_types/src/operator.rs b/anchor/common/ssv_types/src/operator.rs index a376cc2f8..c513c1409 100644 --- a/anchor/common/ssv_types/src/operator.rs +++ b/anchor/common/ssv_types/src/operator.rs @@ -5,7 +5,7 @@ use openssl::{pkey::Public, rsa::Rsa}; use ssz_derive::{Decode, Encode}; use types::Address; -use crate::util::parse_rsa; +use crate::{RsaParseError, util::parse_rsa}; /// Unique identifier for an Operator. #[derive( @@ -41,7 +41,11 @@ pub struct Operator { impl Operator { /// Creates a new operator from its OperatorId and PEM-encoded public key string - pub fn new(pem_data: &str, operator_id: OperatorId, owner: Address) -> Result { + pub fn new( + pem_data: &str, + operator_id: OperatorId, + owner: Address, + ) -> Result { let rsa_pubkey = parse_rsa(pem_data)?; Ok(Self::new_with_pubkey(rsa_pubkey, operator_id, owner)) } diff --git a/anchor/common/ssv_types/src/util.rs b/anchor/common/ssv_types/src/util.rs index 9e9ffc60d..f3c1b8165 100644 --- a/anchor/common/ssv_types/src/util.rs +++ b/anchor/common/ssv_types/src/util.rs @@ -1,16 +1,36 @@ use base64::prelude::*; use openssl::{pkey::Public, rsa::Rsa}; +use thiserror::Error; + +/// Errors that can occur during RSA key parsing +#[derive(Error, Debug, Clone)] +pub enum RsaParseError { + #[error("Unable to decode base64 PEM data: {0}")] + Base64Decode(#[from] base64::DecodeError), + + #[error("Unable to convert decoded PEM data into a string: {0}")] + Utf8Conversion(#[from] std::string::FromUtf8Error), + + #[error("Failed to parse RSA public key: {source}")] + RsaParsing { + #[source] + source: openssl::error::ErrorStack, + }, +} + +impl From for String { + fn from(error: RsaParseError) -> Self { + error.to_string() + } +} // Parse from a RSA public key string into the associated RSA representation -pub fn parse_rsa(pem_data: &str) -> Result, String> { +pub fn parse_rsa(pem_data: &str) -> Result, RsaParseError> { // First decode the base64 data - let pem_decoded = BASE64_STANDARD - .decode(pem_data) - .map_err(|e| format!("Unable to decode base64 pem data: {e}"))?; + let pem_decoded = BASE64_STANDARD.decode(pem_data)?; // Convert the decoded data to a string - let mut pem_string = String::from_utf8(pem_decoded) - .map_err(|e| format!("Unable to convert decoded pem data into a string: {e}"))?; + let mut pem_string = String::from_utf8(pem_decoded)?; // Fix the header - replace PKCS1 header with PKCS8 header pem_string = pem_string @@ -22,7 +42,7 @@ pub fn parse_rsa(pem_data: &str) -> Result, String> { // Parse the PEM string into an RSA public key using PKCS8 format let rsa_pubkey = Rsa::public_key_from_pem(pem_string.as_bytes()) - .map_err(|e| format!("Failed to parse RSA public key: {e}"))?; + .map_err(|source| RsaParseError::RsaParsing { source })?; Ok(rsa_pubkey) } From 9adeafa3e5b53d7130f00228cfefc7050cfb4983 Mon Sep 17 00:00:00 2001 From: diego Date: Tue, 3 Jun 2025 10:25:43 +0200 Subject: [PATCH 3/5] fix conflict --- .githooks/pre-commit | 2 +- anchor/common/ssv_types/src/util.rs | 6 ---- anchor/eth/src/error.rs | 5 ++++ anchor/eth/src/event_processor.rs | 16 ++-------- anchor/eth/src/sync.rs | 10 +++---- anchor/eth/src/util.rs | 46 ++++++++++++++--------------- 6 files changed, 35 insertions(+), 50 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index a5fe257f3..1abe953b5 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -3,7 +3,7 @@ echo "Running cargo fmt --all..." cargo +nightly fmt --all || exit 1 echo "Running cargo clippy --all..." -#cargo clippy --all || exit 1 +cargo clippy --all || exit 1 echo "Running cargo sort workspace..." cargo sort --workspace || exit 1 diff --git a/anchor/common/ssv_types/src/util.rs b/anchor/common/ssv_types/src/util.rs index f3c1b8165..b19a425bd 100644 --- a/anchor/common/ssv_types/src/util.rs +++ b/anchor/common/ssv_types/src/util.rs @@ -18,12 +18,6 @@ pub enum RsaParseError { }, } -impl From for String { - fn from(error: RsaParseError) -> Self { - error.to_string() - } -} - // Parse from a RSA public key string into the associated RSA representation pub fn parse_rsa(pem_data: &str) -> Result, RsaParseError> { // First decode the base64 data diff --git a/anchor/eth/src/error.rs b/anchor/eth/src/error.rs index 9b2a71831..1156ffa6e 100644 --- a/anchor/eth/src/error.rs +++ b/anchor/eth/src/error.rs @@ -1,5 +1,7 @@ use thiserror::Error; +use crate::util::ShareParseError; + // Custom execution integration layer errors #[derive(Debug, Error)] pub enum ExecutionError { @@ -59,6 +61,9 @@ pub enum ExecutionError { #[error("Database error: {0}")] Database(String), + + #[error("Share parse error: {0}")] + ShareParseError(#[from] ShareParseError), } impl ExecutionError { diff --git a/anchor/eth/src/event_processor.rs b/anchor/eth/src/event_processor.rs index 6d31e7acc..113cc10a4 100644 --- a/anchor/eth/src/event_processor.rs +++ b/anchor/eth/src/event_processor.rs @@ -1,17 +1,5 @@ use std::sync::Arc; -/// Simple wrapper to make String compatible with Error trait -#[derive(Debug)] -struct StringError(String); - -impl std::fmt::Display for StringError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - -impl std::error::Error for StringError {} - use alloy::{primitives::Address, rpc::types::Log, sol_types::SolEvent}; use database::{NetworkDatabase, UniqueIndex}; use eth2::types::PublicKeyBytes; @@ -319,7 +307,7 @@ impl EventProcessor { Some(validator_pubkey.to_string()), Some(cluster_id), "Failed to parse shares", - Some(Box::new(StringError(e))), + Some(Box::new(e)), ) })?; @@ -582,7 +570,7 @@ impl EventProcessor { let block_timestamp = log .block_timestamp - .ok_or_else(|| ExecutionError::InvalidEvent("Block timestamp not set".to_string()))?; + .ok_or_else(|| ExecutionError::invalid_event("Block timestamp not set", None))?; let validator_index = match self.get_validator_index(&validator_pubkey) { Ok(Some(value)) => value, diff --git a/anchor/eth/src/sync.rs b/anchor/eth/src/sync.rs index 3c0f20d25..0c84c0826 100644 --- a/anchor/eth/src/sync.rs +++ b/anchor/eth/src/sync.rs @@ -130,7 +130,7 @@ impl SsvEventSyncer { exit_tx: ExitTx, config: Config, ) -> Result { - info!("Creating new SSV Event Syncer"); + debug!("Creating new SSV Event Syncer"); // Construct the rpc provider let rpc_client = http_with_timeout_and_fallback(&config.http_urls); @@ -576,9 +576,9 @@ impl SsvEventSyncer { continue; } - let block_number = log.block_number.ok_or_else(|| { - ExecutionError::InvalidEvent("Block number not available".to_string()) - })?; + let block_number = log + .block_number + .ok_or_else(|| ExecutionError::invalid_event("Block number not available", None))?; if let Some(timestamp) = block_timestamp_cache.get(&block_number) { log.block_timestamp = Some(*timestamp); @@ -595,7 +595,7 @@ impl SsvEventSyncer { block } Ok(None) => { - return Err(ExecutionError::InvalidEvent("Block not found".to_string())); + return Err(ExecutionError::invalid_event("Block not found", None)); } Err(e) => { return Err(ExecutionError::RpcError(format!( diff --git a/anchor/eth/src/util.rs b/anchor/eth/src/util.rs index 83cd54356..791ff247c 100644 --- a/anchor/eth/src/util.rs +++ b/anchor/eth/src/util.rs @@ -10,28 +10,30 @@ use database::NetworkState; use reqwest::Client; use sensitive_url::SensitiveUrl; use ssv_types::{ClusterId, ENCRYPTED_KEY_LENGTH, OperatorId, Share, ValidatorMetadata}; +use thiserror::Error; use tower::ServiceBuilder; use tracing::debug; use types::{Graffiti, PublicKeyBytes, Signature}; -use crate::{error::ExecutionError, sync::MAX_OPERATORS}; +use crate::{error::ExecutionError, sync::MAX_OPERATORS, util::ShareParseError::InvalidLength}; // phase0.SignatureLength const SIGNATURE_LENGTH: usize = 96; // phase0.PublicKeyLength const PUBLIC_KEY_LENGTH: usize = 48; -// Simple wrapper to make String compatible with Error trait -#[derive(Debug)] -struct StringError(String); +/// Errors that can occur during share parsing +#[derive(Error, Debug)] +pub enum ShareParseError { + #[error("Share data has invalid length: expected {expected}, got {actual}")] + InvalidLength { expected: usize, actual: usize }, -impl std::fmt::Display for StringError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} + #[error("Failed to create public key: {0}")] + PublicKeyCreation(String), -impl std::error::Error for StringError {} + #[error("Encrypted key has wrong length")] + EncryptedKeyLength, +} // Parses shares from a ValidatorAdded event // Event contains a bytes stream of the form @@ -41,7 +43,7 @@ pub fn parse_shares( operator_ids: &[OperatorId], cluster_id: &ClusterId, validator_pubkey: &PublicKeyBytes, -) -> Result<(Vec, Vec), String> { +) -> Result<(Vec, Vec), ShareParseError> { let operator_count = operator_ids.len(); // Calculate offsets for different components within the shares @@ -51,11 +53,10 @@ pub fn parse_shares( // Validate total length of shares if shares_expected_length != shares.len() { - return Err(format!( - "Share data has invalid length: expected {}, got {}", - shares_expected_length, - shares.len() - )); + return Err(InvalidLength { + expected: shares_expected_length, + actual: shares.len(), + }); } // Extract all of the components @@ -78,12 +79,12 @@ pub fn parse_shares( // Create public key let share_pubkey = PublicKeyBytes::from_str(&public_key_hex) - .map_err(|e| format!("Failed to create public key: {e}"))?; + .map_err(ShareParseError::PublicKeyCreation)?; // Convert encrypted key into fixed array let encrypted_array: [u8; 256] = encrypted .try_into() - .map_err(|_| "Encrypted key has wrong length".to_string())?; + .map_err(|_| ShareParseError::EncryptedKeyLength)?; Ok(Share { validator_pubkey: *validator_pubkey, @@ -93,7 +94,7 @@ pub fn parse_shares( encrypted_private_key: encrypted_array, }) }) - .collect::, String>>()?; + .collect::, ShareParseError>>()?; Ok((signature, shares)) } @@ -216,7 +217,7 @@ pub fn validate_operators( } /// Helper function to parse validator public keys -pub fn parse_validator_pubkey(pubkey: &Bytes) -> Result { +pub fn parse_validator_pubkey(pubkey: &Bytes) -> Result { let pubkey_str = pubkey.to_string(); PublicKeyBytes::from_str(&pubkey_str).map_err(|e| { debug!( @@ -224,10 +225,7 @@ pub fn parse_validator_pubkey(pubkey: &Bytes) -> Result Date: Tue, 3 Jun 2025 11:05:11 +0200 Subject: [PATCH 4/5] simplify error --- anchor/eth/src/error.rs | 8 ++++---- anchor/eth/src/event_processor.rs | 20 ++++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/anchor/eth/src/error.rs b/anchor/eth/src/error.rs index 1156ffa6e..9467b88e8 100644 --- a/anchor/eth/src/error.rs +++ b/anchor/eth/src/error.rs @@ -10,8 +10,8 @@ pub enum ExecutionError { #[error("Invalid event for operator {operator_id:?} (owner: {owner:?}): {message}")] InvalidOperatorEvent { - operator_id: Option, - owner: Option, + operator_id: ssv_types::OperatorId, + owner: alloy::primitives::Address, message: String, #[source] source: Option>, @@ -68,8 +68,8 @@ pub enum ExecutionError { impl ExecutionError { pub fn invalid_operator_event( - operator_id: Option, - owner: Option, + operator_id: ssv_types::OperatorId, + owner: alloy::primitives::Address, message: impl Into, source: Option>, ) -> Self { diff --git a/anchor/eth/src/event_processor.rs b/anchor/eth/src/event_processor.rs index 113cc10a4..365f9b8d7 100644 --- a/anchor/eth/src/event_processor.rs +++ b/anchor/eth/src/event_processor.rs @@ -161,8 +161,8 @@ impl EventProcessor { // Confirm that this operator does not already exist if self.db.state().operator_exists(&operator_id) { return Err(ExecutionError::invalid_operator_event( - Some(operator_id), - Some(owner), + operator_id, + owner, format!("Operator with id {operator_id:?} already exists in database"), None, )); @@ -173,8 +173,8 @@ impl EventProcessor { let public_key_str = public_key_str.trim_start_matches("0x"); let data = hex::decode(public_key_str).map_err(|e| { ExecutionError::invalid_operator_event( - Some(operator_id), - Some(owner), + operator_id, + owner, "Failed to decode public key data from hex", Some(Box::new(e)), ) @@ -185,8 +185,8 @@ impl EventProcessor { let data = &data[64..]; let data = String::from_utf8(data.to_vec()).map_err(|e| { ExecutionError::invalid_operator_event( - Some(operator_id), - Some(owner), + operator_id, + owner, "Failed to convert to UTF8 String", Some(Box::new(e)), ) @@ -195,8 +195,8 @@ impl EventProcessor { } else { String::from_utf8(data.to_vec()).map_err(|e| { ExecutionError::invalid_operator_event( - Some(operator_id), - Some(owner), + operator_id, + owner, "Failed to convert to UTF8 String", Some(Box::new(e)), ) @@ -206,8 +206,8 @@ impl EventProcessor { // Construct the Operator and insert it into the database let operator = Operator::new(&data, operator_id, owner).map_err(|e| { ExecutionError::invalid_operator_event( - Some(operator_id), - Some(owner), + operator_id, + owner, "Failed to construct operator", Some(Box::new(e)), ) From 1b590f6cc12380b71f0e3d618953ac62a826e5ca Mon Sep 17 00:00:00 2001 From: diego Date: Tue, 3 Jun 2025 11:20:29 +0200 Subject: [PATCH 5/5] improve RsaParseError --- anchor/common/ssv_types/src/util.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/anchor/common/ssv_types/src/util.rs b/anchor/common/ssv_types/src/util.rs index b19a425bd..2d397953d 100644 --- a/anchor/common/ssv_types/src/util.rs +++ b/anchor/common/ssv_types/src/util.rs @@ -11,11 +11,8 @@ pub enum RsaParseError { #[error("Unable to convert decoded PEM data into a string: {0}")] Utf8Conversion(#[from] std::string::FromUtf8Error), - #[error("Failed to parse RSA public key: {source}")] - RsaParsing { - #[source] - source: openssl::error::ErrorStack, - }, + #[error("Failed to parse RSA public key: {0}")] + RsaParsing(#[from] openssl::error::ErrorStack), } // Parse from a RSA public key string into the associated RSA representation @@ -35,8 +32,7 @@ pub fn parse_rsa(pem_data: &str) -> Result, RsaParseError> { .replace("-----END RSA PUBLIC KEY-----", "-----END PUBLIC KEY-----"); // Parse the PEM string into an RSA public key using PKCS8 format - let rsa_pubkey = Rsa::public_key_from_pem(pem_string.as_bytes()) - .map_err(|source| RsaParseError::RsaParsing { source })?; + let rsa_pubkey = Rsa::public_key_from_pem(pem_string.as_bytes())?; Ok(rsa_pubkey) }