fix: contract deployment via forge failing #6118
Conversation
WalkthroughEth RPC methods now use the requested tipset state for code/storage lookups; transaction receipt RPCs return null for missing receipts; a legacy 100M gas-limit check was removed from the message pool. Several test utilities, snapshots, and a shim re-export were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
⏰ 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). (9)
🔇 Additional comments (1)
Comment |
LesnyRumcajs
force-pushed
the
eth-get-code-fix
branch
from
7a773d0 to
bd9f2f0
Compare
LesnyRumcajs
force-pushed
the
eth-get-code-fix
branch
from
bd9f2f0 to
3db5c46
Compare
|
@elmattic as discussed, please drive this to completion. Thank you! |
LesnyRumcajs
force-pushed
the
eth-get-code-fix
branch
3 times, most recently
from
e9f1fa3 to
9968216
Compare
LesnyRumcajs
changed the title
eth_getCode and eth_getStorageAt
LesnyRumcajs
force-pushed
the
eth-get-code-fix
branch
from
716e3ab to
875df02
Compare
LesnyRumcajs
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/methods/eth.rs (2)
2160-2171:EthGetCodenow executes on the tipset state, but actor lookup still uses the parent stateUsing
tipset_stateplusstate_manager.call_on_state(state, &message, Some(ts))ensures theGetBytecodecall itself runs against the post‑tipset state, which is what you want.However, the pre‑check still resolves the target actor against
*ts.parent_state()viaget_required_actor, whileEthGetStorageAthas been updated to look up the actor on the samestateobtained fromtipset_state. That asymmetry likely meanseth_getCodewill still fail (or incorrectly return empty code) for actors that only exist in the tipset’s post‑state, which was precisely the bug this PR is trying to fix.Consider mirroring the
EthGetStorageAtpattern here, e.g.:- let actor = ctx - .state_manager - .get_required_actor(&to_address, *ts.parent_state())?; - // Not a contract. We could try to distinguish between accounts and "native" contracts here, - // but it's not worth it. - if !is_evm_actor(&actor.code) { - return Ok(Default::default()); - } + let (state, _) = ctx.state_manager.tipset_state(&ts).await?; + let Some(actor) = ctx.state_manager.get_actor(&to_address, state)? else { + // No actor at this address in this state ⇒ no code. + return Ok(Default::default()); + }; + // Not a contract. We could try to distinguish between accounts and "native" contracts here, + // but it's not worth it. + if !is_evm_actor(&actor.code) { + return Ok(Default::default()); + } @@ - let (state, _) = ctx.state_manager.tipset_state(&ts).await?; let api_invoc_result = 'invoc: { for ts in ts.chain(ctx.store()) {(adjusting the second
let (state, _)accordingly to avoid duplication).
2797-2858:get_eth_transaction_receiptstill errors on “not indexed” instead of returningNoneThe signature and callers now expect
Result<Option<EthTxReceipt>, ServerError>, and you correctly map lookup errors toOk(None)with a debug log. But whensearch_for_messagereturnsOk(None)(tx not yet on chain / not indexed), the subsequent.context("not indexed")?on theOptionturns this into an error, which bubbles out as a server error instead ofnullin the JSON‑RPC response.You can preserve the new Optional semantics by explicitly handling the
Nonecase before calling.context(...):async fn get_eth_transaction_receipt( @@ - let option = ctx + let option = ctx .state_manager .search_for_message(None, msg_cid, limit, Some(true)) .await .with_context(|| format!("failed to lookup Eth Txn {tx_hash} as {msg_cid}")); - let option = match option { - Ok(opt) => opt, - // Ethereum clients expect an empty response when the message was not found - Err(e) => { - tracing::debug!("could not find transaction receipt for hash {tx_hash}: {e}"); - return Ok(None); - } - }; - - let (tipset, receipt) = option.context("not indexed")?; + let option = match option { + Ok(opt) => opt, + // Ethereum clients expect an empty response when the message was not found + Err(e) => { + tracing::debug!("could not find transaction receipt for hash {tx_hash}: {e}"); + return Ok(None); + } + }; + + // Also treat “not indexed yet” / no result as a `null` receipt. + let Some((tipset, receipt)) = option else { + tracing::debug!( + "transaction hash {tx_hash} (cid {msg_cid}) was not indexed; returning null receipt" + ); + return Ok(None); + }; @@ - let tx_receipt = new_eth_tx_receipt(&ctx, &parent_ts, &tx, &message_lookup.receipt).await?; - - Ok(Some(tx_receipt)) + let tx_receipt = new_eth_tx_receipt(&ctx, &parent_ts, &tx, &message_lookup.receipt).await?; + + Ok(Some(tx_receipt)) }This aligns the implementation with the new
Optionreturn type and the updated snapshots that expectnullfor not‑found receipts.
🧹 Nitpick comments (4)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1986-1996: New EthGetTransactionReceipt not‑found test correctly assertsnullbehaviorUsing
RpcTest::identity(EthGetTransactionReceipt::request((EthHash::from_str("0x…")?,)))ensures both Forest and Lotus now return a successful JSON result withnull(Option::None) instead of an error for a non‑existent transaction hash, which is exactly the intended Lotus‑compatible behavior. The constant hash is well‑formed (32‑byte H256), sofrom_str/serialization will not panic.If you ever want to guarantee this always exercises the “not found” path (rather than just relying on an astronomically low collision probability), you could derive the hash from a different domain (e.g., hash of a fixed string plus a non‑Ethereum domain tag) or document the assumption that it’s intentionally random‑looking.
src/message_pool/msgpool/msg_pool.rs (1)
680-702: Regression test correctly guards against re‑introducing a hard gas_limit capThe
add_helper_message_gas_limit_testusesTestApi, a mocked BLS sig cache, andSignedMessage::mock_bls_signed_messageto exerciseadd_helperdirectly with a largegas_limit(666_666_666), and assertsadd_helperreturnsOk(…). This is a good regression for the removed 100M limit and ensures there is no pool‑specific cap beyond protocol‑level validation. The test is self‑contained and uses only test‑only helpers, so it won’t affect production code.The comment “There are no limits on a single message” is slightly stronger than what actually happens (there are still protocol/network‑level limits enforced elsewhere); consider rephrasing to “There is no additional message‑pool‑specific gas limit” to avoid confusion.
src/message/signed_message.rs (1)
102-108: Test-onlymock_bls_signed_messagehelper is soundUsing
BLS_SIG_LENvia the shim to size the mock BLS signature andnew_uncheckedto bypass verification is appropriate for test scaffolding and avoids magic numbers. Downstream tests can now prefer this helper over ad‑hocvec![0; 96]constructions if desired.src/state_manager/mod.rs (1)
583-678:call_on_statewiring looks correct; consider documenting state expectationsThe refactor to thread an optional
state_cidthroughcall_raw, keepcallbehavior unchanged viaNone, and addcall_on_statethat forces a specific state root is logically consistent and matches the new eth RPC use‑cases.For future callers, it would help to document that
state_cidshould correspond to the “pre‑message” state fortipset(whattipset.parent_state()would otherwise provide); passing a post‑execution state and then re‑applying prior messages could double‑apply effects for non‑SYSTEM actors. Not an issue with current usages, but worth clarifying in docs/comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)src/message/signed_message.rs(1 hunks)src/message_pool/msgpool/msg_pool.rs(1 hunks)src/rpc/methods/eth.rs(8 hunks)src/rpc/methods/miner.rs(1 hunks)src/shim/crypto.rs(1 hunks)src/state_manager/mod.rs(3 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)src/utils/cache/lru.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/shim/crypto.rs
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
🧬 Code graph analysis (4)
src/utils/cache/lru.rs (9)
src/message_pool/msgpool/msg_pool.rs (2)
new(72-77)new(465-583)src/chain/store/chain_store.rs (2)
new(120-147)new(581-585)src/beacon/drand.rs (1)
new(245-263)src/state_migration/common/mod.rs (1)
new(32-39)src/chain/store/index.rs (1)
new(42-46)src/blocks/tipset.rs (2)
new(246-264)new(474-493)src/chain_sync/bad_block_cache.rs (1)
new(26-30)src/db/car/mod.rs (1)
new(72-78)src/state_manager/cache.rs (3)
new(47-49)new(174-187)new(249-251)
src/message/signed_message.rs (2)
src/message/chain_message.rs (1)
message(22-27)src/shim/crypto.rs (1)
new_bls(78-83)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/rpc/reflect/mod.rs (1)
request(288-298)src/rpc/methods/eth.rs (3)
from_str(284-286)from_str(392-403)from_str(452-471)
src/state_manager/mod.rs (1)
src/interpreter/vm.rs (1)
new(175-271)
⏰ 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: Coverage
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (9)
src/utils/cache/lru.rs (1)
152-156: Test-only constructor is well-scoped and consistent with existing API
new_mockedis correctly gated with#[cfg(test)], uses a minimal capacity, and leveragesnew_innerwithout registering metrics, which is appropriate for unit tests. The use ofCow::Borrowed("mocked_cache")andNonZeroUsize::new(1)matches the internal constructor’s expectations and doesn’t affect non-test behavior.CHANGELOG.md (1)
58-62: Changelog entries accurately describe the behavioral changesThe three new “Fixed” bullets correctly capture the EthGetCode/EthGetStorageAt tipset fix, the EthGetTransactionReceipt[Limited] null‑on‑missing behavior, and the removal of the legacy 100M gas limit. Wording, links, and placement are consistent with surrounding entries.
src/tool/subcommands/api_cmd/test_snapshots.txt (3)
64-66: Snapshots for eth_getCode look consistent with new behaviorThe three eth_getCode snapshot entries (latest/concrete/pending) follow existing naming and tagging conventions and align with the new RPC behavior; no issues from the manifest perspective.
69-71: Snapshots for eth_getStorageAt correctly mirror the new state handlingThe added eth_getStorageAt snapshots (latest/concrete/pending) match the established manifest pattern and are appropriate for exercising the updated storage-at semantics.
79-79: Transaction-not-found snapshot matches Optional receipt semanticsThe new
filecoin_ethgettransactionreceipt_... # transaction not foundsnapshot is an appropriate fixture for theOption<EthTxReceipt>behavior and looks correct in this manifest.src/shim/crypto.rs (1)
17-17: Re-exportingBLS_SIG_LENfrom the shim is a good consolidationExposing
BLS_SIG_LENvia the shim is consistent with how other FVM crypto constants are surfaced and simplifies downstream imports without changing behavior.src/rpc/methods/miner.rs (1)
29-29: ImportingBLS_SIG_LENvia the shim keeps FVM details encapsulatedSwitching the
BLS_SIG_LENimport tocrate::shim::cryptocleanly leverages the new re-export and removes a direct dependency on FVM internals from this RPC module.src/rpc/methods/eth.rs (2)
2212-2233:EthGetStorageAtcorrectly uses tipset state andcall_on_stateFor
eth_getStorageAt, fetching(state, _) = tipset_state(&ts), resolving the actor viaget_actor(&to_address, state)?, and driving the VM throughcall_on_state(state, &message, Some(ts))ensures both existence checks and the storage lookup itself see the correct post‑tipset state. This matches the Lotus backport intent and looks good.
2869-2893: RPC types updated toOption<EthTxReceipt>are consistent with helper changesSwitching
EthGetTransactionReceiptandEthGetTransactionReceiptLimitedto returnOption<EthTxReceipt>matches the newget_eth_transaction_receipthelper and the desired eth JSON‑RPC semantics (returningnullwhen a receipt is not found). Once the helper’sNonehandling is fixed, these method signatures are correct.You may want to re-run the eth receipt snapshot tests to confirm that
nullis now returned for missing receipts and that no callers still assume a non‑optional result.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Co-authored-by: hanabi1224 <[email protected]>
Summary of changes
Changes introduced in this pull request:
eth_getCodeandeth_getStorageAtfrom Lotus.eth_getTransactionReceipt[Limited]to return an optional receipt, in lines with what Lotus does.Reference issue to close (if applicable)
Closes #5979
Closes #6214
Other information and links
Change checklist
Summary by CodeRabbit
Bug Fixes
Feature Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.