[language] introduce InternalGasUnits and fix gas check in the adapter #7448
Conversation
bob-wilson
requested review from
tzakian,
sblackshear,
runtian-zhou,
vgao1996 and
meng-xu-cs
bob-wilson
force-pushed
the
gas-scaling
branch
from
8586828 to
5c774a8
Compare
| @@ -301,11 +301,11 @@ fn verify_simple_payment() { | |||
| .script(Script::new( | |||
| p2p_script.clone(), | |||
| vec![account_config::xus_tag()], | |||
| vec![TransactionArgument::U64(42); MAX_TRANSACTION_SIZE_IN_BYTES as usize], | |||
| vec![TransactionArgument::U8(42); MAX_TRANSACTION_SIZE_IN_BYTES as usize], | |||
There was a problem hiding this comment.
Not sure if I quite follow this change. Is this to make sure that max transaction size is getting checked before the bytecode verifier is being run?
There was a problem hiding this comment.
Nothing complicated like that. I should have left a comment in the PR description to explain. This code is just padding out the size of the transaction to trigger the EXCEEDED_MAX_TRANSACTION_SIZE error. If you want to increase the size by MAX_TRANSACTION_SIZE_IN_BYTES, then you should add that many bytes. Adding a vector of that many U64 values adds 8x more space than necessary.
| p2p_script.clone(), | ||
| vec![account_config::xus_tag()], | ||
| args.clone(), | ||
| vec![TransactionArgument::U8(42); extra_txn_bytes as usize], |
There was a problem hiding this comment.
A bit puzzled here as well about the switch to U8 from the U64.
There was a problem hiding this comment.
Same as above -- this is just padding out the size of the transaction by extra_txn_bytes
| let mut extra_txn_bytes = 0; | ||
| if gas_constants.gas_unit_scaling_factor > gas_constants.min_transaction_gas_units.get() { | ||
| extra_txn_bytes = gas_constants.large_transaction_cutoff.get() | ||
| + (gas_constants.gas_unit_scaling_factor / gas_constants.intrinsic_gas_per_byte.get()); | ||
| } |
There was a problem hiding this comment.
Nit (personal preference, so ignore if you prefer the current way):
| let mut extra_txn_bytes = 0; | |
| if gas_constants.gas_unit_scaling_factor > gas_constants.min_transaction_gas_units.get() { | |
| extra_txn_bytes = gas_constants.large_transaction_cutoff.get() | |
| + (gas_constants.gas_unit_scaling_factor / gas_constants.intrinsic_gas_per_byte.get()); | |
| } | |
| let extra_txn_bytes = if gas_constants.gas_unit_scaling_factor > gas_constants.min_transaction_gas_units.get() { | |
| extra_txn_bytes = gas_constants.large_transaction_cutoff.get() | |
| + (gas_constants.gas_unit_scaling_factor / gas_constants.intrinsic_gas_per_byte.get()); | |
| } else { | |
| 0 | |
| }; |
There was a problem hiding this comment.
I had considered that, but my preference is not at all strong. I don't mind changing it.
|
/land |
This fixes a bug (#7238) where a transaction incorrectly fails to validate in the adapter when the transaction's max_gas_amount is less than the min_transaction_gas_units gas constant. The bug is that the comparison does not take into account the gas_unit_scaling_factor gas constant. To help avoid similar bugs in the future, this introduces a new InternalGasUnits type that is distinct from the existing GasUnits type. Both are newtype wrappers around u64 so BCS-serialized values should be interchangeable between these two types (e.g., in the on-chain gas cost table). Closes: #7448
|
💔 Test Failed - ci-test-success |
|
Cluster Test Result Repro cmd: 🎉 Land-blocking cluster test passed! 👌 |
|
Sigh. The ir-testsuite/tests/testing_infra/gas_submitted_too_low.mvir test is failing. I can't wait to get rid of this technical debt from the IR tests! The history of this test is instructive. It was originally set up to check for an account with an insufficient balance to cover the transaction's max gas amount, and that is what the comment in the test still says. But, way back in June 2019, it was changed to expect a failure with MaxGasUnitsBelowMinTransactionGasUnits instead of InsufficientBalanceForTransactionFee. IMO that was a mistake -- the test was at the time not specifying the max gas field in the transaction, and the change in question changed the max gas to default to the account balance. That dropped the max gas below the minimum gas fee and completely changed what the test was checking. The test continued failing when we introduced gas scaling only because of the bug being fixed here. Since we've already got coverage for this in the verify_txn test, I'm just going to delete this redundant and obsolete IR test. @vgao1996 are you OK with that? |
There was a problem hiding this comment.
@bob-wilson yeah deleting the test sounds good to me and thanks for the accurate write up on the history of this test!
There was a problem hiding this comment.
Actually wait a sec, there may be additional issues we need to address. I ran the following tests with your changes applied and the results look a little bit weird
Test 1: balance (0) < actual gas usage (9) < max gas specified (200)
//! account: Alice, 0
// this sets up Alice's account with very low balance, which is not sufficient to cover the gas cost
//! sender: Alice
//! max-gas: 200
main() {
let x: u64;
let y: u64;
x = 3;
y = 5;
assert(move(x) + move(y) == 8, 42);
return;
}
// check: MAX_GAS_UNITS_BELOW_MIN_TRANSACTION_GAS_UNITS
Output:
Output(TransactionOutput(TransactionOutput { write_set: WriteSet(WriteSetMut { write_set: [(AccessPath { address: a4a46d1b1421502568a4a6ac326d7250, path: 01000000000000000000000000000000010b4469656d4163636f756e740b4469656d4163636f756e7400 }, Value(20975741f6b37b591141e96e0c60a57ce8a4a46d1b1421502568a4a6ac326d725001a4a46d1b1421502568a4a6ac326d725001a4a46d1b1421502568a4a6ac326d72500000000000000000180000000000000000a4a46d1b1421502568a4a6ac326d72500000000000000000180000000000000000a4a46d1b1421502568a4a6ac326d72500100000000000000))] }), events: [], gas_used: 9, status: Keep(EXECUTED) }))
Shouldn't it be OUT_OF_GAS or ?
Test 2: max-gas < actual usage (9)
//! account: Alice, 0
// this sets up Alice's account with very low balance, which is not sufficient to cover the gas cost
//! sender: Alice
//! max-gas: 5
main() {
let x: u64;
let y: u64;
x = 3;
y = 5;
assert(move(x) + move(y) == 8, 42);
return;
}
// check: MAX_GAS_UNITS_BELOW_MIN_TRANSACTION_GAS_UNITS
Output:
thread 'run_test::a.mvir' panicked at 'Unexpected offsets. major_status: OUT_OF_GASsub_status: Nonelocation: Undefinedoffsets: []', language/vm/src/errors.rs:69:17
stack backtrace:
0: rust_begin_unwind
at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/std/src/panicking.rs:483
1: std::panicking::begin_panic_fmt
at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/std/src/panicking.rs:437
2: vm::errors::VMError::into_vm_status
at /Users/vgao/libra/language/vm/src/errors.rs:69
3: diem_vm::diem_vm::charge_global_write_gas_usage::{{closure}}
at /Users/vgao/libra/language/diem-vm/src/diem_vm.rs:540
4: core::result::Result<T,E>::map_err
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:595
5: diem_vm::diem_vm::charge_global_write_gas_usage
at /Users/vgao/libra/language/diem-vm/src/diem_vm.rs:538
6: diem_vm::diem_transaction_executor::DiemVM::execute_script
at /Users/vgao/libra/language/diem-vm/src/diem_transaction_executor.rs:187
7: diem_vm::diem_transaction_executor::DiemVM::execute_user_transaction
at /Users/vgao/libra/language/diem-vm/src/diem_transaction_executor.rs:284
8: diem_vm::diem_transaction_executor::DiemVM::execute_block_impl
at /Users/vgao/libra/language/diem-vm/src/diem_transaction_executor.rs:656
9: diem_vm::diem_transaction_executor::DiemVM::execute_block_and_keep_vm_status
at /Users/vgao/libra/language/diem-vm/src/diem_transaction_executor.rs:721
10: language_e2e_tests::executor::FakeExecutor::execute_block_and_keep_vm_status
at /Users/vgao/libra/language/testing-infra/e2e-tests/src/executor.rs:226
11: functional_tests::evaluator::run_transaction
at /Users/vgao/libra/language/testing-infra/functional-tests/src/evaluator.rs:419
12: functional_tests::evaluator::eval_transaction
at /Users/vgao/libra/language/testing-infra/functional-tests/src/evaluator.rs:626
13: functional_tests::evaluator::eval
at /Users/vgao/libra/language/testing-infra/functional-tests/src/evaluator.rs:752
14: functional_tests::testsuite::functional_tests
at /Users/vgao/libra/language/testing-infra/functional-tests/src/testsuite.rs:326
15: testsuite::run_test
at ./tests/testsuite.rs:64
16: datatest_stable::runner::Requirements::expand::{{closure}}::{{closure}}
at /Users/vgao/libra/common/datatest-stable/src/runner.rs:323
17: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1056
18: datatest_stable::runner::run_test::{{closure}}::{{closure}}
at /Users/vgao/libra/common/datatest-stable/src/runner.rs:172
19: core::ops::function::FnOnce::call_once
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:227
20: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:308
21: std::panicking::try::do_call
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:381
22: ___rust_try
23: std::panicking::try
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:345
24: std::panic::catch_unwind
at /Users/vgao/.rustup/toolchains/1.48.0-x86_64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:382
25: datatest_stable::runner::run_test::{{closure}}
at /Users/vgao/libra/common/datatest-stable/src/runner.rs:172
Not sure if these are real bugs in the VM or just things not handled properly by the testing infra yet. Will study them more. It's also weird that some other tests (e.g. out_of_gas_in_nested_module.move) that throws OUT_OF_GAS error do pass.
|
❗ Invalid command Bors help and documentationBors is a Github App used to manage merging of PRs in order to ensure that CI is always green. It does so by maintaining a Merge Queue. Once a PR reaches the head of the Merge Queue it is rebased on top of the latest version of the PR's General
CommandsBors actions can be triggered by posting a comment which includes a line of the form
OptionsOptions for Pull Requests are configured through the application of labels.
|
|
Looks like my first example is due to The second example is caused by impl VMError {
pub fn into_vm_status(self) -> VMStatus {
let VMError {
major_status,
sub_status,
location,
mut offsets,
..
} = self;
match (major_status, sub_status, location) {
(StatusCode::EXECUTED, sub_status, _) => {
debug_assert!(sub_status.is_none());
VMStatus::Executed
}
(StatusCode::ABORTED, Some(code), Location::Script) => {
VMStatus::MoveAbort(vm_status::AbortLocation::Script, code)
}
(StatusCode::ABORTED, Some(code), Location::Module(id)) => {
VMStatus::MoveAbort(vm_status::AbortLocation::Module(id), code)
}
(StatusCode::ABORTED, sub_status, location) => {
debug_assert!(
false,
"Expected a code and module/script location with ABORTED, but got {:?} and {}",
sub_status, location
);
VMStatus::Error(StatusCode::ABORTED)
}
// TODO Errors for OUT_OF_GAS do not always have index set
(major_status, sub_status, location)
if major_status.status_type() == StatusType::Execution =>
{
debug_assert!(
offsets.len() == 1,
"Unexpected offsets. major_status: {:?}\
sub_status: {:?}\
location: {:?}\
offsets: {:#?}",
major_status,
sub_status,
location,
offsets
);
let abort_location = match location {
Location::Script => vm_status::AbortLocation::Script,
Location::Module(id) => vm_status::AbortLocation::Module(id),
Location::Undefined => {
return VMStatus::Error(major_status);
}
};
let (function, code_offset) = match offsets.pop() {
None => {
return VMStatus::Error(major_status);
}
Some((fdef_idx, code_offset)) => (fdef_idx.0, code_offset),
};
VMStatus::ExecutionFailure {
status_code: major_status,
location: abort_location,
function,
code_offset,
}
}
(major_status, _, _) => VMStatus::Error(major_status),
}
}Seems like it's exactly what the TODO says we fail to address: OUT_OF_GAS do not always have index set |
|
Thanks for investigating those. Maybe file a GitHub issue for problem 2? It sounds like this change is still OK then, right? |
|
Yeah I'll file an issue for problem 2. Let me see if I can reproduce this on master (with modified code). There might be another issue this PR may have to address: rounding when converting from internal units to external units. |
I don't think we need to round, but even if we wanted to, it can't be changed without breaking compatibility, so it would need to be done under control of some versioned flag. This PR does not affect how much gas is charged -- it just fixes the inconsistency in the validation check. I believe we definitely should increase the minimum transaction fee above the scaling factor, though (but not as part of this PR -- that has to be done by modifying the on-chain gas schedule). |
|
I don't know why I didn't see that |
|
/land |
This fixes a bug (#7238) where a transaction incorrectly fails to validate in the adapter when the transaction's max_gas_amount is less than the min_transaction_gas_units gas constant. The bug is that the comparison does not take into account the gas_unit_scaling_factor gas constant. To help avoid similar bugs in the future, this introduces a new InternalGasUnits type that is distinct from the existing GasUnits type. Both are newtype wrappers around u64 so BCS-serialized values should be interchangeable between these two types (e.g., in the on-chain gas cost table). Closes: #7448
This fixes a bug (#7238) where a transaction incorrectly fails to validate in the adapter when the transaction's
max_gas_amountis less than themin_transaction_gas_unitsgas constant. The bug is that the comparison does not take into account thegas_unit_scaling_factorgas constant. To help avoid similar bugs in the future, this introduces a new InternalGasUnits type that is distinct from the existing GasUnits type. Both are newtype wrappers around u64 so BCS-serialized values should be interchangeable between these two types (e.g., in the on-chain gas cost table).Motivation
Fixes #7238
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Updated tests
Related PRs
#7239