Merge pull request #1754 from nspcc-dev/fix/mptdelete

mpt: allow to remove non-existent keys in batch

pkg/core/blockchain_test.go

@@ -1591,3 +1591,15 @@ func TestInvalidNotification(t *testing.T) {
 	require.Nil(t, aer.Stack[0])
 	require.Equal(t, stackitem.InteropT, aer.Stack[1].Type())
 }
+
+// Test that deletion of non-existent doesn't result in error in tx or block addition.
+func TestMPTDeleteNoKey(t *testing.T) {
+	bc := newTestChain(t)
+	defer bc.Close()
+
+	cs, _ := getTestContractState(bc)
+	require.NoError(t, bc.contracts.Management.PutContractState(bc.dao, cs))
+	aer, err := invokeContractMethod(bc, 1_00000000, cs.Hash, "delValue", "non-existent-key")
+	require.NoError(t, err)
+	require.Equal(t, vm.HaltState, aer.VMState)
+}

pkg/core/interop_system_test.go

@@ -342,6 +342,10 @@ func getTestContractState(bc *Blockchain) (*state.Contract, *state.Contract) {
 	emit.Syscall(w.BinWriter, interopnames.SystemStorageGetContext)
 	emit.Syscall(w.BinWriter, interopnames.SystemStorageGet)
 	emit.Opcodes(w.BinWriter, opcode.RET)
+	delValOff := w.Len()
+	emit.Syscall(w.BinWriter, interopnames.SystemStorageGetContext)
+	emit.Syscall(w.BinWriter, interopnames.SystemStorageDelete)
+	emit.Opcodes(w.BinWriter, opcode.RET)
 	onNEP17PaymentOff := w.Len()
 	emit.Syscall(w.BinWriter, interopnames.SystemRuntimeGetCallingScriptHash)
 	emit.Int(w.BinWriter, 4)
@@ -464,6 +468,14 @@ func getTestContractState(bc *Blockchain) (*state.Contract, *state.Contract) {
 			},
 			ReturnType: smartcontract.VoidType,
 		},
+		{
+			Name:   "delValue",
+			Offset: delValOff,
+			Parameters: []manifest.Parameter{
+				manifest.NewParameter("key", smartcontract.StringType),
+			},
+			ReturnType: smartcontract.VoidType,
+		},
 		{
 			Name:   manifest.MethodOnNEP11Payment,
 			Offset: onNEP11PaymentOff,

pkg/core/mpt/batch.go

@@ -207,10 +207,8 @@ func (t *Trie) newSubTrieMany(prefix []byte, kv []keyValue, value []byte) (Node,
 	if len(kv[0].key) == 0 {
 		if len(kv[0].value) == 0 {
 			if len(kv) == 1 {
-				if len(value) != 0 {
+				return new(HashNode), 1, nil
-					return new(HashNode), 1, nil
+
-				}
-				return new(HashNode), 0, ErrNotFound
 			}
 			node, n, err := t.newSubTrieMany(prefix, kv[1:], nil)
 			return node, n + 1, err

pkg/core/mpt/batch_test.go

@@ -170,7 +170,7 @@ func TestTrie_PutBatchBranch(t *testing.T) {
 			{[]byte{0x20, 2}, nil},
 			{[]byte{0x30, 3}, []byte("won't be put")},
 		}
-		testIncompletePut(t, ps, 1, tr1, tr2)
+		testIncompletePut(t, ps, 3, tr1, tr2)
 	})
 	t.Run("incomplete put, transform to empty", func(t *testing.T) {
 		tr1, tr2 := prepareBranch(t)
@@ -180,7 +180,7 @@ func TestTrie_PutBatchBranch(t *testing.T) {
 			{[]byte{0x20, 2}, nil},
 			{[]byte{0x30, 3}, []byte("won't be put")},
 		}
-		testIncompletePut(t, ps, 2, tr1, tr2)
+		testIncompletePut(t, ps, 4, tr1, tr2)
 	})
 	t.Run("remove 2, become empty", func(t *testing.T) {
 		tr1, tr2 := prepareBranch(t)
@@ -247,7 +247,7 @@ func TestTrie_PutBatchEmpty(t *testing.T) {
 		}
 		tr1 := NewTrie(new(HashNode), false, newTestStore())
 		tr2 := NewTrie(new(HashNode), false, newTestStore())
-		testIncompletePut(t, ps, 2, tr1, tr2)
+		testIncompletePut(t, ps, 4, tr1, tr2)
 	})
 }
 

pkg/core/mpt/trie.go

@@ -290,7 +290,7 @@ func (t *Trie) deleteFromBranch(b *BranchNode, path []byte) (Node, error) {
 
 func (t *Trie) deleteFromExtension(n *ExtensionNode, path []byte) (Node, error) {
 	if !bytes.HasPrefix(path, n.key) {
-		return nil, ErrNotFound
+		return n, nil
 	}
 	h := n.Hash()
 	bs := n.bytes
@@ -325,14 +325,14 @@ func (t *Trie) deleteFromNode(curr Node, path []byte) (Node, error) {
 			t.removeRef(curr.Hash(), curr.Bytes())
 			return new(HashNode), nil
 		}
-		return nil, ErrNotFound
+		return curr, nil
 	case *BranchNode:
 		return t.deleteFromBranch(n, path)
 	case *ExtensionNode:
 		return t.deleteFromExtension(n, path)
 	case *HashNode:
 		if n.IsEmpty() {
-			return nil, ErrNotFound
+			return n, nil
 		}
 		newNode, err := t.getFromStore(n.Hash())
 		if err != nil {

pkg/core/mpt/trie_test.go

@@ -62,8 +62,8 @@ func testTrieRefcount(t *testing.T, key1, key2 []byte) {
 	tr.testHas(t, key1, nil)
 	tr.testHas(t, key2, []byte{1})
 
-	// error on delete, refcount should not be updated
+	// delete non-existent, refcount should not be updated
-	require.Error(t, tr.Delete(key1))
+	require.NoError(t, tr.Delete(key1))
 	tr.Flush()
 	tr.testHas(t, key1, nil)
 	tr.testHas(t, key2, []byte{1})
@@ -316,7 +316,7 @@ func testTrieDelete(t *testing.T, enableGC bool) {
 
 		t.Run("Empty", func(t *testing.T) {
 			tr := NewTrie(nil, enableGC, newTestStore())
-			require.Error(t, tr.Delete([]byte{}))
+			require.NoError(t, tr.Delete([]byte{}))
 		})
 	})
 
@@ -324,7 +324,7 @@ func testTrieDelete(t *testing.T, enableGC bool) {
 		l := NewLeafNode([]byte{0x12, 0x34})
 		tr := NewTrie(l, enableGC, newTestStore())
 		t.Run("NonExistentKey", func(t *testing.T) {
-			require.Error(t, tr.Delete([]byte{0x12}))
+			require.NoError(t, tr.Delete([]byte{0x12}))
 			tr.testHas(t, []byte{}, []byte{0x12, 0x34})
 		})
 		require.NoError(t, tr.Delete([]byte{}))
@@ -338,7 +338,7 @@ func testTrieDelete(t *testing.T, enableGC bool) {
 			tr := NewTrie(e, enableGC, newTestStore())
 
 			t.Run("NonExistentKey", func(t *testing.T) {
-				require.Error(t, tr.Delete([]byte{}))
+				require.NoError(t, tr.Delete([]byte{}))
 				tr.testHas(t, []byte{0xAB}, []byte{0x12, 0x34})
 			})