refactor: restructure host store API

[FUDCo]

In preparation for moving the transcripts out of the kernel database, this set of changes breaks the host store into a key-value store and a store for streams. The stream store will be used for transcripts, though in this set of changes it is unused (i.e., transcripts are for now still kept in the key-value store). Since it is unused, the stream store stuff itself is only minimally tested; this will come in the next phase, where we actually use the stream store for transcripts, since part of that will necessarily include updating the tests which validate or fiddle with transcripts (e.g., replay tests).

Although the stream store is not yet used, the API changes slightly alter how the principal host application calls (initializeSwingset and makeSwingsetController) are used to set up swingsets for execution. Although the associated changes to the host applications were minor, these API alterations ended up touching a lot of test code.

Along with this, I did a lot of tidying up the organization of the swingset storage code, especially rationalizing a lot of the naming of things which was particularly awkward and confusing. These changes also drew several more swingset files under the TypeScript type checking umbrella.

@michaelfig I've added you to the reviewer list because I fiddled with stuff in cosmic-swingset and ag-solo and you'll want to make sure I didn't mess those up in some subtle way.

@dckc I've added you to the reviewer list since you are going to be dealing storage for snapshots and this may be helpful as a heads-up as to where the swing-store is going and also a chance to head off possible impedance mismatches at the pass.

[michaelfig]

cosmic-swingset and solo LGTM!

I won't go so far as to approve this PR, but whatever my opinion counts for, I'm not blocking landing it.

[dckc]

I skimmed it. I don't see any problems.

Yay for more @ts-check!

swingset-runner/trun seems to have grown a runnerArgs feature, so strictly speaking this seems to go a little beyond a refactor. And --filedb went away? I note this mostly as evidence that I skimmed to the end, not as arguments to hold up the train.

[FUDCo]

swingset-runner/trun seems to have grown a runnerArgs feature, so strictly speaking this seems to go a little beyond a refactor. And --filedb went away? I note this mostly as evidence that I skimmed to the end, not as arguments to hold up the train.

If you're curious, both of those things have to do with prepping for moving the transcripts out of the database -- the trun thing for testing and the --filedb because we're no longer going to support the .jsonlines format going forward (it was there for backwards compatibility with old stuff that no longer exists) as updating it for the stream-based transcript storage would be a day or so of effort for no benefit in return.

[warner]

I suspect we'll be wanting to pass the whole hostStorage object in here soon, but with this refactoring in place, that ought to be pretty trivial.

[warner]

This is all correct, but it makes me wonder how exactly the addTranscriptEntry function will work: should it get held in the crank buffer (and thus this wrapStorage kinda wants to wrap both the kvStore and the streamStore? Or only in the block buffer?

We'll need to take a look at kernel.js or maybe vatManager/transcript.js where the entry gets added, and think about what the commit behavior should be.

[warner]

yeah, I'm thinking the simpleStore wants a debug/test method named copy that gets you everything

[warner]

we might want to check with @michaelfig and delete this fallback, if the simpleStore won't be persistent (or can persist kvStore but not transcripts)

[michaelfig]

Ohhh, did this change modify the behaviour?

Are we maintaining compatibility with platforms on which LMDB does not work? That was needed by some time ago, and I'm not sure if anything's changed since then. I don't like having to defend this code over and over again when I wasn't even its author (@warner, 👀).

In that case, we need to update this fallback to use a jsonlines-based kvstore and the standard persistent streamStore.

[warner]

heh :)

Yeah, we're looking to get rid of the jsonlines-based kvstore, rather than augment it with the streamStore that will hold transcript entries outside the DB. The jsonlines approach could only be used for toy examples anyways (performance-wise), so we weren't really useable on platforms without LMDB.

I'll file a ticket for @FUDCo or I to remove it. Such platforms will start to break (or at least work even worse than they currently do) once the next step of this streamStore process lands (moving transcripts from kvStore to streamStore), and will be properly broken when we remove the check-lmdb.js fallback.

[warner]

Added in #3131 . @michaelfig if you wanna grab it, feel free (you've got more working knowledge of cosmic-swingset than either of us at this point).

[michaelfig]

Yeah, we're looking to get rid of the jsonlines-based kvstore

Okay, then my only concern is that the current implementation of the ag-solo's access token needs to keep its state in a portable fashion (i.e., not LMDB which is architecture-dependent). I leveraged swing-store-simple to make that happen. Its jsonlines implementation will instead need to be inlined in the agoric-cli and solo packages.

[warner]

Ah, ok I forgot it was also being used for that purpose. We'd need to move that out anyways, since the general plan is to include most of the kvstore contents in the apphash, and a local access token is the very definition of "not part of consensus".

Do you need more than one value? Maybe this could just go into a single one-line file.

[michaelfig]

Do you need more than one value? Maybe this could just go into a single one-line file.

I can easily implement what I need. Just please don't remove the jsonlines implementation before I have a chance to do that. The unit tests for packages/agoric-cli don't exercise this requirement (though the packages/solo unit tests do). I'll fix that omission ASAP.

[warner]

gotcha.. I added #3134 to track the removal, with a prohibition on landing it until #3131 is done and cosmic-swingset no longer needs swing-store-simple.

[warner]

I highly approve of this paranoia. Let's add the same check to the readStream side, so this doesn't enable the kernel to read arbitrary parts of the filesystem.

[warner]

Let's also assert that the item is a string, and that there are no newlines inside it, to preserve the format expected by readStream.

(we should put these checks into swing-store-simple too)

[warner]

Nice stuff! The kvStore refactoring looks great. If that were a separate PR, I'd say land it.

The streamStore needs a few changes:

• minor assertions as noted: stream name should be checked on all API points to prohibit ../../etc/shadow games
• please add a basic unit test of the new functionality

The larger issue, which shouldn't hold up landing this because we need to design it carefully (and because changing the way we record transcripts is out of scope for this PR anyways), is the transaction/commit semantics. We currently use vatKeeper.addToTranscript({ deliveryStuff, syscallStuff }) to add them (oh so BTW kernelKeeper is probably going to want the whole HostStorage, not just the kvStore). This happens towards the end of a delivery, before the VatManager has returned a success/fail status to the kernel. So the transcript addition might need to be unwound if the crank actually failed. And it must not be committed until the blockBuffer is committed by the host application.

We talked about the storage facility accepting an additional argument with each stream, which would contain the name of a kvStore key that it is allowed to use. If we store the length of the file in this key, then readStream could just ftruncate the file before reading anything, and then the commit semantics of the stream would match the commit semantics of that key.

The wrinkle is that the streamStore has direct access to the DB, unlike the vatKeeper (which sits behind the crank buffer and the block buffer). When vatKeeper adds something to the transcript, it doesn't want that addition committed until all the rest of the kvStore changes it made get committed. And if streamStore uses it's special bypassing access to write the file length into the given key, that'll get committed to the DB much earlier than when vatKeeper expected, which is going to cause corruption if the crank is unwound or the application crashes during that window.

I think the fix might be to wrap streamStore in the same way we wrap kvStore, putting it behind a blockBuffer and a crankBuffer. So transcript additions made by vatKeeper are held in RAM until the crank buffer commits, then they move one step closer to the actual FDs. Only when the block buffer commits do we actually write them to disk. We must still use a length in the kvStore, to protect against a crash after the FD write happens but before the DB commit happens.

This might mean we keep a bit more data in RAM than we'd like (every transcript from every crank in the current block). We could probably arrange to write stuff to disk directly, as long as we find a good way to keep the length away from the DB until the right commit point.

So: add the streamName asserts and the item-is-a-string asserts, add a basic unit test for both simple- and lmdb-, then land it. After that' we'll figure out a design for the commit semantics, update the streamStore API as necessary, and change vatKeeper.addToTranscript to take advantage of it.

[warner]

Oh, I thought of a pretty simple approach to the commit point, not the most elegant, but easy:

• stream.push(record) (or whatever it's called) should return a length, which the caller is obligated to put into the kvStore at a key of their choosing
• readStream(name, length) takes that length, and truncates the file/array to that length before iterating

It works because the caller (vatKeeper) only has access to a buffered kvStore, so whatever they write will be held up until the same commit point as the rest of their kvStore changes. It's inelegant because the caller could get the length wrong, and they get back partial records or something. But hey, they get what they asked for.

[warner]

.. and because the stream store already has to parse the input file into lines, it'd be fine for the length to be denominated in items, not bytes, which removes the concern about getting back partial records. The kernel could still "look into the future" by submitting an oversized length when doing a read, but it would always get whole lines.

[FUDCo]

• readStream(name, length) takes that length, and truncates the file/array to that length before iterating

It works because the caller (vatKeeper) only has access to a buffered kvStore, so whatever they write will be held up until the same commit point as the rest of their kvStore changes. It's inelegant because the caller could get the length wrong, and they get back partial records or something. But hey, they get what they asked for.

My notion had always been that the reader would know (from looking in the kvStore) how many items to read, so I don't think the API needs anything on that score. However, the implementation needs to know where the proper end of the stream is because after replay you need to start appending again, and that to work right it needs to append after the last valid record rather than to where the end of the file happens to be.

[warner]

• readStream(name, length) takes that length, and truncates the file/array to that length before iterating

It works because the caller (vatKeeper) only has access to a buffered kvStore, so whatever they write will be held up until the same commit point as the rest of their kvStore changes. It's inelegant because the caller could get the length wrong, and they get back partial records or something. But hey, they get what they asked for.

My notion had always been that the reader would know (from looking in the kvStore) how many items to read, so I don't think the API needs anything on that score. However, the implementation needs to know where the proper end of the stream is because after replay you need to start appending again, and that to work right it needs to append after the last valid record rather than to where the end of the file happens to be.

Yeah, I'm hoping there's some way to express the API that can limit how badly the client can mess things up, but which also gets the transactionality right. The original idea of the client giving a key name to the storage implementation would have limited the potential damage (because the value would always be in control of the storage implementation), but that wouldn't get committed at the right time without a lot of other work. Having the client provide the length feels pretty good, but I think I'd prefer that e.g. read() takes the length as an argument, rather than the client being responsible for halting the read() iterator early. And if read() takes the length, now the storage implementation knows the intended length, and it can truncate the file after the iterator finishes, so new records go to the right place.

It's probably a good idea to prohibit push while the read() iterator is still working, to prevent interleaving that would mess up the notion of where new records should go.

[warner]

@FUDCo and I decided that this can land without the suggested API changes and without a unit test. The next step for this task will be to change the API and actually use it from the kernel (which will teach us more about what the right API ought to be), and I won't ask for a test of an API which we know is about to change anyways.