From 1c97c99773db946dd01a828dbe56f5c2ea57cb6a Mon Sep 17 00:00:00 2001 From: summerpro <974741468@qq.com> Date: Tue, 29 Dec 2020 18:44:00 +0800 Subject: [PATCH 1/6] Roll back CommitStateDB after failing to execute handler in evm module --- x/evm/handler.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/x/evm/handler.go b/x/evm/handler.go index 9b99e2bf3..08fc80aac 100644 --- a/x/evm/handler.go +++ b/x/evm/handler.go @@ -14,16 +14,27 @@ import ( // NewHandler returns a handler for Ethermint type messages. func NewHandler(k Keeper) sdk.Handler { - return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { + return func(ctx sdk.Context, msg sdk.Msg) (result *sdk.Result, err error) { + snapshotStateDB := k.CommitStateDB.Copy() + defer func() { + if r := recover(); r != nil { + k.CommitStateDB = snapshotStateDB + panic(r) + } + }() ctx = ctx.WithEventManager(sdk.NewEventManager()) switch msg := msg.(type) { case types.MsgEthereumTx: - return handleMsgEthereumTx(ctx, k, msg) + result, err = handleMsgEthereumTx(ctx, k, msg) case types.MsgEthermint: - return handleMsgEthermint(ctx, k, msg) + result, err = handleMsgEthermint(ctx, k, msg) default: return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized %s message type: %T", ModuleName, msg) } + if err != nil { + k.CommitStateDB = snapshotStateDB + } + return result, err } } From 081d78ceee893e0c42a1eb77b979192db4d88bf6 Mon Sep 17 00:00:00 2001 From: summerpro <974741468@qq.com> Date: Tue, 29 Dec 2020 22:12:12 +0800 Subject: [PATCH 2/6] add function CopyCommitStateDB --- x/evm/handler.go | 4 +-- x/evm/types/statedb.go | 67 +++++++++++++++++++++++------------------- 2 files changed, 38 insertions(+), 33 deletions(-) diff --git a/x/evm/handler.go b/x/evm/handler.go index 08fc80aac..75f7988f1 100644 --- a/x/evm/handler.go +++ b/x/evm/handler.go @@ -18,7 +18,7 @@ func NewHandler(k Keeper) sdk.Handler { snapshotStateDB := k.CommitStateDB.Copy() defer func() { if r := recover(); r != nil { - k.CommitStateDB = snapshotStateDB + types.CopyCommitStateDB(snapshotStateDB, k.CommitStateDB) panic(r) } }() @@ -32,7 +32,7 @@ func NewHandler(k Keeper) sdk.Handler { return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized %s message type: %T", ModuleName, msg) } if err != nil { - k.CommitStateDB = snapshotStateDB + types.CopyCommitStateDB(snapshotStateDB, k.CommitStateDB) } return result, err } diff --git a/x/evm/types/statedb.go b/x/evm/types/statedb.go index b3fcd5889..0a39d5623 100644 --- a/x/evm/types/statedb.go +++ b/x/evm/types/statedb.go @@ -756,63 +756,68 @@ func (csdb *CommitStateDB) CreateAccount(addr ethcmn.Address) { } } + // Copy creates a deep, independent copy of the state. // // NOTE: Snapshots of the copied state cannot be applied to the copy. func (csdb *CommitStateDB) Copy() *CommitStateDB { - csdb.lock.Lock() - defer csdb.lock.Unlock() // copy all the basic fields, initialize the memory ones - state := &CommitStateDB{ - ctx: csdb.ctx, - storeKey: csdb.storeKey, - paramSpace: csdb.paramSpace, - accountKeeper: csdb.accountKeeper, - stateObjects: []stateEntry{}, - addressToObjectIndex: make(map[ethcmn.Address]int), - stateObjectsDirty: make(map[ethcmn.Address]struct{}), - refund: csdb.refund, - logSize: csdb.logSize, - preimages: make([]preimageEntry, len(csdb.preimages)), - hashToPreimageIndex: make(map[ethcmn.Hash]int, len(csdb.hashToPreimageIndex)), - journal: newJournal(), - } + state := &CommitStateDB{} + CopyCommitStateDB(csdb, state) + + return state +} + +func CopyCommitStateDB(from, to *CommitStateDB) { + from.lock.Lock() + defer from.lock.Unlock() + + to.ctx = from.ctx + to.storeKey = from.storeKey + to.paramSpace = from.paramSpace + to.accountKeeper = from.accountKeeper + to.stateObjects = []stateEntry{} + to.addressToObjectIndex = make(map[ethcmn.Address]int) + to.stateObjectsDirty = make(map[ethcmn.Address]struct{}) + to.refund = from.refund + to.logSize = from.logSize + to.preimages = make([]preimageEntry, len(from.preimages)) + to.hashToPreimageIndex = make(map[ethcmn.Hash]int, len(from.hashToPreimageIndex)) + to.journal = newJournal() // copy the dirty states, logs, and preimages - for _, dirty := range csdb.journal.dirties { + for _, dirty := range from.journal.dirties { // There is a case where an object is in the journal but not in the // stateObjects: OOG after touch on ripeMD prior to Byzantium. Thus, we // need to check for nil. // // Ref: https://github.com/ethereum/go-ethereum/pull/16485#issuecomment-380438527 - if idx, exist := csdb.addressToObjectIndex[dirty.address]; exist { - state.stateObjects = append(state.stateObjects, stateEntry{ + if idx, exist := from.addressToObjectIndex[dirty.address]; exist { + to.stateObjects = append(to.stateObjects, stateEntry{ address: dirty.address, - stateObject: csdb.stateObjects[idx].stateObject.deepCopy(state), + stateObject: from.stateObjects[idx].stateObject.deepCopy(to), }) - state.addressToObjectIndex[dirty.address] = len(state.stateObjects) - 1 - state.stateObjectsDirty[dirty.address] = struct{}{} + to.addressToObjectIndex[dirty.address] = len(to.stateObjects) - 1 + to.stateObjectsDirty[dirty.address] = struct{}{} } } // Above, we don't copy the actual journal. This means that if the copy is // copied, the loop above will be a no-op, since the copy's journal is empty. // Thus, here we iterate over stateObjects, to enable copies of copies. - for addr := range csdb.stateObjectsDirty { - if idx, exist := state.addressToObjectIndex[addr]; !exist { - state.setStateObject(csdb.stateObjects[idx].stateObject.deepCopy(state)) - state.stateObjectsDirty[addr] = struct{}{} + for addr := range from.stateObjectsDirty { + if idx, exist := to.addressToObjectIndex[addr]; !exist { + to.setStateObject(from.stateObjects[idx].stateObject.deepCopy(to)) + to.stateObjectsDirty[addr] = struct{}{} } } // copy pre-images - for i, preimageEntry := range csdb.preimages { - state.preimages[i] = preimageEntry - state.hashToPreimageIndex[preimageEntry.hash] = i + for i, preimageEntry := range from.preimages { + to.preimages[i] = preimageEntry + to.hashToPreimageIndex[preimageEntry.hash] = i } - - return state } // ForEachStorage iterates over each storage items, all invoke the provided From 04de4b8954ef048f007e750d634746cabff8fbc4 Mon Sep 17 00:00:00 2001 From: summerpro <974741468@qq.com> Date: Wed, 30 Dec 2020 15:12:31 +0800 Subject: [PATCH 3/6] add comment --- x/evm/handler.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/x/evm/handler.go b/x/evm/handler.go index 75f7988f1..99d3a969a 100644 --- a/x/evm/handler.go +++ b/x/evm/handler.go @@ -16,6 +16,15 @@ import ( func NewHandler(k Keeper) sdk.Handler { return func(ctx sdk.Context, msg sdk.Msg) (result *sdk.Result, err error) { snapshotStateDB := k.CommitStateDB.Copy() + // If the gas is insufficient during the execution of the "handler", + // panic will be thrown from the function "ConsumeGas" and finally + // caught by the function "runTx" from Cosmos. The function "runTx" + // will think that the execution of Msg has failed and the modified + // data in the Store will not take effect. + // The fault is that the modified data in CommitStateDB has not been + // rolled back, resulting in bad data. + // Therefore, the code here specifically deals with this situation. + // See https://github.com/cosmos/ethermint/issues/668 for more information. defer func() { if r := recover(); r != nil { types.CopyCommitStateDB(snapshotStateDB, k.CommitStateDB) From 4989c3e101a916f42f84af603de14f56d2dd542b Mon Sep 17 00:00:00 2001 From: summerpro <974741468@qq.com> Date: Wed, 30 Dec 2020 16:06:22 +0800 Subject: [PATCH 4/6] add comment --- x/evm/handler.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/x/evm/handler.go b/x/evm/handler.go index 99d3a969a..9eda3e079 100644 --- a/x/evm/handler.go +++ b/x/evm/handler.go @@ -16,17 +16,31 @@ import ( func NewHandler(k Keeper) sdk.Handler { return func(ctx sdk.Context, msg sdk.Msg) (result *sdk.Result, err error) { snapshotStateDB := k.CommitStateDB.Copy() + + // The "recover" code here is used to solve the problem of dirty data + // in CommitStateDB due to insufficient gas. + + // The following is a detailed description: // If the gas is insufficient during the execution of the "handler", // panic will be thrown from the function "ConsumeGas" and finally // caught by the function "runTx" from Cosmos. The function "runTx" // will think that the execution of Msg has failed and the modified // data in the Store will not take effect. - // The fault is that the modified data in CommitStateDB has not been - // rolled back, resulting in bad data. + + // Stacktraceļ¼šrunTx->runMsgs->handler->...->gaskv.Store.Set->ConsumeGas + + // The problem is that when the modified data in the Store does not take + // effect, the data in the modified CommitStateDB is not rolled back, + // they take effect, and dirty data is generated. // Therefore, the code here specifically deals with this situation. // See https://github.com/cosmos/ethermint/issues/668 for more information. defer func() { if r := recover(); r != nil { + // We first used "k.CommitStateDB = snapshotStateDB" to roll back + // CommitStateDB, but this can only change the CommitStateDB in the + // current Keeper object, but the Keeper object will be destroyed + // soon, it is not a global variable, so the content pointed to by + // the CommitStateDB pointer can be modified to take effect. types.CopyCommitStateDB(snapshotStateDB, k.CommitStateDB) panic(r) } From 1b271350465a17a22673ece565f3dcd7f6eed9be Mon Sep 17 00:00:00 2001 From: summerpro <974741468@qq.com> Date: Thu, 31 Dec 2020 14:03:29 +0800 Subject: [PATCH 5/6] Add ut about the dirty data generated by CommitStateDB --- x/evm/handler_test.go | 112 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index 3aefc98a5..f66e62120 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -2,6 +2,7 @@ package evm_test import ( "crypto/ecdsa" + "encoding/json" "fmt" "math/big" "strings" @@ -415,3 +416,114 @@ func (suite *EvmTestSuite) TestSendTransaction() { suite.Require().NoError(err) suite.Require().NotNil(result) } + + +func (suite *EvmTestSuite) TestOutOfGasWhenDeployContract() { + // Test contract: + //http://remix.ethereum.org/#optimize=false&evmVersion=istanbul&version=soljson-v0.5.15+commit.6a57276f.js + //2_Owner.sol + // + //pragma solidity >=0.4.22 <0.7.0; + // + ///** + // * @title Owner + // * @dev Set & change owner + // */ + //contract Owner { + // + // address private owner; + // + // // event for EVM logging + // event OwnerSet(address indexed oldOwner, address indexed newOwner); + // + // // modifier to check if caller is owner + // modifier isOwner() { + // // If the first argument of 'require' evaluates to 'false', execution terminates and all + // // changes to the state and to Ether balances are reverted. + // // This used to consume all gas in old EVM versions, but not anymore. + // // It is often a good idea to use 'require' to check if functions are called correctly. + // // As a second argument, you can also provide an explanation about what went wrong. + // require(msg.sender == owner, "Caller is not owner"); + // _; + //} + // + // /** + // * @dev Set contract deployer as owner + // */ + // constructor() public { + // owner = msg.sender; // 'msg.sender' is sender of current call, contract deployer for a constructor + // emit OwnerSet(address(0), owner); + //} + // + // /** + // * @dev Change owner + // * @param newOwner address of new owner + // */ + // function changeOwner(address newOwner) public isOwner { + // emit OwnerSet(owner, newOwner); + // owner = newOwner; + //} + // + // /** + // * @dev Return owner address + // * @return address of owner + // */ + // function getOwner() external view returns (address) { + // return owner; + //} + //} + + // Deploy contract - Owner.sol + gasLimit := uint64(1) + suite.ctx = suite.ctx.WithGasMeter(sdk.NewGasMeter(gasLimit)) + gasPrice := big.NewInt(10000) + + priv, err := ethsecp256k1.GenerateKey() + suite.Require().NoError(err, "failed to create key") + + bytecode := common.FromHex("0x608060405234801561001057600080fd5b50336000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055506000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16600073ffffffffffffffffffffffffffffffffffffffff167f342827c97908e5e2f71151c08502a66d44b6f758e3ac2f1de95f02eb95f0a73560405160405180910390a36102c4806100dc6000396000f3fe608060405234801561001057600080fd5b5060043610610053576000357c010000000000000000000000000000000000000000000000000000000090048063893d20e814610058578063a6f9dae1146100a2575b600080fd5b6100606100e6565b604051808273ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff16815260200191505060405180910390f35b6100e4600480360360208110156100b857600080fd5b81019080803573ffffffffffffffffffffffffffffffffffffffff16906020019092919050505061010f565b005b60008060009054906101000a900473ffffffffffffffffffffffffffffffffffffffff16905090565b6000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff163373ffffffffffffffffffffffffffffffffffffffff16146101d1576040517f08c379a00000000000000000000000000000000000000000000000000000000081526004018080602001828103825260138152602001807f43616c6c6572206973206e6f74206f776e65720000000000000000000000000081525060200191505060405180910390fd5b8073ffffffffffffffffffffffffffffffffffffffff166000809054906101000a900473ffffffffffffffffffffffffffffffffffffffff1673ffffffffffffffffffffffffffffffffffffffff167f342827c97908e5e2f71151c08502a66d44b6f758e3ac2f1de95f02eb95f0a73560405160405180910390a3806000806101000a81548173ffffffffffffffffffffffffffffffffffffffff021916908373ffffffffffffffffffffffffffffffffffffffff1602179055505056fea265627a7a72315820f397f2733a89198bc7fed0764083694c5b828791f39ebcbc9e414bccef14b48064736f6c63430005100032") + tx := types.NewMsgEthereumTx(1, nil, big.NewInt(0), gasLimit, gasPrice, bytecode) + tx.Sign(big.NewInt(3), priv.ToECDSA()) + suite.Require().NoError(err) + + snapshotCommitStateDBJson, err := json.Marshal(suite.app.EvmKeeper.CommitStateDB) + suite.Require().Nil(err) + + defer func() { + if r := recover(); r != nil { + currentCommitStateDBJson, err := json.Marshal(suite.app.EvmKeeper.CommitStateDB) + suite.Require().Nil(err) + suite.Require().Equal(snapshotCommitStateDBJson, currentCommitStateDBJson) + }else { + suite.Require().Fail("panic did not happen") + } + }() + + suite.handler(suite.ctx, tx) + suite.Require().Fail("panic did not happen") +} + + +func (suite *EvmTestSuite) TestErrorWhenDeployContract() { + gasLimit := uint64(1000000) + gasPrice := big.NewInt(10000) + + priv, err := ethsecp256k1.GenerateKey() + suite.Require().NoError(err, "failed to create key") + + bytecode := common.FromHex("0xa6f9dae10000000000000000000000006a82e4a67715c8412a9114fbd2cbaefbc8181424") + + tx := types.NewMsgEthereumTx(1, nil, big.NewInt(0), gasLimit, gasPrice, bytecode) + tx.Sign(big.NewInt(3), priv.ToECDSA()) + suite.Require().NoError(err) + + snapshotCommitStateDBJson, err := json.Marshal(suite.app.EvmKeeper.CommitStateDB) + suite.Require().Nil(err) + + _, sdkErr := suite.handler(suite.ctx, tx) + suite.Require().NotNil(sdkErr) + + currentCommitStateDBJson, err := json.Marshal(suite.app.EvmKeeper.CommitStateDB) + suite.Require().Nil(err) + suite.Require().Equal(snapshotCommitStateDBJson, currentCommitStateDBJson) +} \ No newline at end of file From f4c1ecd2f8f36d17fe189b98ee0730a8fb5074ad Mon Sep 17 00:00:00 2001 From: summerpro <974741468@qq.com> Date: Thu, 31 Dec 2020 14:25:59 +0800 Subject: [PATCH 6/6] format code --- x/evm/handler_test.go | 6 ++---- x/evm/types/statedb.go | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go index f66e62120..7d60c5b96 100644 --- a/x/evm/handler_test.go +++ b/x/evm/handler_test.go @@ -417,7 +417,6 @@ func (suite *EvmTestSuite) TestSendTransaction() { suite.Require().NotNil(result) } - func (suite *EvmTestSuite) TestOutOfGasWhenDeployContract() { // Test contract: //http://remix.ethereum.org/#optimize=false&evmVersion=istanbul&version=soljson-v0.5.15+commit.6a57276f.js @@ -494,7 +493,7 @@ func (suite *EvmTestSuite) TestOutOfGasWhenDeployContract() { currentCommitStateDBJson, err := json.Marshal(suite.app.EvmKeeper.CommitStateDB) suite.Require().Nil(err) suite.Require().Equal(snapshotCommitStateDBJson, currentCommitStateDBJson) - }else { + } else { suite.Require().Fail("panic did not happen") } }() @@ -503,7 +502,6 @@ func (suite *EvmTestSuite) TestOutOfGasWhenDeployContract() { suite.Require().Fail("panic did not happen") } - func (suite *EvmTestSuite) TestErrorWhenDeployContract() { gasLimit := uint64(1000000) gasPrice := big.NewInt(10000) @@ -526,4 +524,4 @@ func (suite *EvmTestSuite) TestErrorWhenDeployContract() { currentCommitStateDBJson, err := json.Marshal(suite.app.EvmKeeper.CommitStateDB) suite.Require().Nil(err) suite.Require().Equal(snapshotCommitStateDBJson, currentCommitStateDBJson) -} \ No newline at end of file +} diff --git a/x/evm/types/statedb.go b/x/evm/types/statedb.go index 0a39d5623..5d7410015 100644 --- a/x/evm/types/statedb.go +++ b/x/evm/types/statedb.go @@ -756,7 +756,6 @@ func (csdb *CommitStateDB) CreateAccount(addr ethcmn.Address) { } } - // Copy creates a deep, independent copy of the state. // // NOTE: Snapshots of the copied state cannot be applied to the copy.