9900 elide comments in bundles #9997

turadg · 2024-08-29T21:09:07Z

Description

This uses the new --elide-comments option in Endo bundle-source to… elide comments. Always.

Measured with:

cd a3p-integration
rm -f **/b1-*.json*
yarn build:submissions
du -csh **/b1-*.json
gzip **/b1-*.json
du -csh **/b1-*.json.gz

Before: 26M (6.7M gz)
After: 19M (4.1M gz)

Security Considerations

No change in runtime. Does affect code auditability. Existing bundles are base64 encoded so readers will have to find the sources anyway. Either way we will need to invest in special tooling: endojs/endo#1656

Scaling Considerations

reduces on-chain storage

Documentation Considerations

End users who base64 decode the on-chain bundles will no longer see comments… I don't expect anyone was doing this.

Testing Considerations

existing coverage

Upgrade Considerations

won't affect any existing deployments

dckc · 2024-08-29T21:20:53Z

Does affect code auditability. Existing bundles are base64 encoded so readers will have to find the sources anyway.

base64 is completely reversible, so while it does introduce friction into any audit, it doesn't affect availability; it doesn't require readers to find the sources elsewhere.

The transformation that's irreversible, and hence means you can't reliably recover the source from what's on chain, is the one that turns index.js in @agoric/time from

export * from './src/timeMath.js';
export * from './src/typeGuards.js';
// eslint-disable-next-line import/export -- just types
export * from './src/types.js';

into

({   imports: $h‍_imports,   liveVar: $h‍_live,   onceVar: $h‍_once,   importMeta: $h‍____meta, }) => (function () { 'use strict';   $h‍_imports([["./src/timeMath.js", []],["./src/typeGuards.js", []],["./src/types.js", []]]);   
})()

p.s. I left implicit: we aim to stop doing that transformation in due course.

Using XS native Compartments for Agoric Vat Workers endojs/endo#400

dckc · 2024-08-29T21:32:27Z

This should let us get rid of the multichain-testing tweak that allows MsgInstallBundle to succeed when it wouldn't with current production norms, right? How about we delete these lines in this PR to test that hypothesis?

agoric-sdk/multichain-testing/scripts/update-config.sh

Lines 19 to 28 in bd64eda

    
           echo "Increase $(*_bytes) parameters for MsgInstallBundle" 
        
           # See https://github.com/Agoric/agoric-sdk/blob/7b684a6268c999b082a326fdb22f63e4575bac4f/packages/agoric-cli/src/chain-config.js#L66 
        
           RPC_MAX_BODY_BYTES=15000000 
        
           MAX_HEADER_BYTES=$((RPC_MAX_BODY_BYTES / 10)) 
        
           MAX_TXS_BYTES=$((RPC_MAX_BODY_BYTES * 50)) 
        
           sed -i -e "s/max_body_bytes = .*/max_body_bytes = $RPC_MAX_BODY_BYTES/g" $CHAIN_DIR/config/config.toml 
        
           sed -i -e "s/max_header_bytes = .*/max_header_bytes = $MAX_HEADER_BYTES/g" $CHAIN_DIR/config/config.toml 
        
           sed -i -e "s/max_txs_bytes = .*/max_txs_bytes = $MAX_TXS_BYTES/g" $CHAIN_DIR/config/config.toml 
        
           sed -i -e "s/max_tx_bytes = .*/max_tx_bytes = $RPC_MAX_BODY_BYTES/g" $CHAIN_DIR/config/config.toml 
        
           sed -i -e "s/^rpc-max-body-bytes =.*/rpc-max-body-bytes = $RPC_MAX_BODY_BYTES/" $CHAIN_DIR/config/app.toml

cloudflare-workers-and-pages · 2024-08-29T21:53:29Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`9e15cb4`
Status:	✅ Deploy successful!
Preview URL:	https://beb76305.agoric-sdk.pages.dev
Branch Preview URL:	https://9900-elide-comments.agoric-sdk.pages.dev

View logs

turadg · 2024-08-30T16:28:31Z

packages/boot/test/bootstrapTests/addAssets.test.ts

@Chris-Hibbert amiright that this has A3P coverage? can we lose this permanently now or will it need a follow-up issue to restore it?

most a3p-integration tests are short-lived--once the release they're attached to goes live, the tests get moved to agoric-3-proposals, and only run against that release. If we want a persistent test, it would have to go into z:acceptance. I don't see a test of adding bids there, but we certainly have the ability to test that there.

kriskowal

All of the changes pertaining to bundling look fine to me, though turning on elideComments by default for all agoric CLI usage might override the intentions of individual contracts. It might be more polite to thread elideComments at the call sites. It’s also currently possible to drive bundleSource for other module formats (nestedEvaluate) which don’t accept the elideComments field in their overload. We should preserve the ability to override the format option.

kriskowal · 2024-08-30T23:40:40Z

packages/SwingSet/src/vats/vat-admin/vat-vat-admin.js

      if (!pendingBundles.has(bundleID)) {
        pendingBundles.set(bundleID, makePromiseKit());
      }
-      return pendingBundles.get(bundleID).promise;
+      return pendingBundles.get(bundleID)?.promise;


It’s a little weird to add a runtime check to appease the type system, when the runtime code above guarantees that the promise is defined.

const bundlePromise = pendingBundles.get(bundleId); if (bundlePromise !== undefined) { return bundlePromise; } const bundleKit = makePromiseKit() pendingBundles.set(bundleId, bundleKit); return bundleKit.promise;

I agree that's better code but are you also asserting it's a better change to make? I opted for the minimum characters changed.

🤷‍♂️

packages/agoric-cli/src/scripts.js

kriskowal

I appended f389f5d to your stack to sort out the bundleSource signature so it’s sensitive to the format option.

the accommodation is no longer needed after enabling elide-comments in bundler

It has some racy failure after elide-comments. These proposals are also tested in a3p-integration where they pass. getBundleCap fails because: ``` bundleID not yet installed: b1-ab8ea6984e25d2e95a6ff94cd475c6e044fba9439daafaa44e333a27efe157c24170febefe336f9eb7394a1b56540bec0a128fd5b183fbe5ee45c06d519c4136 ```

0xpatrickdev · 2024-09-06T20:47:26Z

multichain-testing/scripts/update-config.sh

-sed -i -e "s/max_txs_bytes = .*/max_txs_bytes = $MAX_TXS_BYTES/g" $CHAIN_DIR/config/config.toml
-sed -i -e "s/max_tx_bytes = .*/max_tx_bytes = $RPC_MAX_BODY_BYTES/g" $CHAIN_DIR/config/config.toml
-sed -i -e "s/^rpc-max-body-bytes =.*/rpc-max-body-bytes = $RPC_MAX_BODY_BYTES/" $CHAIN_DIR/config/app.toml
-


This edit was the only motivation for checking in update-config.sh in the first place. We should be able to delete this file and the remove the reference from config.yaml:
ebeb975

Noting to make that clearer next time, maybe with a comment like "This file only exists to support this change. If we find this is not needed, please remove this entire file"

- with #9997, we no longer need to override config.toml script - the defaults work fine

turadg requested review from kriskowal, dckc and erights August 29, 2024 21:09

turadg commented Aug 30, 2024

View reviewed changes

dckc added the force:integration Force integration tests to run on PR label Aug 30, 2024

kriskowal requested changes Aug 30, 2024

View reviewed changes

kriskowal approved these changes Aug 31, 2024

View reviewed changes

kriskowal added the automerge:rebase Automatically rebase updates, then merge label Aug 31, 2024

turadg force-pushed the 9900-elide-comments branch from f389f5d to 47a8fe5 Compare August 31, 2024 01:41

kriskowal added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Aug 31, 2024

turadg added 7 commits August 31, 2024 04:19

chore(deps): bump @endo/bundle-source to 3.4

1439255

chore(deps): resume using the 'agoric' in this repo

68d4fc0

feat: elide comments in agoric run bundle source

2abea2d

feat: elide comments in package build scripts

2d410c7

test: remove MsgInstallBundle hack

9a9e4fb

the accommodation is no longer needed after enabling elide-comments in bundler

chore(types): VatAdmin

d94b404

turadg force-pushed the 9900-elide-comments branch from 47a8fe5 to 9e15cb4 Compare August 31, 2024 04:19

mergify bot merged commit f6011d0 into master Aug 31, 2024
79 of 80 checks passed

mergify bot deleted the 9900-elide-comments branch August 31, 2024 04:52

turadg mentioned this pull request Aug 31, 2024

restore addAsset tests #10011

Closed

0xpatrickdev reviewed Sep 6, 2024

View reviewed changes

0xpatrickdev added a commit that referenced this pull request Sep 6, 2024

chore: remove config override

b9fa3ed

- with #9997, we no longer need to override config.toml script - the defaults work fine

0xpatrickdev mentioned this pull request Sep 6, 2024

chore: remove config override #10043

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

9900 elide comments in bundles #9997

9900 elide comments in bundles #9997

turadg commented Aug 29, 2024

dckc commented Aug 29, 2024 •

edited

Loading

dckc commented Aug 29, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 29, 2024 •

edited

Loading

turadg Aug 30, 2024

Chris-Hibbert Aug 30, 2024

kriskowal left a comment

kriskowal Aug 30, 2024

turadg Aug 30, 2024

kriskowal Aug 31, 2024

kriskowal left a comment

0xpatrickdev Sep 6, 2024

0xpatrickdev Sep 6, 2024

9900 elide comments in bundles #9997

9900 elide comments in bundles #9997

Conversation

turadg commented Aug 29, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc commented Aug 29, 2024 • edited Loading

dckc commented Aug 29, 2024 • edited Loading

cloudflare-workers-and-pages bot commented Aug 29, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

turadg Aug 30, 2024

Choose a reason for hiding this comment

Chris-Hibbert Aug 30, 2024

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

kriskowal Aug 30, 2024

Choose a reason for hiding this comment

turadg Aug 30, 2024

Choose a reason for hiding this comment

kriskowal Aug 31, 2024

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

0xpatrickdev Sep 6, 2024

Choose a reason for hiding this comment

0xpatrickdev Sep 6, 2024

Choose a reason for hiding this comment

dckc commented Aug 29, 2024 •

edited

Loading

dckc commented Aug 29, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Aug 29, 2024 •

edited

Loading