Uh oh!
There was an error while loading. Please reload this page.
fix: incorporate EC finality calculator into tipset lookup-by-height cache - #7112
Conversation
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactors EC finality epoch computation into a public RPC helper; caches the EC-derived finalized epoch in ChainStore and uses it (maxed with F3 epoch) for finality checks; adds async tipset lookup and warms daemon RPC caches using the cached EC epoch. ChangesEC finality epoch caching and integration
Sequence Diagram(s)sequenceDiagram
participant Daemon
participant ChainStore
participant ChainGetTipSetFinalityStatus
participant ChainIndex
Daemon->>ChainStore: on new validated tipset -> read ec_calculator_finalized_epoch
ChainStore->>ChainGetTipSetFinalityStatus: (during init/set_heaviest) get_ec_finality_epoch(chain_index, chain_config, head)
ChainGetTipSetFinalityStatus->>ChainIndex: tipset_by_height(ec_finality_epoch, head, ResolveNullTipset::TakeOlder)
ChainIndex-->>ChainGetTipSetFinalityStatus: finalized tipset
ChainGetTipSetFinalityStatus-->>ChainStore: ec_finality_epoch
Daemon->>ChainIndex: tipset_by_height_async(ec_finality_epoch, head, ResolveNullTipset::TakeOlder)
ChainIndex-->>Daemon: finalized tipset or error
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/chain.rs (1)
1171-1191: 💤 Low valueConsider using
ChainEpochas the return type for consistency.The method returns
i64, but conceptually represents aChainEpoch. While they're the same underlying type, usingChainEpochwould be more self-documenting and consistent with how the result is used inChainStore(stored inArc<RwLock<ChainEpoch>>).Suggested change
pub fn get_ec_finality_epoch( chain_index: &ChainIndex, chain_config: &ChainConfig, head: &Tipset, - ) -> i64 {+ ) -> ChainEpoch { let depth = Self::get_ec_finality_threshold_depth_with_cache(chain_index, chain_config, head); Self::get_ec_finality_epoch_by_depth(chain_config, head, depth) } fn get_ec_finality_epoch_by_depth( chain_config: &ChainConfig, head: &Tipset, depth: i64, - ) -> i64 {+ ) -> ChainEpoch {🤖 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/chain.rs` around lines 1171 - 1191, Change the return type of get_ec_finality_epoch and get_ec_finality_epoch_by_depth from i64 to ChainEpoch to reflect the semantic type; update their signatures (pub fn get_ec_finality_epoch(... ) -> ChainEpoch and fn get_ec_finality_epoch_by_depth(... ) -> ChainEpoch) and convert the arithmetic/clamping logic to produce a ChainEpoch (e.g., perform epoch subtraction and clamp to zero using ChainEpoch-compatible operations or conversions), and update any callers or stored places (e.g., where the result is placed into Arc<RwLock<ChainEpoch>>) to accept ChainEpoch instead of i64; touch the symbols get_ec_finality_epoch, get_ec_finality_epoch_by_depth, ChainIndex, ChainConfig, Tipset and chain_config.policy.chain_finality as needed to keep types consistent.
🤖 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 `@src/rpc/methods/chain.rs`:
- Around line 1171-1191: Change the return type of get_ec_finality_epoch and
get_ec_finality_epoch_by_depth from i64 to ChainEpoch to reflect the semantic
type; update their signatures (pub fn get_ec_finality_epoch(... ) -> ChainEpoch
and fn get_ec_finality_epoch_by_depth(... ) -> ChainEpoch) and convert the
arithmetic/clamping logic to produce a ChainEpoch (e.g., perform epoch
subtraction and clamp to zero using ChainEpoch-compatible operations or
conversions), and update any callers or stored places (e.g., where the result is
placed into Arc<RwLock<ChainEpoch>>) to accept ChainEpoch instead of i64; touch
the symbols get_ec_finality_epoch, get_ec_finality_epoch_by_depth, ChainIndex,
ChainConfig, Tipset and chain_config.policy.chain_finality as needed to keep
types consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 73c9f109-5c91-49a7-a50f-ed4e300a65a7
📒 Files selected for processing (2)
src/chain/store/chain_store.rssrc/rpc/methods/chain.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
4060d7c to
8eef8d9CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary of changes
Previously, we don't cache the lookup for epoch (head-900, head] (Only when F3 is down), with EC finality calculator, the finaltiy is ~100 (actually 20 @6052192) instead of 900 on mainnet
Changes introduced in this pull request:
Benchmark of querying tipset @ (head epoch - 500).
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Refactor
Performance