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.

Sign up for GitHub

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

Configurable teleporter-max-gas-limit #621

Merged
merged 8 commits into from
Jan 17, 2025
Merged

Configurable teleporter-max-gas-limit #621

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
107bca1
relayer/config: add configuration and default value for teleporter-ma…
najeal Jan 9, 2025
6cfeb07
vms/destination_client: use teleporter-max-gas-limit
najeal Jan 9, 2025
50cb709
messages/teleporter: teleporter-max-gas-limit checks are done over ea…
najeal Jan 9, 2025
5cc0762
Merge branch 'main' into configurable-max-gas
geoff-vball Jan 10, 2025
268aba7
move destination blockGasLimit validation into config package
najeal Jan 12, 2025
d9040bb
fix nits - block gas limit
najeal Jan 12, 2025
98082dc
relayer/config: switch default block gas limit const to unexported
najeal Jan 16, 2025
3ab8bbd
Merge branch 'main' into configurable-max-gas
cam-schultz Jan 16, 2025
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
10 changes: 4 additions & 6 deletions messages/teleporter/message_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@ import (
)

const (
// The maximum gas limit that can be specified for a Teleporter message
// Based on the C-Chain 15_000_000 gas limit per block, with other Warp message gas overhead conservatively estimated.
maxTeleporterGasLimit = 12_000_000
defaultBlockAcceptanceTimeout = 30 * time.Second
)

