[authority] More straight forward idempotency and cleanup of TODOs (M…

…ystenLabs#772)

* More straight forward idempotency and cleanup of TODOs
* Guard expensive db lookups with a contains call

Co-authored-by: George Danezis <[email protected]>

sui_core/src/authority.rs

@@ -290,6 +290,15 @@ impl AuthorityState {
         transaction.check_signature()?;
         let transaction_digest = transaction.digest();
 
+        // Ensure an idempotent answer.
+        if self
+            ._database
+            .signed_transaction_exists(&transaction_digest)?
+        {
+            let transaction_info = self.make_transaction_info(&transaction_digest).await?;
+            return Ok(transaction_info);
+        }
+
         let owned_objects: Vec<_> = self
             .check_locks(&transaction)
             .instrument(tracing::trace_span!("tx_check_locks"))
@@ -337,8 +346,8 @@ impl AuthorityState {
         let transaction_digest = transaction.digest();
 
         // Ensure an idempotent answer.
-        let transaction_info = self.make_transaction_info(&transaction_digest).await?;
-        if transaction_info.signed_effects.is_some() {
+        if self._database.signed_effects_exists(&transaction_digest)? {
+            let transaction_info = self.make_transaction_info(&transaction_digest).await?;
             return Ok(transaction_info);
         }
 

sui_core/src/authority/authority_store.rs

@@ -179,6 +179,23 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
         }
     }
 
+    /// Returns true if we have a signed_effects structure for this transaction digest
+    pub fn signed_effects_exists(&self, transaction_digest: &TransactionDigest) -> SuiResult<bool> {
+        self.signed_effects
+            .contains_key(transaction_digest)
+            .map_err(|e| e.into())
+    }
+
+    /// Returns true if we have a signed_effects structure for this transaction digest
+    pub fn signed_transaction_exists(
+        &self,
+        transaction_digest: &TransactionDigest,
+    ) -> SuiResult<bool> {
+        self.signed_transactions
+            .contains_key(transaction_digest)
+            .map_err(|e| e.into())
+    }
+
     /// A function that acquires all locks associated with the objects (in order to avoid deadlocks).
     fn acquire_locks(&self, _input_objects: &[ObjectRef]) -> Vec<parking_lot::MutexGuard<'_, ()>> {
         let num_locks = self.lock_table.len();
@@ -365,7 +382,7 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
     ///
     pub fn set_transaction_lock(
         &self,
-        mutable_input_objects: &[ObjectRef],
+        owned_input_objects: &[ObjectRef],
         signed_transaction: SignedTransaction,
     ) -> Result<(), SuiError> {
         let tx_digest = signed_transaction.transaction.digest();
@@ -374,7 +391,7 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
             .batch()
             .insert_batch(
                 &self.transaction_lock,
-                mutable_input_objects
+                owned_input_objects
                     .iter()
                     .map(|obj_ref| (obj_ref, Some(tx_digest))),
             )?
@@ -385,39 +402,41 @@ impl<const ALL_OBJ_VER: bool> SuiDataStore<ALL_OBJ_VER> {
 
         // This is the critical region: testing the locks and writing the
         // new locks must be atomic, and not writes should happen in between.
+        let mut need_write = false;
         {
             // Acquire the lock to ensure no one else writes when we are in here.
             // MutexGuards are unlocked on drop (ie end of this block)
-            let _mutexes = self.acquire_locks(mutable_input_objects);
+            let _mutexes = self.acquire_locks(owned_input_objects);
 
-            let locks = self.transaction_lock.multi_get(mutable_input_objects)?;
+            let locks = self.transaction_lock.multi_get(owned_input_objects)?;
 
-            for (obj_ref, lock) in mutable_input_objects.iter().zip(locks) {
+            for lock in locks {
                 // The object / version must exist, and therefore lock initialized.
                 let lock = lock.ok_or(SuiError::TransactionLockDoesNotExist)?;
 
                 if let Some(previous_tx_digest) = lock {
                     if previous_tx_digest != tx_digest {
-                        let prev_transaction = self
-                            .get_transaction_lock(obj_ref)?
-                            .expect("If we have a lock we should have a transaction.");
-
                         warn!(prev_tx_digest =? previous_tx_digest,
                               cur_tx_digest =? tx_digest,
                               "Conflicting transaction!  Lock state changed in unexpected way");
-                        // TODO: modify ConflictingTransaction to only return the transaction digest here.
                         return Err(SuiError::ConflictingTransaction {
-                            pending_transaction: prev_transaction.transaction,
+                            pending_transaction: previous_tx_digest,
                         });
                     }
+                } else {
+                    need_write |= true;
                 }
             }
 
-            // Atomic write of all locks
-            lock_batch.write().map_err(|e| e.into())
+            if need_write {
+                // Atomic write of all locks
+                lock_batch.write()?
+            }
 
             // Implicit: drop(_mutexes);
         } // End of critical region
+
+        Ok(())
     }
 
     /// Updates the state resulting from the execution of a certificate.

sui_core/tests/staged/sui.yaml

@@ -388,7 +388,7 @@ SuiError:
       ConflictingTransaction:
         STRUCT:
           - pending_transaction:
-              TYPENAME: Transaction
+              TYPENAME: TransactionDigest
     15:
       ErrorWhileProcessingTransaction: UNIT
     16:

sui_types/src/error.rs

@@ -6,7 +6,6 @@ use std::fmt::Debug;
 use thiserror::Error;
 
 use crate::base_types::*;
-use crate::messages::Transaction;
 use move_binary_format::errors::PartialVMError;
 use serde::{Deserialize, Serialize};
 
@@ -70,7 +69,9 @@ pub enum SuiError {
         expected_sequence: SequenceNumber,
     },
     #[error("Conflicting transaction already received: {pending_transaction:?}")]
-    ConflictingTransaction { pending_transaction: Transaction },
+    ConflictingTransaction {
+        pending_transaction: TransactionDigest,
+    },
     #[error("Transaction was processed but no signature was produced by authority")]
     ErrorWhileProcessingTransaction,
     #[error("Transaction transaction processing failed: {err}")]