Merge pull request #1460 from nspcc-dev/contract/update

core: allow Null in `System.Contract.Update`

pkg/core/interop_neo.go

@@ -112,6 +112,17 @@ func contractCreate(ic *interop.Context) error {
 	return nil
 }
 
+func checkNonEmpty(b []byte, max int) error {
+	if b != nil {
+		if l := len(b); l == 0 {
+			return errors.New("empty")
+		} else if l > max {
+			return fmt.Errorf("len is %d (max %d)", l, max)
+		}
+	}
+	return nil
+}
+
 // contractUpdate migrates a contract. This method assumes that Manifest and Script
 // of the contract can be updated independently.
 func contractUpdate(ic *interop.Context) error {
@@ -119,22 +130,22 @@ func contractUpdate(ic *interop.Context) error {
 	if contract == nil {
 		return errors.New("contract doesn't exist")
 	}
-	script := ic.VM.Estack().Pop().Bytes()
+	script := ic.VM.Estack().Pop().BytesOrNil()
-	if len(script) > MaxContractScriptSize {
+	manifestBytes := ic.VM.Estack().Pop().BytesOrNil()
-		return errors.New("the script is too big")
+	if script == nil && manifestBytes == nil {
+		return errors.New("both script and manifest are nil")
 	}
-	manifestBytes := ic.VM.Estack().Pop().Bytes()
+	if err := checkNonEmpty(script, MaxContractScriptSize); err != nil {
-	if len(manifestBytes) > manifest.MaxManifestSize {
+		return fmt.Errorf("invalid script size: %w", err)
-		return errors.New("manifest is too big")
+	}
+	if err := checkNonEmpty(manifestBytes, manifest.MaxManifestSize); err != nil {
+		return fmt.Errorf("invalid manifest size: %w", err)
 	}
 	if !ic.VM.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 script != nil {
-		if l > MaxContractScriptSize {
-			return errors.New("invalid script len")
-		}
 		newHash := hash.Hash160(script)
 		if newHash.Equals(contract.ScriptHash()) {
 			return errors.New("the script is the same")
@@ -158,7 +169,7 @@ func contractUpdate(ic *interop.Context) error {
 	}
 	// if manifest was provided, update the old contract manifest and check associated
 	// storage items if needed
-	if len(manifestBytes) > 0 {
+	if manifestBytes != nil {
 		var newManifest manifest.Manifest
 		err := newManifest.UnmarshalJSON(manifestBytes)
 		if err != nil {

pkg/core/interop_system_test.go

@@ -627,7 +627,7 @@ func TestContractUpdate(t *testing.T) {
 	defer bc.Close()
 	v.GasLimit = -1
 
-	putArgsOnStack := func(script, manifest []byte) {
+	putArgsOnStack := func(script, manifest interface{}) {
 		v.Estack().PushVal(manifest)
 		v.Estack().PushVal(script)
 	}
@@ -635,8 +635,8 @@ func TestContractUpdate(t *testing.T) {
 	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)
+		putArgsOnStack(stackitem.Null{}, stackitem.Null{})
-		require.NoError(t, contractUpdate(ic))
+		require.Error(t, contractUpdate(ic))
 	})
 
 	t.Run("no contract", func(t *testing.T) {
@@ -648,14 +648,14 @@ func TestContractUpdate(t *testing.T) {
 	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)
+		putArgsOnStack(make([]byte, MaxContractScriptSize+1), stackitem.Null{})
 		require.Error(t, contractUpdate(ic))
 	})
 
 	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))
+		putArgsOnStack(stackitem.Null{}, make([]byte, manifest.MaxManifestSize+1))
 		require.Error(t, contractUpdate(ic))
 	})
 
@@ -671,7 +671,7 @@ func TestContractUpdate(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)
+		putArgsOnStack(cs.Script, stackitem.Null{})
 
 		require.Error(t, contractUpdate(ic))
 	})
@@ -689,16 +689,23 @@ func TestContractUpdate(t *testing.T) {
 			},
 		}))
 		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
-		putArgsOnStack(duplicateScript, nil)
+		putArgsOnStack(duplicateScript, stackitem.Null{})
 
 		require.Error(t, contractUpdate(ic))
 	})
 
 	t.Run("update script, positive", func(t *testing.T) {
 		require.NoError(t, ic.DAO.PutContractState(cs))
+		t.Run("empty manifest", func(t *testing.T) {
 			v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
 			newScript := []byte{9, 8, 7, 6, 5}
-		putArgsOnStack(newScript, nil)
+			putArgsOnStack(newScript, []byte{})
+			require.Error(t, contractUpdate(ic))
+		})
+
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		newScript := []byte{9, 8, 7, 6, 5}
+		putArgsOnStack(newScript, stackitem.Null{})
 
 		require.NoError(t, contractUpdate(ic))
 
@@ -722,7 +729,7 @@ func TestContractUpdate(t *testing.T) {
 	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})
+		putArgsOnStack(stackitem.Null{}, []byte{1, 2, 3})
 
 		require.Error(t, contractUpdate(ic))
 	})
@@ -737,7 +744,7 @@ func TestContractUpdate(t *testing.T) {
 		}
 		manifestBytes, err := manifest.MarshalJSON()
 		require.NoError(t, err)
-		putArgsOnStack(nil, manifestBytes)
+		putArgsOnStack(stackitem.Null{}, manifestBytes)
 
 		require.Error(t, contractUpdate(ic))
 	})
@@ -757,7 +764,7 @@ func TestContractUpdate(t *testing.T) {
 		}
 		manifestBytes, err := manifest.MarshalJSON()
 		require.NoError(t, err)
-		putArgsOnStack(nil, manifestBytes)
+		putArgsOnStack(stackitem.Null{}, manifestBytes)
 
 		require.Error(t, contractUpdate(ic))
 	})
@@ -765,7 +772,6 @@ func TestContractUpdate(t *testing.T) {
 	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(),
@@ -774,8 +780,15 @@ func TestContractUpdate(t *testing.T) {
 		}
 		manifestBytes, err := manifest.MarshalJSON()
 		require.NoError(t, err)
-		putArgsOnStack(nil, manifestBytes)
 
+		t.Run("empty script", func(t *testing.T) {
+			v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+			putArgsOnStack([]byte{}, manifestBytes)
+			require.Error(t, contractUpdate(ic))
+		})
+
+		v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All)
+		putArgsOnStack(stackitem.Null{}, manifestBytes)
 		require.NoError(t, contractUpdate(ic))
 
 		// updated contract should have new scripthash

pkg/vm/stack.go

@@ -100,6 +100,19 @@ func (e *Element) Bytes() []byte {
 	return bs
 }
 
+// BytesOrNil attempts to get the underlying value of the element as a byte array or nil.
+// Will panic if the assertion failed which will be caught by the VM.
+func (e *Element) BytesOrNil() []byte {
+	if _, ok := e.value.(stackitem.Null); ok {
+		return nil
+	}
+	bs, err := e.value.TryBytes()
+	if err != nil {
+		panic(err)
+	}
+	return bs
+}
+
 // String attempts to get string from the element value.
 // It is assumed to be use in interops and panics if string is not a valid UTF-8 byte sequence.
 func (e *Element) String() string {