Reduce gas/storage limits in nested calls #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.

rockbmb · 2024-12-21T00:47:21Z

14 unit tests are still failing, among them

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.

athei

Looks good. Just some nits.

Regarding the failing tests: Your code looks correct to me. However, it is a breaking change as it prevents sub calls from using all the remaining gas which was possible before. So I assume the tests rely in one way or another on this behavior and need to be changed.

substrate/frame/revive/src/exec.rs

substrate/frame/revive/src/gas.rs

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

athei · 2025-01-02T18:46:15Z

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

This can be a bit finicky and requires some knowledge of how the error handling works in contracts to debug. The error you are asserting on here is what is returned from the root contract that is called from the transaction.

However, when this contract calls into another contract and the callee encounters an error like StorageDepositLimitExhausted it will just be returned as an error code to the caller. This is so that the caller can decide how to handle errors in sub calls rather than just reverting.

The change you made by removing enforce_subcall_limit can leadd to the situation that we fail in the sub calls context when running out of storage deposit. At least when we were passing 0. Before we delayed the check to the last frame returning and failed in the callers context. The caller probably just reverts on non-zero return codes from the sub calls. This is why you get the generic ContractReverted instead of the more detailed StorageDepositLimitExhausted.

This is actually expected and nice that it is covered by test cases. You should change the tests to adapt to the new behavior. You can see that the caller contract also returns the sub calls return code in addition to reverting. So you can check this return code in your test and are not at the mercy of the generic ContractReverted error.

Use `BareCallBuilder::bare_call` instead of `CallBuilder::call` to allow scrutiny of contract return code with `assert_return_code`, and disambiguate the cause of the `ContractReverted` error that `CallBuilder::build` would have raised.

rockbmb · 2025-01-03T19:23:11Z

You should change the tests to adapt to the new behavior. You can see that the caller contract also returns the sub calls return code in addition to reverting. So you can check this return code in your test and are not at the mercy of the generic ContractReverted error.

Thanks for clarifying this - in acfcf66 and e0c0845, both tests that threw ContractReverted are corrected.

The remaining ones throw ContractTrapped - as you said, these tests may need to be changed.

substrate/frame/revive/src/exec.rs

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

rockbmb · 2025-01-06T14:27:22Z

Apologies for the force-push - I made a mistake in a commit prior to the master merge.

A limit of `0` no longer signifies "no limit", and must thus be changed to `u64::MAX`.

rockbmb · 2025-01-06T15:40:25Z

/cmd fmt

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

athei · 2025-01-08T14:49:09Z

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

+///
+/// Deposit limits are `U256`, but balances are represented as `u64`.
+/// To represent no deposit limits on an operation, this should be used.
+pub const U256_MAX: [u8; 32] = u64_to_u256_bytes(u64::MAX);


Should be the same, right?

Suggested change

pub const U256_MAX: [u8; 32] = u64_to_u256_bytes(u64::MAX);

pub const U256_MAX: [u8; 32] = [1; 32];

athei · 2025-01-08T14:52:34Z

substrate/frame/revive/src/exec.rs

-						Weight::zero(),
-						U256::zero(),
+						Weight::MAX,
+						U256::from(u64::MAX),


Same for the other occurrences.

Suggested change

U256::from(u64::MAX),

U256::MAX,

paritytech-workflow-stopper · 2025-01-08T15:07:17Z

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

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

rockbmb added 2 commits December 21, 2024 00:36

Refactor RawMeter/Nested

2c8f492

Run fmt

12473b4

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

d21a941

athei reviewed Jan 2, 2025

View reviewed changes

rockbmb added 3 commits January 3, 2025 13:45

Address latest review

2f8ab21

Fix tests delegate_call and deposit_limit_in_nested_calls

acfcf66

Fix tests::deposit_limit_in_nested_instantiate

e0c0845

Use `BareCallBuilder::bare_call` instead of `CallBuilder::call` to allow scrutiny of contract return code with `assert_return_code`, and disambiguate the cause of the `ContractReverted` error that `CallBuilder::build` would have raised.

athei reviewed Jan 6, 2025

View reviewed changes

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

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

Address review comments

b090e5f

rockbmb force-pushed the evm-contracts-limit-nested-calls-gas branch from 1844efa to b090e5f Compare January 6, 2025 14:26

Change default deposit limit in WASM runtime checks

0be4019

A limit of `0` no longer signifies "no limit", and must thus be changed to `u64::MAX`.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce gas/storage limits in nested calls #6890

Reduce gas/storage limits in nested calls #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

rockbmb commented Dec 21, 2024 •

edited

Loading

athei left a comment

athei commented Jan 2, 2025

rockbmb commented Jan 3, 2025 •

edited

Loading

rockbmb commented Jan 6, 2025 •

edited

Loading

rockbmb commented Jan 6, 2025

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 Jan 8, 2025

athei Jan 8, 2025

paritytech-workflow-stopper bot commented Jan 8, 2025

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

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

	pub const U256_MAX: [u8; 32] = u64_to_u256_bytes(u64::MAX);
	pub const U256_MAX: [u8; 32] = [1; 32];

Reduce gas/storage limits in nested calls #6890

Are you sure you want to change the base?

Reduce gas/storage limits in nested calls #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

rockbmb commented Dec 21, 2024 • edited Loading

athei left a comment

Choose a reason for hiding this comment

athei commented Jan 2, 2025

rockbmb commented Jan 3, 2025 • edited Loading

rockbmb commented Jan 6, 2025 • edited Loading

rockbmb commented Jan 6, 2025

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 Jan 8, 2025

Choose a reason for hiding this comment

athei Jan 8, 2025

Choose a reason for hiding this comment

paritytech-workflow-stopper bot commented Jan 8, 2025

rockbmb commented Dec 13, 2024 •

edited by athei

Loading

athei commented Dec 19, 2024 •

edited

Loading

rockbmb commented Dec 21, 2024 •

edited

Loading

rockbmb commented Jan 3, 2025 •

edited

Loading

rockbmb commented Jan 6, 2025 •

edited

Loading