Increase computation limit for Cadence tx #736
Conversation
We set the computation limit for the Cadence tx that wraps EVM txs, to its max value. We also add a validation check, for rejecting EVM txs which go beyond the max gas limit.
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThis pull request introduces a comprehensive approach to managing transaction gas limits across multiple components. A new constant Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
models/errors/errors.go (1)
69-74: Fix the error message format.The error message ends with a colon but doesn't include the actual gas limit value that exceeded the maximum.
Apply this diff to fix the error message format:
- "tx gas limit exceeds the max value of %d: ", + "tx gas limit exceeds the max value of %d",tests/web3js/eth_failure_handling_test.js (1)
6-23: Consider adding more test cases for edge conditions.While the current test case verifies the basic functionality, consider adding tests for:
- Gas limit exactly at the maximum (50,000,000)
- Gas limit at 0
- Gas limit slightly below maximum
Example test case for maximum gas limit:
it('should succeed when tx gas limit equals the max value', async () => { let receiver = web3.eth.accounts.create() const result = await helpers.signAndSend({ from: conf.eoa.address, to: receiver.address, value: 10, gasPrice: conf.minGasPrice, gasLimit: 50_000_000, }) assert.isDefined(result) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
models/errors/errors.go(1 hunks)services/requester/requester.go(4 hunks)tests/web3js/eth_failure_handling_test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test
🔇 Additional comments (3)
services/requester/requester.go (3)
47-47: LGTM! Well-defined constant.The constant follows Go naming conventions and uses a readable numeric format with underscores.
192-194: LGTM! Proper gas limit validation.The validation is correctly placed before other transaction validations and uses the appropriate error type.
598-599: LGTM! Appropriate compute limit setting.The transaction is correctly configured with the default maximum transaction gas limit from Flow.
| @@ -43,6 +44,7 @@ var ( | |||
|
|
|||
| const minFlowBalance = 2 | |||
| const blockGasLimit = 120_000_000 | |||
| const txMaxGasLimit = 50_000_000 | |||
There was a problem hiding this comment.
I thought max gas was 30M
There was a problem hiding this comment.
Ethereum has a max gas limit of 30M, per block. However, that is not the case with Flow EVM, we are not bounded by that. This magic value I found by running EVM transactions against Testnet & Mainnet, with SetComputeLimit(9999).
With >= 51M, we get the following error:
error: [Error Code: 1300] evm runtime error: insufficient computation
--> 8c5303eaa26202d6.EVM:529:8
There was a problem hiding this comment.
The 30M limit on Flow EVM was to keep some room for the Cadence compute units for the Cadence transaction that wraps the EVM transaction. See this FLIP - https://github.com/onflow/flips/blob/main/governance/20240508-computation-limit-hike.md under "Set Gas to compute conversion ratio ...".
There was a problem hiding this comment.
The weights were set so that 9999 computation equates to 50M evm gas (+- rounding error).
Like Vishal mentioned this was done because we wanted to fit at least 30M evm gas into one flow transaction, but each flow transaction also has a variable computation cost coming form the cadence code (that wraps the EVM transaction).
I see nothing wrong with adding this check on the gateway, we just have to remember to change it if we change the fee weights.
Closes: #732
Description
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Tests
Bug Fixes