feat(cosmic-swingset): Add a config property for exporting storage to bootstrap #7489

gibson042 · 2023-04-24T16:34:00Z

Description

When SwingSet configuration includes exportStorageSubtrees, read all chain storage data at or under the specified paths and make the resulting array of entries available to the bootstrap vat at parameter chainStorageEntries.

Security Considerations

None known; launch-chain already had access to the bridge and control over configuration is already a highly privileged capability.

Scaling Considerations

Reading the data can potentially be very slow, but we have accepted that.

Documentation Considerations

I'm not sure where chain configuration and bootstrap vat parameters are officially documented, but would welcome suggestions.

Testing Considerations

There doesn't seem to be any existing test framework that could verify the new behavior. 😢

dckc · 2023-04-24T16:45:44Z

re testing... is it straightforward / cost-effective to mock bridgeOutbound? I suppose this new code would have to be hoisted to its own exported function too. hm.

dckc

the code looks plausible. And nailing down the name chainStorageEntries is handy for coordination with #6645

But I hesitate to approve without a test. I'd like to think about that a bit more.

dckc

I guess I'm willing to take the risk and go without an automated test.

mhofman

This may be out of scope for this PR, but I think we should fix the underlying vstorage "entries" method. cc @michaelfig

Besides that, just some nits in comments.

packages/SwingSet/src/types-external.js

mhofman · 2023-04-24T17:32:00Z

packages/SwingSet/src/types-external.js

+ *   (e.g., `exportStorageSubtrees: ['a.b']` might result in vatParameters including
+ *   `chainStorageEntries: [ ['a.b', ''], ['a.b.c', '"foo"'], ['a.b.c2', '42'] ]`).


It would also include descendant that are not direct children, e.g. ['a.b.c3.d', 'foo'].

mhofman · 2023-04-24T17:34:17Z

packages/cosmic-swingset/src/launch-chain.js

+      const callChainStorage = (method, path) =>
+        bridgeOutbound(BRIDGE_ID.STORAGE, { method, args: [path] });


I imagined we would extend makePrefixedBridgeStorage instead of manually minting chain sends, but I guess there is no need for such abstractions.

packages/SwingSet/src/types-external.js

mhofman

Thanks so much for adding these vstorage unit tests.

I missed it the first time around, but I think we need to add the custom prefix used by chain-storage. I don't want bootstrap to possibly get full access to the root vstorage, which contains the kernel state, etc.

mhofman · 2023-04-25T15:05:28Z

golang/cosmos/x/vstorage/vstorage_test.go

+	keeper.SetStorage(ctx, types.NewStorageEntry("empty", ""))
+	keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("special.nodata"))


nit: I'd prefer if the test was more realistic and no data entries only existed for cases where there are nested entries

Suggested change

keeper.SetStorage(ctx, types.NewStorageEntry("empty", ""))

keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("special.nodata"))

keeper.SetStorage(ctx, types.NewStorageEntry("placeholder.empty", ""))

keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("placeholder"))

Fixed (in both places).

mhofman · 2023-04-25T15:18:24Z

golang/cosmos/x/vstorage/vstorage_test.go

+	keeper.SetStorage(ctx, types.NewStorageEntry("key1", "value1"))
+	keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.grandchild1", "value1grandchild"))
+	keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("key1.child1.grandchild2"))
+	keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.empty-non-terminal.leaf", ""))
+	keeper.SetStorage(ctx, types.NewStorageEntry("key2.child2.grandchild2", "value2grandchild"))
+	keeper.SetStorage(ctx, types.NewStorageEntry("key2.child2.grandchild2a", "value2grandchilda"))


Same nit about representative test data

Suggested change

keeper.SetStorage(ctx, types.NewStorageEntry("key1", "value1"))

keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.grandchild1", "value1grandchild"))

keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("key1.child1.grandchild2"))

keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.empty-non-terminal.leaf", ""))

keeper.SetStorage(ctx, types.NewStorageEntry("key2.child2.grandchild2", "value2grandchild"))

keeper.SetStorage(ctx, types.NewStorageEntry("key2.child2.grandchild2a", "value2grandchilda"))

keeper.SetStorage(ctx, types.NewStorageEntry("key1", "value1"))

keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.grandchild1", "value1grandchild"))

keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("key1.child1"))

keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.empty-non-terminal.leaf", ""))

keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("key2"))

keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("key2.child2"))

keeper.SetStorage(ctx, types.NewStorageEntry("key2.child2.grandchild2", "value2grandchild"))

keeper.SetStorage(ctx, types.NewStorageEntry("key2.child2.grandchild2a", "value2grandchilda"))

mhofman · 2023-04-25T15:23:43Z

packages/cosmic-swingset/src/launch-chain.js

+          const fullPath =
+            !path || path === '.' ? `${nextSegment}` : `${path}.${nextSegment}`;


I believe path === '.' is invalid and I'm tempted to say we should disallow path from ever being empty in the initial "get"

Suggested change

const fullPath =

!path || path === '.' ? `${nextSegment}` : `${path}.${nextSegment}`;

const fullPath =`${path}.${nextSegment}`;

The interface doesn't currently allow enumerating top-level keys AFAICT, and that seems like a problem that could be addressed at some point—but in ignorance of this consumer. This anticipates and accommodates either a DNS-like . or an even more simple empty-string fix.

