Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dynamic vats cannot use XS worker #2868

Closed
warner opened this issue Apr 13, 2021 · 3 comments
Closed

dynamic vats cannot use XS worker #2868

warner opened this issue Apr 13, 2021 · 3 comments
Assignees
Labels
enhancement New feature or request SwingSet package: SwingSet xsnap the XS execution tool

Comments

@warner
Copy link
Member

warner commented Apr 13, 2021

What is the Problem Being Solved?

Currently, if a dynamic vat wants to be metered (managerOptions.metered === true), it is forced to use the "local" worker instead of whatever managerOptions.managerType asked for (i.e. xs-worker):

if (managerType === 'local' || metered || enableSetup) {
if (setup) {
return localFactory.createFromSetup(vatID, setup, managerOptions);
}
return localFactory.createFromBundle(vatID, bundle, managerOptions);
}

We did this because some unit tests were failing when their vats ran under XS. @dckc and I don't remember the details now, but it's some combination of:

  • 1: within-vat metering didn't work in XS because it relies upon:
    • access to the tameMetering transform, which uses Babel, which we must do in the kernel-side Node.js process by proxying the request over the kernel-worker pipe
    • instrumentation of the global objects within the worker process
    • access to the counter modified by said global-object instrumentation
    • note: within-vat metering probably still doesn't work within XS
  • 2: we didn't trust / couldn't use the XS engine-level metering code:
    • it didn't entirely exist (it now sort of exists)
    • it wasn't activated (it is now sort of is)
    • metering faults weren't entirely wired up to report back a failure (it might now be wired up?)
    • the worker wasn't wired up to report back usage (feat: use xsnap worker CPU meter and start reporting consumption #2384 does much of this, at least as far back as the manager)
    • the XS metering code in xsnap is a quick prototype and is neither guaranteed to be sound (inescapable) nor deterministic (both still true)

Within-vat metering is deprecated (#1341), and I hope to remove it entirely. We currently rely upon it to protect the ag-solo -side vat-host, which runs the "spawner" service, against infinite loops in code that deploy scripts spawn into the solo machine (e.g. the HTTP API server). This feature should be changed to create a new dynamic vat, which will default to having metering turned on, and infinite loops will kill the vat. This is not a entirely trivial change, we should think briefly about the progress expectations of the old approach: within-vat metering implies that you can eval some code and regain control if it exceeds the meter, which we no longer think is safe (who knows what state you're left in). However we were using the spawner before (and thought we could recover from a metering fault), we need to rethink that and make sure we're happy with whatever happens when the entire spawned dynamic vat is terminated. For REPL-like services, we might need to have the code build two dynamic vats: one which stays conservative and doesn't evaluate surprising code and stays alive, and the other which evalutes user-submitted code, might die, and can be rebuilt by the first when necessary.

I suspect XS metering is good enough for use in vat-at-a-time infinite loop protection. I don't expect it to be sound or deterministic yet, but we don't need soundness until we accept adversarial code into contracts, and we don't need determinism until we start delimiting chain-side blocks with a cumulative meter-usage count instead of a simple limit on the number of cranks.

If vat-host is the only one relying upon within-vat metering, we could change our cosmic-swingset/ag-solo configs to spawn vat-host with a local worker, change the unit tests that involve a spawner to do the same, and then allow all other dynamic vats (including ones that want metering, which is the default) to use the XS worker.

This isn't entirely a blocker for the phase-2 testnet project (using XS for all/most vats), but it would give us a lot more data, because without it we'll only be exercising the static vats under XS, and all dynamic vats would be corraled into local workers.

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Apr 13, 2021
@warner warner added this to the Testnet: Infrastructure Phase milestone Apr 13, 2021
@dckc
Copy link
Member

dckc commented Apr 13, 2021

closely related: #2837

background: XS metering status quo comes from PR #2384

moddable docs: XS Metering

@warner
Copy link
Member Author

warner commented Apr 18, 2021

#2908 needs to be fixed for this too

warner added a commit that referenced this issue Apr 26, 2021
Previously, any vat which wanted metering was forced to use worker=local,
even if they asked for something different. And worker=xs asserted that
metering was not requested. This removes both checks.

refs #2868
warner added a commit that referenced this issue Apr 26, 2021
Previously, any vat which wanted metering was forced to use worker=local,
even if they asked for something different. And worker=xs asserted that
metering was not requested. This removes both checks.

refs #2868
@dckc dckc added the xsnap the XS execution tool label Apr 28, 2021
@dckc
Copy link
Member

dckc commented Apr 28, 2021

fixed in #2988

@dckc dckc closed this as completed Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SwingSet package: SwingSet xsnap the XS execution tool
Projects
None yet
Development

No branches or pull requests

2 participants