Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Use value copy instead of pointer copy in stateObject.deepCopy #741

Closed
wants to merge 7 commits into from
Closed

Use value copy instead of pointer copy in stateObject.deepCopy #741

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
673e690
Use value copy instead of pointer copy in stateObject.deepCopy
summerpro Jan 21, 2021
7050668
add change log
summerpro Jan 25, 2021
3d6a5d9
Merge branch 'development' into ray/deepCopy
fedekunze Jan 25, 2021
880ad13
Merge branch 'development' into ray/deepCopy
summerpro Jan 28, 2021
9069c70
Merge branch 'development' into ray/deepCopy
summerpro Feb 1, 2021
0f2804f
Merge branch 'development' into ray/deepCopy
summerpro Feb 5, 2021
9347eec
panic when MarshalJSON failed
summerpro Feb 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (api) [\#687](https://github.com/cosmos/ethermint/issues/687) Returns error for a transaction with an incorrect nonce.
* (evm) [\#674](https://github.com/cosmos/ethermint/issues/674) Reset all cache after account data has been committed in `EndBlock` to make sure every node state consistent.
* (evm) [\#672](https://github.com/cosmos/ethermint/issues/672) Fix panic of `wrong Block.Header.AppHash` when restart a node with snapshot.
* (evm) [\#740](https://github.com/cosmos/ethermint/issues/740) Use value copy instead of pointer copy in stateObject.deepCopy.

## [v0.4.0] - 2020-12-15

Expand Down
11 changes: 10 additions & 1 deletion x/evm/types/state_object.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,16 @@ func (so *stateObject) GetCommittedState(_ ethstate.Database, key ethcmn.Hash) e
func (so *stateObject) ReturnGas(gas *big.Int) {}

func (so *stateObject) deepCopy(db *CommitStateDB) *stateObject {
newStateObj := newStateObject(db, so.account)
newAccount := ethermint.ProtoAccount().(*ethermint.EthAccount)
jsonAccount, err := so.account.MarshalJSON()
if err != nil {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems that using value copy is ok here but when error happens during marshal or unmarshal, error will be suppressed and nil will be returned, in this case, any one who invoked deepCopy may encounter nil pointer dereference and there is no way to prevent it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error returned in marshal is usually due to an operating system error. In this case, panic may be more appropriate.

Copy link
Contributor Author

@summerpro summerpro Feb 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote another solution, you can take a look（link）, but I am not familiar with the usage of reflect, and I am worried that there will be any problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freddyli7 Looking forward to your reply

}
err = newAccount.UnmarshalJSON(jsonAccount)
if err != nil {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this for? why don't you copy the existing fields to a new EthAccount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The member variable PubKey of ETHAccount is of interface type, do you have a better suggestion for deep copy of interface type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedekunze Hi, what do you think of this solution? Can it be passed? Are there any possible problems with this solution?

newStateObj := newStateObject(db, newAccount)

newStateObj.code = so.code
newStateObj.dirtyStorage = so.dirtyStorage.Copy()
Expand Down