-
Notifications
You must be signed in to change notification settings - Fork 12
feat: certstore snapshot export #1032
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
- Coverage 64.96% 64.56% -0.41%
==========================================
Files 80 81 +1
Lines 9751 9869 +118
==========================================
+ Hits 6335 6372 +37
- Misses 2918 2975 +57
- Partials 498 522 +24
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to pin down high level abstractions more concretely. Left a bunch of suggestions and thank you for pushing this work forward 🍻
certstore/snapshot.go
Outdated
|
||
"github.com/filecoin-project/go-f3/gpbft" | ||
"github.com/ipfs/go-datastore" | ||
xerrors "golang.org/x/xerrors" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repo avoids using xerrors in favour of standard Go SDK packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
certstore/snapshot.go
Outdated
|
||
var ErrlatestCertificateNil = errors.New("latest certificate is not available") | ||
|
||
// Exports an F3 snapshot that includes the finality certificate chain until the current `latestCertificate`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention start godoc with the function name. Ditto in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
certstore/snapshot.go
Outdated
return cs.ExportSnapshot(ctx, cs.latestCertificate.GPBFTInstance, writer) | ||
} | ||
|
||
// Exports an F3 snapshot that includes the finality certificate chain until the specified `lastInstance`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify what the from instance is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
certstore/snapshot.go
Outdated
} | ||
|
||
// Exports an F3 snapshot that includes the finality certificate chain until the specified `lastInstance`. | ||
func (cs *Store) ExportSnapshot(ctx context.Context, lastInstance uint64, writer io.Writer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply call latestInstance
, to
? There is nothing in the logic that mandates "latest instance".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
certstore/snapshot.go
Outdated
InitialPowerTable gpbft.PowerEntries | ||
} | ||
|
||
func (h *SnapshotHeader) WriteToSnapshot(writer io.Writer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend adhering to the existing SDK interfaces like io.WriterTo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
certstore/snapshot.go
Outdated
} | ||
|
||
// Writes CBOR-encoded header or data block with a varint-encoded length prefix | ||
func writeSnapshotCborEncodedBlock(writer io.Writer, block MarshalCBOR) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems over refactored, i.e. called only from one place.
certstore/snapshot.go
Outdated
return nil | ||
} | ||
|
||
type MarshalCBOR interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this type defined? Consider using existing types from cborgen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. (Only found Marshaler
in github.com/filecoin-project/go-state-types/cbor)
certstore/snapshot.go
Outdated
} | ||
|
||
// Exports an F3 snapshot that includes the finality certificate chain until the specified `lastInstance`. | ||
func (cs *Store) ExportSnapshot(ctx context.Context, lastInstance uint64, writer io.Writer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe makes sense to define an Exporter
type that implements io.WriterTo
instead, which takes Store and any other to-from instance params? also see other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
io.WriterTo
does not take context.Context
for cancellation given the export/import could take some time
certstore/snapshot.go
Outdated
var ErrlatestCertificateNil = errors.New("latest certificate is not available") | ||
|
||
// Exports an F3 snapshot that includes the finality certificate chain until the current `latestCertificate`. | ||
func (cs *Store) ExportLatestSnapshot(ctx context.Context, writer io.Writer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term Snapshot
doesn't carry enough of a weight within the context of go-f3. What we really mean by it at this stage is simply: export certificates.
You could make it mean something: define a type Snapshot
that is either produced by the Store
, or takes the Store
, and implements io.WriterTo
, io.ReaderFrom
etc.
Or avoid the term entirely and instead make the store simply an Iterator
of certs and separate IO ops elsewhere.
I would probably go with the first approach but there is not enough implementation in this PR for me to fully wrap my head around how you are thinking of approaching the problem.
So please feel free to ignore the recommendations if they happen to not make sense as the work progresses forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added FRC link to the comment to define Snapshot
. As I understand, a Snapshot
struct is an in-memory representation of the snapshot which is not ideal for implementing a streaming export with a low RAM usage. Implementing io.WriterTo
would not allow cancellation with context.Context
.
fe81652
to
e1507fa
Compare
e1507fa
to
0ebf227
Compare
// ExportSnapshot exports an F3 snapshot that includes the finality certificate chain from the `Store.firstInstance` to the specified `lastInstance`. | ||
// | ||
// Checkout the snapshot format specification at <https://github.com/filecoin-project/FIPs/blob/master/FRCs/frc-0108.md> | ||
func (cs *Store) ExportSnapshot(ctx context.Context, latestInstance uint64, writer io.Writer) (cid.Cid, *SnapshotHeader, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have questions about how to use it, especially with go-car, because IDK if we can provide the header with the CID afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a draft Forest implementation of v2 snapshot export. The flow is
- exporting F3 snapshot to a tmp file
- exporting v2 Filecoin snapshot with F3 snapshot CID and tmp file
- cleanup the tmp file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-car has ReplaceRootsInFile
which will update the header after write as long as you start off with a dummy root CID that's the same length: https://pkg.go.dev/github.com/ipld/go-car/v2#ReplaceRootsInFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Rod
What are the next steps? Is this needing full review comments from @Kubuxu ? |
Per comment in 2025-07-22 implementer working group, it looks like this is ready for maintainer review. @hanabi1224 : will you look at the failing checks? |
This is a draft implementation of F3 snapshot export functionality in the format proposed in filecoin-project/FIPs#1169 to collect feedbacksThis PR implements
Tried to export snapshots on mainnet(@instance 176187) and calibnet(@instance 483660), below are some basic stats