fix(eth): allow eth_call and eth_estimateGas from contract and non-existent senders - #7435
fix(eth): allow eth_call and eth_estimateGas from contract and non-existent senders#7435sudo-shashank wants to merge 12 commits into
Conversation
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
![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
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)
2109-2124: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winA zero
msg.gas_limitmakes the growth loop run forever.If
msg.gas_limitis0on entry, thenhigh = 0andlow = 0. The conditionhigh < BLOCK_GAS_LIMITholds.can_succeedat limit0fails. Line 2123 then computes0.saturating_mul(2).min(BLOCK_GAS_LIMIT), which is0.highnever grows and the loop never exits. Each iteration performs a full VM execution throughcall_with_gas, so the request thread hangs and consumes CPU without bound.The new
Skippath makes this reachable.eth_estimate_gas_skip_senderderivesgas_limitfromGasEstimateGasLimit::estimate_gas_limit, which returns-1when the receipt is absent (src/rpc/methods/gas.rsLine 286). At Lines 1966-1967 the value becomes((-1i64 as f64) * overestimation) as u64. A negativef64tou64cast saturates to0in Rust, somsg.set_gas_limit(0)runs and0reachesgas_search.Fix the loop so it always makes progress. Also reject the
-1sentinel ineth_estimate_gas_skip_senderbefore you scale it.🐛 Proposed fix
let mut high = msg.gas_limit; let mut low = msg.gas_limit; + // A zero limit would make the doubling below stall at zero. + if high == 0 { + high = 1; + } +Apply this at Lines 1966-1968 so the sentinel never becomes a gas limit:
+ anyhow::ensure!( + gas_limit >= 0, + "gas estimation returned no receipt for a skipped-validation sender" + ); let gas_limit = ((gas_limit as f64 * ctx.mpool.gas_limit_overestimation()) as u64).min(BLOCK_GAS_LIMIT); msg.set_gas_limit(gas_limit);🤖 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 2109 - 2124, Prevent zero gas limits from stalling gas search and reject the missing-receipt sentinel. In gas_search, ensure the growth loop always advances when high is zero while preserving the BLOCK_GAS_LIMIT cap; in eth_estimate_gas_skip_sender, detect the -1 result from GasEstimateGasLimit::estimate_gas_limit before scaling or calling msg.set_gas_limit, and return the existing appropriate error path instead.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
1988-2015: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider accepting the resolved policy as a parameter to avoid a wasted VM execution.
apply_messagealways attemptsSenderValidation::Enforcefirst, then retries withSkip. Callers that already resolved the policy pay for the discarded first execution.
eth_estimate_gas_skip_senderis one such caller. It resolves the policy throughresolve_sender_validationbefore it runs, then its error arm at Line 1956 callsapply_message, which repeats theEnforceattempt and retries. That is two full VM executions on a request already known to needSkip.The PR objective includes benchmarking against Lotus. Adding a
sender_validation: SenderValidationparameter removes the redundant execution on the known-skip path while keeping the detect-and-retry fallback for callers that passEnforce.♻️ Proposed refactor
async fn apply_message( ctx: &Ctx, tipset: Option<Tipset>, msg: Message, + sender_validation: SenderValidation, ) -> Result<ApiInvocResult, Error> { @@ let result = ctx .state_manager .apply_on_state_with_gas( tipset.clone(), msg.clone(), VMFlush::Skip, - SenderValidation::Enforce, + sender_validation, ) .await; - let needs_skip = match &result { + let needs_skip = sender_validation == SenderValidation::Enforce + && match &result { Err(e) => e .downcast_ref::<crate::state_manager::Error>() .is_some_and(|e| matches!(e, crate::state_manager::Error::SenderValidationFailed)), Ok((invoc_res, _)) => invoc_res .msg_rct .as_ref() .is_some_and(|rct| rct.exit_code() == fvm_shared4::error::ExitCode::SYS_SENDER_INVALID), };Then pass
SenderValidation::Skipat Line 1956 andSenderValidation::Enforceat Line 1893.🤖 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 1988 - 2015, Update apply_message to accept a SenderValidation parameter and use it for the initial apply_on_state_with_gas call, while retaining the existing sender-validation failure detection and retry with Skip when the initial policy is Enforce. Pass SenderValidation::Skip from the resolved-policy error path in eth_estimate_gas_skip_sender and SenderValidation::Enforce from the other apply_message caller.
🤖 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.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 1927-1934: Update resolve_sender_validation and
estimate_call_with_gas so sender validation uses the same tipset as execution:
either pass the execution tipset from data.mpool.current_tipset() into
resolve_sender_validation, or change execution to use the requested tipset.
Preserve the existing actor-based SenderValidation decisions once both paths
share the same state.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1651-1669: Update the EthCall and EthEstimateGas cases in the
ApiPaths loop to use strict success assertions instead of
PolicyOnRejected::PassWithIdenticalError, and set msg calldata to a known
non-reverting contract method rather than relying on empty-calldata fallback
behavior. Keep the existing request construction and API-path coverage intact.
---
Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 2109-2124: Prevent zero gas limits from stalling gas search and
reject the missing-receipt sentinel. In gas_search, ensure the growth loop
always advances when high is zero while preserving the BLOCK_GAS_LIMIT cap; in
eth_estimate_gas_skip_sender, detect the -1 result from
GasEstimateGasLimit::estimate_gas_limit before scaling or calling
msg.set_gas_limit, and return the existing appropriate error path instead.
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 1988-2015: Update apply_message to accept a SenderValidation
parameter and use it for the initial apply_on_state_with_gas call, while
retaining the existing sender-validation failure detection and retry with Skip
when the initial policy is Enforce. Pass SenderValidation::Skip from the
resolved-policy error path in eth_estimate_gas_skip_sender and
SenderValidation::Enforce from the other apply_message caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ee698fe-9220-4a0c-b643-5281dbb964e6
📒 Files selected for processing (5)
src/rpc/methods/eth.rssrc/rpc/methods/gas.rssrc/state_manager/errors.rssrc/state_manager/message_simulation.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/tests/api_compare/.env (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Lotus baseline consistently across all test environments.
All three files now use the mutable
v1.36.2-calibnettag. Docker tags can be retargeted, which can change parity and benchmark results without a source change. Use one verified immutable digest across all three files. (docs.docker.com)
scripts/tests/api_compare/.env#L3-L3: replace the tag with the pinned digest.scripts/tests/bootstrapper/.env#L2-L2: use the same pinned digest.scripts/tests/snapshot_parity/.env#L1-L1: use the same pinned digest.Verify that the selected digest is the intended Lotus baseline for PR
#13724.🤖 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 `@scripts/tests/api_compare/.env` at line 3, Replace the mutable Lotus image tag with the verified immutable digest for the intended PR `#13724` baseline in scripts/tests/api_compare/.env:3-3, scripts/tests/bootstrapper/.env:2-2, and scripts/tests/snapshot_parity/.env:1-1, using exactly the same digest in all three files.
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Around line 44-45: Update the changelog entry’s linked reference from pull
request `#7435` to issue `#7394`, preserving the existing description and
formatting.
---
Nitpick comments:
In `@scripts/tests/api_compare/.env`:
- Line 3: Replace the mutable Lotus image tag with the verified immutable digest
for the intended PR `#13724` baseline in scripts/tests/api_compare/.env:3-3,
scripts/tests/bootstrapper/.env:2-2, and scripts/tests/snapshot_parity/.env:1-1,
using exactly the same digest in all three files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b2dff37-88ec-40c4-8e44-351eec8ca545
📒 Files selected for processing (6)
CHANGELOG.mdscripts/tests/api_compare/.envscripts/tests/bootstrapper/.envscripts/tests/snapshot_parity/.envsrc/rpc/methods/eth.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tool/subcommands/api_cmd/api_compare_tests.rs
- src/rpc/methods/eth.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/devnet/.env (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore dotenv key order.
Move
FOREST_P2P_PORTbeforeFOREST_RPC_PORT.dotenv-linterreportsUnorderedKeyat Line 10.🤖 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 `@scripts/devnet/.env` at line 10, Reorder the environment keys in the dotenv configuration so FOREST_P2P_PORT appears before FOREST_RPC_PORT, preserving their existing values.Source: Linters/SAST tools
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1651-1782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to fallible test setup.
Import
anyhow::Contextand add.context(...)to the fallible address, calldata, initcode, and request-construction operations in these helpers. Include the affected test case or API method in each message.🤖 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/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1651 - 1782, Add anyhow::Context and annotate fallible setup operations in eth_skip_sender_success_tests, eth_skip_sender_insufficient_funds_tests, eth_skip_sender_create_reject_tests, and eth_skip_sender_block_param_tests with contextual errors identifying the relevant test case or API method. Apply context to address, calldata/initcode parsing, and EthCall/EthEstimateGas request construction, including failures propagated through eth_skip_sender_cases.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@scripts/devnet/.env`:
- Line 10: Reorder the environment keys in the dotenv configuration so
FOREST_P2P_PORT appears before FOREST_RPC_PORT, preserving their existing
values.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1651-1782: Add anyhow::Context and annotate fallible setup
operations in eth_skip_sender_success_tests,
eth_skip_sender_insufficient_funds_tests, eth_skip_sender_create_reject_tests,
and eth_skip_sender_block_param_tests with contextual errors identifying the
relevant test case or API method. Apply context to address, calldata/initcode
parsing, and EthCall/EthEstimateGas request construction, including failures
propagated through eth_skip_sender_cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5ed9cfb-df83-4111-8be2-8c41afd0b8df
📒 Files selected for processing (7)
CHANGELOG.mdscripts/devnet/.envsrc/rpc/methods/eth.rssrc/rpc/methods/gas.rssrc/state_manager/message_simulation.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- src/rpc/methods/eth.rs
- src/state_manager/message_simulation.rs
- src/rpc/methods/gas.rs
There was a problem hiding this comment.
- What's the grand total size of all new snapshots?
- What are those snapshots doing? Is each one of them testing something distinct or is there some overlap?
There was a problem hiding this comment.
- Each file size is ~5 Mb so the total size of all the new snapshots is around 120 Mb.
- Yes, distinct test cases but the same set is executed for both
v1andv2api paths. I'll add some tags to each file for better clarity.
There was a problem hiding this comment.
are v1/v2 paths any different in practice for these methods?
There was a problem hiding this comment.
No, i'll keep one set and remove other
2 participants
Summary of changes
Changes introduced in this pull request:
eth_callandeth_estimateGasfrom contract and non-existent addresses via a new skip-sender-validation path, matching Lotus/Geth including tests.Reference issue to close (if applicable)
Closes #7394
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Bug Fixes
eth_callandeth_estimateGassupport for contract senders and accounts that do not yet exist on-chain.Tests
Documentation