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 20 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
4 changes: 3 additions & 1 deletion common/config/chaintype.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const (
ChainScroll ChainType = "scroll"
ChainWeMix ChainType = "wemix"
ChainXDai ChainType = "xdai" // Deprecated: use ChainGnosis instead
ChainZkEvm ChainType = "zkevm"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?
Didn't see any behavior depending on this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good call, missed cleaning this out after removing the zkevm specific method

ChainZkSync ChainType = "zksync"
)

Expand All @@ -31,13 +32,14 @@ var ErrInvalidChainType = fmt.Errorf("must be one of %s or omitted", strings.Joi
string(ChainOptimismBedrock),
string(ChainScroll),
string(ChainWeMix),
string(ChainZkEvm),
string(ChainZkSync),
}, ", "))

// IsValid returns true if the ChainType value is known or empty.
func (c ChainType) IsValid() bool {
switch c {
case "", ChainArbitrum, ChainCelo, ChainGnosis, ChainKroma, ChainMetis, ChainOptimismBedrock, ChainScroll, ChainWeMix, ChainXDai, ChainZkSync:
case "", ChainArbitrum, ChainCelo, ChainGnosis, ChainKroma, ChainMetis, ChainOptimismBedrock, ChainScroll, ChainWeMix, ChainXDai, ChainZkEvm, ChainZkSync:
return true
}
return false
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) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

How should the product understand if the received error is OOC type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we standardize the error we send back, they could use ErrOutOfCounters to determine if the error is an OOC error. I think to make it simpler for them though we'd maybe need to return a second output either another error or a flag. But I took this approach in efforts to keep the return a single field.

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 error ahead of time. It will not return an error for non-zk chains.
CheckTxValidity(ctx context.Context, from common.Address, to common.Address, data []byte) error
}

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) error {
return pkgerrors.New("not implemented. client was deprecated. New methods are added only to satisfy type constraints while we are migrating to new alternatives")
}
16 changes: 15 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 Expand Up @@ -511,6 +521,10 @@ func ClassifySendError(err error, lggr logger.SugaredLogger, tx *types.Transacti
)
return commonclient.ExceedsMaxFee
}
if sendError.IsOutOfCounters() {
lggr.Infow("Transaction encountered zk out-of-counters error", "err", sendError)
return commonclient.OutOfCounters
}
lggr.Criticalw("Unknown error encountered when sending transaction", "err", err, "etx", tx)
return commonclient.Unknown
}
18 changes: 18 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) error {
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) error {
return nil
}

func toCallMsg(params map[string]interface{}) ethereum.CallMsg {
var callMsg ethereum.CallMsg
toAddr, err := interfaceToAddress(params["to"])
Expand Down
74 changes: 74 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,74 @@
package client

import (
"context"
"fmt"

"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"
)

const ErrOutOfCounters = "not enough counters to continue the execution"

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
// It will also return service unavailable or timeout errors for callers to react to retryable errors
// 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) error {
err := simulateTransactionDefault(ctx, client, msg)
sendErr := NewSendError(err)
if sendErr == nil {
return nil
}
// Only return select errors to the caller
// Other errors are irrelevant to tx simulation to determin OOC error
if sendErr.IsOutOfCounters() {
return fmt.Errorf("%s: %w", ErrOutOfCounters, err)
}
if sendErr.IsServiceUnavailable() {
return fmt.Errorf("service not available when performing tx simulation: %w", err)
}
if sendErr.IsTimeout() {
return fmt.Errorf("timeout experienced when performing tx simulation: %w", err)
}
return nil
}

// 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
}
114 changes: 114 additions & 0 deletions core/chains/evm/client/tx_simulator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
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"),
}
err = client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg)
require.NoError(t, err)
})

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"),
}
err = client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg)
require.Error(t, err, client.ErrOutOfCounters)
})

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.Result = `"0x100"`
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"),
}
err = client.SimulateTransaction(ctx, ethClient, logger.TestSugared(t), "", msg)
require.NoError(t, err)
})
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
ChainID = '2442'
ChainType = 'zkevm'
FinalityDepth = 500
NoNewHeadsThreshold = '12m'
MinIncomingConfirmations = 1
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
ChainID = '1442'
ChainType = 'zkevm'
FinalityDepth = 500
NoNewHeadsThreshold = '12m'
MinIncomingConfirmations = 1
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
ChainID = '1101'
ChainType = 'zkevm'
FinalityDepth = 500
NoNewHeadsThreshold = '6m'
MinIncomingConfirmations = 1
Expand Down
Loading
Loading