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

[DISCUSSION] Add mediachain protocol code #27

Closed
wants to merge 1 commit into from

Conversation

parkan
Copy link

@parkan parkan commented Nov 1, 2016

Following up on discussion here, I thought doing this via PR would be most illustrative.

The goal is to add support for well-formed mediachain multiaddrs, which I am picturing as

/ip4/99.69.16.78/tcp/4001/mc/QmfWapjgngSkeaA5B34YaYGdVPNocUP72hMoCmosdJVzoK

Questions brought up about this approach in IRC and elsewhere:

  • what's the grammar for multiaddr? $protocol/$data tuples?
  • should this be /secio/QmfWapjgngSkeaA5B34YaYGdVPNocUP72hMoCmosdJVzoK/mc instead? why isn't IPFS /secio/QmfWapjgngSkeaA5B34YaYGdVPNocUP72hMoCmosdJVzoK/ipfs? is the commonly used /ipfs/Q... form a kind of alias/shorthand?
  • is this the wrong approach? should we instead be registering a multistream protocol (as per this comment by @diasdavid)? how should we go about implementing this? if we do choose multistream protocol approach, how should we refer to our nodes so it's clear that it's one of ours (i.e. equivalent of http://... for /http/w3c.org/http/2)?

I'm beginning to think that we do want a multistream protocol on top of secio transport, but I don't understand the relationship sufficiently to make a judgement. FWIW our nodes do not currently support bitswap (though supporting this is a long-term goal).

@daviddias
Copy link
Member

Multiaddr should just go as far as providing the information of the location of a process and the protocol that is at the transport layer that is required to use to that node, these can go from IP+TCP to proxy setups or even dynamic addressing like /dns/name /ipns/name to fetch the IP.

One important thing to note is that although we see a lot of /ipfs/QmHash, we've discussed and we want to move them to be /p2p/QmHAsh addresses, simply because the only information that is provided there, is the hash of the public key that the crypto channel should use to verify that it dialed to the node we wanted, it is not strictly tied secio necessarily (it could be used in conjunction with TLS).

All of that being said and focusing on answering your need:

  • I believe you don't want multiaddrs that say 'mediachain', as it is beneficial for a mediachain node to connect to a regular IPFS node and vice versa (or any other p2p app using libp2p), because things like the DHT and peer discovery will be shared from both networks, making them more robust.
  • If a mediachain node connects to a non mediachain node, that is ok, simply because when it tries to do the 'multistream' handshake, that protocol will fail, but all the others (again for DHT, discovery and so on) will work
  • We currently have IPFS nodes and just libp2p nodes and we identify them by the agent. The identify is its own protocol that happens during the dialing/listening, right after secio+spdy, see: https://github.com/libp2p/js-libp2p-identify/blob/master/src/message/identify.proto#L8

Here is a quick example of this "protocol multiplexing" multistream enabled handshake - https://github.com/ipfs/js-libp2p-ipfs/tree/master/examples/echo, you can see that we mount two new protocols, one "echo" and another "hello world", on top of all of the other libp2p protocols that work underneath and get abstracted away. You can do this in IPFS nodes too :)

@parkan
Copy link
Author

parkan commented Nov 2, 2016

Hi @diasdavid, thanks for the explanation -- registering as protocol handler makes sense. Does this mean that

  1. the multiaddr then becomes
/ip4/99.69.16.78/tcp/4001/p2p/QmfWapjgngSkeaA5B34YaYGdVPNocUP72hMoCmosdJVzoK
  1. p2p,$something needs to be added to protocols.csv and respective implementations?

I get that specifying the supported protocols in a multiaddr can get awkward in a plurality situation (and as things change over time etc) but I wonder if there's human-friendly way to annotate a multiaddr to make its intended function clear at a glance

@parkan
Copy link
Author

parkan commented Nov 3, 2016

Force-pushed with p2p suggestion instead of mc. Would love to get this out ASAP -- we're specifying quite a few things as multiaddrs and not being able to work w/them natively really sucks

@parkan
Copy link
Author

parkan commented Nov 7, 2016

Looks like identity.proto was renamed to https://github.com/libp2p/js-libp2p-identify/blob/master/src/message.js

However, that field is DEPRECATED and I can't find a deprecation notice/relevant commit due to file moves. I also can't seem to find an enumeration of which protocols are supported by a standard IPFS node (for example, bitswap) -- this might be because "handle" and "protocol" are very generic terms 😞

@parkan parkan mentioned this pull request Nov 7, 2016
2 tasks
@daviddias
Copy link
Member

@parkan removed that "deprecation notice", that is a old code comment (1,5 years old) not valid anymore https://github.com/libp2p/js-libp2p-identify/blob/master/src/message.js#L26

These are the protocols that are being mounted on connections inside IPFS: https://github.com/libp2p/specs/blob/master/7-properties.md#757-protocol-multicodecs

@parkan
Copy link
Author

parkan commented Nov 8, 2016

@diasdavid excellent, this is very helpful. I think we just need to mount kad, in addition to our own protocols and the defaults, and we'll be all set.

One remaining small thing is the agent/protocol versions:

  • What are their semantics? The way they're used seems opposite of how they're named:
const LibP2PVersion = "ipfs/0.1.0" // this is ProtocolVersion
const ClientVersion = "go-libp2p/3.3.4" // this is AgentVersion

The comment here suggests AgentVersion should be go-ipfs/x.x.x. js-ipfs uses js-ipfs (unversioned) as its AgentVersion and 9000 as its protocol version.

  • How can I override it in go libp2p? The strings are hardcoded in the identity service so even making my own host (we're fine using basic_host for everything else) won't help. Do I need to create my own identity service?

@parkan
Copy link
Author

parkan commented Nov 21, 2016

Shipping 1.2 of mediachain today with more correct multiaddr support. So far using a multiaddrString.replace('/p2p/', '/ipfs/') hack to support p2p multiaddrs

@parkan
Copy link
Author

parkan commented Dec 7, 2016

paging @jbenet about /p2p/ multiaddrs (call follow-up)

@jbenet
Copy link
Member

jbenet commented Dec 11, 2016

We should be switching our multiaddrs from /ipfs/ to /p2p/ -- cc @whyrusleeping @diasdavid @lgierth

We should prep the upgrade path to that in another issue.

@ghost
Copy link

ghost commented Jan 17, 2017

Sorry this has been laying around for a while.

I agree we should switch /ipfs to /p2p. The swarm protocols that each host speaks would be subject to multistream negotiation, and later announcement via some protocol discovery mechanism (e.g. "give me nodes that speak /some/protocol").

I'm ready to merge this but should we make the code 420 as it's practically the same as /ipfs (421)?

@ghost
Copy link

ghost commented Jan 17, 2017

@parkan changing the code now would probably be a breaking change for you eh?

@parkan
Copy link
Author

parkan commented Jan 17, 2017

420 makes sense due to adjacency, yeah

How breaking this will be depends -- if we store the multiaddrs as text strings , presumably we'll still be able to dial peers? /p2p/ address support is hacked in with string replace here: https://github.com/mediachain/aleph/blob/1b8aa8911891222e15f02bc1a7f9b6a87c6659bc/src/peer/identity.js#L74

@ghost
Copy link

ghost commented Jan 17, 2017

Ah, that makes it a piece of cake since you're only ever dealing with /ipfs in the binary representations :) (which is where the codes matter)

Cool, wanna update the PR? I can take care of the change in go-multiaddr (I'm poking on that for /dns and /relay anyhow).

@ghost ghost mentioned this pull request Jan 18, 2017
@ghost
Copy link

ghost commented Jan 18, 2017

Cool, wanna update the PR?

Nevermind, I included it in #34 ❤️

@parkan
Copy link
Author

parkan commented Jan 18, 2017

Awesome! Closing this.

@parkan parkan closed this Jan 18, 2017
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