Merge pull request #6118 from onflow/ramtin/6115-fix-potential-hash-i…

…ssue-for-direct-calls

[Flow EVM] fix the issue with the hash calculation of the direct calls

fvm/evm/emulator/emulator.go

@@ -115,10 +115,7 @@ func (bl *BlockView) DirectCall(call *types.DirectCall) (*types.Result, error) {
 	if err != nil {
 		return nil, err
 	}
-	txHash, err := call.Hash()
-	if err != nil {
-		return nil, err
-	}
+	txHash := call.Hash()
 	switch call.SubType {
 	case types.DepositCallSubType:
 		return proc.mintTo(call, txHash)

fvm/evm/emulator/emulator_test.go

@@ -60,9 +60,7 @@ func TestNativeTokenBridging(t *testing.T) {
 						res, err := blk.DirectCall(call)
 						require.NoError(t, err)
 						require.Equal(t, defaultCtx.DirectCallBaseGasUsage, res.GasConsumed)
-						expectedHash, err := call.Hash()
-						require.NoError(t, err)
-						require.Equal(t, expectedHash, res.TxHash)
+						require.Equal(t, call.Hash(), res.TxHash)
 						nonce += 1
 					})
 				})
@@ -94,9 +92,7 @@ func TestNativeTokenBridging(t *testing.T) {
 						res, err := blk.DirectCall(call)
 						require.NoError(t, err)
 						require.Equal(t, defaultCtx.DirectCallBaseGasUsage, res.GasConsumed)
-						expectedHash, err := call.Hash()
-						require.NoError(t, err)
-						require.Equal(t, expectedHash, res.TxHash)
+						require.Equal(t, call.Hash(), res.TxHash)
 						nonce += 1
 					})
 				})
@@ -155,9 +151,7 @@ func TestContractInteraction(t *testing.T) {
 						require.NoError(t, err)
 						require.NotNil(t, res.DeployedContractAddress)
 						contractAddr = *res.DeployedContractAddress
-						expectedHash, err := call.Hash()
-						require.NoError(t, err)
-						require.Equal(t, expectedHash, res.TxHash)
+						require.Equal(t, call.Hash(), res.TxHash)
 						nonce += 1
 					})
 					RunWithNewReadOnlyBlockView(t, env, func(blk types.ReadOnlyBlockView) {

fvm/evm/types/call.go

@@ -73,10 +73,12 @@ func (dc *DirectCall) Encode() ([]byte, error) {
 }
 
 // Hash computes the hash of a direct call
-func (dc *DirectCall) Hash() (gethCommon.Hash, error) {
+func (dc *DirectCall) Hash() gethCommon.Hash {
 	// we use geth transaction hash calculation since direct call hash is included in the
 	// block transaction hashes, and thus observed as any other transaction
-	return dc.Transaction().Hash(), nil
+	// We construct this Legacy tx type so the external 3rd party tools
+	// don't have to support a new type for the purpose of hash computation
+	return dc.Transaction().Hash()
 }
 
 // Message constructs a core.Message from the direct call
@@ -98,13 +100,21 @@ func (dc *DirectCall) Message() *gethCore.Message {
 
 // Transaction constructs a geth.Transaction from the direct call
 func (dc *DirectCall) Transaction() *gethTypes.Transaction {
+	// Since a direct call doesn't have a valid siganture
+	// and we need to somehow include the From feild for the purpose
+	// of hash calculation. we define the canonical format by
+	// using the FROM bytes to set the bytes for the R part of the tx (big endian),
+	// S captures the subtype of transaction and V is set to DirectCallTxType (255).
 	return gethTypes.NewTx(&gethTypes.LegacyTx{
 		GasPrice: big.NewInt(0),
 		Gas:      dc.GasLimit,
 		To:       dc.to(),
 		Value:    dc.Value,
 		Data:     dc.Data,
 		Nonce:    dc.Nonce,
+		R:        new(big.Int).SetBytes(dc.From.Bytes()),
+		S:        new(big.Int).SetBytes([]byte{dc.SubType}),
+		V:        new(big.Int).SetBytes([]byte{DirectCallTxType}),
 	})
 }
 

fvm/evm/types/call_test.go

@@ -1,9 +1,13 @@
 package types
 
 import (
+	"bytes"
+	"io"
 	"math/big"
 	"testing"
 
+	gethTypes "github.com/onflow/go-ethereum/core/types"
+	"github.com/onflow/go-ethereum/rlp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -20,20 +24,43 @@ func TestDirectCall(t *testing.T) {
 	}
 
 	t.Run("calculate hash", func(t *testing.T) {
-		h, err := dc.Hash()
+		h := dc.Hash()
+		assert.Equal(t, "0xed76124cc3c59f13e1113f5c380e2a67dab9bf616afc645073d2491fe3aecb62", h.Hex())
+
+		// the hash should stay the same after RLP encoding and decoding
+		var b bytes.Buffer
+		writer := io.Writer(&b)
+		err := dc.Transaction().EncodeRLP(writer)
+		require.NoError(t, err)
+
+		reconstructedTx := &gethTypes.Transaction{}
+		err = reconstructedTx.DecodeRLP(rlp.NewStream(io.Reader(&b), 1000))
 		require.NoError(t, err)
-		assert.Equal(t, "0xe28ff08eca95608646d765e3007b3710f7f2a8ac5e297431da1962c33487e7b6", h.Hex())
+
+		h = reconstructedTx.Hash()
+		assert.Equal(t, "0xed76124cc3c59f13e1113f5c380e2a67dab9bf616afc645073d2491fe3aecb62", h.Hex())
+	})
+
+	t.Run("same content except `from` should result in different hashes", func(t *testing.T) {
+		h := dc.Hash()
+		dc.From = Address{0x4, 0x5}
+		h2 := dc.Hash()
+		assert.NotEqual(t, h2.Hex(), h.Hex())
 	})
 
 	t.Run("construct transaction", func(t *testing.T) {
 		tx := dc.Transaction()
-		h, err := dc.Hash()
-		require.NoError(t, err)
+		h := dc.Hash()
 		assert.Equal(t, dc.Value, tx.Value())
 		assert.Equal(t, dc.To.ToCommon(), *tx.To())
 		assert.Equal(t, h, tx.Hash())
 		assert.Equal(t, dc.GasLimit, tx.Gas())
 		assert.Equal(t, dc.Data, tx.Data())
 		assert.Equal(t, uint64(0), tx.Nonce()) // no nonce exists for direct call
+
+		v, r, s := tx.RawSignatureValues()
+		require.Equal(t, dc.From.Bytes(), r.Bytes())
+		require.Equal(t, []byte{dc.SubType}, s.Bytes())
+		require.Equal(t, []byte{DirectCallTxType}, v.Bytes())
 	})
 }