Skip to content

Commit

Permalink
[fastx authority api] Fix bug MystenLabs#282 relating to `get_object_…
Browse files Browse the repository at this point in the history 
…info` of potentially deleted objects (MystenLabs#315)

* Remove unused request_received_transfers_excluding_first_nth in ObjectOrderRequest.
* Rename to pending_order
* Rename parent_certificate
* Changed the structure of ObjectInfoResponse
* Use helper function to get object

Co-authored-by: George Danezis <[email protected]>
  • Loading branch information
@gdanezis
gdanezis and George Danezis authored Jan 31, 2022
1 parent 7f7c115 commit e544299
Show file tree
Hide file tree
Showing 8 changed files with 201 additions and 162 deletions.
60 changes: 35 additions & 25 deletions fastpay/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,26 +542,32 @@ fn main() {
let obj_info_req = ObjectInfoRequest {
object_id: obj_id,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
};
let obj_info = client_state.get_object_info(obj_info_req).await.unwrap();
println!("Owner: {:#?}", obj_info.object.owner);
println!("Version: {:#?}", obj_info.object.version().value());
println!("ID: {:#?}", obj_info.object.id());
println!("Readonly: {:#?}", obj_info.object.is_read_only());
println!(
"Type: {:#?}",
obj_info
.object
.data
.type_()
.map_or("Type Unwrap Failed".to_owned(), |type_| type_
.module
.as_ident_str()
.to_string())
);
if deep {
println!("Full Info: {:#?}", obj_info.object);
if let Some(object) = client_state
.get_object_info(obj_info_req)
.await
.unwrap()
.object()
{
println!("Owner: {:#?}", object.owner);
println!("Version: {:#?}", object.version().value());
println!("ID: {:#?}", object.id());
println!("Readonly: {:#?}", object.is_read_only());
println!(
"Type: {:#?}",
object
.data
.type_()
.map_or("Type Unwrap Failed".to_owned(), |type_| type_
.module
.as_ident_str()
.to_string())
);
if deep {
println!("Full Info: {:#?}", object);
}
} else {
panic!("Object with id {:?} not found", obj_id);
}
});
}
Expand Down Expand Up @@ -589,13 +595,12 @@ fn main() {
let package_obj_info_req = ObjectInfoRequest {
object_id: config.package_obj_id,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
};
let package_obj_info = client_state
.get_object_info(package_obj_info_req)
.await
.unwrap();
let package_obj_ref = package_obj_info.object.to_object_reference();
let package_obj_ref = package_obj_info.object().unwrap().to_object_reference();

// Fetch the object info for the gas obj
let gas_obj_ref = *client_state
Expand All @@ -610,11 +615,15 @@ fn main() {
let obj_info_req = ObjectInfoRequest {
object_id: obj_id,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
};

let obj_info = client_state.get_object_info(obj_info_req).await.unwrap();
object_args_refs.push(obj_info.object.to_object_reference());
object_args_refs.push(
obj_info
.object()
.unwrap_or_else(|| panic!("Could not find object {:?}", obj_id))
.to_object_reference(),
);
}

let pure_args = convert_txn_args(&config.pure_args);
Expand Down Expand Up @@ -745,7 +754,8 @@ fn main() {
let votes: Vec<_> = responses
.into_iter()
.filter_map(|buf| {
deserialize_response(&buf[..]).and_then(|info| info.pending_confirmation)
deserialize_response(&buf[..])
.and_then(|info| info.object_and_lock.unwrap().lock)
})
.collect();
info!("Received {} valid votes.", votes.len());
Expand Down Expand Up @@ -775,7 +785,7 @@ fn main() {
.iter()
.fold(0, |acc, buf| match deserialize_response(&buf[..]) {
Some(info) => {
confirmed.insert(info.object.id());
confirmed.insert(info.object().unwrap().id());
acc + 1
}
None => acc,
Expand Down
49 changes: 25 additions & 24 deletions fastpay_core/src/authority.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,8 @@ impl AuthorityState {
&self,
request: ObjectInfoRequest,
) -> Result<ObjectInfoResponse, FastPayError> {
let requested_certificate = if let Some(seq) = request.request_sequence_number {
// Only add a certificate if it is requested
let parent_certificate = if let Some(seq) = request.request_sequence_number {
// Get the Transaction Digest that created the object
let parent_iterator = self
.get_parent_iterator(request.object_id, Some(seq))
Expand All @@ -377,12 +378,31 @@ impl AuthorityState {
} else {
None
};
self.make_object_info(request.object_id, requested_certificate)
.await

// Always attempt to return the latest version of the object and the
// current lock if any.
let object_result = self.object_state(&request.object_id).await;
let object_and_lock = match object_result {
Ok(object) => {
let lock = if object.is_read_only() {
// Read only objects have no locks.
None
} else {
self.get_order_lock(&object.to_object_reference()).await?
};

Some(ObjectResponse { object, lock })
}
Err(FastPayError::ObjectNotFound { .. }) => None,
Err(e) => return Err(e),
};

Ok(ObjectInfoResponse {
parent_certificate,
object_and_lock,
})
}
}

impl AuthorityState {
pub async fn new_with_genesis_modules(
committee: Committee,
name: AuthorityName,
Expand Down Expand Up @@ -448,25 +468,6 @@ impl AuthorityState {
self._database.get_order_info(transaction_digest)
}

/// Make an info summary of an object, and include the raw object for clients
async fn make_object_info(
&self,
object_id: ObjectID,
requested_certificate: Option<CertifiedOrder>,
) -> Result<ObjectInfoResponse, FastPayError> {
let object = self.object_state(&object_id).await?;
let lock = self
.get_order_lock(&object.to_object_reference())
.await
.or::<FastPayError>(Ok(None))?;

Ok(ObjectInfoResponse {
requested_certificate,
pending_confirmation: lock,
object,
})
}

fn make_account_info(
&self,
account: FastPayAddress,
Expand Down
2 changes: 1 addition & 1 deletion fastpay_core/src/authority/authority_store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ impl AuthorityStore {

// TODO: modify ConflictingOrder to only return the order digest here.
return Err(FastPayError::ConflictingOrder {
pending_confirmation: prev_order.order,
pending_order: prev_order.order,
});
}
}
Expand Down
56 changes: 36 additions & 20 deletions fastpay_core/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,6 @@ where
let request = ObjectInfoRequest {
object_id,
request_sequence_number: Some(inner_sequence_number),
request_received_transfers_excluding_first_nth: None,
};
// Sequentially try each authority in random order.
// TODO: Improve shuffle, different authorities might different amount of stake.
Expand All @@ -364,7 +363,7 @@ where
let result = client.handle_object_info_request(request.clone()).await;
if let Ok(response) = result {
let certificate = response
.requested_certificate
.parent_certificate
.expect("Unable to get certificate");
if certificate.check(&self.committee).is_ok() {
// BUG (https://github.com/MystenLabs/fastnft/issues/290): Orders do not have a sequence number any more, objects do.
Expand Down Expand Up @@ -456,7 +455,6 @@ where
.handle_object_info_request(ObjectInfoRequest {
object_id: object_kind.object_id(),
request_sequence_number: Some(object_kind.version()),
request_received_transfers_excluding_first_nth: None,
})
.await;

Expand All @@ -470,7 +468,7 @@ where
};

let returned_certificate = object_info
.requested_certificate
.parent_certificate
.ok_or(FastPayError::AuthorityInformationUnavailable)?;
let returned_digest = returned_certificate.order.digest();

Expand Down Expand Up @@ -590,7 +588,6 @@ where
let request = ObjectInfoRequest {
object_id,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
};
let mut authority_clients = self.authority_clients.clone();
let numbers: futures::stream::FuturesUnordered<_> = authority_clients
Expand All @@ -599,7 +596,11 @@ where
let fut = client.handle_object_info_request(request.clone());
async move {
match fut.await {
Ok(info) => Some((*name, info.object.version())),
Ok(info) => {
// TODO(https://github.com/MystenLabs/fastnft/issues/323): This assumes the object is not deleted.
let current_sequence_number = info.object().unwrap().version();
Some((*name, current_sequence_number))
}
_ => None,
}
}
Expand All @@ -620,7 +621,6 @@ where
let request = ObjectInfoRequest {
object_id,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
};
let authority_clients = self.authority_clients.clone();
let numbers: futures::stream::FuturesUnordered<_> = authority_clients
Expand All @@ -629,7 +629,10 @@ where
let fut = client.handle_object_info_request(request.clone());
async move {
match fut.await {
Ok(info) => Some((*name, Some((info.object.owner, info.object.version())))),
Ok(ObjectInfoResponse {
object_and_lock: Some(ObjectResponse { object, .. }),
..
}) => Some((*name, Some((object.owner, object.version())))),
_ => None,
}
}
Expand All @@ -642,13 +645,20 @@ where

#[cfg(test)]
async fn get_framework_object_ref(&mut self) -> Result<ObjectRef, anyhow::Error> {
self.get_object_info(ObjectInfoRequest {
object_id: FASTX_FRAMEWORK_ADDRESS,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
})
.await
.map(|response| response.object.to_object_reference())
let info = self
.get_object_info(ObjectInfoRequest {
object_id: FASTX_FRAMEWORK_ADDRESS,
request_sequence_number: None,
})
.await?;
let reference = info
.object_and_lock
.ok_or(FastPayError::ObjectNotFound {
object_id: FASTX_FRAMEWORK_ADDRESS,
})?
.object
.to_object_reference();
Ok(reference)
}

/// Execute a sequence of actions in parallel for a quorum of authorities.
Expand Down Expand Up @@ -808,11 +818,15 @@ where
let request = ObjectInfoRequest {
object_id,
request_sequence_number: None,
request_received_transfers_excluding_first_nth: None,
};
let response = client.handle_object_info_request(request).await?;

let current_sequence_number = response.object.version();
let current_sequence_number = response
.object_and_lock
.ok_or(FastPayError::ObjectNotFound { object_id })?
.object
.version();

// Download each missing certificate in reverse order using the downloader.
let mut number = target_sequence_number.decrement();
while let Ok(seq) = number {
Expand Down Expand Up @@ -1178,7 +1192,7 @@ where
let response = self
.get_object_info(ObjectInfoRequest {
object_id: *certificate.order.object_id(),
// BUG(https://github.com/MystenLabs/fastnft/issues/290):
// TODO(https://github.com/MystenLabs/fastnft/issues/290):
// This function assumes that requesting the parent cert of object seq+1 will give the cert of
// that creates the object. This is not true, as objects may be deleted and may not have a seq+1
// to look up.
Expand All @@ -1187,11 +1201,13 @@ where
// seq+1. But a lot of the client code makes the above wrong assumption, and the line above reverts
// query to the old (incorrect) behavious to not break tests everywhere.
request_sequence_number: Some(transfer.object_ref.1.increment()),
request_received_transfers_excluding_first_nth: None,
})
.await?;

// TODO(https://github.com/MystenLabs/fastnft/issues/323): This assumes object is not deleted.
let object = &response.object().unwrap();
self.object_refs
.insert(response.object.id(), response.object.to_object_reference());
.insert(object.id(), object.to_object_reference());

// Everything worked: update the local balance.
if !self.certificates.contains_key(&certificate.order.digest()) {
Expand Down
Loading

0 comments on commit e544299

Please sign in to comment.