diff --git a/x/vm/statedb/commit_atomicity_test.go b/x/vm/statedb/commit_atomicity_test.go new file mode 100644 index 000000000..1faddecff --- /dev/null +++ b/x/vm/statedb/commit_atomicity_test.go @@ -0,0 +1,139 @@ +package statedb_test + +import ( + "errors" + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/tracing" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + "github.com/cosmos/evm/x/vm/statedb" + + storetypes "cosmossdk.io/store/types" + + "github.com/cosmos/cosmos-sdk/testutil" + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// atomicTestKeeper routes writes through ctx's real KVStore, unlike the +// in-memory-map mocks elsewhere in this package, so a write discarded via +// CacheContext is actually observable as absent. +type atomicTestKeeper struct { + key *storetypes.KVStoreKey + errAddr common.Address +} + +var _ statedb.Keeper = &atomicTestKeeper{} + +func (k *atomicTestKeeper) store(ctx sdk.Context) storetypes.KVStore { return ctx.KVStore(k.key) } + +func (k *atomicTestKeeper) GetAccount(ctx sdk.Context, addr common.Address) *statedb.Account { + bz := k.store(ctx).Get(addr.Bytes()) + if bz == nil { + return nil + } + return &statedb.Account{Balance: new(uint256.Int).SetBytes(bz)} +} + +func (k *atomicTestKeeper) SetAccount(ctx sdk.Context, addr common.Address, acc statedb.Account) error { + if addr == k.errAddr { + return errors.New("blocked") + } + k.store(ctx).Set(addr.Bytes(), acc.Balance.Bytes()) + return nil +} + +func (k *atomicTestKeeper) DeleteAccount(ctx sdk.Context, addr common.Address) error { + if addr == k.errAddr { + return errors.New("blocked") + } + k.store(ctx).Delete(addr.Bytes()) + return nil +} + +func (k *atomicTestKeeper) GetState(sdk.Context, common.Address, common.Hash) common.Hash { + return common.Hash{} +} +func (k *atomicTestKeeper) GetCode(sdk.Context, common.Hash) []byte { return nil } +func (k *atomicTestKeeper) GetCodeHash(sdk.Context, common.Address) common.Hash { return common.Hash{} } +func (k *atomicTestKeeper) ForEachStorage(sdk.Context, common.Address, func(common.Hash, common.Hash) bool) { +} +func (k *atomicTestKeeper) DeleteState(sdk.Context, common.Address, common.Hash) {} +func (k *atomicTestKeeper) SetState(sdk.Context, common.Address, common.Hash, []byte) {} +func (k *atomicTestKeeper) DeleteCode(sdk.Context, []byte) {} +func (k *atomicTestKeeper) SetCode(sdk.Context, []byte, []byte) {} + +func (k *atomicTestKeeper) KVStoreKeys() map[string]*storetypes.KVStoreKey { + return map[string]*storetypes.KVStoreKey{k.key.Name(): k.key} +} + +// TestCommitAtomicity commits a dirty set sorted [credit, blocked, debit]. A +// late failure on blocked must discard the whole commit, including credit, +// which a non-atomic commit would already have written. +func TestCommitAtomicity(t *testing.T) { + credit := common.BigToAddress(big.NewInt(10)) + blocked := common.BigToAddress(big.NewInt(50)) + debit := common.BigToAddress(big.NewInt(90)) + precompileAddr := common.BigToAddress(big.NewInt(1)) // written via cacheCtx, bypassing the journal + + setup := func(name string, errAddr common.Address) *statedb.StateDB { + key := storetypes.NewKVStoreKey(name) + tkey := storetypes.NewTransientStoreKey(name + "_t") + ctx := testutil.DefaultContext(key, tkey).WithEventManager(sdk.NewEventManager()) + return statedb.New(ctx, &atomicTestKeeper{key: key, errAddr: errAddr}, emptyTxConfig) + } + seed := func(db *statedb.StateDB) { + db.AddBalance(credit, uint256.NewInt(1_000_000), tracing.BalanceChangeUnspecified) + db.AddBalance(blocked, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + db.AddBalance(debit, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + } + // persisted reads through db's own keeper/ctx, bypassing db's in-memory + // cache, to check what actually reached the real store. + persisted := func(db *statedb.StateDB, addr common.Address) *statedb.Account { + return db.Keeper().GetAccount(db.GetContext(), addr) + } + // stageViaPrecompile writes directly through the cache context the way a + // real precompile does, bypassing the journal entirely. + stageViaPrecompile := func(t *testing.T, db *statedb.StateDB) { + t.Helper() + cacheCtx, err := db.GetCacheContext() + require.NoError(t, err) + require.NoError(t, db.Keeper().SetAccount(cacheCtx, precompileAddr, statedb.Account{Balance: uint256.NewInt(7)})) + } + + t.Run("late failure discards the whole commit", func(t *testing.T) { + db := setup("fail", blocked) + seed(db) + require.Error(t, db.Commit()) + require.Nil(t, persisted(db, credit)) + }) + + t.Run("late failure discards precompile-staged writes too", func(t *testing.T) { + db := setup("fail_precompile", blocked) + stageViaPrecompile(t, db) + seed(db) + require.Error(t, db.Commit()) + require.Nil(t, persisted(db, credit)) + require.Nil(t, persisted(db, precompileAddr)) + }) + + t.Run("success still persists everything", func(t *testing.T) { + db := setup("ok", common.Address{}) + seed(db) + require.NoError(t, db.Commit()) + require.Equal(t, uint256.NewInt(1_000_000), persisted(db, credit).Balance) + require.Equal(t, uint256.NewInt(1), persisted(db, debit).Balance) + }) + + t.Run("success persists precompile-staged writes too", func(t *testing.T) { + db := setup("ok_precompile", common.Address{}) + stageViaPrecompile(t, db) + seed(db) + require.NoError(t, db.Commit()) + require.Equal(t, uint256.NewInt(1_000_000), persisted(db, credit).Balance) + require.Equal(t, uint256.NewInt(7), persisted(db, precompileAddr).Balance) + }) +} diff --git a/x/vm/statedb/state_object.go b/x/vm/statedb/state_object.go index a93f3dcdd..42bbb6e20 100644 --- a/x/vm/statedb/state_object.go +++ b/x/vm/statedb/state_object.go @@ -138,7 +138,17 @@ func (s *stateObject) AddBalance(amount *uint256.Int) uint256.Int { if amount.IsZero() { return *(s.Balance()) } - return s.SetBalance(new(uint256.Int).Add(s.Balance(), amount)) + + newBalance, overflowed := new(uint256.Int).AddOverflow(s.Balance(), amount) + if overflowed { + panic(fmt.Sprintf( + "state balance overflow for %s: have=%s add=%s", + s.address.Hex(), + s.Balance().String(), + amount.String(), + )) + } + return s.SetBalance(newBalance) } // SubBalance removes amount from s's balance. diff --git a/x/vm/statedb/statedb.go b/x/vm/statedb/statedb.go index 51972629f..9e1daabce 100644 --- a/x/vm/statedb/statedb.go +++ b/x/vm/statedb/statedb.go @@ -712,15 +712,28 @@ func (s *StateDB) RevertToSnapshot(revid int) { s.validRevisions = s.validRevisions[:idx] } -// Commit writes the dirty states to keeper -// the StateDB object should be discarded after committed. +// Commit writes the dirty states to keeper. +// The StateDB object should be discarded after being committed. func (s *StateDB) Commit() error { // writeCache func will exist only when there's a call to a precompile. // It applies all the store updates preformed by precompile calls. if s.writeCache != nil { + // fold the remaining dirty set into the precompile cache so the + // state changes are atomic. + if err := s.commitWithCtx(s.cacheCtx); err != nil { + return err + } s.writeCache() + return nil + } + + // stage writes here so a late failure leaves s.ctx untouched. + cacheCtx, writeCache := s.ctx.CacheContext() + if err := s.commitWithCtx(cacheCtx); err != nil { + return err } - return s.commitWithCtx(s.ctx) + writeCache() + return nil } // FlushToCacheCtx writes the dirty states to keeper using the cacheCtx. diff --git a/x/vm/statedb/statedb_test.go b/x/vm/statedb/statedb_test.go index 0a423665a..8dc409e20 100644 --- a/x/vm/statedb/statedb_test.go +++ b/x/vm/statedb/statedb_test.go @@ -12,14 +12,24 @@ import ( "github.com/ethereum/go-ethereum/crypto" ethparams "github.com/ethereum/go-ethereum/params" "github.com/holiman/uint256" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/cosmos/evm/x/vm/statedb" "github.com/cosmos/evm/x/vm/types/mocks" + storetypes "cosmossdk.io/store/types" + + "github.com/cosmos/cosmos-sdk/testutil" sdk "github.com/cosmos/cosmos-sdk/types" ) +func newTestCtx() sdk.Context { + key := storetypes.NewKVStoreKey("statedb_test") + tkey := storetypes.NewTransientStoreKey("statedb_test_transient") + return testutil.DefaultContext(key, tkey).WithEventManager(sdk.NewEventManager()) +} + var ( address common.Address = common.BigToAddress(big.NewInt(101)) address2 common.Address = common.BigToAddress(big.NewInt(102)) @@ -58,7 +68,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().Empty(acct.Balance) suite.Require().False(acct.HasCodeHash()) - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db = statedb.New(newTestCtx(), keeper, emptyTxConfig) suite.Require().Equal(true, db.Exist(address)) suite.Require().Equal(true, db.Empty(address)) suite.Require().Equal(common.U2560, db.GetBalance(address)) @@ -81,7 +91,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // SelfDestruct - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.HasSelfDestructed(address)) db.SelfDestruct(address) @@ -96,7 +106,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // not accessible from StateDB anymore - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.Exist(address)) // and cleared in keeper too @@ -134,7 +144,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // not accessible from StateDB anymore - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.Exist(address)) // and cleared in keeper too @@ -159,7 +169,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // SelfDestruct - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.HasSelfDestructed(address)) _, _ = db.SelfDestruct6780(address) @@ -172,7 +182,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // Same-tx maintains state - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().True(db.Exist(address)) suite.Require().False(db.HasSelfDestructed(address)) // but code and state are still accessible in dirty state @@ -191,9 +201,9 @@ func (suite *StateDBTestSuite) TestAccount() { } for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{}.WithEventManager(sdk.NewEventManager()) + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(ctx, db) }) } @@ -201,7 +211,7 @@ func (suite *StateDBTestSuite) TestAccount() { func (suite *StateDBTestSuite) TestAccountOverride() { keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) // test balance carry over when overwritten amount := uint256.NewInt(1) @@ -233,7 +243,7 @@ func (suite *StateDBTestSuite) TestDBError() { }}, } for _, tc := range testCases { - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) tc.malleate(db) suite.Require().Error(db.Commit()) } @@ -264,9 +274,9 @@ func (suite *StateDBTestSuite) TestBalance() { for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{}.WithEventManager(sdk.NewEventManager()) + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(db) // check dirty state @@ -329,9 +339,9 @@ func (suite *StateDBTestSuite) TestState() { for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{}.WithEventManager(sdk.NewEventManager()) + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(db) suite.Require().NoError(db.Commit()) @@ -341,7 +351,7 @@ func (suite *StateDBTestSuite) TestState() { } // check ForEachStorage - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db = statedb.New(newTestCtx(), keeper, emptyTxConfig) collected := CollectContractStorage(db) if len(tc.expStates) > 0 { suite.Require().Equal(tc.expStates, collected) @@ -374,7 +384,7 @@ func (suite *StateDBTestSuite) TestCode() { for _, tc := range testCases { suite.Run(tc.name, func() { keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(db) // check dirty state @@ -385,7 +395,7 @@ func (suite *StateDBTestSuite) TestCode() { suite.Require().NoError(db.Commit()) // check again - db = statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db = statedb.New(newTestCtx(), keeper, emptyTxConfig) suite.Require().Equal(tc.expCode, db.GetCode(address)) suite.Require().Equal(len(tc.expCode), db.GetCodeSize(address)) suite.Require().Equal(tc.expCodeHash, db.GetCodeHash(address)) @@ -439,7 +449,7 @@ func (suite *StateDBTestSuite) TestRevertSnapshot() { } for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{}.WithEventManager(sdk.NewEventManager()) + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() { @@ -478,7 +488,7 @@ func (suite *StateDBTestSuite) TestNestedSnapshot() { value1 := common.BigToHash(big.NewInt(1)) value2 := common.BigToHash(big.NewInt(2)) - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) rev1 := db.Snapshot() db.SetState(address, key, value1) @@ -495,7 +505,7 @@ func (suite *StateDBTestSuite) TestNestedSnapshot() { } func (suite *StateDBTestSuite) TestInvalidSnapshotId() { - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) suite.Require().Panics(func() { db.RevertToSnapshot(1) }) @@ -586,7 +596,7 @@ func (suite *StateDBTestSuite) TestAccessList() { } for _, tc := range testCases { - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) tc.malleate(db) } } @@ -598,7 +608,7 @@ func (suite *StateDBTestSuite) TestLog() { txHash, 1, 1, ) - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), txConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), txConfig) data := []byte("hello world") db.AddLog(ðtypes.Log{ Address: address, @@ -648,7 +658,7 @@ func (suite *StateDBTestSuite) TestRefund() { }, 0, true}, } for _, tc := range testCases { - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) if !tc.expPanic { tc.malleate(db) suite.Require().Equal(tc.expRefund, db.GetRefund()) @@ -661,7 +671,7 @@ func (suite *StateDBTestSuite) TestRefund() { } func (suite *StateDBTestSuite) TestIterateStorage() { - ctx := sdk.Context{}.WithEventManager(sdk.NewEventManager()) + ctx := newTestCtx() key1 := common.BigToHash(big.NewInt(1)) value1 := common.BigToHash(big.NewInt(2)) @@ -669,7 +679,7 @@ func (suite *StateDBTestSuite) TestIterateStorage() { value2 := common.BigToHash(big.NewInt(4)) keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) db.SetState(address, key1, value1) db.SetState(address, key2, value2) @@ -725,7 +735,7 @@ func (suite *StateDBTestSuite) TestSetStorage() { for _, tc := range testCases { suite.Run(tc.name, func() { keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}.WithEventManager(sdk.NewEventManager()), keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) for k, v := range tc.prestate { db.SetState(contract, k, v) } @@ -754,3 +764,18 @@ func CollectContractStorage(db vm.StateDB) statedb.Storage { func TestStateDBTestSuite(t *testing.T) { suite.Run(t, &StateDBTestSuite{}) } + +func TestAddBalanceOverflow(t *testing.T) { + // Overflowing uint256 must panic, not silently wrap around. + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) + db.AddBalance(address, new(uint256.Int).SetAllOne(), tracing.BalanceChangeUnspecified) + require.Panics(t, func() { + db.AddBalance(address, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + }) + + // A non-overflowing add still updates the balance normally. + db = statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) + db.AddBalance(address, uint256.NewInt(100), tracing.BalanceChangeUnspecified) + db.AddBalance(address, uint256.NewInt(23), tracing.BalanceChangeUnspecified) + require.Equal(t, uint256.NewInt(123), db.GetBalance(address)) +}