randomWalk should be tested through interface-peer-discovery tests

[vasco-santos]

The current randomWalk implementation is not being tested with the libp2p/interface-peer-discovery.

This should have a similar implementation suite of tests like the other peer-discovery modules.

[hstove]

So, is this library not actually compatible with peer discovery? I'm just getting started with libp2p, and using KadDHT as a peerDiscovery module doesn't work, and runs into a few errors. I'd love to be able to use randomWalk as a peer discovery mechanism.

Thanks!

[vasco-santos]

Hello @hstove

Thanks for showing interest in libp2p and KadDHT. Currently, we are using random-walk to discover peers inside the DHT.

However, it is not compatible with interface-peer-discovery. We want to have it as the others peer discovery mechanisms, but some things need to be changed for that. Would you like to try it out submitting a PR?

Thanks

[jayarjo]

@vasco-santos still not compatible with interface-peer-discovery? What other things you have in mind when you say they need to be changed?

[jacobheun]

Copying this over from libp2p/go-libp2p-kad-dht#295 (comment):

Random Walk in the DHT does work, and is quite effective at discovering peers. As far as the peer-discovery interface is concerned, random walk is implementing it. The problem (really just an annoyance, there's no runtime impact) is that Random Walk isn't added to js-libp2p's peer discovery services and instead exists and is managed solely by the DHT. Also, interface-peer-discovery actually needs tests https://github.com/libp2p/interface-peer-discovery/issues/6.

---

Ideally we'd like to be able to have RandomWalk added to the discovery services that libp2p manages itself. The highest priority thing for us though is https://github.com/libp2p/interface-peer-discovery/issues/6, so we can ensure all Peer Discovery services are working as intended.

[jayarjo]

@jacobheun so peer discovery is there, just not in a way we would expect from something that implements interface-peer-discovery?

[jacobheun]

Correct, you can see the main difference in an upcoming js-ipfs update. Instead of the normal peerDiscovery registration https://github.com/ipfs/js-ipfs/blob/eb29e666ecc94bfa621023954d6bfc3e2c5357b3/src/core/runtime/libp2p-nodejs.js#L43-L47 and configuration https://github.com/ipfs/js-ipfs/blob/eb29e666ecc94bfa621023954d6bfc3e2c5357b3/src/core/runtime/libp2p-nodejs.js#L51-L62,
randomWalk is done via the dht configuration https://github.com/ipfs/js-ipfs/blob/eb29e666ecc94bfa621023954d6bfc3e2c5357b3/src/core/runtime/libp2p-nodejs.js#L63-L69.

Random Walk is currently internal to the DHT, instead of being an explicitly registered discovery service.

[jayarjo]

@jacobheun I think this wasn't available when I made original comment, 'cause it was giving me config validation error until I updated libp2p.

[jacobheun]

@jayarjo ah, yeah that would have been an issue, depending on the version and the config. The config has changed a bit over the past few versions of the DHT. I just released a patch release of libp2p, 0.25.2, that should be less restrictive for this reason, and allow the DHT to do the config validation for itself.

[jayarjo]

@jacobheun it keeps rediscovering the same nodes, although node seemingly already has them in its peerBook. Is this intended?

[jacobheun]

Ideally it shouldn't be happening, but we have to do some cleanup to resolve this.

One problem is the PeerBook.put() logic happening to frequently in the libp2p code base, libp2p/js-libp2p#345. Ideally, this would be localized to very specific places. Right now a lot of the code uses the put as a catch all to make sure we either have the latest data for a peer, or are updating the latest data. It's also expensive because we have to do buffer compares on the multiaddrs.

The PeerBook.put problem leads to the second problem, which is peer discovery: https://github.com/libp2p/js-libp2p/blob/v0.25.2/src/index.js#L471-L474.

There are some useful side effects we get with continuously discovering peers, but overall it's not good. Once we add the ability to tag peers, such as Bootstrap nodes, and fix these issues, we should be able to remove the redundant discovery.

[jayarjo]

we have to do buffer compares on the multiaddrs

I was thinking to have a simple Map locally to distinguish between already found and new. Doesn't each peer have it's own "unqiue" id? This for example:

if (peerInfo.id.toB58String() === this.peerInfo.id.toB58String()) {
      log.error(new Error(codes.ERR_DISCOVERED_SELF))
      return
}

Perhaps I'm missing a bigger picture here?

[jayarjo]

Oh... I see now toB58String generation logic itself is pretty sophisticated. Is it what you've meant?

[jacobheun]

I was thinking to have a simple Map locally to distinguish between already found and new.

You could easily use a Set to know new vs old and doing something for new peers. Note, if you want to dial them you should turn autoDial off on libp2p.

let seen = new Set()
libp2p.on('peer:discovery', (peerInfo) => {
  // If its in the set, exit early
  if (seen.has(peerInfo.id.toB58String()) return
  // Otherwise, add it and do something neat
  seen.add(peerInfo.id.toB58String())
  doSomething(peerInfo)
})

The caveat of this is for something like Bootstrap peers. Right now, we have a side effect with auto Dial, that if your peer count drops below the minimum peer threshold set for the Connection Manager, already discovered peers will be dialed when they're emitted again as discovered. Ideally, if we drop below that threshold we should dial good, well known peers. But, since we don't have tagging for peers yet, we don't really have a concept of who we should actually dial. We know the Bootstrap peers will get emitted consistently, on an interval, so at the very least we'll connect to them when we get low on peers.

[kumavis]

The readme of libp2p/interface-peer-discovery describes itself has having a set of tests for testing the interface, but it doesnt actually contain anything beyond the readme

Edit: tests have been added 🎉

[kumavis]

randomwalk.start/stop does not match the spec #111

[vasco-santos]

After the latest changes of the go-libp2p-kad-dht, we intend to rewrite most of the this codebase once we have a spec from the go implementation. Once that is done, we will guarantee that the random walk is full compliant with the interface-peer-discovery.

[achingbrain]

Randomwalk has been removed in favour of routing table refresh so this is no longer necessary.