Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Transaction Simulation to the TXM #12503

Merged
merged 27 commits into from
Mar 27, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5a87d87
Added tx simulator to maintain chain specific simulation methods
amit-momin Mar 20, 2024
bdb9fef
Fixed linting
amit-momin Mar 20, 2024
46b13be
Fixed linting and regenerated config docs
amit-momin Mar 20, 2024
141dfea
Generated mock
amit-momin Mar 20, 2024
4835554
Fixed config tests
amit-momin Mar 20, 2024
27922a3
Merge branch 'develop' into zk-tx-simulation
amit-momin Mar 20, 2024
befeaf0
Moved the tx simulator to the chain client
amit-momin Mar 20, 2024
fbdd110
Removed client from Txm struct
amit-momin Mar 20, 2024
2f7078f
Removed config from test helper
amit-momin Mar 20, 2024
fc70e7f
Added tests and logging
amit-momin Mar 21, 2024
389e18f
Added changeset
amit-momin Mar 21, 2024
37b783e
Fixed multinode test
amit-momin Mar 21, 2024
cdc58b2
Merge branch 'develop' into zk-tx-simulation
amit-momin Mar 21, 2024
e126805
Fixed linting
amit-momin Mar 21, 2024
0c22a6f
Merge branch 'develop' into zk-tx-simulation
amit-momin Mar 22, 2024
c7eae50
Fixed comment
amit-momin Mar 22, 2024
53efe22
Added test for non-OOC error
amit-momin Mar 22, 2024
8ad2510
Reduced context initializations in tests
amit-momin Mar 22, 2024
3f53867
Updated to account for all types of OOC errors
amit-momin Mar 22, 2024
ce8a3c7
Removed custom zk counter method and simplified error handling
amit-momin Mar 25, 2024
d7605cc
Removed zkevm chain type
amit-momin Mar 25, 2024
f301d43
Changed simulate tx method return object
amit-momin Mar 25, 2024
eb34bc7
Cleaned up stale comments
amit-momin Mar 25, 2024
68de594
Removed unused error message
amit-momin Mar 25, 2024
1a25270
Changed zk overflow validation method name
amit-momin Mar 26, 2024
6798e4d
Reverted method name change
amit-momin Mar 26, 2024
87c8eac
Merge branch 'develop' into zk-tx-simulation
amit-momin Mar 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/hungry-impalas-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": minor
---

Added a tx simulation feature to the chain client to enable testing for zk out-of-counter (OOC) errors
1 change: 1 addition & 0 deletions common/client/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const (
InsufficientFunds // Tx was rejected due to insufficient funds.
ExceedsMaxFee // Attempt's fee was higher than the node's limit and got rejected.
FeeOutOfValidRange // This error is returned when we use a fee price suggested from an RPC, but the network rejects the attempt due to an invalid range(mostly used by L2 chains). Retry by requesting a new suggested fee price.
OutOfCounters // The error returned when a transaction is too complex to be proven by zk circuits. This error is mainly returned by zk chains.
sendTxReturnCodeLen // tracks the number of errors. Must always be last
)

Expand Down
8 changes: 8 additions & 0 deletions common/client/multi_node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,14 @@ func TestMultiNode_SendTransaction_aggregateTxResults(t *testing.T) {
ExpectedCriticalErr: "expected at least one response on SendTransaction",
ResultsByCode: map[SendTxReturnCode][]error{},
},
{
Name: "Zk out of counter error",
ExpectedTxResult: "not enough keccak counters to continue the execution",
ExpectedCriticalErr: "",
ResultsByCode: map[SendTxReturnCode][]error{
OutOfCounters: {errors.New("not enough keccak counters to continue the execution")},
},
},
}

for _, testCase := range testCases {
Expand Down
12 changes: 11 additions & 1 deletion core/chains/evm/client/chain_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ type chainClient struct {
RPCClient,
rpc.BatchElem,
]
logger logger.SugaredLogger
logger logger.SugaredLogger
chainType config.ChainType
}

