feat(requestid): use uuids for requestids

[rvagg]

Ref: #278
Closes: #279
Closes: #281

WIP, first pass. TODO:

• graphsync_test.go TestGraphsyncRoundTripMultipleAlternatePersistence() is failing and I don't yet understand why
• requestmanager_test.go readNNetworkRequests() used int id ordering to properly sort network requests to remove out-of-order effects, can't do that now so we have flakys, need to figure out at way to deal with that.
• Haven't figured out the network versioning problems yet:
  • Define serialization/deserialization functions for messages in the message layer #280
  • Support multiple libp2p transports in the network layer #282

[rvagg]

Still having trouble figuring out why TestGraphsyncRoundTripMultipleAlternatePersistence uniquely doesn't run. It's to do with the blockstore though, an error comes out of the LoadAttemptQueue on ClearRequest: "No active request" which halts collection of the chain. So something about having multiple persistence stores is getting borked? Somehow the requestid is mixed up this. There's various places where we index by requestid: map[graphsync.RequestID]... but switching from an int to a uuid should bother that. Unless we had overlapping requestids somehow before and now we have guaranteed uniqueness?

[rvagg]

I figured out the TestGraphsyncRoundTripMultipleAlternatePersistence problem, although I'm not really sure why this was the only test that was failing but I think it's something to do with using type RequestID uuid.UUID as a map key since uuid.UUID is type UUID []byte and slices aren't comparable, or something which causes problems for map keys. I wonder if perhaps we're dealing with identity comparison in most cases but TestGraphsyncRoundTripMultipleAlternatePersistence is hitting comparison rather than identity checking ..?

Anyway, by switching it out to type RequestID string and using it to hold bytes (not the string form, you have to .String() it for that), then it becomes comparable and safer to use all over the place. It's also neat because it's shown up other type errors that using uuid.UUID didn't—a bunch of %d in S?printf() calls which were apparently fine with uuid.UUID for some reason, but not with string.

So aside from the network message version switching, this is working as it should I believe.

[hannahhoward]

This is pretty great-- it's nice to see this overall wasn't that bad. I'm excited you have passing tests. I think we should create an epic branch to track the new protocol and do merges to that? I'll discuss with @mvdan and we can figure it out. I realize epic branchs are a pain in the but I know we're going to ship things while all this work is still outstanding.

[hannahhoward]

oh the simplied sorting in this method is a sad state of affairs.... your correction works, but I wonder how I ended writing the tests this way to count on sorting order

[rvagg]

@hannahhoward see latest commit for my first pass at protocol switching and compatibility: f7ce568

No tests yet that exercise v1.0.0.

[rvagg]

Pushed another commit that adds testing, but failing testing. Fixed a few other things along the way. But I think the main problem is the lack of differentiation between the integer ID space for requests and responses. So I need to figure out a nice way to do that.

[hannahhoward]

so baseline: this looks reasonable. The MessageHandler is a good abstraction. But two caveats, one big, one small:

Big: To uniquely identify the OLD requests you need the peer ID && the integer request ID. So, for your MessageHandler, you need the following maps:

type oldRequestKey struct {
   p peer.ID
   requestID in32
}
// ...
type MessageHandler struct {
  // ...
  fromV1Map map[oldRequestKey]graphsync.RequestID
  toV1Map   map[graphsync.RequestID]oldRequestKey
  //...
}

The translation is:

• when I send out a message, if I'm writing to a v1 client:
• requests get encoded as (other peer, integerRequest)
• responses get encoded as (my peer, integerRequests)

Beyond distinguishing requests & responses, if we are talking to old client A & old client B, it's entirely possible both could send you a request with the same integer ID.

2. Personally I would just write two different versions of newMessageFromProto rather than have the pbMessage abstraction. Especially since that's generated code and it's likely to change with the IPLD version

[hannahhoward]

also, in terms of where to get the other peer.ID and the self peer.ID:
LibP2P Host.ID has the self peer
When you call NewMessageSender or SendMessage you have the peer.ID of the other peer

[hannahhoward]

sidebar: goodness why did I decide to have so much object orientation initially? I wonder if this would be made much easier if we took all this encoding out of the message package and just made and encode/decode package with functional implementations

This is more of a "later problem" than a now problem though so let's defer it.

[rvagg]

OK, this is working and ready for review. But it's blocking pending a merge with #323 so that v1.1.0 isn't protobuf but is dag-cbor.

[rvagg]

#332 has these commits and more now, it removes the 1.1.0 protocol, adding a 2.0.0 that's dag-cbor with UUIDs and retains the translation layer that was introduced here for 1.1.0 / 1.0.0.