From 903bf512a9d0deecdd6d3e43eb9152bb19a28582 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 23 Apr 2024 13:34:51 +0200 Subject: [PATCH 1/7] core/state: better randomized testing (postcheck) on journalling --- core/state/access_list.go | 34 ++++++++++++++++++++++++++ core/state/statedb_test.go | 21 ++++++++++++++++- core/state/transient_storage.go | 42 ++++++++++++++++++++++++++++++--- 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/core/state/access_list.go b/core/state/access_list.go index 718bf17cf74..ad7ad385297 100644 --- a/core/state/access_list.go +++ b/core/state/access_list.go @@ -17,7 +17,10 @@ package state import ( + "fmt" "maps" + "slices" + "strings" "github.com/ethereum/go-ethereum/common" ) @@ -130,3 +133,34 @@ func (al *accessList) DeleteSlot(address common.Address, slot common.Hash) { func (al *accessList) DeleteAddress(address common.Address) { delete(al.addresses, address) } + +// Equal returns true if the two access lists are identical +func (al *accessList) Equal(other *accessList) bool { + if !maps.Equal(al.addresses, other.addresses) { + return false + } + return slices.EqualFunc(al.slots, other.slots, + func(m map[common.Hash]struct{}, m2 map[common.Hash]struct{}) bool { + return maps.Equal(m, m2) + }) +} + +func (al *accessList) PrettyPrint() string { + out := new(strings.Builder) + var sortedAddrs []common.Address + for addr, _ := range al.addresses { + sortedAddrs = append(sortedAddrs, addr) + } + slices.SortFunc(sortedAddrs, common.Address.Cmp) + for _, addr := range sortedAddrs { + idx := al.addresses[addr] + fmt.Fprintf(out, "%#x : (idx %d)\n", addr, idx) + if idx >= 0 { + slotmap := al.slots[idx] + for h, _ := range slotmap { + fmt.Fprintf(out, " %#x\n", h) + } + } + } + return out.String() +} diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index 1a3eccfe10b..3ce0324e665 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -21,6 +21,7 @@ import ( "encoding/binary" "errors" "fmt" + "maps" "math" "math/rand" "reflect" @@ -609,11 +610,29 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error { return checkeq("GetState("+key.Hex()+")", checkstate.GetState(addr, key), value) }) } + // Check transient storage. + { + have := state.transientStorage + want := checkstate.transientStorage + eq := maps.EqualFunc(have, want, + func(a Storage, b Storage) bool { + return maps.Equal(a, b) + }) + if !eq { + return fmt.Errorf("transient storage differs ,have\n%v\nwant\n%v", + have.PrettyPrint(), + want.PrettyPrint()) + } + } if err != nil { return err } } - + if !checkstate.accessList.Equal(state.accessList) { // Check access lists + return fmt.Errorf("AccessLists are wrong, have \n%v\nwant\n%v", + checkstate.accessList.PrettyPrint(), + state.accessList.PrettyPrint()) + } if state.GetRefund() != checkstate.GetRefund() { return fmt.Errorf("got GetRefund() == %d, want GetRefund() == %d", state.GetRefund(), checkstate.GetRefund()) diff --git a/core/state/transient_storage.go b/core/state/transient_storage.go index 66e563efa73..735b03956ac 100644 --- a/core/state/transient_storage.go +++ b/core/state/transient_storage.go @@ -17,6 +17,10 @@ package state import ( + "fmt" + "slices" + "strings" + "github.com/ethereum/go-ethereum/common" ) @@ -30,10 +34,19 @@ func newTransientStorage() transientStorage { // Set sets the transient-storage `value` for `key` at the given `addr`. func (t transientStorage) Set(addr common.Address, key, value common.Hash) { - if _, ok := t[addr]; !ok { - t[addr] = make(Storage) + if value == (common.Hash{}) { // this is a 'delete' + if _, ok := t[addr]; ok { + delete(t[addr], key) + if len(t[addr]) == 0 { + delete(t, addr) + } + } + } else { + if _, ok := t[addr]; !ok { + t[addr] = make(Storage) + } + t[addr][key] = value } - t[addr][key] = value } // Get gets the transient storage for `key` at the given `addr`. @@ -53,3 +66,26 @@ func (t transientStorage) Copy() transientStorage { } return storage } + +func (t transientStorage) PrettyPrint() string { + out := new(strings.Builder) + var sortedAddrs []common.Address + for addr := range t { + sortedAddrs = append(sortedAddrs, addr) + slices.SortFunc(sortedAddrs, common.Address.Cmp) + } + + for _, addr := range sortedAddrs { + fmt.Fprintf(out, "%#x:", addr) + var sortedKeys []common.Hash + storage := t[addr] + for key := range storage { + sortedKeys = append(sortedKeys, key) + } + slices.SortFunc(sortedKeys, common.Hash.Cmp) + for _, key := range sortedKeys { + fmt.Fprintf(out, " %X : %X\n", key, storage[key]) + } + } + return out.String() +} From 3380364a4a60878083dc3e11f21e7b2894ab4473 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 23 Apr 2024 14:08:55 +0200 Subject: [PATCH 2/7] core/state: also check created-flag in rando-test --- core/state/state_object.go | 24 ++++++++++++------------ core/state/statedb_test.go | 4 ++++ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/core/state/state_object.go b/core/state/state_object.go index aa748f08ac3..d3d20c3dc48 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -459,22 +459,22 @@ func (s *stateObject) setBalance(amount *uint256.Int) { func (s *stateObject) deepCopy(db *StateDB) *stateObject { obj := &stateObject{ - db: db, - address: s.address, - addrHash: s.addrHash, - origin: s.origin, - data: s.data, + db: db, + address: s.address, + addrHash: s.addrHash, + origin: s.origin, + data: s.data, + code: s.code, + originStorage: s.originStorage.Copy(), + pendingStorage: s.pendingStorage.Copy(), + dirtyStorage: s.dirtyStorage.Copy(), + dirtyCode: s.dirtyCode, + selfDestructed: s.selfDestructed, + newContract: s.newContract, } if s.trie != nil { obj.trie = db.db.CopyTrie(s.trie) } - obj.code = s.code - obj.originStorage = s.originStorage.Copy() - obj.pendingStorage = s.pendingStorage.Copy() - obj.dirtyStorage = s.dirtyStorage.Copy() - obj.dirtyCode = s.dirtyCode - obj.selfDestructed = s.selfDestructed - obj.newContract = s.newContract return obj } diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index 3ce0324e665..f26059e21bb 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -601,6 +601,10 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error { checkeq("GetCode", state.GetCode(addr), checkstate.GetCode(addr)) checkeq("GetCodeHash", state.GetCodeHash(addr), checkstate.GetCodeHash(addr)) checkeq("GetCodeSize", state.GetCodeSize(addr), checkstate.GetCodeSize(addr)) + // Check created-flag + if obj := state.getStateObject(addr); obj != nil { + checkeq("IsNewContract", obj.newContract, checkstate.getStateObject(addr).newContract) + } // Check storage. if obj := state.getStateObject(addr); obj != nil { forEachStorage(state, addr, func(key, value common.Hash) bool { From 29ef67ccfc80eae8a07ce17c2e89224d01c1a091 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 24 Apr 2024 13:10:23 +0200 Subject: [PATCH 3/7] core/state: check dirty storage in post-check --- core/state/statedb_test.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index f26059e21bb..8f5b1abcddd 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -558,10 +558,14 @@ func forEachStorage(s *StateDB, addr common.Address, cb func(key, value common.H if err != nil { return err } - it := trie.NewIterator(trieIt) + var ( + it = trie.NewIterator(trieIt) + visited = make(map[common.Hash]bool) + ) for it.Next() { key := common.BytesToHash(s.trie.GetKey(it.Key)) + visited[key] = true if value, dirty := so.dirtyStorage[key]; dirty { if !cb(key, value) { return nil @@ -579,6 +583,16 @@ func forEachStorage(s *StateDB, addr common.Address, cb func(key, value common.H } } } + // Now visit any remaining dirty storage which is not in trie + for key, value := range so.dirtyStorage { + if visited[key] { + continue + } + if !cb(key, value) { + return nil + } + } + return nil } From 56352c242dddf74c19cc9202621b8c50b4b37aef Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 24 Apr 2024 13:11:58 +0200 Subject: [PATCH 4/7] core/state: method docs --- core/state/access_list.go | 1 + core/state/transient_storage.go | 1 + 2 files changed, 2 insertions(+) diff --git a/core/state/access_list.go b/core/state/access_list.go index ad7ad385297..89815b0fa16 100644 --- a/core/state/access_list.go +++ b/core/state/access_list.go @@ -145,6 +145,7 @@ func (al *accessList) Equal(other *accessList) bool { }) } +// PrettyPrint prints the contents of the access list in a human-readable form func (al *accessList) PrettyPrint() string { out := new(strings.Builder) var sortedAddrs []common.Address diff --git a/core/state/transient_storage.go b/core/state/transient_storage.go index 735b03956ac..e63db39ebab 100644 --- a/core/state/transient_storage.go +++ b/core/state/transient_storage.go @@ -67,6 +67,7 @@ func (t transientStorage) Copy() transientStorage { return storage } +// PrettyPrint prints the contents of the access list in a human-readable form func (t transientStorage) PrettyPrint() string { out := new(strings.Builder) var sortedAddrs []common.Address From 1861642001556a18a27c665c6546a3ec127a1e89 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 24 Apr 2024 13:25:53 +0200 Subject: [PATCH 5/7] core/state: check journal dirties in random-snapshot poststate --- core/state/statedb_test.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index 8f5b1abcddd..3bf42b07bd6 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -25,6 +25,7 @@ import ( "math" "math/rand" "reflect" + "slices" "strings" "sync" "testing" @@ -615,7 +616,7 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error { checkeq("GetCode", state.GetCode(addr), checkstate.GetCode(addr)) checkeq("GetCodeHash", state.GetCodeHash(addr), checkstate.GetCodeHash(addr)) checkeq("GetCodeSize", state.GetCodeSize(addr), checkstate.GetCodeSize(addr)) - // Check created-flag + // Check newContract-flag if obj := state.getStateObject(addr); obj != nil { checkeq("IsNewContract", obj.newContract, checkstate.getStateObject(addr).newContract) } @@ -659,6 +660,23 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error { return fmt.Errorf("got GetLogs(common.Hash{}) == %v, want GetLogs(common.Hash{}) == %v", state.GetLogs(common.Hash{}, 0, common.Hash{}), checkstate.GetLogs(common.Hash{}, 0, common.Hash{})) } + if !maps.Equal(state.journal.dirties, checkstate.journal.dirties) { + getKeys := func(dirty map[common.Address]int) string { + var keys []common.Address + out := new(strings.Builder) + for key := range dirty { + keys = append(keys, key) + } + slices.SortFunc(keys, common.Address.Cmp) + for i, key := range keys { + fmt.Fprintf(out, " %d. %v\n", i, key) + } + return out.String() + } + have := getKeys(state.journal.dirties) + want := getKeys(checkstate.journal.dirties) + return fmt.Errorf("dirty-journal set mismatch.\nhave:\n%v\nwant:\n%v\n", have, want) + } return nil } From 37b712eae67530349f3961f05807f82a7fda75c1 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 24 Apr 2024 15:15:23 +0200 Subject: [PATCH 6/7] core/state: better detection of dirty storage mismatches --- core/state/statedb_test.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index 3bf42b07bd6..8ae443e7c55 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -584,16 +584,6 @@ func forEachStorage(s *StateDB, addr common.Address, cb func(key, value common.H } } } - // Now visit any remaining dirty storage which is not in trie - for key, value := range so.dirtyStorage { - if visited[key] { - continue - } - if !cb(key, value) { - return nil - } - } - return nil } @@ -628,6 +618,26 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error { forEachStorage(checkstate, addr, func(key, value common.Hash) bool { return checkeq("GetState("+key.Hex()+")", checkstate.GetState(addr, key), value) }) + other := checkstate.getStateObject(addr) + // Check dirty storage which is not in trie + if !maps.Equal(obj.dirtyStorage, other.dirtyStorage) { + print := func(dirty map[common.Hash]common.Hash) string { + var keys []common.Hash + out := new(strings.Builder) + for key := range dirty { + keys = append(keys, key) + } + slices.SortFunc(keys, common.Hash.Cmp) + for i, key := range keys { + fmt.Fprintf(out, " %d. %v %v\n", i, key, dirty[key]) + } + return out.String() + } + return fmt.Errorf("dirty storage err, have\n%v\nwant\n%v", + print(obj.dirtyStorage), + print(other.dirtyStorage)) + } + } // Check transient storage. { From 884c23afb8797971a7d66d98651e66a3b40f2906 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 25 Apr 2024 08:15:23 +0200 Subject: [PATCH 7/7] core/state: lint nits --- core/state/access_list.go | 4 ++-- core/state/statedb_test.go | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/core/state/access_list.go b/core/state/access_list.go index 89815b0fa16..b0effbeadc4 100644 --- a/core/state/access_list.go +++ b/core/state/access_list.go @@ -149,7 +149,7 @@ func (al *accessList) Equal(other *accessList) bool { func (al *accessList) PrettyPrint() string { out := new(strings.Builder) var sortedAddrs []common.Address - for addr, _ := range al.addresses { + for addr := range al.addresses { sortedAddrs = append(sortedAddrs, addr) } slices.SortFunc(sortedAddrs, common.Address.Cmp) @@ -158,7 +158,7 @@ func (al *accessList) PrettyPrint() string { fmt.Fprintf(out, "%#x : (idx %d)\n", addr, idx) if idx >= 0 { slotmap := al.slots[idx] - for h, _ := range slotmap { + for h := range slotmap { fmt.Fprintf(out, " %#x\n", h) } } diff --git a/core/state/statedb_test.go b/core/state/statedb_test.go index 8ae443e7c55..71d64f56289 100644 --- a/core/state/statedb_test.go +++ b/core/state/statedb_test.go @@ -637,7 +637,6 @@ func (test *snapshotTest) checkEqual(state, checkstate *StateDB) error { print(obj.dirtyStorage), print(other.dirtyStorage)) } - } // Check transient storage. {