chore: bump go-f3 to apply perf improvements#5932
Merged
hanabi1224 merged 2 commits intomainfrom Aug 14, 2025
Merged
Conversation
Contributor
WalkthroughBroad dependency updates in two Go modules (f3-sidecar and interop test app), including libp2p, quic-go, golang.org/x, and protobuf. Some indirect dependencies removed. One Rust change refactors merge_f3_snapshot to inline store creation and explicitly drop it earlier; no public API changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
hanabi1224
commented
Aug 13, 2025
| store.metadata().is_none(), | ||
| "The filecoin snapshot is not in v1 format" | ||
| ); | ||
| drop(store); |
Contributor
Author
There was a problem hiding this comment.
AI pefers scoping here lol : )
642-647: Early resource release is good; prefer scoping over explicit drop and enrich the error message
hanabi1224
requested review from
akaladarshi 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: 0
🧹 Nitpick comments (3)
src/tool/subcommands/archive_cmd.rs (1)
642-647: Early resource release is good; prefer scoping over explicit drop and enrich the error messageExplicitly dropping the store achieves the desired earlier release of mmap/FDs before re-opening the file via CarStream. Two minor improvements:
- Scope the store to a block instead of calling drop for clearer RAII and to avoid accidental future uses.
- Include the detected variant in the error message to aid debugging when a v2 snapshot is passed by mistake.
Apply this diff:
- let store = AnyCar::try_from(filecoin.as_path())?; - anyhow::ensure!( - store.metadata().is_none(), - "The filecoin snapshot is not in v1 format" - ); - drop(store); + { + let store = AnyCar::try_from(filecoin.as_path())?; + anyhow::ensure!( + store.metadata().is_none(), + "Input snapshot must be v1 (.car) without metadata; got variant {}", + store.variant() + ); + } // drop store earlyAdditionally (outside this hunk), consider being explicit when creating the tokio file for the NamedTempFile to avoid any trait inference ambiguity:
// current: let writer = tokio::io::BufWriter::new(tokio::fs::File::create(&temp_output).await?); // suggested: let writer = tokio::io::BufWriter::new(tokio::fs::File::create(temp_output.path()).await?);interop-tests/src/tests/go_app/go.mod (1)
7-7: Dependency bumps LGTM; verify toolchain and transitive compatibility (libp2p/quic-go changes can be subtle)The direct/indirect updates (boxo, libp2p stack, quic-go/webtransport, x/*, protobuf) look aligned. Action items to de-risk:
- Ensure CI images use Go ≥ 1.24.5 to match the module directive.
- libp2p v0.43.0 + DHT v0.34.0 and quic-go v0.54.0 sometimes tweak defaults (ALPN/token handling, congestion control). Please run interop tests against both IPv4/IPv6, NAT traversal, and QUIC transports.
- Run go mod tidy locally and commit go.sum changes to avoid CI drift.
- Keep an eye on webtransport-go v0.9.0 behavior if enabled in tests, as version skew with browsers/clients can surface flaky tests.
If you’d like, I can draft a checklist for targeted interop scenarios (QUIC, DHT bootstrapping, relay/NAT traversal) to run before merging.
Also applies to: 11-13, 53-53, 62-62, 90-90, 95-95, 103-105, 117-121, 123-127
f3-sidecar/go.mod (1)
6-6: go-f3 pseudo-version bump and networking stack upgrades: confirm runtime compatibility and plan to migrate to a tag
- Pinning to go-f3 pseudo-version (v0.8.10-0.20250813...) is fine to pull perf improvements now; consider switching to the next official tag once cut to improve reproducibility.
- The libp2p/quic-go/pion updates mirror the interop test app. Validate end-to-end flows (pubsub, DHT, QUIC) and confirm no regressions in connection manager or resource manager tuning.
- Ensure the CI builder uses Go ≥ 1.24.5. Commit updated go.sum after a tidy.
When a stable go-f3 tag containing the perf PR lands, I can prep a follow-up PR to pin to it and remove the pseudo-version.
Also applies to: 14-15, 18-18, 24-24, 45-45, 56-56, 65-65, 72-72, 82-83, 96-96, 101-101, 110-111, 128-136, 138-138
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
f3-sidecar/go.sumis excluded by!**/*.suminterop-tests/src/tests/go_app/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
f3-sidecar/go.mod(8 hunks)interop-tests/src/tests/go_app/go.mod(4 hunks)src/tool/subcommands/archive_cmd.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#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.
📚 Learning: 2025-08-11T14:00:47.060Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.
Applied to files:
src/tool/subcommands/archive_cmd.rs
🧬 Code Graph Analysis (1)
src/tool/subcommands/archive_cmd.rs (3)
src/db/car/any.rs (3)
try_from(129-131)try_from(136-138)metadata(55-61)src/db/car/forest.rs (2)
try_from(242-244)metadata(127-139)src/db/car/plain.rs (2)
try_from(241-243)metadata(175-187)
⏰ 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). (9)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
akaladarshi
approved these changes
Aug 14, 2025
sudo-shashank
approved these changes
Aug 14, 2025
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:
go-f3to apply performance improvement made by fix: improve performance of ApplyPowerTableDiffs in ImportSnapshotToDatastore filecoin-project/go-f3#1047go get -u && go mod tidy)go-f3and implement F3 data export and import #5886 (comment)Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit