feat: certstore snapshot export #1032
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
- Coverage 65.58% 65.17% -0.41%
==========================================
Files 80 81 +1
Lines 9703 9747 +44
==========================================
- Hits 6364 6353 -11
- Misses 2847 2900 +53
- Partials 492 494 +2
🚀 New features to boost your workflow:
|
|
|
||
| "github.com/filecoin-project/go-f3/gpbft" | ||
| "github.com/ipfs/go-datastore" | ||
| xerrors "golang.org/x/xerrors" |
There was a problem hiding this comment.
This repo avoids using xerrors in favour of standard Go SDK packages.
| xerrors "golang.org/x/xerrors" | ||
| ) | ||
|
|
||
| var ErrlatestCertificateNil = errors.New("latest certificate is not available") |
There was a problem hiding this comment.
Use consistent CamelCasing (i.e. ErrLatestCertificateNil), or better yet follow the existing naming convention based on which I would name this error ErrUnknownLatestCertificate
| var ErrlatestCertificateNil = errors.New("latest certificate is not available") | |
| var ErrUnknownLatestCertificate = errors.New("latest certificate is not known") |
|
|
||
| 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.
By convention start godoc with the function name. Ditto in other places.
| 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.
Clarify what the from instance is.
| } | ||
|
|
||
| // 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.
Simply call latestInstance, to? There is nothing in the logic that mandates "latest instance".
| InitialPowerTable gpbft.PowerEntries | ||
| } | ||
|
|
||
| func (h *SnapshotHeader) WriteToSnapshot(writer io.Writer) error { |
There was a problem hiding this comment.
I recommend adhering to the existing SDK interfaces like io.WriterTo.
| } | ||
|
|
||
| // 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.
This seems over refactored, i.e. called only from one place.
| return nil | ||
| } | ||
|
|
||
| type MarshalCBOR interface { |
There was a problem hiding this comment.
Why is this type defined? Consider using existing types from cborgen?
| } | ||
|
|
||
| // 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.
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.
| 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.
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.
This is a draft implementation of F3 snapshot export functionality in the format proposed in filecoin-project/FIPs#1169 to collect feedbacks