However, I think you're right about needing to require STORAGE_PATH.CUSTOM so it will be moot.

mhofman · 2023-04-25T15:26:29Z

packages/cosmic-swingset/src/launch-chain.js

+        const value = callChainStorage('get', path);
+        return [path, value];


I believe you need to prepend STORAGE_PATH.CUSTOM

Suggested change

const value = callChainStorage('get', path);

return [path, value];

const fullPath = `${STORAGE_PATH.CUSTOM}.${path}`;

const value = callChainStorage('get', fullPath);

return [fullPath, value];

I've asked for confirmation, but I think we should keep that prefix explicit—albeit with enforcement.

gibson042 · 2023-04-25T16:46:46Z

Okay @mhofman, back to you.

mhofman

LGTM

mhofman · 2023-04-25T17:59:03Z

golang/cosmos/x/vstorage/vstorage_test.go

 func TestEntries(t *testing.T) {
 	kit := makeTestKit()
 	keeper, handler, ctx, cctx := kit.keeper, kit.handler, kit.ctx, kit.cctx

 	keeper.SetStorage(ctx, types.NewStorageEntry("key1", "value1"))
 	keeper.SetStorage(ctx, types.NewStorageEntry("key1.child1.grandchild1", "value1grandchild"))
+	keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("key1.child1.grandchild2"))


Curious why we need to test this case as it's not supposed to ever happen.

Robust handling if that ever changes (or better yet, a failure in this test).

… bootstrap Fixes #7156

gibson042 requested review from mhofman and dckc April 24, 2023 16:34

dckc reviewed Apr 24, 2023

View reviewed changes

dckc approved these changes Apr 24, 2023

View reviewed changes

mhofman reviewed Apr 24, 2023

View reviewed changes

gibson042 requested a review from mhofman April 25, 2023 02:21

mhofman requested changes Apr 25, 2023

View reviewed changes

gibson042 requested a review from mhofman April 25, 2023 16:46

mhofman approved these changes Apr 25, 2023

View reviewed changes

gibson042 added the automerge:no-update (expert!) Automatically merge without updates label Apr 25, 2023

gibson042 added 6 commits April 25, 2023 14:18

feat(cosmic-swingset): Add a config property for exporting storage to…

c065f3b

… bootstrap Fixes #7156

test(vstorage): Add unit tests for vstorageHandler.Receive

6159f3f

refactor: Update vstorage "entries" to emit null values for no-data keys

057d22f

test(vstorage): Improve the fidelity of vstorageHandler.Receive tests

30bf847

chore(cosmic-swingset): Limit chain storage export to non-internal paths

b47281c

test(vstorage): Restore an edge-case vstorageHandler.Receive assertion

4f1589c

gibson042 force-pushed the gibson-7156-bootstrap-chainstorage-export branch from d375995 to 4f1589c Compare April 25, 2023 18:19

mergify bot merged commit 88491d2 into master Apr 25, 2023

mergify bot deleted the gibson-7156-bootstrap-chainstorage-export branch April 25, 2023 18:59

gibson042 mentioned this pull request May 4, 2023

fix: Preserve smart wallets through bulldozer upgrade #7599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cosmic-swingset): Add a config property for exporting storage to bootstrap #7489

feat(cosmic-swingset): Add a config property for exporting storage to bootstrap #7489

gibson042 commented Apr 24, 2023

dckc commented Apr 24, 2023

dckc left a comment

dckc left a comment

mhofman left a comment

mhofman Apr 24, 2023

mhofman Apr 24, 2023

mhofman left a comment

mhofman Apr 25, 2023

gibson042 Apr 25, 2023

mhofman Apr 25, 2023

mhofman Apr 25, 2023

gibson042 Apr 25, 2023

mhofman Apr 25, 2023

gibson042 Apr 25, 2023

gibson042 commented Apr 25, 2023

mhofman left a comment

mhofman Apr 25, 2023

gibson042 Apr 25, 2023

		* (e.g., `exportStorageSubtrees: ['a.b']` might result in vatParameters including
		* `chainStorageEntries: [ ['a.b', ''], ['a.b.c', '"foo"'], ['a.b.c2', '42'] ]`).

		const callChainStorage = (method, path) =>
		bridgeOutbound(BRIDGE_ID.STORAGE, { method, args: [path] });

		keeper.SetStorage(ctx, types.NewStorageEntry("empty", ""))
		keeper.SetStorage(ctx, types.NewStorageEntryWithNoData("special.nodata"))

		const fullPath =
		!path \|\| path === '.' ? `${nextSegment}` : `${path}.${nextSegment}`;

	const fullPath =
	!path \|\| path === '.' ? `${nextSegment}` : `${path}.${nextSegment}`;
	const fullPath =`${path}.${nextSegment}`;

		const value = callChainStorage('get', path);
		return [path, value];

-        const value = callChainStorage('get', path);
-        return [path, value];
+        const fullPath = `${STORAGE_PATH.CUSTOM}.${path}`;
+        const value = callChainStorage('get', fullPath);
+        return [fullPath, value];

feat(cosmic-swingset): Add a config property for exporting storage to bootstrap #7489

feat(cosmic-swingset): Add a config property for exporting storage to bootstrap #7489

Conversation

gibson042 commented Apr 24, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

dckc commented Apr 24, 2023

dckc left a comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman 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

gibson042 commented Apr 25, 2023

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment