Skip to content

Commit

Permalink
Merge pull request onflow#6051 from onflow/ramtin/6035-bug-fix-for-ev…
Browse files Browse the repository at this point in the history 
…m-return-value

[Flow EVM] Bug fix - capture and report returned data in case of evm tx failure
  • Loading branch information
@sideninja
sideninja authored Jun 7, 2024
2 parents 722262d + 754e261 commit 0b14c09
Show file tree
Hide file tree
Showing 15 changed files with 141 additions and 53 deletions.
2 changes: 1 addition & 1 deletion engine/execution/state/bootstrap/bootstrap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestBootstrapLedger(t *testing.T) {
}

func TestBootstrapLedger_ZeroTokenSupply(t *testing.T) {
expectedStateCommitmentBytes, _ := hex.DecodeString("909ef788fb5c7eaf0f58e81c4c7853557785867fa514ac52ef43cdeb23eb2498,")
expectedStateCommitmentBytes, _ := hex.DecodeString("aff1aafa7a34803d7b545791e60b7ffbcae52be602cef514430170969652d050")
expectedStateCommitment, err := flow.ToStateCommitment(expectedStateCommitmentBytes)
require.NoError(t, err)

Expand Down
6 changes: 4 additions & 2 deletions fvm/evm/emulator/emulator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ func (proc *procedure) deployAt(
tracer.CaptureStart(proc.evm, caller.ToCommon(), to.ToCommon(), true, data, gasLimit, value)

defer func() {
tracer.CaptureEnd(res.ReturnedValue, res.GasConsumed, res.VMError)
tracer.CaptureEnd(res.ReturnedData, res.GasConsumed, res.VMError)
}()
}

Expand Down Expand Up @@ -523,9 +523,11 @@ func (proc *procedure) run(
if execResult != nil {
res.GasConsumed = execResult.UsedGas
res.Index = uint16(txIndex)
// we need to capture the returned value no matter the status
// if the tx is reverted the error message is returned as returned value
res.ReturnedData = execResult.ReturnData

if !execResult.Failed() { // collect vm errors
res.ReturnedValue = execResult.ReturnData
// If the transaction created a contract, store the creation address in the receipt,
if msg.To == nil {
deployedAddress := types.NewAddress(gethCrypto.CreateAddress(msg.From, msg.Nonce))
Expand Down
47 changes: 43 additions & 4 deletions fvm/evm/emulator/emulator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"math"
"math/big"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -211,7 +212,7 @@ func TestContractInteraction(t *testing.T) {
require.NoError(t, err)
nonce += 1

ret := new(big.Int).SetBytes(res.ReturnedValue)
ret := new(big.Int).SetBytes(res.ReturnedData)
require.Equal(t, num, ret)
require.GreaterOrEqual(t, res.GasConsumed, uint64(23_000))
})
Expand All @@ -232,11 +233,49 @@ func TestContractInteraction(t *testing.T) {
require.NoError(t, err)
nonce += 1

ret := new(big.Int).SetBytes(res.ReturnedValue)
ret := new(big.Int).SetBytes(res.ReturnedData)
require.Equal(t, blockNumber, ret)
})
})

RunWithNewEmulator(t, backend, rootAddr, func(env *emulator.Emulator) {
RunWithNewBlockView(t, env, func(blk types.BlockView) {
res, err := blk.DirectCall(
types.NewContractCall(
testAccount,
contractAddr,
testContract.MakeCallData(t, "assertError"),
1_000_000,
big.NewInt(0), // this should be zero because the contract doesn't have receiver
nonce,
),
)
require.NoError(t, err)
nonce += 1
require.Error(t, res.VMError)
strings.Contains(string(res.ReturnedData), "Assert Error Message")
})
})

RunWithNewEmulator(t, backend, rootAddr, func(env *emulator.Emulator) {
RunWithNewBlockView(t, env, func(blk types.BlockView) {
res, err := blk.DirectCall(
types.NewContractCall(
testAccount,
contractAddr,
testContract.MakeCallData(t, "customError"),
1_000_000,
big.NewInt(0), // this should be zero because the contract doesn't have receiver
nonce,
),
)
require.NoError(t, err)
nonce += 1
require.Error(t, res.VMError)
strings.Contains(string(res.ReturnedData), "Value is too low")
})
})

RunWithNewEmulator(t, backend, rootAddr, func(em *emulator.Emulator) {
ctx := types.NewDefaultBlockContext(blockNumber.Uint64())
blk, err := em.NewBlockView(ctx)
Expand All @@ -254,7 +293,7 @@ func TestContractInteraction(t *testing.T) {
require.NoError(t, err)
nonce += 1

ret := new(big.Int).SetBytes(res.ReturnedValue)
ret := new(big.Int).SetBytes(res.ReturnedData)
require.Equal(t, types.FlowEVMPreviewNetChainID, ret)
})
})
Expand Down Expand Up @@ -752,7 +791,7 @@ func TestCallingExtraPrecompiles(t *testing.T) {
),
)
require.NoError(t, err)
require.Equal(t, output, res.ReturnedValue)
require.Equal(t, output, res.ReturnedData)
})
})
})
Expand Down
26 changes: 13 additions & 13 deletions fvm/evm/evm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func TestEVMRun(t *testing.T) {
require.Equal(t, blockEventPayload.Hash, txEventPayload.BlockHash)
require.Equal(t, blockEventPayload.Height, txEventPayload.BlockHeight)
require.Equal(t, blockEventPayload.TotalGasUsed, txEventPayload.GasConsumed)
require.Equal(t, uint64(43807), blockEventPayload.TotalGasUsed)
require.Equal(t, uint64(43785), blockEventPayload.TotalGasUsed)
require.Empty(t, txEventPayload.ContractAddress)

// append the state
Expand Down Expand Up @@ -206,7 +206,7 @@ func TestEVMRun(t *testing.T) {
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
require.Empty(t, res.ErrorMessage)
require.Nil(t, res.DeployedContractAddress)
require.Equal(t, num, new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.Equal(t, num, new(big.Int).SetBytes(res.ReturnedData).Int64())
})
})

