Skip to content

Commit

Permalink
Verify locks exist before attempting to execute transactions.
Browse files Browse the repository at this point in the history
  • Loading branch information
mystenmark committed Nov 15, 2022
1 parent 4465503 commit 379afbe
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 17 deletions.
8 changes: 8 additions & 0 deletions crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,11 @@ impl AuthorityState {
.tap_err(|e| debug!(?tx_digest, "process_certificate failed: {e}"))
}

#[instrument(level = "trace", skip_all)]
async fn check_owned_locks(&self, owned_object_refs: &[ObjectRef]) -> SuiResult {
self.database.check_owned_locks(owned_object_refs).await
}

#[instrument(level = "trace", skip_all)]
async fn check_shared_locks(
&self,
Expand Down Expand Up @@ -1024,6 +1029,9 @@ impl AuthorityState {
let (gas_status, input_objects) =
transaction_input_checker::check_certificate_input(&self.database, certificate).await?;

let owned_object_refs = input_objects.filter_owned_objects();
self.check_owned_locks(&owned_object_refs).await?;

// At this point we need to check if any shared objects need locks,
// and whether they have them.
let shared_object_refs = input_objects.filter_shared_objects();
Expand Down
29 changes: 25 additions & 4 deletions crates/sui-core/src/authority/authority_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,14 +335,15 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {

/// When making changes, please see if check_sequenced_input_objects() below needs
/// similar changes as well.
pub fn get_missing_input_objects(
pub async fn get_missing_input_objects(
&self,
digest: &TransactionDigest,
objects: &[InputObjectKind],
) -> Result<Vec<ObjectKey>, SuiError> {
let shared_locks_cell: OnceCell<HashMap<_, _>> = OnceCell::new();

let mut missing = Vec::new();
let mut probe_lock_exists = Vec::new();
for kind in objects {
match kind {
InputObjectKind::SharedMoveObject { id, .. } => {
Expand Down Expand Up @@ -374,13 +375,27 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
}
}
InputObjectKind::ImmOrOwnedMoveObject(objref) => {
if !self.object_exists(&objref.0, objref.1)? {
if let Some(obj) = self.get_object_by_key(&objref.0, objref.1)? {
if !obj.is_immutable() {
probe_lock_exists.push(*objref);
}
} else {
missing.push(ObjectKey::from(objref));
}
}
};
}

if !probe_lock_exists.is_empty() {
match self.lock_service.locks_exist(probe_lock_exists).await {
Err(SuiError::ObjectLocksUninitialized { obj_refs }) => {
missing.extend(obj_refs.into_iter().map(ObjectKey::from));
}
Err(err) => return Err(err),
Ok(_) => (),
}
}

Ok(missing)
}

Expand Down Expand Up @@ -455,8 +470,8 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
object_ref: &ObjectRef,
) -> SuiResult<Option<VerifiedEnvelope<SenderSignedData, S>>> {
let tx_lock = self.lock_service.get_lock(*object_ref).await?.ok_or(
SuiError::ObjectLockUninitialized {
obj_ref: *object_ref,
SuiError::ObjectLocksUninitialized {
obj_refs: vec![*object_ref],
},
)?;

Expand Down Expand Up @@ -560,6 +575,12 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
.map_err(SuiError::from)
}

pub async fn check_owned_locks(&self, owned_object_refs: &[ObjectRef]) -> SuiResult {
self.lock_service
.locks_exist(owned_object_refs.into())
.await
}

/// Read a lock for a specific (transaction, shared object) pair.
pub fn all_shared_locks(
&self,
Expand Down
1 change: 1 addition & 0 deletions crates/sui-core/src/transaction_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ impl TransactionManager {
let missing = self
.authority_store
.get_missing_input_objects(&digest, &cert.data().data.input_objects()?)
.await
.expect("Are shared object locks set prior to enqueueing certificates?");

if missing.is_empty() {
Expand Down
42 changes: 31 additions & 11 deletions crates/sui-storage/src/lock_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,16 @@ impl LockServiceImpl {
/// Returns Err(TransactionLockDoesNotExist) if at least one object lock is not initialized.
fn locks_exist(&self, objects: &[ObjectRef]) -> SuiResult {
let locks = self.transaction_lock.multi_get(objects)?;
let mut missing = Vec::new();
for (lock, obj_ref) in locks.into_iter().zip(objects) {
fp_ensure!(
lock.is_some(),
SuiError::ObjectLockUninitialized { obj_ref: *obj_ref }
);
if lock.is_none() {
missing.push(*obj_ref);
}
}
fp_ensure!(
missing.is_empty(),
SuiError::ObjectLocksUninitialized { obj_refs: missing }
);
debug!(?objects, "locks_exist: all locks do exist");
Ok(())
}
Expand Down Expand Up @@ -250,14 +254,19 @@ impl LockServiceImpl {
) -> SuiResult {
debug!(?tx_digest, ?owned_input_objects, "acquire_locks");
let mut locks_to_write = Vec::new();
let mut missing = Vec::new();
let locks = self.transaction_lock.multi_get(owned_input_objects)?;

for ((i, lock), obj_ref) in locks.iter().enumerate().zip(owned_input_objects) {
// The object / version must exist, and therefore lock initialized.
let lock = lock
.as_ref()
.ok_or(SuiError::ObjectLockUninitialized { obj_ref: *obj_ref })?;
let lock = match &lock {
Some(lock) => lock,
None => {
missing.push(*obj_ref);
continue;
}
};

// Note: if lock is None, that indicates that the lock existed but was unset.
if let Some(LockInfo {
epoch: previous_epoch,
tx_digest: previous_tx_digest,
Expand Down Expand Up @@ -295,6 +304,11 @@ impl LockServiceImpl {
locks_to_write.push((obj_ref, Some(LockInfo { epoch, tx_digest })));
}

fp_ensure!(
missing.is_empty(),
SuiError::ObjectLocksUninitialized { obj_refs: missing }
);

if !locks_to_write.is_empty() {
trace!(?locks_to_write, "Writing locks");
self.transaction_lock
Expand Down Expand Up @@ -741,7 +755,9 @@ mod tests {
// Should not be able to acquire lock for uninitialized locks
assert_eq!(
ls.acquire_locks(0, &[ref1, ref2], tx1),
Err(SuiError::ObjectLockUninitialized { obj_ref: ref1 })
Err(SuiError::ObjectLocksUninitialized {
obj_refs: vec![ref1]
})
);
assert_eq!(ls.get_lock(ref1), Ok(None));

Expand All @@ -754,7 +770,9 @@ mod tests {
// Should not be able to acquire lock if not all objects initialized
assert_eq!(
ls.acquire_locks(0, &[ref1, ref2, ref3], tx1),
Err(SuiError::ObjectLockUninitialized { obj_ref: ref3 })
Err(SuiError::ObjectLocksUninitialized {
obj_refs: vec![ref3]
})
);

// Should be able to acquire lock if all objects initialized
Expand All @@ -771,7 +789,9 @@ mod tests {
assert_eq!(ls.locks_exist(&[ref1, ref2]), Ok(()));
assert_eq!(
ls.locks_exist(&[ref2, ref3]),
Err(SuiError::ObjectLockUninitialized { obj_ref: ref3 })
Err(SuiError::ObjectLocksUninitialized {
obj_refs: vec![ref3]
})
);

// Should get TransactionLockExists if try to initialize already locked object
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-types/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@ pub enum SuiError {
InvalidTxUpdate,
#[error("Attempt to re-initialize a transaction lock for objects {:?}.", refs)]
ObjectLockAlreadyInitialized { refs: Vec<ObjectRef> },
#[error("Object {obj_ref:?} lock has not been initialized.")]
ObjectLockUninitialized { obj_ref: ObjectRef },
#[error("Some object locks are uninitialized: {obj_refs:?}")]
ObjectLocksUninitialized { obj_refs: Vec<ObjectRef> },
#[error(
"Object {obj_ref:?} already locked by a different transaction: {pending_transaction:?}"
)]
Expand Down

0 comments on commit 379afbe

Please sign in to comment.