func NewChainClient(
Expand Down Expand Up @@ -269,3 +270,12 @@ func (c *chainClient) TransactionReceipt(ctx context.Context, txHash common.Hash
func (c *chainClient) LatestFinalizedBlock(ctx context.Context) (*evmtypes.Head, error) {
return c.multiNode.LatestFinalizedBlock(ctx)
}

func (c *chainClient) CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) *SendError {
Copy link
Collaborator

@dimriou dimriou Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would it make sense to rename this function to something like CheckTxOverflow or something like that to highlight the Zk scope? CheckTxValidity seems a bit vague.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this change

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is CheckTxValidity going to be used by products? Do they need a SendError code or a bool response would be sufficient enough. The reason I'm asking is because with this implementation the caller is forced to know about the SendError type, whereas it might be better if it gets abstracted completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prashant and I talked this over a bit in the thread. I don't think this could be handled with just a bool or a basic error type. There might be some additional errors that the product teams could check for when simulating like service unavailable or timeout. If it was a simple bool or error type, they wouldn't be able to differentiate between these which is why we opted to return SendError. That way they could check the specific types of errors with the built in methods like IsOutOfCounters()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an opportunity to clean up our error responses handling across ALL of chain client interface.
Can't be tackled in this PR, but likely a bigger effort into how to handle errors in standard way, for all of the Chain Client interfaces. Maybe a new ticket.
Created one here: https://smartcontract-it.atlassian.net/browse/BCI-2863

msg := ethereum.CallMsg{
From: from,
To: &to,
Data: data,
}
return SimulateTransaction(ctx, c, c.logger, c.chainType, msg)
}
7 changes: 7 additions & 0 deletions core/chains/evm/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ type Client interface {
PendingCallContract(ctx context.Context, msg ethereum.CallMsg) ([]byte, error)

IsL2() bool

// Simulate the transaction prior to sending to catch zk out-of-counters errors ahead of time
CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) *SendError
}

