fix: use ethereum block in eth_subscribe newHeads
#6402
Conversation
WalkthroughThe PR fixes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PubSub as Subscription Handler
participant Chain as Chain Methods
participant BlockConv as Block Converter
participant DB as Blockstore
participant Sender as Stream Sender
Client->>PubSub: eth_subscribe(newHeads)
PubSub->>Chain: chain::new_heads(ctx)
Chain->>Chain: create subscriber & spawn handler
Chain-->>PubSub: (Subscriber, JoinHandle)
PubSub-->>Client: subscription_id
Note over Chain: Awaiting Apply events
Chain->>Chain: receive tipset (Apply event)
Chain->>BlockConv: EthBlock::from_filecoin_tipset(ctx, tipset, TxInfo::Full)
BlockConv->>DB: fetch transaction data
DB-->>BlockConv: transaction data
BlockConv->>BlockConv: build Ethereum block
BlockConv-->>Chain: Result<EthBlock>
alt Conversion Success
Chain->>Chain: wrap in ApiHeaders(block)
Chain->>Sender: send headers
Sender-->>Client: eth_subscription notification
else Conversion Error
Chain->>Chain: log error
Chain->>Chain: continue loop
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
LesnyRumcajs
force-pushed
the
fix-eth-subscribe-heads
branch
from
d6912e8 to
6eabb3d
Compare
LesnyRumcajs
requested review from
hanabi1224 and
sudo-shashank
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
🧹 Nitpick comments (2)
src/rpc/methods/eth.rs (2)
557-630: Consider extracting shared transaction processing logic.This new method duplicates significant logic from
block_from_filecoin_tipset(lines 1601-1627), particularly the message iteration andApiEthTxconstruction. While the non-caching behavior is intentional for theeth_subscribeuse case, consider extracting the common transaction-building logic into a helper function to reduce duplication.♻️ Suggested helper extraction
// Helper to build transactions from messages and receipts fn build_eth_transactions<DB: Blockstore>( msgs_and_receipts: &[(ChainMessage, Receipt)], state_tree: &StateTree<DB>, eth_chain_id: EthChainIdType, block_hash: &EthHash, block_number: &EthUint64, ) -> Result<Vec<ApiEthTx>> { let mut transactions = Vec::new(); for (i, (msg, receipt)) in msgs_and_receipts.iter().enumerate() { let ti = EthUint64(i as u64); let smsg = match msg { ChainMessage::Signed(msg) => msg.clone(), ChainMessage::Unsigned(msg) => { let sig = Signature::new_bls(vec![]); SignedMessage::new_unchecked(msg.clone(), sig) } }; let mut tx = new_eth_tx_from_signed_message(&smsg, state_tree, eth_chain_id)?; tx.block_hash = block_hash.clone(); tx.block_number = block_number.clone(); tx.transaction_index = ti; transactions.push(tx); } Ok(transactions) }
1678-1683: Consider implementingFrom<bool>forTxInfoto reduce repetition.The bool-to-enum conversion pattern is repeated in three handlers. A
Fromimplementation would make this more concise.♻️ Optional: Add From implementation
impl From<bool> for TxInfo { fn from(full: bool) -> Self { if full { TxInfo::Full } else { TxInfo::Hash } } }Then call sites become:
let block = block_from_filecoin_tipset(ctx, ts, full_tx_info.into()).await?;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdsrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/pubsub.rssrc/rpc/methods/eth/types.rs
🧰 Additional context used
🧠 Learnings (5)
📓 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: 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/rpc/methods/eth/types.rssrc/rpc/methods/eth/pubsub.rssrc/rpc/methods/chain.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/rpc/methods/eth/types.rssrc/rpc/methods/eth/pubsub.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rs
📚 Learning: 2025-10-16T11:05:13.586Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6166
File: src/auth/mod.rs:127-150
Timestamp: 2025-10-16T11:05:13.586Z
Learning: In Rust 2024, `std::env::set_var` and `std::env::remove_var` are unsafe functions. They require `unsafe` blocks because they are only safe in single-threaded programs (Windows is an exception). When these functions are used in tests, ensure proper serialization with `#[serial]` or similar mechanisms.
Applied to files:
src/rpc/methods/chain.rs
📚 Learning: 2025-08-08T12:11:55.266Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/rpc/methods/chain.rs
🧬 Code graph analysis (3)
src/rpc/methods/eth/pubsub.rs (1)
src/rpc/methods/chain.rs (19)
handle(173-197)handle(240-255)handle(269-284)handle(299-313)handle(328-360)handle(373-381)handle(394-405)handle(418-530)handle(543-567)handle(580-587)handle(600-643)handle(656-680)handle(696-705)handle(720-725)handle(740-778)handle(793-809)chain(463-463)chain(501-501)new_heads(81-113)
src/rpc/methods/chain.rs (1)
src/rpc/methods/eth.rs (5)
eth_logs_with_filter(1537-1561)from_filecoin_tipset(561-630)new(544-556)new(783-788)e(2069-2069)
src/rpc/methods/eth.rs (3)
src/shim/state_tree.rs (1)
new_from_root(168-188)src/shim/crypto.rs (1)
new_bls(78-83)src/message/signed_message.rs (1)
new_unchecked(35-37)
⏰ 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: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: tests-release
- GitHub Check: Coverage
- GitHub Check: rubocop
🔇 Additional comments (9)
CHANGELOG.md (1)
59-60: Changelog entry looks accurate and appropriately scoped.src/rpc/methods/eth/types.rs (1)
424-428: Confirm schema/doc generation doesn’t requireJsonSchemafor subscription payload types. If subscriptions are included in OpenRPC/schemars output,ApiHeadersmay need#[derive(JsonSchema)](and any necessaryschemars(...)attrs) to avoid doc/test breakage.src/rpc/methods/eth/pubsub.rs (1)
121-130: Call-site update matches the newchain::new_heads(Ctx<DB>)signature.src/rpc/methods/chain.rs (1)
81-113: Double-checkTxInfo::FullfornewHeads(payload size + performance). If consumers only expect header-ish fields or tx hashes, consider emittingTxInfo::Hashinstead to reduce per-head execution/serialization cost.Proposed adjustment (if compatible with Lotus/tooling expectations)
- match EthBlock::from_filecoin_tipset(data.clone(), Arc::new(ts), TxInfo::Full) + match EthBlock::from_filecoin_tipset(data.clone(), Arc::new(ts), TxInfo::Hash) .await {src/rpc/methods/eth.rs (5)
534-541: LGTM!Clean enum definition with appropriate derives and clear documentation. The
Copytrait is correctly derived for this simple two-variant enum.
1578-1582: LGTM!Good refactor from boolean to enum parameter - improves code readability at call sites.
1648-1653: LGTM!Correct transformation from boolean condition to enum comparison.
1708-1713: LGTM!Consistent with the conversion pattern used in
EthGetBlockByHash.
1735-1740: LGTM!Consistent implementation across all V2 handlers.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| let tx_info = if full_tx_info { | ||
| TxInfo::Full | ||
| } else { | ||
| TxInfo::Hash | ||
| }; |
There was a problem hiding this comment.
we can add an impl From<bool> for TxInfo to avoid doing this multiple times
There was a problem hiding this comment.
I'd prefer to be more verbose. That said, I noticed there's some existing code I didn't know of that could be reused.
Summary of changes
Changes introduced in this pull request:
newHeadsresponse to Ethereum block (it was an array of tipset headers (???)).filecoin-subwaydoesn't complain anymore.Reference issue to close (if applicable)
Closes #6400
Other information and links
Change checklist
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.