From 0cb6ec734546b95d8f1f1045923a7892560025d6 Mon Sep 17 00:00:00 2001 From: Evgeniy Stratonikov Date: Wed, 17 Feb 2021 12:26:32 +0300 Subject: [PATCH] mpt: allow to remove non-existent keys in batch This bug was here before batch were intoduced. `Delete` is allowed to be called on missing keys with HALT result, MPT needs to take this into account. --- pkg/core/blockchain_test.go | 12 ++++++++++++ pkg/core/interop_system_test.go | 12 ++++++++++++ pkg/core/mpt/batch.go | 6 ++---- pkg/core/mpt/batch_test.go | 6 +++--- pkg/core/mpt/trie.go | 6 +++--- pkg/core/mpt/trie_test.go | 10 +++++----- 6 files changed, 37 insertions(+), 15 deletions(-) diff --git a/pkg/core/blockchain_test.go b/pkg/core/blockchain_test.go index bb6d2b4df..a8f0cbf08 100644 --- a/pkg/core/blockchain_test.go +++ b/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) +} diff --git a/pkg/core/interop_system_test.go b/pkg/core/interop_system_test.go index 461216970..3c03c82fd 100644 --- a/pkg/core/interop_system_test.go +++ b/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, diff --git a/pkg/core/mpt/batch.go b/pkg/core/mpt/batch.go index 6a25dae89..7a9409151 100644 --- a/pkg/core/mpt/batch.go +++ b/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), 0, ErrNotFound + return new(HashNode), 1, nil + } node, n, err := t.newSubTrieMany(prefix, kv[1:], nil) return node, n + 1, err diff --git a/pkg/core/mpt/batch_test.go b/pkg/core/mpt/batch_test.go index fcf612ae2..eebc21caa 100644 --- a/pkg/core/mpt/batch_test.go +++ b/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) }) } diff --git a/pkg/core/mpt/trie.go b/pkg/core/mpt/trie.go index 751b1b2f3..6a0335273 100644 --- a/pkg/core/mpt/trie.go +++ b/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 { diff --git a/pkg/core/mpt/trie_test.go b/pkg/core/mpt/trie_test.go index f1fb1aa5a..bbb3b9828 100644 --- a/pkg/core/mpt/trie_test.go +++ b/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 - require.Error(t, tr.Delete(key1)) + // delete non-existent, refcount should not be updated + 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}) })