Problem: max-tx-gas-wanted is checked in consensus logic #1697
Conversation
Solution: - revert the change to check-tx mode only
yihuang
requested review from
JayT106 and
leejw51crypto
and removed request for
a team
WalkthroughThis pull request introduces several changes, including an update to the changelog for version Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@yihuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." 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 (
|
Signed-off-by: yihuang <[email protected]>
Signed-off-by: yihuang <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1697 +/- ##
===========================================
+ Coverage 16.87% 35.47% +18.59%
===========================================
Files 72 124 +52
Lines 6163 11897 +5734
===========================================
+ Hits 1040 4220 +3180
- Misses 5000 7253 +2253
- Partials 123 424 +301
|
![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
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
3-10: LGTM with a minor suggestion to improve clarity.The changelog entry is well-structured and properly categorized. The description could be slightly more descriptive to better explain the impact of the change.
Consider expanding the description to:
-* [#1697](https://github.com/crypto-org-chain/cronos/pull/1697) Check max-tx-gas-wanted only in CheckTx mode. +* [#1697](https://github.com/crypto-org-chain/cronos/pull/1697) Move max-tx-gas-wanted validation to CheckTx mode only, removing it from consensus logic to improve transaction processing efficiency.🧰 Tools
🪛 Markdownlint
3-3: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)app/upgrades.go(1 hunks)go.mod(1 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
3-3: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 GitHub Check: codecov/patch
app/upgrades.go
[warning] 48-49: app/upgrades.go#L48-L49
Added lines #L48 - L49 were not covered by tests
🔇 Additional comments (3)
app/upgrades.go (2)
45-49: Add documentation and tests for the hotfix upgrade handler
While the implementation is straightforward, it would benefit from:
- A comment explaining why this hotfix is needed and what migrations it runs
- Unit tests to verify the upgrade handler's behavior
Add a documentation comment above the handler:
+ // hotfixPlanName defines a testnet-specific upgrade handler that only runs migrations
+ // without modifying any parameters. This is used to test the v1.4.0-rc5 upgrade process
+ // in a controlled environment before mainnet deployment.
hotfixPlanName := "v1.4.0-rc5-testnet"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-49: app/upgrades.go#L48-L49
Added lines #L48 - L49 were not covered by tests
47-49: Verify the max-tx-gas-wanted modification
The PR objectives mention moving the max-tx-gas-wanted check to check-tx mode, but this change isn't directly visible in the upgrade handler. Please clarify:
- Is this change handled through the migrations?
- Are there additional files that implement this change?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-49: app/upgrades.go#L48-L49
Added lines #L48 - L49 were not covered by tests
go.mod (1)
276-276: Verify the changes in the new ethermint version
The update to ethermint version v0.6.1-0.20241118003049-cd93407bcf05 aligns with the PR objective of fixing the max-tx-gas-wanted check in consensus logic.
Let's verify the changes in the new ethermint version:
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Release Notes for Version v1.4.0-rc5
New Features
Improvements
max-tx-gas-wanted.These changes aim to enhance the overall functionality and reliability of the system.