feat(vats): upgradeable board #6903

dckc · 2023-02-01T06:55:13Z

refs: #6553

Description

Use the prepareExoClassKit pattern make board state virtual, durable, and heritable by upgrade successors.

Marshallers are provided ephemerally, keyed by this.

Security Considerations

Interface guards tighten things up a bit.

Scaling Considerations

on getId(val):

      /**
       * Scaling Consideration: since we cannot know when consumers
       * are no longer interested in an ID, a board holds a strong reference
       * to `value` for its entire lifetime. For a well-known board, this
       * is essentially forever.
       */

Documentation Considerations

Since this was my first (non-trivial) work with the virtual, durable, upgrade APIs, I tried to leave cookie crumbs.

Since I touched so much code, I added external API documentation to lib-board.js while I was at it.

Testing Considerations

1 happy-path null upgrade test seemed sufficient to force me to get VDO / upgrade stuff right.

test-lib-board.js came in quite handy for testing interface guards and making sure I didn't break the marshal stuff.
I added 1 test to be more explicit that values have to be passable keys.

packages/vats/src/lib-board.js

dckc · 2023-02-02T15:25:40Z

@warner @gibson042 @FUDCo I'd like to add a virtualization test that shows that heap size stays flat even with many items added to the board. Any suggestions on how to go about it?

packages/smart-wallet/src/marshal-contexts.js

packages/vats/src/vat-board.js

packages/vats/test/test-lib-board.js

packages/vats/test/upgrading/bootstrapVatUpgrade.js

packages/vats/test/upgrading/test-upgrade-vats.js

packages/vats/src/lib-board.js

packages/smart-wallet/src/marshal-contexts.js

packages/vats/src/lib-board.js

packages/vats/test/upgrading/bootstrapVatUpgrade.js

turadg · 2023-02-03T18:15:51Z

packages/wallet/api/src/lib-wallet.js

