Skip to content

Commit

Permalink
fix missing return on ProcessUnsignedTransaction (#1917)
Browse files Browse the repository at this point in the history 
* fix missing return on ProcessUnsignedTransaction

* refactor

* fix unsigned tx revert response

* fix unit tests

* fix ProcessBatch err check before reading gas used

---------

Co-authored-by: tclemos <thiago@iden3.com>
  • Loading branch information
@ToniRamirezM @tclemos
ToniRamirezM and tclemos committed Mar 24, 2023
1 parent 2e0662e commit 1c8cd3d
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 40 deletions.
10 changes: 8 additions & 2 deletions jsonrpc/endpoints_eth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,17 @@ func (e *EthEndpoints) Call(arg *types.TxArgs, blockArg *types.BlockNumberOrHash
return rpcErrorResponse(types.DefaultErrorCode, "failed to convert arguments into an unsigned transaction", err)
}

result := e.state.ProcessUnsignedTransaction(ctx, tx, sender, blockNumber, false, dbTx)
if result.Failed() {
result, err := e.state.ProcessUnsignedTransaction(ctx, tx, sender, blockNumber, false, dbTx)
if err != nil {
return rpcErrorResponse(types.DefaultErrorCode, "failed to execute the unsigned transaction", err)
}

if result.Reverted() {
data := make([]byte, len(result.ReturnValue))
copy(data, result.ReturnValue)
return rpcErrorResponseWithData(types.RevertedErrorCode, result.Err.Error(), &data, nil)
} else if result.Failed() {
return rpcErrorResponse(types.DefaultErrorCode, result.Err.Error(), nil)
}

return types.ArgBytesPtr(result.ReturnValue), nil
Expand Down
67 changes: 59 additions & 8 deletions jsonrpc/endpoints_eth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,18 +145,19 @@ func TestCall(t *testing.T) {
txMatchBy := mock.MatchedBy(func(tx *ethTypes.Transaction) bool {
gasPrice := big.NewInt(0).SetBytes(*txArgs.GasPrice)
value := big.NewInt(0).SetBytes(*txArgs.Value)
return tx != nil &&
match := tx != nil &&
tx.To().Hex() == txArgs.To.Hex() &&
tx.Gas() == uint64(*txArgs.Gas) &&
tx.GasPrice().Uint64() == gasPrice.Uint64() &&
tx.Value().Uint64() == value.Uint64() &&
hex.EncodeToHex(tx.Data()) == hex.EncodeToHex(*txArgs.Data) &&
tx.Nonce() == nonce
return match
})
m.State.On("GetNonce", context.Background(), *txArgs.From, blockNumber, m.DbTx).Return(nonce, nil).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, *txArgs.From, blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
Once()
},
},
Expand Down Expand Up @@ -199,7 +200,7 @@ func TestCall(t *testing.T) {
m.State.On("GetNonce", context.Background(), *txArgs.From, blockNumber, m.DbTx).Return(nonce, nil).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, *txArgs.From, blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
Once()
},
},
Expand Down Expand Up @@ -238,7 +239,7 @@ func TestCall(t *testing.T) {
m.State.On("GetNonce", context.Background(), *txArgs.From, blockNumber, m.DbTx).Return(nonce, nil).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, *txArgs.From, blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
Once()
},
},
Expand Down Expand Up @@ -274,7 +275,10 @@ func TestCall(t *testing.T) {
dataMatch := hex.EncodeToHex(tx.Data()) == hex.EncodeToHex(*txArgs.Data)
return hasTx && gasMatch && toMatch && gasPriceMatch && valueMatch && dataMatch
})
m.State.On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(c.DefaultSenderAddress), blockNumber, false, m.DbTx).Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(c.DefaultSenderAddress), blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
Once()
},
},
{
Expand Down Expand Up @@ -309,7 +313,10 @@ func TestCall(t *testing.T) {
dataMatch := hex.EncodeToHex(tx.Data()) == hex.EncodeToHex(*txArgs.Data)
return hasTx && gasMatch && toMatch && gasPriceMatch && valueMatch && dataMatch
})
m.State.On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(c.DefaultSenderAddress), blockNumber, false, m.DbTx).Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, common.HexToAddress(c.DefaultSenderAddress), blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{ReturnValue: testCase.expectedResult}, nil).
Once()
},
},
{
Expand Down Expand Up @@ -347,7 +354,48 @@ func TestCall(t *testing.T) {
"latest",
},
expectedResult: nil,
expectedError: types.NewRPCError(types.RevertedErrorCode, "failed to process unsigned transaction"),
expectedError: types.NewRPCError(types.DefaultErrorCode, "failed to process unsigned transaction"),
setupMocks: func(c Config, m *mocksWrapper, testCase *testCase) {
blockNumber := uint64(1)
nonce := uint64(7)
m.DbTx.On("Rollback", context.Background()).Return(nil).Once()
m.State.On("BeginStateTransaction", context.Background()).Return(m.DbTx, nil).Once()
m.State.On("GetLastL2BlockNumber", context.Background(), m.DbTx).Return(blockNumber, nil).Once()
txArgs := testCase.params[0].(types.TxArgs)
txMatchBy := mock.MatchedBy(func(tx *ethTypes.Transaction) bool {
gasPrice := big.NewInt(0).SetBytes(*txArgs.GasPrice)
value := big.NewInt(0).SetBytes(*txArgs.Value)
hasTx := tx != nil
gasMatch := tx.Gas() == uint64(*txArgs.Gas)
toMatch := tx.To().Hex() == txArgs.To.Hex()
gasPriceMatch := tx.GasPrice().Uint64() == gasPrice.Uint64()
valueMatch := tx.Value().Uint64() == value.Uint64()
dataMatch := hex.EncodeToHex(tx.Data()) == hex.EncodeToHex(*txArgs.Data)
nonceMatch := tx.Nonce() == nonce
return hasTx && gasMatch && toMatch && gasPriceMatch && valueMatch && dataMatch && nonceMatch
})
m.State.On("GetNonce", context.Background(), *txArgs.From, blockNumber, m.DbTx).Return(nonce, nil).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, *txArgs.From, blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{Err: errors.New("failed to process unsigned transaction")}, nil).
Once()
},
},
{
name: "Transaction with all information but reverted to process unsigned transaction",
params: []interface{}{
types.TxArgs{
From: state.HexToAddressPtr("0x1"),
To: state.HexToAddressPtr("0x2"),
Gas: types.ArgUint64Ptr(24000),
GasPrice: types.ArgBytesPtr(big.NewInt(1).Bytes()),
Value: types.ArgBytesPtr(big.NewInt(2).Bytes()),
Data: types.ArgBytesPtr([]byte("data")),
},
"latest",
},
expectedResult: nil,
expectedError: types.NewRPCError(types.RevertedErrorCode, "execution reverted"),
setupMocks: func(c Config, m *mocksWrapper, testCase *testCase) {
blockNumber := uint64(1)
nonce := uint64(7)
Expand All @@ -368,7 +416,10 @@ func TestCall(t *testing.T) {
return hasTx && gasMatch && toMatch && gasPriceMatch && valueMatch && dataMatch && nonceMatch
})
m.State.On("GetNonce", context.Background(), *txArgs.From, blockNumber, m.DbTx).Return(nonce, nil).Once()
m.State.On("ProcessUnsignedTransaction", context.Background(), txMatchBy, *txArgs.From, blockNumber, false, m.DbTx).Return(&runtime.ExecutionResult{Err: errors.New("failed to process unsigned transaction")}).Once()
m.State.
On("ProcessUnsignedTransaction", context.Background(), txMatchBy, *txArgs.From, blockNumber, false, m.DbTx).
Return(&runtime.ExecutionResult{Err: runtime.ErrExecutionReverted}, nil).
Once()
},
},
}
Expand Down
14 changes: 12 additions & 2 deletions jsonrpc/mocks/mock_state.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion jsonrpc/types/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ type StateInterface interface {
GetTransactionReceipt(ctx context.Context, transactionHash common.Hash, dbTx pgx.Tx) (*types.Receipt, error)
IsL2BlockConsolidated(ctx context.Context, blockNumber uint64, dbTx pgx.Tx) (bool, error)
IsL2BlockVirtualized(ctx context.Context, blockNumber uint64, dbTx pgx.Tx) (bool, error)
ProcessUnsignedTransaction(ctx context.Context, tx *types.Transaction, senderAddress common.Address, l2BlockNumber uint64, noZKEVMCounters bool, dbTx pgx.Tx) *runtime.ExecutionResult
ProcessUnsignedTransaction(ctx context.Context, tx *types.Transaction, senderAddress common.Address, l2BlockNumber uint64, noZKEVMCounters bool, dbTx pgx.Tx) (*runtime.ExecutionResult, error)
RegisterNewL2BlockEventHandler(h state.NewL2BlockEventHandler)
GetLastVirtualBatchNum(ctx context.Context, dbTx pgx.Tx) (uint64, error)
GetLastVerifiedBatch(ctx context.Context, dbTx pgx.Tx) (*state.VerifiedBatch, error)
Expand Down
42 changes: 21 additions & 21 deletions state/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,13 @@ func (s *State) EstimateGas(transaction *types.Transaction, senderAddress common

txExecutionOnExecutorTime := time.Now()
processBatchResponse, err := s.executorClient.ProcessBatch(ctx, processBatchRequest)
gasUsed = processBatchResponse.Responses[0].GasUsed
log.Debugf("executor time: %vms", time.Since(txExecutionOnExecutorTime).Milliseconds())
if err != nil {
log.Errorf("error estimating gas: %v", err)
return false, false, gasUsed, err
} else if processBatchResponse.Error != executor.EXECUTOR_ERROR_NO_ERROR {
}
gasUsed = processBatchResponse.Responses[0].GasUsed
if processBatchResponse.Error != executor.EXECUTOR_ERROR_NO_ERROR {
err = executor.ExecutorErr(processBatchResponse.Error)
s.LogExecutorError(processBatchResponse.Error, processBatchRequest)
return false, false, gasUsed, err
Expand Down Expand Up @@ -1212,34 +1213,35 @@ func (s *State) PreProcessTransaction(ctx context.Context, tx *types.Transaction
}

response, err := s.internalProcessUnsignedTransaction(ctx, tx, sender, lastL2BlockNumber, false, dbTx)
if err != nil && !errors.Is(err, runtime.ErrExecutionReverted) {
if err != nil {
return nil, err
}

return response, nil
}

// ProcessUnsignedTransaction processes the given unsigned transaction.
func (s *State) ProcessUnsignedTransaction(ctx context.Context, tx *types.Transaction, senderAddress common.Address, l2BlockNumber uint64, noZKEVMCounters bool, dbTx pgx.Tx) *runtime.ExecutionResult {
func (s *State) ProcessUnsignedTransaction(ctx context.Context, tx *types.Transaction, senderAddress common.Address, l2BlockNumber uint64, noZKEVMCounters bool, dbTx pgx.Tx) (*runtime.ExecutionResult, error) {
result := new(runtime.ExecutionResult)
response, err := s.internalProcessUnsignedTransaction(ctx, tx, senderAddress, l2BlockNumber, noZKEVMCounters, dbTx)
if err != nil {
result.Err = err
}
if response != nil && response.Responses[0] != nil {
r := response.Responses[0]
result.ReturnValue = r.ReturnValue
result.GasLeft = r.GasLeft
result.GasUsed = r.GasUsed
result.CreateAddress = r.CreateAddress
result.StateRoot = r.StateRoot.Bytes()

if result.Err == nil {
result.Err = r.RomError
}
return nil, err
}

return result
r := response.Responses[0]
result.ReturnValue = r.ReturnValue
result.GasLeft = r.GasLeft
result.GasUsed = r.GasUsed
result.CreateAddress = r.CreateAddress
result.StateRoot = r.StateRoot.Bytes()

if errors.Is(r.RomError, runtime.ErrExecutionReverted) {
result.Err = constructErrorFromRevert(r.RomError, r.ReturnValue)
} else {
result.Err = r.RomError
}

return result, nil
}

// ProcessUnsignedTransaction processes the given unsigned transaction.
Expand Down Expand Up @@ -1325,9 +1327,7 @@ func (s *State) internalProcessUnsignedTransaction(ctx context.Context, tx *type

if processBatchResponse.Responses[0].Error != pb.RomError(executor.ROM_ERROR_NO_ERROR) {
err := executor.RomErr(processBatchResponse.Responses[0].Error)
if isEVMRevertError(err) {
return response, constructErrorFromRevert(err, processBatchResponse.Responses[0].ReturnValue)
} else {
if !isEVMRevertError(err) {
return response, err
}
}
Expand Down
18 changes: 12 additions & 6 deletions state/state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1029,7 +1029,8 @@ func TestExecutorRevert(t *testing.T) {

unsignedTx := types.NewTransaction(2, scAddress, new(big.Int), 40000, new(big.Int).SetUint64(1), common.Hex2Bytes("4abbb40a"))

result := testState.ProcessUnsignedTransaction(ctx, unsignedTx, auth.From, lastL2BlockNumber, false, nil)
result, err := testState.ProcessUnsignedTransaction(ctx, unsignedTx, auth.From, lastL2BlockNumber, false, nil)
require.NoError(t, err)
require.NotNil(t, result.Err)
assert.Equal(t, fmt.Errorf("execution reverted: Today is not juernes").Error(), result.Err.Error())
}
Expand Down Expand Up @@ -1787,7 +1788,8 @@ func TestExecutorUnsignedTransactions(t *testing.T) {
})
l2BlockNumber := uint64(3)

result := testState.ProcessUnsignedTransaction(context.Background(), unsignedTxSecondRetrieve, common.HexToAddress("0x1000000000000000000000000000000000000000"), l2BlockNumber, true, nil)
result, err := testState.ProcessUnsignedTransaction(context.Background(), unsignedTxSecondRetrieve, common.HexToAddress("0x1000000000000000000000000000000000000000"), l2BlockNumber, true, nil)
require.NoError(t, err)
// assert unsigned tx
assert.Nil(t, result.Err)
assert.Equal(t, "0000000000000000000000000000000000000000000000000000000000000001", hex.EncodeToString(result.ReturnValue))
Expand Down Expand Up @@ -2977,25 +2979,29 @@ func TestExecutorUnsignedTransactionsWithCorrectL2BlockStateRoot(t *testing.T) {
})

l2BlockNumber := uint64(1)
result := testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
result, err := testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
require.NoError(t, err)
// assert unsigned tx
assert.Nil(t, result.Err)
assert.Equal(t, "0000000000000000000000000000000000000000000000000000000000000000", hex.EncodeToString(result.ReturnValue))

l2BlockNumber = uint64(2)
result = testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
result, err = testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
require.NoError(t, err)
// assert unsigned tx
assert.Nil(t, result.Err)
assert.Equal(t, "0000000000000000000000000000000000000000000000000000000000000001", hex.EncodeToString(result.ReturnValue))

l2BlockNumber = uint64(3)
result = testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
result, err = testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
require.NoError(t, err)
// assert unsigned tx
assert.Nil(t, result.Err)
assert.Equal(t, "0000000000000000000000000000000000000000000000000000000000000002", hex.EncodeToString(result.ReturnValue))

l2BlockNumber = uint64(4)
result = testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
result, err = testState.ProcessUnsignedTransaction(context.Background(), getCountUnsignedTx, auth.From, l2BlockNumber, true, nil)
require.NoError(t, err)
// assert unsigned tx
assert.Nil(t, result.Err)
assert.Equal(t, "0000000000000000000000000000000000000000000000000000000000000003", hex.EncodeToString(result.ReturnValue))
Expand Down

0 comments on commit 1c8cd3d

Please sign in to comment.