feat(x/swingset): include swing-store in genesis #8152

mhofman · 2023-08-06T16:44:17Z

closes: #6527

Description

This PR adds swing-store artifacts to the genesis export, and the ability to start from a genesis that includes a full swing-store export.

The artifacts are exported and imported from a directory alongside the genesis.json file, instead of being inlined like other state.

While the full swing-store state is now included in the genesis state, it doesn't mean it's editable in any kind of capacity. Most swingset state is likely replicated in multiple places inside the swing-store, like transcripts and xsnap heap snapshots.

Security Considerations

As with any genesis start, the validators are in charge of verifying the validity of the genesis file against an expected hash, this PR does not change that.

They do not need to perform manual validation of the artifacts since those are checked for consistency by the swing-store import process, using metadata stored in the genesis file.

Scaling Considerations

The swing-store artifacts are pretty large binary blobs, and as such are not included in the protobuf (genesis state) that ends up JSON serialized. However their size is still significant, and I believe a genesis export of mainnet would result in a total size of about 10GB today. Validators may want to exchange the genesis and artifacts compressed as that should compress down to about 800MB as seen with state-sync snapshots.

Documentation Considerations

A new --export-dir option is required for agd export to specify a path to a directory where the genesis export process will create the genesis.json file and a swing-store directory containing the swing-store export artifacts. On start from genesis, this swing-store directory must be placed in the config folder of the node's home (alongside where the genesis.json usual reside), at least until InitGenesis has completed.

We do not expect this mechanism to be used for normal operations, just as a backstop in case of chain failure.

Testing Considerations

Added a genesis test to the docker upgrade test framework.

Upgrade Considerations

While this mechanism can be used for manual chain upgrades, it is fully transparent to any swingset operation.

mhofman · 2023-08-10T04:35:06Z

The size of the uncompressed artifacts is too large to be included in the genesis.json. We'll look into an alternative, likely a new option to agd export to indicate a directory path used to output everything.

mhofman · 2023-08-19T00:59:51Z

Best reviewed commit-by-commit

michaelfig

A few comments. I'll continue with review later.

michaelfig · 2023-08-19T18:43:26Z

golang/cosmos/cmd/agd/main.go

