feat(rpc): impl Filecoin.ChainGetTipSetFinalityStatus#6811
Open
hanabi1224 wants to merge 14 commits intomainfrom
Open
feat(rpc): impl Filecoin.ChainGetTipSetFinalityStatus#6811hanabi1224 wants to merge 14 commits intomainfrom
hanabi1224 wants to merge 14 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new RPC method Filecoin.ChainGetTipSetFinalityStatus (with helpers and an in-function cache), exposes ec_finality/calculator publicly, adjusts EC finality calculator constants and defaults, updates tests and API-compare configuration/snapshots, and registers the RPC in the method registry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as "RPC Handler"
participant Cache as "In-Function Cache\n(mutex)"
participant Store as "ChainStore / Blockstore"
participant Calc as "EC Finality Calculator"
Client->>RPC: Filecoin.ChainGetTipSetFinalityStatus(request)
RPC->>Cache: lookup(head)
alt cache hit
Cache-->>RPC: cached(ec_threshold, ec_tipset)
else cache miss
RPC->>Store: fetch parent epochs (fill null rounds)
Store-->>RPC: parent_epochs
RPC->>Calc: find_threshold_depth(parent_epochs)
Calc-->>RPC: ec_threshold
RPC->>Store: tipset_by_height(ec_threshold)
Store-->>RPC: ec_finalized_tipset
RPC->>Cache: store(head => (ec_threshold, ec_finalized_tipset))
end
RPC->>Store: fetch F3 finalized tipset
Store-->>RPC: f3_finalized_tipset
RPC->>RPC: choose finalized_tip_set (compare epochs)
RPC-->>Client: ChainFinalityStatus{ec_threshold, ec_finalized_tipset, f3_finalized_tipset, finalized_tip_set, head}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
force-pushed
the
hm/ChainGetTipSetFinalityStatus
branch
from
50e2ab7 to
00abf12
Compare
hanabi1224
force-pushed
the
hm/ChainGetTipSetFinalityStatus
branch
from
59f3462 to
6db0a02
Compare
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: 2
🧹 Nitpick comments (2)
src/rpc/methods/chain.rs (2)
1236-1245: Redundant.clone()on tipset.The
tsis already owned from theOk(ts)binding, so the.clone()is unnecessary.Proposed fix
let finalized = if depth >= 0 && let Ok(ts) = ctx.chain_index().tipset_by_height( (head.epoch() - depth).max(0), head, ResolveNullTipset::TakeOlder, ) { - Some(ts.clone()) + Some(ts) } else { None };🤖 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 1236 - 1245, The code constructs `finalized` by matching `let Ok(ts) = ctx.chain_index().tipset_by_height(...)` but then calls `ts.clone()` even though `ts` is already moved into the `Ok(ts)` binding; remove the redundant `.clone()` and use `ts` directly when returning `Some(ts)` in the `finalized` assignment (the change affects the block that references `finalized`, the `let Ok(ts)` pattern, `ctx.chain_index().tipset_by_height`, `head`, and `ResolveNullTipset::TakeOlder`).
1261-1267: Consider async blocking for cache miss path.On a cache miss,
get_finality_statuswalks ~905 epochs through the blockstore and runs a binary search with floating-point probability calculations viafind_threshold_depth. While blockstore lookups are cached and the binary search is bounded (O(log 450)), this synchronous work could briefly block the async runtime on every new block when the cache invalidates.Cache hits will be frequent in practice since the cache invalidates only when
heaviest_tipsetchanges, which happens infrequently compared to typical RPC request rates. However, if high RPC concurrency is a concern for your load patterns, consider usingtokio::task::spawn_blockingfor the cache miss path, consistent with similar CPU-bound work likeeth.rs::execution_trace.🤖 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 1261 - 1267, The handler currently calls get_finality_status synchronously inside handle, which can perform heavy CPU/blocking work on cache misses; change handle to offload that work to a blocking thread by invoking tokio::task::spawn_blocking (or equivalent) for get_finality_status and awaiting its JoinHandle so the async runtime isn't blocked on the ~905-epoch walk and binary search; update the handle function to call spawn_blocking(|| Self::get_finality_status(&ctx)) and await the result, mirroring the pattern used for other CPU-bound work like eth.rs::execution_trace.
🤖 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/chain/ec_finality/calculator/mod.rs`:
- Around line 27-29: Update the test comment in the EC finality calculator tests
to reflect the new BISECT_HIGH value (450) instead of 200: locate the comment
above the all-ones chain test in tests.rs (referencing the BISECT/BisectHigh
behavior) and change its text to mention "BisectHigh=450" or "BISECT_HIGH=450"
(e.g., "// All-1s chain is too degraded to achieve 2^-30 within the bisect
search range (BisectHigh=450), so threshold is not found"); no logic changes
needed—the test already references the BISECT_HIGH constant.
In `@src/rpc/methods/chain/types.rs`:
- Around line 14-16: The doc comment block above the type that begins "Describes
how the node is currently determining finality" has a regular comment line ("//
combining probabilistic...") which breaks the doc comment flow; change that line
to a doc comment ("/// combining probabilistic...") so the entire comment block
is contiguous and will appear in generated documentation for the associated
type/enum in types.rs.
---
Nitpick comments:
In `@src/rpc/methods/chain.rs`:
- Around line 1236-1245: The code constructs `finalized` by matching `let Ok(ts)
= ctx.chain_index().tipset_by_height(...)` but then calls `ts.clone()` even
though `ts` is already moved into the `Ok(ts)` binding; remove the redundant
`.clone()` and use `ts` directly when returning `Some(ts)` in the `finalized`
assignment (the change affects the block that references `finalized`, the `let
Ok(ts)` pattern, `ctx.chain_index().tipset_by_height`, `head`, and
`ResolveNullTipset::TakeOlder`).
- Around line 1261-1267: The handler currently calls get_finality_status
synchronously inside handle, which can perform heavy CPU/blocking work on cache
misses; change handle to offload that work to a blocking thread by invoking
tokio::task::spawn_blocking (or equivalent) for get_finality_status and awaiting
its JoinHandle so the async runtime isn't blocked on the ~905-epoch walk and
binary search; update the handle function to call spawn_blocking(||
Self::get_finality_status(&ctx)) and await the result, mirroring the pattern
used for other CPU-bound work like eth.rs::execution_trace.
🪄 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: 3713c0e0-a502-4285-a18b-3d12c21fceb8
⛔ Files ignored due to path filters (1)
src/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
scripts/tests/api_compare/.envscripts/tests/api_compare/filter-list-gatewaysrc/chain/ec_finality/calculator/mod.rssrc/chain/ec_finality/calculator/tests.rssrc/chain/ec_finality/mod.rssrc/chain/mod.rssrc/rpc/methods/chain.rssrc/rpc/methods/chain/types.rssrc/rpc/mod.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 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Member
|
no green checkmark, no review! |
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
Part of #6769
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Chores
Tests
Documentation