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.

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

Reduce gas/storage limits in nested calls #6890

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

rockbmb
Copy link
Contributor

@rockbmb rockbmb commented Dec 13, 2024

Closes #6846 .

@rockbmb rockbmb added T2-pallets This PR/Issue is related to a particular pallet. T7-smart_contracts This PR/Issue is related to smart contracts. T10-tests This PR/Issue is related to tests. labels Dec 13, 2024
@rockbmb rockbmb self-assigned this Dec 13, 2024
@rockbmb rockbmb force-pushed the evm-contracts-limit-nested-calls-gas branch 2 times, most recently from 376e944 to 4832380 Compare December 17, 2024 02:17
Also, a limit of 0 when determining a nested call's metering limit would
mean it was free to use all of the callee's resources.
Now, a limit of 0 means that the nested call will have an empty storage
meter.
In particular, this commit removes the usage of `0` as unlimited
metering in the following tests:

- `nonce`
- `last_frame_output_works_on_instantiate`
- `instantiation_from_contract`
- `immutable_data_set_works_only_once`
- `immutable_data_set_errors_with_empty_data`
- `immutable_data_access_checks_work`
@rockbmb rockbmb force-pushed the evm-contracts-limit-nested-calls-gas branch from ed1f312 to 8e66f50 Compare December 17, 2024 19:15
@rockbmb rockbmb marked this pull request as ready for review December 17, 2024 19:31
@athei athei requested review from athei and xermicus December 17, 2024 20:08
Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

The basic idea of the 63/64 rule looks correct to me but there are many failing tests. I left some nits.

.min(self.gas_left);
self.gas_left -= amount;
GasMeter::new(amount)
// The reduction to 63/64 is to emulate EIP-150
Copy link
Member

Choose a reason for hiding this comment

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

This should be a doc-comment

substrate/frame/revive/src/storage/meter.rs Outdated Show resolved Hide resolved
debug_assert!(matches!(self.contract_state(), ContractState::Alive));

// Limit gas for nested call, per EIP-150.
Copy link
Member

Choose a reason for hiding this comment

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

This could go into the doc-comment (which is no no-longer correct I think)

This fixes, among other tests:
* `tests::gas_consumed_is_linear_for_nested_calls` test
* `tests::deposit_limit_in_nested_calls`
* `tests::transient_storage_limit_in_call`
* `tests::call_return_code`
* `test::chain_extension_temp_storage_works`
* `tests::origin_api_works`
* `tests::read_only_call_works`
@rockbmb
Copy link
Contributor Author

rockbmb commented Dec 19, 2024

@xermicus the meters in frame::contracts::{gas, storage::meter} also need to reflect this, correct?

@athei
Copy link
Member

athei commented Dec 19, 2024

Sorry if this wasn't clear: Only pallet_revive and its supporting crates should be changed. pallet_contracts should remain untouched. It does't receive any updates beyond critical bug fixes anymore. Can you roll back the changes there? Will have a deeper look then.

@rockbmb
Copy link
Contributor Author

rockbmb commented Dec 19, 2024

@athei frame/contracts changes have been reverted, thanks for explaining this.

Most of the previously failing tests were caused by the contract code of those tests using the now-removed '0 means "use all available gas"' semantic.

After fixing this, cargo test --package pallet-revive:0.1.0 --lib --all-features yields:

failures:
    benchmarking::benchmarks::bench_seal_instantiate
    tests::call_diverging_out_len_works
    tests::create1_with_value_works
    tests::delegate_call
    tests::deploy_and_call_other_contract
    tests::deposit_limit_in_nested_calls
    tests::deposit_limit_in_nested_instantiate
    tests::destroy_contract_and_transfer_funds
    tests::failed_deposit_charge_should_roll_back_call
    tests::gas_estimation_call_runtime
    tests::gas_estimation_for_subcalls
    tests::return_data_api_works
    tests::skip_transfer_works
    tests::storage_deposit_callee_works
    tests::test_debug::debugging_works

The contracts the above tests rely on have removed usages of 0 as "use all", so their failures are somewhere else.

@rockbmb
Copy link
Contributor Author

rockbmb commented Dec 19, 2024

/bot fmt

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Due to the changes here you can get rid of the fn enforce_subcall_limit() and enum Nested. They are no longer needed because we now always enforce the limit at the sub call boundry (calling fn enforce_limit unconditionally).

Apart from a few other comments it looks pretty much like what I expect. Thanks.

Comment on lines 176 to 177
let amt = amount.min(self.gas_left - self.gas_left / 64);
self.gas_left -= amt;
Copy link
Member

Choose a reason for hiding this comment

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

Please use amount and not amt. No need to introduce a less readable identifier when we can just shadow.

Comment on lines 382 to 385
#[test]
/// Previously, passing a `Weight` of 0 to `nested` would consume all of the meter's current gas.
///
/// Now, a `Weight` of 0 means no gas for the nested call.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[test]
/// Previously, passing a `Weight` of 0 to `nested` would consume all of the meter's current gas.
///
/// Now, a `Weight` of 0 means no gas for the nested call.
/// Previously, passing a `Weight` of 0 to `nested` would consume all of the meter's current gas.
///
/// Now, a `Weight` of 0 means no gas for the nested call.
#[test]

Copy link
Member

Choose a reason for hiding this comment

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

Still some left over changes in the wrong pallet to roll back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out.

Comment on lines -888 to +890
Weight::zero(),
Weight::max_value(),
storage_meter,
BalanceOf::<T>::zero(),
BalanceOf::<T>::max_value(),
Copy link
Member

Choose a reason for hiding this comment

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

The first frame is also created by nested and will be limited to 63/64. We need to make sure the first one can use the full resources.

Copy link
Contributor Author

@rockbmb rockbmb Dec 20, 2024

Choose a reason for hiding this comment

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

We need to make sure the first one can use the full resources.

I did not understand this; may you elaborate on what exactly that would mean here?

Copy link
Member

Choose a reason for hiding this comment

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

In the current code the overall transaction will be limited to 63/64. We don't want this. We only want sub calls to be limited. But not the first contract called by the externally owned account. That one should be allowed to use all the gas.

@rockbmb
Copy link
Contributor Author

rockbmb commented Dec 20, 2024

Due to the changes here you can get rid of the fn enforce_subcall_limit() and enum Nested. They are no longer needed because we now always enforce the limit at the sub call boundry (calling fn enforce_limit unconditionally).

If I raze enum Nested, does it make sense to

  1. Remove RawMeter's 3rd type parameter S, so it becomes pub struct RawMeter<T: Config, E>
  2. move all methods in impl<T: Config, E: Ext<T>> RawMeter<T, E, Nested> into impl<T, E> RawMeter<T, E, Root>

?

@athei
Copy link
Member

athei commented Dec 20, 2024

The type stating is still useful for other functions than enforce_subcall_limit that are exist on the nested meter. I wasn't aware the enum I told you to delete was also for used that. So no, keep the third type parameter.

Just remove the variants of Nested (or convert to unit structure like Root) and removed the nested field from RawMeter. Just add S to the _phantom.

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12440165471
Failed job name: quick-benchmarks-omni

@rockbmb
Copy link
Contributor Author

rockbmb commented Dec 21, 2024

14 unit tests are still failing, among them

  1. tests::deposit_limit_in_nested_calls, and
  2. tests::deposit_limit_in_nested_instantiate

these 2 tests fail because of a ContractReverted error where StorageDepositLimitExhausted was expected.

The remaining tests fail with ContractTrapped when assert_oking the result of CallBuilder::build.
E.g. for tests::delegate_call:

assert_ok!(builder::call(caller_addr) <-- assert fails!
	.value(1337)
	.data((callee_addr, 0u64, 0u64).encode())
	.build());

⬆️ is context in case you have any insights to share - the tests all failing for the same reason must be a clue to the underlying problem.

I'll move fast so paritytech/revive#117 can continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet. T7-smart_contracts This PR/Issue is related to smart contracts. T10-tests This PR/Issue is related to tests.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Limit resources to 63/64 on sub calls and remove 0 as special case for "take all"
3 participants