From 2d0ad30fcf6648761fd79391d2644dc2332d2114 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 31 Mar 2020 13:30:38 +0300 Subject: [PATCH] vm: rework Map with internal slice representation Which makes iterating over map stable which is important for serialization and and even fixes occasional test failures. We use the same ordering here as NEO 3.0 uses, but it should also be fine for NEO 2.0 because it has no defined order. --- pkg/core/interop_neo.go | 5 ++- pkg/vm/interop.go | 12 ++---- pkg/vm/interop_iterators.go | 9 ++-- pkg/vm/serialization.go | 6 +-- pkg/vm/stack.go | 8 ++-- pkg/vm/stack_item.go | 82 ++++++++++++++++++++++--------------- pkg/vm/stack_item_test.go | 41 ++++--------------- pkg/vm/vm.go | 27 ++++++------ pkg/vm/vm_test.go | 6 ++- 9 files changed, 91 insertions(+), 105 deletions(-) diff --git a/pkg/core/interop_neo.go b/pkg/core/interop_neo.go index fcb4843ff..c4797e517 100644 --- a/pkg/core/interop_neo.go +++ b/pkg/core/interop_neo.go @@ -450,10 +450,11 @@ func (ic *interopContext) storageFind(v *vm.VM) error { return err } - filteredMap := make(map[interface{}]vm.StackItem) + filteredMap := vm.NewMapItem() for k, v := range siMap { if strings.HasPrefix(k, prefix) { - filteredMap[k] = vm.NewByteArrayItem(v.Value) + filteredMap.Add(vm.NewByteArrayItem([]byte(k)), + vm.NewByteArrayItem(v.Value)) } } diff --git a/pkg/vm/interop.go b/pkg/vm/interop.go index b6f6b8221..d1ec282ba 100644 --- a/pkg/vm/interop.go +++ b/pkg/vm/interop.go @@ -185,7 +185,7 @@ func IteratorCreate(v *VM) error { value: t.Value().([]StackItem), }) case *MapItem: - item = NewMapIterator(t.value) + item = NewMapIterator(t) default: return errors.New("non-iterable type") } @@ -195,16 +195,10 @@ func IteratorCreate(v *VM) error { } // NewMapIterator returns new interop item containing iterator over m. -func NewMapIterator(m map[interface{}]StackItem) *InteropItem { - keys := make([]interface{}, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - +func NewMapIterator(m *MapItem) *InteropItem { return NewInteropItem(&mapWrapper{ index: -1, - keys: keys, - m: m, + m: m.value, }) } diff --git a/pkg/vm/interop_iterators.go b/pkg/vm/interop_iterators.go index 62a8b507e..8ecf69b83 100644 --- a/pkg/vm/interop_iterators.go +++ b/pkg/vm/interop_iterators.go @@ -25,8 +25,7 @@ type ( mapWrapper struct { index int - keys []interface{} - m map[interface{}]StackItem + m []MapElement } concatIter struct { @@ -91,7 +90,7 @@ func (i *concatIter) Key() StackItem { } func (m *mapWrapper) Next() bool { - if next := m.index + 1; next < len(m.keys) { + if next := m.index + 1; next < len(m.m) { m.index = next return true } @@ -100,11 +99,11 @@ func (m *mapWrapper) Next() bool { } func (m *mapWrapper) Value() StackItem { - return m.m[m.keys[m.index]] + return m.m[m.index].Value } func (m *mapWrapper) Key() StackItem { - return makeStackItem(m.keys[m.index]) + return m.m[m.index].Key } func (e *keysWrapper) Next() bool { diff --git a/pkg/vm/serialization.go b/pkg/vm/serialization.go index b7c7cceed..a3d30941f 100644 --- a/pkg/vm/serialization.go +++ b/pkg/vm/serialization.go @@ -73,9 +73,9 @@ func serializeItemTo(item StackItem, w *io.BinWriter, seen map[StackItem]bool) { w.WriteBytes([]byte{byte(mapT)}) w.WriteVarUint(uint64(len(t.value))) - for k, v := range t.value { - serializeItemTo(makeStackItem(k), w, seen) - serializeItemTo(v, w, seen) + for i := range t.value { + serializeItemTo(t.value[i].Key, w, seen) + serializeItemTo(t.value[i].Value, w, seen) } } } diff --git a/pkg/vm/stack.go b/pkg/vm/stack.go index 361b81854..fc41a0406 100644 --- a/pkg/vm/stack.go +++ b/pkg/vm/stack.go @@ -228,8 +228,8 @@ func (s *Stack) updateSizeAdd(item StackItem) { s.updateSizeAdd(it) } case *MapItem: - for _, v := range t.value { - s.updateSizeAdd(v) + for i := range t.value { + s.updateSizeAdd(t.value[i].Value) } } } @@ -253,8 +253,8 @@ func (s *Stack) updateSizeRemove(item StackItem) { s.updateSizeRemove(it) } case *MapItem: - for _, v := range t.value { - s.updateSizeRemove(v) + for i := range t.value { + s.updateSizeRemove(t.value[i].Value) } } } diff --git a/pkg/vm/stack_item.go b/pkg/vm/stack_item.go index b185e6e85..6cfd772f3 100644 --- a/pkg/vm/stack_item.go +++ b/pkg/vm/stack_item.go @@ -435,15 +435,25 @@ func (i *ArrayItem) ToContractParameter(seen map[StackItem]bool) smartcontract.P } } -// MapItem represents Map object. +// MapElement is a key-value pair of StackItems. +type MapElement struct { + Key StackItem + Value StackItem +} + +// MapItem represents Map object. It's ordered, so we use slice representation +// which should be fine for maps with less than 32 or so elements. Given that +// our VM has quite low limit of overall stack items, it should be good enough, +// but it can be extended with a real map for fast random access in the future +// if need be. type MapItem struct { - value map[interface{}]StackItem + value []MapElement } // NewMapItem returns new MapItem object. func NewMapItem() *MapItem { return &MapItem{ - value: make(map[interface{}]StackItem), + value: make([]MapElement, 0), } } @@ -466,10 +476,19 @@ func (i *MapItem) String() string { return "Map" } +// Index returns an index of the key in map. +func (i *MapItem) Index(key StackItem) int { + for k := range i.value { + if i.value[k].Key.Equals(key) { + return k + } + } + return -1 +} + // Has checks if map has specified key. -func (i *MapItem) Has(key StackItem) (ok bool) { - _, ok = i.value[toMapKey(key)] - return +func (i *MapItem) Has(key StackItem) bool { + return i.Index(key) >= 0 } // Dup implements StackItem interface. @@ -483,12 +502,10 @@ func (i *MapItem) ToContractParameter(seen map[StackItem]bool) smartcontract.Par value := make([]smartcontract.ParameterPair, 0) if !seen[i] { seen[i] = true - for key, val := range i.value { - pValue := val.ToContractParameter(seen) - pKey := fromMapKey(key).ToContractParameter(seen) + for k := range i.value { value = append(value, smartcontract.ParameterPair{ - Key: pKey, - Value: pValue, + Key: i.value[k].Key.ToContractParameter(seen), + Value: i.value[k].Value.ToContractParameter(seen), }) } } @@ -500,34 +517,31 @@ func (i *MapItem) ToContractParameter(seen map[StackItem]bool) smartcontract.Par // Add adds key-value pair to the map. func (i *MapItem) Add(key, value StackItem) { - i.value[toMapKey(key)] = value -} - -// toMapKey converts StackItem so that it can be used as a map key. -func toMapKey(key StackItem) interface{} { - switch t := key.(type) { - case *BoolItem: - return t.value - case *BigIntegerItem: - return t.value.Int64() - case *ByteArrayItem: - return string(t.value) - default: + if !isValidMapKey(key) { panic("wrong key type") } + index := i.Index(key) + if index >= 0 { + i.value[index].Value = value + } else { + i.value = append(i.value, MapElement{key, value}) + } } -// fromMapKey converts map key to StackItem -func fromMapKey(key interface{}) StackItem { - switch t := key.(type) { - case bool: - return &BoolItem{value: t} - case int64: - return &BigIntegerItem{value: big.NewInt(t)} - case string: - return &ByteArrayItem{value: []byte(t)} +// Drop removes given index from the map (no bounds check done here). +func (i *MapItem) Drop(index int) { + copy(i.value[index:], i.value[index+1:]) + i.value = i.value[:len(i.value)-1] +} + +// isValidMapKey checks whether it's possible to use given StackItem as a Map +// key. +func isValidMapKey(key StackItem) bool { + switch key.(type) { + case *BoolItem, *BigIntegerItem, *ByteArrayItem: + return true default: - panic("wrong key type") + return false } } diff --git a/pkg/vm/stack_item_test.go b/pkg/vm/stack_item_test.go index c67745bfd..069682878 100644 --- a/pkg/vm/stack_item_test.go +++ b/pkg/vm/stack_item_test.go @@ -282,13 +282,13 @@ var equalsTestCases = map[string][]struct { result: false, }, { - item1: &MapItem{value: map[interface{}]StackItem{"first": NewBigIntegerItem(1), true: NewByteArrayItem([]byte{2})}}, - item2: &MapItem{value: map[interface{}]StackItem{"first": NewBigIntegerItem(1), true: NewByteArrayItem([]byte{2})}}, + item1: &MapItem{value: []MapElement{{NewByteArrayItem([]byte("first")), NewBigIntegerItem(1)}, {NewBoolItem(true), NewByteArrayItem([]byte{2})}}}, + item2: &MapItem{value: []MapElement{{NewByteArrayItem([]byte("first")), NewBigIntegerItem(1)}, {NewBoolItem(true), NewByteArrayItem([]byte{2})}}}, result: false, }, { - item1: &MapItem{value: map[interface{}]StackItem{"first": NewBigIntegerItem(1), true: NewByteArrayItem([]byte{2})}}, - item2: &MapItem{value: map[interface{}]StackItem{"first": NewBigIntegerItem(1), true: NewByteArrayItem([]byte{3})}}, + item1: &MapItem{value: []MapElement{{NewByteArrayItem([]byte("first")), NewBigIntegerItem(1)}, {NewBoolItem(true), NewByteArrayItem([]byte{2})}}}, + item2: &MapItem{value: []MapElement{{NewByteArrayItem([]byte("first")), NewBigIntegerItem(1)}, {NewBoolItem(true), NewByteArrayItem([]byte{3})}}}, result: false, }, }, @@ -414,10 +414,10 @@ var toContractParameterTestCases = []struct { result: smartcontract.Parameter{Type: smartcontract.InteropInterfaceType, Value: nil}, }, { - input: &MapItem{value: map[interface{}]StackItem{ - toMapKey(NewBigIntegerItem(1)): NewBoolItem(true), - toMapKey(NewByteArrayItem([]byte("qwerty"))): NewBigIntegerItem(3), - toMapKey(NewBoolItem(true)): NewBoolItem(false), + input: &MapItem{value: []MapElement{ + {NewBigIntegerItem(1), NewBoolItem(true)}, + {NewByteArrayItem([]byte("qwerty")), NewBigIntegerItem(3)}, + {NewBoolItem(true), NewBoolItem(false)}, }}, result: smartcontract.Parameter{ Type: smartcontract.MapType, @@ -445,28 +445,3 @@ func TestToContractParameter(t *testing.T) { assert.Equal(t, res, tc.result) } } - -var fromMapKeyTestCases = []struct { - input interface{} - result StackItem -}{ - { - input: true, - result: NewBoolItem(true), - }, - { - input: int64(4), - result: NewBigIntegerItem(4), - }, - { - input: "qwerty", - result: NewByteArrayItem([]byte("qwerty")), - }, -} - -func TestFromMapKey(t *testing.T) { - for _, tc := range fromMapKeyTestCases { - res := fromMapKey(tc.input) - assert.Equal(t, res, tc.result) - } -} diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index d5d9b5886..13ac42441 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -1014,11 +1014,11 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro item := arr[index].Dup() v.estack.PushVal(item) case *MapItem: - if !t.Has(key.value) { + index := t.Index(key.Item()) + if index < 0 { panic("invalid key") } - k := toMapKey(key.value) - v.estack.Push(&Element{value: t.value[k].Dup()}) + v.estack.Push(&Element{value: t.value[index].Value.Dup()}) default: arr := obj.Bytes() if index < 0 || index >= len(arr) { @@ -1091,10 +1091,12 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro a = append(a[:k], a[k+1:]...) t.value = a case *MapItem: - m := t.value - k := toMapKey(key.value) - v.estack.updateSizeRemove(m[k]) - delete(m, k) + index := t.Index(key.Item()) + // NEO 2.0 doesn't error on missing key. + if index >= 0 { + v.estack.updateSizeRemove(t.value[index].Value) + t.Drop(index) + } default: panic("REMOVE: invalid type") } @@ -1106,7 +1108,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro switch t := elem.Value().(type) { case []StackItem: v.estack.PushVal(len(t)) - case map[interface{}]StackItem: + case []MapElement: v.estack.PushVal(len(t)) default: v.estack.PushVal(len(elem.Bytes())) @@ -1253,7 +1255,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro arr := make([]StackItem, 0, len(m.value)) for k := range m.value { - arr = append(arr, makeStackItem(k)) + arr = append(arr, m.value[k].Key.Dup()) } v.estack.PushVal(arr) @@ -1274,7 +1276,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro case *MapItem: arr = make([]StackItem, 0, len(t.value)) for k := range t.value { - arr = append(arr, cloneIfStruct(t.value[k])) + arr = append(arr, cloneIfStruct(t.value[k].Value)) } default: panic("not a Map, Array or Struct") @@ -1298,7 +1300,7 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro } v.estack.PushVal(index < int64(len(c.Array()))) case *MapItem: - v.estack.PushVal(t.Has(key.value)) + v.estack.PushVal(t.Has(key.Item())) default: panic("wrong collection type") } @@ -1552,8 +1554,7 @@ func validateMapKey(key *Element) { if key == nil { panic("no key found") } - switch key.value.(type) { - case *ArrayItem, *StructItem, *MapItem: + if !isValidMapKey(key.Item()) { panic("key can't be a collection") } } diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index d05576fb5..e0b13920b 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -1371,8 +1371,10 @@ func TestPICKITEMDupMap(t *testing.T) { runVM(t, vm) assert.Equal(t, 2, vm.estack.Len()) assert.Equal(t, int64(1), vm.estack.Pop().BigInt().Int64()) - items := vm.estack.Pop().Value().(map[interface{}]StackItem) - assert.Equal(t, big.NewInt(-1), items[string([]byte{42})].Value()) + items := vm.estack.Pop().Value().([]MapElement) + assert.Equal(t, 1, len(items)) + assert.Equal(t, []byte{42}, items[0].Key.Value()) + assert.Equal(t, big.NewInt(-1), items[0].Value.Value()) } func TestPICKITEMMap(t *testing.T) {