consolidate tx input errors into TransactionInputObjectsErrors (Mys…

…tenLabs#7369) Today transaction input checking could return 100 types of `SuiError`s. This makes determining if a failure is forgivable (e.g. the txn can be retried) hard. In this PR we make `fn check_transaction_input` only return `TransactionInputObjectsErrors` to simplify the logic.
futureengco · Jan 14, 2023 · b11e3a6 · b11e3a6
1 parent 62a5c1e
commit b11e3a6
Show file tree

Hide file tree

Showing 13 changed files with 179 additions and 105 deletions.
diff --git a/crates/sui-adapter-transactional-tests/tests/entry_points/obj_vector.exp b/crates/sui-adapter-transactional-tests/tests/entry_points/obj_vector.exp
@@ -63,11 +63,11 @@ created: object(127)
 written: object(126)
 
 task 16 'run'. lines 163-166:
-Error: The transaction inputs contain duplicates ObjectRef's
+Error: Error checking transaction input objects: [DuplicateObjectRefInput]
 
 task 17 'run'. lines 168-168:
 created: object(130)
 written: object(129)
 
 task 18 'run'. lines 170-170:
-Error: The transaction inputs contain duplicates ObjectRef's
+Error: Error checking transaction input objects: [DuplicateObjectRefInput]
diff --git a/crates/sui-adapter-transactional-tests/tests/entry_points/obj_vector_generic.exp b/crates/sui-adapter-transactional-tests/tests/entry_points/obj_vector_generic.exp
@@ -60,11 +60,11 @@ created: object(126)
 written: object(125)
 
 task 15 'run'. lines 160-163:
-Error: The transaction inputs contain duplicates ObjectRef's
+Error: Error checking transaction input objects: [DuplicateObjectRefInput]
 
 task 16 'run'. lines 165-165:
 created: object(129)
 written: object(128)
 
 task 17 'run'. lines 167-167:
-Error: The transaction inputs contain duplicates ObjectRef's
+Error: Error checking transaction input objects: [DuplicateObjectRefInput]
diff --git a/crates/sui-benchmark/tests/simtest.rs b/crates/sui-benchmark/tests/simtest.rs
@@ -24,7 +24,6 @@ mod test {
     use sui_types::object::Owner;
     use test_utils::messages::get_sui_gas_object_with_wallet_context;
     use test_utils::network::TestClusterBuilder;
-    use tracing::log::error;
 
     fn test_config() -> SimConfig {
         env_config(

diff --git a/crates/sui-core/src/authority/authority_store.rs b/crates/sui-core/src/authority/authority_store.rs
@@ -275,7 +275,7 @@ impl AuthorityStore {
             .contains_key(&ObjectKey(*object_id, version))?)
     }
 
-    /// Read an object and return it, or Err(ObjectNotFound) if the object was not found.
+    /// Read an object and return it, or Ok(None) if the object was not found.
     pub fn get_object(&self, object_id: &ObjectID) -> Result<Option<Object>, SuiError> {
         self.perpetual_tables.get_object(object_id)
     }
@@ -289,6 +289,7 @@ impl AuthorityStore {
         Ok(result)
     }
 
+    // Transaction input errors should be aggregated in TransactionInputObjectsErrors
     pub fn check_input_objects(
         &self,
         objects: &[InputObjectKind],

diff --git a/crates/sui-core/src/authority/authority_store_tables.rs b/crates/sui-core/src/authority/authority_store_tables.rs
@@ -95,7 +95,7 @@ impl AuthorityPerpetualTables {
         Self::get_read_only_handle(Self::path(parent_path), None, None)
     }
 
-    /// Read an object and return it, or Err(ObjectNotFound) if the object was not found.
+    /// Read an object and return it, or Ok(None) if the object was not found.
     pub fn get_object(&self, object_id: &ObjectID) -> Result<Option<Object>, SuiError> {
         let obj_entry = self
             .objects

diff --git a/crates/sui-core/src/transaction_input_checker.rs b/crates/sui-core/src/transaction_input_checker.rs
@@ -40,14 +40,19 @@ async fn get_gas_status(
         extra_gas_object_refs,
     )
     .await
+    .map_err(SuiError::into_transaction_input_error)
 }
 
+// Note: Transaction Input related errors returned from this function
+// should be aggregated into Sui::TransactionInputObjectsErrors
 #[instrument(level = "trace", skip_all)]
 pub async fn check_transaction_input(
     store: &AuthorityStore,
     transaction: &TransactionData,
 ) -> SuiResult<(SuiGasStatus<'static>, InputObjects)> {
-    transaction.validity_check()?;
+    transaction
+        .validity_check()
+        .map_err(SuiError::into_transaction_input_error)?;
     let gas_status = get_gas_status(store, transaction).await?;
     let input_objects = transaction.input_objects()?;
     let objects = store.check_input_objects(&input_objects)?;
@@ -138,11 +143,9 @@ async fn check_gas(
         Ok(SuiGasStatus::new_unmetered())
     } else {
         let gas_object = store.get_object_by_key(&gas_payment.0, gas_payment.1)?;
-        let gas_object = gas_object.ok_or(SuiError::TransactionInputObjectsErrors {
-            errors: vec![SuiError::ObjectNotFound {
-                object_id: gas_payment.0,
-                version: Some(gas_payment.1),
-            }],
+        let gas_object = gas_object.ok_or(SuiError::ObjectNotFound {
+            object_id: gas_payment.0,
+            version: Some(gas_payment.1),
         })?;
 
         // TODO: cache this storage_gas_price in memory
@@ -167,11 +170,9 @@ async fn check_gas(
             let mut additional_objs = vec![];
             for obj_ref in additional_objects_for_gas_payment.iter() {
                 let obj = store.get_object_by_key(&obj_ref.0, obj_ref.1)?;
-                let obj = obj.ok_or(SuiError::TransactionInputObjectsErrors {
-                    errors: vec![SuiError::ObjectNotFound {
-                        object_id: gas_payment.0,
-                        version: None,
-                    }],
+                let obj = obj.ok_or(SuiError::ObjectNotFound {
+                    object_id: obj_ref.0,
+                    version: Some(obj_ref.1),
                 })?;
                 additional_objs.push(obj);
             }
@@ -186,14 +187,13 @@ async fn check_gas(
             gas::check_gas_balance(&gas_object, gas_budget, gas_price, extra_amount, vec![])?;
         }
 
-        let gas_status =
-            gas::start_gas_metering(gas_budget, computation_gas_price, storage_gas_price)?;
-        Ok(gas_status)
+        gas::start_gas_metering(gas_budget, computation_gas_price, storage_gas_price)
     }
 }
 
 /// Check all the objects used in the transaction against the database, and ensure
 /// that they are all the correct version and number.
+// Transaction input errors should be aggregated in TransactionInputObjectsErrors
 #[instrument(level = "trace", skip_all)]
 async fn check_objects(
     transaction: &TransactionData,
@@ -213,8 +213,10 @@ async fn check_objects(
         if !object.is_immutable() {
             fp_ensure!(
                 used_objects.insert(object.id().into()),
-                SuiError::InvalidBatchTransaction {
-                    error: format!("Mutable object {} cannot appear in more than one single transactions in a batch", object.id()),
+                SuiError::TransactionInputObjectsErrors { errors: vec![
+                    SuiError::InvalidBatchTransaction {
+                        error: format!("Mutable object {} cannot appear in more than one single transactions in a batch", object.id()),
+                    }]
                 }
             );
         }
@@ -254,7 +256,9 @@ async fn check_objects(
         return Err(SuiError::TransactionInputObjectsErrors { errors });
     }
     if !transaction.kind.is_genesis_tx() && all_objects.is_empty() {
-        return Err(SuiError::ObjectInputArityViolation);
+        return Err(SuiError::TransactionInputObjectsErrors {
+            errors: vec![SuiError::ObjectInputArityViolation],
+        });
     }
 
     Ok(InputObjects::new(all_objects))

diff --git a/crates/sui-core/src/unit_tests/authority_tests.rs b/crates/sui-core/src/unit_tests/authority_tests.rs
@@ -1049,12 +1049,12 @@ async fn test_handle_transfer_transaction_with_max_sequence_number() {
     let res = authority_state
         .handle_transaction(transfer_transaction)
         .await;
-    assert!(res.is_err());
+
     assert_eq!(
-        res.err(),
-        Some(SuiError::TransactionInputObjectsErrors {
-            errors: vec![SuiError::InvalidSequenceNumber],
-        })
+        res.unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        &SuiError::InvalidSequenceNumber,
     );
 }
 
@@ -1064,12 +1064,12 @@ async fn test_handle_shared_object_with_max_sequence_number() {
         construct_shared_object_transaction_with_sequence_number(Some(SequenceNumber::MAX)).await;
     // Submit the transaction and assemble a certificate.
     let response = authority.handle_transaction(transaction.clone()).await;
-    assert!(response.is_err());
     assert_eq!(
-        response.err(),
-        Some(SuiError::TransactionInputObjectsErrors {
-            errors: vec![SuiError::InvalidSequenceNumber],
-        })
+        response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        &SuiError::InvalidSequenceNumber,
     );
 }
 
@@ -1297,7 +1297,10 @@ async fn test_immutable_gas() {
         .handle_transaction(transfer_transaction.clone())
         .await;
     assert!(matches!(
-        result.unwrap_err(),
+        *result
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
         SuiError::GasObjectNotOwnedObject { .. }
     ));
 }
@@ -1326,7 +1329,10 @@ async fn test_objected_owned_gas() {
     let transaction = to_sender_signed_transaction(data, &sender_key);
     let result = authority_state.handle_transaction(transaction).await;
     assert!(matches!(
-        result.unwrap_err(),
+        *result
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
         SuiError::GasObjectNotOwnedObject { .. }
     ));
 }
@@ -1768,8 +1774,12 @@ async fn test_handle_transfer_sui_with_amount_insufficient_gas() {
     );
     let transaction = to_sender_signed_transaction(data, &sender_key);
     let result = authority_state.handle_transaction(transaction).await;
+
     assert!(matches!(
-        result.unwrap_err(),
+        *result
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
         SuiError::GasBalanceTooLowToCoverGasBudget { .. }
     ));
 }
@@ -4077,8 +4087,12 @@ async fn test_blocked_move_calls() {
         ),
         &sender_key,
     );
-    assert!(matches!(
-        authority_state.handle_transaction(tx).await,
-        Err(SuiError::BlockedMoveFunction)
-    ));
+    let response = authority_state.handle_transaction(tx).await;
+    assert_eq!(
+        *response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        SuiError::BlockedMoveFunction
+    );
 }
diff --git a/crates/sui-core/src/unit_tests/batch_transaction_tests.rs b/crates/sui-core/src/unit_tests/batch_transaction_tests.rs
@@ -161,7 +161,10 @@ async fn test_batch_contains_publish() -> anyhow::Result<()> {
     let tx = to_sender_signed_transaction(data, &sender_key);
     let response = send_and_confirm_transaction(&authority_state, tx).await;
     assert!(matches!(
-        response.unwrap_err(),
+        *response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
         SuiError::InvalidBatchTransaction { .. }
     ));
     Ok(())
@@ -191,7 +194,10 @@ async fn test_batch_contains_transfer_sui() -> anyhow::Result<()> {
     let tx = to_sender_signed_transaction(data, &sender_key);
     let response = send_and_confirm_transaction(&authority_state, tx).await;
     assert!(matches!(
-        response.unwrap_err(),
+        *response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
         SuiError::InvalidBatchTransaction { .. }
     ));
     Ok(())
@@ -236,8 +242,12 @@ async fn test_batch_insufficient_gas_balance() -> anyhow::Result<()> {
 
     let tx = to_sender_signed_transaction(data, &sender_key);
     let response = send_and_confirm_transaction(&authority_state, tx).await;
+
     assert!(matches!(
-        response.unwrap_err(),
+        *response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
         SuiError::GasBalanceTooLowToCoverGasBudget { .. }
     ));
 

diff --git a/crates/sui-core/src/unit_tests/gas_tests.rs b/crates/sui-core/src/unit_tests/gas_tests.rs
@@ -27,10 +27,14 @@ async fn test_tx_less_than_minimum_gas_budget() {
     // handling phase.
     let budget = *MIN_GAS_BUDGET - 1;
     let result = execute_transfer(*MAX_GAS_BUDGET, budget, false).await;
-    let err = result.response.unwrap_err();
+
     assert_eq!(
-        err,
-        SuiError::GasBudgetTooLow {
+        result
+            .response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        &SuiError::GasBudgetTooLow {
             gas_budget: budget,
             min_budget: *MIN_GAS_BUDGET
         }
@@ -44,10 +48,14 @@ async fn test_tx_more_than_maximum_gas_budget() {
     // handling phase.
     let budget = *MAX_GAS_BUDGET + 1;
     let result = execute_transfer(*MAX_GAS_BUDGET, budget, false).await;
-    let err = result.response.unwrap_err();
+
     assert_eq!(
-        err,
-        SuiError::GasBudgetTooHigh {
+        result
+            .response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        &SuiError::GasBudgetTooHigh {
             gas_budget: budget,
             max_budget: *MAX_GAS_BUDGET
         }
@@ -63,10 +71,13 @@ async fn test_tx_gas_balance_less_than_budget() {
     let budget = *MIN_GAS_BUDGET;
     let gas_price = 1;
     let result = execute_transfer_with_price(gas_balance, budget, gas_price, false).await;
-    let err = result.response.unwrap_err();
     assert_eq!(
-        err,
-        SuiError::GasBalanceTooLowToCoverGasBudget {
+        result
+            .response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        &SuiError::GasBalanceTooLowToCoverGasBudget {
             gas_balance: gas_balance as u128,
             gas_budget: (gas_price * budget) as u128,
             gas_price
@@ -140,10 +151,13 @@ async fn test_native_transfer_gas_price_is_used() {
     let gas_budget = *MAX_GAS_BUDGET;
     let gas_price = u64::MAX;
     let result = execute_transfer_with_price(gas_balance, gas_budget, gas_price, true).await;
-    let err = result.response.unwrap_err();
     assert_eq!(
-        err,
-        SuiError::GasBalanceTooLowToCoverGasBudget {
+        result
+            .response
+            .unwrap_err()
+            .collapse_if_single_transaction_input_error()
+            .unwrap(),
+        &SuiError::GasBalanceTooLowToCoverGasBudget {
             gas_balance: (gas_balance as u128),
             gas_budget: (gas_budget as u128) * (gas_price as u128),
             gas_price