From 8b83c755821d10baea3fa635a8ae38afce06a636 Mon Sep 17 00:00:00 2001 From: nanocryk <6422796+nanocryk@users.noreply.github.com> Date: Mon, 3 Oct 2022 16:07:53 +0000 Subject: [PATCH 1/6] handle original_storage --- frame/evm/src/runner/stack.rs | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index 886ccfb2f4..550ef38f6c 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -31,7 +31,14 @@ use fp_evm::{CallInfo, CreateInfo, ExecutionInfo, Log, Vicinity}; use frame_support::traits::{Currency, ExistenceRequirement, Get}; use sp_core::{H160, H256, U256}; use sp_runtime::traits::UniqueSaturatedInto; -use sp_std::{boxed::Box, collections::btree_set::BTreeSet, marker::PhantomData, mem, vec::Vec}; +use sp_std::{ + boxed::Box, + cell::RefCell, + collections::{btree_map::BTreeMap, btree_set::BTreeSet}, + marker::PhantomData, + mem, + vec::Vec, +}; #[derive(Default)] pub struct Runner { @@ -495,6 +502,7 @@ pub struct SubstrateStackState<'vicinity, 'config, T> { vicinity: &'vicinity Vicinity, substate: SubstrateStackSubstate<'config>, _marker: PhantomData, + original_storage: RefCell>, } impl<'vicinity, 'config, T: Config> SubstrateStackState<'vicinity, 'config, T> { @@ -509,6 +517,7 @@ impl<'vicinity, 'config, T: Config> SubstrateStackState<'vicinity, 'config, T> { parent: None, }, _marker: PhantomData, + original_storage: RefCell::new(BTreeMap::new()), } } } @@ -573,11 +582,26 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity, } fn storage(&self, address: H160, index: H256) -> H256 { - >::get(address, index) + let value = >::get(address, index); + + let mut original_storage = self.original_storage.borrow_mut(); + if !original_storage.contains_key(&(address, index)) { + let _ = original_storage.insert((address, index), value.clone()); + } + + value } - fn original_storage(&self, _address: H160, _index: H256) -> Option { - None + fn original_storage(&self, address: H160, index: H256) -> Option { + { + let original_storage = self.original_storage.borrow(); + + if let Some(value) = original_storage.get(&(address, index)) { + return Some(value.clone()); + } + } // original_storage borrow is dropped here + + Some(self.storage(address, index)) } fn block_base_fee_per_gas(&self) -> sp_core::U256 { From 31ccd8e255963901b3278d1bc39bb80d08017ba9 Mon Sep 17 00:00:00 2001 From: nanocryk <6422796+nanocryk@users.noreply.github.com> Date: Tue, 4 Oct 2022 10:14:11 +0000 Subject: [PATCH 2/6] only cache original_storage in set_storage --- frame/evm/src/runner/stack.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index 550ef38f6c..602511d84d 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -582,14 +582,7 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity, } fn storage(&self, address: H160, index: H256) -> H256 { - let value = >::get(address, index); - - let mut original_storage = self.original_storage.borrow_mut(); - if !original_storage.contains_key(&(address, index)) { - let _ = original_storage.insert((address, index), value.clone()); - } - - value + >::get(address, index) } fn original_storage(&self, address: H160, index: H256) -> Option { @@ -601,6 +594,8 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity, } } // original_storage borrow is dropped here + // Not being cached means that it was never changed. + // We thus fetch it from storage. Some(self.storage(address, index)) } @@ -653,6 +648,15 @@ where } fn set_storage(&mut self, address: H160, index: H256, value: H256) { + // We cache the current value if this is the first time we modify it + // in the transaction. + let mut original_storage = self.original_storage.borrow_mut(); + if !original_storage.contains_key(&(address, index)) { + let value = >::get(address, index); + let _ = original_storage.insert((address, index), value); + } + + // Then we insert or remove the entry based on the value. if value == H256::default() { log::debug!( target: "evm", From 2db8ec9a447880d02c1210e32cc92dfe0feb6753 Mon Sep 17 00:00:00 2001 From: nanocryk <6422796+nanocryk@users.noreply.github.com> Date: Tue, 4 Oct 2022 13:54:34 +0000 Subject: [PATCH 3/6] test --- ts-tests/tests/test-contract-storage.ts | 46 ++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/ts-tests/tests/test-contract-storage.ts b/ts-tests/tests/test-contract-storage.ts index a08bdedc78..d39fa7fe4c 100644 --- a/ts-tests/tests/test-contract-storage.ts +++ b/ts-tests/tests/test-contract-storage.ts @@ -2,7 +2,7 @@ import { expect } from "chai"; import { AbiItem } from "web3-utils"; import Test from "../build/contracts/Storage.json"; -import { GENESIS_ACCOUNT, GENESIS_ACCOUNT_PRIVATE_KEY } from "./config"; +import { GENESIS_ACCOUNT, GENESIS_ACCOUNT_PRIVATE_KEY, FIRST_CONTRACT_ADDRESS } from "./config"; import { createAndFinalizeBlock, customRequest, describeWithFrontier } from "./util"; describeWithFrontier("Frontier RPC (Contract)", (context) => { @@ -80,4 +80,48 @@ describeWithFrontier("Frontier RPC (Contract)", (context) => { expect(getStorage1.result).to.be.eq(expectedStorage); }); + + it("SSTORE cost should properly take into account transaction initial value", async function () { + this.timeout(5000); + + let nonce = await context.web3.eth.getTransactionCount(GENESIS_ACCOUNT); + + await context.web3.eth.accounts.wallet.add(GENESIS_ACCOUNT_PRIVATE_KEY); + const contract = new context.web3.eth.Contract(TEST_CONTRACT_ABI, FIRST_CONTRACT_ADDRESS, { + from: GENESIS_ACCOUNT, + gasPrice: "0x3B9ACA00", + }); + + const promisify = (inner) => new Promise((resolve, reject) => inner(resolve, reject)); + + let tx1 = contract.methods + .setStorage("0x2A", "0x1") + .send({ from: GENESIS_ACCOUNT, gas: "0x100000", nonce: nonce++ }); + + let tx2 = contract.methods + .setStorage("0x2A", "0x1") + .send({ from: GENESIS_ACCOUNT, gas: "0x100000", nonce: nonce++ }); + + let tx3 = contract.methods + .setStorage("0x2A", "0x2") + .send( + { from: GENESIS_ACCOUNT, gas: "0x100000", nonce: nonce++ }, + async (hash) => await createAndFinalizeBlock(context.web3) + ); + + tx1 = await tx1; + tx2 = await tx2; + tx3 = await tx3; + + // cost minus SSTORE + const baseCost = 24029; + + // going from unset storage to some value (original = 0) + expect(tx1.gasUsed - baseCost).to.be.eq(20000); + // in London config, setting back the same value have cost of warm read + expect(tx2.gasUsed - baseCost).to.be.eq(100); + // - the original storage didn't change in the current transaction + // - the original storage is not zero (otherwise tx1) + expect(tx3.gasUsed - baseCost).to.be.eq(2900); + }); }); From 54a922f5514b76160ceb183cac27c2a166de80b9 Mon Sep 17 00:00:00 2001 From: nanocryk <6422796+nanocryk@users.noreply.github.com> Date: Tue, 4 Oct 2022 13:54:47 +0000 Subject: [PATCH 4/6] clippy --- frame/evm/src/runner/stack.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index 602511d84d..f87945c5db 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -590,7 +590,7 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity, let original_storage = self.original_storage.borrow(); if let Some(value) = original_storage.get(&(address, index)) { - return Some(value.clone()); + return Some(*value); } } // original_storage borrow is dropped here @@ -651,9 +651,11 @@ where // We cache the current value if this is the first time we modify it // in the transaction. let mut original_storage = self.original_storage.borrow_mut(); - if !original_storage.contains_key(&(address, index)) { + + use sp_std::collections::btree_map::Entry::Vacant; + if let Vacant(e) = original_storage.entry((address, index)) { let value = >::get(address, index); - let _ = original_storage.insert((address, index), value); + e.insert(value); } // Then we insert or remove the entry based on the value. From 11fa10134fc39023a7681ba62d911db68f4b5fbc Mon Sep 17 00:00:00 2001 From: nanocryk <6422796+nanocryk@users.noreply.github.com> Date: Tue, 4 Oct 2022 14:43:28 +0000 Subject: [PATCH 5/6] remove RefCell --- frame/evm/src/runner/stack.rs | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index f87945c5db..822b02e4e4 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -33,7 +33,6 @@ use sp_core::{H160, H256, U256}; use sp_runtime::traits::UniqueSaturatedInto; use sp_std::{ boxed::Box, - cell::RefCell, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, marker::PhantomData, mem, @@ -501,8 +500,8 @@ impl<'config> SubstrateStackSubstate<'config> { pub struct SubstrateStackState<'vicinity, 'config, T> { vicinity: &'vicinity Vicinity, substate: SubstrateStackSubstate<'config>, + original_storage: BTreeMap<(H160, H256), H256>, _marker: PhantomData, - original_storage: RefCell>, } impl<'vicinity, 'config, T: Config> SubstrateStackState<'vicinity, 'config, T> { @@ -517,7 +516,7 @@ impl<'vicinity, 'config, T: Config> SubstrateStackState<'vicinity, 'config, T> { parent: None, }, _marker: PhantomData, - original_storage: RefCell::new(BTreeMap::new()), + original_storage: BTreeMap::new(), } } } @@ -586,17 +585,13 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity, } fn original_storage(&self, address: H160, index: H256) -> Option { - { - let original_storage = self.original_storage.borrow(); - - if let Some(value) = original_storage.get(&(address, index)) { - return Some(*value); - } - } // original_storage borrow is dropped here - - // Not being cached means that it was never changed. - // We thus fetch it from storage. - Some(self.storage(address, index)) + if let Some(value) = self.original_storage.get(&(address, index)) { + Some(*value) + } else { + // Not being cached means that it was never changed. + // We thus fetch it from storage. + Some(self.storage(address, index)) + } } fn block_base_fee_per_gas(&self) -> sp_core::U256 { @@ -650,10 +645,8 @@ where fn set_storage(&mut self, address: H160, index: H256, value: H256) { // We cache the current value if this is the first time we modify it // in the transaction. - let mut original_storage = self.original_storage.borrow_mut(); - use sp_std::collections::btree_map::Entry::Vacant; - if let Vacant(e) = original_storage.entry((address, index)) { + if let Vacant(e) = self.original_storage.entry((address, index)) { let value = >::get(address, index); e.insert(value); } @@ -723,7 +716,7 @@ where // // This function exists in EVM because a design issue // (arguably a bug) in SELFDESTRUCT that can cause total - // issurance to be reduced. We do not need to replicate this. + // issuance to be reduced. We do not need to replicate this. } fn touch(&mut self, _address: H160) { From 018eb959e5950c05a426b3aeb0b7e4c5a28558ec Mon Sep 17 00:00:00 2001 From: nanocryk <6422796+nanocryk@users.noreply.github.com> Date: Wed, 5 Oct 2022 08:20:43 +0000 Subject: [PATCH 6/6] don't write to cache if = to original --- frame/evm/src/runner/stack.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/frame/evm/src/runner/stack.rs b/frame/evm/src/runner/stack.rs index 822b02e4e4..0747e03a23 100644 --- a/frame/evm/src/runner/stack.rs +++ b/frame/evm/src/runner/stack.rs @@ -585,13 +585,14 @@ impl<'vicinity, 'config, T: Config> BackendT for SubstrateStackState<'vicinity, } fn original_storage(&self, address: H160, index: H256) -> Option { - if let Some(value) = self.original_storage.get(&(address, index)) { - Some(*value) - } else { - // Not being cached means that it was never changed. - // We thus fetch it from storage. - Some(self.storage(address, index)) - } + // Not being cached means that it was never changed, which means we + // can fetch it from storage. + Some( + self.original_storage + .get(&(address, index)) + .cloned() + .unwrap_or_else(|| self.storage(address, index)), + ) } fn block_base_fee_per_gas(&self) -> sp_core::U256 { @@ -647,8 +648,11 @@ where // in the transaction. use sp_std::collections::btree_map::Entry::Vacant; if let Vacant(e) = self.original_storage.entry((address, index)) { - let value = >::get(address, index); - e.insert(value); + let original = >::get(address, index); + // No need to cache if same value. + if original != value { + e.insert(original); + } } // Then we insert or remove the entry based on the value.