refactor: Switch default archive format to endoZipBase64 #3273

kriskowal · 2021-06-09T05:51:07Z

packages/zoe/test/swingsetTests/zoe/vat-dave.js

The import and archive functions now accepta `dev` flag that indicates that the `devDependencies` of the entry module's package should be included in the compartment graph. This must be passed explicitly and is off by default since that will be the norm for applications. This is the bug of the day blocking Agoric/agoric-sdk#3273 In furtherance of Agoric/agoric-sdk#2684

packages/SwingSet/test/test-xsnap-store.js

kriskowal · 2021-09-20T22:50:56Z

packages/SwingSet/src/kernel/vatManager/supervisor-subprocess-xsnap.js

@@ -286,6 +286,9 @@ function makeWorker(port) {
      assert,
      // bootstrap provides HandledPromise
      HandledPromise: globalThis.HandledPromise,
+      TextEncoder,
+      TextDecoder,
+      Base64: globalThis.Base64, // Present only in XSnap


Absent from this list is URL, which is never present inside XSnap.

kriskowal · 2021-09-20T22:51:46Z

packages/bundle-source/src/index.js

@@ -15,7 +15,7 @@ import { SourceMapConsumer } from 'source-map';

 import './types.js';

-const DEFAULT_MODULE_FORMAT = 'nestedEvaluate';
+const DEFAULT_MODULE_FORMAT = 'endoZipBase64';


This is the biggest/riskiest line in the diff. cc @warner

kriskowal · 2021-09-20T22:52:45Z

packages/transform-metering/src/meter.js

+/*
+ * Getting the properties of a SES-shim module exports namespace object throws
+ * an error if inspected before the module initializes.
+ * Allowing this case is necessary for the compartment mapper to be hosted by
+ * the compartment mapper under metering.
+ */
+const maybeGetOwnPropertyDescriptors = object => {
+  try {
+    return getOwnPropertyDescriptors(object);
+  } catch (_error) {
+    return {};
+  }
+};
+


This change is necessary for making Zip files work on Node.js with metering. We might not be exercising this anymore, but I include the change for completeness. cc @michaelfig

kriskowal · 2021-09-20T22:57:29Z

packages/vats/src/crc.js

+ * Forked from npm polycrc@1.1.0 for module system compatibility, then trimmed
+ * down to a version that assumes TypedArrays are universal.


Embedding CRC6 is a reversible expedient. The existing polycrc doesn’t work with Endo because of a bad interaction with named exports. I may be able to fix that with more work on the Endo side. Otherwise, bringing this in-house and getting it past lint and tsc increased my confidence in the code, so there’s an upside. The downside is that I don’t think it’s worthwhile to also bring in the tests from polycrc. So, if this isn’t palatable, the alternatives are:

Do more work on Endo until the CommonJS/ESM boundary works for this particular case. That is, an ESM importing a CJS with the previously working incantation import { models as crcmodels } from 'polycrc', which requires detecting that models is an export detected by module.exports = { models } (approximately) in the lexical analysis.

Patch polycrc so it’s "type": "module" and uses the appropriate ESM gestures.

kriskowal · 2021-09-20T22:59:04Z

packages/zoe/src/zoeService/zoe.js

@@ -73,7 +73,7 @@ const makeZoeKit = (
  },
  meteringConfig = {
    incrementBy: 25_000_000n,
-    initial: 50_000_000n, // executeContract for treasury is 13.5M
+    initial: 75_000_000n, // executeContract for treasury is 13.5M


@katelynsills This was enough of a boost to clear CI. More boost might be necessary to preserve more overhead. The comment is probably stale now. Do we have a tool for measuring computron cost? cc @warner

@kriskowal does it seem plausible that zip archives are 50% more compute-intensive? What is it doing that take so much more computrons?

The nestedEvaluate format is a string, whereas the endoZipArchive is a string in base64, which we then convert to a TypedArray with Base64.decode (which is probably free regardless of size since it’s native and I assume not metered), but then we slice the TypedArray into small files and UTF-8 decode them (which might be metered, but should be close to free on XS).

Some of this overhead should turn in our favor when we switch CapTP to a binary protocol, which will reduce the size of setBundle messages by about 25% and eliminate the base64 decode. The UTF-8 decode might not change.

The nestedEvaluate format is a code string that contains code strings. The link phase is expressed as code, whereas with endoZipBase64, it’s expressed as a JSON blob that creates a Compartment graph and links them. That’s overhead. The overhead could probably be reduced by translating the JSON compartment map into code.

There would remain some overhead though because nestedEvaluate is a single-compartment runtime and endoZipBase64 puts us on footing for LavaMoat style dependency sandboxing.

ah. right. Compartment-per-package for least-authority linkage is why we would pay this cost.

I wonder whether Base64.decode should be metered... or if all uses of it are going to cost enough to build the input so that it doesn't matter. I probably should open an issue.

Opening an issue sounds good to me. Where does Moddable want us to open issues?

it should be an agoric-sdk issue.

We probably don't need help from Moddable. We should be able to just combine the native base64 implementation that does something O(n) on the length of the input... for example, since regexps are already metered: s.match(/.*/)

Excellent. Filed #3874

kriskowal · 2021-09-20T22:59:53Z

packages/zoe/test/unitTests/test-zoe.js

@@ -415,7 +418,7 @@ test(`zoe.getConfiguration`, async t => {
    },
    meteringConfig: {
      incrementBy: 25000000n,
-      initial: 50000000n,
+      initial: 75000000n,


This test detects any change to the default metering config, which isn’t terribly valuable. Could this instead import the defaults and inject them back here?

Yup, that's a good idea. I'll make an issue.

Issue here: #3884

katelynsills

This looks good to me as per @dtribble's decision that we will add validation to Zoe later! Some small requests to leave in code that I think got wrongly deleted.

katelynsills · 2021-09-23T18:32:40Z

packages/zoe/test/swingsetTests/zoe/vat-dave.js

-      assert(installation === installations.atomicSwap, X`wrong installation`);
-      assert(
-        sameStructure(
-          harden({ Asset: invitationIssuer, Price: bucksIssuer }),
-          issuerKeywordRecord,
-        ),
-        X`issuerKeywordRecord were not as expected`,
-      );


I think these lines are wrongly removed. They're unrelated to the changes in this PR.

katelynsills · 2021-09-23T18:33:25Z

packages/zoe/test/swingsetTests/zoe/vat-dave.js

+      // const instance = await E(zoe).getInstance(invitation);
+      // const installation = await E(zoe).getInstallation(invitation);


Suggested change

// const instance = await E(zoe).getInstance(invitation);

// const installation = await E(zoe).getInstallation(invitation);

// const installation = await E(zoe).getInstallation(invitation);

I'm not sure why instance is gotten here if it's the source/hash that we care about

katelynsills · 2021-09-23T18:34:21Z

packages/zoe/test/swingsetTests/zoe/vat-dave.js

-      const instance = await E(zoe).getInstance(invitation);
-      const installation = await E(zoe).getInstallation(invitation);
-      const issuerKeywordRecord = await E(zoe).getIssuers(instance);


These lines should remain in.

katelynsills · 2021-09-23T18:34:30Z

packages/zoe/test/swingsetTests/zoe/vat-dave.js

-  const [moolaIssuer, simoleanIssuer, bucksIssuer] = issuers;
+  const [moolaIssuer, simoleanIssuer] = issuers;


Should remain the same

katelynsills · 2021-09-23T18:35:23Z

packages/zoe/test/unitTests/test-zoe.js

@@ -415,7 +418,7 @@ test(`zoe.getConfiguration`, async t => {
    },
    meteringConfig: {
      incrementBy: 25000000n,
-      initial: 50000000n,
+      initial: 75000000n,


Yup, that's a good idea. I'll make an issue.

katelynsills · 2021-09-23T18:37:18Z

packages/zoe/test/unitTests/test-zoe.js

@@ -415,7 +418,7 @@ test(`zoe.getConfiguration`, async t => {
    },
    meteringConfig: {
      incrementBy: 25000000n,
-      initial: 50000000n,
+      initial: 75000000n,


Issue here: #3884

katelynsills · 2021-09-23T22:11:06Z

I should add, I did not look closely at the crc code. That is probably best for @michaelfig

*BREAKING CHANGE*: This change switches the default bundle format from `nestedEvaluate` to `endoZipBase64`. The new bundle format has a fundamentally different shape and content, so any existing code that depends on being able to inspect the `source` string of the bundle will break. This functionality will be replaced with a new feature for consistent-hash integrity checking. #3859

…chive support

kriskowal force-pushed the kris-sync-endo branch from 2c60897 to b5dcea5 Compare June 9, 2021 05:54

kriskowal force-pushed the kris-zip branch from 6916016 to a38cebe Compare June 9, 2021 05:54

kriskowal force-pushed the kris-sync-endo branch from b5dcea5 to 68ea016 Compare June 9, 2021 05:57

kriskowal force-pushed the kris-zip branch 2 times, most recently from 145ac71 to c688fd8 Compare June 9, 2021 16:34

Base automatically changed from kris-sync-endo to master June 9, 2021 17:01

kriskowal force-pushed the kris-zip branch 5 times, most recently from ee4fd38 to 534641f Compare June 11, 2021 17:16

katelynsills reviewed Jun 11, 2021

View reviewed changes

packages/zoe/test/swingsetTests/zoe/vat-dave.js Outdated Show resolved Hide resolved

kriskowal mentioned this pull request Jun 11, 2021

Compartment Mapper must account for symbolic links endojs/endo#774

Closed

kriskowal force-pushed the kris-zip branch 2 times, most recently from a11d96e to eb0def8 Compare June 15, 2021 02:42

kriskowal mentioned this pull request Jun 15, 2021

feat(compartment-mapper): Developer dependencies endojs/endo#780

Merged

kriskowal force-pushed the kris-zip branch 2 times, most recently from c74a8cd to a049acb Compare June 16, 2021 23:03

This was referenced Jun 19, 2021

fix: Patch external-editor constructor override error #3360

Merged

fix(SwingSet): Use extension for vat-controller jig #3361

Merged

kriskowal force-pushed the kris-zip branch from a049acb to d5834cb Compare June 20, 2021 18:21

kriskowal changed the base branch from master to kris-bundle-source-dev-syms June 20, 2021 18:21

kriskowal force-pushed the kris-bundle-source-dev-syms branch 2 times, most recently from 2d36ee8 to 7bfa155 Compare June 21, 2021 04:12

kriskowal force-pushed the kris-zip branch from d5834cb to 4e85e09 Compare June 21, 2021 04:12

kriskowal force-pushed the kris-bundle-source-dev-syms branch from 7bfa155 to 4bf1772 Compare June 21, 2021 04:12

kriskowal force-pushed the kris-zip branch from 4e85e09 to 4bb41d5 Compare June 21, 2021 04:12

kriskowal force-pushed the kris-bundle-source-dev-syms branch from 4bf1772 to be0d555 Compare June 21, 2021 04:26

kriskowal force-pushed the kris-zip branch from 4bb41d5 to 1bd7c86 Compare June 21, 2021 04:26

kriskowal force-pushed the kris-zip branch from c3b594b to 2d2f6ad Compare September 20, 2021 22:17

kriskowal commented Sep 20, 2021

View reviewed changes

packages/SwingSet/test/test-xsnap-store.js Show resolved Hide resolved

kriskowal commented Sep 20, 2021

View reviewed changes

kriskowal mentioned this pull request Sep 21, 2021

Bundle hashing #3859

Closed

kriskowal force-pushed the kris-zip branch from 2d2f6ad to 35e9f55 Compare September 21, 2021 00:20

kriskowal mentioned this pull request Sep 21, 2021

Epic: Evolution of Zoe Installations #3871

Closed

4 tasks

katelynsills mentioned this pull request Sep 23, 2021

E(zoe).getConfiguration() test #3884

Closed

katelynsills approved these changes Sep 23, 2021

View reviewed changes

kriskowal force-pushed the kris-zip branch from 35e9f55 to d459e9b Compare September 23, 2021 22:58

kriskowal added 10 commits September 23, 2021 16:26

chore: Sync endo version

7e7ef43

chore: Update yarn.lock

fb199c9

fix(SwingSet): Adjust SES change detectors

3efb36e

fix(transform-meter): Accommodate SES module proxies in meter

9f7d456

test(zoe): Note to test integrity checks

758957c

feat: Thread URL and Base64 endowments

b52269d

fix: Increase default initial computrons for Zoe contracts for zip ar…

01c833e

…chive support

fix(vats): Fork polycrc for ESM compat with Endo/Zip

6d9df0e

refactor(vats): Pare down CRC and meet style

b6f823b

kriskowal force-pushed the kris-zip branch from d459e9b to b6f823b Compare September 23, 2021 23:27

kriskowal enabled auto-merge September 23, 2021 23:42

kriskowal merged commit 8ce5f8e into master Sep 23, 2021

kriskowal deleted the kris-zip branch September 23, 2021 23:50

mhofman mentioned this pull request Oct 16, 2021

Fix loadgen and runner to accommodate endo bundles Agoric/testnet-load-generator#32

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Switch default archive format to endoZipBase64 #3273

refactor: Switch default archive format to endoZipBase64 #3273

kriskowal commented Jun 9, 2021 •

edited

Loading

kriskowal Sep 20, 2021

kriskowal Sep 20, 2021

kriskowal Sep 20, 2021

kriskowal Sep 20, 2021

kriskowal Sep 20, 2021

dckc Sep 20, 2021

kriskowal Sep 20, 2021 •

edited

Loading

dckc Sep 21, 2021

kriskowal Sep 21, 2021

dckc Sep 21, 2021

kriskowal Sep 22, 2021

kriskowal Sep 20, 2021

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills left a comment

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills Sep 23, 2021

katelynsills commented Sep 23, 2021

		* Forked from npm polycrc@1.1.0 for module system compatibility, then trimmed
		* down to a version that assumes TypedArrays are universal.

		// const instance = await E(zoe).getInstance(invitation);
		// const installation = await E(zoe).getInstallation(invitation);

		const [moolaIssuer, simoleanIssuer, bucksIssuer] = issuers;
		const [moolaIssuer, simoleanIssuer] = issuers;

refactor: Switch default archive format to endoZipBase64 #3273

refactor: Switch default archive format to endoZipBase64 #3273

Conversation

kriskowal commented Jun 9, 2021 • 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

kriskowal Sep 20, 2021 • 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

katelynsills 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

katelynsills commented Sep 23, 2021

kriskowal commented Jun 9, 2021 •

edited

Loading

kriskowal Sep 20, 2021 •

edited

Loading