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] Followup to the module initializers PR (#337) #436

Merged
merged 7 commits into from
Feb 14, 2022
Merged

[FastX adapter/verifier] Followup to the module initializers PR (#337) #436

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
41366b3
Added assertions representing current set of restrictions for accessi…
awelc Feb 11, 2022
1f15275
Gas-related fixes to the module initializers implementation
awelc Feb 11, 2022
d651ddd
Do not apply gas budget to the actual publishing operation for now
awelc Feb 11, 2022
574317f
Changes to assertions
awelc Feb 11, 2022
17ea24d
Gas-related cleanup and fixes
awelc Feb 12, 2022
e74c8f5
Cleaned up gas handling during MoveVM operations
awelc Feb 14, 2022
f1a0106
Replaced a macro with a function
awelc Feb 14, 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
24 changes: 18 additions & 6 deletions sui_core/src/unit_tests/authority_tests.rs
View file
Open in desktop
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
84 changes: 50 additions & 34 deletions sui_programmability/adapter/src/adapter.rs
View file
Open in desktop
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 Down Expand Up @@ -107,7 +108,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 @@ -121,7 +122,9 @@ pub fn execute<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
mutable_ref_objects,
by_value_objects,
object_owner_map,
gas_budget,
gas_budget, // total budget
gas_budget, // budget for this single call (same as total in this case)
0, // gas_already_used
ctx,
false,
) {
Expand Down Expand Up @@ -153,21 +156,28 @@ fn execute_internal<
mutable_ref_objects: Vec<Object>,
by_value_objects: BTreeMap<AccountAddress, Object>,
object_owner_map: HashMap<AccountAddress, AccountAddress>,
gas_budget: u64,
total_gas_budget: u64, // gas budget for all operations in this transaction
current_gas_budget: u64, // gas budget for the current call operation
gas_previously_used: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems really complicated. Not sure if I fully understand why we need all these three, but execute_internal shouldn't need to worry about the total budget or previously used gas. All it needs to know is the current remaining budget, and if fail, return the amount used (which will always be <= current budget). And the caller handle the final numbers

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 think that the logic is sound, and the additional parameters are used to create the correct error values in execute_internal. I agree that this is not easy to follow, though, and I will try to refactor it further to post-process failure results coming from execute_internal to reflect total gas values in its caller, rather than handling these directly in execute_internal and mostly propagating the errors up the call chain. This should hopefully simplify execute_internal and make the whole gas handling more intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An attempt to clean this up is in. Hopefully it's easier to read (and makes some sense) - it's surprisingly subtle (particularly to uninitiated not used to thinking in these terms).

ctx: &mut TxContext,
for_publish: bool,
) -> SuiResult<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 {
let mut gas_status = match get_gas_status(cost_table, Some(current_gas_budget)).map_err(|e| {
SuiError::GasBudgetTooHigh {
error: e.to_string(),
}) {
Ok(ok) => ok,
Err(err) => {
exec_failure!(gas::MIN_MOVE_CALL_GAS, err);
}
};
}
}) {
Ok(ok) => ok,
Err(err) => {
// some work might have been already completed
// (e.g. during publishing), so charge max between the
// default value and what the computation so far had
// cost
exec_failure!(cmp::max(gas::MIN_MOVE, gas_previously_used), err);
}
};
let session = vm.new_session(state_view);
match session.execute_function_for_effects(
module_id,
Expand All @@ -190,7 +200,7 @@ fn execute_internal<
debug_assert!(change_set.accounts().is_empty());
// Input ref parameters we put in should be the same number we get out, plus one for the &mut TxContext
debug_assert!(mutable_ref_objects.len() + 1 == mutable_ref_values.len());
debug_assert!(gas_used <= gas_budget);
debug_assert!(gas_used <= current_gas_budget);
if for_publish {
// When this function is used during publishing, it
// may be executed several times, with objects being
Expand Down Expand Up @@ -218,17 +228,21 @@ fn execute_internal<
ctx,
object_owner_map,
);
let total_gas = gas::aggregate_gas(gas_used + extra_gas_used, gas_refund);
let aggregate_gas_used = 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);
// Cap total_gas by current_gas_budget in the fail case.
exec_failure!(
cmp::min(gas_previously_used + aggregate_gas_used, total_gas_budget),
err
);
}
Ok(ExecutionStatus::Success {
gas_used: total_gas,
gas_used: aggregate_gas_used,
})
}
ExecutionResult::Fail { error, gas_used } => exec_failure!(
gas_used,
// charge for all computations so far
gas_previously_used + gas_used,
SuiError::AbortedExecution {
error: error.to_string(),
}
Expand All @@ -255,7 +269,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 +278,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 +316,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 @@ -310,7 +332,9 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =
Vec::new(),
BTreeMap::new(),
HashMap::new(),
gas_budget,
current_gas_budget,
total_gas_used,
ctx,
true,
) {
Expand All @@ -332,15 +356,7 @@ pub fn publish<E: Debug, S: ResourceResolver<Error = E> + ModuleResolver<Error =

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);
}
}
Err(err) => exec_failure!(total_gas_used, err),
};

