Fix a few issues in get_object_info_execute in AuthorityAggregator (#473

)
MystenLabs · Feb 18, 2022 · 4a2385f · 4a2385f · github-actions · Feb 18, 2022
1 parent ec02fe5
commit 4a2385f
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 46 deletions.
diff --git a/sui_core/src/authority_aggregator.rs b/sui_core/src/authority_aggregator.rs
@@ -331,8 +331,13 @@ where
         Ok(accumulated_state)
     }
 
-    /// Return all the information in the network about a specific object, including all versions of it
-    /// as well as all certificates that lead to the versions and the authorities at that version.
+    /// Return all the information in the network regarding the latest state of a specific object.
+    /// For each authority queried, we obtain the latest object state along with the certificate that
+    /// lead up to that state. The results from each authority are aggreated for the return.
+    /// The first part of the return value is a map from each unique (ObjectRef, TransactionDigest)
+    /// pair to the content of the object as well as a list of authorities that responded this
+    /// pair.
+    /// The second part of the return value is a map from transaction digest to the cert.
     async fn get_object_by_id(
         &self,
         object_id: ObjectID,
@@ -992,40 +997,21 @@ where
                 .map(|(name, _)| self.committee.weight(name))
                 .sum();
 
-            // If we have f+1 stake telling us of the latest version of the object, we just accept it.
+            let mut is_ok = false;
             if stake >= self.committee.validity_threshold() {
-                if obj_option.is_none() {
-                    return Ok(ObjectRead::Deleted(obj_ref));
-                } else {
-                    // safe due to check
-                    return Ok(ObjectRead::Exists(
-                        obj_ref,
-                        obj_option.unwrap(),
-                        layout_option,
-                    ));
-                }
+                // If we have f+1 stake telling us of the latest version of the object, we just accept it.
+                is_ok = true;
             } else if cert_map.contains_key(&tx_digest) {
                 // If we have less stake telling us about the latest state of an object
                 // we re-run the certificate on all authorities to ensure it is correct.
                 if let Ok(_effects) = self
                     .process_certificate(cert_map[&tx_digest].clone(), AUTHORITY_REQUEST_TIMEOUT)
                     .await
                 {
-                    let mut is_ok = false;
-
                     // The mutated or created case
                     if _effects
-                        .mutated
-                        .iter()
-                        .filter(|(oref, _)| *oref == obj_ref)
-                        .count()
-                        != 0
-                        || _effects
-                            .created
-                            .iter()
-                            .filter(|(oref, _)| *oref == obj_ref)
-                            .count()
-                            != 0
+                        .mutated_and_created()
+                        .any(|(oref, _)| *oref == obj_ref)
                     {
                         is_ok = true;
                     }
@@ -1035,30 +1021,26 @@ where
                         && _effects
                             .deleted
                             .iter()
-                            .filter(|(id, seq, _)| *id == obj_ref.0 && seq.increment() == obj_ref.1)
-                            .count()
-                            != 0
+                            .any(|(id, seq, _)| *id == obj_ref.0 && seq.increment() == obj_ref.1)
                     {
                         is_ok = true;
                     }
 
                     if !is_ok {
-                        // Report a byzantine fault here
+                        // TODO: Report a byzantine fault here
                         continue;
                     }
-
-                    // NOTE: here we should validate the object is correct from the effects
-                    if obj_option.is_none() {
+                }
+            }
+            if is_ok {
+                match obj_option {
+                    Some(obj) => {
+                        return Ok(ObjectRead::Exists(obj_ref, obj, layout_option));
+                    }
+                    None => {
                         return Ok(ObjectRead::Deleted(obj_ref));
-                    } else {
-                        // safe due to check
-                        return Ok(ObjectRead::Exists(
-                            obj_ref,
-                            obj_option.unwrap(),
-                            layout_option,
-                        ));
                     }
-                }
+                };
             }
         }
 

diff --git a/sui_core/src/safe_client.rs b/sui_core/src/safe_client.rs
@@ -98,20 +98,49 @@ impl<C> SafeClient<C> {
             certificate.check(&self.committee)?;
         }
 
-        // Check the right version is returned
-        if let Some(requested_version) = &request.request_sequence_number {
-            if let Some(object_ref) = &response.requested_object_reference {
+        // Check the right object ID and version is returned
+        if let Some((object_id, version, _)) = &response.requested_object_reference {
+            fp_ensure!(
+                object_id == &request.object_id,
+                SuiError::ByzantineAuthoritySuspicion {
+                    authority: self.address
+                }
+            );
+            if let Some(requested_version) = &request.request_sequence_number {
                 fp_ensure!(
-                    object_ref.1 == *requested_version,
+                    version == requested_version,
                     SuiError::ByzantineAuthoritySuspicion {
                         authority: self.address
                     }
                 );
             }
         }
 
-        // If an order lock is returned it is valid.
         if let Some(object_and_lock) = &response.object_and_lock {
+            if request.request_sequence_number.is_none() {
+                // If request_sequence_number is none, we are requesting the latest version.
+                match response.requested_object_reference {
+                    Some(obj_ref) => {
+                        // Since we are requesting the latest version, we should validate that if the object's
+                        // reference actually match with the one from the responded object reference.
+                        fp_ensure!(
+                            object_and_lock.object.to_object_reference() == obj_ref,
+                            SuiError::ByzantineAuthoritySuspicion {
+                                authority: self.address
+                            }
+                        );
+                    }
+                    None => {
+                        // Since we are returning the object for the latest version,
+                        // we must also have the requested object reference in the response.
+                        // Otherwise the authority has inconsistent data.
+                        return Err(SuiError::ByzantineAuthoritySuspicion {
+                            authority: self.address,
+                        });
+                    }
+                }
+            }
+
             if let Some(signed_order) = &object_and_lock.lock {
                 signed_order.check(&self.committee)?;
                 // Check it has the right signer