Expand Down Expand Up @@ -165,15 +162,16 @@ func (m *messageHandler) ShouldSendMessage(destinationClient vms.DestinationClie
if err != nil {
return false, fmt.Errorf("failed to calculate Teleporter message ID: %w", err)
}

requiredGasLimit := m.teleporterMessage.RequiredGasLimit.Uint64()
maxGasLimit := destinationClient.TeleporterMaxGasLimit()
// Check if the specified gas limit is below the maximum threshold
if m.teleporterMessage.RequiredGasLimit.Uint64() > maxTeleporterGasLimit {
if requiredGasLimit > maxGasLimit {
m.logger.Info(
"Gas limit exceeds maximum threshold",
zap.String("destinationBlockchainID", destinationBlockchainID.String()),
zap.String("teleporterMessageID", teleporterMessageID.String()),
zap.Uint64("requiredGasLimit", m.teleporterMessage.RequiredGasLimit.Uint64()),
zap.Uint64("maxGasLimit", maxTeleporterGasLimit),
zap.Uint64("maxGasLimit", maxGasLimit),
)
return false, nil
}
Expand Down
3 changes: 2 additions & 1 deletion messages/teleporter/message_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func TestShouldSendMessage(t *testing.T) {
require.NoError(t, err)

gasLimitExceededTeleporterMessage := validTeleporterMessage
gasLimitExceededTeleporterMessage.RequiredGasLimit = big.NewInt(maxTeleporterGasLimit + 1)
gasLimitExceededTeleporterMessage.RequiredGasLimit = big.NewInt(config.DefaultTeleporterMaxGasLimit + 1)
gasLimitExceededTeleporterMessageBytes, err := gasLimitExceededTeleporterMessage.Pack()
require.NoError(t, err)

Expand Down Expand Up @@ -233,6 +233,7 @@ func TestShouldSendMessage(t *testing.T) {
SenderAddress().
Return(test.senderAddressResult).
Times(test.senderAddressTimes)
mockClient.EXPECT().TeleporterMaxGasLimit().Return(uint64(config.DefaultTeleporterMaxGasLimit)).AnyTimes()
mockClient.EXPECT().DestinationBlockchainID().Return(destinationBlockchainID).AnyTimes()
if test.messageReceivedCall != nil {
messageReceivedInput := interfaces.CallMsg{
Expand Down
24 changes: 17 additions & 7 deletions relayer/config/destination_blockchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,23 @@ import (
"github.com/ethereum/go-ethereum/crypto"
)

const (
// The maximum gas limit that can be specified for a Teleporter message
// Based on the C-Chain 15_000_000 gas limit per block, with other Warp message gas overhead conservatively estimated.
DefaultTeleporterMaxGasLimit = 12_000_000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once the validation is moved to this package as I suggested in another comment, let's not export this const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we really want to make it unexported ?
We use it in two places :

Copy link
Collaborator

Choose a reason for hiding this comment

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

That unit test doesn't need the value of DefaultTeleporterMaxGasLimit specifically. Instead, we can define a gas limit const in the test itself.

Not a huge deal either way, but I'd prefer to not export application variables for unit tests as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I will make the change 👍

)

// Destination blockchain configuration. Specifies how to connect to and issue
// transactions on the destination blockchain.
type DestinationBlockchain struct {
SubnetID string `mapstructure:"subnet-id" json:"subnet-id"`
BlockchainID string `mapstructure:"blockchain-id" json:"blockchain-id"`
VM string `mapstructure:"vm" json:"vm"`
RPCEndpoint basecfg.APIConfig `mapstructure:"rpc-endpoint" json:"rpc-endpoint"`
KMSKeyID string `mapstructure:"kms-key-id" json:"kms-key-id"`
KMSAWSRegion string `mapstructure:"kms-aws-region" json:"kms-aws-region"`
AccountPrivateKey string `mapstructure:"account-private-key" json:"account-private-key"`
SubnetID string `mapstructure:"subnet-id" json:"subnet-id"`
BlockchainID string `mapstructure:"blockchain-id" json:"blockchain-id"`
VM string `mapstructure:"vm" json:"vm"`
RPCEndpoint basecfg.APIConfig `mapstructure:"rpc-endpoint" json:"rpc-endpoint"`
KMSKeyID string `mapstructure:"kms-key-id" json:"kms-key-id"`
KMSAWSRegion string `mapstructure:"kms-aws-region" json:"kms-aws-region"`
AccountPrivateKey string `mapstructure:"account-private-key" json:"account-private-key"`
TeleporterMaxGasLimit uint64 `mapstructure:"teleporter-max-gas-limit" json:"teleporter-max-gas-limit"`

// Fetched from the chain after startup
warpConfig WarpConfig
Expand All @@ -34,6 +41,9 @@ type DestinationBlockchain struct {

// Validates the destination subnet configuration
func (s *DestinationBlockchain) Validate() error {
if s.TeleporterMaxGasLimit == 0 {
s.TeleporterMaxGasLimit = DefaultTeleporterMaxGasLimit
}
if err := s.RPCEndpoint.Validate(); err != nil {
return fmt.Errorf("invalid rpc-endpoint in destination subnet configuration: %w", err)
}
Expand Down
23 changes: 20 additions & 3 deletions vms/destination_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
package vms

import (
"context"
"fmt"

"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/utils/logging"
"github.com/ava-labs/avalanchego/vms/platformvm/warp"
"github.com/ava-labs/icm-services/peers"
"github.com/ava-labs/icm-services/relayer/config"
"github.com/ava-labs/icm-services/vms/evm"
"github.com/ethereum/go-ethereum/common"
Expand All @@ -34,12 +36,17 @@ type DestinationClient interface {

// DestinationBlockchainID returns the ID of the destination chain
DestinationBlockchainID() ids.ID

// MaxGasLimit returns destination blockchain max gas limit
TeleporterMaxGasLimit() uint64
}

func NewDestinationClient(logger logging.Logger, subnetInfo *config.DestinationBlockchain) (DestinationClient, error) {
func NewDestinationClient(
logger logging.Logger, subnetInfo *config.DestinationBlockchain, cChainID ids.ID,
) (DestinationClient, error) {
switch config.ParseVM(subnetInfo.VM) {
case config.EVM:
return evm.NewDestinationClient(logger, subnetInfo)
return evm.NewDestinationClient(logger, subnetInfo, cChainID)
default:
return nil, fmt.Errorf("invalid vm")
}
Expand All @@ -51,6 +58,16 @@ func CreateDestinationClients(
relayerConfig config.Config,
) (map[ids.ID]DestinationClient, error) {
destinationClients := make(map[ids.ID]DestinationClient)
infoClient, err := peers.NewInfoAPI(relayerConfig.InfoAPI)
if err != nil {
logger.Error("Failed to create info API", zap.Error(err))
return nil, err
}
cChainID, err := infoClient.GetBlockchainID(context.Background(), "C")
if err != nil {
logger.Error("Failed to get C-Chain ID", zap.Error(err))
return nil, err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't need this either if we move the C-Chain gas limit validation to the config package, as I suggested in another comment.

for _, subnetInfo := range relayerConfig.DestinationBlockchains {
blockchainID, err := ids.FromString(subnetInfo.BlockchainID)
if err != nil {
Expand All @@ -69,7 +86,7 @@ func CreateDestinationClients(
continue
}

destinationClient, err := NewDestinationClient(logger, subnetInfo)
destinationClient, err := NewDestinationClient(logger, subnetInfo, cChainID)
if err != nil {
logger.Error(
"Could not create destination client",
Expand Down
44 changes: 32 additions & 12 deletions vms/evm/destination_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package evm

import (
"context"
"fmt"
"math/big"
"sync"

Expand Down Expand Up @@ -44,41 +45,55 @@ type destinationClient struct {
signer signer.Signer
evmChainID *big.Int
currentNonce uint64
teleporterMaxGasLimit uint64
logger logging.Logger
}

func NewDestinationClient(
logger logging.Logger,
destinationBlockchain *config.DestinationBlockchain,
cChainID ids.ID,
) (*destinationClient, error) {
// Dial the destination RPC endpoint
client, err := utils.NewEthClientWithConfig(
context.Background(),
destinationBlockchain.RPCEndpoint.BaseURL,
destinationBlockchain.RPCEndpoint.HTTPHeaders,
destinationBlockchain.RPCEndpoint.QueryParams,
)
destinationID, err := ids.FromString(destinationBlockchain.BlockchainID)
if err != nil {
logger.Error(
"Failed to dial rpc endpoint",
"Could not decode destination chain ID from string",
zap.Error(err),
)
return nil, err
}

destinationID, err := ids.FromString(destinationBlockchain.BlockchainID)
maxGasLimit := destinationBlockchain.TeleporterMaxGasLimit
isCChain := destinationID == cChainID
if isCChain && maxGasLimit > config.DefaultTeleporterMaxGasLimit {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this validation in DestinationBlockchain.Validate instead? If we do so, there shouldn't be any need to pass in cChainID as a parameter.

logger.Error("C-Chain max-gas-limit exceeded",
zap.Uint64("value", maxGasLimit),
zap.Uint64("cChainMaxValue", config.DefaultTeleporterMaxGasLimit),
)
return nil, fmt.Errorf(
"C-Chain max-gas-limit max gas limit %d exceeded", config.DefaultTeleporterMaxGasLimit,
)
}

sgnr, err := signer.NewSigner(destinationBlockchain)
if err != nil {
logger.Error(
"Could not decode destination chain ID from string",
"Failed to create signer",
zap.Error(err),
)
return nil, err
}

sgnr, err := signer.NewSigner(destinationBlockchain)
// Dial the destination RPC endpoint
client, err := utils.NewEthClientWithConfig(
context.Background(),
destinationBlockchain.RPCEndpoint.BaseURL,
destinationBlockchain.RPCEndpoint.HTTPHeaders,
destinationBlockchain.RPCEndpoint.QueryParams,
)
if err != nil {
logger.Error(
"Failed to create signer",
"Failed to dial rpc endpoint",
zap.Error(err),
)
return nil, err
Expand Down Expand Up @@ -117,6 +132,7 @@ func NewDestinationClient(
evmChainID: evmChainID,
currentNonce: nonce,
logger: logger,
teleporterMaxGasLimit: maxGasLimit,
}, nil
}

Expand Down Expand Up @@ -210,3 +226,7 @@ func (c *destinationClient) SenderAddress() common.Address {
func (c *destinationClient) DestinationBlockchainID() ids.ID {
return c.destinationBlockchainID
}

func (c *destinationClient) TeleporterMaxGasLimit() uint64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename TeleporterMaxGasLimit to BlockGasLimit here and elsewhere. The destination client tx construction limits are not exclusive to Teleporter.

return c.teleporterMaxGasLimit
}
14 changes: 14 additions & 0 deletions vms/mocks/mock_destination_client.go
View file
Open in desktop

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

Loading