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

feat: use the actual libp2p version #1137

Closed
wants to merge 1 commit into from
Closed

Conversation

Stebalien
Copy link
Member

fixes #714

Problem: using "ipfs/0.1.0" for the protocol version doesn't make sense in libp2p.
Solution: set it to the go libp2p version (per #714).

The question is, does this even make sense? The protocol version was supposed to identify the libp2p protocol version, allowing peers to determine if they should even bother speaking to each other in the first place. Given that, using the go-libp2p version doesn't make any sense.

However, I feel like systems should just do something like what Filecoin does and have a simple "hello" protocol to determine if two peers want to speak. It's often more complicated than a simple string test (Filecoin tests genesis blocks).

Should we just "rebrand" the "ProtocolVersion" to "Libp2pVersion"?

@Stebalien Stebalien marked this pull request as draft July 23, 2021 19:00
In the past, we _needed_ to keep this at a specific version so we didn't
just disconnect. Now, we can _finally_ set it to something reasonable.
@vyzo
Copy link
Contributor

vyzo commented Jul 23, 2021

I think we should just call it libp2p version; it's useful for debugging purposes.

@mxinden
Copy link
Member

mxinden commented Jul 23, 2021

Thanks for the ping @Stebalien!

Do I understand correctly that you are planning to set the identify protocol version field to the go libp2p version? Would users still be able to override the identify protocol version field, e.g. could an IPFS client still set it to ipfs/0.1.0?

However, I feel like systems should just do something like what Filecoin does and have a simple "hello" protocol to determine if two peers want to speak. It's often more complicated than a simple string test (Filecoin tests genesis blocks).

Today at least one live network (Polkadot) uses the identify protocol version to differentiate e.g. production networks from test networks. Among other things the identify protocol version is used to prevent clients from different networks to connect. In my eyes the identify protocol version field is a core functionality of the identify protocol.

A remote can infer the go libp2p version based on the agent version. E.g. I can match an agent version like go-ipfs/0.8.0/48f94e2 to a specific go libp2p version. With that in mind, what would be the benefit of including the go libp2p version in the identify protocol version field once more?

For what it is worth rust-libp2p requires a user to provide the identify protocol version at setup time.

In case this is not go-libp2p specific, we should be consistent across implementations and thus this decision should not happen on the go-libp2p repository, but on the libp2p/specs repository instead.

@Stebalien
Copy link
Member Author

Do I understand correctly that you are planning to set the identify protocol version field to the go libp2p version? Would users still be able to override the identify protocol version field, e.g. could an IPFS client still set it to ipfs/0.1.0?

Not in the current patch. Basically, we have two versions:

  1. The "protocol version" (this).
  2. The "agent version" (the application).

Applications, like go-ipfs, will set the agent version.

With that in mind, what would be the benefit of including the go libp2p version in the identify protocol version field once more?

The agent version doesn't always include the version (devel builds) and the agent may not be open source. Including the specific libp2p version would allow for implementation specific decisions (in case some quick comes up).

On the other hand, I generally agree.

In case this is not go-libp2p specific, we should be consistent across implementations and thus this decision should not happen on the go-libp2p repository, but on the libp2p/specs repository instead.

You're right, we absolutely should. I'll close this and open a spec issue.

@Stebalien Stebalien closed this Jul 23, 2021
@mxinden
Copy link
Member

mxinden commented Jul 23, 2021

Thanks @Stebalien for the quick reply and for moving it to libp2p/specs.

tshakalekholoane pushed a commit to NethermindEth/go-libp2p that referenced this pull request Aug 29, 2022
Allows the protocolVersion field of the Idenfity protocol to be
configured on the host. The current value is fixed for what appears to
be for backwards compatibility with IPFS which makes it difficult for
non-IPFS protocols to use the library.

References:
  - libp2p#714
  - libp2p#1137
  - https://github.com/libp2p/rust-libp2p/blob/6855ab943bd7427a2135b46ad3d08f48fbf10872/protocols/identify/src/identify.rs#L125-L127
marten-seemann pushed a commit that referenced this pull request Sep 2, 2022
* Configure protocolVersion for Identify protocol

Allows the protocolVersion field of the Idenfity protocol to be
configured on the host. The current value is fixed for what appears to
be for backwards compatibility with IPFS which makes it difficult for
non-IPFS protocols to use the library.

References:
  - #714
  - #1137
  - https://github.com/libp2p/rust-libp2p/blob/6855ab943bd7427a2135b46ad3d08f48fbf10872/protocols/identify/src/identify.rs#L125-L127

* Fix protocol version assignment

Fix an issue where the protocolVersion string for the Identify protocol
was wrongly being assigned the agentVersion string.

* Delete trailing whitespace
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.

Make LibP2PVersion configurable and dedup with UserAgent
3 participants