Convert Cancel/Update on graphsync request to single RequestType

[hannahhoward]

Goals

Right now, a graphsync request has two bools -- cancel & update. Really, it has three modes:

• cancel = false, update = false -> new request
• cancel = true, update = false -> cancel request
• cancel = false, update = true -> update request
• cancel = true, update = true -> ????? undefined behavior

In the new protocol, it should be a single request type, especially since we may want to add more (in the future

Implementation

type GraphSyncRequestType enum {
   # New means a new request
   | New             ("n")
   # Cancel means cancel the request referenced by request ID
   | Cancel   ("c")
   # Update means the extensions contain an update about this request
   | Update             ("u")
   # Restart means restart this request from the begging, respecting the any DoNotSendCids/DoNotSendBlocks contained
   # in the extensions -- essentially a cancel followed by a new
   # | Restart ("r")
} representation string

type GraphSyncRequest struct {
  id                GraphSyncRequestID  (rename "ID")   # unique id set on the requester side
  root     optional Link                (rename "Root") # a CID for the root node in the query
  selector optional Any                 (rename "Sel")  # see https://github.com/ipld/specs/blob/master/selectors/selectors.md
  extensions        GraphSyncExtensions (rename "Ext")  # side channel information
  priority          GraphSyncPriority   (rename "Pri")  # the priority (normalized). default to 1
  requestType GraphSyncRequestType (rename "Typ") # the request type
} representation map

I put restart in as an example but it's left out intentionally for implementing first version

Hopefully the conversion is fairly clear based on the table above. I honestly don't know what we should do with the undefeined behavior for old messages where cancel + update are both true -- maybe just drop the message entirely? I have no idea what we're doing now but no implementation of graphsync should be doing this.

[rvagg]

I honestly don't know what we should do with the undefeined behavior for old messages where cancel + update are both true -- maybe just drop the message entirely?

The current behaviour just lets them through:

go-graphsync/message/message.go

Line 165 in e8cdcb2

requests[graphsync.RequestID(req.Id)] = newRequest(graphsync.RequestID(req.Id), root, selector, graphsync.Priority(req.Priority), req.Cancel, req.Update, exts)

but then deals them in priority order: cancel, then update, then assumes new

go-graphsync/responsemanager/server.go

Lines 186 to 193 in e8cdcb2

	if request.IsCancel() {
	_ = rm.abortRequest(ctx, p, request.ID(), ipldutil.ContextCancelError{})
	continue
	}
	if request.IsUpdate() {
	rm.processUpdate(ctx, key, request)
	continue
	}

In the v2 work I've already pushed this same logic down into message decoding so it short-circuits when it finds cancel and update messages:

go-graphsync/message/v1/message.go

Lines 180 to 209 in 21a4546

	if req.Cancel {
	requests[id] = message.NewCancelRequest(id)
	continue
	}
	exts, metadata, err := fromEncodedExtensions(req.GetExtensions())
	if err != nil {
	return message.GraphSyncMessage{}, err
	}
	if metadata != nil {
	return message.GraphSyncMessage{}, fmt.Errorf("received unexpected metadata in request extensions for request id: %s", id)
	}
	if req.Update {
	requests[id] = message.NewUpdateRequest(id, exts...)
	continue
	}
	root, err := cid.Cast(req.Root)
	if err != nil {
	return message.GraphSyncMessage{}, err
	}
	selector, err := ipldutil.DecodeNode(req.Selector)
	if err != nil {
	return message.GraphSyncMessage{}, err
	}
	requests[id] = message.NewRequest(id, root, selector, graphsync.Priority(req.Priority), exts...)

But that does leave a hole in the cancel=true & update=true case, in both the current release and next release they'd just be treated as a cancel. We could drop them, but I'm not sure we gain much by closing that gap? Easy to do so if you reckon it's worth it.

[rvagg]

done in #352