go/oasis-node/cmd/storage: Add create and import checkpoint cmd#6454
go/oasis-node/cmd/storage: Add create and import checkpoint cmd#6454martintomazic wants to merge 3 commits intomasterfrom
Conversation
ea89ecc to
c5e2f2a
Compare
9761a3c to
0bba8dd
Compare
fe09fe6 to
f833d73
Compare
0bba8dd to
41b49b4
Compare
f833d73 to
b47eb6c
Compare
41b49b4 to
b31dfff
Compare
✅ Deploy Preview for oasisprotocol-oasis-core canceled.
|
b31dfff to
744884b
Compare
| // bootstrapTrustedState synchronizes the cometbft databases after the state sync | ||
| // has been performed offline. | ||
| // | ||
| // It is expected that the block store and state store are empty at the time the | ||
| // function is called. | ||
| // | ||
| // Adapted from https://github.com/oasisprotocol/cometbft/blob/08e22df73d354512fc27bd0c5731b3dcf1f8fef7/node/node.go#L198. | ||
| func bootstrapTrustedState(ctx context.Context, dataDir string, meta bootstrapMeta) error { |
There was a problem hiding this comment.
Might be reasonable to write this function next to BootstrapState in the oasis-cometbft...
Pros: Logically fits there + less things to import.
Cons: You can change things here, if we move it there you need to bump cometbft which is not that practical.
| currentMeta := blockStore.LoadBlockMeta(h + 1) | ||
| if currentMeta == nil { | ||
| return cmtState.State{}, fmt.Errorf("block meta not found at height %d", h+1) | ||
| } |
There was a problem hiding this comment.
This follows upstream where current is h+1. In practice (confirmed) if you do state sync, either managed by cometbft or using the new import command, the blockstore will start from h+1 then, meaning you shadowed start height.
I am inclined to deviate here and use h-1, h and h+1 to work around this.
|
Works! :) The only thing that is impractical is finding corresponding runtime rounds to given consensus height and the fact that bootstrap "eats" one height as described. Finally, one should be very careful with creation/import height/rounds so that you have all relevant light history for the runtime checkpoints you are importing. |
ef92148 to
a41d394
Compare
| if height != 0 { // TODO handle zero value vs not set correctly. | ||
| if err := createConsensusCp(); err != nil { | ||
| return fmt.Errorf("failed to create consensus checkpoint (height: %d): %w", height, err) |
There was a problem hiding this comment.
Maybe use default undefined round (aka max uint64), alternative is cmd.Flags().Changed("height").
Update: Alternative is explicit --consensus flag or possible consensus/runtime sub-commands. No height/round could also mean latest height. -all flag with --height would be also interesting if it would find corresponding runtime rounds for the given height.
There was a problem hiding this comment.
Currently things are not fine, as you can do consensus and runtime checkpoints at the same time. Furthermore, this might be confusing for users, e.g.., do they need to set height, round, both?
There was a problem hiding this comment.
Yes I left it intentional for now. I can easily allow only one at the time. The question is would using sub-commands make things even clearer? Also any preference for what should omitting height/round do?
Observe also that if you want to be able to create checkpoints for multiple heights/versions (same NodeDB) the import command also grows in complexity.
Finally, one should be very careful for which height to specific runtime round combinations you are creating as you can easily get left missing missing runtime light blocks, and therefore stuck runtime state restore.
We could also make import command use consensus/runtime/height/round and import the actual directories created one by one to make it symmetric to create.
Any pref?
a41d394 to
206c70e
Compare
|
Creating checkpoints from the penultimate snapshot, is dominated by the Sapphire checkpoint creation. With 6 chunker threads current projection is 5-7 hours (will update). Import is a matter of minutes. |
7f2aac9 to
817bc76
Compare
817bc76 to
2be35e9
Compare
|
Added unit and e2e tests, fixed empty state corner case and improved code quality. Two minor things left to discuss:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6454 +/- ##
==========================================
- Coverage 64.73% 64.56% -0.18%
==========================================
Files 699 700 +1
Lines 68246 68581 +335
==========================================
+ Hits 44179 44279 +100
- Misses 19060 19183 +123
- Partials 5007 5119 +112 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
peternose
left a comment
There was a problem hiding this comment.
When I import a consensus checkpoint, I get few lines of the following error. Afterwards, blocks execute normally.
{"caller":"grpc.go:194","err":"failed to get consensus status: failed to fetch current block: cometbft: block query failed: height 28800866 must be less than or equal to the current blockchain height 0","level":"error","method":"/oasis-core.NodeController/GetStatus","module":"grpc/internal","msg":"request failed","req_seq":15,"ts":"2026-02-24T13:00:28.934344662Z"}
| } | ||
| earliest := ndb.GetEarliestVersion() | ||
| if version < earliest || version > latest { | ||
| return fmt.Errorf("version not finalized (finalized range: %d-%d)", earliest, latest) |
There was a problem hiding this comment.
| return fmt.Errorf("version not finalized (finalized range: %d-%d)", earliest, latest) | |
| return fmt.Errorf("version not found") |
There was a problem hiding this comment.
Go style discourages opaque errors.
I believe good error trace gives you context to debug the issues just looking at the error trace. E.g. adding a range here, makes the root command return the exact issue to the caller, saving him debug time and calling other commands such as inspect to find a valid range.
| }) | ||
| require.NoError(t, ndb.Finalize([]node.Root{stateRoot})) | ||
|
|
||
| outDir := filepath.Join(t.TempDir(), "checkpoints") |
There was a problem hiding this comment.
You should use a constant for directory name.
There was a problem hiding this comment.
In the target test, with a short scope, where in-lining reads nicer?
| require.NoError(t, os.WriteFile(filepath.Join(outDir, "existing"), []byte{}, 0o600)) | ||
|
|
||
| err = createCheckpoints(ctx, ndb, ns, testVersion, outDir) | ||
| require.Error(t, err, "createCheckpoints should fail for non-empty output directory") |
There was a problem hiding this comment.
| require.Error(t, err, "createCheckpoints should fail for non-empty output directory") | |
| require.ErrorContains(t, err, "output directory is not empty") |
You should check if tested function fails for the right reason.
There was a problem hiding this comment.
Probably you mean this for other tests as well? In general I wanted to avoid it as errors are not part of the public API, but then yes the test can fail for a wrong reason. Will add ErrorContains as you suggest.
| return cmd | ||
| } | ||
|
|
||
| func createCheckpoints(ctx context.Context, ndb api.NodeDB, ns common.Namespace, version uint64, outputDir string) error { |
There was a problem hiding this comment.
Maybe creating a struct checkpointer would be better, as you could create multiple checkpoints with the same struct, e.g.
cp.Create(ctx, 1, "/checkpoints/1")
cp.Create(ctx, 2, "/checkpoints/2")
...
or even without outputDir if accepted in the constructor. The new struct might also be easier to test and could be decoupled from the commands.
There was a problem hiding this comment.
update:
The new struct might also be easier to test and could be decoupled from the commands.
newCheckpointer + cp.create (proposed) = createCheckpoints (current) so this is only a matter of style.
As usual I prefer an explicit function over abstractions until multiple methods share same parameter set.
The question is do we want to allow multiple checkpoint heights/rounds for the same NodeDB.
If you want to make this generic helper I think this would fit inside checkpoint package.
See ( #6467):
// Consider using functional optional arguments to shorten args list.
CreateCheckpoint(ctx context.Context, ndb db.NodeDB, store Store, root node.Root, chunkSize uint64, chunkerThreads uint16) (*Metadata, error)
// Maybe add CreateAllCheckpoints as well and avoid passing root there.| ) | ||
|
|
||
| // CheckpointCreateImport is the checkpoint create/import e2e scenario. | ||
| var CheckpointCreateImport scenario.Scenario = newCheckpointCreateImportImpl() |
There was a problem hiding this comment.
One could argue that this functionality should not be tested with E2E tests as a bug in the functions doesn't affect nodes running the buggy version of the oasis-node.
There was a problem hiding this comment.
I see your point but also observe this is still part of the oasis-node binary.
As pointed in the commit message introducing this I would prefer to:
Furthermore, given that e2e tests are expensive and meant to
test complex scenarios, my suggestions would be to also run
prune, compact, and inspect command on the source node prior
to creating a checkpoint. This way we would "smoke test"
remaining storage commands, and the scenario could be called
storaged_cmds instead
So e2e test for complex scenarios + smoke testing command wiring. Everything else should be tested via unit+integration.
With the new new*Cmd pattern you can also easily test cmd argument validation/combinations. The reason why we need e2e here is 1. smoke testing wiring of the existing storage commands + boostrap cometbft DBs would be very complex to mock and test otherwise.
Update: We could also make e2e tests like this only run prior to release.
| if height != 0 { // TODO handle zero value vs not set correctly. | ||
| if err := createConsensusCp(); err != nil { | ||
| return fmt.Errorf("failed to create consensus checkpoint (height: %d): %w", height, err) |
There was a problem hiding this comment.
Currently things are not fine, as you can do consensus and runtime checkpoints at the same time. Furthermore, this might be confusing for users, e.g.., do they need to set height, round, both?
The test should be ideally hardened by also making sure the target node also syncs up to the tip of the runtime chain and not just consensus. Furthermore, given that e2e tests are expensive and meant to test complex scenarios, my suggestions would be to also run prune, compact, and inspect command on the source node prior to creating a checkpoint. This way we would "smoke test" remaining storage commands, and the scenario could be called storaged_cmds instead.
2be35e9 to
06b5cc4
Compare
Closes #6423
Optional:
Add another command that uses the logic from those two commands, e.g.storage reset --heightto enable snapshot creation with exact start height: