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 19 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
134 changes: 134 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,134 @@
package client

import (
"context"
"errors"
"fmt"

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

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

commonclient "github.com/smartcontractkit/chainlink/v2/common/client"
)

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
// Used as an entry point for custom simulation across different chains
func SimulateTransaction(ctx context.Context, client simulatorClient, lggr logger.SugaredLogger, chainType config.ChainType, msg ethereum.CallMsg) error {
var err error
switch chainType {
case config.ChainZkEvm:
err = simulateTransactionZkEvm(ctx, client, lggr, msg)
default:
err = simulateTransactionDefault(ctx, client, msg)
}
// ClassifySendError will not have the proper fields for logging within the method due to the empty Transaction passed
code := ClassifySendError(err, lggr, &types.Transaction{}, msg.From, chainType.IsL2())
Copy link
Contributor

Choose a reason for hiding this comment

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

ClassifySendError is used only for Errors that are a response to eth_sendTransaction/eth_sendRawTransaction.

Any reason why you want to use that for thus simulate call?

I would think we just need to extract reason from the json response like you did for simulateTransactionDefault.

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 think it was a suggestion from Dimitris so we could take advantage of configurable errors after Nick's changes unless I misunderstood. You bring up a good point though that this would be mixing different types of errors though. I could setup a different parser for tx simulation errors but would that be overkill?

// Only return error if ZK OOC error is identified
if code == commonclient.OutOfCounters {
return errors.New(ErrOutOfCounters)
}
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
errCall := client.CallContext(ctx, &result, "eth_estimateGas", toCallArg(msg), "pending")
jsonErr, _ := ExtractRPCError(errCall)
if jsonErr != nil && len(jsonErr.Message) > 0 {
return errors.New(jsonErr.Message)
}
return nil
}

type zkEvmEstimateCountResponse struct {
CountersUsed struct {
GasUsed string
UsedKeccakHashes string
UsedPoseidonHashes string
UsedPoseidonPaddings string
UsedMemAligns string
UsedArithmetics string
UsedBinaries string
UsedSteps string
UsedSHA256Hashes string
}
CountersLimit struct {
MaxGasUsed string
MaxKeccakHashes string
MaxPoseidonHashes string
MaxPoseidonPaddings string
MaxMemAligns string
MaxArithmetics string
MaxBinaries string
MaxSteps string
MaxSHA256Hashes string
}
OocError string
}

// zkEVM implemented a custom zkevm_estimateCounters method to detect if a transaction would result in an out-of-counters (OOC) error
func simulateTransactionZkEvm(ctx context.Context, client simulatorClient, lggr logger.SugaredLogger, msg ethereum.CallMsg) error {
var result zkEvmEstimateCountResponse
err := client.CallContext(ctx, &result, "zkevm_estimateCounters", toCallArg(msg), "pending")
if err != nil {
return fmt.Errorf("failed to simulate tx: %w", err)
}
if detectZkEvmCounterOverflow(result) && len(result.OocError) > 0 {
friedemannf marked this conversation as resolved.
Show resolved Hide resolved
lggr.Debugw("zkevm_estimateCounters returned", "result", result)
return errors.New(result.OocError)
}
return nil
}

// Helper method for zkEvm to determine if response indicates an overflow
func detectZkEvmCounterOverflow(result zkEvmEstimateCountResponse) bool {
if result.CountersUsed.UsedKeccakHashes > result.CountersLimit.MaxKeccakHashes ||
result.CountersUsed.UsedPoseidonHashes > result.CountersLimit.MaxPoseidonHashes ||
result.CountersUsed.UsedPoseidonPaddings > result.CountersLimit.MaxPoseidonPaddings ||
result.CountersUsed.UsedMemAligns > result.CountersLimit.MaxMemAligns ||
result.CountersUsed.UsedArithmetics > result.CountersLimit.MaxArithmetics ||
result.CountersUsed.UsedBinaries > result.CountersLimit.MaxBinaries ||
result.CountersUsed.UsedSteps > result.CountersLimit.MaxSteps ||
result.CountersUsed.UsedSHA256Hashes > result.CountersLimit.MaxSHA256Hashes {
friedemannf marked this conversation as resolved.
Show resolved Hide resolved
return true
}
return false
}

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
}
Loading
Loading