Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Handle errors when unpacking swarm.peers() response #885

Closed
olizilla opened this issue Nov 5, 2018 · 3 comments · Fixed by #887
Closed

Handle errors when unpacking swarm.peers() response #885

olizilla opened this issue Nov 5, 2018 · 3 comments · Fixed by #887
Assignees

Comments

@olizilla
Copy link
Contributor

olizilla commented Nov 5, 2018

The addition of the quic protocol means there are now peers on the network with a multiaddr that cannot be unpacked by older verions of js-multiaddr, and it throws.

In the current implementation of ipfsApi.swarm.peers() we don't handle any errors that could occur when trying to wrap the peer addr from the repsonse in a Multiaddr:

https://github.com/ipfs/js-ipfs-api/blob/ead35992e9d5c60f85d74f0d80aad1b0e994ea5c/src/swarm/peers.js#L46-L51

the simplest solution would be to re-throw the error with an informative error message, but that still leaves us with the siutation when newer peers get added to the network, they can break my user-experience, as I suddenly start seeing 0 peers; js-ipfs-api throws an error if 1 peer appears invalid like in ipfs/ipfs-webui#878

A more helpful solution might be to provide some way to signal to the caller that some peers have information that we can't validate, but still do a best effort to return information about the response.

If we go the best-effort route, we've got

  1. easy - remove peers from the list if we hit an error trying to validate their info, and write a debug log. This is simple, but leaves the peer list that you get from ipfs-api containing fewer items than the http repsonse did, which is undesireable.

  2. hard - update the api response info to include peers that can't be validated. We could add a property to an idividual peer like error with the validation error, and the raw with the unvalidated response data. Or we could pull out unvalidtable peers into a sepearate list and return two arrays, could be peers (the valid ones) and rawPeers (all peers)...

@lidel
Copy link
Contributor

lidel commented Nov 5, 2018

We can quickly fix ipfs/ipfs-webui#878 for now by switching to js-multiaddr v5.0.2. I feel we should do that and then try solving future problems proactively going the hard way.

In the UI we should display "invalid" multiaddrs as-is (in faded out colors) or replace it with a placeholder text, just like we do with unknown countries for unknown geoip data.

@olizilla
Copy link
Contributor Author

olizilla commented Nov 5, 2018

The case of js-multiaddr not being able to validate addrs for the quic protocol, opens up an interesting problem.

The contract of a Multiaddr right now is that you will be able to get either the buffer representation or the string representation out of it for any valid instance. There is always the possibility that you can see potentially valid multiaddrs on the network that your copy of multiaddr can't verify. You can't say a multiaddr is semantically valid if your local copy of the protocol-table is out of date. (it will always be a out of date).

If an ipfs instance returns a peer-info object from a call to swarm.peers we can assume it is valid as far as the node is concened, but if js-ipfs-api is older, we can't verify that fact.

we could

  • Upgrade multiaddr to be able to represent "unvalidated" multiaddrs. If it is contstructed with a string, then getString returns the value. Other methods throw. (a significant change to the contrat of multiaddr)
  • or change the contract of swarm.peers to return the raw info from the api, so multiaddrs and peerIds as Strings. We already pass the muxer, latency and streams properties through, unvalidated: https://github.com/ipfs/js-ipfs-api/blob/ead35992e9d5c60f85d74f0d80aad1b0e994ea5c/src/swarm/peers.js#L50-L59 (this may just be pushing the error on to the caller)
  • or change the contract of swarm.peers to indicate which of the peer-info objets the client can validate and which it can't. (this seems slightly redundant as the node tacitly validates these peers as being valid by connecting to them and returning them in the swarm peers list)

@olizilla olizilla self-assigned this Nov 6, 2018
@olizilla
Copy link
Contributor Author

olizilla commented Nov 6, 2018

I'm working on a PR for this.

olizilla added a commit that referenced this issue Nov 6, 2018
BREAKING CHANGE. Previously swarm.peers would throw an uncaught error
if any peer in the reponse could have its peerId or multiaddr validated.

This PR catches errors that occur while validating the peer info. The
returned array will contain an entry for every peer in the ipfs response.
peer-info objects that couldn't be validated, now have an `error` property
and a `rawPeerInfo` property. This at least means the count of peers in
the response will be accurate, and there the info is available to the caller.

This means that callers now have to deal with peer-info objects that may
not have a `peer or `addr` property.

Adds `nock` tests to exercice the code under different error conditions.
Doing so uncovered a bug in our legacy go-ipfs <= 0.4.4 peer info parsing,
which is also fixed. The code was trying to decapusalate the peerId from
the multiaddr, but doing so trims the peerId rather than returning it.

fixes #885

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost added the in progress label Nov 6, 2018
alanshaw pushed a commit that referenced this issue Nov 26, 2018
BREAKING CHANGE. Previously swarm.peers would throw an uncaught error
if any peer in the reponse could have its peerId or multiaddr validated.

This PR catches errors that occur while validating the peer info. The
returned array will contain an entry for every peer in the ipfs response.
peer-info objects that couldn't be validated, now have an `error` property
and a `rawPeerInfo` property. This at least means the count of peers in
the response will be accurate, and there the info is available to the caller.

This means that callers now have to deal with peer-info objects that may
not have a `peer or `addr` property.

Adds `nock` tests to exercice the code under different error conditions.
Doing so uncovered a bug in our legacy go-ipfs <= 0.4.4 peer info parsing,
which is also fixed. The code was trying to decapusalate the peerId from
the multiaddr, but doing so trims the peerId rather than returning it.

fixes #885

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>
@ghost ghost removed the in progress label Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants