explicit usage of libp2p.dial()/libp2p's autodial

[danisharora099]

This is a change request

Problem

We currently use libp2p.dial() to dial every one we discover. However, libp2p by default autodials to all nodes. (https://github.com/libp2p/js-libp2p/blob/master/doc/PEER_DISCOVERY.md)

The question now is:

• whether we want to manually handle dials (considering we can't find much information about the nodes by guarantee (eg protocols they support) and have to connect to them before we know whether its a useful node or not, it's good to auto connect)

Proposed Solutions

Delete the usage of libp2p.dial() and let libp2p auto conect

Notes

I'm not entirely sure if this is a good diea is why I haven't opened a PR for it yet, but just something that might open a discussion to help understand things better. (especially since we're considering a peer management overhaul)

[fryorcraken]

However, libp2p by default autodials to all nodes. (libp2p/js-libp2p@master/doc/PEER_DISCOVERY.md)

Are you sure? I believe this was removed in 0.40.0 as documented in: https://github.com/libp2p/js-libp2p/blob/master/doc/migrations/v0.39-v0.40.md#autodial

If you remove

js-waku/packages/core/src/lib/waku.ts

Line 140 in 0b08320

libp2p.dial(peerId).catch((err) => {

Does it still auto dial?

[danisharora099]

However, libp2p by default autodials to all nodes. (libp2p/js-libp2p@master/doc/PEER_DISCOVERY.md)

Are you sure? I believe this was removed in 0.40.0 as documented in: https://github.com/libp2p/js-libp2p/blob/master/doc/migrations/v0.39-v0.40.md#autodial

If you remove

js-waku/packages/core/src/lib/waku.ts

Line 140 in 0b08320

libp2p.dial(peerId).catch((err) => {

Does it still auto dial?

Yes - I've been able to confirm the libp2p autodials discovered nodes by default as can be seen by the logs also:

  libp2p:connection-manager:auto-dialler options: {"enabled":true,"minConnections":50,"autoDialInterval":10000} +0ms

and autodial attempts:

  libp2p:connection-manager:auto-dialler connecting to a peerStore stored peer 16Uiu2HAkvWiyFsgRhuJEb9JfjYxEkoHLgnUQmr1N5mKWnYjxYRVm +10s
  libp2p:connection-manager dial to 16Uiu2HAkvWiyFsgRhuJEb9JfjYxEkoHLgnUQmr1N5mKWnYjxYRVm +10s
  libp2p:dialer check multiaddrs 16Uiu2HAkvWiyFsgRhuJEb9JfjYxEkoHLgnUQmr1N5mKWnYjxYRVm +0ms
  libp2p:dialer creating dial target for 16Uiu2HAkvWiyFsgRhuJEb9JfjYxEkoHLgnUQmr1N5mKWnYjxYRVm +0ms
  libp2p:peer-store:address-book:trace get wait for read lock +9s
  libp2p:peer-store:address-book:trace get got read lock +0ms
  libp2p:peer-store:address-book:trace get release read lock +0ms
  libp2p:dialer 1 tokens request, returning 1, 99 remaining +4ms
  libp2p:websockets dialing /dns4/node-01.ac-cn-hongkong-c.wakuv2.test.statusim.net/tcp/8000/wss/p2p/16Uiu2HAkvWiyFsgRhuJEb9JfjYxEkoHLgnUQmr1N5mKWnYjxYRVm +0ms
  libp2p:websockets dialing node-01.ac-cn-hongkong-c.wakuv2.test.statusim.net:8000 +1ms

[fryorcraken]

right. Thank you for this investigation. Which means the current dialign we do is useless. We should probably remove our manual dial.
When we tackle #914 we can disable auto dial and do things manually instead.

It would also explain why libp2p/bootstrap works. I am guessing the autodial uses the multiaddr or adds to the peerStore.

In short, I agree with the proposed solution of this ticket as a first step.

But, we will change that when tackling #914 so we have more fine control on connection and peer management.

[danisharora099]

Sounds good.

I have addressed this issue here #1118

[danisharora099]

The inconsistency is also being confirmed here: libp2p/js-libp2p#1397 (comment)

[danisharora099]

Update: It is concluded that libp2p by default autodials until minConnections and then gives control over dial attempts. However, for the scope of our project, we want complete control to be handled by Waku. Thus, disabling autoDial.

Ref: libp2p/js-libp2p#1397 (comment)

[danisharora099]

This issue will be closed with the linked PR merge.

[fryorcraken]

which one is the linked PR?

[danisharora099]

which one is the linked PR?

the one linked is updated: #1135