core: adjust System.Contract.Update interop

Part of #1055.

We should check contract scripthash against the one provided in manifest
and manifest groups. We shouldn't put on stack anything after return.
And ofcourse, we mast not destroy the old contract at the end, as
`contractDestroy` removes all storage items associated with the
old contract ID (which equals to the new contract ID). We just remove
old contract state - it's enough.

pkg/core/interop_neo.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/nspcc-dev/neo-go/pkg/core/interop"
 	"github.com/nspcc-dev/neo-go/pkg/core/state"
+	"github.com/nspcc-dev/neo-go/pkg/crypto/hash"
 	"github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest"
 	"github.com/nspcc-dev/neo-go/pkg/vm"
 	"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
@@ -109,45 +110,77 @@ func contractCreate(ic *interop.Context, v *vm.VM) error {
 	return nil
 }
 
-// contractUpdate migrates a contract.
+// contractUpdate migrates a contract. This method assumes that Manifest and Script
+// of the contract can be updated independently.
 func contractUpdate(ic *interop.Context, v *vm.VM) error {
-	contract, err := ic.DAO.GetContractState(v.GetCurrentScriptHash())
+	contract, _ := ic.DAO.GetContractState(v.GetCurrentScriptHash())
 	if contract == nil {
 		return errors.New("contract doesn't exist")
 	}
-	newcontract, err := createContractStateFromVM(ic, v)
-	if err != nil {
-		return err
+	script := v.Estack().Pop().Bytes()
+	if len(script) > MaxContractScriptSize {
+		return errors.New("the script is too big")
 	}
-	if newcontract.Script != nil {
-		if l := len(newcontract.Script); l == 0 || l > MaxContractScriptSize {
+	manifestBytes := v.Estack().Pop().Bytes()
+	if len(manifestBytes) > manifest.MaxManifestSize {
+		return errors.New("manifest is too big")
+	}
+	if !v.AddGas(int64(StoragePrice * (len(script) + len(manifestBytes)))) {
+		return errGasLimitExceeded
+	}
+	// if script was provided, update the old contract script and Manifest.ABI hash
+	if l := len(script); l > 0 {
+		if l > MaxContractScriptSize {
 			return errors.New("invalid script len")
 		}
-		h := newcontract.ScriptHash()
-		if h.Equals(contract.ScriptHash()) {
+		newHash := hash.Hash160(script)
+		if newHash.Equals(contract.ScriptHash()) {
 			return errors.New("the script is the same")
-		} else if _, err := ic.DAO.GetContractState(h); err == nil {
+		} else if _, err := ic.DAO.GetContractState(newHash); err == nil {
 			return errors.New("contract already exists")
 		}
-		newcontract.ID = contract.ID
-		if err := ic.DAO.PutContractState(newcontract); err != nil {
-			return err
+		oldHash := contract.ScriptHash()
+		// re-write existing contract variable, as we need it to be up-to-date during manifest update
+		contract = &state.Contract{
+			ID:       contract.ID,
+			Script:   script,
+			Manifest: contract.Manifest,
 		}
-		if err := ic.DAO.DeleteContractState(contract.ScriptHash()); err != nil {
-			return err
+		contract.Manifest.ABI.Hash = newHash
+		if err := ic.DAO.PutContractState(contract); err != nil {
+			return fmt.Errorf("failed to update script: %v", err)
+		}
+		if err := ic.DAO.DeleteContractState(oldHash); err != nil {
+			return fmt.Errorf("failed to update script: %v", err)
 		}
 	}
-	if !newcontract.HasStorage() {
+	// if manifest was provided, update the old contract manifest and check associated
+	// storage items if needed
+	if len(manifestBytes) > 0 {
+		var newManifest manifest.Manifest
+		err := newManifest.UnmarshalJSON(manifestBytes)
+		if err != nil {
+			return fmt.Errorf("unable to retrieve manifest from stack: %v", err)
+		}
+		// we don't have to perform `GetContractState` one more time as it's already up-to-date
+		contract.Manifest = newManifest
+		if !contract.Manifest.IsValid(contract.ScriptHash()) {
+			return errors.New("failed to check contract script hash against new manifest")
+		}
+		if !contract.HasStorage() {
 			siMap, err := ic.DAO.GetStorageItems(contract.ID)
 			if err != nil {
-			return err
+				return fmt.Errorf("failed to update manifest: %v", err)
 			}
 			if len(siMap) != 0 {
 				return errors.New("old contract shouldn't have storage")
 			}
 		}
-	v.Estack().PushVal(stackitem.NewInterop(contract))
-	return contractDestroy(ic, v)
+		if err := ic.DAO.PutContractState(contract); err != nil {
+			return fmt.Errorf("failed to update manifest: %v", err)
+		}
+	}
+	return nil
 }
 
 // runtimeSerialize serializes top stack item into a ByteArray.

pkg/core/interop_system_test.go

@@ -11,6 +11,8 @@ import (
 	"github.com/nspcc-dev/neo-go/pkg/core/transaction"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/hash"
 	"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
+	"github.com/nspcc-dev/neo-go/pkg/smartcontract"
+	"github.com/nspcc-dev/neo-go/pkg/smartcontract/manifest"
 	"github.com/nspcc-dev/neo-go/pkg/util"
 	"github.com/nspcc-dev/neo-go/pkg/vm/opcode"
 	"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
@@ -364,3 +366,219 @@ func compareContractStates(t *testing.T, expected *state.Contract, actual stacki
 	require.Equal(t, expected.HasStorage(), act[2].Bool())
 	require.Equal(t, expected.IsPayable(), act[3].Bool())
 }
+
+func TestContractUpdate(t *testing.T) {
+	v, cs, ic, bc := createVMAndContractState(t)
+	defer bc.Close()
+	v.GasLimit = -1
+
+	putArgsOnStack := func(script, manifest []byte) {
+		v.Estack().PushVal(manifest)
+		v.Estack().PushVal(script)
+	}
+
+	t.Run("no args", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(nil, nil)
+		require.NoError(t, contractUpdate(ic, v))
+	})
+
+	t.Run("no contract", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, util.Uint160{8, 9, 7}, smartcontract.All)
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("too large script", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(make([]byte, MaxContractScriptSize+1), nil)
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("too large manifest", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(nil, make([]byte, manifest.MaxManifestSize+1))
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("gas limit exceeded", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.GasLimit = 0
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack([]byte{1}, []byte{2})
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("update script, the same script", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.GasLimit = -1
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(cs.Script, nil)
+
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("update script, already exists", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		duplicateScript := []byte{byte(opcode.PUSHDATA4)}
+		require.NoError(t, ic.DAO.PutContractState(&state.Contract{
+			ID:     95,
+			Script: duplicateScript,
+			Manifest: manifest.Manifest{
+				ABI: manifest.ABI{
+					Hash: hash.Hash160(duplicateScript),
+				},
+			},
+		}))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(duplicateScript, nil)
+
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("update script, positive", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		newScript := []byte{9, 8, 7, 6, 5}
+		putArgsOnStack(newScript, nil)
+
+		require.NoError(t, contractUpdate(ic, v))
+
+		// updated contract should have new scripthash
+		actual, err := ic.DAO.GetContractState(hash.Hash160(newScript))
+		require.NoError(t, err)
+		expected := &state.Contract{
+			ID:       cs.ID,
+			Script:   newScript,
+			Manifest: cs.Manifest,
+		}
+		expected.Manifest.ABI.Hash = hash.Hash160(newScript)
+		_ = expected.ScriptHash()
+		require.Equal(t, expected, actual)
+
+		// old contract should be deleted
+		_, err = ic.DAO.GetContractState(cs.ScriptHash())
+		require.Error(t, err)
+	})
+
+	t.Run("update manifest, bad manifest", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(nil, []byte{1, 2, 3})
+
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("update manifest, bad contract hash", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		manifest := &manifest.Manifest{
+			ABI: manifest.ABI{
+				Hash: util.Uint160{4, 5, 6},
+			},
+		}
+		manifestBytes, err := manifest.MarshalJSON()
+		require.NoError(t, err)
+		putArgsOnStack(nil, manifestBytes)
+
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("update manifest, old contract shouldn't have storage", func(t *testing.T) {
+		cs.Manifest.Features |= smartcontract.HasStorage
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		require.NoError(t, ic.DAO.PutStorageItem(cs.ID, []byte("my_item"), &state.StorageItem{
+			Value:   []byte{1, 2, 3},
+			IsConst: false,
+		}))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		manifest := &manifest.Manifest{
+			ABI: manifest.ABI{
+				Hash: cs.ScriptHash(),
+			},
+		}
+		manifestBytes, err := manifest.MarshalJSON()
+		require.NoError(t, err)
+		putArgsOnStack(nil, manifestBytes)
+
+		require.Error(t, contractUpdate(ic, v))
+	})
+
+	t.Run("update manifest, positive", func(t *testing.T) {
+		cs.Manifest.Features = smartcontract.NoProperties
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		manifest := &manifest.Manifest{
+			ABI: manifest.ABI{
+				Hash: cs.ScriptHash(),
+				EntryPoint: manifest.Method{
+					Name: "Main",
+					Parameters: []manifest.Parameter{
+						manifest.NewParameter("NewParameter", smartcontract.IntegerType),
+					},
+					ReturnType: smartcontract.StringType,
+				},
+			},
+			Features: smartcontract.HasStorage,
+		}
+		manifestBytes, err := manifest.MarshalJSON()
+		require.NoError(t, err)
+		putArgsOnStack(nil, manifestBytes)
+
+		require.NoError(t, contractUpdate(ic, v))
+
+		// updated contract should have new scripthash
+		actual, err := ic.DAO.GetContractState(cs.ScriptHash())
+		expected := &state.Contract{
+			ID:       cs.ID,
+			Script:   cs.Script,
+			Manifest: *manifest,
+		}
+		_ = expected.ScriptHash()
+		require.Equal(t, expected, actual)
+	})
+
+	t.Run("update both script and manifest", func(t *testing.T) {
+		require.NoError(t, ic.DAO.PutContractState(cs))
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		newScript := []byte{12, 13, 14}
+		newManifest := manifest.Manifest{
+			ABI: manifest.ABI{
+				Hash: hash.Hash160(newScript),
+				EntryPoint: manifest.Method{
+					Name: "Main",
+					Parameters: []manifest.Parameter{
+						manifest.NewParameter("VeryNewParameter", smartcontract.IntegerType),
+					},
+					ReturnType: smartcontract.StringType,
+				},
+			},
+			Features: smartcontract.HasStorage,
+		}
+		newManifestBytes, err := newManifest.MarshalJSON()
+		require.NoError(t, err)
+
+		putArgsOnStack(newScript, newManifestBytes)
+
+		require.NoError(t, contractUpdate(ic, v))
+
+		// updated contract should have new script and manifest
+		actual, err := ic.DAO.GetContractState(hash.Hash160(newScript))
+		require.NoError(t, err)
+		expected := &state.Contract{
+			ID:       cs.ID,
+			Script:   newScript,
+			Manifest: newManifest,
+		}
+		expected.Manifest.ABI.Hash = hash.Hash160(newScript)
+		_ = expected.ScriptHash()
+		require.Equal(t, expected, actual)
+
+		// old contract should be deleted
+		_, err = ic.DAO.GetContractState(cs.ScriptHash())
+		require.Error(t, err)
+	})
+}

pkg/interop/contract/contract.go

@@ -28,8 +28,8 @@ func Create(script []byte, manifest []byte) Contract {
 // Create. The old contract will be deleted by this call, if it has any storage
 // associated it will be migrated to the new contract. New contract is returned.
 // This function uses `System.Contract.Update` syscall.
-func Update(script []byte, manifest []byte) Contract {
-	return Contract{}
+func Update(script []byte, manifest []byte) {
+	return
 }
 
 // Destroy deletes calling contract (the one that calls Destroy) from the