From 2e73763951694351048ccbfab3a1c2337a223345 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Wed, 12 Nov 2025 15:31:27 +0100 Subject: [PATCH 01/15] added all zero storage refund in storage.rs --- substrate/frame/revive/src/storage.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/substrate/frame/revive/src/storage.rs b/substrate/frame/revive/src/storage.rs index 46c175a9b7155..ad25eb0b8c924 100644 --- a/substrate/frame/revive/src/storage.rs +++ b/substrate/frame/revive/src/storage.rs @@ -311,6 +311,12 @@ impl ContractInfo { storage_meter: Option<&mut meter::NestedMeter>, take: bool, ) -> Result { + // If new_value is Some(all zero bytes), treat it as None (deletion) + if let Some(value) = new_value { + if value.is_empty() || value.iter().all(|&b| b == 0) { + return self.write_raw(key, None, storage_meter, take); + } + } let child_trie_info = &self.child_trie_info(); let (old_len, old_value) = if take { let val = child::get_raw(child_trie_info, key); From 63a2db00ce26aee75ff71dbb7586b2e5d47962b6 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Wed, 12 Nov 2025 15:31:50 +0100 Subject: [PATCH 02/15] added all zero storage refund in EVM sstore host.rs instruction --- substrate/frame/revive/src/vm/evm/instructions/host.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/substrate/frame/revive/src/vm/evm/instructions/host.rs b/substrate/frame/revive/src/vm/evm/instructions/host.rs index 160d6c3d6dd2f..19ff87e7911b3 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/host.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/host.rs @@ -146,8 +146,9 @@ fn store_helper<'ext, E: Ext>( let charged_amount = interpreter.ext.charge_or_halt(cost_before)?; let key = Key::Fix(index.to_big_endian()); let take_old = false; + let value_to_store = if value.is_zero() { None } else { Some(value.to_big_endian().to_vec()) }; let Ok(write_outcome) = - set_function(interpreter.ext, &key, Some(value.to_big_endian().to_vec()), take_old) + set_function(interpreter.ext, &key, value_to_store.clone(), take_old) else { return ControlFlow::Break(Error::::ContractTrapped.into()); }; @@ -155,7 +156,7 @@ fn store_helper<'ext, E: Ext>( interpreter .ext .gas_meter_mut() - .adjust_gas(charged_amount, adjust_cost(32, write_outcome.old_len())); + .adjust_gas(charged_amount, adjust_cost(if value_to_store.is_some() { 32 } else { 0 }, write_outcome.old_len())); ControlFlow::Continue(()) } From 698283eaa5793d276df09c3527b83f38e30b11c0 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Wed, 12 Nov 2025 15:32:46 +0100 Subject: [PATCH 03/15] added test for messing around with sstore and sload --- substrate/frame/revive/src/tests/sol/host.rs | 86 ++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index eaef4fa753797..96fa790166228 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -606,3 +606,89 @@ fn logs_denied_for_static_call(caller_type: FixtureType, callee_type: FixtureTyp assert_eq!(decoded_result.success, false); }); } + +#[test] +fn shared_storage_item() { + use pallet_revive_fixtures::Caller; + use pallet_revive_fixtures::Host; + let (caller_code, _) = compile_module_with_type("Caller", FixtureType::Solc).unwrap(); + let (host_code_resolc, _) = compile_module_with_type("Host", FixtureType::Resolc).unwrap(); + let (host_code_solc, _) = compile_module_with_type("Host", FixtureType::Solc).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + // Deploy Host contracts + let Contract { addr: host_addr_solc, .. } = + builder::bare_instantiate(Code::Upload(host_code_solc)).build_and_unwrap_contract(); + let Contract { addr: host_addr_resolc, .. } = + builder::bare_instantiate(Code::Upload(host_code_resolc)).build_and_unwrap_contract(); + + // Deploy Caller contract + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); + + + let index = 13u64; + let key = Key::Fix(U256::from(index).to_big_endian()); + let expected_value = 17u64; + + // write storage item using resolc + // let result = builder::bare_call(caller_addr) + // .data( + // Caller::delegateCall { + // _callee: host_addr_resolc.0.into(), + // _data: Host::sstoreOpCall { + // slot: index, + // value: expected_value, + // }.abi_encode().into(), + // _gas: u64::MAX, + // } + // .abi_encode(), + // ) + // .build_and_unwrap_result(); + + // // read storage item using solc + // let result = builder::bare_call(caller_addr) + // .data( + // Caller::delegateCall { + // _callee: host_addr_solc.0.into(), + // _data: Host::sloadOpCall { slot: index }.abi_encode().into(), + // _gas: u64::MAX, + // } + // .abi_encode(), + // ) + // .build_and_unwrap_result(); + // println!("result data: {:?}", result.data); + + + // // write storage item to all zero using solc + // let result = builder::bare_call(caller_addr) + // .data( + // Caller::delegateCall { + // _callee: host_addr_solc.0.into(), + // _data: Host::sstoreOpCall { + // slot: index, + // value: 0u64, + // }.abi_encode().into(), + // _gas: u64::MAX, + // } + // .abi_encode(), + // ) + // .build_and_unwrap_result(); + + // read storage item using resolc + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr_resolc.0.into(), + _data: Host::sloadOpCall { slot: index }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + println!("result data: {:?}", result.data); + // TODO: parse result.data + }); +} From eb6fbb93e91425dcaf645358a1e52d8e5c540ab3 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 13 Nov 2025 09:28:45 +0100 Subject: [PATCH 04/15] Revert "added all zero storage refund in storage.rs" This reverts commit 2e73763951694351048ccbfab3a1c2337a223345. --- substrate/frame/revive/src/storage.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/substrate/frame/revive/src/storage.rs b/substrate/frame/revive/src/storage.rs index ad25eb0b8c924..46c175a9b7155 100644 --- a/substrate/frame/revive/src/storage.rs +++ b/substrate/frame/revive/src/storage.rs @@ -311,12 +311,6 @@ impl ContractInfo { storage_meter: Option<&mut meter::NestedMeter>, take: bool, ) -> Result { - // If new_value is Some(all zero bytes), treat it as None (deletion) - if let Some(value) = new_value { - if value.is_empty() || value.iter().all(|&b| b == 0) { - return self.write_raw(key, None, storage_meter, take); - } - } let child_trie_info = &self.child_trie_info(); let (old_len, old_value) = if take { let val = child::get_raw(child_trie_info, key); From bdc829e2745400666dec2946a3285998c3b9d3bb Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 13 Nov 2025 10:19:21 +0100 Subject: [PATCH 05/15] fix typo --- substrate/frame/revive/src/tests/pvm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/tests/pvm.rs b/substrate/frame/revive/src/tests/pvm.rs index fc163113453b4..55fc0091834b0 100644 --- a/substrate/frame/revive/src/tests/pvm.rs +++ b/substrate/frame/revive/src/tests/pvm.rs @@ -5159,7 +5159,7 @@ fn consume_all_gas_works() { } #[test] -fn existential_deposit_shall_not_charged_twice() { +fn existential_deposit_shall_not_be_charged_twice() { let (code, _) = compile_module("dummy").unwrap(); let salt = [0u8; 32]; From 8f8dbb7796b74df4b463c47481d2e903b8ba9b9b Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 13 Nov 2025 10:21:04 +0100 Subject: [PATCH 06/15] make evm::host::sstore write None instead of all zero bytes --- substrate/frame/revive/src/tests/sol/host.rs | 147 ++++++++++-------- .../revive/src/vm/evm/instructions/host.rs | 11 +- 2 files changed, 86 insertions(+), 72 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 96fa790166228..2801cea1f8718 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -19,14 +19,14 @@ use crate::{ address::AddressMapper, test_utils::{builder::Contract, ALICE, BOB, BOB_ADDR}, - tests::{builder, test_utils, ExtBuilder, RuntimeEvent, Test}, + tests::{builder, test_utils, test_utils::get_contract, ExtBuilder, RuntimeEvent, Test}, Code, Config, Error, Key, System, H256, U256, }; use frame_support::assert_err_ignore_postinfo; use alloy_core::sol_types::{SolCall, SolInterface}; use frame_support::traits::{fungible::Mutate, Get}; -use pallet_revive_fixtures::{compile_module_with_type, FixtureType, Host}; +use pallet_revive_fixtures::{compile_module_with_type, Caller, FixtureType, Host}; use pretty_assertions::assert_eq; use test_case::test_case; @@ -607,88 +607,103 @@ fn logs_denied_for_static_call(caller_type: FixtureType, callee_type: FixtureTyp }); } -#[test] -fn shared_storage_item() { - use pallet_revive_fixtures::Caller; - use pallet_revive_fixtures::Host; - let (caller_code, _) = compile_module_with_type("Caller", FixtureType::Solc).unwrap(); - let (host_code_resolc, _) = compile_module_with_type("Host", FixtureType::Resolc).unwrap(); - let (host_code_solc, _) = compile_module_with_type("Host", FixtureType::Solc).unwrap(); +#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] +#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] +#[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] +#[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] +fn empty_storage_item_is_zero(caller_type: FixtureType, callee_type: FixtureType) { + let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); + let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); ExtBuilder::default().build().execute_with(|| { ::Currency::set_balance(&ALICE, 100_000_000_000); - // Deploy Host contracts - let Contract { addr: host_addr_solc, .. } = - builder::bare_instantiate(Code::Upload(host_code_solc)).build_and_unwrap_contract(); - let Contract { addr: host_addr_resolc, .. } = - builder::bare_instantiate(Code::Upload(host_code_resolc)).build_and_unwrap_contract(); + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); - // Deploy Caller contract let Contract { addr: caller_addr, .. } = builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); - let index = 13u64; - let key = Key::Fix(U256::from(index).to_big_endian()); - let expected_value = 17u64; - // write storage item using resolc - // let result = builder::bare_call(caller_addr) - // .data( - // Caller::delegateCall { - // _callee: host_addr_resolc.0.into(), - // _data: Host::sstoreOpCall { - // slot: index, - // value: expected_value, - // }.abi_encode().into(), - // _gas: u64::MAX, - // } - // .abi_encode(), - // ) - // .build_and_unwrap_result(); - - // // read storage item using solc - // let result = builder::bare_call(caller_addr) - // .data( - // Caller::delegateCall { - // _callee: host_addr_solc.0.into(), - // _data: Host::sloadOpCall { slot: index }.abi_encode().into(), - // _gas: u64::MAX, - // } - // .abi_encode(), - // ) - // .build_and_unwrap_result(); - // println!("result data: {:?}", result.data); - - - // // write storage item to all zero using solc - // let result = builder::bare_call(caller_addr) - // .data( - // Caller::delegateCall { - // _callee: host_addr_solc.0.into(), - // _data: Host::sstoreOpCall { - // slot: index, - // value: 0u64, - // }.abi_encode().into(), - // _gas: u64::MAX, - // } - // .abi_encode(), - // ) - // .build_and_unwrap_result(); - - // read storage item using resolc + // read non-existent storage item let result = builder::bare_call(caller_addr) .data( Caller::delegateCall { - _callee: host_addr_resolc.0.into(), + _callee: host_addr.0.into(), _data: Host::sloadOpCall { slot: index }.abi_encode().into(), _gas: u64::MAX, } .abi_encode(), ) .build_and_unwrap_result(); - println!("result data: {:?}", result.data); - // TODO: parse result.data + + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + assert!(result.success, "delegateCall did not succeed"); + let decoded = Host::sloadOpCall::abi_decode_returns(&result.output).unwrap(); + assert_eq!(decoded, 0u64, "sloadOpCall should return zero for empty storage item"); + }) +} + +#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] +#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] +#[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] +#[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] +fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: FixtureType) { + let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); + let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); + + let index = 13u64; + + let base_deposit = get_contract(&caller_addr).total_deposit(); + + // write storage item + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr.0.into(), + _data: Host::sstoreOpCall { slot: index, value: 17u64 }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + assert!(result.success, "delegateCall did not succeed"); + + // 32 for key, 32 for value, 2 for item + assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2); + + // write storage item to all zeros + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr.0.into(), + _data: Host::sstoreOpCall { slot: index, value: 0u64 }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + assert!(result.success, "delegateCall did not succeed"); + + if callee_type == FixtureType::Resolc { + // Resolc will not refund + assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2); + } else { + // solc will refund + assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit); + } }); } diff --git a/substrate/frame/revive/src/vm/evm/instructions/host.rs b/substrate/frame/revive/src/vm/evm/instructions/host.rs index 19ff87e7911b3..44957c0edea85 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/host.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/host.rs @@ -147,16 +147,15 @@ fn store_helper<'ext, E: Ext>( let key = Key::Fix(index.to_big_endian()); let take_old = false; let value_to_store = if value.is_zero() { None } else { Some(value.to_big_endian().to_vec()) }; - let Ok(write_outcome) = - set_function(interpreter.ext, &key, value_to_store.clone(), take_old) + let Ok(write_outcome) = set_function(interpreter.ext, &key, value_to_store.clone(), take_old) else { return ControlFlow::Break(Error::::ContractTrapped.into()); }; - interpreter - .ext - .gas_meter_mut() - .adjust_gas(charged_amount, adjust_cost(if value_to_store.is_some() { 32 } else { 0 }, write_outcome.old_len())); + interpreter.ext.gas_meter_mut().adjust_gas( + charged_amount, + adjust_cost(if value_to_store.is_some() { 32 } else { 0 }, write_outcome.old_len()), + ); ControlFlow::Continue(()) } From 4a647d170fd86a1df750b96db049a7d3d6f61d47 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 13 Nov 2025 09:28:30 +0000 Subject: [PATCH 07/15] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_10309.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_10309.prdoc diff --git a/prdoc/pr_10309.prdoc b/prdoc/pr_10309.prdoc new file mode 100644 index 0000000000000..86ede5e86d9de --- /dev/null +++ b/prdoc/pr_10309.prdoc @@ -0,0 +1,10 @@ +title: '[pallet-revive] evm remove contract storage slot when writing all zero bytes' +doc: +- audience: Runtime Dev + description: |- + fixes https://github.com/paritytech/contract-issues/issues/216 + + When writing all zero bytes to a storage item, the item shall be deleted and deposit refunded. +crates: +- name: pallet-revive + bump: patch From d18302379a5f41bdbfe7cbd07a89ba11e22905b3 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 13 Nov 2025 10:43:19 +0100 Subject: [PATCH 08/15] format --- substrate/frame/revive/src/tests/sol/host.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 2801cea1f8718..481b62d087967 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -611,7 +611,7 @@ fn logs_denied_for_static_call(caller_type: FixtureType, callee_type: FixtureTyp #[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] #[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] #[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] -fn empty_storage_item_is_zero(caller_type: FixtureType, callee_type: FixtureType) { +fn reading_empty_storage_item_returns_zero(caller_type: FixtureType, callee_type: FixtureType) { let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); @@ -637,7 +637,7 @@ fn empty_storage_item_is_zero(caller_type: FixtureType, callee_type: FixtureType .abi_encode(), ) .build_and_unwrap_result(); - + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); assert!(result.success, "delegateCall did not succeed"); let decoded = Host::sloadOpCall::abi_decode_returns(&result.output).unwrap(); @@ -694,7 +694,7 @@ fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: .abi_encode(), ) .build_and_unwrap_result(); - + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); assert!(result.success, "delegateCall did not succeed"); From b844838fe53abfa18d054bc747f7b84967a641de Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 13 Nov 2025 11:07:32 +0100 Subject: [PATCH 09/15] added text to assertions --- substrate/frame/revive/src/tests/sol/host.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 481b62d087967..7ba2dcbb6ed0e 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -681,7 +681,7 @@ fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: assert!(result.success, "delegateCall did not succeed"); // 32 for key, 32 for value, 2 for item - assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2); + assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2, "Unexpected deposit sum charged for non-zero storage item"); // write storage item to all zeros let result = builder::bare_call(caller_addr) @@ -700,10 +700,10 @@ fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: if callee_type == FixtureType::Resolc { // Resolc will not refund - assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2); + assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2, "PVM contract does not refund deposit on zeroing storage item"); } else { // solc will refund - assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit); + assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit, "EVM contract should refund deposit on zeroing storage item"); } }); } From 42bd24a88492f36e03d61cdbc64a53152f9765c6 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 13 Nov 2025 11:08:07 +0100 Subject: [PATCH 10/15] format --- substrate/frame/revive/src/tests/sol/host.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 7ba2dcbb6ed0e..8dba36acd6014 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -681,7 +681,11 @@ fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: assert!(result.success, "delegateCall did not succeed"); // 32 for key, 32 for value, 2 for item - assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2, "Unexpected deposit sum charged for non-zero storage item"); + assert_eq!( + get_contract(&caller_addr).total_deposit(), + base_deposit + 32 + 32 + 2, + "Unexpected deposit sum charged for non-zero storage item" + ); // write storage item to all zeros let result = builder::bare_call(caller_addr) @@ -700,10 +704,18 @@ fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: if callee_type == FixtureType::Resolc { // Resolc will not refund - assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit + 32 + 32 + 2, "PVM contract does not refund deposit on zeroing storage item"); + assert_eq!( + get_contract(&caller_addr).total_deposit(), + base_deposit + 32 + 32 + 2, + "PVM contract does not refund deposit on zeroing storage item" + ); } else { // solc will refund - assert_eq!(get_contract(&caller_addr).total_deposit(), base_deposit, "EVM contract should refund deposit on zeroing storage item"); + assert_eq!( + get_contract(&caller_addr).total_deposit(), + base_deposit, + "EVM contract should refund deposit on zeroing storage item" + ); } }); } From c9d82a0b63b8b4e2adb6c88ca92f531146aaa32e Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Fri, 14 Nov 2025 12:53:09 +0100 Subject: [PATCH 11/15] fixed review omment --- substrate/frame/revive/src/tests/sol/host.rs | 29 +++++--------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 8dba36acd6014..bee07d855bedd 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -607,13 +607,10 @@ fn logs_denied_for_static_call(caller_type: FixtureType, callee_type: FixtureTyp }); } -#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] -#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] -#[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] -#[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] -fn reading_empty_storage_item_returns_zero(caller_type: FixtureType, callee_type: FixtureType) { - let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); - let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); +#[test_case(FixtureType::Solc; "solc")] +#[test_case(FixtureType::Resolc; "resolc")] +fn reading_empty_storage_item_returns_zero(fixture_type: FixtureType) { + let (host_code, _) = compile_module_with_type("Host", fixture_type).unwrap(); ExtBuilder::default().build().execute_with(|| { ::Currency::set_balance(&ALICE, 100_000_000_000); @@ -621,26 +618,14 @@ fn reading_empty_storage_item_returns_zero(caller_type: FixtureType, callee_type let Contract { addr: host_addr, .. } = builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); - let Contract { addr: caller_addr, .. } = - builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); - let index = 13u64; // read non-existent storage item - let result = builder::bare_call(caller_addr) - .data( - Caller::delegateCall { - _callee: host_addr.0.into(), - _data: Host::sloadOpCall { slot: index }.abi_encode().into(), - _gas: u64::MAX, - } - .abi_encode(), - ) + let result = builder::bare_call(host_addr) + .data(Host::sloadOpCall { slot: index }.abi_encode()) .build_and_unwrap_result(); - let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); - assert!(result.success, "delegateCall did not succeed"); - let decoded = Host::sloadOpCall::abi_decode_returns(&result.output).unwrap(); + let decoded = Host::sloadOpCall::abi_decode_returns(&result.data).unwrap(); assert_eq!(decoded, 0u64, "sloadOpCall should return zero for empty storage item"); }) } From f34c6e26152c2576d2e215c67aa3befd74e1c9bf Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 20 Nov 2025 10:48:28 +0100 Subject: [PATCH 12/15] added another test --- substrate/frame/revive/src/tests/sol/host.rs | 42 ++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index bee07d855bedd..93b60de90ac35 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -630,6 +630,48 @@ fn reading_empty_storage_item_returns_zero(fixture_type: FixtureType) { }) } +#[test_case(FixtureType::Solc)] +#[test_case(FixtureType::Resolc)] +fn storage_item_zero_shall_refund_deposit_simple(fixture_type: FixtureType) { + let (host_code, _) = compile_module_with_type("Host", fixture_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let index = 13u64; + + let base_deposit = get_contract(&host_addr).total_deposit(); + + // write storage item + let result = builder::bare_call(host_addr) + .data(Host::sstoreOpCall { slot: index, value: 17u64 }.abi_encode()) + .build_and_unwrap_result(); + assert!(!result.did_revert(), "sstoreOpCall reverted"); + + // 32 for key, 32 for value, 2 for item + assert_eq!( + get_contract(&host_addr).total_deposit(), + base_deposit + 32 + 32 + 2, + "Unexpected deposit sum charged for non-zero storage item" + ); + + // write storage item to all zeros + let result = builder::bare_call(host_addr) + .data(Host::sstoreOpCall { slot: index, value: 0u64 }.abi_encode()) + .build_and_unwrap_result(); + assert!(!result.did_revert(), "sstoreOpCall reverted"); + + assert_eq!( + get_contract(&host_addr).total_deposit(), + base_deposit, + "contract should refund deposit on zeroing storage item" + ); + }); +} + #[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] #[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] #[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] From fc4ce9e3b8871d26a6ed152d67d89d5ef8c02c3e Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 20 Nov 2025 10:54:05 +0100 Subject: [PATCH 13/15] added another delegatecall test --- substrate/frame/revive/src/tests/sol/host.rs | 53 ++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 93b60de90ac35..31412e77e2832 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -607,9 +607,9 @@ fn logs_denied_for_static_call(caller_type: FixtureType, callee_type: FixtureTyp }); } -#[test_case(FixtureType::Solc; "solc")] -#[test_case(FixtureType::Resolc; "resolc")] -fn reading_empty_storage_item_returns_zero(fixture_type: FixtureType) { +#[test_case(FixtureType::Solc)] +#[test_case(FixtureType::Resolc)] +fn reading_empty_storage_item_returns_zero_simple(fixture_type: FixtureType) { let (host_code, _) = compile_module_with_type("Host", fixture_type).unwrap(); ExtBuilder::default().build().execute_with(|| { @@ -630,6 +630,48 @@ fn reading_empty_storage_item_returns_zero(fixture_type: FixtureType) { }) } +#[test_case(FixtureType::Solc, FixtureType::Solc; "solc->solc")] +#[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] +#[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] +#[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] +fn reading_empty_storage_item_returns_zero_delegatecall( + caller_type: FixtureType, + callee_type: FixtureType, +) { + let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); + let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); + + ExtBuilder::default().build().execute_with(|| { + ::Currency::set_balance(&ALICE, 100_000_000_000); + + let Contract { addr: host_addr, .. } = + builder::bare_instantiate(Code::Upload(host_code)).build_and_unwrap_contract(); + + let Contract { addr: caller_addr, .. } = + builder::bare_instantiate(Code::Upload(caller_code)).build_and_unwrap_contract(); + + let index = 13u64; + + // read non-existent storage item + let result = builder::bare_call(caller_addr) + .data( + Caller::delegateCall { + _callee: host_addr.0.into(), + _data: Host::sloadOpCall { slot: index }.abi_encode().into(), + _gas: u64::MAX, + } + .abi_encode(), + ) + .build_and_unwrap_result(); + assert!(!result.did_revert(), "delegateCall reverted"); + let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); + + assert!(result.success, "sloadOpCall did not succeed"); + let decoded = Host::sloadOpCall::abi_decode_returns(&result.output).unwrap(); + assert_eq!(decoded, 0u64, "sloadOpCall should return zero for empty storage item"); + }) +} + #[test_case(FixtureType::Solc)] #[test_case(FixtureType::Resolc)] fn storage_item_zero_shall_refund_deposit_simple(fixture_type: FixtureType) { @@ -676,7 +718,10 @@ fn storage_item_zero_shall_refund_deposit_simple(fixture_type: FixtureType) { #[test_case(FixtureType::Solc, FixtureType::Resolc; "solc->resolc")] #[test_case(FixtureType::Resolc, FixtureType::Solc; "resolc->solc")] #[test_case(FixtureType::Resolc, FixtureType::Resolc; "resolc->resolc")] -fn storage_item_zero_shall_refund_deposit(caller_type: FixtureType, callee_type: FixtureType) { +fn storage_item_zero_shall_refund_deposit_delegatecall( + caller_type: FixtureType, + callee_type: FixtureType, +) { let (caller_code, _) = compile_module_with_type("Caller", caller_type).unwrap(); let (host_code, _) = compile_module_with_type("Host", callee_type).unwrap(); From 6401dc75434cf6477f5d47c0c68126d6ec862c28 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 20 Nov 2025 17:12:28 +0100 Subject: [PATCH 14/15] changed the test so that pvm also refunds on sstore(0) --- substrate/frame/revive/src/tests/sol/host.rs | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/host.rs b/substrate/frame/revive/src/tests/sol/host.rs index 31412e77e2832..a62f2bb9120c0 100644 --- a/substrate/frame/revive/src/tests/sol/host.rs +++ b/substrate/frame/revive/src/tests/sol/host.rs @@ -774,20 +774,10 @@ fn storage_item_zero_shall_refund_deposit_delegatecall( let result = Caller::delegateCall::abi_decode_returns(&result.data).unwrap(); assert!(result.success, "delegateCall did not succeed"); - if callee_type == FixtureType::Resolc { - // Resolc will not refund - assert_eq!( - get_contract(&caller_addr).total_deposit(), - base_deposit + 32 + 32 + 2, - "PVM contract does not refund deposit on zeroing storage item" - ); - } else { - // solc will refund - assert_eq!( - get_contract(&caller_addr).total_deposit(), - base_deposit, - "EVM contract should refund deposit on zeroing storage item" - ); - } + assert_eq!( + get_contract(&caller_addr).total_deposit(), + base_deposit, + "contract should refund deposit on zeroing storage item" + ); }); } From 448de3bc0756aaffc186ad833117187e7101209a Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 24 Nov 2025 21:11:29 +0000 Subject: [PATCH 15/15] nit --- substrate/frame/revive/src/vm/evm/instructions/host.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/vm/evm/instructions/host.rs b/substrate/frame/revive/src/vm/evm/instructions/host.rs index 44957c0edea85..b3b3c1fefba26 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/host.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/host.rs @@ -154,7 +154,7 @@ fn store_helper<'ext, E: Ext>( interpreter.ext.gas_meter_mut().adjust_gas( charged_amount, - adjust_cost(if value_to_store.is_some() { 32 } else { 0 }, write_outcome.old_len()), + adjust_cost(value_to_store.unwrap_or_default().len() as u32, write_outcome.old_len()), ); ControlFlow::Continue(())