Open Stebalien opened 6 years ago
@whyrusleeping I decided to turn off overriding by default as personally, that's what I'd expect.
Thinking through this, this does have a significant downside: It makes it hard to negotiate an alternative protocol. Let's say I support /ipfs/bitswap/1.*.*
, /ipfs/graphsync/1.*.*
and my peer supports /ipfs/bitswap/2.1.1
and /ipfs/graphsync/1.9.0
. Ideally, we'd negotiate /ipfs/graphsync/1.9.0
but, given prefix support only, I can't find a way to do this.
One solution is to add an additional Semver
option that means "treat the final component as a version". That should still allow us to check for conflicts. Unfortunately, the implementation is looking a bit complicated. I'm going to see if we can pull the p2p
patch through without fixing this (yet).
Specifically, allowing one version constraint per protocol is easy. The tricky part is registering different handlers for different version ranges.
I wonder what js does here
This kind of version-based protocol negotiation has proven important in devp2p (Ethereum), where the core protocol has evolved from eth62 to eth63 in the last years.
What if –internally– we make all protocols have a version? We can then fall back to a virtual 0.0.0
version at handler registration time when unversioned protocols are registered.
Version 0 would then implicitly the lowest precedence, so registering a /1.0.0
afterwards, would have the effect of the former handler managing [0.0.0, 1.0.0), and the latter handling [1.0.0, ∞), which, from the user standpoint, I'd argue is valid behaviour.
With the above approach, the protocol handler with higher specificity would win, if multiple were applicable. Just some thoughts ;-)
We can do it, the code just gets tricker and I want to make sure we'd end up covering all cases we need. The code this replaces accepts an arbitrary function and is therefor maximally flexible. The issue is that, due to this flexibility, we can't detect conflicts. This isn't an issue now but we're trying to make this system extensible (by end users) so allowing conflict detection/resolution directly in the protocol handler would be nice.
Implicit versions is an interesting idea, we've been working on a replacement for this protocol but we'll need to keep versioning in bind for that. For some historical context, protocol names were supposed to be IPFS paths: /ipfs/Qm.../my-protocol/README.md
(or something like that). In that world, semver doesn't really work, but, in practice, we never went this route anyways (protocol names were too long).
Personally, I'm not really convinced semver is the right way to go for protocols in a decentralized system regardless. My preference is a major version (or just change the protocol name) and feature detection/declaration. Semver was designed for an inherently centralized system (software publishing) and doesn't enable decentralized evolution (extension by multiple parties) very well.
I wonder what js does here
They just use a function.
Motivation: As it stands, multiple, independent services trying to use the same libp2p swarm can clobber each-other's protocol handlers without noticing. This is fine when protocol registration is all centrally manged but can go horribly wrong in pluggable systems (e.g., the
ipfs p2p
commands and a theoretical libp2p daemon).This patch:
AddHandlerFunc
as it's impossible to tell if any two handlers registered this way conflict.AddHandler
return an error on conflict.AddHandler
to help with conflict handling and cover the primary use-case ofAddHandlerFunc
See the documentation on the options added in this patch for more details.
This shouldn't break anything except:
AddHandlerFunc
(i.e., nobody).AddHandler
overriding existing protocol handlers (hopefully nobody).