@@ -1521,7 +1521,6 @@ export function makeWalletRoot({
    return E(board)
      .getValue(boardId)
      .then(value => {
-        // @ts-expect-error type is too specific


I take it this is from the string template thing and not an implicit any

packages/zoe/src/contractSupport/durability.js

turadg

Made the changes I proposed

packages/vats/src/lib-board.js

gibson042 · 2023-02-05T20:58:31Z

packages/vats/src/vat-board.js

+ * @param {import('@agoric/vat-data').Baggage} baggage
+ */
+export function buildRootObject(_vatPowers, _vatParameters, baggage) {
+  const makeBoardKit = prepareBoardKit(baggage);


I think anything accepting a baggage argument to write into should also accept a key rather than hard-coding it.

Suggested change

const makeBoardKit = prepareBoardKit(baggage);

const makeBoardKit = prepareBoardKit(baggage, 'Board');

Disagree, but I see an extensive internal slack discussion, so will continue there. Whatever the outcome, someone should publicly summarize.

packages/vats/src/vat-board.js

gibson042 · 2023-02-05T21:07:18Z

packages/vats/test/test-lib-board.js

@@ -49,6 +49,14 @@ test('makeBoard', async t => {
  t.is(idObj2b, 'tooboard012');
 });

+test('board values must be scalar keys', async t => {
+  const board = makeBoard();
+  const nonKey = harden({ a: 1 });


Can we get more test coverage here? Array, symbol, number, bigint, boolean, etc.?

Seems like the patterns API is reasonably well tested elsewhere.

Hmm, so what exactly is this testing? The "invalid key type" error seems to come from collectionManager.js, which means that the bad input was accepted by the method guard, which suggests to me that ValShape in packages/vats/src/lib-board.js should be M.scalar() rather than M.key().

Digging deeper, I think this hits an issue that was apparently not caught by testing: makeScalarBigMapStore (the function that makes durable maps such as baggage, sometimes through provideDurableMapStore) is built on makeCollection, which defaults to a keyShape of M.scalar(). But if a board's values can be anything matched by M.key(), then it's valToId map should be specified with a corresponding keyShape of M.key() or else it won't actually be able to support non-scalar values (which seems like the testing gap here).

Currently, non-scalar keys are not yet implemented by *MapStore and *SetStore. We make the "Scalar" qualifier explicit in the maker names to emphasize that these implement only a subset of the full MapStore and SetStore semantics.

Non-scalar keys are implemented by CopyMap, CopySet, and CopyBag.

So whether the correct restriction to express is M.key() or M.scalar() depends on if you're trying to detect a violation of the specified semantics, or a violation of the current implementation restrictions. Either way, please put a comment at the relevant place in the code to explain.

Currently, non-scalar keys are not yet implemented by *MapStore and *SetStore.

Hmm, then it seems like a problem for e.g. makeScalarBigMapStore("dummy", { keyShape: M.key() }).init(harden({ label: "not a scalar" }), "val") to succeed (which it currently does, at least in isolation). But more importantly, doesn't that restriction make Board durability (and thus upgradability) an order of magnitude more challenging? How should it store the value-to-id data if not in a durable map?

We make the "Scalar" qualifier explicit in the maker names to emphasize that these implement only a subset of the full MapStore and SetStore semantics.

D'oh! I should have picked up on/remembered that!

makeScalarBigMapStore("dummy", { keyShape: M.key() }).init(harden({ label: "not a scalar" }), "val") succeeds? Really? That is very surprising. Are you sure?

$ node --input-type=module -e ' import "@agoric/swingset-vat/tools/prepare-test-env.js"; import { M } from "@agoric/store"; import { makeScalarBigMapStore } from "@agoric/vat-data"; const bigMap = makeScalarBigMapStore("dummy", { keyShape: M.key() }); const key = harden({ label: "not a scalar" }); bigMap.init(key, "val"); console.log(bigMap.get(key)); ' val

@FUDCo @warner , do you have any idea how this could possibly be working? Are our Big*Stores no longer limited to scalar keys? Did this happen in switching to sqlite?

@erights I don't think we're that lucky; more likely the in-memory implementation set up for testing is just not very restrictive and the keyShape argument for makeScalarBigMapStore et al. is treated as a replacement for the M.scalar() default rather than a refinement of it. And assertKeyPattern(M.key()) passes, although I don't know whether or not that's appropriate because there's some mismatch between checkKeyPattern vs. checkKey (the former rejecting non-scalar specimens like harden({}) but not Matchers like the "match:key" tagged record returned from M.key() because checkKeyPattern internally delegates checking of a tagged record specimen that is a Matcher to the checkKeyPattern method of the corresponding matchHelper from HelpersByMatchTag, which for "match:key" accepts all input).

EDIT: Having explored that, I think something is indeed amiss in checkKeyPattern.

packages/vats/test/upgrading/test-upgrade-vats.js

packages/smart-wallet/src/marshal-contexts.js

packages/vats/src/lib-board.js

turadg · 2023-02-07T20:22:12Z

It failed deployment-test due to GH's Ubuntu brownout which lasts for another couple hours. This doesn't change the behavior of vat-update so I am betting on it not affecting deployment-test and have added a bypass-integration label.

turadg · 2023-02-07T20:25:24Z

@Mergifyio requeue

conflicts with type from `E(board).getId()`, now that getId is declared to return string rather than defaulting to any docs(board): BoardId description

- add docs - exo class kit shaped board w/marshallers - interface guards - factor out makeMarshallers() - re-work makeBoard() based on prepareBoardClass() - ./types.js import not needed - new makeSimpleRemote in boot-relay

dckc mentioned this pull request Feb 1, 2023

all vats that will be deployed in Vaults Release must be upgradable #6553

Closed

dckc self-assigned this Feb 1, 2023

dckc force-pushed the 6553-board-upgrade branch 4 times, most recently from d9ab429 to 2b73dfe Compare February 2, 2023 03:01

dckc changed the title ~~feat(vats): upgradeable board (WIP)~~ feat(vats): upgradeable board Feb 2, 2023

dckc requested review from turadg, michaelfig and gibson042 February 2, 2023 03:12

dckc marked this pull request as ready for review February 2, 2023 03:17

dckc force-pushed the 6553-board-upgrade branch from 2b73dfe to 5ec107a Compare February 2, 2023 03:17

dckc commented Feb 2, 2023

View reviewed changes

packages/vats/src/lib-board.js Outdated Show resolved Hide resolved

turadg reviewed Feb 2, 2023

View reviewed changes

dckc force-pushed the 6553-board-upgrade branch 2 times, most recently from 953ce44 to cab791f Compare February 3, 2023 04:40

dckc added the automerge:rebase Automatically rebase updates, then merge label Feb 3, 2023

dckc requested a review from turadg February 3, 2023 17:38

turadg reviewed Feb 3, 2023

View reviewed changes

dckc removed the automerge:rebase Automatically rebase updates, then merge label Feb 3, 2023

turadg force-pushed the 6553-board-upgrade branch 2 times, most recently from 53579b6 to 9b600df Compare February 3, 2023 19:22

turadg added the automerge:no-update (expert!) Automatically merge without updates label Feb 3, 2023

turadg approved these changes Feb 3, 2023

View reviewed changes

turadg force-pushed the 6553-board-upgrade branch from 9b600df to ffe52ee Compare February 3, 2023 20:43

dckc commented Feb 5, 2023

View reviewed changes

packages/vats/src/lib-board.js Outdated Show resolved Hide resolved

turadg reviewed Feb 5, 2023

View reviewed changes

packages/vats/src/lib-board.js Outdated Show resolved Hide resolved

turadg removed the automerge:no-update (expert!) Automatically merge without updates label Feb 5, 2023

gibson042 reviewed Feb 6, 2023

View reviewed changes

turadg mentioned this pull request Feb 6, 2023

docs: Clarify the upgrade process and requirements for vats and contracts #6916

Merged

erights self-requested a review February 6, 2023 22:09

turadg force-pushed the 6553-board-upgrade branch from 6a15f95 to 7c9355e Compare February 7, 2023 18:26

turadg added automerge:no-update (expert!) Automatically merge without updates bypass:integration Prevent integration tests from running on PR labels Feb 7, 2023

gibson042 mentioned this pull request Feb 7, 2023

assertKeyPattern(M.kind("null")) fails with "null keys are not supported") endojs/endo#1489

Closed

dckc and others added 12 commits February 7, 2023 15:44

feat(zoe): pass key to init in makeEphemeraProvider

32ad4bc

refactor: clarify isDefaultBoardId

504a0a8

test(smart-wallet): provide baggage when creating board vat root

be5ecf8

test(inter-protocol): provide baggage when creating mock vats

9b242cc

build(vats): use @agoric/vat-data for upgrade

5b77444

chore(types): BoardId in lib-board

f01c3f0

conflicts with type from `E(board).getId()`, now that getId is declared to return string rather than defaulting to any docs(board): BoardId description

test(swingtest): makeSimpleRemotable helper

b47aedb

test(swingset): preserve identity across messageVat

9d066f6

feat(vats): upgradeable board

b990271

- add docs - exo class kit shaped board w/marshallers - interface guards - factor out makeMarshallers() - re-work makeBoard() based on prepareBoardClass() - ./types.js import not needed - new makeSimpleRemote in boot-relay

test: move marshall-contexts test

51355f4

docs(board): lookup

293b7a0

docs: prefer bootstrap-relay for upgrade tests

4ec36bf

turadg force-pushed the 6553-board-upgrade branch from 7c9355e to 4ec36bf Compare February 7, 2023 23:44

Merge branch 'master' into 6553-board-upgrade

e81c273

mergify bot merged commit 1323a20 into master Feb 8, 2023

mergify bot deleted the 6553-board-upgrade branch February 8, 2023 02:20

erights mentioned this pull request Feb 8, 2023

Big Stores act as if they accept non-scalars as keys #6953

Closed

erights mentioned this pull request Feb 19, 2023

fix: without assertKeyPattern #7035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vats): upgradeable board #6903

feat(vats): upgradeable board #6903

dckc commented Feb 1, 2023 •

edited

Loading

dckc commented Feb 2, 2023

turadg Feb 3, 2023

turadg left a comment

gibson042 Feb 5, 2023

erights Feb 6, 2023

gibson042 Feb 5, 2023

dckc Feb 6, 2023

gibson042 Feb 6, 2023

gibson042 Feb 6, 2023 •

edited

Loading

erights Feb 6, 2023

gibson042 Feb 6, 2023 •

edited

Loading

erights Feb 6, 2023

gibson042 Feb 7, 2023

erights Feb 7, 2023

gibson042 Feb 7, 2023 •

edited

Loading

turadg commented Feb 7, 2023 •

edited

Loading

turadg commented Feb 7, 2023

	const makeBoardKit = prepareBoardKit(baggage);
	const makeBoardKit = prepareBoardKit(baggage, 'Board');

feat(vats): upgradeable board #6903

feat(vats): upgradeable board #6903

Conversation

dckc commented Feb 1, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

dckc commented Feb 2, 2023

Choose a reason for hiding this comment

turadg 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

gibson042 Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Feb 6, 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

gibson042 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

turadg commented Feb 7, 2023 • edited Loading

turadg commented Feb 7, 2023

dckc commented Feb 1, 2023 •

edited

Loading

gibson042 Feb 6, 2023 •

edited

Loading

gibson042 Feb 6, 2023 •

edited

Loading

gibson042 Feb 7, 2023 •

edited

Loading

turadg commented Feb 7, 2023 •

edited

Loading