fix(rpc): raise eth_estimateGas until the message succeeds + tests - #7473
fix(rpc): raise eth_estimateGas until the message succeeds + tests#7473LesnyRumcajs wants to merge 1 commit into
eth_estimateGas until the message succeeds + tests#7473Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR adds a ChangesEthereum gas estimation parity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant forest_dev
participant DevnetCommand
participant EthGasTestCommand
participant ForestRPC
participant LotusRPC
forest_dev->>DevnetCommand: run eth-gas
DevnetCommand->>ForestRPC: preflight block-number request
DevnetCommand->>LotusRPC: preflight block-number request
DevnetCommand->>EthGasTestCommand: run parity tests
EthGasTestCommand->>ForestRPC: eth_estimateGas
EthGasTestCommand->>LotusRPC: eth_estimateGas
EthGasTestCommand->>ForestRPC: submit estimated transaction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
e55d26b to
d44b015
Compare
abed524 to
fda49f2
Compare
4beeb1e to
98c7163
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/eth.rs (1)
2010-2036: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
gas_searchcan return a limit it never verified.
highstarts atmsg.gas_limit.max(1). Ifmsg.gas_limit >= BLOCK_GAS_LIMIT, thewhile high < BLOCK_GAS_LIMITloop never runs and the binary loop is skipped becausehigh == low. The function then returnsmsg.gas_limit, which the caller already proved fails ineth_gas_search.Clamp the start to below the block limit so the search still runs:
🐛 Proposed fix
- // `max(1)` keeps the doubling below able to make progress. - let mut high = msg.gas_limit.max(1); + // `max(1)` keeps the doubling below able to make progress. The clamp keeps a caller-supplied + // limit at or above the block maximum from skipping the search entirely. + let mut high = msg.gas_limit.clamp(1, BLOCK_GAS_LIMIT - 1); let mut low = high;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth.rs` around lines 2010 - 2036, Clamp the initial `high` value in gas_search to a value below BLOCK_GAS_LIMIT while preserving the minimum of 1, so the expansion loop runs even when msg.gas_limit is at or above the block limit. Keep low initialized from this clamped high value and preserve the existing search behavior.
🧹 Nitpick comments (1)
src/dev/subcommands/devnet_cmd/eth_gas.rs (1)
195-203: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe pinned block can precede the deploy on one node.
common_block_numbertakes the lower of the two heads. That guarantees both nodes have executed the block. It does not guarantee the block is at or after the height where the contract and the sender landed. If Forest lags Lotus afterlotus evm deploy, the pinned block can sit below the deploy height. The contract then does not exist at that block and the estimate fails for a reason unrelated to gas parity.The funding poll usually absorbs the skew, so this is an intermittent risk rather than a certain failure. Consider polling until both nodes return non-empty code for the deployed address before you pin the block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dev/subcommands/devnet_cmd/eth_gas.rs` around lines 195 - 203, Update pinned_common_block to wait until both forest_c and lotus_c report non-empty code for the deployed contract address before calling common_block_number. Reuse the existing deployed-address and polling utilities where available, preserving the final pinned block selection only after both nodes have observed the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 2010-2036: Clamp the initial `high` value in gas_search to a value
below BLOCK_GAS_LIMIT while preserving the minimum of 1, so the expansion loop
runs even when msg.gas_limit is at or above the block limit. Keep low
initialized from this clamped high value and preserve the existing search
behavior.
---
Nitpick comments:
In `@src/dev/subcommands/devnet_cmd/eth_gas.rs`:
- Around line 195-203: Update pinned_common_block to wait until both forest_c
and lotus_c report non-empty code for the deployed contract address before
calling common_block_number. Reuse the existing deployed-address and polling
utilities where available, preserving the final pinned block selection only
after both nodes have observed the contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64363bca-fa46-4b0f-9d4e-74bbe2ec9e51
📒 Files selected for processing (20)
.config/forest.dic.github/workflows/forest.ymlCHANGELOG.mdmise.tomlscripts/devnet/README.mdscripts/devnet/lotus.envscripts/devnet/test_harness.shsrc/dev/subcommands/devnet_cmd.rssrc/dev/subcommands/devnet_cmd/contracts/compile.shsrc/dev/subcommands/devnet_cmd/contracts/nested_gas/nested_gas.hexsrc/dev/subcommands/devnet_cmd/contracts/nested_gas/nested_gas.solsrc/dev/subcommands/devnet_cmd/eth_gas.rssrc/dev/subcommands/mod.rssrc/dev/subcommands/tests_cmd.rssrc/dev/subcommands/tests_cmd/helpers.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/errors.rssrc/rpc/methods/gas.rssrc/shim/executor.rssrc/state_manager/message_simulation.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
98c7163 to
c2292b3
Compare
1 participant
Summary of changes
Changes introduced in this pull request:
eth_estimateGasnow probes at the caller's gas limit (not block max), so it searches upward when EIP-150's 63/64 rule makes a nested call need more.forest-dev devnet eth-gas- theforest-dev devnet *is supposed to be run strictly on a devnet, due to some docker calls inside and expectations about the environment. It does parity checks with Lotus and proper contract checks, without relying on calibnet. I feel we can use this approach more; no need to overengineer it for now, once we have more use-cases we could give it more form.forest-dev devnet *as well, but I'd make it a separate PR.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
eth_estimateGashandling for nested calls and gas-limit scenarios.New Features
Documentation