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, adds tests for the V2 endpoint, and updates the changelog. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client
participant Handler as EthEstimateGasV2 Handler
participant Estimator as eth_estimate_gas()
participant Search as eth_gas_search()
participant Probe as gas_search()/can_succeed()
participant Chain as Blockchain (estimate/apply/call)
Client->>Handler: Filecoin.EthEstimateGas (EthCallMessage, blockParam)
Handler->>Handler: resolve tipset (V2 resolver)
Handler->>Estimator: invoke eth_estimate_gas(tx, tipset)
Estimator->>Chain: estimate_message_gas(msg)
alt estimate_message_gas succeeds
Chain-->>Estimator: gas_estimate
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
Estimator-->>Handler: propagate error
end
Search->>Probe: perform exponential then binary search
loop search iterations
Probe->>Chain: call_with_gas(msg, gas_limit)
Chain-->>Probe: execution result (exit code, data)
Probe-->>Probe: adjust bounds
end
Probe-->>Search: found gas limit
Search->>Estimator: apply mpool overestimation factor
Estimator-->>Handler: return EthUint64 result
Handler-->>Client: RPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 18 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 |
Summary of changes
Changes introduced in this pull request:
Filecoin.EthEstimateGasV2 API and test snapshots.Filecoin.EthCallto remove 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.