feat: resolve multiaddrs before dial #782

vasco-santos · 2020-10-13T09:18:26Z

This PR adds the ability to resolve multiaddrs before dialling. The multiaddr resolvers are customizable within the dialer configuration and come with a default for dnsaddr. At this moment, we do not resolve dns4 and dns6 per #772 (comment) . I will make sure to create an issue to track this.

Needs:

vasco-santos · 2020-10-13T09:19:24Z

src/dialer/index.js

+   * @param {Multiaddr} ma
+   * @returns {Promise<Array<Multiaddr>>}
+   */
+  async _resolve (ma) {


Once we support recursion in multiaddr, as well as dns4+dns6 resolution, we will not need this function, only the function below

jacobheun · 2020-10-13T12:38:17Z

src/dialer/index.js

+   */
+  async _resolve (ma) {
+    // TODO: recursive logic should live in multiaddr once dns4/dns6 support is in place
+    const resolvableProto = ma.protos().find((p) => p.resolvable)


I think we should simplify this for now and only resolve if ma.protoNames().includes('dnsaddr'). The checks below don't account for dns addresses. If new resolvable addresses are added, we may need to account for them further, so I would just simplify this for now.

yes, I agree!

vasco-santos · 2020-10-27T11:00:07Z

Just tested this out with a direct dial from libp2p: /dnsaddr/bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN and it resolved correctly to /dns4/sjc-1.bootstrap.libp2p.io/tcp/443/wss/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN. I could not test the examples from #772 at this moment, as without the multiaddr + mafmt releases, the bootstrap module uses different versions that I can link here. It should just work once we do the releases

jacobheun · 2020-10-27T12:46:16Z

src/dialer/index.js

+
+    const addrs = []
+    for (const a of knownAddrs) {
+      const resolvedAddrs = await this._resolve(a)


resolve can throw, we shouldn't let a single address failure stop the whole dial, we need to catch it

src/dialer/index.js

jacobheun · 2020-10-27T12:51:21Z

src/dialer/index.js

+      return [ma]
+    }
+
+    const resolvedMultiaddrs = await this._resolveRecord(ma)


This can throw which will stop the whole dial, we should let individual addresses fail.

jacobheun · 2020-10-27T12:52:15Z

test/dialing/resolver.spec.js

+  [`dnsaddr=${relayedAddr(peerId)}`]
+]
+
+describe('Dialing (resolvable addresses)', () => {


Add a test where the resolve throws for and address to ensure we can still succeed if others pass.

vasco-santos · 2020-10-30T11:47:13Z

I merged in @mburns PR and tested out the example with the new dnsaddr. We get the peers connected, but it seems we have some unreachable addresses?

vasco-santos · 2020-11-02T17:08:45Z

@mburns

dig +short TXT _dnsaddr.nyc-2.bootstrap.libp2p.io

"dnsaddr=/ip4/104.236.76.40/udp/4001/quic/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip4/104.236.76.40/tcp/443/wss/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip6/2604:a880:800:10::4a:5001/tcp/4001/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip6/2604:a880:800:10::4a:5001/udp/4001/quic/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip4/104.236.76.40/tcp/4001/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/dns6/nyc-2.bootstrap.libp2p.io/tcp/443/wss/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip6/2604:a880:800:10::4a:5001/tcp/443/wss/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/dns4/nyc-2.bootstrap.libp2p.io/tcp/443/wss/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"

Per #772 (comment) we should get the ip4 and ip6 addresses with ws out. These addresses will fail on the certificate validation. Regarding the unreachable addresses I am not sure about the issue

mburns · 2020-11-04T00:32:47Z

$ dig +short TXT _dnsaddr.nyc-2.bootstrap.libp2p.io
"dnsaddr=/dns4/nyc-2.bootstrap.libp2p.io/tcp/443/wss/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/dns6/nyc-2.bootstrap.libp2p.io/tcp/443/wss/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip4/104.236.76.40/tcp/4001/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip4/104.236.76.40/udp/4001/quic/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip6/2604:a880:800:10::4a:5001/tcp/4001/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"
"dnsaddr=/ip6/2604:a880:800:10::4a:5001/udp/4001/quic/p2p/QmSoLV4Bbm51jM9C4gDYZQ9Cy3U6aXMJDAbzgu2fzaDs64"

Still investigating the unreachable hosts, they should be back up shortly.

vasco-santos · 2020-11-04T09:18:03Z

Thanks @mburns ! Everything seems great not 🎉

jacobheun

LGTM just two minor nits

src/dialer/index.js

jacobheun · 2020-11-04T12:19:54Z

test/dialing/resolver.spec.js

+    let firstCall = false
+    let secondCall = false
+
+    const stub = sinon.stub(Resolver.prototype, 'resolveTxt')
+    stub.callsFake(() => {
+      if (!firstCall) {
+        firstCall = true
+        // Return an array of dnsaddr
+        return Promise.resolve(getDnsaddrStub(remoteId))
+      } else if (!secondCall) {
+        secondCall = true
+        // Address failed to resolve
+        return Promise.reject(new Error())
+      }
+      return Promise.resolve(getDnsRelayedAddrStub(remoteId))
+    })


You should be able to use sinon to simplify this a bit, something like:

Suggested change

let firstCall = false

let secondCall = false

const stub = sinon.stub(Resolver.prototype, 'resolveTxt')

stub.callsFake(() => {

if (!firstCall) {

firstCall = true

// Return an array of dnsaddr

return Promise.resolve(getDnsaddrStub(remoteId))

} else if (!secondCall) {

secondCall = true

// Address failed to resolve

return Promise.reject(new Error())

}

return Promise.resolve(getDnsRelayedAddrStub(remoteId))

})

const stub = sinon.stub(Resolver.prototype, 'resolveTxt')

stub.onCall(0).callsFake(() => Promise.resolve(getDnsaddrStub(remoteId)))

stub.onCall(1).callsFake(() => Promise.reject(new Error()))

stub.callsFake(() => Promise.resolve(getDnsRelayedAddrStub(remoteId)))

Nice, that works. I did not understand that I could use callsFake for the "other" calls, which was making things difficult 👍

Co-authored-by: Jacob Heun <[email protected]>

Adds the new `dnsaddr` multiaddrs for browser bootstrapping peers per libp2p/js-libp2p#772 and libp2p/js-libp2p#782

vasco-santos commented Oct 13, 2020

View reviewed changes

vasco-santos force-pushed the feat/resolve-multiaddrs-before-dial branch from 16a0032 to 79d2852 Compare October 13, 2020 09:19

vasco-santos requested a review from jacobheun October 13, 2020 09:51

jacobheun reviewed Oct 13, 2020

View reviewed changes

jacobheun reviewed Oct 27, 2020

View reviewed changes

vasco-santos force-pushed the feat/resolve-multiaddrs-before-dial branch from 5ae4f29 to 4e0a78e Compare October 27, 2020 14:40

feat: resolve multiaddrs before dial

3004543

vasco-santos force-pushed the feat/resolve-multiaddrs-before-dial branch from 4e0a78e to 2019f74 Compare October 28, 2020 09:45

chore: address review

389d0db

vasco-santos force-pushed the feat/resolve-multiaddrs-before-dial branch from 2019f74 to 389d0db Compare October 30, 2020 09:16

vasco-santos marked this pull request as ready for review October 30, 2020 10:18

chore: update example multiaddrs of boostrap nodes (#772)

336ee0c

vasco-santos force-pushed the feat/resolve-multiaddrs-before-dial branch from 3b3911b to 336ee0c Compare October 30, 2020 11:31

vasco-santos requested a review from jacobheun October 30, 2020 11:47

vasco-santos mentioned this pull request Nov 2, 2020

Not getting peers nor pubsub messages libp2p/js-libp2p-webrtc-star#268

Closed

jacobheun approved these changes Nov 4, 2020

View reviewed changes

chore: apply suggestions from code review

33fac9a

Co-authored-by: Jacob Heun <[email protected]>

vasco-santos force-pushed the feat/resolve-multiaddrs-before-dial branch from ff56987 to 33fac9a Compare November 4, 2020 12:36

vasco-santos merged commit 093c0ea into master Nov 4, 2020

vasco-santos deleted the feat/resolve-multiaddrs-before-dial branch November 4, 2020 12:55

vasco-santos mentioned this pull request Nov 5, 2020

chore: use dnsaddr for bootstrap nodes ipfs/js-ipfs#3370

Merged

achingbrain pushed a commit to ipfs/js-ipfs that referenced this pull request Nov 6, 2020

chore: use dnsaddr for bootstrap nodes (#3370)

7a0f1a2

Adds the new `dnsaddr` multiaddrs for browser bootstrapping peers per libp2p/js-libp2p#772 and libp2p/js-libp2p#782

feat: resolve multiaddrs before dial #782

feat: resolve multiaddrs before dial #782

Uh oh!

Conversation

vasco-santos commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasco-santos commented Oct 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasco-santos commented Oct 30, 2020

Uh oh!

vasco-santos commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mburns commented Nov 4, 2020

Uh oh!

vasco-santos commented Nov 4, 2020

Uh oh!

jacobheun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vasco-santos commented Oct 13, 2020 •

edited

Loading

vasco-santos commented Nov 2, 2020 •

edited

Loading