fix: Preserve smart wallets through bulldozer upgrade #7599

gibson042 · 2023-05-04T01:36:18Z

Description

Propagate chain storage data in which keys represent addresses into provisionPool, which exports a reviver facet for the wallet factory (that itself uses the wallet factory when called).

This is somewhat convoluted, but avoids making more Far objects in the bootstrap vat.

Security Considerations

I believe all new interfaces (and in particular the one for indicating an address as revivable) are closely held in the bootstrap vat.

Scaling Considerations

Revivable address are stored in a virtual collection, and I don't foresee any other scaling risks.

Documentation Considerations

This is a specific interpretation of exported chain storage data (every path is assumed to be a wallet address), but I don't know how to document it.

Testing Considerations

This is unit tested, but should also be extended to cover upgrade as realistically as possible. Is there a good starting point?

packages/vats/src/core/startWalletFactory.js

dckc

mostly looks good, but I don't think the result of startGovernedInstance will have a .walletReviver

golang/cosmos/x/vstorage/vstorage.go

dckc · 2023-05-04T02:50:16Z

golang/cosmos/x/vstorage/vstorage_test.go

-		{path: "key1", want: `[["child1",null]]`},
+		{path: "key1", want: `[["child1"]]`},


Why this change? does null act too much like an empty string at some FFI boundary?

It was requested in #7489 (comment) , and I went with null at the time but reconsidered when I got to this PR because it was unnecessary overhead.

packages/vats/src/core/startWalletFactory.js

packages/vats/src/provisionPoolKit.js

packages/vats/src/core/startWalletFactory.js

dckc · 2023-05-04T17:25:11Z

packages/vats/src/core/startWalletFactory.js

-      walletReviver: ppFacets.walletReviver,
+      walletReviver: E(ppFacets.creatorFacet).getWalletReviver(),


nit: seems like more of a fixup than a fix; that is: this doesn't address a defect in master.

packages/vats/src/core/startWalletFactory.js

dckc · 2023-05-04T22:03:52Z

packages/vats/src/core/startWalletFactory.js

-  const oldAddresses = harden(chainStorageEntries.map(entry => entry[0]));
+  // Carry forward wallets with an address already in chain storage.
+  const dataReviver = makeHistoryReviver(chainStorageEntries);
+  const walletStoragePath = await E(walletStorageNode).getPath();


If someone chose to deploy the new contract with a different storage path, that won't change where the old data is. Hard-coding the published.wallet path seems appropriate.

Even though nothing else in this file has awareness of the "published" prefix? I was thinking that if the storage path ever changed, we'd introduce a vat parameter to communicate the old path.

In practice I suppose it doesn't matter.

packages/vats/test/test-provisionPool.js

gibson042 · 2023-05-08T14:26:41Z

@dckc This should be ready for final review!

gibson042 · 2023-05-08T15:54:18Z

packages/internal/src/storage-test-utils.js

+/**
+ * A map corresponding with a total function such that `get(key)`
+ * is assumed to always succeed.
+ *
+ * @template K, V
+ * @typedef {{[k in Exclude<keyof Map<K, V>, 'get'>]: Map<K, V>[k]} & {get: (key: K) => V}} TotalMap
+ */
+/**
+ * @template T
+ * @typedef {T extends Map<infer K, infer V> ? TotalMap<K, V> : never} TotalMapFrom
+ */


I wrote these because I got sick of TypeScript complaining that the result of map.get(key) can be undefined in circumstances where we know it won't. They should probably migrate to internal or endo at some point.

dckc

the re-bootstrap test looks nifty. Here's hoping I can study it some more.

Meanwhile... the contract API change needs a BREAKING CHANGE indicator

dckc · 2023-05-09T06:37:38Z

packages/vats/src/provisionPoolKit.js

-        makeHandler: M.call({
+        makeBridgeHandler: M.call({


This is a breaking contract API change. It needs a suitable conventional commit message.

I think at this point in the release, a breaking change isn't warranted. Please use the existing name and we can choose to change the name in a future issue.

packages/vats/src/provisionPoolKit.js

packages/vats/test/test-provisionPool.js

packages/vats/src/provisionPoolKit.js

dckc · 2023-05-09T06:51:13Z

packages/vats/src/provisionPoolKit.js

-          if (!(bankManager && namesByAddressAdmin && walletFactory)) {
+          if (!bankManager || !namesByAddressAdmin || !walletFactory) {


consider assertAllDefined

Hm... it doesn't have a place to put "must do X before Y"

dckc · 2023-05-10T02:00:11Z

packages/vats/src/provisionPoolKit.js

+        addRevivableAddresses(oldAddresses) {
+          this.state.revivableAddresses.addAll(oldAddresses);


let's log oldAddresses.length here, please

Just console.log('revivableAddresses count', oldAddresses.length)?

dckc

good stuff!

good to go once the ci checks pass

[key] is smaller than [key, value] and arguably clearer, and is already accepted by `set`.

Fixes #7537 Propagate chain storage data in which keys represent addresses into provisionPool, which exports a reviver facet for the wallet factory (that itself *uses* the wallet factory when called). This is somewhat convoluted, but avoids making more Far objects in the bootstrap vat.

refs #7480

…storage path

…n `sequence: true` But for compatibility with preexisting expectations, `sequence` defaults to true.

This will be needed for testing bulldozer scenarios

The wallet factory calls the provision pool wallet reviver reviveWallet method when receiving a bridge message about a currently-unknown address, and reviveWallet checks that the address is revivable but not yet revived to either reject the request or to call back into the factory's provideSmartWallet method with the correct bank and naming hub (which the provision pool has but the wallet factory does not). provideSmartWallet, whether called in this way or more directly, follows wallet construction with a finisher that informs the provision pool about the corresponding address, which it removes from the collection of revivable addresses and then reports back whether or not it was present (i.e., whether or not it recognizes the invocation as completing a revival). provideSmartWallet consumes this result to report whether or not the wallet is actually new. This increases the count of round trips associated with both new wallet provisioning (adding one from the wallet factory to the reviver) and with handling messages about not-yet-known addresses (adding one from the wallet factory to the reviver for what would previously have been a more direct failure, and in the case of successful revival [which previously did not exist at all] one more still from the reviver to the factory). Since wallet creation is relatively rare and a single internal round trip is not a particularly attractive vector for attacks whose own initiation has a message cost, this seems acceptable.

…re-bootstrap

The expected shape is a JSON serialization of { blockHeight: DecimalString, values: Array<string> } in which each element of values can be unmarshalled as CapData.

Smallcaps is strict, so input that looks like a remotable must be unmarshalled to a remotable rather than to a string/undefined/etc. Instead, decode them to "board remotables" that remember the slot ID (presumably a board ID) when we don't otherwise care.

…llbacks

dckc · 2023-05-10T05:03:51Z

packages/inter-protocol/src/proposals/startPSM.js

+    // TODO: validate that `metrics.anchorPoolBalance.value` is
+    // pass-by-copy PureData (e.g., contains no remotables).


you can use Nat or M.nat(). PSM brands have to be fungible.

gibson042 requested a review from dckc May 4, 2023 01:36

gibson042 commented May 4, 2023

View reviewed changes

packages/vats/src/core/startWalletFactory.js Outdated Show resolved Hide resolved

gibson042 changed the title ~~Gibson 7537 wallet migration~~ fix: Preserve smart wallets through bulldozer upgrade May 4, 2023

dckc requested changes May 4, 2023

View reviewed changes

dckc reviewed May 4, 2023

View reviewed changes

packages/vats/src/provisionPoolKit.js Outdated Show resolved Hide resolved

dckc reviewed May 4, 2023

View reviewed changes

packages/vats/src/core/startWalletFactory.js Show resolved Hide resolved

dckc reviewed May 4, 2023

View reviewed changes

packages/vats/src/core/startWalletFactory.js Outdated Show resolved Hide resolved

dckc reviewed May 4, 2023

View reviewed changes

packages/vats/src/core/startWalletFactory.js Outdated Show resolved Hide resolved

dckc reviewed May 4, 2023

View reviewed changes

gibson042 force-pushed the gibson-7537-wallet-migration branch from 892a53c to 5ca0186 Compare May 5, 2023 19:57

dckc reviewed May 6, 2023

View reviewed changes

packages/vats/test/test-provisionPool.js Show resolved Hide resolved

gibson042 requested a review from dckc May 8, 2023 14:26

dckc mentioned this pull request May 8, 2023

4548 boot no precious state #7579

Closed

gibson042 commented May 8, 2023

View reviewed changes

dckc requested changes May 9, 2023

View reviewed changes

gibson042 force-pushed the gibson-7537-wallet-migration branch from d0943f4 to 7dcba79 Compare May 9, 2023 21:32

gibson042 requested a review from dckc May 10, 2023 01:15

gibson042 force-pushed the gibson-7537-wallet-migration branch from 64b1120 to 9e78f54 Compare May 10, 2023 01:51

dckc reviewed May 10, 2023

View reviewed changes

dckc approved these changes May 10, 2023

View reviewed changes

gibson042 added 2 commits May 9, 2023 23:29

refactor(vstorage): Export empty non-terminal entries without a value

6b2f8f4

[key] is smaller than [key, value] and arguably clearer, and is already accepted by `set`.

style: Clean up smartWallet and provisionPool files

3147039

gibson042 force-pushed the gibson-7537-wallet-migration branch from 18741fd to 51c3bf7 Compare May 10, 2023 03:39

gibson042 and others added 5 commits May 10, 2023 00:10

chore: add PSM exportStorageSubtrees to bootstrap swingset config

1e6f1b6

refs #7480

fix(vats): Be mindful of unsettled promises

5a1e322

fix(vats): Extract revivable wallet addresses from the correct chain …

2454d3f

…storage path

fix(internal): Make makeFakeStorageKit value auto-wrapping depend upo…

2751e76

…n `sequence: true` But for compatibility with preexisting expectations, `sequence` defaults to true.

gibson042 added 16 commits May 10, 2023 00:10

feat(internal): makeFakeStorageKit supports "get" and "entries"

6a69aab

This will be needed for testing bulldozer scenarios

chore: Fix lint issues

a708b92

chore(vats): Improve typing

f8abb60

chore(vats): harden in the right place

4b7afc4

test: Update makeSwingsetTestKit etc. to support providing storage

339292d

test(vats): Add coverage for smart wallet survival through bulldozer …

e3a38ad

…re-bootstrap

test(vats): Update for ackWallet

45249fa

chore: Fix bootstrap swingset config exportStorageSubtrees

d6c70a2

fix(vats): Store and read vstorage StreamCell values correctly

de7a92f

The expected shape is a JSON serialization of { blockHeight: DecimalString, values: Array<string> } in which each element of values can be unmarshalled as CapData.

test(vats): Use better variable names and assertion messages

d6bb01d

fix(vats): Revert breaking change to contract facets

94081be

test: Update makeMockChainStorageRoot for proper StreamCell handling

196616c

fix(vats): Preserve receiver for unmarshalFromVstorage fromCapData ca…

21809ba

…llbacks

chore(vats): Add logging for addRevivableAddresses

60c504f

gibson042 force-pushed the gibson-7537-wallet-migration branch from 51c3bf7 to 60c504f Compare May 10, 2023 04:12

gibson042 enabled auto-merge May 10, 2023 04:13

gibson042 added this pull request to the merge queue May 10, 2023

dckc reviewed May 10, 2023

View reviewed changes

Merged via the queue into master with commit 7ca740e May 10, 2023

gibson042 deleted the gibson-7537-wallet-migration branch May 10, 2023 05:30

This was referenced May 10, 2023

[NOT FOR VAULTS] refactor: Update reviveWallet/ackWallet signatures for better promise pipelining #7669

Open

Bulldozer upgrade should purge non-exported vstorage entries #7681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Preserve smart wallets through bulldozer upgrade #7599

fix: Preserve smart wallets through bulldozer upgrade #7599

gibson042 commented May 4, 2023

dckc left a comment

dckc May 4, 2023

gibson042 May 4, 2023

dckc May 4, 2023

dckc May 4, 2023

gibson042 May 5, 2023

dckc May 6, 2023

gibson042 commented May 8, 2023

gibson042 May 8, 2023

dckc May 8, 2023

dckc left a comment

dckc May 9, 2023

ivanlei May 9, 2023 •

edited

Loading

gibson042 May 10, 2023

dckc May 9, 2023

dckc May 10, 2023

gibson042 May 10, 2023

dckc left a comment

dckc May 10, 2023

		{path: "key1", want: `[["child1",null]]`},
		{path: "key1", want: `[["child1"]]`},

		walletReviver: ppFacets.walletReviver,
		walletReviver: E(ppFacets.creatorFacet).getWalletReviver(),

		if (!(bankManager && namesByAddressAdmin && walletFactory)) {
		if (!bankManager \|\| !namesByAddressAdmin \|\| !walletFactory) {

		addRevivableAddresses(oldAddresses) {
		this.state.revivableAddresses.addAll(oldAddresses);

		// TODO: validate that `metrics.anchorPoolBalance.value` is
		// pass-by-copy PureData (e.g., contains no remotables).

fix: Preserve smart wallets through bulldozer upgrade #7599

fix: Preserve smart wallets through bulldozer upgrade #7599

Conversation

gibson042 commented May 4, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

dckc 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

gibson042 commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanlei May 9, 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

dckc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanlei May 9, 2023 •

edited

Loading