Expand Down Expand Up @@ -317,7 +317,7 @@ func TestEVMRun(t *testing.T) {
require.Equal(t, types.StatusSuccessful, res.Status)
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
require.Empty(t, res.ErrorMessage)
require.Equal(t, int64(0), new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.Equal(t, int64(0), new(big.Int).SetBytes(res.ReturnedData).Int64())
})
})

Expand Down Expand Up @@ -535,7 +535,7 @@ func TestEVMBatchRun(t *testing.T) {
blockEventPayload, err := types.DecodeBlockEventPayload(cadenceEvent)
require.NoError(t, err)
require.NotEmpty(t, blockEventPayload.Hash)
require.Equal(t, uint64(155183), blockEventPayload.TotalGasUsed)
require.Equal(t, uint64(155513), blockEventPayload.TotalGasUsed)

// append the state
snapshot = snapshot.Append(state)
Expand Down Expand Up @@ -584,7 +584,7 @@ func TestEVMBatchRun(t *testing.T) {
require.Equal(t, types.StatusSuccessful, res.Status)
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
require.Empty(t, res.ErrorMessage)
require.Equal(t, storedValues[len(storedValues)-1], new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.Equal(t, storedValues[len(storedValues)-1], new(big.Int).SetBytes(res.ReturnedData).Int64())
})
})

Expand Down Expand Up @@ -724,7 +724,7 @@ func TestEVMBatchRun(t *testing.T) {
require.Equal(t, types.StatusSuccessful, res.Status)
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
require.Empty(t, res.ErrorMessage)
require.Equal(t, num, new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.Equal(t, num, new(big.Int).SetBytes(res.ReturnedData).Int64())
})
})

Expand Down Expand Up @@ -869,7 +869,7 @@ func TestEVMBatchRun(t *testing.T) {
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
require.Equal(t, types.StatusSuccessful, res.Status)
require.Empty(t, res.ErrorMessage)
require.Equal(t, num, new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.Equal(t, num, new(big.Int).SetBytes(res.ReturnedData).Int64())
})
})
}
Expand Down Expand Up @@ -933,7 +933,7 @@ func TestEVMBlockData(t *testing.T) {
require.Equal(t, types.StatusSuccessful, res.Status)
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
require.Empty(t, res.ErrorMessage)
require.Equal(t, ctx.BlockHeader.Timestamp.Unix(), new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.Equal(t, ctx.BlockHeader.Timestamp.Unix(), new(big.Int).SetBytes(res.ReturnedData).Int64())

})
}
Expand Down Expand Up @@ -1331,7 +1331,7 @@ func TestCadenceOwnedAccountFunctionalities(t *testing.T) {
let res = cadenceOwnedAccount.deploy(
code: code,
gasLimit: 1000000,
gasLimit: 2_000_000,
value: EVM.Balance(attoflow: 1230000000000000000)
)
destroy cadenceOwnedAccount
Expand Down Expand Up @@ -1364,7 +1364,7 @@ func TestCadenceOwnedAccountFunctionalities(t *testing.T) {
require.Empty(t, res.ErrorMessage)
require.NotNil(t, res.DeployedContractAddress)
// we strip away first few bytes because they contain deploy code
require.Equal(t, testContract.ByteCode[17:], []byte(res.ReturnedValue))
require.Equal(t, testContract.ByteCode[17:], []byte(res.ReturnedData))
})
})
}
Expand Down Expand Up @@ -1535,7 +1535,7 @@ func TestDryRun(t *testing.T) {
require.Equal(t, types.StatusSuccessful, res.Status)
require.Equal(t, types.ErrCodeNoError, res.ErrorCode)
// make sure the value we used in the dry-run is not the same as the value stored in contract
require.NotEqual(t, updatedValue, new(big.Int).SetBytes(res.ReturnedValue).Int64())
require.NotEqual(t, updatedValue, new(big.Int).SetBytes(res.ReturnedData).Int64())
})
})

Expand All @@ -1551,7 +1551,7 @@ func TestDryRun(t *testing.T) {
tx := gethTypes.NewContractCreation(
0,
big.NewInt(0),
uint64(1000000),
uint64(10_000_000),
big.NewInt(0),
testContract.ByteCode,
)
Expand All @@ -1560,7 +1560,7 @@ func TestDryRun(t *testing.T) {
require.Equal(t, types.ErrCodeNoError, result.ErrorCode)
require.Equal(t, types.StatusSuccessful, result.Status)
require.Greater(t, result.GasConsumed, uint64(0))
require.NotNil(t, result.ReturnedValue)
require.NotNil(t, result.ReturnedData)
require.NotNil(t, result.DeployedContractAddress)
require.NotEmpty(t, result.DeployedContractAddress.String())
})
Expand Down
38 changes: 19 additions & 19 deletions fvm/evm/handler/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ func TestHandler_TransactionRunOrPanic(t *testing.T) {
aa := handler.NewAddressAllocator()

result := &types.Result{
ReturnedValue: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
ReturnedData: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
testutils.GetRandomLogFixture(t),
Expand Down Expand Up @@ -443,7 +443,7 @@ func TestHandler_COA(t *testing.T) {
0, 0, 0, 0, 0, 0, 0, 0,
})
require.Equal(t, types.StatusSuccessful, ret.Status)
require.Equal(t, expected, ret.ReturnedValue)
require.Equal(t, expected, ret.ReturnedData)
})
})
})
Expand Down Expand Up @@ -593,7 +593,7 @@ func TestHandler_COA(t *testing.T) {
require.NotNil(t, result.DeployedContractAddress)
addr := *result.DeployedContractAddress
// skip first few bytes as they are deploy codes
assert.Equal(t, testContract.ByteCode[17:], []byte(result.ReturnedValue))
assert.Equal(t, testContract.ByteCode[17:], []byte(result.ReturnedData))
require.NotNil(t, addr)

num := big.NewInt(22)
Expand All @@ -609,7 +609,7 @@ func TestHandler_COA(t *testing.T) {
math.MaxUint64,
types.NewBalanceFromUFix64(0))

require.Equal(t, num, res.ReturnedValue.AsBigInt())
require.Equal(t, num, res.ReturnedData.AsBigInt())
})
})
})
Expand All @@ -634,7 +634,7 @@ func TestHandler_COA(t *testing.T) {
arch := handler.MakePrecompileAddress(1)

ret := foa.Call(arch, precompiles.FlowBlockHeightFuncSig[:], math.MaxUint64, types.NewBalanceFromUFix64(0))
require.Equal(t, big.NewInt(int64(blockHeight)), new(big.Int).SetBytes(ret.ReturnedValue))
require.Equal(t, big.NewInt(int64(blockHeight)), new(big.Int).SetBytes(ret.ReturnedData))
})
})
})
Expand Down Expand Up @@ -670,7 +670,7 @@ func TestHandler_COA(t *testing.T) {
math.MaxUint64,
types.EmptyBalance)

