feat(rpc): ChainGetTipset v2 #6231
Conversation
WalkthroughThis PR implements Filecoin.ChainGetTipSet v2 with flexible tipset selection (key/height/tag), adds finality-aware resolution combining F3 and EC sources, new selector types, updates RPC routing to use ApiPaths.path(), threads offline-awareness through the API test harness, and adds related tests and snapshots. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as RPC Server
participant Method as ChainGetTipSetV2
participant Store as ChainStore
participant Finality as FinalityLogic
Client->>RPC: Call ChainGetTipSetV2(selector)
RPC->>Method: handle(selector)
Method->>Method: selector.validate()
alt selector: key
Method->>Store: lookup by key
Store-->>Method: Option<Tipset>
else selector: height (with optional anchor)
Method->>Store: resolve height/anchor (apply null-tipset policy)
Store-->>Method: Option<Tipset>
else selector: tag (Latest/Finalized/Safe)
opt Finalized
Method->>Finality: get_latest_finalized_tipset()
Finality->>Finality: choose F3 vs EC finalized
Finality-->>Method: Option<Tipset>
end
opt Safe
Method->>Finality: get_latest_safe_tipset()
Finality-->>Method: Tipset
end
opt Latest
Method->>Store: get_heaviest_tipset()
Store-->>Method: Tipset
end
end
Method-->>RPC: Ok(Some(tipset)) / Ok(None) / Err
RPC-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
hanabi1224
force-pushed
the
hm/ChainGetTipSetV2
branch
from
100c140 to
43ad5ef
Compare
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
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 (7)
CHANGELOG.md (1)
32-32: Use consistent “Tipset” casing across entries.Elsewhere you use “Tipset” (e.g., ChainGetFinalizedTipset). Recommend:
Filecoin.ChainGetTipsetfor consistency and grepability.src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)
47-49: Also accept method aliases to avoid false “RPC method not found”.Snapshots/tests might record an alias. Consider matching
NAMEorNAME_ALIASbefore theAPI_PATHScheck.Apply this minimal change:
- if test_dump.request.method_name.as_ref() == <$ty>::NAME - && <$ty>::API_PATHS.contains(test_dump.path) + let name = test_dump.request.method_name.as_ref(); + if (name == <$ty>::NAME + || <$ty>::NAME_ALIAS.is_some_and(|a| a == name)) + && <$ty>::API_PATHS.contains(test_dump.path) {src/rpc/reflect/mod.rs (1)
166-172: Clarify path semantics and make formatting reusable.The helper returns a path segment (no leading slash). Document this to avoid misuse and consider a Display impl for reuse.
You can add:
- pub fn path(&self) -> &'static str { + /// Returns the relative path segment (without leading slash), e.g. `rpc/v1`. + pub fn path(&self) -> &'static str { match self { Self::V0 => "rpc/v0", Self::V1 => "rpc/v1", Self::V2 => "rpc/v2", } }Optionally:
impl core::fmt::Display for ApiPaths { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { f.write_str(self.path()) } }src/rpc/methods/chain/types/tests.rs (1)
6-17: Add a non-empty round‑trip case as well.Great baseline. Add one with key/height/tag populated to catch serde attribute regressions.
src/rpc/request.rs (1)
46-52: Make ApiPaths precedence explicit via refactor for code clarity.The current implementation using
iter().max()works correctly because ApiPaths has explicit discriminants (V0=1, V1=2, V2=4), making the Ord comparison deterministic. However, relying on implicit enum Ord behavior is a maintainability risk. The suggested explicit precedence refactor guards against accidental reordering and clarifies intent:- pub fn max_api_path(api_paths: BitFlags<ApiPaths>) -> anyhow::Result<ApiPaths> { - api_paths.iter().max().context("No supported versions") - } + pub fn max_api_path(api_paths: BitFlags<ApiPaths>) -> anyhow::Result<ApiPaths> { + // Highest supported first to make precedence explicit. + if api_paths.contains(ApiPaths::V2) { + Ok(ApiPaths::V2) + } else if api_paths.contains(ApiPaths::V1) { + Ok(ApiPaths::V1) + } else if api_paths.contains(ApiPaths::V0) { + Ok(ApiPaths::V0) + } else { + anyhow::bail!("No supported versions") + } + }Server capability negotiation is already in place:
api_path()operates onself.api_paths(server-supported versions), and the result is passed toget_or_init_client()for correct URL routing.src/tool/subcommands/api_cmd.rs (1)
476-476: Fix grammar in documentation.The documentation reads "The nodes to test against is offline" but should be "The node to test against is offline" (singular) or "The nodes to test against are offline" (plural).
Apply this diff to fix the grammar:
- /// The nodes to test against is offline, the chain is out of sync. + /// The node to test against is offline, the chain is out of sync.src/rpc/methods/chain.rs (1)
1144-1144: Unreachable error case due to validation.Line 1144 returns an error for "no tipset found for selector," but this should be unreachable because
selector.validate()(line 1123) ensures exactly one ofkey,height, ortagis specified. The three preceding conditionals (lines 1125, 1130, 1140) should cover all valid cases.Consider replacing with:
unreachable!("selector validation ensures one criterion is specified")This makes the invariant explicit and will help catch bugs if the validation logic changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)scripts/tests/api_compare/docker-compose.yml(1 hunks)src/chain/store/index.rs(1 hunks)src/rpc/client.rs(3 hunks)src/rpc/methods/chain.rs(3 hunks)src/rpc/methods/chain/types.rs(1 hunks)src/rpc/methods/chain/types/tests.rs(1 hunks)src/rpc/reflect/mod.rs(1 hunks)src/rpc/request.rs(2 hunks)src/rpc/types/mod.rs(1 hunks)src/tool/subcommands/api_cmd.rs(6 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(13 hunks)src/tool/subcommands/api_cmd/generate_test_snapshot.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 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
📚 Learning: 2025-08-25T14:11:10.790Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:86-96
Timestamp: 2025-08-25T14:11:10.790Z
Learning: hanabi1224's RPC snapshot tests include a checksum validation mechanism that compares against HTTP HEAD headers, automatically re-downloading snapshots if the cached version doesn't match, providing protection against corrupted cached files.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
📚 Learning: 2025-08-11T12:42:42.848Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 5836
File: src/tool/subcommands/api_cmd/api_run_tests.rs:186-190
Timestamp: 2025-08-11T12:42:42.848Z
Learning: In the Forest project's test infrastructure (specifically in `src/tool/subcommands/api_cmd/api_run_tests.rs`), the team prefers to use only `ws` (non-secure WebSocket) connections for simplicity in their stateful API test scenarios, as security is not a concern in their testing context.
Applied to files:
src/tool/subcommands/api_cmd.rs
📚 Learning: 2025-08-08T12:10:45.218Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:553-558
Timestamp: 2025-08-08T12:10:45.218Z
Learning: Forest project targets Rust stable >=1.89; features stabilized in 1.88 like let-chains are acceptable in this codebase.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rs
📚 Learning: 2025-08-28T12:52:46.927Z
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 6011
File: src/cli/main.rs:18-25
Timestamp: 2025-08-28T12:52:46.927Z
Learning: In Forest CLI (src/cli/main.rs), the early RPC network check before Cli::parse_from() does not block help/version commands because clap processes these internally before reaching the RPC call. LesnyRumcajs confirmed this implementation works correctly and that RPC call failures are acceptable in this context.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rs
📚 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/rpc/methods/chain.rs
📚 Learning: 2025-08-18T03:09:47.932Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Applied to files:
src/rpc/methods/chain.rs
🧬 Code graph analysis (4)
src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)
src/rpc/reflect/mod.rs (2)
request(290-300)path(166-172)
src/rpc/client.rs (3)
src/rpc/request.rs (1)
max_api_path(46-48)src/rpc/segregation_layer.rs (2)
req(99-99)req(110-110)src/rpc/reflect/mod.rs (1)
path(166-172)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
src/chain/store/index.rs (1)
chain(191-198)src/rpc/reflect/mod.rs (1)
path(166-172)src/rpc/segregation_layer.rs (2)
req(99-99)req(110-110)src/rpc/methods/chain/types.rs (3)
validate(38-56)validate(73-82)validate(109-115)
src/rpc/methods/chain.rs (2)
src/blocks/tipset.rs (5)
tsk(265-269)epoch(306-308)epoch(543-545)key(336-339)key(530-533)src/rpc/methods/state.rs (16)
handle(109-114)handle(132-138)handle(151-157)handle(172-178)handle(196-205)handle(223-231)handle(248-255)handle(272-282)handle(298-305)handle(335-465)handle(484-492)handle(508-538)handle(555-561)handle(577-597)handle(615-624)handle(641-663)
⏰ 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). (7)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
🔇 Additional comments (4)
scripts/tests/api_compare/docker-compose.yml (1)
274-286: LGTM: offline compare wired correctly.
--offlinefits the api-serve flow and keeps Lotus as reference. Timeout env present; no further changes needed.src/chain/store/index.rs (1)
158-163: Message-only change; ensure tests don’t hard‑match full string.If any tests assert on the exact error text, switch them to substring/pattern to avoid brittleness.
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
16-18: Entries are correctly formatted and follow the established snapshot workflow.The three new ChainGetTipset snapshots (lines 16-18) are properly formatted, unique, and follow the naming convention of existing entries. The manifest file is embedded at compile-time and watched by the build system. Per the established workflow, entries should only be added after uploading corresponding
.rpcsnap.json.zstfiles to the DigitalOcean bucket viascripts/tests/upload_rcpsnaps.sh, which updates this manifest atomically. CI automatically validates checksums via HTTP HEAD headers during test execution. Assuming the standard upload process was followed, these entries will be validated when tests run.src/rpc/methods/chain.rs (1)
996-1005: Verify behavior change for ChainGetTipSet v0/v1 with empty tipset key.The updated logic returns an error when
tskisNone(line 1003), but this might differ from the previous behavior. Please verify:
- Whether the previous implementation defaulted to the heaviest tipset when given an empty key
- If this error behavior matches Lotus for v0/v1 API paths
- Whether this constitutes a breaking change for existing clients
The error message matches Lotus per the comment, but it's worth confirming that v0/v1 clients expect this behavior.
There was a problem hiding this comment.
What is this testing? We should trust that Serialize and Deserialize from serde work fine; no need to test our deps, especially on basic usage.
| impl TipsetSelector { | ||
| /// Validate ensures that the TipSetSelector is valid. It checks that only one of | ||
| /// the selection criteria is specified. If no criteria are specified, it returns | ||
| /// nil, indicating that the default selection criteria should be used as defined |
There was a problem hiding this comment.
Is this copied from Lotus? There's no nil, no?
| anyhow::bail!( | ||
| "exactly one tipset selection criteria must be specified, found: {criteria}" | ||
| ) | ||
| } |
| /// Validate ensures that the TipSetSelector is valid. It checks that only one of | ||
| /// the selection criteria is specified. If no criteria are specified, it returns | ||
| /// nil, indicating that the default selection criteria should be used as defined | ||
| /// by the Lotus API Specification. |
| /// the selection criteria is specified. If no criteria are specified, it returns | ||
| /// nil, indicating that the default selection criteria should be used as defined | ||
| /// by the Lotus API Specification. | ||
| pub fn validate(&self) -> anyhow::Result<()> { |
There was a problem hiding this comment.
The logic is a bit wasteful - if there's key AND there's tag then it's just wasteful to to height validation; we could break early with an error.
I guess a match would be a more idiomatic choice for this kind of logic.
| if self.key.0.is_some() && self.tag.is_some() { | ||
| anyhow::bail!("invalid tipset anchor: at most one of key or tag must be specified"); | ||
| } | ||
| // Zero-valued anchor is valid, and considered to be an equivalent to whatever the API decides the default to be. | ||
| Ok(()) |
There was a problem hiding this comment.
either ensure! or perhaps match (it's more idiomatic Rust than conditionals here)
There was a problem hiding this comment.
Let's have some coverage for the logic in this file.
|
|
||
| const HEAD_CHANNEL_CAPACITY: usize = 10; | ||
|
|
||
| // SafeHeightDistance is the distance from the latest tipset, i.e. heaviest, that |
There was a problem hiding this comment.
Again, let's not just copy over Lotus docs.
| // SafeHeightDistance is the distance from the latest tipset, i.e. heaviest, that | |
| // `SAFE_HEIGHT_DISTANCE` is the distance from the latest tipset, i.e. heaviest, that |
| let ts = ctx.chain_index().load_required_tipset(tsk)?; | ||
| Ok(ts) | ||
| } else { | ||
| // Error message here matches lotus |
There was a problem hiding this comment.
Do we need to match the Lotus error? This would actually be a very confusing message for me.
| tag: None, | ||
| },))?) | ||
| .policy_on_rejected(PolicyOnRejected::PassWithQuasiIdenticalError), | ||
| RpcTest::identity(ChainGetTipSetV2::request((TipsetSelector { |
There was a problem hiding this comment.
Won't this test be flaky? What if we're at the epoch boundary, e.g., 29s and Forest's latest is X and Lotus's is X+1?
Summary of changes
Changes introduced in this pull request:
Filecoin.GhainGetTipSetv2Reference issue to close (if applicable)
Closes #6220
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests