Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
329 stars 208 forks source link

provide makeMarshal and other vatPowers uniformly to all worker types #1776

Open warner opened 4 years ago

warner commented 4 years ago

docs/static-vats.md defines the "vatPowers" that all vats receive. That document currently lists the following:

In addition, the code actually provides testLog (which appends strings to an array that unit tests can read), and I think I've seen evidence of makeMarshal being passed in under certain circumstances too.

However only the "local" vatManager currently receives all of these. Our other worker types don't. We need to provide uniform support for the same values everywhere.

Remotable and getInterfaceOf need to be shared with the same liveslots instance that the vat uses, since they close over an internal WeakMap to recognize pass-by-reference objects. In the cases where makeMarshal is provided, I think it might fall into the same category.

We could probably do without transformTildot (it's only used for the REPL), but for consistency we should probably implement it. In non-local workers, we must add a new worker-to-kernel protocol message. It will behave like a syscall (blocking, vat waits for kernel to respond). The vat worker will send a string of source code to the kernel, and the kernel will send a string of transformed source code back. This will keep Babel out of the worker process.

testLog can be implemented with a new one-way worker-to-kernel protocol message (it just takes a single string as argument), with no response. Alternatively, we might implement it by including an array of accumulated testLog strings in the deliveryResults message, but I'm not sure I like that approach.

We got one test to fail in a more-interesting way by providing a non-functional stub for testLog with the following diff:

diff --git a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js
index 08cc1e91..723b6e02 100644
--- a/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js
+++ b/packages/SwingSet/src/kernel/vatManager/nodeWorkerSupervisor.js
@@ -103,7 +102,9 @@ parentPort.on('message', ([type, ...margs]) => {
       // vatPowers, but only if options tell us they're wanted. Maybe
       // transformTildot should be async and outsourced to the kernel
       // process/thread.
-      const vatPowers = { Remotable, getInterfaceOf };
+      const vatPowers = { Remotable, getInterfaceOf,
+                          testLog(_ignored) {}, // TODO: convey to kernel, append to testLog
+                        };
       dispatch = makeLiveSlots(
         syscall,
         state,
diff --git a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js
index 74326c46..218f724b 100644
--- a/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js
+++ b/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js
@@ -118,7 +117,9 @@ fromParent.on('data', data => {
       // vatPowers, but only if options tell us they're wanted. Maybe
       // transformTildot should be async and outsourced to the kernel
       // process/thread.
-      const vatPowers = { Remotable, getInterfaceOf };
+      const vatPowers = { Remotable, getInterfaceOf,
+                          testLog(_ignored) {}, // TODO: convey to kernel, append to testLog
+                        };
       dispatch = makeLiveSlots(
         syscall,
         state,
diff --git a/packages/xs-vat-worker/src/vatWorker.js b/packages/xs-vat-worker/src/vatWorker.js
index cfdfc58a..8dfccc4e 100644
--- a/packages/xs-vat-worker/src/vatWorker.js
+++ b/packages/xs-vat-worker/src/vatWorker.js
@@ -124,7 +123,9 @@ function makeWorker(io, setImmediate) {
         // vatPowers, but only if options tell us they're wanted. Maybe
         // transformTildot should be async and outsourced to the kernel
         // process/thread.
-        const vatPowers = { Remotable, getInterfaceOf };
+        const vatPowers = { Remotable, getInterfaceOf,
+                            testLog(_ignored) {}, // TODO: convey to kernel, append to testLog
+                          };
         dispatch = makeLiveSlots(
           syscall,
           state,
warner commented 3 years ago

@dckc this overlaps with code you were looking at this week. Not urgent, but wanted to circle back to the notion of a (non-transcript) syscall-like worker-to-kernel message to implement transformTildot

warner commented 3 years ago

This ought to be improved by the fixes for #2671 , although perhaps it won't go far enough. Ideally we'll have a single shared function that creates the vatPowers and liveslots instances for all worker types.

warner commented 2 years ago

All four worker types now provide makeMarshal and testLog to liveslots, and the rest are created by liveslots internally. We no longer provide transformTildot, transformMetering, or makeGetMeter. And Remotable and getInterfaceOf are imported by vats bundles internally.

I did notice that makeMarshal is provided to liveslots from import { makeMarshal } from '@endo/marshal' in three worker types, but there's a copy passed into allVatPowers for manager-local. I don't think this copy is used by liveslots, and it would be weird for the vat code to get a different copy than what liveslots is using. I think this is a leftover from when we needed to share an (impure) module instance between liveslots and the vat code (because it used a WeakMap to recognize Remotable/Far -marked objects), or maybe it has to do with stack trace censoring between the different consoles.

Anyways, I'm going to redefine this ticket to be about figuring out the makeMarshal confusion, and then we can close it.