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

@@ -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))
 		}
 	}
 

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,
 	})
 }
 

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 {

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)
 		}
 	}
 }

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)
 			}
 		}
 	}

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
 	}
 }
 

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)
-	}
-}

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")
 	}
 }

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) {