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

Revert "Merge pull request #759 from onflow/mpeter/consolidate-tx-max-gas-limit" #769

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Feb 25, 2025

Description

This reverts commit 7274b59, reversing changes made to da6552b.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Summary by CodeRabbit

  • New Features

    • Introduced a unified gas limit threshold for processing raw transactions, ensuring that transaction gas limits exceeding the set maximum are clearly rejected.
  • Refactor

    • Standardized error reporting across transaction operations, resulting in consistent and structured error messages.
  • Tests

    • Updated and streamlined tests for transaction validations to align with the new gas limit control and error handling improvements.

…-gas-limit"

This reverts commit 7274b59, reversing
changes made to da6552b.
Copy link
Contributor

coderabbitai bot commented Feb 25, 2025

Walkthrough

The changes standardize gas limit references and error handling across the codebase. In the API endpoints, error returns are now processed via a centralized handleError function, and the gas limit is updated to use BlockGasLimit instead of the previous models.TxMaxGasLimit. Additionally, gas limit validation checks have been removed from transaction models and tests, while a new constant and validation logic are introduced in the requester service.

Changes

File(s) Change Summary
api/api.go, api/debug.go, api/utils.go Updated error handling to use handleError; replaced occurrences of models.TxMaxGasLimit with BlockGasLimit (and removed the models import in api/utils.go).
models/transaction.go, models/transaction_test.go Removed the TxMaxGasLimit constant and the associated validation check in ValidateTransaction, along with the corresponding test case.
services/requester/requester.go Introduced a new constant txMaxGasLimit set to 50,000,000; added a check in SendRawTransaction to enforce the gas limit; updated EstimateGas and getBlockView to use blockGasLimit.
tests/web3js/debug_traces_test.js, tests/web3js/eth_failure_handling_test.js Removed tests for gas limit violation scenarios and streamlined error handling tests by using a consolidated helpers.signAndSend approach in lieu of previous separate method calls.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant R as Requester/SendRawTransaction
    C->>R: Send Raw Transaction Request
    alt Gas limit too high
      R-->>C: Return error "transaction gas limit is too high"
    else Within allowed gas limit
      R->>R: Process transaction using EstimateGas (BlockGasLimit)
      R-->>C: Return transaction result
    end
Loading

Possibly related PRs

Suggested labels

Bugfix, EVM

Suggested reviewers

  • peterargue
  • zhangchiqing
  • janezpodhostnik

Poem

I'm a rabbit in the code burrow, hopping light and free,
Chewing on bugs and tweaks, crafting changes with glee.
Error handling now sings a song so neat,
And gas limits march in step, oh what a feat!
With constants updated and tests refined, I hop into the night,
A joyful bunny coding away, in pure delight!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between acc0b78 and 323165b.

📒 Files selected for processing (8)
  • api/api.go (3 hunks)
  • api/debug.go (1 hunks)
  • api/utils.go (1 hunks)
  • models/transaction.go (0 hunks)
  • models/transaction_test.go (0 hunks)
  • services/requester/requester.go (4 hunks)
  • tests/web3js/debug_traces_test.js (0 hunks)
  • tests/web3js/eth_failure_handling_test.js (1 hunks)
💤 Files with no reviewable changes (3)
  • models/transaction_test.go
  • tests/web3js/debug_traces_test.js
  • models/transaction.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Test
🔇 Additional comments (13)
services/requester/requester.go (4)

49-49: New transaction gas limit constant added.

A new constant txMaxGasLimit with value 50,000,000 has been introduced, which is distinct from the existing blockGasLimit (120,000,000). This establishes a separate limit specifically for individual transactions.


206-208: Added validation for transaction gas limit.

This validation ensures transactions don't exceed the maximum allowed gas limit of 50,000,000. This is a critical safeguard against potential resource exhaustion attacks where a malicious user might attempt to submit transactions with excessively high gas limits.


361-361: Changed gas limit reference in EstimateGas.

The code now uses blockGasLimit instead of the previously used models.TxMaxGasLimit. This aligns with the reversion mentioned in the PR title, separating block gas limit from transaction gas limit.


501-501: Updated gas limit reference in getBlockView.

The code now passes blockGasLimit to NewViewProvider instead of the previously used models.TxMaxGasLimit. This change is consistent with the other modifications in this PR.

api/debug.go (1)

192-192: Updated gas limit reference in TraceCall.

The code now uses BlockGasLimit instead of the previously used models.TxMaxGasLimit when creating the view provider. This change ensures consistency with the modifications in other files.

api/utils.go (1)

158-158: Updated gas limit reference in encodeTxFromArgs.

The variable gasLimit now uses BlockGasLimit instead of the previously used models.TxMaxGasLimit. This change is consistent with the overall approach of the PR, which separates block gas limit from transaction gas limit.

api/api.go (4)

29-29: Block gas limit constant defined.

A block gas limit constant is defined here with value 120,000,000, which is much higher than the transaction gas limit (50,000,000) defined in the requester service. This separation allows for more granular control over resource allocation.


578-578: Improved error handling in Call.

The error handling has been standardized to use a centralized handleError function instead of directly returning the error. This provides more consistent error reporting and makes the code more maintainable.


727-727: Updated gas limit reference in EstimateGas.

The code now returns BlockGasLimit instead of the previously used models.TxMaxGasLimit. This change aligns with the changes in other files and maintains consistent gas limit references throughout the codebase.


747-747: Improved error handling in EstimateGas.

Similar to the change in the Call method, error handling has been standardized to use the centralized handleError function, providing more consistent error reporting and improving code maintainability.

tests/web3js/eth_failure_handling_test.js (3)

10-16: LGTM: Using the helper function for transaction testing.

The use of helpers.signAndSend() simplifies the test code by encapsulating the transaction signing and sending logic in a reusable function.


6-23:

✅ Verification successful

Verify gas limit constants are in sync across the codebase.

Since this PR is reverting changes related to gas limit consolidation, ensure that the maximum gas limit value (50,000,000) used in this test matches the corresponding constant defined in the Go backend code.


🏁 Script executed:

#!/bin/bash
# Find all gas limit constant declarations in the Go code
grep -r "const.*gasLimit\|const.*GasLimit\|TxMaxGasLimit\|BlockGasLimit" --include="*.go" .

Length of output: 738


Gas limit values in test and backend are consistent
The gas limit constant defined in the Go backend (txMaxGasLimit = 50_000_000 in services/requester/requester.go) correctly aligns with the maximum gas limit used in your test. The error assertion in the test (expecting an error message that includes "tx gas limit exceeds the max value of 50000000") is therefore valid, and no changes to the test are needed.


18-18:

✅ Verification successful

Confirm the error message assertion matches the actual API response.

This assertion checks for an error message that mentions "tx gas limit exceeds the max value of 50000000". Ensure this matches the exact error message returned by the API after reverting the changes.


🏁 Script executed:

#!/bin/bash
# Check if the error message is consistent with API implementation
grep -r "tx gas limit exceeds the max value" --include="*.go" .

Length of output: 139


Verified: Error Message Assertion Correct as Implemented

  • The API's error message in ./models/errors/errors.go is defined as "tx gas limit exceeds the max value of %d: ", where %d is dynamically replaced at runtime.
  • In the test file tests/web3js/eth_failure_handling_test.js, the assertion checks for the substring "tx gas limit exceeds the max value of 50000000", which is present when the formatted error message is generated.
  • No changes are needed.
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@peterargue peterargue merged commit 6d72473 into main Feb 25, 2025
2 checks passed
@peterargue peterargue deleted the mpeter/revert-tx-max-gas-limit-change branch February 25, 2025 18:23
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