Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FastX adapter/verifier] Implement Move module initializers #337

Merged
merged 10 commits into from
Feb 11, 2022
Merged

[FastX adapter/verifier] Implement Move module initializers #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ce41193
Added adapter test that can use Move code from source files
awelc Feb 1, 2022
b7c085b
Renamed directory containing Move tests data to make it more clear th…
awelc Feb 2, 2022
fda1c07
First take at running module initializers
awelc Feb 5, 2022
c11f64f
Fixes and changes to get all the current tests working in presence of…
awelc Feb 10, 2022
1d9f823
Added additional restrictions on module initializer function and rele…
awelc Feb 10, 2022
29f6f79
Added non-zero gas budget to make sure that publish calls do not (int…
awelc Feb 10, 2022
69f77fd
Make sure to test 0 gas budget when publishing without initializers
awelc Feb 10, 2022
56efb2f
Update context modified in the Move VM only for publishing-related calls
awelc Feb 10, 2022
02347ec
Cosmetic changes
awelc Feb 10, 2022
d471a59
Handle running out of gas gracefully rather than panicking due to ove…
awelc Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fastpay/src/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ impl ClientServerBenchmark {
match reply_message {
Ok(SerializedMessage::OrderResp(res)) => {
if let Some(e) = res.signed_effects {
if e.effects.status != ExecutionStatus::Success {
if matches!(e.effects.status, ExecutionStatus::Success { .. }) {
info!("Execution Error {:?}", e.effects.status);
}
}
Expand Down
17 changes: 13 additions & 4 deletions fastpay/src/wallet_commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ pub enum WalletCommands {
/// ID of the gas object for gas payment, in 20 bytes Hex string
#[structopt(long)]
gas: ObjectID,

/// gas budget for running module initializers
#[structopt(default_value = "0")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering about this--do we have any tests that try publishing from the wallet? I'd expect them to fail unless they explicitly pass a nonzero gas budget, but perhaps I'm wrong on that...

gas_budget: u64,
},

/// Call Move
Expand Down Expand Up @@ -130,11 +134,15 @@ pub enum WalletCommands {
impl WalletCommands {
pub async fn execute(&mut self, context: &mut WalletContext) -> Result<(), anyhow::Error> {
match self {
WalletCommands::Publish { path, gas } => {
WalletCommands::Publish {
path,
gas,
gas_budget,
} => {
// Find owner of gas object
let owner = context.find_owner(gas)?;
let client_state = context.get_or_create_client_state(&owner)?;
publish(client_state, path.clone(), *gas).await;
publish(client_state, path.clone(), *gas, *gas_budget).await;
}

WalletCommands::Object { id, deep } => {
Expand Down Expand Up @@ -256,17 +264,18 @@ async fn publish(
client_state: &mut ClientState<AuthorityClient>,
path: String,
gas_object_id: ObjectID,
gas_budget: u64,
) {
let gas_obj_ref = *client_state
.object_refs()
.get(&gas_object_id)
.expect("Gas object not found");

let pub_resp = client_state.publish(path, gas_obj_ref).await;
let pub_resp = client_state.publish(path, gas_obj_ref, gas_budget).await;

match pub_resp {
Ok((_, effects)) => {
if effects.status != ExecutionStatus::Success {
if !matches!(effects.status, ExecutionStatus::Success { .. }) {
error!("Error publishing module: {:#?}", effects.status);
}
show_object_effects(effects);
Expand Down
3 changes: 2 additions & 1 deletion fastpay_core/src/authority.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ impl AuthorityState {
m.modules,
m.sender,
tx_ctx,
m.gas_budget,
gas_object.clone(),
),
}?;
Expand Down Expand Up @@ -383,7 +384,7 @@ impl AuthorityState {

output_object.transfer(Authenticator::Address(recipient));
temporary_store.write_object(output_object);
Ok(ExecutionStatus::Success)
Ok(ExecutionStatus::Success { gas_used })
}

pub async fn handle_order_info_request(
Expand Down
11 changes: 7 additions & 4 deletions fastpay_core/src/authority/temporary_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,14 @@ impl Storage for AuthorityTemporaryStore {
}

fn read_object(&self, id: &ObjectID) -> Option<Object> {
match self.objects.get(id) {
match self.written.get(id) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changing the behavior of this function from "read the state of the object before executing the tx" to "read the state of the object after executing the tx, but before committing to persistent storage". This may very well be ok, but CC @lxfind @gdanezis to double check on this. Also, I think we should add a comment explaining what this does (pre-existing issue, but will help avoid some future confusion).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added @gdanezis as a reviewer (@lxfind already is one). This problem has also been reported in #394.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should return nothing if the object is in the deleted set. That being said, this only matters (for now) when running initializers and there is a limited amount of stuff that can be done in those as they are parameter-less, so perhaps it's not strictly necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to clearly define (with comments) what the current new assumptions are and what could happen.
We know that we could have read after write now. But is it possible to have delete after write? Read after delete?
Once we define these assumptions, let's also assert them in the right place to make sure they are not violated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even the original set of assumptions (e.g., why we expected only either a write or an update to the same object but not both) are a bit unclear to me, so I was not sure what is needed here.

Perhaps we should go for the most general spec? That is:

  1. An object can be only in either deleted or in written at any given time.
  2. When reading an object:
  • check if it's deleted and, if so, return None
  • return its value in written if any
  • return its value in objects if any
  • return None

Though I am not sure if we even need 1 if we have 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the current need (i.e. module initializers), I think we only have:

  1. Read after write is OK
  2. Nothing else is permitted (add assertions for read after delete, or delete after write, or write after delete)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

Some(x) => Some(x.clone()),
None => match self.object_store.get_object(id) {
Ok(o) => o,
Err(e) => panic!("Could not read object {}", e),
None => match self.objects.get(id) {
Some(x) => Some(x.clone()),
None => match self.object_store.get_object(id) {
Ok(o) => o,
Err(e) => panic!("Could not read object {}", e),
},
},
}
}
Expand Down
3 changes: 3 additions & 0 deletions fastpay_core/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ pub trait Client {
&mut self,
package_source_files_path: String,
gas_object_ref: ObjectRef,
gas_budget: u64,
) -> Result<(CertifiedOrder, OrderEffects), anyhow::Error>;

/// Get the object information
Expand Down Expand Up @@ -565,13 +566,15 @@ where
&mut self,
package_source_files_path: String,
gas_object_ref: ObjectRef,
gas_budget: u64,
) -> Result<(CertifiedOrder, OrderEffects), anyhow::Error> {
// Try to compile the package at the given path
let compiled_modules = build_move_package_to_bytes(Path::new(&package_source_files_path))?;
let move_publish_order = Order::new_module(
self.address,
gas_object_ref,
compiled_modules,
gas_budget,
&*self.secret,
);
self.execute_transaction(move_publish_order).await
Expand Down
Loading