search

ipfs / go-graphsync

Initial Implementation Of GraphSync Wire Protocol
Other
100 stars 38 forks source link

Convert Cancel/Update on graphsync request to single RequestType #345

Closed hannahhoward closed 2 years ago

hannahhoward commented 2 years ago

Goals

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

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 commented 2 years ago

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: https://github.com/ipfs/go-graphsync/blob/e8cdcb24c0bac2cb343de1e63e5792b99b0a0611/message/message.go#L165 but then deals them in priority order: cancel, then update, then assumes new https://github.com/ipfs/go-graphsync/blob/e8cdcb24c0bac2cb343de1e63e5792b99b0a0611/responsemanager/server.go#L186-L193

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: https://github.com/ipfs/go-graphsync/blob/21a4546cdfd2e53d132c4015c24c487e166c9103/message/v1/message.go#L180-L209

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 commented 2 years ago

done in https://github.com/ipfs/go-graphsync/pull/352