Should multiaddr set .replace maintain order?

[jacobheun]

Currently peerInfo.multiaddrs.replace simply deletes all instances of the existing addresses and pushes the new addresses onto the internal array. This prevents the multiaddr set from maintaining any type of order.

Ideally, I think replace should insert the new addresses at the first index on the existing addresses. This would allow PeerInfo to maintain an order of preference. In libp2p-switch we already deprioritize circuit addresses by adding them to the end of the list. If any addresses are replaced during listening, it's possible that an unwanted order could occur.

For example, currently in js-ipfs addresses before start for nodejs would look like:

/ip4/0.0.0.0/tcp/4002
/ip6/::/tcp/4009
/ip4/0.0.0.0/tcp/4003/ws
/dns4/ws-star.discovery.libp2p.io/tcp/443/wss/p2p-websocket-star

And after startup they look something like:

/p2p-websocket-star/ipfs/Qm...
/ip4/127.0.0.1/tcp/4002/ipfs/Qm...
/ip6/::/tcp/4009/ipfs/Qm...
/ip4/127.0.0.1/tcp/4003/ws/ipfs/Qm...
/p2p-circuit/ipfs/Qm...
/p2p-circuit/p2p-websocket-star/ipfs/Qm...
...

Because the tcp wildcard addresses are replaced, they end up getting added after /p2p-websocket-star/, instead of inserted in the place of the old addresses.

[jacobheun]

cc @libp2p/javascript-team

[alanshaw]

This would allow PeerInfo to maintain an order of preference

Why is it important to do this?

My gut feeling is that PeerInfo isn't the right place to be storing address priority - it feels to me like something that needs to be managed elsewhere in libp2p and would need to change dynamically anyway. I'm imagining shutting my laptop and opening it in somewhere else where it's more beneficial to be connecting over a different transport - it's impractical to then reorder all addresses for all peers you've discovered.

[jacobheun]

For your nodes addresses specifically. When I listen, transports may replace addresses in the multiaddr list with new addresses. For example /ip4/0.0.0.0/tcp/0 may get replaced by something like /ip4/127.0.0.1/tcp/8080. When another node discovers me, they'll get the list I advertised with, which is currently whatever is in peerInfo.multiaddrs.

When that node dials me, if they dial in serial, they'll start with their transport order and then the ordered list of multiaddrs for that transport. In js, since we don't have a mechanism to properly handle merging/aborting dials we'll be doing serial dials in the interim, to avoid flooding peers with connections that aren't properly accounted for. This is less of an issue with parallel dials.

There are other protocols we've discussed like dialme that are better suited for getting peers to connect over preferred addresses. And in the future we'll likely look to have a way to deterministically select the best connection and just use that.

Additionally, I would expect any .replace method on a list to account for indexing,

['january', 'march', 'february'] === ['january', 'febs', 'march'].replace('febs', 'february')

doesn't make a lot of sense to me.

[dryajov]

Initially I had the same feeling as @alanshaw, but after running through a few scenarios in my head it doesn't sound like such a bad idea... Overall, while not ideal it should work relatively well for most scenarios that are currently supported.

---

This is actually a bit of a rabbit hole of complexity, here are just a few cases I can think of that I'm not sure how to handle:

• Should internal addresses be advertised alongside the external ones (for example connecting two peers that are on the same network)?
• What happens with NAT, when a valid external address:port have been acquired, what should they replace? (see above)
• What happens when moving around, how should addresses be re-announced?

In this context, something like the above mentioned dialme seemed like the only reliable solution.

[alanshaw]

I neglected to mention, I'm not against replace doing an in-place replace, but the API is currently replace(existingAddrsArray, newAddrsArray) and I don't know how you'd do in-place replacing for that.

If the API changed to a single item replace replace(existingAddr, newAddr) or single with multi replace(existingAddr, newAddrsArray) then sure.

If that solves a priority issue as a side effect then 👍, but we should be aware that it is a side effect and not a proper solution.