Return protocol ID that is not supported in error message.

[p-shahi]

Migrating libp2p/go-libp2p#1988

It would be convenient if the protocol ID that is not supported by the remote peer is returned as well in the error message.

Current:

protocol not supported

Desired:

protocol not supported: <protocol_ID>

@renaynay

Not opposed to it, but is this particularly useful? You know which protocol(s) you were trying to negotiate when you receive this error.

If we wanted to add the protocol IDs, we'd run into libp2p/go-libp2p#1886 again.

This is a go-multistream issue, not a go-libp2p issue by the way. Moving it there.

@marten-seemann

[renaynay]

@marten-seemann yes this is particularly useful when debugging on multiple networks or performing upgrades where we upgrade the protocolID.

[marten-seemann]

That doesn't answer my question. You can easily annotate the error yourself:

str, err := host.NewStream(ctx, peerID, proto)
if err != nil {
     return fmt.Errorf("failed to open stream with proto %s: %w", proto, err)
}

[Wondertan]

Adding two cents here, @marten-seemann.

This is totally valid argument when you open or handle a new stream.

You know which protocol(s) you were trying to negotiate when you receive this error.

However, in the system, when a high-level operation consisting of multiple subprotocols fails, the error "protocol not supported" requires going over each protocol to understand where it's coming from. Of course, one could wrap the error in each protocol with its name, but it's still better to do it in one place. Also, add on the top @renaynay's point above multiple different networks, which are appended as suffixes to the protocol id. In the end, debugging something like this starts to be painful and simple mitigation is to add the protocol name.

[marten-seemann]

That's a fair point. Want to submit a PR?

We should probably introduce an error type here, so you can get the list of protocols by using an error assertion. wdyt?

type ErrNotSupport struct {
     Protos []string
}

[p-shahi]

@sukunrt are you interested in picking this up (other folks lmk if you've already started work on this)?
This would be a good quality of life change!

[sukunrt]

Yes I'd like to pick this up.

[sukunrt]

Here we need to change ErrNotSupported response to use a new type

type ErrNotSupport struct {
     Protos []string
}

which will include all the protocols which were not supported.
This change will only impact client.go with methods like SelectOneOf, right?

[marten-seemann]

I also see one usage in doReadHandshake, what about that one?

Here we need to change ErrNotSupported response to use a new type

type ErrNotSupport struct {
     Protos []string
}

This would also need to be generic, right?

[sukunrt]

Yes. doReadHandshake will change.

Agreed, ErrNotSupport should be generic otherwise it'll be a lot of string(proto).

[sukunrt]

type ErrNotSupport struct {
     Protos []string
}```
@marten-seemann This type can be named ```ErrNotSupported[T]```, right?