fix: Delete state on empty common.Hash{}

Previous to this change, keeper SetState() only deletes when the
len(value) == 0. However, since value in the StateDB is a common.Hash{},
this will always be the HashLength. Deleting the value when the empty
hash is set will also ensure specific no-op changes will not be committed,
e.g. Creating a state, then deleting it in the same tx will result in
no state added.

x/evm/genesis.go

@@ -79,7 +79,7 @@ func InitGenesis(
 		k.SetCode(ctx, codeHash.Bytes(), code)
 
 		for _, storage := range account.Storage {
-			k.SetState(ctx, address, common.HexToHash(storage.Key), common.HexToHash(storage.Value).Bytes())
+			k.SetState(ctx, address, common.HexToHash(storage.Key), common.HexToHash(storage.Value))
 		}
 	}
 

x/evm/keeper/statedb.go

@@ -168,14 +168,16 @@ func (k *Keeper) SetAccount(ctx sdk.Context, addr common.Address, account stated
 }
 
 // SetState update contract storage, delete if value is empty.
-func (k *Keeper) SetState(ctx sdk.Context, addr common.Address, key common.Hash, value []byte) {
+func (k *Keeper) SetState(ctx sdk.Context, addr common.Address, key, value common.Hash) {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.AddressStoragePrefix(addr))
 	action := "updated"
-	if len(value) == 0 {
+
+	// Value is always HashLength long, so check if empty by comparing to zero hash
+	if (value == common.Hash{}) {
 		store.Delete(key.Bytes())
 		action = "deleted"
 	} else {
-		store.Set(key.Bytes(), value)
+		store.Set(key.Bytes(), value.Bytes())
 	}
 	k.Logger(ctx).Debug(
 		fmt.Sprintf("state %s", action),
@@ -227,7 +229,7 @@ func (k *Keeper) DeleteAccount(ctx sdk.Context, addr common.Address) error {
 
 	// clear storage
 	k.ForEachStorage(ctx, addr, func(key, _ common.Hash) bool {
-		k.SetState(ctx, addr, key, nil)
+		k.SetState(ctx, addr, key, common.Hash{})
 		return true
 	})
 

x/evm/statedb/interfaces.go

@@ -44,7 +44,7 @@ type Keeper interface {
 
 	// Write methods, only called by `StateDB.Commit()`
 	SetAccount(ctx sdk.Context, addr common.Address, account Account) error
-	SetState(ctx sdk.Context, addr common.Address, key common.Hash, value []byte)
+	SetState(ctx sdk.Context, addr common.Address, key, value common.Hash)
 	SetCode(ctx sdk.Context, codeHash []byte, code []byte)
 	SetBalance(ctx sdk.Context, addr common.Address, amount *big.Int) error
 	DeleteAccount(ctx sdk.Context, addr common.Address) error

x/evm/statedb/statedb.go

@@ -307,19 +307,15 @@ func (s *StateDB) SetCode(addr common.Address, code []byte) {
 
 // SetState sets the contract state.
 func (s *StateDB) SetState(addr common.Address, key, value common.Hash) {
-	// Skip dirty noop state changes
-	currentState := s.keeper.GetState(s.ctx.CurrentCtx(), addr, key)
-	if currentState == value {
-		return
-	}
-
-	// Skip committed noop state changes
-	commitedState := s.GetCommittedState(addr, key)
-	if commitedState == value {
-		return
-	}
-
-	s.keeper.SetState(s.ctx.CurrentCtx(), addr, key, value.Bytes())
+	// We cannot attempt to skip noop changes by just checking committed state
+	// Example:
+	// 1. With committed state to 0x0
+	// 2. Dirty change to 0x1
+	// 3. Dirty change to 0x0 - cannot skip this
+	// 4. Commit
+	//
+	// End result: 0x0, but we cannot skip step 3 or it will be incorrectly 0x1
+	s.keeper.SetState(s.ctx.CurrentCtx(), addr, key, value)
 }
 
 // Suicide marks the given account as suicided.

x/evm/statedb/statedb_test.go

@@ -224,21 +224,16 @@ func (suite *StateDBTestSuite) TestState() {
 	}{
 		{"empty state", func(db *statedb.StateDB) {
 		}, nil},
-		{"set empty value", func(db *statedb.StateDB) {
+
+		{"set empty value deletes", func(db *statedb.StateDB) {
 			db.SetState(address, key1, common.Hash{})
-		}, map[common.Hash]common.Hash{
-			// empty value still persisted
-			key1: common.Hash{},
-		}},
+		}, map[common.Hash]common.Hash{}},
+
 		{"noop state change", func(db *statedb.StateDB) {
-			// TODO: This doesn't actually change anything compared to committed state.
-			// Is this okay?
 			db.SetState(address, key1, value1)
 			db.SetState(address, key1, common.Hash{})
-		}, map[common.Hash]common.Hash{
-			// Still sets the key to an empty value even if there is no overall change
-			key1: common.Hash{},
-		}},
+		}, map[common.Hash]common.Hash{}},
+
 		{"set state", func(db *statedb.StateDB) {
 			// check empty initial state
 			suite.Require().Equal(common.Hash{}, db.GetState(address, key1))