fix: changes based on review comments

- make crank buffer key iteration work correctly in the face of data changes
- get rid of nugatory blockBuffer

Author: FUDCo

packages/SwingSet/src/blockBuffer.js

@@ -1,139 +1,16 @@
 // @ts-check
 import { initSwingStore, openSwingStore } from '@agoric/swing-store';
-import { assert, details as X } from '@agoric/assert';
 
 /*
 The "Storage API" is a set of functions { has, getKeys, get, set, delete } that
 work on string keys and accept string values.  A lot of kernel-side code
 expects to get an object which implements the Storage API, which is usually
 associated with a write-back buffer wrapper.
 
-The "HostDB API" is a different set of functions { has, getKeys, get,
-applyBatch } which the host is expected to provide to the Controller in the
-config object. This API allows SwingSet to deliver batches of changes to the
-host-side storage medium.
-
-buildHostDBInMemory creates hostDB objects for testing and casual hosts that
-can afford to hold all state in RAM. They must arrange to call getState() at
-the end of each block and save the resulting string to disk.
-
 A more sophisticated host will build a hostDB that writes changes to disk
 directly.
 */
 
-/**
- * Create a new instance of a bare-bones implementation of the HostDB API.
- *
- * @param {KVStore} kvStore Storage object that the new HostDB object will be based on.
- *    If omitted, defaults to a new in memory store.
- */
-export function buildHostDBInMemory(kvStore) {
-  if (!kvStore) {
-    kvStore = initSwingStore(null).kvStore;
-  }
-
-  /**
-   * Test if the storage contains a value for a given key.
-   *
-   * @param {string} key  The key that is of interest.
-   *
-   * @returns {boolean} true if a value is stored for the key, false if not.
-   */
-  function has(key) {
-    return kvStore.has(key);
-  }
-
-  /**
-   * Obtain an iterator over all the keys within a given range.
-   *
-   * @param {string} start  Start of the key range of interest (inclusive)
-   * @param {string} end  End of the key range of interest (exclusive)
-   *
-   * @returns {Iterable<string>} an iterator for the keys from start <= key < end
-   */
-  function getKeys(start, end) {
-    return kvStore.getKeys(start, end);
-  }
-
-  /**
-   * Obtain the value stored for a given key.
-   *
-   * @param {string} key  The key whose value is sought.
-   *
-   * @returns {string=} the (string) value for the given key, or undefined if there is no
-   *    such value.
-   */
-  function get(key) {
-    return kvStore.get(key);
-  }
-
-  /**
-   * @typedef {{
-   *   op: 'set',
-   *   key: string,
-   *   value: string,
-   * }} SetOperation
-   *
-   * @typedef {{
-   *   op: 'delete',
-   *   key: string,
-   * }} DeleteOperation
-   *
-   * @typedef {{
-   *   op: Exclude<string, 'set' | 'delete'>,
-   *   key: string,
-   * }} UnknownOperation
-   *
-   * @typedef {SetOperation | DeleteOperation} BatchOperation
-   * x typedef {Exclude<AnyOperation, BatchOperation>} UnknownOperation
-   */
-
-  /**
-   * Make an ordered set of changes to the state that is stored.  The changes
-   * are described by a series of change description objects, each of which
-   * describes a single change.  There are currently two forms:
-   *
-   * { op: 'set', key: <KEY>, value: <VALUE> }
-   * or
-   * { op: 'delete', key: <KEY> }
-   * which describe a set or delete operation respectively.
-   *
-   * @param {Array<BatchOperation>} changes  An array of the changes to be
-   *    applied in order.
-   * @throws {Error} if any of the changes are not well formed.
-   */
-  function applyBatch(changes) {
-    // TODO: Note that the parameter checking is done incrementally, thus a
-    // malformed change descriptor later in the list will only be discovered
-    // after earlier changes have actually been applied, potentially leaving
-    // the store in an indeterminate state.  Problem?  I suspect so...
-    for (const c of changes) {
-      assert(`${c.op}` === c.op, X`non-string c.op ${c.op}`);
-      assert(`${c.key}` === c.key, X`non-string c.key ${c.key}`);
-      switch (c.op) {
-        case 'set':
-          assert(`${c.value}` === c.value, X`non-string c.value ${c.value}`);
-          kvStore.set(c.key, c.value);
-          break;
-        case 'delete':
-          kvStore.delete(c.key);
-          break;
-        default:
-          assert.fail(X`unknown c.op ${/** @type {*} */ (c).op}`);
-      }
-    }
-  }
-
-  const hostDB = {
-    has,
-    getKeys,
-    get,
-    applyBatch,
-  };
-
-  return harden(hostDB);
-}
-
 /**
  * Helper function to initialize the appropriate storage objects for the kernel
  *

packages/SwingSet/src/hostStorage.js

@@ -84,6 +84,17 @@ export function makeKernelSyscallHandler(tools) {
     kernelKeeper.incStat('syscalls');
     kernelKeeper.incStat('syscallVatstoreGetAfter');
     let nextIter;
+    // Note that the working key iterator will be invalidated if the parameters
+    // to `vatstoreGetAfter` don't correspond to the working key iterator's
+    // belief about what iteration was in progress.  In particular,
+    // `actualKeyPrefix` incorporates the vatID.  Additionally, when this
+    // syscall is used for iteration over a collection `keyPrefix` also
+    // incorporates the collection ID.  This ensures that uncoordinated
+    // concurrent iterations cannot interfere with each other.  If such
+    // concurrent iterations *do* happen, there will be a modest performance
+    // cost since the working key iterator will have to be regenerated each
+    // time, but we expect this to be a rare case since the normal use pattern
+    // is a single iteration in a loop within a single crank.
     if (
       workingKeyPrefix === actualKeyPrefix &&
       workingPriorKey === actualPriorKey &&

packages/SwingSet/src/kernel/kernelSyscall.js

@@ -13,6 +13,43 @@ import { insistStorageAPI } from '../../storageAPI.js';
 // that signals that the object could be foreign and thus deserving of
 // xenophobia.
 
+/**
+ * Given two iterators over ordered sequences, produce a new iterator that will
+ * iterate in order over the merged output of the two iterators.
+ *
+ * @param { Iterator } it1
+ * @param { Iterator } it2
+ *
+ * @yields any
+ */
+function* mergeSortedIterators(it1, it2) {
+  let v1 = it1.next();
+  let v2 = it2.next();
+  while (!v1.done && !v2.done) {
+    if (v1.value < v2.value) {
+      const result = v1.value;
+      v1 = it1.next();
+      yield result;
+    } else if (v1.value === v2.value) {
+      const result = v1.value;
+      v1 = it1.next();
+      v2 = it2.next();
+      yield result;
+    } else {
+      const result = v2.value;
+      v2 = it2.next();
+      yield result;
+    }
+  }
+  const itrest = v1.done ? it2 : it1;
+  let v = v1.done ? v2 : v1;
+  while (!v.done) {
+    const result = v.value;
+    v = itrest.next();
+    yield result;
+  }
+}
+
 /**
  * Create and return a crank buffer, which wraps a storage object with logic
  * that buffers any mutations until told to commit them.
@@ -40,6 +77,7 @@ 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 = {
@@ -54,18 +92,30 @@ export function buildCrankBuffer(
     },
 
     *getKeys(start, end) {
+      const generation = liveGeneration;
       assert.typeof(start, 'string');
       assert.typeof(end, 'string');
-      const keys = new Set(kvStore.getKeys(start, end));
+
+      // find additions within the query range for use during iteration
+      const added = [];
       for (const k of additions.keys()) {
-        keys.add(k);
-      }
-      for (const k of deletions.keys()) {
-        keys.delete(k);
+        if ((start === '' || start <= k) && (end === '' || k < end)) {
+          added.push(k);
+        }
       }
-      for (const k of Array.from(keys).sort()) {
+      added.sort();
+
+      for (const k of mergeSortedIterators(
+        added.values(),
+        kvStore.getKeys(start, end),
+      )) {
+        if (liveGeneration > generation) {
+          assert.fail('store modified during iteration');
+        }
         if ((start === '' || start <= k) && (end === '' || k < end)) {
-          yield k;
+          if (!deletions.has(k)) {
+            yield k;
+          }
         }
       }
     },
@@ -86,6 +136,9 @@ 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');
@@ -100,6 +153,7 @@ 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');