feat(rpc): integrate ec finality calulator into Eth TipsetResolver#6897
Merged
LesnyRumcajs merged 5 commits intomainfrom Apr 14, 2026
Merged
feat(rpc): integrate ec finality calulator into Eth TipsetResolver#6897LesnyRumcajs merged 5 commits intomainfrom
LesnyRumcajs merged 5 commits intomainfrom
Conversation
Contributor
WalkthroughThis PR integrates EC (Expected Consensus) finality resolution into RPC tipset resolution paths, makes the EC finality helper public, updates tipset resolver and chain RPC usage to call that helper, adjusts module visibility for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EthRPC as Eth RPC
participant TipsetResolver as TipsetResolver
participant Finality as ChainGetTipSetFinalityStatus
participant Chain as Chain / TipsetLookup
Client->>EthRPC: eth_getFinalizedTipset request
EthRPC->>TipsetResolver: resolve tipset
TipsetResolver->>Finality: get_ec_finality_threshold_depth_and_tipset_with_cache(ctx, head)
Finality->>Chain: compute/lookup tipset (may call tipset_by_height)
Chain-->>Finality: tipset (or None)
Finality-->>TipsetResolver: finalized tipset (Option)
TipsetResolver-->>EthRPC: resolved tipset or error
EthRPC-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
hanabi1224
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/chain.rs (1)
1157-1174:⚠️ Potential issue | 🟡 MinorDocument the newly public cache helper.
This helper is now
pub, but its contract is non-obvious: the cache is process-global, keyed byhead, and-1/Nonemean calculator failure rather than “no finalized tipset”. Please add a doc comment before exposing it cross-module.As per coding guidelines, "Document public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/chain.rs` around lines 1157 - 1174, Add a doc comment above the newly pub function get_ec_finality_threshold_depth_and_tipset_with_cache describing its contract: that it uses a process-global cache (the static CACHE) keyed only by the provided head Tipset, that it returns (i64, Option<Tipset>) where a value of -1 for the i64 and None for the Tipset indicate a calculation failure (not “no finalized tipset”), and note that the cached Tipset is shallow-cloned and callers should not assume ownership semantics; include brief notes on thread-safety (parking_lot Mutex) and that ctx is used for underlying calculation.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
40-40: Prefer explicit “expected-consensus (EC) finality” wording in changelog text.This improves terminology consistency and avoids ambiguity for external readers.
✏️ Proposed wording
-- [`#6897`](https://github.com/ChainSafe/forest/pull/6897): Integrated EC finality into Eth RPC methods. +- [`#6897`](https://github.com/ChainSafe/forest/pull/6897): Integrated expected-consensus (EC) finality into Eth RPC methods.Based on learnings: In the Forest codebase,
ecin function names stands for "expected-consensus", and docs/descriptions should use "expected-consensus finality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 40, Update the changelog entry for PR `#6897` to replace the ambiguous "EC finality" wording with the explicit phrase "expected-consensus (EC) finality" (or "expected-consensus finality" depending on style) so the line reading "[`#6897`]: Integrated EC finality into Eth RPC methods." becomes "[`#6897`]: Integrated expected-consensus (EC) finality into Eth RPC methods." to match the codebase convention where "ec" stands for expected-consensus.src/rpc/methods/eth/tipset_resolver.rs (1)
186-198: Move this fallback helper out ofeth::tipset_resolver.This is generic chain-finality logic, but its current location forces
src/rpc/methods/chain.rsto depend on an ETH-specific module. I'd keep it next toChainGetTipSetFinalityStatusor in a small shared finality helper so both RPC paths reuse it without reversing the dependency direction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth/tipset_resolver.rs` around lines 186 - 198, The helper get_fallback_ec_finalized_tipset currently lives in eth::tipset_resolver, creating an unnecessary ETH-specific dependency for chain RPCs; move this function into a shared finality helper module (or next to the ChainGetTipSetFinalityStatus implementation) and update callers to import it from that new module instead of eth::tipset_resolver. Ensure the function signature and generic DB bound remain identical (pub fn get_fallback_ec_finalized_tipset<DB: Blockstore>(cs: &ChainStore<DB>) -> anyhow::Result<Tipset>) so ChainGetTipSetFinalityStatus and any ETH callers simply change their use site import, and remove the eth module dependency from src/rpc/methods/chain.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1073-1076: The method currently returns a fallback tipset from
crate::rpc::eth::tipset_resolver::get_fallback_ec_finalized_tipset(ctx.chain_store())
when ChainGetTipSetFinalityStatus::get_finality_status(ctx) yields None, but the
status response still reports finalized_tip_set = None; update
ChainGetTipSetFinalityStatus::get_finality_status (or its caller) so that when
the calculator returns None you either (a) populate finalized_tip_set with the
fallback tipset returned by get_fallback_ec_finalized_tipset, or (b) add and
populate an explicit fallback/source field in the status response indicating the
returned fallback tipset and its origin; ensure you reference
ChainGetTipSetFinalityStatus::get_finality_status and
crate::rpc::eth::tipset_resolver::get_fallback_ec_finalized_tipset(ctx.chain_store())
when making the change so the two RPCs cannot disagree.
---
Outside diff comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1157-1174: Add a doc comment above the newly pub function
get_ec_finality_threshold_depth_and_tipset_with_cache describing its contract:
that it uses a process-global cache (the static CACHE) keyed only by the
provided head Tipset, that it returns (i64, Option<Tipset>) where a value of -1
for the i64 and None for the Tipset indicate a calculation failure (not “no
finalized tipset”), and note that the cached Tipset is shallow-cloned and
callers should not assume ownership semantics; include brief notes on
thread-safety (parking_lot Mutex) and that ctx is used for underlying
calculation.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 40: Update the changelog entry for PR `#6897` to replace the ambiguous "EC
finality" wording with the explicit phrase "expected-consensus (EC) finality"
(or "expected-consensus finality" depending on style) so the line reading
"[`#6897`]: Integrated EC finality into Eth RPC methods." becomes "[`#6897`]:
Integrated expected-consensus (EC) finality into Eth RPC methods." to match the
codebase convention where "ec" stands for expected-consensus.
In `@src/rpc/methods/eth/tipset_resolver.rs`:
- Around line 186-198: The helper get_fallback_ec_finalized_tipset currently
lives in eth::tipset_resolver, creating an unnecessary ETH-specific dependency
for chain RPCs; move this function into a shared finality helper module (or next
to the ChainGetTipSetFinalityStatus implementation) and update callers to import
it from that new module instead of eth::tipset_resolver. Ensure the function
signature and generic DB bound remain identical (pub fn
get_fallback_ec_finalized_tipset<DB: Blockstore>(cs: &ChainStore<DB>) ->
anyhow::Result<Tipset>) so ChainGetTipSetFinalityStatus and any ETH callers
simply change their use site import, and remove the eth module dependency from
src/rpc/methods/chain.rs.
🪄 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: b2fed9de-89e3-414d-aaf0-f2069cbcd242
📒 Files selected for processing (6)
CHANGELOG.mdsrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/eth/tipset_resolver.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Member
|
no green checkmark! |
LesnyRumcajs
approved these changes
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6769
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Changed
Tests