refactor: Add a buffered StorageAPI helper

[gibson042]

As suggested at #5258 (comment)

Description

This PR extends #5292, and lays some groundwork for #4558. c67df20 is the most relevant commit.

Security Considerations

None that I can see.

Documentation Considerations

As commented, I wonder if StorageAPI functions should be extracted into their own module.

Testing Considerations

makeBufferedStorage really should be tested. Any pointers on a good analog?

[gibson042]

I'm expecting to introduce this for #4558.

[FUDCo]

Seems legit.

[warner]

The way swingset manages the kvStore is pretty old and creaky, so refactoring is welcome. That said, I'm slightly skeptical that swingset is a good source for a generic "buffered KV store" library (as your "should this live in a different package?" comment notes). And I don't really understand how cosmic-swingset uses its own buffering layer, so I can't effectively argue for or against sharing an implementation. I'm not excited about deep imports (it imposes hidden constraints on future internal refactoring). But most of those concerns might go away if this moved into a separate package some day.

So I guess I'm -0 on the change.. it's a good refactoring, it feels slightly weird, but it's probably a net win overall.

The activityhash write sequencing definitely needs to change to match the previous commit boundaries. Our tests on activityhash should be observing the writes made to the backing store.. if they aren't, we should improve them, and maybe they should also test two cranks in a row (which might have caught the inclusion of activityhash in the hash).

[warner]

By using the buffered kvStore for this following kvStore.set('activityhash'), the hash gets put into the pending buffer, not the actual store. To match the previous behavior, I think this needs an additional commit() call afterwards, or perhaps it'd be safe to move the one commit() from the start of this method to the end.

Also, we haven't been including the set('activityhash') in the activityhash itself. I don't know how to best achieve that with this new layering.. making onPendingSet ignore the one key feels weird.

[gibson042]

kvStore in this scope is the backing StorageAPI instance, not the crankBuffer wrapping it. These bindings are identical to those currently in buildCrankBuffer, and so is the call sequencing AFAICT.

Current: https://github.com/Agoric/agoric-sdk/blob/6a5f377c5ba115576a0e813cc6266039452732c0/packages/SwingSet/src/kernel/state/storageWrapper.js

• crankBuffer.set() and crankBuffer.delete() update additions and deletions and feed consensus-relevant data to crankhasher.
• commitCrank() sends a sequence of calls to the backing kvStore:
  1. kvStore.set(key, value) for each addition
  2. kvStore.delete(key) for each deletion
  3. oldActivityhash = kvStore.get('activityhash')
  4. kvStore.set('activityhash', activityhash), where activityhash is a digest of data including oldActivityhash and crankhasher.digest() (before the replacement of crankhasher between steps 2 and 3).

Proposed: https://github.com/Agoric/agoric-sdk/blob/45406c68018b36f94cfd5cd380be888ca31ed3ac/packages/SwingSet/src/kernel/state/storageWrapper.js

• crankBuffer.set() and crankBuffer.delete() update additions and deletions [buffered storage set() and delete()] and feed consensus-relevant data to crankhasher [buildCrankBuffer behavior via buffered storage onPendingSet and onPendingDelete].
• commitCrank() sends a sequence of calls to the backing kvStore:
  1. kvStore.set(key, value) for each addition and then kvStore.delete(key) for each deletion [in buffered storage commit()]
  2. oldActivityhash = kvStore.get('activityhash')
  3. kvStore.set('activityhash', activityhash), where activityhash is a digest of data including oldActivityhash and crankhasher.digest() (before the replacement of crankhasher after step 1).

I've added clarifying comments:

agoric-sdk/packages/SwingSet/src/kernel/state/storageWrapper.js

Lines 81 to 98 in 80afe86

	// Get the old activityhash directly from (unbuffered) backing storage.
	let oldActivityhash = kvStore.get('activityhash');
	if (oldActivityhash === undefined) {
	oldActivityhash = '';
	}
	// Digest the old activityhash and new crankhash into the new activityhash.
	const hasher = createSHA256();
	hasher.add('activityhash');
	hasher.add('\n');
	hasher.add(oldActivityhash);
	hasher.add('\n');
	hasher.add(crankhash);
	hasher.add('\n');
	// Store the new activityhash directly into (unbuffered) backing storage.
	const activityhash = hasher.finish();
	kvStore.set('activityhash', activityhash);

[warner]

Ah, gotcha, I was confused by the overlapping names (and I never remember which way around JS's destructuring assignment works). The extra comments helped a lot.

[warner]

Tentatively approving. Please get @michaelfig 's opinion on the chain-main.js changes to make sure I didn't miss something, and please find a way to write a test for it. packages/SwingSet/test/test-state.js has tests for buildCrankBuffer, maybe you could crib something from there to build a new packages/cosmic-swingset/test/test-chain-storage.js.

[warner]

is there any particular reason to not use if (onGet) onGet(key, value) ?

[gibson042]

No, the behavior would be equivalent either way. Is the coercing form preferred?

[warner]

I can't quite trace what's happening here. There's a weird property that I think we need to maintain: if the caller does x = s.get('foo'); x.prop = 'changed'; s.commit(), then we need to write back the modified .prop to the backing store. (I'm embarrassed that this seems to be necessary, and I'm sure that it's because I made a really weird API for the original mailbox structure, and my only defense is that this was some of the earliest Swingset code ever written).

Does this code maintain that property? I can tell there's a cache here at this level (storage), and a buffer of pending writes at the makeBufferedStorage level, but I'd like the expert to tell me if this buffered.set is sufficient to get the same value into both places.

I like makeBufferedStorage on the swingset side, it seems like a nice refactoring. I'm a little dubious about the utility using it here if maintaining that funky mutable-read-data behavior requires something weird. But if it works, and it supports the next phase of your work, I'm ok with it.

I would feel better if there were tests on this code, and I believe there are not. makeBufferedStorage should be testable within swingset, and this storage definition should be testable in isolation (from swingset and from the chain's backing store).

[gibson042]

Does this code maintain that property?

The short answer is "yes". This onGet configuration for the makeBufferedStorage() call that wraps the chainSend()-backed storage interface results in every get(key) call on the returned storage interface being immediately followed with a set(key, fetchedValue) that adds a reference to its additions map (where commit() will find it and translate to a set(key, valueWithAnyMutations) against the chainSend()-backed storage—which performs its own translation to a stringified {method: 'set', key, value} message). All of which is intended to match current behavior, complete with idiosyncrasies.

[michaelfig]

I'd like to test this PR against actual chain+ag-solo, but I think I have to wait for you, @gibson042, to resolve the conflicts and bring up-to-date with current master.

[michaelfig]

Great stuff!