fix(sequencer): structured JSON errors for skipped transactions

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

@@ -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),
-                    }),
+                    _ => match serde_json::to_value(&receipt) {
+                        Ok(serde_json::Value::Object(m)) => m,
+                        _ => json_obj!({"error": format!("{:?}", receipt)}),
+                    },
                 };
                 ErrorObject {
                     status: StatusCode::BAD_REQUEST,

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

@@ -982,19 +982,21 @@ 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 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,
+                },
+            ),
         }),
     };
     Ok(Err(AcceptTxError::NewTxError(DoNewTxError::ExecutorError(
@@ -1334,7 +1336,12 @@ 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,
+                            },
+                        ),
                     }),
                 };
                 return Ok(Err(AcceptTxError::NewTxError(DoNewTxError::ExecutorError(
@@ -1593,7 +1600,7 @@ mod tests {
                     result.unwrap().unwrap().await.is_ok(),
                     "Expected tx {i} to succeed"
                 ),
-                Outcome::Err(reason) => {
+                Outcome::Err(_reason) => {
                     let inner = result.expect("Expected Ok from TransactionReceiverResult");
                     let err =
                         inner.expect_err(&format!("Expected tx {i} to fail with nonce error"));
@@ -1606,28 +1613,10 @@ mod tests {
                             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}\"",
-                                            );
-                                        }
-                                        other => panic!("Tx {i}: Expected CheckUniquenessFailed error, got: {other:?}"),
+                                        TxProcessingError::CheckUniquenessFailed(
+                                            sov_modules_api::CheckUniquenessError::BadNonce { .. },
+                                        ) => {}
+                                        other => panic!("Tx {i}: Expected CheckUniquenessFailed(BadNonce), got: {other:?}"),
                                     }
                                 }
                                 other => panic!("Tx {i}: Expected Skipped receipt, got: {other:?}"),
@@ -1691,24 +1680,25 @@ 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, 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(|e| CheckUniquenessError::Internal(e.to_string()))?
             .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(CheckUniquenessError::Internal("PAST_TRANSACTION_GENERATIONS should be greater than 0. Please ensure you have set this value correctly".into()))?;
 
-        // 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
+        // Ensure we're not below the current generation range.
         // 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,37 @@ 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::Internal(format!(
+                        "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(CheckUniquenessError::Internal(
+                        "Overflow when summing transaction counts".into(),
+                    ))
             })?
             .checked_add(1)
-            .ok_or(anyhow::anyhow!(
-                "Overflow when adding 1 to the number of transactions in bucket"
+            .ok_or(CheckUniquenessError::Internal(
+                "Overflow when incrementing transaction count".into(),
             ))?;
 
         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."
+                .ok_or(CheckUniquenessError::Internal(
+                    "Overflow when computing next valid generation. This account can no longer accept transactions.".into(),
                 ))?;
 
-            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.");
+            return Err(CheckUniquenessError::GenerationCapacityExceeded {
+                current_generation: transaction_generation,
+                next_valid_generation,
+            });
         }
 
         Ok(())

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

@@ -1,4 +1,4 @@
-use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader};
+use sov_modules_api::{CheckUniquenessError, CredentialId, Spec, StateAccessor, StateReader};
 use sov_state::User;
 
 use crate::Uniqueness;
@@ -8,13 +8,19 @@ impl<S: Spec> Uniqueness<S> {
         credential_id: &CredentialId,
         transaction_nonce: u64,
         state: &mut impl StateReader<User>,
-    ) -> anyhow::Result<()> {
-        let nonce = self.nonces.get(credential_id, state)?.unwrap_or_default();
-
-        anyhow::ensure!(
-            nonce == transaction_nonce,
-            "Tx bad nonce for credential id: {credential_id}, expected: {nonce}, but found: {transaction_nonce}",
-        );
+    ) -> Result<(), CheckUniquenessError> {
+        let nonce = self
+            .nonces
+            .get(credential_id, state)
+            .map_err(|e| CheckUniquenessError::Internal(e.to_string()))?
+            .unwrap_or_default();
+
+        if nonce != transaction_nonce {
+            return Err(CheckUniquenessError::BadNonce {
+                expected_nonce: nonce,
+                provided_nonce: transaction_nonce,
+            });
+        }
 
         Ok(())
     }
@@ -24,13 +30,19 @@ impl<S: Spec> Uniqueness<S> {
         credential_id: &CredentialId,
         transaction_nonce: u64,
         state: &mut impl StateReader<User>,
-    ) -> anyhow::Result<()> {
-        let nonce = self.nonces.get(credential_id, state)?.unwrap_or_default();
-
-        anyhow::ensure!(
-            nonce <= transaction_nonce,
-            "Tx bad nonce for credential id: {credential_id}, expected at least: {nonce}, but found: {transaction_nonce}",
-        );
+    ) -> Result<(), CheckUniquenessError> {
+        let nonce = self
+            .nonces
+            .get(credential_id, state)
+            .map_err(|e| CheckUniquenessError::Internal(e.to_string()))?
+            .unwrap_or_default();
+
+        if nonce > transaction_nonce {
+            return Err(CheckUniquenessError::NonceTooLow {
+                minimum_nonce: nonce,
+                provided_nonce: transaction_nonce,
+            });
+        }
 
         Ok(())
     }

crates/module-system/module-implementations/sov-uniqueness/tests/integration/call_tests.rs

@@ -1,5 +1,5 @@
 use sov_modules_api::macros::config_value;
-use sov_modules_api::{CredentialId, TxEffect};
+use sov_modules_api::{CheckUniquenessError, CredentialId, TxEffect};
 use sov_test_utils::{BatchType, SlotInput, TransactionTestCase, TxProcessingError};
 use sov_uniqueness::Uniqueness;
 
@@ -93,9 +93,9 @@ fn do_max_stored_tx_hashes_per_credential_test() {
                 panic!("Transaction should be skipped");
             };
             match skipped.error {
-                TxProcessingError::CheckUniquenessFailed(reason) => {
-                    assert!(reason.contains("Too many transactions for credential_id"));
-                }
+                TxProcessingError::CheckUniquenessFailed(
+                    CheckUniquenessError::GenerationCapacityExceeded { .. },
+                ) => {}
                 _ => {
                     panic!("Transaction should be rejected because it's not unique");
                 }

crates/module-system/sov-capabilities/src/lib.rs

@@ -278,7 +278,7 @@ impl<S: Spec, T> TransactionAuthorizer<S> for StandardProvenRollupCapabilities<'
         _context: &Context<S>,
         execution_context: &ExecutionContext,
         state: &mut impl StateReader<User>,
-    ) -> anyhow::Result<()> {
+    ) -> Result<(), sov_modules_api::CheckUniquenessError> {
         self.uniqueness.check_uniqueness(
             &auth_data.credential_id,
             auth_data.uniqueness,