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-19T10:29:52Z

/bot fmt

athei

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.

substrate/frame/revive/src/gas.rs

substrate/frame/contracts/src/benchmarking/mod.rs

...us/parachains/integration-tests/emulated/tests/people/people-westend/src/tests/governance.rs

substrate/frame/revive/src/exec.rs

rockbmb · 2024-12-20T01:24:49Z

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

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

?

athei · 2024-12-20T07:27:41Z

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.

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

athei · 2025-01-02T18:21:10Z

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

 	///
 	/// We normally need to call this **once** for every call stack and not for every cross contract
 	/// call. However, if a dedicated limit is specified for a sub-call, this needs to be called
-	/// once the sub-call has returned. For this, the [`Self::enforce_subcall_limit`] wrapper is
-	/// used.
+	/// once the sub-call has returned.


Doc comment needs updating to reflect the new much simpler behavior: This has to be called every time a sub call returns. No special casing anymore for a 'dedicated limit'. We just removed this special casing.

This comment has been changed on my tree, and no longer contains any reference to enforce_subcall_limit.

The rest of the comment is still stale:

/// We normally need to call this **once** for every call stack and not for every cross contract /// call. However, if a dedicated limit is specified for a sub-call, this needs to be called

There is no special limit anymore. Also, we never call it once per call stack as stated here. We call it for every sub call now.

😵

I was wrong, thanks for clarifying.

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.

athei · 2025-01-06T11:45:22Z

substrate/frame/revive/src/exec.rs

 			// If a special limit was set for the sub-call, we enforce it here.
 			// The sub-call will be rolled back in case the limit is exhausted.


Suggested change

// If a special limit was set for the sub-call, we enforce it here.

// The sub-call will be rolled back in case the limit is exhausted.

// We are only charging the storage deposit at the end of every call stack.

// To make sure that no sub call uses more than it is allowed we need to

// manually enforce the limit here.

athei · 2025-01-06T11:47:09Z

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

@@ -126,7 +121,7 @@ pub struct RawMeter<T: Config, E, S: State + Default + Debug> {
 	/// limited by the maximum call depth.
 	charges: Vec<Charge<T>>,
 	/// We store the nested state to determine if it has a special limit for sub-call.
-	nested: S,
+	_nested: PhantomData<S>,
 	/// Type parameter only used in impls.
 	_phantom: PhantomData<E>,


You can remove the nested field.

Suggested change

_phantom: PhantomData<E>,

_phantom: PhantomData<(E, S)>,

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

athei · 2025-01-07T08:50:30Z

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

+		let deposit_limit = if deposit_ptr == SENTINEL {
+			U256::from(u64::MAX)
+		} else {
+			memory.read_u256(deposit_ptr)?
+		};


Suggested change

let deposit_limit = if deposit_ptr == SENTINEL {

U256::from(u64::MAX)

} else {

memory.read_u256(deposit_ptr)?

};

memory.read_u256(deposit_ptr)?

We don't want any special handling here anymore.

athei · 2025-01-07T08:50:52Z

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

+		let deposit_limit: U256 = if deposit_ptr == SENTINEL {
+			U256::from(u64::MAX)
+		} else {
+			memory.read_u256(deposit_ptr)?
+		};


Suggested change

let deposit_limit: U256 = if deposit_ptr == SENTINEL {

U256::from(u64::MAX)

} else {

memory.read_u256(deposit_ptr)?

};

memory.read_u256(deposit_ptr)?

This raises OutOfBounds errors in many tests (it replaced the last 2 tests' failure ContractTrapped failure, which is progress).

Do you have an idea of what might be causing this?

The test fixtures need to be adapted to always pass a valid pointer. SENTINEL is used to indicate a None so to say.

For example, in substrate/frame/revive/fixtures/contracts/call.rs, the deposit limit is None - this is was we want, right

Yes. You need to change the wrapper in pallet-fixtures-uapi. It no longer takes an Option since the value is no longer optional. You then pass in U256::MAX where None was passes before.

Where there is U256::MAX, do you mean &[u8::MAX; 32], since in trait HostFn one has deposit_limit: &[u8; 32],?

Furthermore, instead of &[u8::MAX; 32], won't we want something like &[u8::MAX; 8].concat([0u8; 24]? Equivalent to U256::from(u64::MAX).
Using &[u8::MAX; 32] will raise Error::BalanceConversionFailed when the value is read from memory and converted into a Balance<T>.

I opted for (something similar to) &[0xFF; 8].concat([0u8; 24] to solve this issue.

paritytech-workflow-stopper · 2025-01-07T14:54:48Z

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

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.

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

athei reviewed Dec 19, 2024

View reviewed changes

rockbmb added 3 commits December 20, 2024 15:45

Address latest PR comments (enum Nested)

36690cd

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

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

Modify how deposit limits are stored in revive/uapi

11c8858

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

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

		// If a special limit was set for the sub-call, we enforce it here.
		// The sub-call will be rolled back in case the limit is exhausted.

-			// If a special limit was set for the sub-call, we enforce it here.
-			// The sub-call will be rolled back in case the limit is exhausted.
+			// We are only charging the storage deposit at the end of every call stack.
+			// To make sure that no sub call uses more than it is allowed we need to
+			// manually enforce the limit here.

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

Choose a reason for hiding this comment

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 19, 2024

athei left a comment

Choose a reason for hiding this comment

rockbmb commented Dec 20, 2024

athei commented Dec 20, 2024 • edited Loading

rockbmb commented Dec 21, 2024 • edited Loading

athei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athei commented Jan 2, 2025

rockbmb commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbmb Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbmb Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paritytech-workflow-stopper bot commented Jan 7, 2025

rockbmb commented Jan 7, 2025

rockbmb commented Dec 13, 2024 •

edited by athei

Loading

athei commented Dec 19, 2024 •

edited

Loading

athei commented Dec 20, 2024 •

edited

Loading

rockbmb commented Dec 21, 2024 •

edited

Loading

rockbmb commented Jan 3, 2025 •

edited

Loading

rockbmb commented Jan 6, 2025 •

edited

Loading

rockbmb Jan 7, 2025 •

edited

Loading

rockbmb Jan 7, 2025 •

edited

Loading