fix(sequencer): structured JSON errors for skipped transactions

crates/full-node/sov-sequencer/src/preferred/block_executor.rs

@@ -13,9 +13,9 @@ use sov_modules_api::capabilities::{
 };
 use sov_modules_api::macros::config_value;
 use sov_modules_api::{
-    call_message_repr, Amount, BlobDataWithId, ChangeSet, DaSpec, ExecutionContext, FullyBakedTx,
-    Gas, GasSpec, HexString, KernelStateAccessor, NoOpControlFlow, RejectReason, Runtime,
-    RuntimeEventProcessor, RuntimeEventResponse, SelectedBlob, Spec, StateCheckpoint,
+    call_message_repr, Amount, BlobDataWithId, ChangeSet, DaSpec, ErrorDetail, ExecutionContext,
+    FullyBakedTx, Gas, GasSpec, HexString, KernelStateAccessor, NoOpControlFlow, RejectReason,
+    Runtime, RuntimeEventProcessor, RuntimeEventResponse, SelectedBlob, Spec, StateCheckpoint,
     TransactionReceipt, TxChangeSet, TxHash, VersionReader, VisibleSlotNumber,
 };
 use sov_modules_api::{CryptoSpec, HDTimestamp};
@@ -95,9 +95,10 @@ impl<S: Spec> RollupBlockExecutorError<S> {
                     sov_rollup_interface::stf::TxEffect::Reverted(reverted) => {
                         reverted.reason.error_detail().unwrap_or(json_obj!({}))
                     }
-                    _ => json_obj!({
-                        "error": format!("{:?}", receipt),
-                    }),
+                    sov_rollup_interface::stf::TxEffect::Skipped(skipped) => {
+                        skipped.error.error_detail().unwrap_or(json_obj!({}))
+                    }
+                    _ => json_obj!({"error": format!("{:?}", receipt)}),
                 };
                 ErrorObject {
                     status: StatusCode::BAD_REQUEST,

crates/full-node/sov-sequencer/src/preferred/nonce_buffer_task.rs

@@ -2,7 +2,8 @@ use async_trait::async_trait;
 use sov_modules_api::prelude::UnwrapInfallible;
 use sov_modules_api::rest::ApiState;
 use sov_modules_api::{
-    FullyBakedTx, Gas, Runtime, SkippedTxContents, Spec, TransactionReceipt, TxProcessingError,
+    BadNonceReason, FullyBakedTx, Gas, Runtime, SkippedTxContents, Spec, TransactionReceipt,
+    TxProcessingError,
 };
 use sov_rollup_interface::{
     crypto::CredentialId,
@@ -982,19 +983,29 @@ fn err_invalid_nonce<S: Spec, Rt: Runtime<S>>(
         InvalidNonceReason::Replaced => format!("{was_queued_msg} But a new transaction with the same nonce has arrived and replaced it in the account's queue."),
         InvalidNonceReason::AlreadyQueued => format!("An identical transaction with the same hash is already in the nonce queue; its existing status is unchanged from this request. {was_queued_msg}"),
     };
-    let error_msg = format!(
-        "Tx bad nonce for credential id: {credential_id}, expected: {expected_nonce}, but found: {tx_nonce}. {queue_error_msg}"
-    );
     tracing::debug!(
-        "Sequencer rejecting nonce transaction with error: {error_msg}, tx_hash: {tx_hash}"
+        "Sequencer rejecting nonce transaction: credential_id={credential_id}, expected_nonce={expected_nonce}, tx_nonce={tx_nonce}, reason={queue_error_msg}, tx_hash={tx_hash}"
     );
+    let bad_nonce_reason = match queue_rejection_reason {
+        InvalidNonceReason::Invalid => BadNonceReason::OutsideQueueRange,
+        InvalidNonceReason::Timeout => BadNonceReason::QueueTimeout,
+        InvalidNonceReason::EvictedBeforeExecution => BadNonceReason::QueueEvicted,
+        InvalidNonceReason::Replaced => BadNonceReason::Replaced,
+        InvalidNonceReason::AlreadyQueued => BadNonceReason::AlreadyQueued,
+    };
     let receipt = TransactionReceipt {
         tx_hash,
         body_to_save: None,
         events: Vec::new(),
         receipt: sov_rollup_interface::stf::TxEffect::Skipped(SkippedTxContents {
             gas_used: <S::Gas as Gas>::zero(),
-            error: TxProcessingError::CheckUniquenessFailed(error_msg),
+            error: TxProcessingError::CheckUniquenessFailed(
+                sov_modules_api::CheckUniquenessError::BadNonce {
+                    expected_nonce,
+                    provided_nonce: tx_nonce,
+                    reason: bad_nonce_reason,
+                },
+            ),
         }),
     };
     Ok(Err(AcceptTxError::NewTxError(DoNewTxError::ExecutorError(
@@ -1334,7 +1345,13 @@ mod tests {
                     events: Vec::new(),
                     receipt: sov_rollup_interface::stf::TxEffect::Skipped(SkippedTxContents {
                         gas_used: <TestSpec as Spec>::Gas::from([0, 0]),
-                        error: TxProcessingError::CheckUniquenessFailed(format!("Tx bad nonce for credential id: {}, expected: {executor_nonce}, but found: {tx_nonce}", CredentialId::from_bytes([1u8; 32]))),
+                        error: TxProcessingError::CheckUniquenessFailed(
+                            sov_modules_api::CheckUniquenessError::BadNonce {
+                                expected_nonce: executor_nonce,
+                                provided_nonce: tx_nonce,
+                                reason: BadNonceReason::WrongNonce,
+                            },
+                        ),
                     }),
                 };
                 return Ok(Err(AcceptTxError::NewTxError(DoNewTxError::ExecutorError(
@@ -1593,41 +1610,36 @@ mod tests {
                     result.unwrap().unwrap().await.is_ok(),
                     "Expected tx {i} to succeed"
                 ),
-                Outcome::Err(reason) => {
+                Outcome::Err(queue_reason) => {
                     let inner = result.expect("Expected Ok from TransactionReceiverResult");
                     let err =
                         inner.expect_err(&format!("Expected tx {i} to fail with nonce error"));
 
-                    // Pattern match to extract the error message
+                    let expected_reason = match queue_reason {
+                        InvalidNonceReason::Invalid => BadNonceReason::OutsideQueueRange,
+                        InvalidNonceReason::Timeout => BadNonceReason::QueueTimeout,
+                        InvalidNonceReason::EvictedBeforeExecution => BadNonceReason::QueueEvicted,
+                        InvalidNonceReason::Replaced => BadNonceReason::Replaced,
+                        InvalidNonceReason::AlreadyQueued => BadNonceReason::AlreadyQueued,
+                    };
+
                     match err {
                         AcceptTxError::NewTxError(DoNewTxError::ExecutorError(
                             RollupBlockExecutorError::UnsuccessfulTransaction { receipt },
                         )) => {
                             match receipt.receipt {
                                 sov_rollup_interface::stf::TxEffect::Skipped(contents) => {
                                     match contents.error {
-                                        TxProcessingError::CheckUniquenessFailed(msg) => {
-                                            // Verify the message contains "bad nonce"
-                                            assert!(
-                                                msg.contains("bad nonce"),
-                                                "Tx {i}: Error should mention bad nonce: \"{msg}\"",
-                                            );
-
-                                            // Verify the reason-specific substring
-                                            let expected_substring = match reason {
-                                                InvalidNonceReason::Invalid => "did not attempt to queue",
-                                                InvalidNonceReason::Timeout => "has timed out and is being evicted",
-                                                InvalidNonceReason::EvictedBeforeExecution => "dropped from the queue for an unknown reason",
-
-                                                InvalidNonceReason::Replaced => "new transaction with the same nonce has arrived and replaced it",
-                                                InvalidNonceReason::AlreadyQueued => "identical transaction with the same hash",
-                                            };
-                                            assert!(
-                                                msg.contains(expected_substring),
-                                                "Tx {i}: Error message should contain \"{expected_substring}\" for reason {reason:?}, but got: \"{msg}\"",
+                                        TxProcessingError::CheckUniquenessFailed(
+                                            sov_modules_api::CheckUniquenessError::BadNonce { reason, .. },
+                                        ) => {
+                                            assert_eq!(
+                                                reason,
+                                                expected_reason,
+                                                "Tx {i}: BadNonce reason mismatch",
                                             );
                                         }
-                                        other => panic!("Tx {i}: Expected CheckUniquenessFailed error, got: {other:?}"),
+                                        other => panic!("Tx {i}: Expected CheckUniquenessFailed(BadNonce), got: {other:?}"),
                                     }
                                 }
                                 other => panic!("Tx {i}: Expected Skipped receipt, got: {other:?}"),
@@ -1691,24 +1703,26 @@ mod tests {
                     let inner = result.expect("Expected Ok from TransactionReceiverResult");
                     let err = inner.expect_err(&format!("Expected tx {i} to fail with rejection"));
 
-                    let expected_msg = format!(
-                        "Tx bad nonce for credential id: {}, expected: {expected}, but found: {tx}",
-                        CredentialId::from_bytes([1u8; 32])
-                    );
                     assert!(
                         matches!(
                             &err,
                             AcceptTxError::NewTxError(DoNewTxError::ExecutorError(
                                 RollupBlockExecutorError::UnsuccessfulTransaction {
                                     receipt: TransactionReceipt {
                                         receipt: sov_rollup_interface::stf::TxEffect::Skipped(SkippedTxContents {
-                                            error: TxProcessingError::CheckUniquenessFailed(msg),
+                                            error: TxProcessingError::CheckUniquenessFailed(
+                                                sov_modules_api::CheckUniquenessError::BadNonce {
+                                                    expected_nonce,
+                                                    provided_nonce,
+                                                    ..
+                                                }
+                                            ),
                                             ..
                                         }),
                                         ..
                                     }
                                 },
-                            )) if msg == &expected_msg
+                            )) if *expected_nonce == expected as u64 && *provided_nonce == tx as u64
                         ),
                         "Tx {i}: Expected STF nonce rejection with expected={expected}, tx={tx}, got: {err:?}"
                     );

crates/module-system/module-implementations/sov-uniqueness/src/capabilities.rs

@@ -1,6 +1,8 @@
 use sov_modules_api::capabilities::UniquenessData;
 use sov_modules_api::ExecutionContext;
-use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, TxHash};
+use sov_modules_api::{
+    CheckUniquenessError, CredentialId, Spec, StateAccessor, StateReader, TxHash,
+};
 use sov_state::User;
 
 use crate::Uniqueness;
@@ -20,7 +22,7 @@ impl<S: Spec> Uniqueness<S> {
         transaction_hash: TxHash,
         execution_context: &ExecutionContext,
         state: &mut impl StateReader<User>,
-    ) -> anyhow::Result<()> {
+    ) -> Result<(), CheckUniquenessError> {
         match transaction_uniqueness {
             UniquenessData::Nonce(nonce) => match execution_context {
                 ExecutionContext::SequencerWarmUp => {

crates/module-system/module-implementations/sov-uniqueness/src/generations.rs

@@ -1,5 +1,7 @@
 use sov_modules_api::macros::config_value;
-use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, TxHash};
+use sov_modules_api::{
+    CheckUniquenessError, CoreModuleError, CredentialId, Spec, StateAccessor, StateReader, TxHash,
+};
 use sov_state::User;
 
 use crate::Uniqueness;
@@ -10,10 +12,11 @@ impl<S: Spec> Uniqueness<S> {
         transaction_generation: u64,
         transaction_hash: TxHash,
         state: &mut impl StateReader<User>,
-    ) -> anyhow::Result<()> {
+    ) -> Result<(), CheckUniquenessError> {
         let mut senders_buckets = self
             .generations
-            .get(credential_id, state)?
+            .get(credential_id, state)
+            .map_err(CoreModuleError::state_read)?
             .unwrap_or_default();
 
         // The "currently active" generations is the range containing the latest seen generation
@@ -24,15 +27,9 @@ impl<S: Spec> Uniqueness<S> {
 
         let past_transaction_generations: u64 = config_value!("PAST_TRANSACTION_GENERATIONS");
         let transaction_generation_cutoff: u64 = past_transaction_generations.checked_sub(1)
-            .ok_or( anyhow::anyhow!("PAST_TRANSACTION_GENERATIONS should be greater than 0. Please ensure you have set this value correctly"))?;
+            .ok_or_else(|| CheckUniquenessError::from(anyhow::anyhow!("PAST_TRANSACTION_GENERATIONS should be greater than 0. Please ensure you have set this value correctly")))?;
 
-        // If we're below the current generation range, always fail
-        // Note about the arithmetic: for a given PAST_TRANSACTION_GENERATIONS, the correct
-        // comparison is
-        // `transaction_generation <= latest_generation - PAST_TRANSACTION_GENERATIONS`.
-        // For example, at generation 10 and the last 5 being valid, the valid generations are 6,
-        // 7, 8, 9 and 10; thus we fail the transaction if `transaction_generation <= 5`.
-        // Ensure we're not below the current generation range
+        // If we're below the current generation range, reject this transaction.
         // Note about the arithmetic: for a given PAST_TRANSACTION_GENERATIONS, the correct
         // comparison is
         // `transaction_generation > latest_generation - PAST_TRANSACTION_GENERATIONS`.
@@ -43,14 +40,23 @@ impl<S: Spec> Uniqueness<S> {
         // `transaction_generation >= latest_generation - (PAST_TRANSACTION_GENERATIONS - 1)`.
         // N.B. this does add one edge case where an extra generation is accepted when `latest generation == PAST_TRANSACTION_GENERATIONS`, which is deemed acceptable.
         // which amounts to `latest_generation - (PAST_TRANSACTION_GENERATIONS - 1) <= transaction_generation`
-        anyhow::ensure!(latest_generation.saturating_sub(transaction_generation_cutoff) <= transaction_generation,
-            "Bad generation number for credential id: {credential_id}, latest known generation is: {latest_generation}, provided generation {transaction_generation} is older than cutoff limit");
+        if latest_generation.saturating_sub(transaction_generation_cutoff) > transaction_generation
+        {
+            return Err(CheckUniquenessError::BadGeneration {
+                latest_generation,
+                provided_generation: transaction_generation,
+            });
+        }
 
         // If we're within or above the active range, we check the hash against previously stored hashes in
         // the same generation, if any
         if let Some(bucket) = senders_buckets.get(&transaction_generation) {
-            anyhow::ensure!(!bucket.contains(&transaction_hash), "Duplicate transaction for credential_id {credential_id} at generation {transaction_generation}: hash {transaction_hash:} has already been seen");
-        };
+            if bucket.contains(&transaction_hash) {
+                return Err(CheckUniquenessError::DuplicateGeneration {
+                    generation: transaction_generation,
+                });
+            }
+        }
 
         // If we reach this point, the transaction is not a duplicate. However, we may still need to reject it to avoid overflowing our capacity for
         // remembering past transactions.
@@ -68,35 +74,40 @@ impl<S: Spec> Uniqueness<S> {
         let num_txs_after_increment = senders_buckets
             .values()
             .try_fold(0_u64, |acc, bucket| {
-                let bucket_len = bucket.len().try_into().map_err(|e| {
-                    anyhow::anyhow!("Overflow when converting the bucket length to u64 {e}")
+                let bucket_len: u64 = bucket.len().try_into().map_err(|e| {
+                    CheckUniquenessError::from(anyhow::anyhow!(
+                        "Overflow when converting bucket length: {e}"
+                    ))
                 })?;
-
-                let acc = acc.checked_add(bucket_len).ok_or(anyhow::anyhow!(
-                    "Overflow when adding number of transactions in bucket"
-                ))?;
-
-                anyhow::Ok(acc)
+                acc.checked_add(bucket_len).ok_or_else(|| {
+                    CheckUniquenessError::from(anyhow::anyhow!(
+                        "Overflow when summing transaction counts"
+                    ))
+                })
             })?
             .checked_add(1)
-            .ok_or(anyhow::anyhow!(
-                "Overflow when adding 1 to the number of transactions in bucket"
-            ))?;
+            .ok_or_else(|| {
+                CheckUniquenessError::from(anyhow::anyhow!(
+                    "Overflow when incrementing transaction count"
+                ))
+            })?;
 
         if num_txs_after_increment > config_value!("MAX_STORED_TX_HASHES_PER_CREDENTIAL") {
-            // If we overflow, compute the next generation number that the user needs and include it in the error message.
             let earliest_valid_bucket = senders_buckets
                 .keys()
                 .next()
                 .expect("Since `num_txs_after_increment` is greater than 0, there must be at least one non-empty bucket in the iterator");
 
-            let last_generation_before_pruning = earliest_valid_bucket
+            let next_valid_generation = earliest_valid_bucket
                 .checked_add(config_value!("PAST_TRANSACTION_GENERATIONS"))
-                .ok_or(anyhow::anyhow!(
-                    "Overflow when computing last non empty generation. This shouldn't happen. It means that a user a) fills up their existing buckets AND b) has already incremented their generation to `u64::MAX` and can no longer prune transactions. This account can no longer accept transactions."
-                ))?;
-
-            anyhow::bail!("Too many transactions for credential_id {credential_id} at generation {transaction_generation}: hash {transaction_hash:} would cause the bucket to overflow. Increment your generation number to a value greater than {last_generation_before_pruning} and try again.");
+                .ok_or_else(|| CheckUniquenessError::from(anyhow::anyhow!(
+                    "Overflow when computing next valid generation. This account can no longer accept transactions."
+                )))?;
+
+            return Err(CheckUniquenessError::GenerationCapacityExceeded {
+                current_generation: transaction_generation,
+                next_valid_generation,
+            });
         }
 
         Ok(())