Open
Conversation
Contributor
WalkthroughAdded a new CLI subcommand to export tipset-epoch → tipset-key mappings as a ForestCAR, extended MemoryDB CAR export to accept explicit roots, and refactored the existing export to delegate to the new root-parameterized export method. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI User
participant Cmd as ExportTipsetLookupCommand
participant DB as Database
participant Store as ChainStore
participant AMT as AMT (In-Memory)
participant CAR as ForestCAR Exporter
participant File as Output File
CLI->>Cmd: run(chain, db?, from?, to?, skip?, output)
Cmd->>DB: open_db(db_root)
DB-->>Cmd: db instance
Cmd->>Store: ChainStore::new(db, genesis)
Store-->>Cmd: store ready
Cmd->>Store: heaviest_tipset()
Store-->>Cmd: tipset
loop iterate tipsets backward
Cmd->>Store: parent_tipset(current)
Store-->>Cmd: parent tipset
Cmd->>Cmd: filter by epoch (from/to/skip)
alt include
Cmd->>AMT: insert(epoch -> tipset_key)
end
end
Cmd->>AMT: flush()
AMT-->>Cmd: root CID
Cmd->>File: create output file
Cmd->>CAR: export_forest_car_with_roots(rootCID, writer)
CAR->>File: write CAR blocks
File-->>CAR: write complete
CAR-->>Cmd: success
Cmd-->>CLI: print stats & exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/db/memory.rs (2)
28-30: Consider adding a doc comment for the new public method.The
blockstore_lenmethod is public but lacks documentation. Per coding guidelines, public functions should have doc comments.📝 Suggested doc comment
impl MemoryDB { + /// Returns the total number of CIDs stored across both the in-memory + /// and persistent blockchain databases. pub fn blockstore_len(&self) -> usize { self.blockchain_db.read().len() + self.blockchain_persistent_db.read().len() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/memory.rs` around lines 28 - 30, The public method blockstore_len is missing a doc comment; add a concise Rust doc comment (///) above pub fn blockstore_len(&self) -> usize describing what it returns (total number of blocks/entries by summing self.blockchain_db.read().len() and self.blockchain_persistent_db.read().len()) and any semantics (e.g., snapshot consistency or that it sums in-memory and persistent stores), referencing the involved fields blockchain_db and blockchain_persistent_db for clarity.
52-76: Consider adding a doc comment for the new public method.The
export_forest_car_with_rootsmethod is a new public API entry point. Per coding guidelines, public functions should have doc comments explaining its purpose and parameters.📝 Suggested doc comment
+ /// Exports the blockchain data to a ForestCAR file using the provided root CIDs. + /// + /// Unlike [`export_forest_car`], this method allows the caller to specify + /// explicit root CIDs rather than deriving them from the chain head. pub async fn export_forest_car_with_roots<W: tokio::io::AsyncWrite + Unpin>( &self, roots: NonEmpty<Cid>, writer: &mut W, ) -> anyhow::Result<()> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/memory.rs` around lines 52 - 76, Add a doc comment for the public async function export_forest_car_with_roots explaining its purpose (exporting a deterministic CAR stream containing blocks from in-memory DBs), the parameters (roots: NonEmpty<Cid> as the CAR roots, writer: AsyncWrite target for the CAR), what it returns (anyhow::Result<()> on success/failure), and any important behavior (merges blockchain_db and blockchain_persistent_db, sorts blocks to make output deterministic, uses forest::Encoder to compress and write). Place the doc comment immediately above pub async fn export_forest_car_with_roots so it’s visible in generated docs and IDE tooltips.src/dev/subcommands/mod.rs (1)
53-53: Add a doc comment for consistency with other variants.Other variants like
UpdateCheckpointsandArchiveMissinghave doc comments. Consider adding one forExportTipsetLookupto maintain consistency.📝 Suggested doc comment
/// Find missing archival snapshots on the Forest Archive for a given epoch range ArchiveMissing(archive_missing_cmd::ArchiveMissingCommand), + /// Export epoch-to-tipset-key lookup AMT as a ForestCAR file ExportTipsetLookup(export_tipset_lookup_cmd::ExportTipsetLookupCommand),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dev/subcommands/mod.rs` at line 53, Add a doc comment above the enum variant ExportTipsetLookup(export_tipset_lookup_cmd::ExportTipsetLookupCommand) to match the style used by other variants (e.g., UpdateCheckpoints, ArchiveMissing); ensure the comment briefly describes the purpose of ExportTipsetLookup and uses the same comment format and tone as the surrounding variants for consistency.src/dev/subcommands/export_tipset_lookup_cmd.rs (1)
98-103: Add error context to file creation.Per coding guidelines, errors should have context. If file creation fails, the error message should indicate which file couldn't be created.
🔧 Suggested fix
+ use anyhow::Context as _; amt_db .export_forest_car_with_roots( nunny::vec![root], - &mut tokio::fs::File::create(output).await?, + &mut tokio::fs::File::create(&output) + .await + .with_context(|| format!("failed to create output file: {}", output.display()))?, ) .await?;As per coding guidelines: "Add context with
.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dev/subcommands/export_tipset_lookup_cmd.rs` around lines 98 - 103, The file creation call passed to amt_db.export_forest_car_with_roots lacks error context; wrap the tokio::fs::File::create(output).await? result with a Context (e.g., anyhow::Context) so failures include which output file couldn't be created, then pass that file handle into amt_db.export_forest_car_with_roots (i.e., replace the raw await? with .await.context(format!("failed to create output file: {}", output))? and keep the rest of the call unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/db/memory.rs`:
- Around line 28-30: The public method blockstore_len is missing a doc comment;
add a concise Rust doc comment (///) above pub fn blockstore_len(&self) -> usize
describing what it returns (total number of blocks/entries by summing
self.blockchain_db.read().len() and self.blockchain_persistent_db.read().len())
and any semantics (e.g., snapshot consistency or that it sums in-memory and
persistent stores), referencing the involved fields blockchain_db and
blockchain_persistent_db for clarity.
- Around line 52-76: Add a doc comment for the public async function
export_forest_car_with_roots explaining its purpose (exporting a deterministic
CAR stream containing blocks from in-memory DBs), the parameters (roots:
NonEmpty<Cid> as the CAR roots, writer: AsyncWrite target for the CAR), what it
returns (anyhow::Result<()> on success/failure), and any important behavior
(merges blockchain_db and blockchain_persistent_db, sorts blocks to make output
deterministic, uses forest::Encoder to compress and write). Place the doc
comment immediately above pub async fn export_forest_car_with_roots so it’s
visible in generated docs and IDE tooltips.
In `@src/dev/subcommands/export_tipset_lookup_cmd.rs`:
- Around line 98-103: The file creation call passed to
amt_db.export_forest_car_with_roots lacks error context; wrap the
tokio::fs::File::create(output).await? result with a Context (e.g.,
anyhow::Context) so failures include which output file couldn't be created, then
pass that file handle into amt_db.export_forest_car_with_roots (i.e., replace
the raw await? with .await.context(format!("failed to create output file: {}",
output))? and keep the rest of the call unchanged).
In `@src/dev/subcommands/mod.rs`:
- Line 53: Add a doc comment above the enum variant
ExportTipsetLookup(export_tipset_lookup_cmd::ExportTipsetLookupCommand) to match
the style used by other variants (e.g., UpdateCheckpoints, ArchiveMissing);
ensure the comment briefly describes the purpose of ExportTipsetLookup and uses
the same comment format and tone as the surrounding variants for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a756ab0-ab53-47e1-9798-c5fe85a96c78
📒 Files selected for processing (3)
src/db/memory.rssrc/dev/subcommands/export_tipset_lookup_cmd.rssrc/dev/subcommands/mod.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:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/dev/subcommands/export_tipset_lookup_cmd.rs (1)
33-38: Consider validating thatfrom >= toto prevent silent empty exports.Since the chain iteration proceeds from higher epochs (head) down to lower epochs (genesis),
frommust be ≥tofor the range to make sense. If a user mistakenly provides--from 100 --to 200, the loop will break almost immediately (when it reaches an epoch < 200), producing an empty or near-empty AMT with no warning.💡 Suggested validation at the start of run()
let skip_length = skip_length.get() as i64; + if let (Some(from), Some(to)) = (from, to) { + anyhow::ensure!( + from >= to, + "--from ({from}) must be >= --to ({to}) since epochs are processed in descending order" + ); + } let db_root_path = if let Some(db) = db {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dev/subcommands/export_tipset_lookup_cmd.rs` around lines 33 - 38, Validate the epoch range at the start of ExportTipsetLookupCmd::run(): if both self.from and self.to are Some, check that self.from.unwrap() >= self.to.unwrap(); if not, return an error (or print a clear message and exit) so the command fails fast instead of producing a silent empty export — update the run() entry validation to reference the struct fields `from` and `to` (both Option<ChainEpoch>) and perform the comparison before any chain iteration begins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dev/subcommands/export_tipset_lookup_cmd.rs`:
- Around line 33-38: Validate the epoch range at the start of
ExportTipsetLookupCmd::run(): if both self.from and self.to are Some, check that
self.from.unwrap() >= self.to.unwrap(); if not, return an error (or print a
clear message and exit) so the command fails fast instead of producing a silent
empty export — update the run() entry validation to reference the struct fields
`from` and `to` (both Option<ChainEpoch>) and perform the comparison before any
chain iteration begins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef696737-c7b6-49a1-af62-d7c211932976
📒 Files selected for processing (1)
src/dev/subcommands/export_tipset_lookup_cmd.rs
Comment on lines
+36
to
+38
| /// End epoch (inclusive). Defaults to 0 (genesis) | ||
| #[arg(long)] | ||
| to: Option<ChainEpoch>, |
Member
There was a problem hiding this comment.
Why Option if we have a sane default?
Member
There was a problem hiding this comment.
What's the mapping in case of null tipsets?
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
This PR implements an internal tool to export epoch -> tipset key lookup AMT to ForestCAR
skip-length=1skip-length=10Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit