From c3610398b2897586eee5609527acbd14d46422ca Mon Sep 17 00:00:00 2001 From: HuangYi Date: Fri, 30 Jul 2021 17:31:10 +0800 Subject: [PATCH 1/7] keep the original context for GetCommittedState api --- x/evm/keeper/keeper.go | 8 ++++++++ x/evm/keeper/state_transition.go | 25 ++++++++++++++++++------- x/evm/keeper/statedb.go | 14 +++++++++----- x/evm/keeper/statedb_test.go | 20 ++++++++++++++++++++ 4 files changed, 55 insertions(+), 12 deletions(-) diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index 5098098a5c..3cdef28603 100644 --- a/x/evm/keeper/keeper.go +++ b/x/evm/keeper/keeper.go @@ -38,6 +38,8 @@ type Keeper struct { stakingKeeper types.StakingKeeper ctx sdk.Context + // set in `BeginCachedContext`, used by `GetCommittedState` api. + committedCtx sdk.Context // chain ID number obtained from the context's chain id eip155ChainID *big.Int @@ -75,6 +77,11 @@ func NewKeeper( } } +// CommittedCtx returns the committed context +func (k Keeper) CommittedCtx() sdk.Context { + return k.committedCtx +} + // Logger returns a module-specific logger. func (k Keeper) Logger(ctx sdk.Context) log.Logger { return ctx.Logger().With("module", types.ModuleName) @@ -83,6 +90,7 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger { // WithContext sets an updated SDK context to the keeper func (k *Keeper) WithContext(ctx sdk.Context) { k.ctx = ctx + k.committedCtx = ctx } // WithChainID sets the chain id to the local variable in the keeper diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index d9fee80904..9850b6f5c5 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -137,11 +137,8 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT return nil, stacktrace.Propagate(err, "failed to return ethereum transaction as core message") } - // create an ethereum StateTransition instance and run TransitionDb - // we use a ctx context to avoid modifying to state in case EVM msg is reverted - originalCtx := k.ctx - cacheCtx, commit := k.ctx.CacheContext() - k.ctx = cacheCtx + // we use a cached context to avoid modifying to state in case EVM msg is reverted + commit := k.BeginCachedContext() // get the coinbase address from the block proposer coinbase, err := k.GetCoinbaseAddress() @@ -149,6 +146,7 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT return nil, stacktrace.Propagate(err, "failed to obtain coinbase address") } + // create an ethereum EVM instance and run the message evm := k.NewEVM(msg, ethCfg, params, coinbase) k.SetTxHashTransient(tx.Hash()) @@ -163,11 +161,12 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT res.Hash = txHash.Hex() logs := k.GetTxLogs(txHash) - // Commit and switch to original context + // Commit and switch to committed context if !res.Failed() { commit() } - k.ctx = originalCtx + + k.EndCachedContext() // Logs needs to be ignored when tx is reverted // Set the log and bloom filter only when the tx is NOT REVERTED @@ -373,3 +372,15 @@ func (k Keeper) GetCoinbaseAddress() (common.Address, error) { coinbase := common.BytesToAddress(validator.GetOperator()) return coinbase, nil } + +// BeginCachedContext create the cached context +func (k Keeper) BeginCachedContext() (commit func()) { + k.committedCtx = k.ctx + k.ctx, commit = k.ctx.CacheContext() + return +} + +// EndCachedContext recover the committed context +func (k Keeper) EndCachedContext() { + k.ctx = k.committedCtx +} diff --git a/x/evm/keeper/statedb.go b/x/evm/keeper/statedb.go index 8059504e0c..02a312eebc 100644 --- a/x/evm/keeper/statedb.go +++ b/x/evm/keeper/statedb.go @@ -352,10 +352,8 @@ func (k *Keeper) GetRefund() uint64 { // State // ---------------------------------------------------------------------------- -// GetCommittedState returns the value set in store for the given key hash. If the key is not registered -// this function returns the empty hash. -func (k *Keeper) GetCommittedState(addr common.Address, hash common.Hash) common.Hash { - store := prefix.NewStore(k.ctx.KVStore(k.storeKey), types.AddressStoragePrefix(addr)) +func doGetState(ctx sdk.Context, storeKey sdk.StoreKey, addr common.Address, hash common.Hash) common.Hash { + store := prefix.NewStore(ctx.KVStore(storeKey), types.AddressStoragePrefix(addr)) key := types.KeyAddressStorage(addr, hash) value := store.Get(key.Bytes()) @@ -366,10 +364,16 @@ func (k *Keeper) GetCommittedState(addr common.Address, hash common.Hash) common return common.BytesToHash(value) } +// GetCommittedState returns the value set in store for the given key hash. If the key is not registered +// this function returns the empty hash. +func (k *Keeper) GetCommittedState(addr common.Address, hash common.Hash) common.Hash { + return doGetState(k.committedCtx, k.storeKey, addr, hash) +} + // GetState returns the committed state for the given key hash, as all changes are committed directly // to the KVStore. func (k *Keeper) GetState(addr common.Address, hash common.Hash) common.Hash { - return k.GetCommittedState(addr, hash) + return doGetState(k.ctx, k.storeKey, addr, hash) } // SetState sets the given hashes (key, value) to the KVStore. If the value hash is empty, this diff --git a/x/evm/keeper/statedb_test.go b/x/evm/keeper/statedb_test.go index b5b31e26ec..3ecd675262 100644 --- a/x/evm/keeper/statedb_test.go +++ b/x/evm/keeper/statedb_test.go @@ -383,6 +383,26 @@ func (suite *KeeperTestSuite) TestState() { } } +func (suite *KeeperTestSuite) TestCommittedState() { + suite.SetupTest() + + var key = common.BytesToHash([]byte("key")) + var value1 = common.BytesToHash([]byte("value1")) + var value2 = common.BytesToHash([]byte("value2")) + + suite.app.EvmKeeper.SetState(suite.address, key, value1) + + suite.app.EvmKeeper.BeginCachedContext() + + suite.app.EvmKeeper.SetState(suite.address, key, value2) + tmp := suite.app.EvmKeeper.GetState(suite.address, key) + suite.Require().Equal(value2, tmp) + tmp = suite.app.EvmKeeper.GetCommittedState(suite.address, key) + suite.Require().Equal(value1, tmp) + + suite.app.EvmKeeper.EndCachedContext() +} + func (suite *KeeperTestSuite) TestSuicide() { testCases := []struct { name string From 6ba92ad13593c3db8276f527f7d87c97674e1635 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 3 Aug 2021 11:18:58 +0800 Subject: [PATCH 2/7] fix method mutation --- x/evm/keeper/state_transition.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index 9850b6f5c5..93d51df730 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -374,13 +374,13 @@ func (k Keeper) GetCoinbaseAddress() (common.Address, error) { } // BeginCachedContext create the cached context -func (k Keeper) BeginCachedContext() (commit func()) { +func (k *Keeper) BeginCachedContext() (commit func()) { k.committedCtx = k.ctx k.ctx, commit = k.ctx.CacheContext() return } // EndCachedContext recover the committed context -func (k Keeper) EndCachedContext() { +func (k *Keeper) EndCachedContext() { k.ctx = k.committedCtx } From ef14a16751ae806a0fc1ed415905390062bc8d65 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Tue, 3 Aug 2021 13:20:55 +0800 Subject: [PATCH 3/7] keep estimateGas consistant --- x/evm/keeper/grpc_query.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x/evm/keeper/grpc_query.go b/x/evm/keeper/grpc_query.go index 2196cc0f5b..5ecee5f792 100644 --- a/x/evm/keeper/grpc_query.go +++ b/x/evm/keeper/grpc_query.go @@ -444,15 +444,14 @@ func (k Keeper) EstimateGas(c context.Context, req *types.EthCallRequest) (*type args.Gas = (*hexutil.Uint64)(&gas) // Execute the call in an isolated context - sandboxCtx, _ := ctx.CacheContext() - k.WithContext(sandboxCtx) + k.BeginCachedContext() msg := args.ToMessage(req.GasCap) evm := k.NewEVM(msg, ethCfg, params, coinbase) // pass true means execute in query mode, which don't do actual gas refund. rsp, err := k.ApplyMessage(evm, msg, ethCfg, true) - k.WithContext(ctx) + k.EndCachedContext() if err != nil { if errors.Is(stacktrace.RootCause(err), core.ErrIntrinsicGas) { From 06c2797388e81460334ab54835a83a780906c995 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 4 Aug 2021 10:13:00 +0800 Subject: [PATCH 4/7] added test after the original context is recovered --- x/evm/keeper/statedb_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x/evm/keeper/statedb_test.go b/x/evm/keeper/statedb_test.go index 3ecd675262..b0aacb4a43 100644 --- a/x/evm/keeper/statedb_test.go +++ b/x/evm/keeper/statedb_test.go @@ -401,6 +401,9 @@ func (suite *KeeperTestSuite) TestCommittedState() { suite.Require().Equal(value1, tmp) suite.app.EvmKeeper.EndCachedContext() + + tmp = suite.app.EvmKeeper.GetCommittedState(suite.address, key) + suite.Require().Equal(value1, tmp) } func (suite *KeeperTestSuite) TestSuicide() { From d35a219274ae0e347d3f7d7988aec9bc3c9146fd Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 4 Aug 2021 10:23:03 +0800 Subject: [PATCH 5/7] add integration test for the gas consumption of sstore --- .../suites/basic/contracts/Storage.sol | 29 +++++++++++++++++++ .../solidity/suites/basic/test/estimateGas.js | 22 ++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 tests/solidity/suites/basic/contracts/Storage.sol create mode 100644 tests/solidity/suites/basic/test/estimateGas.js diff --git a/tests/solidity/suites/basic/contracts/Storage.sol b/tests/solidity/suites/basic/contracts/Storage.sol new file mode 100644 index 0000000000..35c1d06463 --- /dev/null +++ b/tests/solidity/suites/basic/contracts/Storage.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-3.0 + +pragma solidity ^0.5.11; + +/** + * @title Storage + * @dev Store & retrieve value in a variable + */ +contract Storage { + + uint256 number; + + /** + * @dev Store value in variable + * @param num value to store + */ + function store(uint256 num) public { + number = num + 1; + number = num; + } + + /** + * @dev Return value + * @return value of 'number' + */ + function retrieve() public view returns (uint256){ + return number; + } +} diff --git a/tests/solidity/suites/basic/test/estimateGas.js b/tests/solidity/suites/basic/test/estimateGas.js new file mode 100644 index 0000000000..60e22e8487 --- /dev/null +++ b/tests/solidity/suites/basic/test/estimateGas.js @@ -0,0 +1,22 @@ +const Storage = artifacts.require("Storage") + +contract('Storage', (accounts) => { + + let storage + beforeEach(async () => { + storage = await Storage.new() + }) + + it('estimated gas should match', async () => { + // set new value + let gasUsage = await storage.store.estimateGas(10); + expect(gasUsage.toString()).to.equal('43754'); + + await storage.store(10); + + // set existing value + gasUsage = await storage.store.estimateGas(10); + expect(gasUsage.toString()).to.equal('28754'); + }) + +}) From de71be5f273054264444a209230339561ceaa802 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 4 Aug 2021 13:41:32 +0800 Subject: [PATCH 6/7] test the committed case --- x/evm/keeper/statedb_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x/evm/keeper/statedb_test.go b/x/evm/keeper/statedb_test.go index b0aacb4a43..64a6cffbce 100644 --- a/x/evm/keeper/statedb_test.go +++ b/x/evm/keeper/statedb_test.go @@ -392,7 +392,7 @@ func (suite *KeeperTestSuite) TestCommittedState() { suite.app.EvmKeeper.SetState(suite.address, key, value1) - suite.app.EvmKeeper.BeginCachedContext() + commit := suite.app.EvmKeeper.BeginCachedContext() suite.app.EvmKeeper.SetState(suite.address, key, value2) tmp := suite.app.EvmKeeper.GetState(suite.address, key) @@ -400,10 +400,11 @@ func (suite *KeeperTestSuite) TestCommittedState() { tmp = suite.app.EvmKeeper.GetCommittedState(suite.address, key) suite.Require().Equal(value1, tmp) + commit() suite.app.EvmKeeper.EndCachedContext() tmp = suite.app.EvmKeeper.GetCommittedState(suite.address, key) - suite.Require().Equal(value1, tmp) + suite.Require().Equal(value2, tmp) } func (suite *KeeperTestSuite) TestSuicide() { From a65a4a3d8cf06ff2a981a14cb3e87c0b3baaf857 Mon Sep 17 00:00:00 2001 From: HuangYi Date: Wed, 4 Aug 2021 13:42:32 +0800 Subject: [PATCH 7/7] move methods to keeper module --- x/evm/keeper/keeper.go | 12 ++++++++++++ x/evm/keeper/state_transition.go | 12 ------------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index 3cdef28603..913d7031f8 100644 --- a/x/evm/keeper/keeper.go +++ b/x/evm/keeper/keeper.go @@ -349,3 +349,15 @@ func (k Keeper) ResetAccount(addr common.Address) { k.DeleteCode(addr) k.DeleteAccountStorage(addr) } + +// BeginCachedContext create the cached context +func (k *Keeper) BeginCachedContext() (commit func()) { + k.committedCtx = k.ctx + k.ctx, commit = k.ctx.CacheContext() + return +} + +// EndCachedContext recover the committed context +func (k *Keeper) EndCachedContext() { + k.ctx = k.committedCtx +} diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go index 93d51df730..c03d10d2fa 100644 --- a/x/evm/keeper/state_transition.go +++ b/x/evm/keeper/state_transition.go @@ -372,15 +372,3 @@ func (k Keeper) GetCoinbaseAddress() (common.Address, error) { coinbase := common.BytesToAddress(validator.GetOperator()) return coinbase, nil } - -// BeginCachedContext create the cached context -func (k *Keeper) BeginCachedContext() (commit func()) { - k.committedCtx = k.ctx - k.ctx, commit = k.ctx.CacheContext() - return -} - -// EndCachedContext recover the committed context -func (k *Keeper) EndCachedContext() { - k.ctx = k.committedCtx -}