Skip to content

Commit

Permalink
[FastX adapter/verifier] Followup to the module initializers PR (Myst…
Browse files Browse the repository at this point in the history 
…enLabs#337) (MystenLabs#436)

* Added assertions representing current set of restrictions for accessing temporary stores

* Gas-related fixes to the module initializers implementation

* Do not apply gas budget to the actual publishing operation for now

* Changes to assertions

* Gas-related cleanup and fixes

* Cleaned up gas handling during MoveVM operations

* Replaced a macro with a function
  • Loading branch information
@awelc
awelc authored Feb 14, 2022
1 parent da3368a commit 82003cd
Show file tree
Hide file tree
Showing 7 changed files with 148 additions and 105 deletions.
6 changes: 6 additions & 0 deletions sui_core/src/authority/temporary_store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ impl Storage for AuthorityTemporaryStore {
}

fn read_object(&self, id: &ObjectID) -> Option<Object> {
// there should be no read after delete
debug_assert!(self.deleted.get(id) == None);
match self.written.get(id) {
Some(x) => Some(x.clone()),
None => match self.objects.get(id) {
Expand All @@ -217,6 +219,8 @@ impl Storage for AuthorityTemporaryStore {
*/

fn write_object(&mut self, mut object: Object) {
// there should be no write after delete
debug_assert!(self.deleted.get(&object.id()) == None);
// Check it is not read-only
#[cfg(test)] // Movevm should ensure this
if let Some(existing_object) = self.read_object(&object.id()) {
Expand All @@ -234,6 +238,8 @@ impl Storage for AuthorityTemporaryStore {
}

fn delete_object(&mut self, id: &ObjectID) {
// there should be no deletion after write
debug_assert!(self.written.get(id) == None);
// Check it is not read-only
#[cfg(test)] // Movevm should ensure this
if let Some(object) = self.read_object(id) {
Expand Down
24 changes: 18 additions & 6 deletions sui_core/src/unit_tests/authority_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ async fn test_publish_dependent_module_ok() {
sender,
gas_payment_object_ref,
vec![dependent_module_bytes],
0,
MAX_GAS,
&sender_key,
);
let dependent_module_id = TxContext::new(&sender, order.digest()).fresh_id();
Expand Down Expand Up @@ -410,7 +410,13 @@ async fn test_publish_module_no_dependencies_ok() {
module.serialize(&mut module_bytes).unwrap();
let module_bytes = vec![module_bytes];
let gas_cost = calculate_module_publish_cost(&module_bytes);
let order = Order::new_module(sender, gas_payment_object_ref, module_bytes, 0, &sender_key);
let order = Order::new_module(
sender,
gas_payment_object_ref,
module_bytes,
MAX_GAS,
&sender_key,
);
let _module_object_id = TxContext::new(&sender, order.digest()).fresh_id();
let response = send_and_confirm_order(&authority, order).await.unwrap();
response.signed_effects.unwrap().effects.status.unwrap();
Expand Down Expand Up @@ -464,7 +470,7 @@ async fn test_publish_non_existing_dependent_module() {
sender,
gas_payment_object_ref,
vec![dependent_module_bytes],
0,
MAX_GAS,
&sender_key,
);

Expand Down Expand Up @@ -506,11 +512,17 @@ async fn test_publish_module_insufficient_gas() {
let mut module_bytes = Vec::new();
module.serialize(&mut module_bytes).unwrap();
let module_bytes = vec![module_bytes];
let order = Order::new_module(sender, gas_payment_object_ref, module_bytes, 0, &sender_key);
let order = Order::new_module(
sender,
gas_payment_object_ref,
module_bytes,
10,
&sender_key,
);
let response = authority.handle_order(order.clone()).await.unwrap_err();
assert!(response
.to_string()
.contains("Gas balance is 9, smaller than minimum requirement of 10 for module publish"));
.contains("Gas balance is 9, smaller than the budget 10 for move operation"));
}

#[tokio::test]
Expand Down Expand Up @@ -598,7 +610,7 @@ async fn test_handle_move_order_insufficient_budget() {
.unwrap_err();
assert!(response
.to_string()
.contains("Gas budget is 9, smaller than minimum requirement of 10 for move call"));
.contains("Gas budget is 9, smaller than minimum requirement of 10 for move operation"));
}

#[tokio::test]
Expand Down
12 changes: 9 additions & 3 deletions sui_core/src/unit_tests/client_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1140,7 +1140,9 @@ async fn test_module_publish_and_call_good() {
let mut hero_path = env!("CARGO_MANIFEST_DIR").to_owned();
hero_path.push_str("/../sui_programmability/examples/");

let pub_res = client1.publish(hero_path, gas_object_ref, 1000).await;
let pub_res = client1
.publish(hero_path, gas_object_ref, GAS_VALUE_FOR_TESTING / 2)
.await;

let (_, published_effects) = pub_res.unwrap();

Expand Down Expand Up @@ -1259,7 +1261,9 @@ async fn test_module_publish_file_path() {
// Use a path pointing to a different file
hero_path.push_str("/../sui_programmability/examples/Hero.move");

let pub_resp = client1.publish(hero_path, gas_object_ref, 1000).await;
let pub_resp = client1
.publish(hero_path, gas_object_ref, GAS_VALUE_FOR_TESTING / 2)
.await;

let (_, published_effects) = pub_resp.unwrap();

Expand Down Expand Up @@ -1544,7 +1548,9 @@ async fn test_object_store() {
let mut hero_path = env!("CARGO_MANIFEST_DIR").to_owned();
hero_path.push_str("/../sui_programmability/examples/");

let pub_res = client1.publish(hero_path, gas_object_ref, 1000).await;
let pub_res = client1
.publish(hero_path, gas_object_ref, GAS_VALUE_FOR_TESTING / 2)
.await;

let (_, published_effects) = pub_res.as_ref().unwrap();

Expand Down
104 changes: 55 additions & 49 deletions sui_programmability/adapter/src/adapter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use move_core_types::{
use move_vm_runtime::{native_functions::NativeFunctionTable, session::ExecutionResult};
use std::{
borrow::Borrow,
cmp,
collections::{BTreeMap, HashMap},
convert::TryFrom,
fmt::Debug,
Expand All @@ -46,10 +47,7 @@ pub use move_vm_runtime::move_vm::MoveVM;

macro_rules! exec_failure {
($gas:expr, $err:expr) => {
return Ok(ExecutionStatus::Failure {
gas_used: $gas,
error: Box::new($err),
})
return Ok(ExecutionStatus::new_failure($gas, $err))
};
}

Expand Down Expand Up @@ -107,7 +105,7 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
) {
Ok(ok) => ok,
Err(err) => {
exec_failure!(gas::MIN_MOVE_CALL_GAS, err);
exec_failure!(gas::MIN_MOVE, err);
}
};

Expand All @@ -125,7 +123,10 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
ctx,
false,
) {
Ok(ExecutionStatus::Success { gas_used }) => {
ExecutionStatus::Failure { gas_used, error } => {
exec_failure!(gas_used, *error)
}
ExecutionStatus::Success { gas_used } => {
match gas::try_deduct_gas(&mut gas_object, gas_used) {
Ok(()) => {
state_view.write_object(gas_object);
Expand All @@ -134,11 +135,12 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
Err(err) => exec_failure!(gas_budget, err),
}
}
Ok(ExecutionStatus::Failure { gas_used, error }) => exec_failure!(gas_used, *error),
Err(err) => Err(err),
}
}

/// This function calls into Move VM to execute a Move function
/// call. It returns gas that needs to be colleted for this particular
/// Move call both on successful and failed execution.
#[allow(clippy::too_many_arguments)]
fn execute_internal<
E: Debug,
Expand All @@ -153,20 +155,18 @@ fn execute_internal<
mutable_ref_objects: Vec<Object>,
by_value_objects: BTreeMap<AccountAddress, Object>,
object_owner_map: HashMap<AccountAddress, AccountAddress>,
gas_budget: u64,
gas_budget: u64, // gas budget for the current call operation
ctx: &mut TxContext,
for_publish: bool,
) -> SuiResult<ExecutionStatus> {
) -> ExecutionStatus {
// TODO: Update Move gas constants to reflect the gas fee on sui.
let cost_table = &move_vm_types::gas_schedule::INITIAL_COST_SCHEDULE;
let mut gas_status =
match get_gas_status(cost_table, Some(gas_budget)).map_err(|e| SuiError::GasBudgetTooHigh {
error: e.to_string(),
}) {
Ok(ok) => ok,
Err(err) => {
exec_failure!(gas::MIN_MOVE_CALL_GAS, err);
}
Err(err) => return ExecutionStatus::new_failure(gas::MIN_MOVE, err),
};
let session = vm.new_session(state_view);
match session.execute_function_for_effects(
Expand Down Expand Up @@ -203,7 +203,7 @@ fn execute_internal<
let ctx_bytes = mutable_ref_values.pop().unwrap();
let updated_ctx: TxContext = bcs::from_bytes(ctx_bytes.as_slice()).unwrap();
if let Err(err) = ctx.update_state(updated_ctx) {
exec_failure!(gas::MIN_MOVE_CALL_GAS, err);
return ExecutionStatus::new_failure(gas_used, err);
}
}

Expand All @@ -221,17 +221,18 @@ fn execute_internal<
let total_gas = gas::aggregate_gas(gas_used + extra_gas_used, gas_refund);
if let Err(err) = result {
// Cap total_gas by gas_budget in the fail case.
exec_failure!(std::cmp::min(total_gas, gas_budget), err);
return ExecutionStatus::new_failure(cmp::min(total_gas, gas_budget), err);
}
Ok(ExecutionStatus::Success {
ExecutionStatus::Success {
gas_used: total_gas,
})
}
}
ExecutionResult::Fail { error, gas_used } => exec_failure!(
// charge for all computations so far
ExecutionResult::Fail { error, gas_used } => ExecutionStatus::new_failure(
gas_used,
SuiError::AbortedExecution {
error: error.to_string(),
}
},
),
}
}
Expand All @@ -255,7 +256,7 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
Ok(ok) => ok,
Err(err) => {
exec_failure!(
gas::MIN_MOVE_PUBLISH_GAS,
gas::MIN_MOVE,
SuiError::ModuleDeserializationFailure {
error: err.to_string(),
}
Expand All @@ -264,21 +265,31 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
};

// run validation checks
let gas_cost = gas::calculate_module_publish_cost(&module_bytes);
if gas_cost > gas_budget {
exec_failure!(
gas::MIN_MOVE,
SuiError::InsufficientGas {
error: "Gas budget insufficient to publish a package".to_string(),
}
);
}
if modules.is_empty() {
exec_failure!(
gas::MIN_MOVE_PUBLISH_GAS,
gas::MIN_MOVE,
SuiError::ModulePublishFailure {
error: "Publishing empty list of modules".to_string(),
}
);
}

let package_id = match generate_package_id(&mut modules, ctx) {
Ok(ok) => ok,
Err(err) => exec_failure!(gas::MIN_MOVE_PUBLISH_GAS, err),
Err(err) => exec_failure!(gas::MIN_MOVE, err),
};
let vm = match verify_and_link(state_view, &modules, package_id, natives) {
Ok(ok) => ok,
Err(err) => exec_failure!(gas::MIN_MOVE_PUBLISH_GAS, err),
Err(err) => exec_failure!(gas::MIN_MOVE, err),
};

let mut modules_to_init = Vec::new();
Expand All @@ -292,11 +303,9 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
let package_object = Object::new_package(modules, Authenticator::Address(sender), ctx.digest());
state_view.write_object(package_object);

let gas_cost = gas::calculate_module_publish_cost(&module_bytes);
let mut total_gas_used = gas_cost;
let no_init_calls = modules_to_init.is_empty();
if !no_init_calls {
let mut current_gas_budget = gas_budget;
if !modules_to_init.is_empty() {
let mut current_gas_budget = gas_budget - total_gas_used;
for module_id in modules_to_init {
let args = vec![ctx.to_vec()];

Expand All @@ -314,35 +323,32 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
ctx,
true,
) {
Ok(ExecutionStatus::Success { gas_used }) => gas_used,
Ok(ExecutionStatus::Failure { gas_used, error }) => exec_failure!(gas_used, *error),
Err(err) => exec_failure!(gas::MIN_MOVE_CALL_GAS, err),
ExecutionStatus::Success { gas_used } => gas_used,
ExecutionStatus::Failure { gas_used, error } => {
exec_failure!(total_gas_used + gas_used, *error)
}
};
if current_gas_budget > gas_used {
current_gas_budget -= gas_used;
} else {
current_gas_budget = 0;
}
// This should never be the case as current_gas_budget
// (before the call) must be larger than gas_used (after
// the call) in order for the call to succeed in the first
// place.
debug_assert!(current_gas_budget >= gas_used);
current_gas_budget -= gas_used;
total_gas_used += gas_used;
}
}

// successful execution of both publishign operation and or all
// (optional) initializer calls
match gas::try_deduct_gas(&mut gas_object, total_gas_used) {
Ok(()) => state_view.write_object(gas_object),
Err(err) => {
if no_init_calls {
// no init calls so charge the "usual" publishing fee
exec_failure!(gas::MIN_MOVE_PUBLISH_GAS, err);
} else {
// charge the same as for failed gas deduction for Move calls
exec_failure!(gas_budget, err);
}
Ok(()) => {
state_view.write_object(gas_object);
Ok(ExecutionStatus::Success {
gas_used: total_gas_used,
})
}
};

Ok(ExecutionStatus::Success {
gas_used: total_gas_used,
})
Err(err) => exec_failure!(gas_budget, err),
}
}

const INIT_FN_NAME: &IdentStr = ident_str!("init");
Expand Down
Loading

0 comments on commit 82003cd

Please sign in to comment.