fix: Extend non retriable rpc messages (#696)

* fix: Extend non retriable rpc messages

* chore: PR suggestion

---------

Co-authored-by: tirumerla <tirumerla@gmail.com>

Author: zeljkoX

src/constants/evm_transaction.rs

@@ -96,3 +96,29 @@ pub fn get_evm_status_check_initial_delay() -> Duration {
 pub fn get_evm_min_age_for_hash_recovery() -> Duration {
     Duration::minutes(EVM_MIN_AGE_FOR_HASH_RECOVERY_MINUTES)
 }
+
+/// Error message patterns indicating a transaction was already submitted or its nonce consumed.
+/// Shared between the retry layer (`is_non_retriable_transaction_rpc_message`) and
+/// the domain layer (`is_already_submitted_error`).
+///
+/// Each entry is a lowercased substring to match against the RPC error message.
+pub const ALREADY_SUBMITTED_PATTERNS: &[&str] = &[
+    "nonce too low",
+    "nonce is too low",
+    "already known",
+    "replacement transaction underpriced",
+    "same hash was already imported",
+];
+
+/// Checks if a lowercased message matches "known transaction" without matching
+/// "unknown transaction" (substring false positive).
+pub fn matches_known_transaction(msg_lower: &str) -> bool {
+    if let Some(pos) = msg_lower.find("known transaction") {
+        // Reject if preceded by "un" (i.e. "unknown transaction")
+        if msg_lower[..pos].ends_with("un") {
+            return false;
+        }
+        return true;
+    }
+    false
+}

src/domain/transaction/evm/evm_transaction.rs

@@ -11,7 +11,10 @@ use std::sync::Arc;
 use tracing::{debug, error, info, warn};
 
 use crate::{
-    constants::{DEFAULT_EVM_GAS_LIMIT_ESTIMATION, GAS_LIMIT_BUFFER_MULTIPLIER},
+    constants::{
+        matches_known_transaction, ALREADY_SUBMITTED_PATTERNS, DEFAULT_EVM_GAS_LIMIT_ESTIMATION,
+        GAS_LIMIT_BUFFER_MULTIPLIER,
+    },
     domain::{
         evm::is_noop,
         transaction::{
@@ -145,11 +148,15 @@ where
 
     /// Checks if a provider error indicates the transaction was already submitted to the blockchain.
     /// This handles cases where the transaction was submitted by another instance or in a previous retry.
+    ///
+    /// Uses the shared `ALREADY_SUBMITTED_PATTERNS` from constants, consistent with
+    /// `is_non_retriable_transaction_rpc_message` in `services::provider`.
     fn is_already_submitted_error(error: &impl std::fmt::Display) -> bool {
         let error_msg = error.to_string().to_lowercase();
-        error_msg.contains("already known")
-            || error_msg.contains("nonce too low")
-            || error_msg.contains("replacement transaction underpriced")
+        ALREADY_SUBMITTED_PATTERNS
+            .iter()
+            .any(|p| error_msg.contains(p))
+            || matches_known_transaction(&error_msg)
     }
 
     /// Helper method to schedule a transaction status check job.
@@ -2774,6 +2781,22 @@ mod tests {
             &"Error: nonce too low"
         ));
 
+        // Test "nonce is too low" variants (some providers use this wording)
+        assert!(DefaultEvmTransaction::is_already_submitted_error(
+            &"nonce is too low"
+        ));
+        assert!(DefaultEvmTransaction::is_already_submitted_error(
+            &"Error: nonce is too low"
+        ));
+
+        // Test "known transaction" variants (Besu)
+        assert!(DefaultEvmTransaction::is_already_submitted_error(
+            &"known transaction"
+        ));
+        assert!(DefaultEvmTransaction::is_already_submitted_error(
+            &"Known Transaction"
+        ));
+
         // Test "replacement transaction underpriced" variants
         assert!(DefaultEvmTransaction::is_already_submitted_error(
             &"replacement transaction underpriced"
@@ -2782,6 +2805,11 @@ mod tests {
             &"Replacement Transaction Underpriced"
         ));
 
+        // Test "same hash was already imported" (OpenEthereum)
+        assert!(DefaultEvmTransaction::is_already_submitted_error(
+            &"same hash was already imported"
+        ));
+
         // Test non-matching errors
         assert!(!DefaultEvmTransaction::is_already_submitted_error(
             &"insufficient funds"
@@ -2795,6 +2823,10 @@ mod tests {
         assert!(!DefaultEvmTransaction::is_already_submitted_error(
             &"timeout"
         ));
+        // "unknown transaction" must NOT match "known transaction"
+        assert!(!DefaultEvmTransaction::is_already_submitted_error(
+            &"Unknown transaction status"
+        ));
     }
 
     /// Test submit_transaction with "already known" error in Sent status

src/services/provider/mod.rs

@@ -7,6 +7,7 @@ use tracing::debug;
 
 use crate::config::ServerConfig;
 use crate::constants::{
+    matches_known_transaction, ALREADY_SUBMITTED_PATTERNS,
     DEFAULT_HTTP_CLIENT_CONNECT_TIMEOUT_SECONDS,
     DEFAULT_HTTP_CLIENT_HTTP2_KEEP_ALIVE_INTERVAL_SECONDS,
     DEFAULT_HTTP_CLIENT_HTTP2_KEEP_ALIVE_TIMEOUT_SECONDS,
@@ -441,6 +442,20 @@ pub fn should_mark_provider_failed(error: &ProviderError) -> bool {
     }
 }
 
+/// Returns true if the RPC error message indicates a transaction-level error
+/// that should not be retried — the RPC is working correctly, but rejecting
+/// the transaction itself.
+///
+/// Uses the shared `ALREADY_SUBMITTED_PATTERNS` from constants, consistent with
+/// `is_already_submitted_error` in `domain::transaction::evm::evm_transaction`.
+fn is_non_retriable_transaction_rpc_message(message: &str) -> bool {
+    let msg_lower = message.to_lowercase();
+    ALREADY_SUBMITTED_PATTERNS
+        .iter()
+        .any(|p| msg_lower.contains(p))
+        || matches_known_transaction(&msg_lower)
+}
+
 // Errors that are retriable
 pub fn is_retriable_error(error: &ProviderError) -> bool {
     match error {
@@ -470,14 +485,16 @@ pub fn is_retriable_error(error: &ProviderError) -> bool {
         }
 
         // JSON-RPC error codes (EIP-1474)
-        ProviderError::RpcErrorCode { code, .. } => {
+        ProviderError::RpcErrorCode { code, message } => {
             match code {
-                // -32002: Resource unavailable (temporary state)
-                -32002 => true,
+                // -32002: Resource unavailable — retriable unless the message indicates a
+                // transaction-level rejection (some providers wrap nonce/tx errors here)
+                -32002 => !is_non_retriable_transaction_rpc_message(message),
                 // -32005: Limit exceeded / rate limited
                 -32005 => true,
-                // -32603: Internal error (may be temporary)
-                -32603 => true,
+                // -32603: Internal error — retriable unless the message indicates a
+                // transaction-level rejection (some providers wrap nonce/tx errors here)
+                -32603 => !is_non_retriable_transaction_rpc_message(message),
                 // -32000: Invalid input
                 -32000 => false,
                 // -32001: Resource not found
@@ -1320,4 +1337,85 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn test_is_non_retriable_transaction_rpc_message() {
+        // Positive cases: these messages should be recognized as non-retriable
+        assert!(is_non_retriable_transaction_rpc_message("nonce too low"));
+        assert!(is_non_retriable_transaction_rpc_message("Nonce Too Low"));
+        assert!(is_non_retriable_transaction_rpc_message("nonce is too low"));
+        assert!(is_non_retriable_transaction_rpc_message("already known"));
+        assert!(is_non_retriable_transaction_rpc_message(
+            "known transaction"
+        ));
+        assert!(is_non_retriable_transaction_rpc_message(
+            "Known Transaction"
+        ));
+        assert!(is_non_retriable_transaction_rpc_message(
+            "replacement transaction underpriced"
+        ));
+        assert!(is_non_retriable_transaction_rpc_message(
+            "same hash was already imported"
+        ));
+        assert!(is_non_retriable_transaction_rpc_message(
+            "Transaction nonce too low"
+        ));
+
+        // Negative cases: generic/unrelated messages should not match
+        assert!(!is_non_retriable_transaction_rpc_message("Internal error"));
+        assert!(!is_non_retriable_transaction_rpc_message("server busy"));
+        assert!(!is_non_retriable_transaction_rpc_message(""));
+        // "unknown transaction" must NOT match "known transaction"
+        assert!(!is_non_retriable_transaction_rpc_message(
+            "Unknown transaction status"
+        ));
+    }
+
+    #[test]
+    fn test_is_retriable_error_rpc_tx_errors_not_retriable() {
+        // Transaction-level messages that should NOT be retriable regardless of code
+        let non_retriable_messages = vec![
+            "Transaction nonce too low",
+            "nonce too low",
+            "nonce is too low",
+            "already known",
+            "known transaction",
+            "replacement transaction underpriced",
+            "same hash was already imported",
+        ];
+
+        // Messages that should remain retriable (generic/unrelated)
+        let retriable_messages = vec![
+            "Internal error",
+            "",
+            // "unknown transaction" must NOT false-positive on "known transaction"
+            "Unknown transaction status",
+            "Resource unavailable",
+        ];
+
+        // Both -32603 and -32002 should behave the same way for tx-level messages
+        for code in [-32603, -32002] {
+            for message in &non_retriable_messages {
+                let error = ProviderError::RpcErrorCode {
+                    code,
+                    message: message.to_string(),
+                };
+                assert!(
+                    !is_retriable_error(&error),
+                    "{code} with message {message:?} should NOT be retriable"
+                );
+            }
+
+            for message in &retriable_messages {
+                let error = ProviderError::RpcErrorCode {
+                    code,
+                    message: message.to_string(),
+                };
+                assert!(
+                    is_retriable_error(&error),
+                    "{code} with message {message:?} should be retriable"
+                );
+            }
+        }
+    }
 }

src/services/provider/retry.rs

@@ -1948,4 +1948,76 @@ mod tests {
             selected.len()
         );
     }
+
+    #[tokio::test]
+    #[serial]
+    async fn test_retry_rpc_call_non_retriable_rpc_error_no_failover() {
+        use crate::services::provider::{
+            is_retriable_error, should_mark_provider_failed, ProviderError,
+        };
+
+        RpcHealthStore::instance().clear_all();
+
+        let url1 = "http://localhost:9977";
+        let url2 = "http://localhost:9976";
+        let configs = vec![
+            RpcConfig::new(url1.to_string()),
+            RpcConfig::new(url2.to_string()),
+        ];
+        let selector = RpcSelector::new(configs, 1, 60, 60).expect("Failed to create selector");
+
+        let attempts = Arc::new(AtomicU8::new(0));
+        let attempts_clone = attempts.clone();
+
+        let provider_initializer =
+            |_url: &str| -> Result<String, ProviderError> { Ok("mock_provider".to_string()) };
+
+        // Simulate the exact error a Monad-style provider returns:
+        // JSON-RPC -32603 with "nonce too low" in the message
+        let operation = move |_provider: String| {
+            let attempts = attempts_clone.clone();
+            async move {
+                attempts.fetch_add(1, AtomicOrdering::SeqCst);
+                Err::<i32, ProviderError>(ProviderError::RpcErrorCode {
+                    code: -32603,
+                    message: "nonce too low".to_string(),
+                })
+            }
+        };
+
+        let config = RetryConfig::new(3, 1, 0, 0);
+
+        let result = retry_rpc_call(
+            &selector,
+            "test_operation",
+            is_retriable_error,
+            should_mark_provider_failed,
+            provider_initializer,
+            operation,
+            Some(config),
+        )
+        .await;
+
+        assert!(result.is_err(), "Expected Err result but got: {result:?}");
+        assert_eq!(
+            attempts.load(AtomicOrdering::SeqCst),
+            1,
+            "Operation should be called exactly once (no retries)"
+        );
+
+        // Verify neither provider was marked as failed in the health store
+        let health = RpcHealthStore::instance();
+        let meta1 = health.get_metadata(url1);
+        let meta2 = health.get_metadata(url2);
+        assert!(
+            meta1.failure_timestamps.is_empty(),
+            "Provider 1 should have 0 failures, got: {}",
+            meta1.failure_timestamps.len()
+        );
+        assert!(
+            meta2.failure_timestamps.is_empty(),
+            "Provider 2 should have 0 failures, got: {}",
+            meta2.failure_timestamps.len()
+        );
+    }
 }