func ContextWithDefaultTimeout() (ctx context.Context, cancel context.CancelFunc) {
Expand Down Expand Up @@ -371,3 +374,7 @@ func (client *client) IsL2() bool {
func (client *client) LatestFinalizedBlock(_ context.Context) (*evmtypes.Head, error) {
return nil, pkgerrors.New("not implemented. client was deprecated. New methods are added only to satisfy type constraints while we are migrating to new alternatives")
}

func (client *client) CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) *SendError {
return NewSendError(pkgerrors.New("not implemented. client was deprecated. New methods are added only to satisfy type constraints while we are migrating to new alternatives"))
}
12 changes: 11 additions & 1 deletion core/chains/evm/client/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const (
TransactionAlreadyMined
Fatal
ServiceUnavailable
OutOfCounters
)

type ClientErrors = map[int]*regexp.Regexp
Expand Down Expand Up @@ -227,7 +228,11 @@ var zkSync = ClientErrors{
Fatal: regexp.MustCompile(`(?:: |^)(?:exceeds block gas limit|intrinsic gas too low|Not enough gas for transaction validation|Failed to pay the fee to the operator|Error function_selector = 0x, data = 0x|invalid sender. can't start a transaction from a non-account|max(?: priority)? fee per (?:gas|pubdata byte) higher than 2\^64-1|oversized data. max: \d+; actual: \d+)$`),
}

var clients = []ClientErrors{parity, geth, arbitrum, metis, substrate, avalanche, nethermind, harmony, besu, erigon, klaytn, celo, zkSync}
var zkEvm = ClientErrors{
OutOfCounters: regexp.MustCompile(`(?:: |^)not enough .* counters to continue the execution$`),
}

var clients = []ClientErrors{parity, geth, arbitrum, metis, substrate, avalanche, nethermind, harmony, besu, erigon, klaytn, celo, zkSync, zkEvm}

func (s *SendError) is(errorType int) bool {
if s == nil || s.err == nil {
Expand Down Expand Up @@ -309,6 +314,11 @@ func (s *SendError) IsServiceUnavailable() bool {
return s.is(ServiceUnavailable)
}

// IsOutOfCounters is a zk chain specific error returned if the transaction is too complex to prove on zk circuits
func (s *SendError) IsOutOfCounters() bool {
return s.is(OutOfCounters)
}

// IsTimeout indicates if the error was caused by an exceeded context deadline
func (s *SendError) IsTimeout() bool {
if s == nil {
Expand Down
22 changes: 22 additions & 0 deletions core/chains/evm/client/mocks/client.go

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

4 changes: 4 additions & 0 deletions core/chains/evm/client/null_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,3 +231,7 @@ func (nc *NullClient) IsL2() bool {
func (nc *NullClient) LatestFinalizedBlock(_ context.Context) (*evmtypes.Head, error) {
return nil, nil
}

func (nc *NullClient) CheckTxValidity(_ context.Context, _ common.Address, _ common.Address, _ []byte) *SendError {
return nil
}
4 changes: 4 additions & 0 deletions core/chains/evm/client/simulated_backend_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,10 @@ func (c *SimulatedBackendClient) ethGetLogs(ctx context.Context, result interfac
}
}

func (c *SimulatedBackendClient) CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) *SendError {
return nil
}

func toCallMsg(params map[string]interface{}) ethereum.CallMsg {
var callMsg ethereum.CallMsg
toAddr, err := interfaceToAddress(params["to"])
Expand Down
55 changes: 55 additions & 0 deletions core/chains/evm/client/tx_simulator.go
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create a separate simulatorClient instead of adding the logic directly to the existing client?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before when we were using the custom zkEvm method, it was so we could maintain chain specific code without crowding the client. It's possible now that it's simplified but I think there's a realistic possibility that we may need to support a custom method for a chain that's still in-progress.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might regret this in the future, but I still prefer to have this inside CheckTxOverflow method. Right now CheckTxOverflow is like a passthrough and has no logic. Chain client is supposed to serve exactly this purpose, which is to implement chain-specific logic that can't go inside multinode. As a reader, it would be easier for me to just read the entire thing under one file instead of having to understand why tx_simulator was created. I don't have a hard stance on this, so if you guys think it's cleaner as it is, then I'm ok keeping it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe argue that chain client is more so EVM specific code. So if something requires chain specific code where different EVM chains have different implementation, my opinion is we'd want to separate that from the chain client. That being said, we skipped on using the zkevm chain specific code here so I'm not against moving this to the chain client. We could always separate this out again later when there's a need. But before I do, let me get agreement from others.

Copy link
Contributor

@prashantkumar1982 prashantkumar1982 Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be ok with anything here.
Either make some changes here, or wait for the final solution to be clearer wrt other ZK chains, and then make the appropriate code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to wait to avoid another back and forth while product teams are waiting. But I agree we'll have more changes come to this code in the near future when we can make this decision.

Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package client

import (
"context"

"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/common/hexutil"

"github.com/smartcontractkit/chainlink-common/pkg/logger"
"github.com/smartcontractkit/chainlink/v2/common/config"
)

type simulatorClient interface {
CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error
}

// ZK chains can return an out-of-counters error
// This method allows a caller to determine if a tx would fail due to OOC error by simulating the transaction
// Used as an entry point in case custom simulation is required across different chains
func SimulateTransaction(ctx context.Context, client simulatorClient, lggr logger.SugaredLogger, chainType config.ChainType, msg ethereum.CallMsg) *SendError {
err := simulateTransactionDefault(ctx, client, msg)
return NewSendError(err)
}

// eth_estimateGas returns out-of-counters (OOC) error if the transaction would result in an overflow
func simulateTransactionDefault(ctx context.Context, client simulatorClient, msg ethereum.CallMsg) error {
var result hexutil.Big
return client.CallContext(ctx, &result, "eth_estimateGas", toCallArg(msg), "pending")
}

func toCallArg(msg ethereum.CallMsg) interface{} {
arg := map[string]interface{}{
"from": msg.From,
"to": msg.To,
}
if len(msg.Data) > 0 {
arg["input"] = hexutil.Bytes(msg.Data)
}
if msg.Value != nil {
arg["value"] = (*hexutil.Big)(msg.Value)
}
if msg.Gas != 0 {
arg["gas"] = hexutil.Uint64(msg.Gas)
}
if msg.GasPrice != nil {
arg["gasPrice"] = (*hexutil.Big)(msg.GasPrice)
}
if msg.GasFeeCap != nil {
arg["maxFeePerGas"] = (*hexutil.Big)(msg.GasFeeCap)
}
if msg.GasTipCap != nil {
arg["maxPriorityFeePerGas"] = (*hexutil.Big)(msg.GasTipCap)
}
return arg
}
113 changes: 113 additions & 0 deletions core/chains/evm/client/tx_simulator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package client_test

import (
"testing"

"github.com/ethereum/go-ethereum"
"github.com/stretchr/testify/require"
"github.com/tidwall/gjson"

"github.com/smartcontractkit/chainlink-common/pkg/logger"

"github.com/smartcontractkit/chainlink/v2/core/chains/evm/client"
"github.com/smartcontractkit/chainlink/v2/core/internal/cltest"
"github.com/smartcontractkit/chainlink/v2/core/internal/testutils"
)

func TestSimulateTx_Default(t *testing.T) {
t.Parallel()

fromAddress := testutils.NewAddress()
toAddress := testutils.NewAddress()
ctx := testutils.Context(t)

amit-momin marked this conversation as resolved.
Show resolved Hide resolved
t.Run("returns without error if simulation passes", func(t *testing.T) {
wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) {
switch method {
case "eth_subscribe":
resp.Result = `"0x00"`
resp.Notify = headResult
return
case "eth_unsubscribe":
resp.Result = "true"
return
case "eth_estimateGas":
resp.Result = `"0x100"`
}
return
}).WSURL().String()

ethClient := mustNewChainClient(t, wsURL)
err := ethClient.Dial(ctx)
require.NoError(t, err)

msg := ethereum.CallMsg{
From: fromAddress,
To: &toAddress,
Data: []byte("0x00"),
}
sendErr := client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg)
require.Empty(t, sendErr)
})

t.Run("returns error if simulation returns zk out-of-counters error", func(t *testing.T) {
wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) {
switch method {
case "eth_subscribe":
resp.Result = `"0x00"`
resp.Notify = headResult
return
case "eth_unsubscribe":
resp.Result = "true"
return
case "eth_estimateGas":
resp.Error.Code = -32000
resp.Result = `"0x100"`
resp.Error.Message = "not enough keccak counters to continue the execution"
}
return
}).WSURL().String()

ethClient := mustNewChainClient(t, wsURL)
err := ethClient.Dial(ctx)
require.NoError(t, err)

msg := ethereum.CallMsg{
From: fromAddress,
To: &toAddress,
Data: []byte("0x00"),
}
sendErr := client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg)
require.Equal(t, true, sendErr.IsOutOfCounters())
})

t.Run("returns without error if simulation returns non-OOC error", func(t *testing.T) {
wsURL := testutils.NewWSServer(t, &cltest.FixtureChainID, func(method string, params gjson.Result) (resp testutils.JSONRPCResponse) {
switch method {
case "eth_subscribe":
resp.Result = `"0x00"`
resp.Notify = headResult
return
case "eth_unsubscribe":
resp.Result = "true"
return
case "eth_estimateGas":
resp.Error.Code = -32000
resp.Error.Message = "something went wrong"
}
return
}).WSURL().String()

ethClient := mustNewChainClient(t, wsURL)
err := ethClient.Dial(ctx)
require.NoError(t, err)

msg := ethereum.CallMsg{
From: fromAddress,
To: &toAddress,
Data: []byte("0x00"),
}
sendErr := client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg)
require.Equal(t, false, sendErr.IsOutOfCounters())
})
}
Loading