require.Equal(t, random.Bytes(), []byte(ret.ReturnedValue))
require.Equal(t, random.Bytes(), []byte(ret.ReturnedData))
})
})
})
Expand All @@ -692,8 +692,8 @@ func TestHandler_TransactionRun(t *testing.T) {
aa := handler.NewAddressAllocator()

result := &types.Result{
ReturnedValue: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
ReturnedData: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
testutils.GetRandomLogFixture(t),
Expand Down Expand Up @@ -736,9 +736,9 @@ func TestHandler_TransactionRun(t *testing.T) {
aa := handler.NewAddressAllocator()

result := &types.Result{
VMError: gethVM.ErrOutOfGas,
ReturnedValue: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
VMError: gethVM.ErrOutOfGas,
ReturnedData: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
testutils.GetRandomLogFixture(t),
Expand Down Expand Up @@ -835,7 +835,7 @@ func TestHandler_TransactionRun(t *testing.T) {
result := func(tx *gethTypes.Transaction) *types.Result {
return &types.Result{
DeployedContractAddress: &addr,
ReturnedValue: testutils.RandomData(t),
ReturnedData: testutils.RandomData(t),
GasConsumed: gasConsumed,
TxHash: tx.Hash(),
Logs: []*gethTypes.Log{
Expand Down Expand Up @@ -944,7 +944,7 @@ func TestHandler_TransactionRun(t *testing.T) {
result := func() *types.Result {
return &types.Result{
DeployedContractAddress: &addr,
ReturnedValue: testutils.RandomData(t),
ReturnedData: testutils.RandomData(t),
GasConsumed: gasConsumed,
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
Expand Down Expand Up @@ -1008,7 +1008,7 @@ func TestHandler_TransactionRun(t *testing.T) {
addr := testutils.RandomAddress(t)
result := &types.Result{
DeployedContractAddress: &addr,
ReturnedValue: testutils.RandomData(t),
ReturnedData: testutils.RandomData(t),
GasConsumed: testutils.RandomGas(1000),
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
Expand Down Expand Up @@ -1055,8 +1055,8 @@ func TestHandler_TransactionRun(t *testing.T) {
uploaded := make(chan struct{})

result := &types.Result{
TxHash: txID,
ReturnedValue: testutils.RandomData(t),
TxHash: txID,
ReturnedData: testutils.RandomData(t),
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
},
Expand Down Expand Up @@ -1123,8 +1123,8 @@ func TestHandler_TransactionRun(t *testing.T) {

uploaded := make(chan struct{})
result := &types.Result{
TxHash: gethCommon.HexToHash("0x1"),
ReturnedValue: testutils.RandomData(t),
TxHash: gethCommon.HexToHash("0x1"),
ReturnedData: testutils.RandomData(t),
Logs: []*gethTypes.Log{
testutils.GetRandomLogFixture(t),
},
Expand Down
2 changes: 2 additions & 0 deletions fvm/evm/stdlib/contract.cdc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,8 @@ contract EVM {
/// the evm for the call. For coa.deploy
/// calls it returns the code deployed to
/// the address provided in the contractAddress field.
/// in case of revert, the smart contract custom error message
/// is also returned here (see EIP-140 for more details).
access(all)
let data: [UInt8]

Expand Down
4 changes: 2 additions & 2 deletions fvm/evm/stdlib/contract.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ func NewResultValue(
},
{
Name: "data",
Value: interpreter.ByteSliceToByteArrayValue(inter, result.ReturnedValue),
Value: interpreter.ByteSliceToByteArrayValue(inter, result.ReturnedData),
},
{
Name: "deployedContract",
Expand Down Expand Up @@ -2372,7 +2372,7 @@ func ResultSummaryFromEVMResultValue(val cadence.Value) (*types.ResultSummary, e
ErrorCode: types.ErrorCode(errorCode),
ErrorMessage: string(errorMsg),
GasConsumed: uint64(gasUsed),
ReturnedValue: convertedData,
ReturnedData: convertedData,
DeployedContractAddress: convertedDeployedAddress,
}, nil

Expand Down
Loading

0 comments on commit 0b14c09

Please sign in to comment.