Ok(ExecutionStatus::Success {
Expand Down
20 changes: 9 additions & 11 deletions sui_programmability/adapter/src/unit_tests/adapter_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,8 +538,6 @@ fn test_publish_module_insufficient_gas() {
.1
.to_string()
.contains("Gas balance is 30, not enough to pay"));
// Minimum gas is charged.
assert_eq!(err.0, gas::MIN_MOVE_PUBLISH_GAS);
}

#[test]
Expand Down Expand Up @@ -622,7 +620,7 @@ fn test_transfer_and_freeze() {
.contains("Argument 0 is expected to be mutable, immutable object found"));
// Since it failed before VM execution, during type resolving,
// only minimum gas will be charged.
assert_eq!(err.0, gas::MIN_MOVE_CALL_GAS);
assert_eq!(err.0, gas::MIN_MOVE);

// 4. Call set_value (pass as mutable reference) should fail as well.
let obj1 = storage.read_object(&id1).unwrap();
Expand All @@ -645,7 +643,7 @@ fn test_transfer_and_freeze() {
.contains("Argument 0 is expected to be mutable, immutable object found"));
// Since it failed before VM execution, during type resolving,
// only minimum gas will be charged.
assert_eq!(err.0, gas::MIN_MOVE_CALL_GAS);
assert_eq!(err.0, gas::MIN_MOVE);
}

#[test]
Expand Down Expand Up @@ -675,7 +673,7 @@ fn test_move_call_args_type_mismatch() {
)
.unwrap();
let (gas_used, err) = status.unwrap_err();
assert_eq!(gas_used, gas::MIN_MOVE_CALL_GAS);
assert_eq!(gas_used, gas::MIN_MOVE);
assert!(err
.to_string()
.contains("Expected 3 arguments calling function 'create', but found 2"));
Expand All @@ -700,7 +698,7 @@ fn test_move_call_args_type_mismatch() {
)
.unwrap();
let (gas_used, err) = status.unwrap_err();
assert_eq!(gas_used, gas::MIN_MOVE_CALL_GAS);
assert_eq!(gas_used, gas::MIN_MOVE);
// Assert on the error message as well.
*/
}
Expand Down Expand Up @@ -734,7 +732,7 @@ fn test_move_call_incorrect_function() {
)
.unwrap();
let (gas_used, err) = status.unwrap_err();
assert_eq!(gas_used, gas::MIN_MOVE_CALL_GAS);
assert_eq!(gas_used, gas::MIN_MOVE);
assert!(err
.to_string()
.contains("Expected a module object, but found a Move object"));
Expand All @@ -754,7 +752,7 @@ fn test_move_call_incorrect_function() {
)
.unwrap();
let (gas_used, err) = status.unwrap_err();
assert_eq!(gas_used, gas::MIN_MOVE_CALL_GAS);
assert_eq!(gas_used, gas::MIN_MOVE);
assert!(err.to_string().contains(&format!(
"Could not resolve function 'foo' in module {}::ObjectBasics",
SUI_FRAMEWORK_ADDRESS
Expand Down Expand Up @@ -821,7 +819,7 @@ fn test_publish_module_linker_error() {
gas_object,
);
let err = response.unwrap().unwrap_err();
assert_eq!(err.0, gas::MIN_MOVE_PUBLISH_GAS);
assert_eq!(err.0, gas::MIN_MOVE);
let err_str = err.1.to_string();
// make sure it's a linker error
assert!(err_str.contains("VMError with status LOOKUP_FAILED"));
Expand Down Expand Up @@ -862,7 +860,7 @@ fn test_publish_module_non_zero_address() {
gas_object,
);
let err = response.unwrap().unwrap_err();
assert_eq!(err.0, gas::MIN_MOVE_PUBLISH_GAS);
assert_eq!(err.0, gas::MIN_MOVE);
let err_str = err.1.to_string();
println!("{:?}", err_str);
assert!(err_str.contains("Publishing modules with non-zero address is not allowed"));
Expand Down Expand Up @@ -963,7 +961,7 @@ fn test_simple_call() {
&natives,
"src/unit_tests/data/simple_call",
gas_object.clone(),
0,
GAS_BUDGET,
);
// TODO: to be honest I am not sure why this flush is needed but
// without it, the following assertion below fails:
Expand Down
Loading