Refactor, implement OpStore interface

[DavidBuchanan314]

Previously, the LogValidationContext struct stored all the state required for verifying operation logs. It was also responsible for some aspects of operation validation (e.g. 72h limit, nullification checks, etc.) but not others (e.g. signature verification).

NOTE: the API description here is stale, see review comments / the code for up-to-date details.

This PR introduces the OpStore interface, which abstracts the storage layer, along with an InMemoryOpStore implementation. (Separately, I have postgres and sqlite implementations of the interface via gorm, as part of the replica service impl)

type OpStore interface {
	// GetHead returns the CID of the most recent valid operation for a DID.
	// Returns empty string if the DID does not exist.
	GetHead(ctx context.Context, did string) (string, error)

	// GetMetadata returns metadata about a specific operation.
	// Returns an error if the operation does not exist.
	GetMetadata(ctx context.Context, did string, cid string) (*OpStatus, error)

	// GetOperation returns the operation data for a specific DID and CID.
	// Returns an error if the operation does not exist.
	GetOperation(ctx context.Context, did string, cid string) (Operation, error)

	// CommitOperations atomically commits a batch of prepared operations to the store.
	// All operations in the batch are committed or none are (all-or-nothing).

	// For each PreparedOperation, `prevHead` MUST match the head value returned by an earlier call to GetHead.
	// If multiple updates to the same DID are attempted concurrently, one will return an error due to head mismatch.
	CommitOperations(ctx context.Context, ops []*PreparedOperation) error
}

func VerifyOperation(ctx context.Context, store OpStore, did string, op Operation, createdAt time.Time) (*PreparedOperation, error) {

The VerifyOperation method uses an OpStore to build a verified PreparedOperation struct, which represents a description of changes to be later applied to the store via a call to CommitOperations. CommitOperations takes a slice of PreparedOperations so that commits can be batched (optionally), which supports bulk verification use cases (e.g. replica backfill).

VerifyOpLog has been updated to use OpStore instead of LogValidationContext, which shows a basic example of how it's used (which is much simpler!). Aside from syntactic validation, almost all the verification logic now happens inside VerifyOperation

[DavidBuchanan314]

This addition is unrelated to the OpStore interface, it's just something else I'm using in the replica impl

[bnewbold]

I like the general approach here.

Left some comments on the interface/API shape; hoping it can be tweaked to be a tighter without too much refactoring trouble.

I haven't reviewed the implementation in detail yet. A couple more tests around corner-cases would probably be good, even just against the in-memory implementation. Eg, conflicting calls to CommitOperations() for the same DID.

[bnewbold]

I might update this comment after reviewing the replica code, but my initial feeling is that this interface could use a bit of iteration.

It feels like OpStatus could be replaced with something like an OpEntry which contains both the metadata and the raw operation itself, similar to the output of the /{did}/log/audit endpoint. It should include the OpCID.

Instead of needing both GetMetadata and GetOperation, could have a single GetEntry(ctx, did, cid) (*OpEntry, error) which returns both together.

Instead of GetHead, do GetLatest(ctx, did) (*OpEntry, error). That avoids additional GetMetadata and/or GetOperation calls; and if those weren't necessary, the overhead should not be much.

I think CommitOperations is generally good: takes a batch, does everything as a transaction. It feels like PreparedOperation could be an OpEntry plus one or two fields?

Feels like the interface should include GetAllEntries(ctx, did) ([]OpEntry, error) which includes all operations and metdata, even if nullified. I guess maybe we'd maybe want to paginate that at some point like GetEntriesSince(ctx, did, index) or something.

I could be wrong, but the above is my intuition of what will work well for SQL stores, and avoid race-y corner-cases like GetHead(), but then by the time you call GetMetadata with that CID the row has already been nullified.

[DavidBuchanan314]

re: merging GetHead/GetMetadata/GetOperation - my initial motivation for the current design was to avoid over-reading. The metadata is small, but operations are large-ish, so it seemed wasteful to e.g. read (and deserialise) a whole operation when you just wanted to know its timestamp, for example. But maybe it's more important to reduce the total number of queries - I'll have a re-think on this.

The GetHead/GetMetadata race is a non-issue in practice because the head value gets checked on commit, and a nullification implies a head update. So, any update that was verified over an inconsistent view of the db would not be successfully committed.

re: GetAllEntries. One reason I didn't go for this is that in theory you could have an "amnesic" replica service, which stores only a) the most recent op for each DID b) only 72h worth of history (to be able to validate nullifications). I didn't want to rule out impls like this from using the OpStore interface. Perhaps there could be an ArchivalOpStore interface that extends the basic OpStore interface, adding GetAllEntries (or similar).

[dholms]

I have the same instinct as Bryan: these ops are small enough that we should just prioritize passing them around rather than requiring multiple roundtrips/connections to the DB

On the GetAllEntries one, I don't think it's a big deal. Though I'd be inclined to just include it on this interface and if an implementation doesn't keep around history then have it return an empty list

[bnewbold]

I'd lean towards including GetAllEntries in the basic interface, and have it return a "not implemented" error if appropriate (as opposed to empty list)

[DavidBuchanan314]

I realised I was actually reading the operations from the db in the GetMetadata implementation anyway (to get the rotation keys list), so merging GetMetadata+GetOperation into a single GetEntry does not cause any more over-reading than there was already. So I've implemented that, as well as GetLatest which combines what was previously GetHead.

I'll add GetAllEntries to the interface too.

[DavidBuchanan314]

This added boolean addresses the issue discussed here: #24 (comment)

[bnewbold]

I would do this using defined error types instead of a flag. eg:

var (
    ErrInvalidOp := errors.New("invalid PLC op")

    // transient/retryable 
    ErrOpStoreAccess := errors.New("failed to read or write OpStore")
)

[...]

// creating a wrapped error
return nil, fmt.Errorf("%w: opening database connection", ErrOpStoreAccess)

[...]

// checking error type
resp, err := VerifyOperation(ctx, store, op, createdAt)
if err != nil && errors.Is(err, ErrOpStoreAccess) {
    // retry...
} else if err != nil {
    // fail...
}

I would be conservative with defining errors to start, we can always add more later. Eg, we don't need to define distinct errors for every path, or have every error response wrap a defined type.

[dholms]

nice i like the new interface 👌

[DavidBuchanan314]

I'm going to merge this now so I can start rebasing the replica impl branch on top of it - any additional interface tweaks can be made there (although I think it's good now!)