Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Wrapped Libp2p Transport Protocol #331

Conversation

hannahhoward
Copy link
Collaborator

@hannahhoward hannahhoward commented Jun 2, 2022

Goals

Support multiple transports on the libp2p protocol

Implementation

  • move the network protocol underneath the transport folder as a helper
  • introduce a new version of the protocol "/datatransfer/1.2.0" that is setup for multiple transports. The old one remains around but ONLY supports Graphsync as the transport (the old protocol is "/fil/datatransfer/1.2.0")
  • add a new type to the schema: a "WrappedTransferMessage" -- contains simply a message & a transport ID -- characterized as a string
  • add a new method on the message interface called WrappedForTransport(transportID) that allows you to convert to from a message type to a wrapped message type encoded for a transport.
  • introduce the concept of a "message version" which identifies the format of a data transfer message regardless of the underlying method of moving over the wire. I want to distinguish between message format (i.e. the current 1.2.0 format) and the underlying protocol name, and whether or not it's wrapped.
  • the naming scheme for the libp2p protocols is libp2p protocol name/message version

For Discussion

  1. Why does the message interface implement the "WrappedForTransport" method instead of having all the wrapping just done in the network library?

Put simply, cause right now bindnode & schemas are pretty rough to work with across modules. I tried this way first, gave up, and put it all in message. Maybe this will help: ipld/go-ipld-prime#364 ? I'm not sure. Even then we have all the go types all spread out.

  1. What's the deal with the complete mess of versions?

Well, this is the state of affairs at the moment cause of some of the hacks we employed to make everything work. At one point we introduced a 1.2.0 protocol without changing the message format, solely so Graphsync could figure out what protocols the other side supports to decide what extensions to encode (eek).

That's why we have a message1_1 but a protocol 1.2 and various graphsync extensions rated 1.2.0 or 1.1.0

  1. What about versioning by transport ID?

Yea I wonder about this, specifically cause of the versioning issue above that created the mess we're in. I wonder if a transport should have an identifier and a version.

  1. What about how we do version negotiation with different transport versions?

I once again wonder if we should just byte the bullet and register lots of libp2p protocols of the scheme

/datatransfer/message-version/transport-id/transport-version

What's the harm? It would mean no more wrapping (though maybe we keep it to be safe and for record keeping). It would mean registering lots of protocols, but my impression is that's low cost to do so, and ultimately we get a big win in terms of protocol negotiation by doing so.

  1. What about a capabilities endpoint?

That feels needed at this point maybe?

@hannahhoward hannahhoward requested a review from rvagg June 2, 2022 02:11
@codecov-commenter
Copy link

Codecov Report

Merging #331 (351d16e) into feat/refactor-transport-part-1 (1076301) will increase coverage by 2.40%.
The diff coverage is 63.00%.

Impacted file tree graph

@@                        Coverage Diff                         @@
##           feat/refactor-transport-part-1     #331      +/-   ##
==================================================================
+ Coverage                           52.39%   54.80%   +2.40%     
==================================================================
  Files                                  26       28       +2     
  Lines                                2670     2854     +184     
==================================================================
+ Hits                                 1399     1564     +165     
- Misses                               1096     1111      +15     
- Partials                              175      179       +4     
Impacted Files Coverage Δ
itest/gstestdata.go 98.29% <ø> (ø)
transport/graphsync/graphsync.go 0.00% <0.00%> (ø)
transport/graphsync/receiver.go 0.00% <0.00%> (ø)
transport/helpers/network/libp2p_impl.go 63.15% <45.45%> (ø)
transport/helpers/network/interface.go 60.00% <60.00%> (ø)
message/message1_1prime/message.go 52.38% <75.00%> (+3.80%) ⬆️
message/message1_1prime/transfer_request.go 75.34% <88.88%> (+6.37%) ⬆️
message/message1_1prime/transfer_response.go 86.66% <89.47%> (ø)
message/message1_1prime/schema.go 75.00% <100.00%> (+3.57%) ⬆️
message/message1_1prime/transfer_message.go 33.33% <100.00%> (+13.33%) ⬆️
... and 3 more


// MessageVersionFromString parses a string into a message version
func MessageVersionFromString(versionString string) (MessageVersion, error) {
versions := strings.Split(versionString, ".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we have a stringjoin representation here, maybe something we can add in later
could also use the new CustomType stuff in bindnode to put MessageVersion in the struct itself

@@ -35,3 +36,8 @@ type TransferMessage struct {
Request nullable TransferRequest
Response nullable TransferResponse
}

type WrappedTransferMessage struct {
TransportID TransportID (rename "ID")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how strict can we be here? I assume that because we have an explicit list of transports we support that we get to be very strict with the values that can appear here. Because if we can, then maybe this should be a keyed union:

type WrappedTransferMessage union {
   Message "1.2.0"
} representation keyed

We get more compact representation and very strict matching on allowable version(s), and we can adjust the Message format with version changes.

@rvagg
Copy link
Member

rvagg commented Jun 2, 2022

Looks mostly OK to me. Wrt to the versioning options - I'm still finding this set of tradeoffs difficult to make clear decisions on. We have a number of places we can do this, none of which stand out as the obviously right way to do it. Maybe it's the case that using libp2p protocol negotiation would be a better option than having to decode messages to discover they are the wrong format? Not being able to connect might be better than connecting and not understanding each other. 🤷

@hannahhoward hannahhoward force-pushed the feat/refactor-transport-part-1 branch from 1076301 to 7fa9bb2 Compare June 3, 2022 01:19
move the network to a subdirectory, also cleanup usage of all selector, move gstestdata to the itest
directory
introduces a new wrapped protocol that encodes the transport id so that we can support multiple
transports using the network protocol
@hannahhoward hannahhoward force-pushed the feat/refactor-transport-protocol-update branch from 351d16e to 4478974 Compare June 3, 2022 18:51
@hannahhoward hannahhoward merged commit 4478974 into feat/refactor-transport-part-1 Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants