fix: tweaks based on review comments

packages/SwingSet/src/kernel/kernelSyscall.js

@@ -51,6 +51,10 @@ export function makeKernelSyscallHandler(tools) {
     return `${vatID}.vs.${key}`;
   }
 
+  function descopeVatstoreKey(key) {
+    return key.replace(/^([^.]+)\.vs\.(.+)$/, '$2');
+  }
+
   let workingPriorKey;
   let workingLowerBound;
   let workingUpperBound;
@@ -80,6 +84,64 @@ export function makeKernelSyscallHandler(tools) {
     return OKNULL;
   }
 
+  /**
+   * Execute one step of iteration over a range of vatstore keys.
+   *
+   * @param {string} vatID  The vat whose vatstore is being iterated
+   * @param {string} priorKey  The key that was returned by the prior cycle of
+   *   the iteration, or '' if this is the first cycle
+   * @param {string} lowerBound  The lower bound of the iteration range
+   * @param {string} [upperBound]  The uppper bound of the iteration range.  If
+   *   omitted, this defaults to a string equivalent to the `lowerBound` string
+   *   with its rightmost character replaced by the lexically next character.
+   *   For example, if `lowerBound` is 'hello', then `upperBound` would default
+   *   to 'hellp')
+   *
+   * @returns {[string, string]|undefined}  A pair of the key and value of the
+   *   first vatstore entry whose key is lexically greater than both `priorKey`
+   *   and `lowerBound`.  If there are no such entries or if the first such
+   *   entry has a key that is lexically greater than or equal to `upperBound`,
+   *   undefined is returned instead, signalling the end of iteration.
+   *
+   * Usage notes:
+   *
+   * Iteration is accomplished by repeatedly calling `vatstoreGetAfter` in a
+   * loop, each time pass the key that was returned in the previous iteration,
+   * until undefined is returned.  While the initial value for `priorKey` is
+   * conventionally the empty string, in fact any value that is less than
+   * `lowerBound` may be used to the same effect.
+   *
+   * Keys in the vatstore are arbitrary strings, but subsets of the keyspace are
+   * often organized hierarchically.  This iteration API allows simple iteration
+   * over an explicit key range or iteration over the set of keys with a given
+   * prefix, depending on how you use it:
+   *
+   * Explicitly providing both a lower and an upper bound will enable iteration
+   * over the key range `lowerBound` <= key < `upperBound`
+   *
+   * Providing only the lower bound while letting the upper bound default will
+   * iterate over all keys that have `lowerBound` as a prefix.
+   *
+   * For example, if the stored keys are:
+   * bar
+   * baz
+   * foocount
+   * foopriority
+   * joober
+   * plugh.3
+   * plugh.47
+   * plugh.8
+   * zot
+   *
+   * Then the bounds    would iterate over the key sequence
+   * ----------------   -----------------------------------
+   * 'bar', 'goomba'    bar, baz, foocount, foopriority
+   * 'bar', 'joober'    bar, baz, foocount, foopriority
+   * 'foo'              foocount, foopriority
+   * 'plugh.'           plugh.3, plugh.47, plugh.8
+   * 'baz', 'plugh'     baz, foocount, foopriority, joober
+   * 'bar', '~'         bar, baz, foocount, foopriority, joober, plugh.3, plugh.47, plugh.8, zot
+   */
   function vatstoreGetAfter(vatID, priorKey, lowerBound, upperBound) {
     const actualPriorKey = vatstoreKeyKey(vatID, priorKey);
     const actualLowerBound = vatstoreKeyKey(vatID, lowerBound);
@@ -138,7 +200,7 @@ export function makeKernelSyscallHandler(tools) {
       const nextKey = nextIter.value;
       const resultValue = kvStore.get(nextKey);
       workingPriorKey = nextKey;
-      const resultKey = nextKey.slice(vatID.length + 4); // `${vatID}.vs.`.length
+      const resultKey = descopeVatstoreKey(nextKey);
       return harden(['ok', [resultKey, resultValue]]);
     }
   }

packages/SwingSet/src/kernel/liveSlots.js

@@ -1090,24 +1090,24 @@ function build(
           syscall.vatstoreSet(`vvs.${key}`, value);
         },
         getAfter: (priorKey, lowerBound, upperBound) => {
+          let scopedPriorKey = '';
           if (priorKey !== '') {
             insistValidVatstoreKey(priorKey);
             assert(priorKey >= lowerBound, 'priorKey must be >= lowerBound');
-            priorKey = `vvs.${priorKey}`;
+            scopedPriorKey = `vvs.${priorKey}`;
           }
           insistValidVatstoreKey(lowerBound);
-          lowerBound = `vvs.${lowerBound}`;
-          if (upperBound !== undefined) {
+          const scopedLowerBound = `vvs.${lowerBound}`;
+          let scopedUpperBound;
+          if (upperBound) {
             insistValidVatstoreKey(upperBound);
             assert(upperBound > lowerBound, 'upperBound must be > lowerBound');
-          }
-          if (upperBound !== undefined) {
-            upperBound = `vvs.${upperBound}`;
+            scopedUpperBound = `vvs.${upperBound}`;
           }
           const fetched = syscall.vatstoreGetAfter(
-            priorKey,
-            lowerBound,
-            upperBound,
+            scopedPriorKey,
+            scopedLowerBound,
+            scopedUpperBound,
           );
           if (fetched) {
             const [key, value] = fetched;

packages/SwingSet/src/kernel/state/storageWrapper.js

@@ -77,7 +77,6 @@ export function buildCrankBuffer(
   // to avoid confusion, additions and deletions should never share a key
   const additions = new Map();
   const deletions = new Set();
-  let liveGeneration = 0n;
   resetCrankhash();
 
   const crankBuffer = {
@@ -92,11 +91,27 @@ export function buildCrankBuffer(
     },
 
     *getKeys(start, end) {
-      const generation = liveGeneration;
+      // Warning: this function introduces a consistency risk that callers must
+      // take into account in their usage of it.  If keys are added to the store
+      // within the range from `start` to `end` while iteration is in progress,
+      // these additions will not be visible to this iterator and thus not
+      // reflected in the stream of keys it returns.  Callers who might be
+      // vulnerable to any resulting data inconsistencies thus introduced must
+      // take measures to protect themselves, either by avoiding making such
+      // changes while iterating or by arranging to invalidate and reconstruct
+      // the iterator when such changes are made.  At this layer of abstraction,
+      // the store does not possess the necessary knowledge to protect the
+      // caller from these kinds of risks, as the nature of the risks themselves
+      // varies depending on what the caller is trying to do.  This API should
+      // not be made available to user (i.e., vat) code.  Rather, it is intended
+      // as a low-level mechanism to use in implementating higher level storage
+      // abstractions that are expected to provide their own consistency
+      // protections as appropriate to their own circumstances.
+
       assert.typeof(start, 'string');
       assert.typeof(end, 'string');
 
-      // find additions within the query range for use during iteration
+      // Find additions within the query range for use during iteration.
       const added = [];
       for (const k of additions.keys()) {
         if ((start === '' || start <= k) && (end === '' || k < end)) {
@@ -109,9 +124,6 @@ export function buildCrankBuffer(
         added.values(),
         kvStore.getKeys(start, end),
       )) {
-        if (liveGeneration > generation) {
-          assert.fail('store modified during iteration');
-        }
         if ((start === '' || start <= k) && (end === '' || k < end)) {
           if (!deletions.has(k)) {
             yield k;
@@ -136,9 +148,6 @@ export function buildCrankBuffer(
       assert.typeof(value, 'string');
       additions.set(key, value);
       deletions.delete(key);
-      if (!crankBuffer.has(key)) {
-        liveGeneration += 1n;
-      }
       if (isConsensusKey(key)) {
         crankhasher.add('add');
         crankhasher.add('\n');
@@ -153,7 +162,6 @@ export function buildCrankBuffer(
       assert.typeof(key, 'string');
       additions.delete(key);
       deletions.add(key);
-      // liveGeneration += 1n; // XXX can this be made to work? I fear not...
       if (isConsensusKey(key)) {
         crankhasher.add('delete');
         crankhasher.add('\n');

packages/SwingSet/src/message.js

@@ -155,7 +155,7 @@ export function insistVatSyscallObject(vso) {
       const [priorKey, lowerBound, upperBound] = rest;
       assert.typeof(priorKey, 'string');
       assert.typeof(lowerBound, 'string');
-      if (upperBound !== undefined) {
+      if (upperBound !== undefined && upperBound !== null) {
         assert.typeof(upperBound, 'string');
       }
       break;

packages/SwingSet/src/types.js

@@ -103,15 +103,16 @@
  * @typedef { [ vpid: string, rejected: boolean, data: SwingSetCapData ]} VatResolutions
  * @typedef { [tag: 'resolve', resolutions: VatResolutions ]} VatSyscallResolve
  * @typedef { [tag: 'vatstoreGet', key: string ]} VatSyscallVatstoreGet
+ * @typedef { [tag: 'vatstoreGetAfter', priorKey: string, lowerBound: string, upperBound: string | undefined | null ]} VatSyscallVatstoreGetAfter
  * @typedef { [tag: 'vatstoreSet', key: string, data: string ]} VatSyscallVatstoreSet
  * @typedef { [tag: 'vatstoreDelete', key: string ]} VatSyscallVatstoreDelete
  * @typedef { [tag: 'dropImports', slots: string[] ]} VatSyscallDropImports
  * @typedef { [tag: 'retireImports', slots: string[] ]} VatSyscallRetireImports
  * @typedef { [tag: 'retireExports', slots: string[] ]} VatSyscallRetireExports
  *
  * @typedef { VatSyscallSend | VatSyscallCallNow | VatSyscallSubscribe
- *    | VatSyscallResolve | VatSyscallVatstoreGet | VatSyscallVatstoreSet
- *    | VatSyscallVatstoreDelete | VatSyscallDropImports
+ *    | VatSyscallResolve | VatSyscallVatstoreGet | VatSyscallVatstoreGetAfter
+ *    | VatSyscallVatstoreSet | VatSyscallVatstoreDelete | VatSyscallDropImports
  *    | VatSyscallRetireImports | VatSyscallRetireExports
  * } VatSyscallObject
  *
@@ -134,15 +135,16 @@
  * @typedef { [ kpid: string, rejected: boolean, data: SwingSetCapData ]} KernelResolutions
  * @typedef { [tag: 'resolve', resolutions: KernelResolutions ]} KernelSyscallResolve
  * @typedef { [tag: 'vatstoreGet', key: string ]} KernelSyscallVatstoreGet
+ * @typedef { [tag: 'vatstoreGetAfter', priorKey: string, lowerBound: string, upperBound: string | undefined ]} KernelSyscallVatstoreGetAfter
  * @typedef { [tag: 'vatstoreSet', key: string, data: string ]} KernelSyscallVatstoreSet
  * @typedef { [tag: 'vatstoreDelete', key: string ]} KernelSyscallVatstoreDelete
  * @typedef { [tag: 'dropImports', krefs: string[] ]} KernelSyscallDropImports
  * @typedef { [tag: 'retireImports', krefs: string[] ]} KernelSyscallRetireImports
  * @typedef { [tag: 'retireExports', krefs: string[] ]} KernelSyscallRetireExports
  *
  * @typedef { KernelSyscallSend | KernelSyscallCallNow | KernelSyscallSubscribe
- *    | KernelSyscallResolve | KernelSyscallVatstoreGet | KernelSyscallVatstoreSet
- *    | KernelSyscallVatstoreDelete | KernelSyscallDropImports
+ *    | KernelSyscallResolve | KernelSyscallVatstoreGet | KernelSyscallVatstoreGetAfter
+ *    | KernelSyscallVatstoreSet | KernelSyscallVatstoreDelete | KernelSyscallDropImports
  *    | KernelSyscallRetireImports | KernelSyscallRetireExports
  * } KernelSyscallObject
  * @typedef { [tag: 'ok', data: SwingSetCapData | string | null ]} KernelSyscallResultOk

packages/SwingSet/test/test-vatstore.js

@@ -72,6 +72,9 @@ test('vatstore', async t => {
   send('scan', 'z.');
   // exercise various calls with malformed parameters
   send('apiAbuse', 'x.');
+  // check on handling of reads and writes during iteration
+  send('scanWithActivity', 'y.', 2, 'y.b', 4, 'y.q');
+
   await c.run();
   t.deepEqual(c.dump().log, [
     'get zot -> <undefined>',
@@ -135,5 +138,13 @@ test('vatstore', async t => {
     '  getAfter(aaax., x.) threw Error: priorKey must be >= lowerBound',
     'apiAbuse x.: use invalid key prefix',
     '  getAfter("", "ab@%%$#") threw Error: invalid vatstore key',
+    'scanWithActivity y. 2 y.b 4 y.q',
+    '    y.1 -> oney',
+    '    y.2 -> twoy',
+    '    y.3 -> threey',
+    '    y.a -> foury',
+    '    y.c -> sixy',
+    '    y.d -> seveny',
+    '    y.q -> whacka whacka',
   ]);
 });

packages/SwingSet/test/test-vattp.js

@@ -5,7 +5,7 @@ import { provideHostStorage } from '../src/hostStorage.js';
 import { initializeSwingset, makeSwingsetController } from '../src/index.js';
 import { buildMailboxStateMap, buildMailbox } from '../src/devices/mailbox.js';
 
-test('vattp', async t => {
+test.serial('vattp', async t => {
   const s = buildMailboxStateMap();
   const mb = buildMailbox(s);
   const config = {
@@ -50,7 +50,7 @@ test('vattp', async t => {
   t.deepEqual(s.exportToData(), { remote1: { outbox: [], inboundAck: 2 } });
 });
 
-test('vattp 2', async t => {
+test.serial('vattp 2', async t => {
   const s = buildMailboxStateMap();
   const mb = buildMailbox(s);
   const config = {

packages/SwingSet/test/vat-vatstore.js

@@ -125,6 +125,26 @@ export function buildRootObject(vatPowers) {
         log(`  getAfter("", "ab@%%$#") threw ${e}`);
       }
     },
+    scanWithActivity(prefix, delThreshold, toDel, addThreshold, toAdd) {
+      log(
+        `scanWithActivity ${prefix} ${delThreshold} ${toDel} ${addThreshold} ${toAdd}`,
+      );
+      let key = '';
+      let value;
+      let fetched;
+      let count = 0;
+      // eslint-disable-next-line no-cond-assign
+      while ((fetched = vatstore.getAfter(key, prefix))) {
+        count += 1;
+        [key, value] = fetched;
+        log(`    ${key} -> ${value}`);
+        if (count === delThreshold) {
+          vatstore.delete(toDel);
+        } else if (count === addThreshold) {
+          vatstore.set(toAdd, 'whacka whacka');
+        }
+      }
+    },
     delete(key) {
       vatstore.delete(key);
       log(`delete ${key}`);