Merge pull request #3667 from Agoric/3458-unmetered-liveslots

exclude liveslots deserialization code from metering

packages/SwingSet/docs/liveslots.md

@@ -69,11 +69,37 @@ Some useful things cannot be serialized: they will trigger an error.
 Uncertain:
 
 * Maps: This might actually serialize as pass-by-presence, since it has no non-function properties (in fact it has no own properties at all, they all live on `Map.prototype`, whose properties are all functions). The receiving side gets a Presence, not a Map, but invoking e.g. `E(p).get(123)` will return a promise that will be fulfilled with the results of `m.get(123)` on the sending side.
-* WeakMaps: same, except the values being passed into `get()` would be coming from the deserializer, and so they might not be that useful (especially since Liveslots does not implement distributed GC yet).
+* WeakMaps: same, except the values being passed into `get()` would be coming from the deserializer, and so they might not be that useful.
 
 ## How things get serialized
 
 * pass-by-presence objects: `{@qclass: "slot", index: slotIndex}`
 * local Promises: passed as a promise
 * promise returned by `E(p).foo()`: passes as a promise, with pipelining enabled
 * Function: rejected (todo: wrap)
+
+## Garbage Collection vs Metering
+
+When a swingset kernel is part of a consensus machine, the visible state must be a deterministic function of userspace activity. Every member kernel must perform the same set of operations.
+
+However we are not yet confident that the timing of garbage collection will remain identical between kernels that experience different patterns of snapshot+restart. In particular, up until recently, the amount of "headroom" in the XS memory allocator was reset upon snapshot reload: the new XS engine only allocates as much RAM as the snapshot needs, whereas before the snapshot was taken, the RAM footprint could have been larger (e.g. if a large number of objects we allocated and then released), leading to more "headroom". Automatic GC is triggered by an attempt to allocate space which cannot be satisfied by this headroom, so it will happen more frequently in the post-reload engine than before the snapshot. See issues #3428, #3458, and #3577 for details.
+
+We rely upon the engine to only invoke finalizer callback at explicitly-deterministic times, but we tolerate (guard against) objects becoming collected spontaneously, which will e.g. cause a WeakRef to become "dead" (`wr.deref() === undefined`) at a random point in the middle of a turn. Any code which calls `wr.deref`, or is conditionally executed/skipped according to the results, is "GC-sensitive". This includes `convertSlotToVal`, and therefore `m.unserialize`.
+
+We cannot allow metering results to diverge between validators, because:
+
+* 1: it might make the difference between the crank completing successfully, and the vat being terminated for a per-crank metering fault
+* 2: it will change the large-scale Meter value, which is reported to userspace
+* 3: it might cause the runPolicy to finish the block earlier on one validator than on others
+
+all of which would cause a consensus failure.
+
+To prevent this, we run most of the "inbound" side of liveslots without metering. This includes the first turn of all `dispatch.*` methods, which runs entirely within liveslots:
+
+* `dispatch.deliver` performs argument deserialization in the first turn, then executes user code in the second and subsequent turns
+* `dispatch.notify` does the same
+* the GC deliveries (`dispatch.dropExport`, etc) only use one turn
+
+We also disable metering when deserializing the return value from a (synchronous) device call, and when retiring a promise ID (which touches `slotToVal`).
+
+Finally, we disable metering for all turns of the post-crank GC `finish()` call. This excludes all invocations of the finalizer callbacks, as well as all the `processDeadSet` code which is highly sensitive to the results.

packages/SwingSet/src/kernel/dummyMeterControl.js

@@ -0,0 +1,55 @@
+// @ts-check
+import { assert } from '@agoric/assert';
+
+export function makeDummyMeterControl() {
+  let meteringDisabled = 0;
+
+  function isMeteringDisabled() {
+    return !!meteringDisabled;
+  }
+
+  function assertIsMetered(msg) {
+    assert(!meteringDisabled, msg);
+  }
+
+  function assertNotMetered(msg) {
+    assert(!!meteringDisabled, msg);
+  }
+
+  function runWithoutMetering(thunk) {
+    meteringDisabled += 1;
+    try {
+      return thunk();
+    } finally {
+      meteringDisabled -= 1;
+    }
+  }
+
+  async function runWithoutMeteringAsync(thunk) {
+    meteringDisabled += 1;
+    return Promise.resolve()
+      .then(() => thunk())
+      .finally(() => {
+        meteringDisabled -= 1;
+      });
+  }
+
+  // return a version of f that runs outside metering
+  function unmetered(f) {
+    function wrapped(...args) {
+      return runWithoutMetering(() => f(...args));
+    }
+    return harden(wrapped);
+  }
+
+  /** @type { MeterControl } */
+  const meterControl = {
+    isMeteringDisabled,
+    assertIsMetered,
+    assertNotMetered,
+    runWithoutMetering,
+    runWithoutMeteringAsync,
+    unmetered,
+  };
+  return harden(meterControl);
+}

packages/SwingSet/src/kernel/kernel.js

@@ -16,6 +16,7 @@ import { insistMessage, insistVatDeliveryResult } from '../message.js';
 import { insistDeviceID, insistVatID } from './id.js';
 import { makeKernelSyscallHandler, doSend } from './kernelSyscall.js';
 import { makeSlogger, makeDummySlogger } from './slogger.js';
+import { makeDummyMeterControl } from './dummyMeterControl.js';
 import { getKpidsToRetire } from './cleanup.js';
 import { processNextGCAction } from './gc-actions.js';
 
@@ -812,6 +813,7 @@ export default function buildKernel(
     FinalizationRegistry,
     waitUntilQuiescent,
     gcAndFinalize,
+    meterControl: makeDummyMeterControl(),
   });
   const vatManagerFactory = makeVatManagerFactory({
     allVatPowers,

packages/SwingSet/src/kernel/liveSlots.js

@@ -31,7 +31,8 @@ const DEFAULT_VIRTUAL_OBJECT_CACHE_SIZE = 3; // XXX ridiculously small value to
  * @param {boolean} enableVatstore
  * @param {*} vatPowers
  * @param {*} vatParameters
- * @param {*} gcTools { WeakRef, FinalizationRegistry, waitUntilQuiescent, gcAndFinalize }
+ * @param {*} gcTools { WeakRef, FinalizationRegistry, waitUntilQuiescent, gcAndFinalize,
+ *                      meterControl }
  * @param {Console} console
  * @returns {*} { vatGlobals, inescapableGlobalProperties, dispatch, setBuildRootObject }
  *
@@ -52,7 +53,7 @@ function build(
   gcTools,
   console,
 ) {
-  const { WeakRef, FinalizationRegistry } = gcTools;
+  const { WeakRef, FinalizationRegistry, meterControl } = gcTools;
   const enableLSDebug = false;
   function lsdebug(...args) {
     if (enableLSDebug) {
@@ -408,6 +409,7 @@ function build(
       // controlled by the `console` option given to makeLiveSlots.
       console.info('Logging sent error stack', err),
   });
+  const unmeteredUnserialize = meterControl.unmetered(m.unserialize);
 
   function getSlotForVal(val) {
     return valToSlot.get(val);
@@ -461,6 +463,10 @@ function build(
       valToSlot.set(val, slot);
       slotToVal.set(slot, new WeakRef(val));
       if (type === 'object') {
+        // Set.delete() metering seems unaffected by presence/absence, but it
+        // doesn't matter anyway because deadSet.add only happens when
+        // finializers run, and we wrote xsnap.c to ensure they only run
+        // deterministically (during gcAndFinalize)
         deadSet.delete(slot);
         droppedRegistry.register(val, slot, val);
       }
@@ -489,7 +495,14 @@ function build(
     }
   }
 
+  // The meter usage of convertSlotToVal is strongly affected by GC, because
+  // it only creates a new Presence if one does not already exist. Userspace
+  // moves from REACHABLE to UNREACHABLE, but the JS engine then moves to
+  // COLLECTED (and maybe FINALIZED) on its own, and we must not allow the
+  // latter changes to affect metering. So every call to convertSlotToVal (or
+  // m.unserialize) must be wrapped by unmetered().
   function convertSlotToVal(slot, iface = undefined) {
+    meterControl.assertNotMetered();
     const { type, allocatedByVat, virtual } = parseVatSlot(slot);
     const wr = slotToVal.get(slot);
     let val = wr && wr.deref();
@@ -552,6 +565,7 @@ function build(
       for (const slot of slots) {
         const { type } = parseVatSlot(slot);
         if (type === 'promise') {
+          // this can run metered because it's supposed to always be present
           const wr = slotToVal.get(slot);
           const p = wr && wr.deref();
           assert(p, X`should have a value for ${slot} but didn't`);
@@ -567,6 +581,7 @@ function build(
 
     function collect(promiseID, rejected, value) {
       doneResolutions.add(promiseID);
+      meterControl.assertIsMetered(); // else userspace getters could escape
       const valueSer = m.serialize(value);
       valueSer.slots.map(retainExportedVref);
       resolutions.push([promiseID, rejected, valueSer]);
@@ -599,6 +614,7 @@ function build(
       }
     }
 
+    meterControl.assertIsMetered(); // else userspace getters could escape
     const serArgs = m.serialize(harden(args));
     serArgs.slots.map(retainExportedVref);
     const resultVPID = allocatePromiseID();
@@ -659,12 +675,14 @@ function build(
           return undefined;
         }
         return (...args) => {
+          meterControl.assertIsMetered(); // userspace getters shouldn't escape
           const serArgs = m.serialize(harden(args));
           serArgs.slots.map(retainExportedVref);
           forbidPromises(serArgs);
           const ret = syscall.callNow(slot, prop, serArgs);
           insistCapData(ret);
-          const retval = m.unserialize(ret);
+          // but the unserialize must be unmetered, to prevent divergence
+          const retval = unmeteredUnserialize(ret);
           return retval;
         };
       },
@@ -708,6 +726,7 @@ function build(
       method = Symbol.asyncIterator;
     }
 
+    meterControl.assertNotMetered();
     const args = m.unserialize(argsdata);
 
     // If the method is missing, or is not a Function, or the method throws a
@@ -747,6 +766,7 @@ function build(
   function retirePromiseID(promiseID) {
     lsdebug(`Retiring ${forVatID}:${promiseID}`);
     importedPromisesByPromiseID.delete(promiseID);
+    meterControl.assertNotMetered();
     const wr = slotToVal.get(promiseID);
     const p = wr && wr.deref();
     if (p) {
@@ -757,6 +777,7 @@ function build(
   }
 
   function thenHandler(p, promiseID, rejected) {
+    // this runs metered
     insistVatType('promise', promiseID);
     return value => {
       knownResolutions.set(p, harden([rejected, value]));
@@ -778,7 +799,7 @@ function build(
           pRec.resolve(value);
         }
       }
-      retirePromiseID(promiseID);
+      meterControl.runWithoutMetering(() => retirePromiseID(promiseID));
     };
   }
 
@@ -802,6 +823,7 @@ function build(
       X`unknown promiseID '${promiseID}'`,
     );
     const pRec = importedPromisesByPromiseID.get(promiseID);
+    meterControl.assertNotMetered();
     const val = m.unserialize(data);
     if (rejected) {
       pRec.reject(val);
@@ -824,6 +846,7 @@ function build(
     const imports = finishCollectingPromiseImports();
     for (const slot of imports) {
       if (slotToVal.get(slot)) {
+        // we'll only subscribe to new promises, which is within consensus
         syscall.subscribe(slot);
       }
     }
@@ -860,6 +883,7 @@ function build(
     } else {
       // Remotable
       // console.log(`-- liveslots acting on retireExports ${vref}`);
+      meterControl.assertNotMetered();
       const wr = slotToVal.get(vref);
       if (wr) {
         const val = wr.deref();
@@ -903,12 +927,14 @@ function build(
   // TODO: when we add notifyForward, guard against cycles
 
   function exitVat(completion) {
+    meterControl.assertIsMetered(); // else userspace getters could escape
     const args = m.serialize(harden(completion));
     args.slots.map(retainExportedVref);
     syscall.exit(false, args);
   }
 
   function exitVatWithFailure(reason) {
+    meterControl.assertIsMetered(); // else userspace getters could escape
     const args = m.serialize(harden(reason));
     args.slots.map(retainExportedVref);
     syscall.exit(true, args);
@@ -1030,8 +1056,21 @@ function build(
     }
   }
 
+  // the first turn of each dispatch is spent in liveslots, and is not
+  // metered
+  const unmeteredDispatch = meterControl.unmetered(dispatchToUserspace);
+
   const { waitUntilQuiescent, gcAndFinalize } = gcTools;
 
+  async function finish() {
+    await gcAndFinalize();
+    const doMore = processDeadSet();
+    if (doMore) {
+      return finish();
+    }
+    return undefined;
+  }
+
   /**
    * This low-level liveslots code is responsible for deciding when userspace
    * is done with a crank. Userspace code can use Promises, so it can add as
@@ -1048,7 +1087,7 @@ function build(
     // *not* directly wait for the userspace function to complete, nor for
     // any promise it returns to fire.
     Promise.resolve(delivery)
-      .then(dispatchToUserspace)
+      .then(unmeteredDispatch)
       .catch(err =>
         console.log(`liveslots error ${err} during delivery ${delivery}`),
       );
@@ -1060,15 +1099,7 @@ function build(
 
     // Now that userspace is idle, we can drive GC until we think we've
     // stopped.
-    async function finish() {
-      await gcAndFinalize();
-      const doMore = processDeadSet();
-      if (doMore) {
-        return finish();
-      }
-      return undefined;
-    }
-    return finish();
+    return meterControl.runWithoutMeteringAsync(finish);
   }
   harden(dispatch);
 

packages/SwingSet/src/kernel/vatManager/supervisor-nodeworker.js

@@ -13,6 +13,7 @@ import engineGC from '../../engine-gc.js';
 import { WeakRef, FinalizationRegistry } from '../../weakref.js';
 import { makeGcAndFinalize } from '../../gc-and-finalize.js';
 import { waitUntilQuiescent } from '../../waitUntilQuiescent.js';
+import { makeDummyMeterControl } from '../dummyMeterControl.js';
 import { makeLiveSlots } from '../liveSlots.js';
 import {
   makeSupervisorDispatch,
@@ -70,11 +71,13 @@ parentPort.on('message', ([type, ...margs]) => {
       makeMarshal,
       testLog,
     };
+
     const gcTools = harden({
       WeakRef,
       FinalizationRegistry,
       waitUntilQuiescent,
       gcAndFinalize: makeGcAndFinalize(engineGC),
+      meterControl: makeDummyMeterControl(),
     });
     const ls = makeLiveSlots(
       syscall,

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

@@ -10,6 +10,7 @@ import { makeMarshal } from '@agoric/marshal';
 import engineGC from '../../engine-gc.js';
 import { WeakRef, FinalizationRegistry } from '../../weakref.js';
 import { makeGcAndFinalize } from '../../gc-and-finalize.js';
+import { makeDummyMeterControl } from '../dummyMeterControl.js';
 import {
   arrayEncoderStream,
   arrayDecoderStream,
@@ -86,11 +87,13 @@ fromParent.on('data', ([type, ...margs]) => {
       makeMarshal,
       testLog,
     };
+
     const gcTools = harden({
       WeakRef,
       FinalizationRegistry,
       waitUntilQuiescent,
       gcAndFinalize: makeGcAndFinalize(engineGC),
+      meterControl: makeDummyMeterControl(),
     });
     const ls = makeLiveSlots(
       syscall,