@@ -13,7 +13,7 @@ import (

 func main() {
 	// We need to delegate to our default app for running the actual chain.
-	daemoncmd.OnStartHook = func(logger log.Logger) {
+	daemoncmd.OnStartHook = func(logger log.Logger, daemonize bool) {


I dislike the use of a boolean to distinguish agd start from agd export, especially since then the names OnStartHook and daemonize are pretty misleading. Can you instead just add daemoncmd.OnExportHook and leave the meaning of daemoncmd.OnStartHook alone?

This main.go could use this same closure for both:

Suggested change

daemoncmd.OnStartHook = func(logger log.Logger, daemonize bool) {

launchVM := func(logger log.Logger) {

...

}

daemoncmd.OnExportHook = launchVM

daemoncmd.OnStartHook = launchVM

michaelfig · 2023-08-19T18:44:07Z

golang/cosmos/cmd/libdaemon/main.go

+		daemoncmd.OnStartHook = func(logger log.Logger, daemonize bool) {
+			if daemonize {
+				// We tried running start, which should never exit, so exit with non-zero
+				// code if we ever stop.
+				exitCode = 99
+			}


As per the above in agd/main.go, please revert this change.

michaelfig · 2023-08-19T18:45:13Z

golang/cosmos/daemon/cmd/root.go

@@ -40,7 +41,7 @@ import (
 type Sender func(needReply bool, str string) (string, error)

 var AppName = "agd"
-var OnStartHook func(log.Logger)
+var OnStartHook func(logger log.Logger, daemonize bool)


Suggested change

var OnStartHook func(logger log.Logger, daemonize bool)

var OnStartHook func(logger log.Logger)

var OnExportHook func(logger log.Logger)

michaelfig · 2023-08-19T18:45:34Z

golang/cosmos/daemon/cmd/root.go

-		OnStartHook(logger)
+		OnStartHook(logger, true)


Please revert.

michaelfig · 2023-08-19T18:47:07Z

golang/cosmos/daemon/cmd/root.go

+	if OnStartHook != nil {
+		OnStartHook(logger, false)
+	}


Suggested change

if OnStartHook != nil {

OnStartHook(logger, false)

}

if OnExportHook != nil {

OnExportHook(logger)

}

michaelfig

LGTM (conditional on addressing comments)! You probably should get a Golang wizard's review before landing, but I didn't see any obvious faux pas.

michaelfig · 2023-08-19T21:56:42Z

golang/cosmos/daemon/cmd/root.go

+		if err != nil {
+			return err
+		}
+		serverCtx.Viper.Set(gaia.FlagSwingStoreExportDir, swingStoreExportPath)


What I've discerned is that agd export --export-dir=foo is a required flag, but --swing-store-export-dir=foo/something can be used to override where the swingstore is exported. Is that true? In either case, could you add this in comments near FlagExportDir and FlagSwingStoreExportDir?

FlagSwingStoreExportDir can only be used to set where to read the swing-store artifacts from for InitGenesis, not for export

Do they need to be different? It seems like the purpose is aligned.

michaelfig · 2023-08-19T22:01:34Z

golang/cosmos/daemon/cmd/root.go

+	// OnExportHook is only set when the VM is not started
+	// We don't need to override the behavior if the process will exec to the VM
+	if OnExportHook == nil {
+		cmd.RunE = extendedRunE
+	}


I don't understand why this special case is necessary. Isn't it more robust to have the same behaviour regardless of whether or not we're starting the VM?

Suggested change

// OnExportHook is only set when the VM is not started

// We don't need to override the behavior if the process will exec to the VM

if OnExportHook == nil {

cmd.RunE = extendedRunE

}

cmd.RunE = extendedRunE

Without it we'd try to create the genesis file twice, and fail the second time around. I initially worked around that by truncating existing files

Ah, okay. That's more subtle than I saw at first. I suggest instead detecting whether sender has been set, and only running this function in that particular case, as I have commented above in this file.

Then, you can omit this condition based on OnExportHook, which is a separate concern from whether the current binary is executing a different one to obtain the controller (sender == nil).

I did a variation on this as I think we should fail early if --export-dir is missing. PTAL

michaelfig · 2023-08-19T23:05:51Z

golang/cosmos/daemon/cmd/root.go

+	// OnExportHook is only set when the VM is not started
+	// We don't need to override the behavior if the process will exec to the VM
+	if OnExportHook == nil {
+		cmd.RunE = extendedRunE
+	}


Ah, okay. That's more subtle than I saw at first. I suggest instead detecting whether sender has been set, and only running this function in that particular case, as I have commented above in this file.

Then, you can omit this condition based on OnExportHook, which is a separate concern from whether the current binary is executing a different one to obtain the controller (sender == nil).

michaelfig · 2023-08-19T23:17:57Z

golang/cosmos/daemon/cmd/root.go

+	for _, command := range rootCmd.Commands() {
+		if command.Name() != "export" {
+			continue
+		}
+
+		extendCosmosExportCommand(command)
+		break
+	}


Suggested change

for _, command := range rootCmd.Commands() {

if command.Name() != "export" {

continue

}

extendCosmosExportCommand(command)

break

}

// Only do the following modifications if we actually have a controller.

// Otherwise, there will be an exec of the VM program, and it will

// handle all of the export logic.

if sender != nil {

for _, command := range rootCmd.Commands() {

if command.Name() != "export" {

continue

}

extendCosmosExportCommand(command)

break

}

}

michaelfig

Thanks, the new code looks good.

JimLarson

Golang code LGTM with some minor requests and suggestions.

JimLarson · 2023-08-21T21:33:19Z

golang/cosmos/daemon/cmd/root.go

+		if command.Name() != "export" {
+			continue
+		}
+
+		extendCosmosExportCommand(command, sender != nil)
+		break


You can save a few lines by inverting the test and using the implicit continue at the end of the loop body.

Suggested change

if command.Name() != "export" {

continue

}

extendCosmosExportCommand(command, sender != nil)

break

if command.Name() == "export" {

extendCosmosExportCommand(command, sender != nil)

break

}

Hmm, sender != nil is a bit opaque. Maybe it should be assigned to a named variable with an explanatory comment?

JimLarson · 2023-08-21T21:55:20Z

golang/cosmos/daemon/cmd/root.go

@@ -28,6 +28,7 @@ import (
 	genutilcli "github.com/cosmos/cosmos-sdk/x/genutil/client/cli"
 	"github.com/spf13/cast"
 	"github.com/spf13/cobra"
+	"github.com/spf13/viper"


I don't think viper provides enough functionality to justify its use. However, I see it's already used in cosmos-sdk, so consistency with existing code trumps my concerns.

Yeah this is just imported so I can cast to the Viper instance that cosmos-sdk creates and that's underlying the appOpts

JimLarson · 2023-08-21T22:47:10Z

golang/cosmos/daemon/cmd/root.go

+)
+
+func extendCosmosExportCommand(cmd *cobra.Command, hasVMController bool) {
+	cmd.Flags().String(FlagExportDir, "", "The directory where to create the genesis export")


Add note in the flag help string explaining difference from the swing store export dir flag, and in that one's help string explaining the difference from this one.

I didn't add a help string for FlagSwingStoreExportDir as from what I understand it could technically be set by config as well, not just by the command line. I have to admit I'm a little fuzzy with how the whole config thing is plumbed in cosmos-sdk.

Actually I realize now that I didn't actually declare it as an official flag anywhere, I just abused the flag mechanism to plumb through an app option. Where do you recommend I declare the FlagSwingStoreExportDir flag?

I second the confusion, but FlagSwingStoreExportDir seems to have strange wiring. At minimum I would look for doc comments.

JimLarson · 2023-08-21T22:48:20Z

golang/cosmos/daemon/cmd/root.go

+	ExportedSwingStoreDirectoryName = "swing-store"
+)
+
+func extendCosmosExportCommand(cmd *cobra.Command, hasVMController bool) {


Document, briefly, what this function should do.

JimLarson · 2023-08-21T23:23:43Z

golang/cosmos/x/swingset/genesis.go

+		keeper.SwingStoreExportProvider{BlockHeight: snapshotHeight, GetExportDataReader: getExportDataReader, ReadNextArtifact: artifactProvider.ReadNextArtifact},
+		keeper.SwingStoreRestoreOptions{ArtifactMode: keeper.SwingStoreArtifactModeReplay, ExportDataMode: keeper.SwingStoreExportDataModeAll},


Suggested change

keeper.SwingStoreExportProvider{BlockHeight: snapshotHeight, GetExportDataReader: getExportDataReader, ReadNextArtifact: artifactProvider.ReadNextArtifact},

keeper.SwingStoreRestoreOptions{ArtifactMode: keeper.SwingStoreArtifactModeReplay, ExportDataMode: keeper.SwingStoreExportDataModeAll},

keeper.SwingStoreExportProvider{

BlockHeight: snapshotHeight,

GetExportDataReader: getExportDataReader,

ReadNextArtifact: artifactProvider.ReadNextArtifact

},

keeper.SwingStoreRestoreOptions{

ArtifactMode: keeper.SwingStoreArtifactModeReplay,

ExportDataMode: keeper.SwingStoreExportDataModeAll

},

gibson042 · 2023-08-22T03:57:15Z

golang/cosmos/cmd/agd/main.go

@@ -29,5 +29,8 @@ func main() {
 		}
 	}

+	daemoncmd.OnStartHook = launchVM
+	daemoncmd.OnExportHook = launchVM


There's some log output in launchVM that could probably be improved for export.

gibson042 · 2023-08-22T04:08:24Z

golang/cosmos/daemon/cmd/root.go

+		if command.Name() != "export" {
+			continue
+		}
+
+		extendCosmosExportCommand(command, sender != nil)
+		break


Hmm, sender != nil is a bit opaque. Maybe it should be assigned to a named variable with an explanatory comment?

golang/cosmos/daemon/cmd/root.go

gibson042 · 2023-08-22T04:19:22Z

golang/cosmos/daemon/cmd/root.go

+)
+
+func extendCosmosExportCommand(cmd *cobra.Command, hasVMController bool) {
+	cmd.Flags().String(FlagExportDir, "", "The directory where to create the genesis export")


I second the confusion, but FlagSwingStoreExportDir seems to have strange wiring. At minimum I would look for doc comments.

gibson042 · 2023-08-22T04:25:28Z

golang/cosmos/daemon/cmd/root.go

+		if err != nil {
+			return err
+		}
+		serverCtx.Viper.Set(gaia.FlagSwingStoreExportDir, swingStoreExportPath)


Do they need to be different? It seems like the purpose is aligned.

gibson042 · 2023-08-22T04:30:56Z

golang/cosmos/daemon/cmd/root.go

+		}
+		serverCtx.Viper.Set(gaia.FlagSwingStoreExportDir, swingStoreExportPath)
+
+		genesisFile, err := os.OpenFile(genesisPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, os.ModePerm)


This will catch a preexisting genesis file, but is there also a potential issue if swingStoreExportPath was already created? I suspect not because it would be empty if this line succeeds, but it's worth confirming.

If the export-manifest.json file exists in the swingStoreExportPath, the JS export process will fail. Directories existing are not a problem.

Do not assume deamonization

…store feat(x/swingset): include swing-store in genesis

closes: #8420 ref: #8152 ## Description This PR adds a new optional flag `--swing-store-export-mode` to the `agd export` command which can be used to specify the nature of swing-store export ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations An optional flag `--swing-store-export-mode` option is now supported for `agd export` to specify the kind of swing store export required: - `operational` (default) option will retain the existing behavior and export the swing-store artifacts using the latest height - `skip` option will not export any swing-store artifact - `debug` option will export all swing-store artifacts, starting from height `0` ### Testing Considerations Tested manually ### Upgrade Considerations None

mhofman linked an issue Aug 6, 2023 that may be closed by this pull request

tools to export/import swing-store to/from genesis block #6527

Closed

mhofman force-pushed the mhofman/8081-fix-state-sync branch from 539d8b0 to fe58cd1 Compare August 6, 2023 20:38

mhofman force-pushed the mhofman/6527-genesis-with-swing-store branch 2 times, most recently from a0baaf3 to 9f17baa Compare August 7, 2023 15:36

mhofman force-pushed the mhofman/8081-fix-state-sync branch 2 times, most recently from 33e99cd to 0e2987e Compare August 10, 2023 04:30

mhofman force-pushed the mhofman/8081-fix-state-sync branch 2 times, most recently from 1e7b20f to 38ac134 Compare August 15, 2023 19:28

Base automatically changed from mhofman/8081-fix-state-sync to master August 15, 2023 20:18

mhofman force-pushed the mhofman/6527-genesis-with-swing-store branch 2 times, most recently from fe86f55 to 98c5236 Compare August 19, 2023 00:44

mhofman marked this pull request as ready for review August 19, 2023 00:59

mhofman requested review from michaelfig and warner August 19, 2023 00:59

mhofman force-pushed the mhofman/6527-genesis-with-swing-store branch from af1a396 to 39dc27a Compare August 19, 2023 15:28

michaelfig reviewed Aug 19, 2023

View reviewed changes

michaelfig approved these changes Aug 19, 2023

View reviewed changes

mhofman requested a review from JimLarson August 19, 2023 22:27

michaelfig reviewed Aug 19, 2023

View reviewed changes

michaelfig reviewed Aug 21, 2023

View reviewed changes

mhofman force-pushed the mhofman/6527-genesis-with-swing-store branch from 82bf011 to 35414a0 Compare August 21, 2023 18:50

JimLarson approved these changes Aug 22, 2023

View reviewed changes

gibson042 reviewed Aug 22, 2023

View reviewed changes

mhofman force-pushed the mhofman/6527-genesis-with-swing-store branch from 127d93b to b034250 Compare August 22, 2023 15:13

mhofman added 4 commits August 22, 2023 08:14

refactor(x/swingset): explicit read/write swing-store export directory

7a4fa83

fix(cosmic-swingset): log level for swing-store export

33c4a51

feat(cosmos): spawn JS on export command

592948d

Do not assume deamonization

feat(cosmos)!: add required export-dir export cmd option

3be2986

mhofman added 3 commits August 22, 2023 08:14

feat(x/swingset): export swing store in genesis

598abf7

feat(x/swingset): import swing store from genesis state

55ba7f6

feat(deployment): add genesis export test

46d3466

mhofman force-pushed the mhofman/6527-genesis-with-swing-store branch from b034250 to 46d3466 Compare August 22, 2023 15:14

mhofman enabled auto-merge August 22, 2023 15:15

mhofman added this pull request to the merge queue Aug 22, 2023

Merged via the queue into master with commit 9048d65 Aug 22, 2023

mhofman deleted the mhofman/6527-genesis-with-swing-store branch August 22, 2023 15:58

mhofman added a commit that referenced this pull request Aug 22, 2023

Merge pull request #8152 from Agoric/mhofman/6527-genesis-with-swing-…

d2975a7

…store feat(x/swingset): include swing-store in genesis

mhofman mentioned this pull request Oct 2, 2023

agd export option for diagnosis #8420

Closed

usmanmani1122 mentioned this pull request Oct 31, 2024

agd export option for diagnosis #10344

Merged

	var OnStartHook func(logger log.Logger, daemonize bool)
	var OnStartHook func(logger log.Logger)
	var OnExportHook func(logger log.Logger)

		keeper.SwingStoreExportProvider{BlockHeight: snapshotHeight, GetExportDataReader: getExportDataReader, ReadNextArtifact: artifactProvider.ReadNextArtifact},
		keeper.SwingStoreRestoreOptions{ArtifactMode: keeper.SwingStoreArtifactModeReplay, ExportDataMode: keeper.SwingStoreExportDataModeAll},

feat(x/swingset): include swing-store in genesis #8152

feat(x/swingset): include swing-store in genesis #8152

Conversation

mhofman commented Aug 6, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

mhofman commented Aug 10, 2023

mhofman commented Aug 19, 2023

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

JimLarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Aug 6, 2023 •

edited

Loading

mhofman Aug 19, 2023 •

edited

Loading