Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autodialer uses 0.0.0.0 address #1484

Closed
fryorcraken opened this issue Nov 21, 2022 · 10 comments
Closed

Autodialer uses 0.0.0.0 address #1484

fryorcraken opened this issue Nov 21, 2022 · 10 comments
Labels
kind/stale need/author-input Needs input from the original author

Comments

@fryorcraken
Copy link
Contributor

  • Version: 0.40.0

  • Platform: Firefox 106.0.4 (64-bit) Fedora Linux

  • Subsystem: AutoDialler/Dialer/Websocket

Severity: High

Description:

Using libp2p with websockets in the browser, connecting to remote peer:

11:08:38.658 libp2p:websockets dialing /dns4/node-01.ac-cn-hongkong-c.wakuv2.prod.statusim.net/tcp/443/wss/p2p/16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD +0ms

Goes fine, after ~10min the websocket connection drops.

I can see that the autodialer kicks in and try to re-dial the target. However, instead of using the known multiaddr, the multiaddr gets replaced by 0.0.0.0 in the process:

11:18:37.934 libp2p:connection-manager:auto-dialler connecting to a peerStore stored peer 16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD +10s common.js:108
11:18:37.934 libp2p:connection-manager dial to 16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD +10s common.js:108
11:18:37.934 libp2p:dialer check multiaddrs 16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD +10s common.js:108
11:18:37.935 libp2p:dialer creating dial target for 16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD +0ms common.js:108
11:18:37.935 libp2p:peer-store:address-book:trace get wait for read lock +10s common.js:108
11:18:37.935 libp2p:peer-store:address-book:trace get got read lock +1ms common.js:108
11:18:37.936 libp2p:peer-store:address-book:trace get release read lock +0ms common.js:108
11:18:37.938 libp2p:dialer 1 tokens request, returning 1, 99 remaining +3ms common.js:108
11:18:37.939 libp2p:websockets dialing /ip4/0.0.0.0/tcp/8000/wss/p2p/16Uiu2HAm4v86W3bmT1BiH6oSPzcsSr24iDQpSN5Qa992BCjjwgrD +10s common.js:108
11:18:37.939 libp2p:websockets dialing 0.0.0.0:8000 +0ms common.js:108
 libp2p:dialer:error dial to /ip4/0.0.0.0/tcp/8000/wss/p2p/16Uiu2HAmVkKntsECaYfefR1V2yCR79CegLAT libp2p:dialer:error dial to /ip4/0.0.0.0/tcp/8000/wss/p2p/16Uiu2HAmVkKntsECaYfefR1V2yCR79CegLATuTPE6B9TxgxBiiiA failed +10s 
error { target: WebSocket, isTrusted: true, srcElement: WebSocket, currentTarget: WebSocket, eventPhase: 2, bubbles: false, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }uTPE6B9TxgxBiiiA failed +10s 
error { target: WebSocket, isTrusted: true, srcElement: WebSocket, currentTarget: WebSocket, eventPhase: 2, bubbles: false, cancelable: false, returnValue: true, defaultPrevented: false, composed: false, … }

Steps to reproduce the error:

I have not yet been able to have a minimal reproduction repository. I have tried to investigate the issue but I see a lot of refactoring has happen so I need to get familiar with the new code.

@fryorcraken fryorcraken added the need/triage Needs initial labeling and prioritization label Nov 21, 2022
@fryorcraken
Copy link
Contributor Author

Investigation so far:

I can see that addrs at

const addrs = await pipe(
contains the 0.0.0.0 ip.

I am logging it as soon as it is set:

On first dial:

15:29:15.620 libp2p:dialer using addresses:  +1ms 
Array [ "/ip4/127.0.0.1/tcp/8000/ws/p2p/16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem" ]
[common.js:108](http://localhost:3004/home/fryorcraken/src/libp2p/js-libp2p/node_modules/.pnpm/[email protected]/node_modules/debug/src/common.js)

On redial (by killing other libp2p node)

15:29:30.794 libp2p:dialer using addresses:  +3ms 
Array [ "/ip4/0.0.0.0/tcp/8000/ws/p2p/16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem" ]
[common.js:108](http://localhost:3004/home/fryorcraken/src/libp2p/js-libp2p/node_modules/.pnpm/[email protected]/node_modules/debug/src/common.js)

@fryorcraken
Copy link
Contributor Author

One step earlier,

15:35:23.503 libp2p:dialer redial: got from address book for  +2ms 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem /ip4/0.0.0.0/tcp/60000 common.js:108
15:35:23.503 libp2p:dialer redial: got from address book for  +1ms 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem /ip4/0.0.0.0/tcp/8000/ws common.js:108
15:35:33.526 libp2p:dialer redial: got from address book for  +2ms 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem /ip4/0.0.0.0/tcp/60000 common.js:108
15:35:33.527 libp2p:dialer redial: got from address book for  +1ms 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem /ip4/0.0.0.0/tcp/8000/ws

When logging what is returned by the address book at

(source) => filter(source, async (address) => {

So it means that the right address is never added to the address book (but it is present in the peer store when I do peerStore.all()).

@fryorcraken
Copy link
Contributor Author

More investigation:

  1. I use @libp2p/[email protected]
  2. When a peer is discovered, I use libp2p.dial but only pass the peer id

This adds the address to the address book (not sure how)

16:07:25.107 libp2p:peer-store:address-book added multiaddrs for 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem +0ms 
Array [ "/ip4/127.0.0.1/tcp/8000/ws/p2p/16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem" ]
  1. the peer store returns the right address in dialer.dial:
16:07:25.111 libp2p:peer-store:address-book got address for peer 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem +4ms 
Array [ "/ip4/127.0.0.1/tcp/8000/ws/p2p/16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem" ]
  1. Then, shortly after, it sets the multiaddrs to 0.0.0.0:
16:07:25.402 libp2p:peer-store:address-book set multiaddrs for 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem +290ms 
Array [ "/ip4/0.0.0.0/tcp/60000", "/ip4/0.0.0.0/tcp/8000/ws" ]
  1. Then, connection stops and it needs to get the multiaddr and gets 0.0.0.0:
16:09:54.156 libp2p:peer-store:address-book got address for peer 16Uiu2HAmB7Q24Cv6DWZEVfhqZGKri9hEKjXL2H1y48wVH6Xwumem +2m 
Array [ "/ip4/0.0.0.0/tcp/60000", "/ip4/0.0.0.0/tcp/8000/ws" ]

So I am guessing the issue is that something sets it to 0.0.0.0, maybe because the other peer returns 0.0.0.0 as part of identify protocol? Will investigate

Full logs: console-export-2022-11-21_16-16-16.txt

@fryorcraken
Copy link
Contributor Author

Ok, found the issue:

await this.components.peerStore.addressBook.set(id, listenAddrs.map((addr) => multiaddr(addr)))

As this is part of the identify protocol. It means that somehow we are already connected to the remote peer.

Hence, if there is no signed peer record, is it really best for a addressBook.set to be done, shouldn't it be addressBook.add to not override existing addresses?

Identify message:

16:25:22.030 libp2p:identify identify message +0ms 
Object { listenAddrs: (2) […], protocols: (6) […], publicKey: Uint8Array(37), observedAddr: Uint8Array(10), protocolVersion: "ipfs/0.1.0", agentVersion: "nwaku" }
agentVersion: "nwaku"
listenAddrs: Array [ Uint8Array(8), Uint8Array(10) ]
observedAddr: Uint8Array(10) [ 4, 127, 0, … ]
protocolVersion: "ipfs/0.1.0"
protocols: Array(6) [ "/ipfs/id/1.0.0", "/ipfs/ping/1.0.0", "/vac/waku/filter/2.0.0-beta1", … ]
publicKey: Uint8Array(37) [ 8, 2, 18, … ]

I believe I can solve my problem by changing the set to an add so that it does not override the existing address in the address book.

Also, I changed my handling of peer discovered by adding the mulitaddrs to the address book first:

    libp2p.addEventListener("peer:discovery", async (evt) => {
      const peerId = evt.detail.id;

      await libp2p.peerStore.addressBook.add(peerId, evt.detail.multiaddrs);

      log(`Found peer ${peerId.toString()}, dialing.`);
      libp2p.dial(peerId).catch((err) => {
        log(`Fail to dial ${peerId}`, err);
      });
    });

fryorcraken added a commit to fryorcraken/js-libp2p that referenced this issue Nov 21, 2022
Identify protocol being executed it means that both peers are somehow connected. Hence, it should not override working multiaddresses but only add to it.

Fixes libp2p#1484
@fryorcraken
Copy link
Contributor Author

Confirmed that no issue redialing with this fix: #1485

@achingbrain
Copy link
Member

Hence, if there is no signed peer record, is it really best for a addressBook.set to be done, shouldn't it be addressBook.add to not override existing addresses?

Signed peer records exist so I can (for example) as peer A, receive a record from peer B and give it to peer C. Since the record is signed by peer B's key, peer C does not need to trust me.

During identify you only receive addresses directly from the remote peer. It's not in the remote peer's interest to send you false addresses as you'll then not be able to dial it later on. Bad addresses could be part of the signed record too, of course.

Could you dig in to where the 0.0.0.0 address is coming from? It should get swapped out for a routable IP address once the port is open.

@fryorcraken
Copy link
Contributor Author

Thank you @achingbrain. I agree with your statement and that the isue is more likely to be the remote peer sending 0.0.0.0 in the listenAddrs of the identify message.

Other implementation is nwaku/nim-libp2p. Will follow-up on this side.

I can see that a signedPeerRecord is expected in the identify message but it's not part of the spec https://github.com/libp2p/specs/tree/master/identify am I looking at the wrong place?

@fryorcraken
Copy link
Contributor Author

Issue reported upstream: waku-org/nwaku#1427.

I am happy to close this or keep it open until it's 100% clarified but explanation provided for js-libp2p's behaviour seems fair.

@achingbrain achingbrain added need/author-input Needs input from the original author and removed need/triage Needs initial labeling and prioritization labels Nov 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

@github-actions
Copy link
Contributor

This issue was closed because it is missing author input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/stale need/author-input Needs input from the original author
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants