Skip to content

Commit

Permalink
Emit a better error when shared locks have not been set (#4772)
Browse files Browse the repository at this point in the history
* Emit a better error when shared locks have not be set

* Rename SharedObjectLockNotSetObject -> SharedObjectLockNotSetError
  • Loading branch information
mystenmark committed Sep 26, 2022
1 parent 553e27d commit 71a96f6
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 25 deletions.
2 changes: 1 addition & 1 deletion crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ impl AuthorityState {
.iter()
.filter_map(|(object_id, version, _)| {
if !shared_locks.contains_key(object_id) {
Some(SuiError::SharedObjectLockNotSetObject)
Some(SuiError::SharedObjectLockNotSetError)
} else if shared_locks[object_id] != *version {
Some(SuiError::UnexpectedSequenceNumber {
object_id: *object_id,
Expand Down
36 changes: 26 additions & 10 deletions crates/sui-core/src/authority/authority_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,9 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
}

/// Get many objects by their (id, version number) key.
pub fn get_input_objects(
&self,
objects: &[InputObjectKind],
) -> Result<Vec<Option<Object>>, SuiError> {
pub fn get_input_objects(&self, objects: &[InputObjectKind]) -> Result<Vec<Object>, SuiError> {
let mut result = Vec::new();
let mut errors = Vec::new();
for kind in objects {
let obj = match kind {
InputObjectKind::MovePackage(id) | InputObjectKind::SharedMoveObject(id) => {
Expand All @@ -351,34 +349,52 @@ impl<S: Eq + Debug + Serialize + for<'de> Deserialize<'de>> SuiDataStore<S> {
self.get_object_by_key(&objref.0, objref.1)?
}
};
result.push(obj);
match obj {
Some(obj) => result.push(obj),
None => errors.push(kind.object_not_found_error()),
}
}
if !errors.is_empty() {
Err(SuiError::ObjectErrors { errors })
} else {
Ok(result)
}
Ok(result)
}

/// Get many objects by their (id, version number) key.
pub fn get_sequenced_input_objects(
&self,
digest: &TransactionDigest,
objects: &[InputObjectKind],
) -> Result<Vec<Option<Object>>, SuiError> {
) -> Result<Vec<Object>, SuiError> {
let shared_locks: HashMap<_, _> = self.all_shared_locks(digest)?.into_iter().collect();

let mut result = Vec::new();
let mut errors = Vec::new();
for kind in objects {
let obj = match kind {
InputObjectKind::MovePackage(id) => self.get_object(id)?,
InputObjectKind::SharedMoveObject(id) => match shared_locks.get(id) {
Some(version) => self.get_object_by_key(id, *version)?,
None => None,
None => {
errors.push(SuiError::SharedObjectLockNotSetError);
continue;
}
},
InputObjectKind::ImmOrOwnedMoveObject(objref) => {
self.get_object_by_key(&objref.0, objref.1)?
}
};
result.push(obj);
match obj {
Some(obj) => result.push(obj),
None => errors.push(kind.object_not_found_error()),
}
}
if !errors.is_empty() {
Err(SuiError::ObjectErrors { errors })
} else {
Ok(result)
}
Ok(result)
}

/// Read a transaction envelope via lock or returns Err(TransactionLockDoesNotExist) if the lock does not exist.
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-core/src/node_sync/node_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ where
Ok(_) => Ok(SyncStatus::CertExecuted),
Err(SuiError::ObjectNotFound { .. })
| Err(SuiError::ObjectErrors { .. })
| Err(SuiError::SharedObjectLockNotSetObject) => {
| Err(SuiError::SharedObjectLockNotSetError) => {
debug!(?digest, "cert execution failed due to missing parents");

let effects = self.get_true_effects(&cert).await?;
Expand Down
12 changes: 2 additions & 10 deletions crates/sui-core/src/transaction_input_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ where
async fn check_objects(
transaction: &TransactionData,
input_objects: Vec<InputObjectKind>,
objects: Vec<Option<Object>>,
objects: Vec<Object>,
) -> Result<InputObjects, SuiError> {
// Constructing the list of objects that could be used to authenticate other
// objects. Any mutable object (either shared or owned) can be used to
Expand All @@ -154,7 +154,7 @@ async fn check_objects(
// in more than one SingleTransactionKind. We need to ensure that their
// version number only increases once at the end of the Batch execution.
let mut owned_object_authenticators: HashSet<SuiAddress> = HashSet::new();
for object in objects.iter().flatten() {
for object in objects.iter() {
if !object.is_immutable() {
fp_ensure!(
owned_object_authenticators.insert(object.id().into()),
Expand All @@ -181,14 +181,6 @@ async fn check_objects(
.collect();

for (object_kind, object) in input_objects.into_iter().zip(objects) {
// All objects must exist in the DB.
let object = match object {
Some(object) => object,
None => {
errors.push(object_kind.object_not_found_error());
continue;
}
};
if transfer_object_ids.contains(&object.id()) {
object.ensure_public_transfer_eligible()?;
}
Expand Down
12 changes: 10 additions & 2 deletions crates/sui-core/src/unit_tests/authority_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2266,7 +2266,7 @@ async fn make_test_transaction(
unreachable!("couldn't form cert")
}

#[tokio::test]
#[tokio::test(flavor = "current_thread", start_paused = true)]
async fn shared_object() {
let (sender, keypair): (_, AccountKeyPair) = get_key_pair();

Expand Down Expand Up @@ -2300,7 +2300,15 @@ async fn shared_object() {

// Sending the certificate now fails since it was not sequenced.
let result = authority.handle_certificate(certificate.clone()).await;
assert!(matches!(result, Err(SuiError::ObjectErrors { .. })));
assert!(
matches!(
result,
Err(SuiError::ObjectErrors { ref errors })
if errors.len() == 1 && matches!(errors[0], SuiError::SharedObjectLockNotSetError)
),
"{:#?}",
result
);

// Sequence the certificate to assign a sequence number to the shared object.
send_consensus(&authority, &certificate).await;
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-types/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ pub enum SuiError {
#[error("An object that's owned by another object cannot be deleted or wrapped. It must be transferred to an account address first before deletion")]
DeleteObjectOwnedObject,
#[error("The shared locks for this transaction have not yet been set.")]
SharedObjectLockNotSetObject,
SharedObjectLockNotSetError,
#[error("Invalid Batch Transaction: {}", error)]
InvalidBatchTransaction { error: String },
#[error("Object {child_id:?} is owned by object {parent_id:?}, which is not in the input")]
Expand Down

0 comments on commit 71a96f6

Please sign in to comment.