refactor snapshotter

[mhofman]

refs: #6527
refs: #8025
refs: #8031
Best reviewed commit-by-commit

Description

In order to support the migrations needed for #8025 #8031, and to implement #6527, we need to enable using the snapshot pathways between golang and JS outside of the cosmos state-sync mechanism.

This PR splits the snapshotter into a SwingStoreExportsHandler module responsible for the communication and synchronization with JS, and a simplified cosmos ExtensionSnapshotter that uses the new SwingStoreExportsHandler hide the JS interactions.

Given the goroutine synchronization and multistep process of performing a SwingStore snapshot, there is a decent amount of interleaving needed, which is now expressed as facets provided as arguments to methods representing each step:

• The SwingStoreExportsHandler's InitiateExport() takes a SwingStoreExportEventHandler, which has a ExportInitiated() method called from the goroutine it starts.
• ExportInitiated() is provided a retrieveExport callback. The event handler implemented by the ExtensionSnapshotter stores that callback and invokes it in its SnapshotExtension() method (internally invoked by the cosmos snapshot manager)
• retrieveExport() processes the data received from JS, and creates a SwingStoreExportProvider for it, which is provided as an argument to the ExportRetrieved() method of the SwingStoreExportEventHandler
• ExportRetrieved() is also implemented by the ExtensionsSnapshotter and uses that provider to recover the snapshot artifacts and write them out as state-sync payloads.

The new layering allows the SwingStoreExportsHandler to more easily notice errors happening during the retrieval phase, which are currently swallowed by cosmos's snapshot manager. It also introduces a way to explicitly await for the completion of a snapshot in progress, which will be needed for genesis export and the SwingStore shadow copy migration.

Finally the swing-store import/export options are threaded through all the way into the SwingStoreExportsHandler interface, allowing different kind of snapshot artifacts to be handled, as needed for the future use cases above.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

This is a refactor splitting an existing module. The tests covering that module were similarly split, and sometimes duplicated where appropriate.
With the split it may be possible to cover some more granular steps which could only be covered in aggregate before.

Upgrade Considerations

This PR does not change any behavior or stored data, as such it does not have any upgrade impact.

[mhofman]

I followed up with a lot of code comment clarifications / godoc fixes, so it may be easier to review if I'm allowed to rebase all the fixup commits. Any objections from reviewers?

[gibson042]

Is this a good place to document and/or reference external documentation of how a Swingset snapshot is structured and/or the general process for constructing one?

[gibson042]

Suggested change

	// InitiateSnapshot initiates a SwingStore snapshot for the given
	// height. If a snapshot is already in progress, this will fail. The snapshot
	// processing is delegated to the provided `snapshotTaker`.
	// InitiateSnapshot synchronously verifies that there is not already a snapshot
	// in progress and records the start of a new one for the provided block height,
	// then launches a goroutine to asynchronously coordinate with JS for initiating
	// and retrieving a snapshot, sending it to the provided `snapshotTaker` for processing,
	// and ultimately releasing it.

That the behavior includes retrieval and processing also suggests to me that "initiate" is not the best name, although a similar argument would probably apply to using the name "take" for a function that synchronously starts a process which finishes asynchronously. It might be best to just reify the separation such that e.g. call sites look like

if err := snapshotter.InitiateSnapshot(height); err != nil {
	…
}
go snapshotter.FinishSnapshot(taker, exportOptions)

[mhofman]

That shape is not possible. Let's chat

[gibson042]

As discussed in real time, I'd like to see a refactor in which the swing-store snapshotter communicates completion of its synchronous initiation to the main thread rather than directly driving the cosmos snapshot itself, which seems like an inversion of responsibility. But to reiterate, this need not be considered a blocking concern.

[gibson042]

Do we have anywhere that describes how swing-store exports fit into the overall structure of cosmos state-sync snapshots? My recollection from our call is that there is some hook by which a module registers its integration(s) for cosmos to call when creating/loading/etc. a snapshot, and that the responsibility for creation is to produce an arbitrary number of protobuf "extension" entries with a common format version and for loading is to validate the format version of each extension entry and then consume it as appropriate.

[JimLarson]

Giving up on the commit-by-commit review halfway through the 5th commit - too much back-and-forth - and publishing the comments so far. I'm going to update to see recent commits and review the PR in its entirety.

[mhofman]

@michaelfig @JimLarson I believe I've addressed all feedback. It ended up being extensive changes, and introduces a couple conflicts with master, so right now I squashed everything and will rebase / relayer everything. I don't expect any code to change until your new review.

[JimLarson]

The rebase landed while I was in mid-review. I've submitted what was in-flight. Please address those comments as much as possible then please confirm that I have a stable target for review.

[mhofman]

I have added a sequence diagram of the snapshot creation process: https://github.com/Agoric/agoric-sdk/blob/mhofman/refactor-snapshotter/docs/architecture/state-sync.md

[gibson042]

Skimmed changes since last review.

[mhofman]

I think I've addressed all new feedback and then some, PTAL @JimLarson

[JimLarson]

LGTM at last! Thanks for applying all the previous feedback - I've verified that previous issues have been addressed to my satisfaction. Approval is conditional on applying the two comment suggestions here. One is trivial, the other explains why the dual-channel race (which is still present) is harmless.