Remove 0 as a special case in gas/storage meters #6890

rockbmb · 2024-12-13T17:45:04Z

Closes #6846 .

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`

xermicus

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

xermicus · 2024-12-18T07:39:17Z

substrate/frame/revive/src/gas.rs

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


This should be a doc-comment

substrate/frame/revive/src/storage/meter.rs

xermicus · 2024-12-18T09:31:53Z

substrate/frame/revive/src/storage/meter.rs

 		debug_assert!(matches!(self.contract_state(), ContractState::Alive));
+
+		// Limit gas for nested call, per EIP-150.


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 · 2024-12-19T00:57:44Z

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

athei · 2024-12-19T09:12:35Z

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 · 2024-12-19T10:14:13Z

@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.

github-actions · 2025-01-06T15:42:31Z

Command "fmt" has started 🚀 See logs here

github-actions · 2025-01-06T15:42:53Z

Command "fmt" has finished ✅ See logs here

A deposit limit of 0 no longer is a special case, and is treated as no limit being available for use in a benchmarking function.

rockbmb · 2025-01-06T19:17:02Z

The only tests that still need to be addressed:

failures:
    tests::gas_estimation_for_subcalls
    tests::skip_transfer_works

substrate/frame/revive/src/wasm/runtime.rs

rockbmb · 2025-01-07T18:42:30Z

@athei One question:

What is the meaning of value: 0u32.into()?
I.e. after the semantic change in the meaning of 0 introduced by this PR, should its default still be 0?

This is causing tests::gas_estimation_for_subcalls to fail when creating the storage meter for Pallet::bare_call by creating a meter with 0 limit, eventually leading to ContractTrapped.

athei · 2025-01-08T13:43:28Z

@athei One question:

What is the meaning of value: 0u32.into()? I.e. after the semantic change in the meaning of 0 introduced by this PR, should its default still be 0?

This is causing tests::gas_estimation_for_subcalls to fail when creating the storage meter for Pallet::bare_call by creating a meter with 0 limit, eventually leading to ContractTrapped.

The line you quoted is value which is the amount of balance sent to the callee as part of the call. This is unrelated to the deposit limit you changed. Its behavior should be unaffected by this PR.

substrate/frame/revive/uapi/src/lib.rs

There, its meaning was "use all available gas", which needed to be updated to `Weight::MAX`.

substrate/frame/revive/uapi/src/lib.rs

substrate/frame/revive/src/exec.rs

Keep the "`0` as zero gas limit" changes.

substrate/frame/revive/src/storage/meter.rs

athei · 2025-01-10T12:24:56Z

For context, we agreed to roll back the EIP-150 changes because limiting the amount of gas that can be used depending on call depth breaks dry running: You can't just run the dry-run once with a lot of gas. The gas that was consumed in this run will not be enough if used as a gas limit. This is because not all of the available gas can be used if sub calls are involved.

So this PR only removes the special casing of 0 meaning "all resources".

substrate/frame/revive/src/storage/meter.rs

athei · 2025-01-10T16:57:24Z

I don't get why the benchmarks are failing.

rockbmb · 2025-01-10T17:38:30Z

@athei I can always run /cmd bench --pallet pallet_revive.

No significant code was added, only removed (enfore_subcall_limit is only a handful of instructions), but maybe the numbers were lowered enough to warrant rerunning benchmarks.

paritytech-workflow-stopper · 2025-01-10T17:50:02Z

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

rockbmb · 2025-01-10T19:06:25Z

/cmd bench --pallet pallet_revive

github-actions · 2025-01-10T19:10:24Z

Command "bench --pallet pallet_revive" has started 🚀 See logs here

github-actions · 2025-01-10T21:50:50Z

Command "bench --pallet pallet_revive" has failed ❌! See logs here

Command output:

❌ Failed benchmarks of runtimes/pallets:
-- dev: ['pallet_revive']
-- asset-hub-westend: ['pallet_revive']

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 self-assigned this Dec 13, 2024

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

rockbmb added 11 commits December 17, 2024 19:15

Fix typo in integration test

fadb396

Add unit tests to current behavior of GasMeter.nested

db02307

Add unit tests to simplest behavior of RawMeter

2c2a7f7

Add PRDoc

11dd382

Limit gas for nested calls per EIP-150

5a8312a

Reflect EIP-150 change in some unit tests

a7781c4

Address missing EIP-150 fix in one gas meter unit test

b6e7290

Apply EIP-150 to storage metering

8d7be23

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.

Fix storage meter unit test for fn nested

ee2a183

Remove usage of 0 as unlimited storage meter in test

b89495f

rockbmb force-pushed the evm-contracts-limit-nested-calls-gas branch from ed1f312 to 8e66f50 Compare December 17, 2024 19:15

rockbmb marked this pull request as ready for review December 17, 2024 19:31

athei requested review from athei and xermicus December 17, 2024 20:08

xermicus reviewed Dec 18, 2024

View reviewed changes

rockbmb added 2 commits December 19, 2024 10:52

Rollback changes in frame/contracts

f5e6b2b

Add EIP-150 note to doc comment

33758aa

Run fmt

d1b40ed

xermicus mentioned this pull request Dec 19, 2024

Gas limits for calls paritytech/revive#117

Open

Update from rockbmb running command 'fmt'

c3b9510

Update deposit limit in benchmark

7531f72

A deposit limit of 0 no longer is a special case, and is treated as no limit being available for use in a benchmarking function.

athei reviewed Jan 7, 2025

View reviewed changes

substrate/frame/revive/src/wasm/runtime.rs Outdated Show resolved Hide resolved

substrate/frame/revive/src/wasm/runtime.rs Outdated Show resolved Hide resolved

Modify how deposit limits are stored in revive/uapi

11c8858

athei reviewed Jan 8, 2025

View reviewed changes

substrate/frame/revive/uapi/src/lib.rs Outdated Show resolved Hide resolved

rockbmb added 2 commits January 8, 2025 14:15

Rename const with confusing name

222e6df

Update usage of Weight::zero() in test

93ac5ba

There, its meaning was "use all available gas", which needed to be updated to `Weight::MAX`.

athei reviewed Jan 8, 2025

View reviewed changes

substrate/frame/revive/uapi/src/lib.rs Outdated Show resolved Hide resolved

substrate/frame/revive/src/exec.rs Outdated Show resolved Hide resolved

rockbmb added 2 commits January 9, 2025 19:14

Revert EIP-150 changes

b228eb2

Keep the "`0` as zero gas limit" changes.

Remove U256_MAX const, use U256::MAX

3e4bbe8

rockbmb commented Jan 9, 2025

View reviewed changes

substrate/frame/revive/src/storage/meter.rs Outdated Show resolved Hide resolved

Remove enforce_subcall_limit and associated logic

7c6aedb

rockbmb changed the title ~~Reduce gas/storage limits in nested calls~~ Remove 0 as a special case in gas/storage meters Jan 10, 2025

rockbmb added 3 commits January 10, 2025 15:20

Unspecify cause of failure in tests::skip_transfer_works

908c371

Merge branch 'master' into evm-contracts-limit-nested-calls-gas

f3a379c

Run cargo fmt

6426401

athei approved these changes Jan 10, 2025

View reviewed changes

substrate/frame/revive/src/storage/meter.rs Outdated Show resolved Hide resolved

Remove unnecessary Bounded trait bound

1e48276

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove 0 as a special case in gas/storage meters #6890

Remove 0 as a special case in gas/storage meters #6890

rockbmb commented Dec 13, 2024 •

edited by athei

Loading

xermicus left a comment

xermicus Dec 18, 2024

xermicus Dec 18, 2024

rockbmb commented Dec 19, 2024

athei commented Dec 19, 2024 •

edited

Loading

rockbmb commented Dec 19, 2024

github-actions bot commented Jan 6, 2025

github-actions bot commented Jan 6, 2025

rockbmb commented Jan 6, 2025

rockbmb commented Jan 7, 2025

athei commented Jan 8, 2025

athei commented Jan 10, 2025

athei commented Jan 10, 2025

rockbmb commented Jan 10, 2025

paritytech-workflow-stopper bot commented Jan 10, 2025

rockbmb commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

		debug_assert!(matches!(self.contract_state(), ContractState::Alive));

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

Remove 0 as a special case in gas/storage meters #6890

Are you sure you want to change the base?

Remove 0 as a special case in gas/storage meters #6890

Conversation

rockbmb commented Dec 13, 2024 • edited by athei Loading

xermicus left a comment

Choose a reason for hiding this comment

xermicus Dec 18, 2024

Choose a reason for hiding this comment

xermicus Dec 18, 2024

Choose a reason for hiding this comment

rockbmb commented Dec 19, 2024

athei commented Dec 19, 2024 • edited Loading

rockbmb commented Dec 19, 2024

github-actions bot commented Jan 6, 2025

github-actions bot commented Jan 6, 2025

rockbmb commented Jan 6, 2025

rockbmb commented Jan 7, 2025

athei commented Jan 8, 2025

athei commented Jan 10, 2025

athei commented Jan 10, 2025

rockbmb commented Jan 10, 2025

paritytech-workflow-stopper bot commented Jan 10, 2025

rockbmb commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

rockbmb commented Dec 13, 2024 •

edited by athei

Loading

athei commented Dec 19, 2024 •

edited

Loading