Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[evm] fix snapshot bug #2802

Merged
merged 3 commits into from
Sep 17, 2021
Merged

[evm] fix snapshot bug #2802

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1abb3c
fix snapshot bug
CoderZhi Sep 14, 2021
158e562
Merge branch 'master' into fix_snapshot_bug
raullenchai Sep 17, 2021
528f0a2
add unit test
CoderZhi Sep 17, 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
2 changes: 1 addition & 1 deletion action/protocol/execution/evm/contract_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func TestCreateContract(t *testing.T) {
addr := identityset.Address(28)
_, err = accountutil.LoadOrCreateAccount(sm, addr.String())
require.NoError(err)
stateDB := NewStateDBAdapter(sm, 0, !cfg.Genesis.IsAleutian(0), cfg.Genesis.IsGreenland(0), hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 0, !cfg.Genesis.IsAleutian(0), cfg.Genesis.IsGreenland(0), cfg.Genesis.IsKamchatka(0), hash.ZeroHash256)
contract := addr.Bytes()
var evmContract common.Address
copy(evmContract[:], contract[:])
Expand Down
1 change: 1 addition & 0 deletions action/protocol/execution/evm/evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ func ExecuteContract(
blkCtx.BlockHeight,
!g.IsAleutian(blkCtx.BlockHeight),
g.IsGreenland(blkCtx.BlockHeight),
g.IsKamchatka(blkCtx.BlockHeight),
actionCtx.ActionHash,
opts...,
)
Expand Down
2 changes: 1 addition & 1 deletion action/protocol/execution/evm/evm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ func TestConstantinople(t *testing.T) {
)
require.NoError(err)

stateDB := NewStateDBAdapter(sm, e.height, !g.IsAleutian(e.height), g.IsGreenland(e.height), hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, e.height, !g.IsAleutian(e.height), g.IsGreenland(e.height), g.IsKamchatka(e.height), hash.ZeroHash256)
ctx = protocol.WithBlockCtx(ctx, protocol.BlockCtx{
Producer: identityset.Address(27),
GasLimit: testutil.TestGasLimit,
Expand Down
18 changes: 14 additions & 4 deletions action/protocol/execution/evm/evmstatedbadapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type (
preimageSnapshot map[int]preimageMap
notFixTopicCopyBug bool
asyncContractTrie bool
fixSnapshotOrder bool
sortCachedContracts bool
usePendingNonce bool
}
Expand Down Expand Up @@ -94,6 +95,7 @@ func NewStateDBAdapter(
blockHeight uint64,
notFixTopicCopyBug bool,
asyncContractTrie bool,
fixSnapshotOrder bool,
executionHash hash.Hash256,
opts ...StateDBAdapterOption,
) *StateDBAdapter {
Expand All @@ -111,6 +113,7 @@ func NewStateDBAdapter(
preimageSnapshot: make(map[int]preimageMap),
notFixTopicCopyBug: notFixTopicCopyBug,
asyncContractTrie: asyncContractTrie,
fixSnapshotOrder: fixSnapshotOrder,
}
for _, opt := range opts {
if err := opt(s); err != nil {
Expand Down Expand Up @@ -464,6 +467,13 @@ func (stateDB *StateDBAdapter) cachedContractAddrs() []hash.Hash160 {

// Snapshot returns the snapshot id
func (stateDB *StateDBAdapter) Snapshot() int {
// save a copy of modified contracts
c := make(contractMap)
if stateDB.fixSnapshotOrder {
for _, addr := range stateDB.cachedContractAddrs() {
c[addr] = stateDB.cachedContract[addr].Snapshot()
}
}
sn := stateDB.sm.Snapshot()
if _, ok := stateDB.suicideSnapshot[sn]; ok {
err := errors.New("unexpected error: duplicate snapshot version")
Expand All @@ -477,10 +487,10 @@ func (stateDB *StateDBAdapter) Snapshot() int {
sa[k] = v
}
stateDB.suicideSnapshot[sn] = sa
// save a copy of modified contracts
c := make(contractMap)
for _, addr := range stateDB.cachedContractAddrs() {
c[addr] = stateDB.cachedContract[addr].Snapshot()
if !stateDB.fixSnapshotOrder {
for _, addr := range stateDB.cachedContractAddrs() {
c[addr] = stateDB.cachedContract[addr].Snapshot()
}
}
stateDB.contractSnapshot[sn] = c
// save a copy of preimages
Expand Down
30 changes: 17 additions & 13 deletions action/protocol/execution/evm/evmstatedbadapter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func TestAddBalance(t *testing.T) {
sm, err := initMockStateManager(ctrl)
require.NoError(err)
addr := common.HexToAddress("02ae2a956d21e8d481c3a69e146633470cf625ec")
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)
addAmount := big.NewInt(40000)
stateDB.AddBalance(addr, addAmount)
amount := stateDB.GetBalance(addr)
Expand All @@ -109,7 +109,7 @@ func TestRefundAPIs(t *testing.T) {

sm, err := initMockStateManager(ctrl)
require.NoError(err)
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)
require.Zero(stateDB.GetRefund())
refund := uint64(1024)
stateDB.AddRefund(refund)
Expand All @@ -123,7 +123,7 @@ func TestEmptyAndCode(t *testing.T) {
sm, err := initMockStateManager(ctrl)
require.NoError(err)
addr := common.HexToAddress("02ae2a956d21e8d481c3a69e146633470cf625ec")
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)
require.True(stateDB.Empty(addr))
stateDB.CreateAccount(addr)
require.True(stateDB.Empty(addr))
Expand All @@ -139,7 +139,7 @@ func TestForEachStorage(t *testing.T) {
sm, err := initMockStateManager(ctrl)
require.NoError(err)
addr := common.HexToAddress("02ae2a956d21e8d481c3a69e146633470cf625ec")
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)
stateDB.CreateAccount(addr)
kvs := map[common.Hash]common.Hash{
common.HexToHash("0123456701234567012345670123456701234567012345670123456701234560"): common.HexToHash("0123456701234567012345670123456701234567012345670123456701234560"),
Expand Down Expand Up @@ -174,20 +174,20 @@ func TestNonce(t *testing.T) {
sm, err := initMockStateManager(ctrl)
require.NoError(err)
addr := common.HexToAddress("02ae2a956d21e8d481c3a69e146633470cf625ec")
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)
require.Equal(uint64(0), stateDB.GetNonce(addr))
stateDB.SetNonce(addr, 1)
require.Equal(uint64(1), stateDB.GetNonce(addr))
}

func TestSnapshotRevertAndCommit(t *testing.T) {
testSnapshotAndRevert := func(cfg config.Config, t *testing.T) {
testSnapshotAndRevert := func(_ config.Config, t *testing.T, async bool) {
require := require.New(t)
ctrl := gomock.NewController(t)

sm, err := initMockStateManager(ctrl)
require.NoError(err)
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, async, true, hash.ZeroHash256)
tests := []stateDBTest{
{
[]bal{
Expand Down Expand Up @@ -377,9 +377,13 @@ func TestSnapshotRevertAndCommit(t *testing.T) {
//[TODO] need e2etest to verify state factory commit/re-open (whether result from state/balance/suicide/exist is same)
}

t.Run("contract snapshot/revert/commit with in memery DB", func(t *testing.T) {
t.Run("contract snapshot/revert/commit with in memory DB", func(t *testing.T) {
cfg := config.Default
testSnapshotAndRevert(cfg, t)
testSnapshotAndRevert(cfg, t, false)
})
t.Run("contract snapshot/revert/commit with async trie in memory DB", func(t *testing.T) {
cfg := config.Default
testSnapshotAndRevert(cfg, t, true)
Copy link
Member

@dustinxie dustinxie Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: is it possible to verify with async = true, fixSnapshotOrder = false, it will lead to that "failed to load root" error? I know this actually happened on the chain, but good to have it in test for record-keeping and as part of history/knowledge base.

if not easy to modify in testSnapshotAndRevert, maybe add a new small test

})
}

Expand All @@ -390,7 +394,7 @@ func TestGetCommittedState(t *testing.T) {

sm, err := initMockStateManager(ctrl)
require.NoError(err)
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)

stateDB.SetState(c1, k1, v1)
// k2 does not exist
Expand Down Expand Up @@ -422,7 +426,7 @@ func TestGetBalanceOnError(t *testing.T) {
for _, err := range errs {
sm.EXPECT().State(gomock.Any(), gomock.Any()).Return(uint64(0), err).Times(1)
addr := common.HexToAddress("test address")
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)
amount := stateDB.GetBalance(addr)
assert.Equal(t, big.NewInt(0), amount)
}
Expand All @@ -434,7 +438,7 @@ func TestPreimage(t *testing.T) {

sm, err := initMockStateManager(ctrl)
require.NoError(err)
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256)

stateDB.AddPreimage(common.BytesToHash(v1[:]), []byte("cat"))
stateDB.AddPreimage(common.BytesToHash(v2[:]), []byte("dog"))
Expand Down Expand Up @@ -466,7 +470,7 @@ func TestSortMap(t *testing.T) {
}

testFunc := func(t *testing.T, sm *mock_chainmanager.MockStateManager, opts ...StateDBAdapterOption) bool {
stateDB := NewStateDBAdapter(sm, 1, true, false, hash.ZeroHash256, opts...)
stateDB := NewStateDBAdapter(sm, 1, true, false, true, hash.ZeroHash256, opts...)
size := 10

for i := 0; i < size; i++ {
Expand Down