Add support for the Filecoin.EthEstimateGas V2 #6364
Conversation
WalkthroughAdds a V2 RPC method for Filecoin.EthEstimateGas, centralizes EthCall and gas-estimation logic into shared helpers (eth_estimate_gas, eth_gas_search, gas_search, eth_call), updates RPC exports and tests, and adds changelog entries for V2 methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client
participant Handler as EthEstimateGasV2 Handler
participant Resolver as Tipset Resolver (V2)
participant Estimator as eth_estimate_gas()
participant Search as eth_gas_search()/gas_search()
participant Chain as Blockchain (estimate/apply/call)
Client->>Handler: Filecoin.EthEstimateGas(tx, blockParam)
Handler->>Resolver: resolve tipset (ExtBlockNumberOrHash or heaviest)
Resolver-->>Handler: Tipset
Handler->>Estimator: eth_estimate_gas(tx, tipset)
Estimator->>Chain: estimate_message_gas(msg)
alt estimate_message_gas succeeds
Chain-->>Estimator: estimated_gas
Estimator->>Search: eth_gas_search(msg, tipset)
else estimate_message_gas fails
Chain-->>Estimator: error
Estimator->>Chain: apply_message(msg) to capture revert info
Chain-->>Estimator: execution result (revert/error)
Estimator-->>Handler: return error
end
Search->>Chain: probe msg with gas limits (exponential -> binary)
loop search iterations
Chain-->>Search: execution result (exit code, data)
Search-->>Search: adjust bounds
end
Search-->>Estimator: found_gas_limit
Estimator->>Estimator: apply mpool overestimation factor
Estimator-->>Handler: EthUint64(gas)
Handler-->>Client: RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Comment |
sudo-shashank
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2047-2075: EthEstimateGas / EthEstimateGasV2 API comparison tests are well‑wiredThe updated address conversion with
EthAddress::try_from(msg.to)plus the pairedEthEstimateGas(v1) andEthEstimateGasV2(v2)RpcTest::identitycases correctly exercise both versions using the appropriate block selector types. Ownership (eth_to_addr.clone()for v1, move into v2) is handled cleanly, and the sharedPolicyOnRejected::Passkeeps failure behavior aligned.src/rpc/methods/eth.rs (1)
1831-1885: Shared EthEstimateGas / EthEstimateGasV2 implementation is sound; consider logging levelThe new
eth_estimate_gashelper cleanly centralizes gas estimation for bothEthEstimateGas(legacy paths) andEthEstimateGasV2(v2-only path) while:
- Forcing the
Messagegas limit to the zero sentinel sogas::estimate_message_gasperforms a full estimation.- Re-running the message via
apply_messageon estimation failure to surface properEthErrors::ExecutionReverteddetails when applicable.- Delegating to
eth_gas_searchto refine the gas limit when estimation succeeds.Functionally this is a solid design and aligns the error story between v1 and v2. The only minor nit is the use of
log::info!insideeth_estimate_gasfor every estimation; if this path is hot, you may want to downgrade those logs todebug!to avoid noisy production logs.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)src/rpc/methods/eth.rs(4 hunks)src/rpc/mod.rs(1 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rs
🧬 Code graph analysis (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
src/rpc/methods/eth/types.rs (9)
try_from(235-242)try_from(248-250)try_from(256-258)try_from(270-272)try_from(278-280)try_from(353-393)default(483-485)default(629-631)default(657-659)
src/rpc/methods/eth.rs (3)
src/rpc/methods/gas.rs (2)
estimate_message_gas(317-343)estimate_call_with_gas(214-266)src/rpc/error.rs (1)
error(103-103)src/rpc/methods/eth/utils.rs (1)
decode_payload(63-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Coverage
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
🔇 Additional comments (2)
src/rpc/mod.rs (1)
107-109: Wiring EthEstimateGasV2 into the RPC registry looks correctRegistering
EthEstimateGasV2infor_each_rpc_method!next toEthEstimateGasis consistent with howEthCall/EthCallV2are exposed and ensures the new v2 endpoint participates in both routing and OpenRPC generation.src/rpc/methods/eth.rs (1)
2671-2693: EthCall / EthCallV2 refactor via eth_call helper preserves behavior and improves reuseThe changes to
EthCallandEthCallV2to delegate into the sharedeth_callhelper look good:
- Both variants still resolve the correct tipset (
tipset_by_block_number_or_hashfor v1, asynctipset_by_block_number_or_hash_v2for v2), then calleth_call(&ctx, tx, ts).eth_callcentralizes:
EthCallMessage→Messageconversion.- Execution via
apply_message, so all EthCall paths now share consistent revert handling (EthErrors::ExecutionRevertedmapping).- Special-case handling for
ETHEREUM_ACCOUNT_MANAGER_ACTOR(empty bytes) and CBOR-decoding of non-empty return data viadecode_payload.- Restricting
EthCallV2toApiPaths::V2is aligned with the versioned API design introduced elsewhere.Net effect: less duplicated logic with no obvious behavioral regression.
Also applies to: 2695-2718
Pull request was converted to draft
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/rpc/methods/eth.rs (1)
1967-2063: [Critical] Infinite loop and unverified gas limit remain unfixed from previous review.The previous review identified two critical issues in
gas_searchthat have not been addressed:
- Infinite loop when message cannot succeed at block gas limit (lines 2043-2049):
When
can_succeed(..., BLOCK_GAS_LIMIT)returnsfalse, the exponential search will loop forever:
highsaturates toBLOCK_GAS_LIMIT(line 2048)lowis set toBLOCK_GAS_LIMIT(line 2047)- Loop condition
high <= BLOCK_GAS_LIMITremainstrue- Next iteration re-runs with same values indefinitely
This will hang RPC requests when users submit messages that cannot execute even with maximum gas.
Binary search now calls
can_succeed(median)correctly (line 2054) ✓ — this was fixed.Missing final verification (line 2062):
The function returns
highwithout confirming it actually allows execution to succeed. The binary search may converge to a failing gas limit.🔎 Apply this fix to prevent infinite loops and verify the result:
async fn gas_search<DB>( data: &Ctx<DB>, msg: &Message, prior_messages: &[ChainMessage], ts: Tipset, ) -> anyhow::Result<u64> where DB: Blockstore + Send + Sync + 'static, { let mut high = msg.gas_limit; let mut low = msg.gas_limit; async fn can_succeed<DB>( data: &Ctx<DB>, mut msg: Message, prior_messages: &[ChainMessage], ts: Tipset, limit: u64, ) -> anyhow::Result<bool> where DB: Blockstore + Send + Sync + 'static, { msg.gas_limit = limit; let (_invoc_res, apply_ret, _) = data .state_manager .call_with_gas( &mut msg.into(), prior_messages, Some(ts), VMTrace::NotTraced, ) .await?; Ok(apply_ret.msg_receipt().exit_code().is_success()) } while high <= BLOCK_GAS_LIMIT { if can_succeed(data, msg.clone(), prior_messages, ts.clone(), high).await? { break; } + // If we're already at the block gas limit and still failing, give up. + if high == BLOCK_GAS_LIMIT { + anyhow::bail!( + "message execution failed even at block gas limit ({})", + BLOCK_GAS_LIMIT + ); + } low = high; high = high.saturating_mul(2).min(BLOCK_GAS_LIMIT); } let mut check_threshold = high / 100; while (high - low) > check_threshold { let median = (high + low) / 2; if can_succeed(data, msg.clone(), prior_messages, ts.clone(), median).await? { high = median; } else { low = median; } check_threshold = median / 100; } + // Final sanity check: ensure the returned `high` actually succeeds. + if !can_succeed(data, msg.clone(), prior_messages, ts, high).await? { + anyhow::bail!("failed to find a gas limit that allows execution to succeed"); + } + Ok(high) }
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
1924-1926: Consider usingdebug!instead ofinfo!for these logs.Lines 1924 and 1926 use
log::info!for messages that appear to be debugging information ("correct gassed_msg: do eth_gas_search", "trying eth_gas search"). These will create noise in production logs for every gas estimation request.🔎 Reduce log verbosity:
- log::info!("correct gassed_msg: do eth_gas_search {gassed_msg:?}"); + tracing::debug!("running eth_gas_search for message: {gassed_msg:?}"); let expected_gas = eth_gas_search(ctx, gassed_msg, &tipset.key().into()).await?; - log::info!("trying eth_gas search: {expected_gas}"); + tracing::debug!("eth_gas_search result: {expected_gas}"); Ok(expected_gas.into())
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rpc/methods/eth.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/rpc/methods/eth.rs (4)
src/rpc/methods/eth/types.rs (3)
default(483-485)default(629-631)default(657-659)src/rpc/methods/gas.rs (1)
estimate_message_gas(317-343)src/rpc/error.rs (1)
error(103-103)src/rpc/methods/eth/utils.rs (1)
decode_payload(63-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Coverage
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
🔇 Additional comments (2)
src/rpc/methods/eth.rs (2)
1861-1886: LGTM! V2 endpoint correctly configured.The
EthEstimateGasV2implementation properly:
- Uses
ApiPaths::V2flag to expose only on the V2 API path- Accepts
ExtBlockNumberOrHashfor extended block parameters (Safe/Finalized)- Delegates tipset resolution to the async
tipset_by_block_number_or_hash_v2helper- Shares the core estimation logic with V1 via
eth_estimate_gasThis follows the same pattern as
EthCallV2and maintains backward compatibility.
2695-2718: Good refactoring that eliminates duplication.The new
eth_callhelper successfully consolidates the logic fromEthCallandEthCallV2:
- Preserves the special-case handling for
ETHEREUM_ACCOUNT_MANAGER_ACTOR(line 2706)- Properly decodes return data via CBOR when present (line 2714)
- Handles empty return data correctly (lines 2711-2712)
- Error handling looks correct with context messages (line 2709)
Both V1 and V2 endpoints now delegate to this shared implementation, improving maintainability.
|
@akaladarshi @hanabi1224 this PR needs your review |
sudo-shashank
changed the title
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/rpc/methods/eth.rs:
- Around line 1967-2063: The eth_gas_search path incorrectly treats any
Some(...) exit code as SYS_OUT_OF_GAS; change the pattern to compare against a
real bound value (rename _expected_exit_code to expected_exit_code and match
Some(expected_exit_code) or use an if guard) inside eth_gas_search so only true
SYS_OUT_OF_GAS traces trigger gas_search; in gas_search (and its helper
can_succeed) add sanity checks: when doubling reaches BLOCK_GAS_LIMIT, call
can_succeed(..., high) and bail with an error if it still fails (do not proceed
to binary search), and after the binary refinement perform a final
can_succeed(..., high) and return an error if that final high still fails,
ensuring gas_search never returns a non‑working gas limit.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
1844-1893: Sharedeth_estimate_gashelper and V2 wiring look correct; consider lowering log level.
- Delegating both
EthEstimateGasandEthEstimateGasV2to a sharedeth_estimate_gashelper, and usingExtBlockNumberOrHashplustipset_by_block_number_or_hash_v2for V2, is consistent with the existingEthFeeHistory/EthFeeHistoryV2pattern and keeps behavior aligned across versions.- The error-path re‑execution via
apply_messageto surfaceEthErrors::ExecutionRevertedlooks sound and preserves revert reasons.The only concern is verbosity:
log::info!on Lines 1924 and 1926 will fire on every gas estimation, which is in a hot RPC path and may be too noisy in production. Consider downgrading these todebug!(or removing) unless you explicitly want them at info level.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdsrc/rpc/methods/eth.rssrc/rpc/mod.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/mod.rs
- CHANGELOG.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-05T12:54:40.850Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rssrc/rpc/methods/eth.rs
📚 Learning: 2026-01-05T12:56:13.802Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rssrc/rpc/methods/eth.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: Check
- GitHub Check: Coverage
- GitHub Check: All lint checks
🔇 Additional comments (2)
src/rpc/methods/eth.rs (1)
2690-2756: Centralizedeth_callhelper correctly unifies EthCall/EthCallV2 behavior.
- Both
EthCall(V1) andEthCallV2now delegate to a sharedeth_callhelper after resolving the appropriate tipset; this removes duplicated logic and keeps V1/V2 semantics in sync.eth_callusesMessage::try_from(EthCallMessage)plus the sharedapply_messagepath, so revert reasons continue to be surfaced viaEthErrors::execution_reverted.- The ETH account manager special‑case (return empty
EthByteswhenmsg.to()isETHEREUM_ACCOUNT_MANAGER_ACTOR) is preserved, and CBOR decoding ofreturn_dataviadecode_payloadmatches the existing pattern elsewhere.No functional issues spotted here; this refactor improves maintainability.
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2104-2132: EthEstimateGas/EthEstimateGasV2 comparison tests look correct and reuse address safely.
- Switching to
EthAddress::try_from(msg.to)(Line 2104) is clearer and avoids consumingmsg.toviatry_into(), which is safer when reusingmsg.- The added
EthEstimateGasV2test mirrors the existingEthEstimateGastest, including thePolicyOnRejected::Passpolicy and usingExtBlockNumberOrHash::BlockNumber(shared_tipset.epoch().into()), so V1/V2 gas estimation are exercised under identical conditions.- Reusing
eth_to_addr.clone()for V1 and theneth_to_addrfor V2 avoids unnecessary copies while keeping ownership correct.These tests should provide good coverage for the new V2 method and the refactored estimation logic.
Summary of changes
Changes introduced in this pull request:
Filecoin.EthEstimateGasV2 API and test snapshots.Filecoin.EthCallto fix logic duplication.Reference issue to close (if applicable)
Closes #6291
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.