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

refactor swing-store/makeSnapStore #3431

Closed
warner opened this issue Jun 29, 2021 · 4 comments · Fixed by #3753
Closed

refactor swing-store/makeSnapStore #3431

warner opened this issue Jun 29, 2021 · 4 comments · Fixed by #3753
Assignees
Labels
enhancement New feature or request SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Jun 29, 2021

What is the Problem Being Solved?

We should refactor the various pieces of the kernel's state management library. The kernel currently needs three kinds of storage:

  • the key-value store (currently provided by LMDB, code in swing-store-lmdb)
  • the transcript stream store (currently provided by SQLite, previously by linear files, code in swing-store-lmdb)
  • the XS snapshot store (currently provided by makeSnapStore, code in xsnap)

To build a snapshot store, the caller must provide a whole bunch of filesystem authorities, and merge the result with an object that comes back from the kernel's provideHostStorage() function, in a pattern that looks like this:

  const snapstorePath = path.resolve(
    __dirname,
    './fixture-test-reload-snapshot-gc/',
  );
  fs.mkdirSync(snapstorePath, { recursive: true });
  t.teardown(() => fs.rmdirSync(snapstorePath, { recursive: true }));

  const snapStore = makeSnapStore(snapstorePath, {
    tmpName,
    existsSync: fs.existsSync,
    createReadStream: fs.createReadStream,
    createWriteStream: fs.createWriteStream,
    rename: fs.promises.rename,
    unlink: fs.promises.unlink,
    resolve: path.resolve,
  });
  const hostStorage = { snapStore, ...provideHostStorage() };

This is repeated in both host applications (chain and solo), as well as several swingset unit tests.

I'd like to see this refactored to remove that code duplication.

I don't know how/if that squares with @dckc 's laudable goal of reducing the authority in the imported snapstore source code (i.e. continuing to avoid an import fs from 'fs' in that module, and requiring the caller to do that import instead).

Also, we need to rationalize the disparate locations of the source code for these three pieces, and make some decisions about how/if to continue supporting alternate DB tools. E.g. most swingset unit tests (and swingset-using tests in downstream consumer packages like Zoe/ERTP) are content with an in-RAM swingstore, which is easier to use because you don't have to pick a directory for it. The tests that need a snapstore must provide a directory, but those tests would be easier to write if it didn't have to pick one (e.g. keep the snapshots in RAM, and use tempfiles to have a real file on disk for the brief period when xsnap needs to read or write. Or make an automatically-deleting tempdir when the swingstore is created, and use it, instead of a named outlives-the-test directory).

cc @FUDCo @dckc

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Jun 29, 2021
dckc added a commit that referenced this issue Jul 21, 2021
@warner
Copy link
Member Author

warner commented Jul 23, 2021

Between the three of us, we might need to make this change now, rather than later, to support @dckc 's #3505 work. Or at least some subset of it, enough to allow the snapstore to be told to delete snapshots at the right time (just after the host has committed to the new swingset state, and before swingset has a chance to change any vat's state).

@dckc
Copy link
Member

dckc commented Jul 23, 2021

yes, I made a small step in 73573da

dckc added a commit that referenced this issue Jul 27, 2021
dckc added a commit that referenced this issue Jul 27, 2021
dckc added a commit that referenced this issue Jul 28, 2021
dckc added a commit that referenced this issue Jul 29, 2021
dckc added a commit that referenced this issue Jul 29, 2021
In addition to maintaining a mapping from vatID to snapshotID,
vatKeeper maintains a reverse mapping. When the reverse mapping
is empty, `snapStore.prepareToDelete()` queues the snapshot for deletion.

When the host calls `swingstore.commit()`, swingstore calls `snapStore.commitDeletes()`.

fixes #3374
refs #3431

* feat(swing-store-lmdb): host commit() deletes snapshots
  * feat(snapstore): prepare/commit deletes
  * feat(swing-store-lmdb): provide snapStore with kvStore, streamStore
  * chore: move snapStore to swing-store-lmdb package
  * refactor: factor out makeSnapStoreIO
 - vatKeeper.saveSnapshot() prepares deletes
   - removeFromSnapshot() returns consumers.length
* test: use tmp directories for snapStore in tests
* test(swingset): move c.shutdown to caller of runSteps:
  runSteps didn't create it; its caller still has a reference
* chore(cosmis-swingset): use consolidated snapStore API
* chore(solo): use consolidated snapStore API
* refactor(swingset): static type for kvStore in vatKeeper
 - getRequired() asserts that get() does not return undefined
 - fix addHelpers() return type by declaring arg type
 - where kvStore.get() is ensured by getKeys() or has(),
   mark the alternative with assert.fail().
* docs(swingset): mark DB keys excluded from consensus
* chore(snapstore): assert hash has no path separators
* test(snapstore): robust prepare / commit delete
* docs(swingstore): document snapStore commit constraint
* test: move xsnap/snapstore integration test to solo
   to avoid @agoric/xsnap in swing-store-lmdb devDependencies
* test(snapstore): re-save between prepare and commit delete
  plus one more /etc/passwd test
@dckc
Copy link
Member

dckc commented Aug 2, 2021

@warner I think maybe this is done as part of #3505 (317959d).

Or is there more to do?

@dckc
Copy link
Member

dckc commented Aug 5, 2021

swing-store-lmdb has become a misnomer and we should rename it, suggests @warner , so there is more to do.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants