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 explicit gas check #430

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Add explicit gas check #430

merged 2 commits into from
Aug 15, 2024

Conversation

cam-schultz
Copy link
Collaborator

Why this should be merged

Adds an explicit check against the gas limit specified in the Teleporter message.

It's possible for the required gas limit in a Teleporter message to exceed the block gas limit. This causes the transaction to fail as expected, but also causes the relayer to enter an unhealthy state, preventing it from updating the checkpoint in its database. This can cause compounding latency that will eventually prevent the relayer from running at all.

How this works

Generally speaking, such validity checks are done in ShouldSendMessage and the Decider service. This is done before
signature aggregation, to short circuit that step if the message is invalid. However, the number of signers impacts the gas estimation, so we can't use that in ShouldSendMessage. Instead, I added a hard cap of 12 million gas, which is the C-Chain's 15 million gas block limit minus other Warp message related overhead that is conservatively estimated. Specifically, the 12 million figure assumes:

  • 10kb Warp message size
  • 8kb Teleporter message size
  • 1000 signing validators
  • 5 Teleporter receipts (the maximum amount)

How this was tested

CI, updated unit tests

How is this documented

N/A

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks for turning it around quickly 🙏

We may want to consider making the max required gas limit configurable per destination chain (since subnet-evm chains can set their own block gas limits), but not necessary for this PR.

Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

LGTM and I agree that ShouldSendMessage is the right place for it for now.

If we come up with more cases like this we could group them together down the road into a separate helper method to distinguish between cases where the message is invalid due to relayer settings vs message validity like in this case.

@cam-schultz
Copy link
Collaborator Author

We may want to consider making the max required gas limit configurable per destination chain (since subnet-evm chains can set their own block gas limits), but not necessary for this PR.

Created an issue for this: #432

@cam-schultz cam-schultz merged commit cc96c98 into main Aug 15, 2024
7 checks passed
@cam-schultz cam-schultz deleted the explicit-gas-check branch August 15, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants