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

fix: max gas allowed per transaction #940

Merged
merged 1 commit into from
May 28, 2024
Merged

fix: max gas allowed per transaction #940

merged 1 commit into from
May 28, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner May 28, 2024 19:01
@dinhani-cw dinhani-cw enabled auto-merge (squash) May 28, 2024 19:01
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the changes are localized to a few lines and involve straightforward additions such as a constant definition and a minimal logic change in the gas limit handling. The context and impact of the changes are clear, making the review process relatively simple.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of min to cap the gas_limit might not account for scenarios where input.gas_limit could be of a different type or unexpectedly large, potentially leading to incorrect behavior if not properly validated or cast.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/evm/revm.rs
suggestion      

Consider adding a type check or ensuring that input.gas_limit is of a compatible type before using it in the min function to prevent type mismatches or overflow issues. [important]

relevant linetx_env.gas_limit = min(input.gas_limit.into(), GAS_MAX_LIMIT);

relevant filesrc/eth/evm/revm.rs
suggestion      

It might be beneficial to log or handle cases where the gas_limit is reduced due to exceeding GAS_MAX_LIMIT. This could aid in debugging and operational monitoring. [medium]

relevant linetx_env.gas_limit = min(input.gas_limit.into(), GAS_MAX_LIMIT);

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Security
Use checked arithmetic operations to prevent potential integer overflow

It's a good practice to handle potential integer overflow when performing operations like
minimum comparison between gas limits. Rust provides checked operations for integers that
return None on overflow. Using input.gas_limit.into().checked_min(GAS_MAX_LIMIT) ensures
that the operation is safe from overflow.

src/eth/evm/revm.rs [135]

-tx_env.gas_limit = min(input.gas_limit.into(), GAS_MAX_LIMIT);
+tx_env.gas_limit = input.gas_limit.into().checked_min(GAS_MAX_LIMIT).unwrap_or(GAS_MAX_LIMIT);
 
Suggestion importance[1-10]: 9

Why: This suggestion addresses a potential security issue by using checked arithmetic operations to prevent integer overflow. This is a significant improvement in terms of code safety and robustness.

9
Enhancement
Rename the constant to provide clearer context

Consider using a more descriptive name for the constant GAS_MAX_LIMIT to reflect its
purpose and context more clearly. For instance, MAX_TRANSACTION_GAS_LIMIT provides
immediate context that this limit is specific to transactions.

src/eth/evm/revm.rs [59]

-const GAS_MAX_LIMIT: u64 = 1_000_000_000;
+const MAX_TRANSACTION_GAS_LIMIT: u64 = 1_000_000_000;
 
Suggestion importance[1-10]: 7

Why: The suggestion to rename GAS_MAX_LIMIT to MAX_TRANSACTION_GAS_LIMIT improves code clarity by making the constant's purpose more explicit. However, the original name is not incorrect, so this is a minor enhancement.

7
Maintainability
Add a comment explaining the rationale behind setting the gas limit

To improve the maintainability and readability of the code, consider adding a brief
comment above the line where tx_env.gas_limit is set. This comment should explain why the
minimum of input.gas_limit and GAS_MAX_LIMIT is used, especially for future developers or
reviewers who might not be familiar with the context.

src/eth/evm/revm.rs [135]

+// Set the gas limit to the lesser of the input limit and the maximum allowed to prevent excessive resource use.
 tx_env.gas_limit = min(input.gas_limit.into(), GAS_MAX_LIMIT);
 
Suggestion importance[1-10]: 6

Why: Adding a comment to explain the rationale behind setting the gas limit improves code maintainability and readability. While helpful, it is not critical, hence a moderate score.

6
Best practice
Import only the min function explicitly to avoid potential naming conflicts and improve clarity

To ensure that the min function is used correctly and efficiently, consider explicitly
importing only the min function from the std::cmp module at the beginning of the file.
This helps in avoiding the entire cmp module's scope being accessible, which can prevent
potential naming conflicts and improve code clarity.

src/eth/evm/revm.rs [8]

+use std::cmp::min;
 
-
Suggestion importance[1-10]: 3

Why: The suggestion to explicitly import only the min function is correct but offers minimal improvement in this context. The likelihood of naming conflicts is low, and the current import is already clear.

3

@dinhani-cw dinhani-cw merged commit dd874b1 into main May 28, 2024
32 checks passed
@dinhani-cw dinhani-cw deleted the limit-gas branch May 28, 2024 19:08
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.

1 participant