bug: missing `multiaddrs` in received response from peer-exchange and dns-discovery query

danisharora099 commented 1 year ago

Problem

Using the nwaku test fleet to do a peer-exchange query, I successfully receive new nodes. However, on decoding the bytes, I seem to be missing multiaddrs on these nodes, while they should include multiaddrs.

Update: same issue is also seen while receiving a dns-discovery peer

Impact

Blocker to enable peer-exchange js-waku as a discovery mechanism

To reproduce

Go to js-waku
Checkout to danisharora/fix-px-peer-discovery
cd packages/peer-exchange
run npx mocha and check dev console logs (I've left a few console.log statements in there to help with debugging)

Expected behavior

The enr bytes received in the peer-exchange response should have multiaddrs to be dialable by libp2p.

Screenshots/logs

Received peerIds and multiaddrs:

{
peerId: '16Uiu2HAmJb2e28qLXxT5kZxVUUoJt72EMzNGXB47Rxx5hw3q4YjS',
multiaddrs: undefined
}
{
peerId: '16Uiu2HAmBgXG2e29f7ontgKYxESWmwPXpqLCPdfXvGwKvCsKC8zA',
multiaddrs: undefined
}
{
peerId: '16Uiu2HAmBgXG2e29f7ontgKYxESWmwPXpqLCPdfXvGwKvCsKC8zA',
multiaddrs: undefined
}
{
peerId: '16Uiu2HAkvWiyFsgRhuJEb9JfjYxEkoHLgnUQmr1N5mKWnYjxYRVm',
multiaddrs: undefined
}

ENRs received:

enr:-JK4QAmmBzjI8YAKKT-9pH_-a5y-9lSjNyY8dNd45AW1M6BhbhPtfGDQrLD73ZjgRKqqmRRAvKiUmSA-5hKoZfDhPqwBgmlkgnY0gmlwhGia74CJc2VjcDI1NmsxoQNYImLJllfa9siWWAIjBhyX4y5S3qgLkwKJFBjzf_6ZMYN0Y3CCdl-DdWRwgiMohXdha3UyDw,

enr:-JK4QBcAt01zOZ-mu7JYvjNrYt2AGb2MZz_eH1ciG9vauuPbDwe1VkL5_XsnOyNYI_F-2iCtrPloeiG7e05q8dpm4bABgmlkgnY0gmlwhC_y0kmJc2VjcDI1NmsxoQIQJvj-KupljjSnBp2yz1PFl2SeELNwMSG68nL3Oz1SrIN0Y3CCdl-DdWRwgiMohXdha3UyDw,

enr:-JK4QK3z-AuQU_QGpYjj3G8qRKzFjRo4xdezGdLkqr5cJ6OGVLTatuUHjK2qaYdOHlztrVvFHDfAxJRntoKMYQhqQAABgmlkgnY0gmlwhIbRi9KJc2VjcDI1NmsxoQOevTdO6jvv3fRruxguKR-3Ge4bcFsLeAIWEDjrfaigNoN0Y3CCdl-DdWRwgiMohXdha3UyDw

sample decoded ENR:

{
ENR: ENR(6) [Map] {
'id' => Uint8Array(2) [ 118, 52 ],
'ip' => Uint8Array(4) [ 34, 225, 106, 190 ],
'secp256k1' => Uint8Array(33) [
    2, 241, 136, 215, 215, 186,  56, 243,
  231, 202, 171,  93, 153, 239,  39, 175,
  207, 234, 236, 227, 170, 234, 173, 212,
  151,  25,  69, 200,  75, 159, 246,  80,
   71
],
'tcp' => Uint8Array(2) [ 234, 96 ],
'udp' => Uint8Array(2) [ 35, 40 ],
'waku2' => Uint8Array(1) [ 1 ],
signature: Uint8Array(64) [
   50,  78,  19,  66, 195, 108, 202, 183,  17, 202, 123,
  243, 173, 190,  14, 177, 216, 166, 190,  81, 125, 126,
   12,  63, 130,  84, 255, 127, 160,  50,   1,  99,  12,
  226,  67,  29, 130,  51,  59,  94, 236, 200,  85,  59,
   22, 192, 154, 241,  29, 137, 144, 149,  65, 144, 108,
   94,  14, 101, 234,  57, 172,  36, 154,  77
],
seq: 1n,
peerId: Secp256k1PeerIdImpl [PeerId(16Uiu2HAmBgXG2e29f7ontgKYxESWmwPXpqLCPdfXvGwKvCsKC8zA)] {
  type: 'secp256k1',
  multihash: [Digest],
  privateKey: undefined,
  publicKey: <Buffer 08 02 12 21 02 f1 88 d7 d7 ba 38 f3 e7 ca ab 5d 99 ef 27 af cf ea ec e3 aa ea ad d4 97 19 45 c8 4b 9f f6 50 47>
}
}
}

nwaku version/commit hash

v0.13.0

Additional context

Query was made at 11:20 AM IST
@fryorcraken pointed that the nodes received are also test fleet nodes, with websockets enabled.

fryorcraken commented 1 year ago

@danisharora099 please provide:

what node was queried
the query
hex string of the ENRs in the response

danisharora099 commented 1 year ago

Node queried is the test fleet
It's a peer-exchange query requesting for 10 as numPeers

hex string of one of the ENRs received:

f892b840adf3f80b9053f406a588e3dc6f2a44acc58d1a38c5d7b319d2e4aabe5c27a38654b4dab6e5078cadaa69874e1e5cedad5bc51c37c0c49467b6828c61086a4000018269648276348269708486d18bd289736563703235366b31a1039ebd374eea3befddf46bbb182e291fb719ee1b705b0b7802161038eb7da8a0368374637082765f837564708223288577616b75320f

fryorcraken commented 1 year ago

Node queried is the test fleet There are several nodes in the test fleet, would be good to provide a peer id.

danisharora099 commented 1 year ago

Node queried is the test fleet There are several nodes in the test fleet, would be good to provide a peer id.

Few of the peer IDs in test fleet queried: 12D3KooWRoeQEUnLJo6x2uGpwuN11gEXZF97m7kxhAM46VAFWKSe, 16Uiu2HAmPLe7Mzm8TsYUubgCAW1aJoeFScxrLj8ppHFivPo97bUZ

danisharora099 commented 1 year ago

Update: I've been able to reproduce the same issue in dns-discovery as well: https://github.com/waku-org/js-waku/pull/1084

Acc to my interpretation, there are two possibilities:

ENR isn't parsed properly in js-waku (seems more probable at this point? in which case, we'll move this issue to js-waku)
multiaddr isn't sent properly by nwaku in the query response

fryorcraken commented 1 year ago

1. ENR isn't parsed properly in js-waku (seems more probable at this point? in which case, we'll move this issue to `js-waku`)

Locally I can confirm that nwaku v0.13 does include multiaddrs in ENR:

▶ git diff
diff --git a/packages/tests/tests/enr.node.spec.ts b/packages/tests/tests/enr.node.spec.ts
index 415e91aa..0d897e2f 100644
--- a/packages/tests/tests/enr.node.spec.ts
+++ b/packages/tests/tests/enr.node.spec.ts
@@ -24,6 +24,7 @@ describe("ENR Interop: nwaku", function () {
       store: false,
       filter: false,
       lightpush: false,
+      websocketSupport: true,
     });
     const multiAddrWithId = await nwaku.getMultiaddrWithId();

@@ -39,6 +40,7 @@ describe("ENR Interop: nwaku", function () {

     expect(nwakuInfo.enrUri).to.not.be.undefined;
     const dec = await ENR.decodeTxt(nwakuInfo.enrUri ?? "");
+    console.log(dec.multiaddrs?.map((ma) => ma.toString()));
     expect(dec.peerId?.toString()).to.eq(nimPeerId.toString());
     expect(dec.waku2).to.deep.eq({
       relay: true,

output:

[ '/ip4/127.0.0.1/tcp/8005/ws' ]

danisharora099 commented 1 year ago

Update: upon looking into why multiaddrs aren't being parsed correctly for the original issue/ENRs, it was seen that those ENRs don't contain decoded fields called multiaddrs while the ones retrieved from nwakuInfo.enrUri locally started nwaku from js-waku does contain that.

The ENRs sent from the fleet do contain tcp4 and udp4 multiaddrs, but the current ENR parser in js-waku looks for multiaddrs field which leads it to return undefined

I'm unsure why the inconsistencies with the ENR serialization exist, and which is the correct standard, and would appreciate some direction on that.

Some ENRs to decode/look into to better understand the above:

locally spun nwaku node: enr:-KO4QGYWs6ATSXyvAf3s9o42HQSAo8xSrVosWf5TOgHnsz3WBBH7ttwqeUiePhVCkhOWVLBNyBV4MM5GCer9Xj6y0tQBgmlkgnY0gmlwhH8AAAGKbXVsdGlhZGRyc4wACgR_AAABBh9C3QOJc2VjcDI1NmsxoQJoE1OnRsVkx7oLeA9ufq_EnKzCPyDy-gzHwn4HNf7_XIN0Y3CCH0GFd2FrdTIB
- multiaddrs
- tcp4
one of the ENRs received received back from the fleet: enr:-JK4QAmmBzjI8YAKKT-9pH_-a5y-9lSjNyY8dNd45AW1M6BhbhPtfGDQrLD73ZjgRKqqmRRAvKiUmSA-5hKoZfDhPqwBgmlkgnY0gmlwhGia74CJc2VjcDI1NmsxoQNYImLJllfa9siWWAIjBhyX4y5S3qgLkwKJFBjzf_6ZMYN0Y3CCdl-DdWRwgiMohXdha3UyDw
- tcp4
- udp4

https://enr-viewer.com/ can be used to easily decode/visualise decoded ENRs

cc @fryorcraken

fryorcraken commented 1 year ago

Investigation seems fair to me. @kaiserd are you able to take a look?

alrevuelta commented 1 year ago

I would say its not a bug but a feature. As per the WAKU2-ENR spec multiaddrs section:

To save space, multiaddrs key SHOULD only be used for connection details that cannot be represented using the EIP-778 pre-defined keys.

Which I assume its websockets.

I can confirm that a nwaku node that doesn't enable websocket-support=true flag, does not contain any multiaddrs , which is expected since this code just checks if websockets is enabled. Also the comment "# Only add ws/wss to multiaddrs field" makes me think its expected.

So long story short, a node not supporting websockets, will have its multiaddrs section empty.

fryorcraken commented 1 year ago

I can confirm that a nwaku node that doesn't enable websocket-support=true flag, does not contain any multiaddrs

Except that some of the nodes returned in the issue description are nodes from the wakuv2 fleet, where all nodes have websocket enabled.

danisharora099 commented 1 year ago

update:

enr:-Nm4QLHYoJ5WYQoVzyqPR-pwIeQvi3ONWs-EPwk3uUiBiDseN9Dd7fYbCvkMdeXcuZ-8U9IYdGm38VxSDf_Oq3zZ0cEBgmlkgnY0gmlwhGia74CKbXVsdGlhZGRyc7g6ADg2MW5vZGUtMDEuZ2MtdXMtY2VudHJhbDEtYS53YWt1djIudGVzdC5zdGF0dXNpbS5uZXQGH0DeA4lzZWNwMjU2azGhA1giYsmWV9r2yJZYAiMGHJfjLlLeqAuTAokUGPN__pkxg3RjcIJ2X4N1ZHCCIyiFd2FrdTIP

and

enr:-JK4QAmmBzjI8YAKKT-9pH_-a5y-9lSjNyY8dNd45AW1M6BhbhPtfGDQrLD73ZjgRKqqmRRAvKiUmSA-5hKoZfDhPqwBgmlkgnY0gmlwhGia74CJc2VjcDI1NmsxoQNYImLJllfa9siWWAIjBhyX4y5S3qgLkwKJFBjzf_6ZMYN0Y3CCdl-DdWRwgiMohXdha3UyDw

point to the same IP/node ID/peer ID, and are different ENRs.

The difference is: the first one is picked directly from https://fleets.status.im and contains multiaddrs, while the second one was parsed and derived from the enrtree: AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM:test.waku.nodes.status.im and does not contain multiaddrs.

cc @fryorcraken @alrevuelta

alrevuelta commented 1 year ago

Looking into the code, nwaku peer exchange just sends the enr it gets from discv5, so perhaps the issue is not in pex but in the enrs stored in dns disc.

Using nwaku, if I use the following dns disc url, I get the following enrs. Note that all of them contain the multiaddress field. enrtree://AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM@prod.nodes.status.im

enr:-M-4QHL_casP1Jy4KntHNWT3p1XkPxm1BJSxDi7KucSqZ2PgT97d4xEQ4cJx-bgw0SRu-nO4y5k0jTQN4AH7utodtZMBgmlkgnY0gmlwhKEj9HmKbXVsdGlhZGRyc7EALzYobm9kZS0wMi5kby1hbXMzLnN0YXR1cy5wcm9kLnN0YXR1c2ltLm5ldAYBu94DiXNlY3AyNTZrMaED1AYI2Ox27DnSqf2qoih5M2fNpHFq-OzJ3thREEApdiiDdGNwgnZfg3VkcIIjKIV3YWt1Mg8

enr:-Nm4QHesfm1iRyZHJ8QXrk8kHm6CrgI_oYRfFY8hdoqeqo9bczZeQ2pHZ05AS6XJp0qs_LKX96SxbC50L_LX9QIJ_pABgmlkgnY0gmlwhC_zgIaKbXVsdGlhZGRyc7g6ADg2MW5vZGUtMDIuYWMtY24taG9uZ2tvbmctYy5zdGF0dXMucHJvZC5zdGF0dXNpbS5uZXQGAbveA4lzZWNwMjU2azGhAzE4YIBrPK5oT_9MvYEeggmaDd205iU4Nn1MAhU5xqY5g3RjcIJ2X4N1ZHCCIyiFd2FrdTIP

enr:-Nm4QEermmt-_nFLc3xvWS2Lqboi2hgZbRY514NuzsLVTaQtXYQBm3PRZq16pdo03XjwLFeuSzvoTS8FAs-cswaqTSoBgmlkgnY0gmlwhCPKN5mKbXVsdGlhZGRyc7g6ADg2MW5vZGUtMDEuZ2MtdXMtY2VudHJhbDEtYS5zdGF0dXMucHJvZC5zdGF0dXNpbS5uZXQGAbveA4lzZWNwMjU2azGhAhoqdPkAZNvNRuzEecDGwnt5UbF98qwM16PZG3JmW6kUg3RjcIJ2X4N1ZHCCIyiFd2FrdTIP

enr:-Nm4QLBE-XE_GISWc8vEonOMRWSGVZ9TUbwlvauBmN7mc_V_bH9gGUspPiN6tPZSKG0XI2P1W50LutJdeZvFhXMuDoUBgmlkgnY0gmlwhC_yyjuKbXVsdGlhZGRyc7g6ADg2MW5vZGUtMDEuYWMtY24taG9uZ2tvbmctYy5zdGF0dXMucHJvZC5zdGF0dXNpbS5uZXQGAbveA4lzZWNwMjU2azGhAgwDLi50TMXqxWTCnR_rjQP3Eeznjs645ZNBoRh5B3jhg3RjcIJ2X4N1ZHCCIyiFd2FrdTIP

enr:-M-4QOth4Dg45mYtbfMf3YZOLaAVrQuNWEyb-rahElFJHeBkCTXe0AMPXO_XtT05UK3_v6nEfQOLWaVGt6WUsM_BpA0BgmlkgnY0gmlwhI_G-a6KbXVsdGlhZGRyc7EALzYobm9kZS0wMS5kby1hbXMzLnN0YXR1cy5wcm9kLnN0YXR1c2ltLm5ldAYBu94DiXNlY3AyNTZrMaECoVyonsTGEQvVioM562Q1fjzTb_vKD152PPIdsV7sM6SDdGNwgnZfg3VkcIIjKIV3YWt1Mg8

enr:-Nm4QEM3eNGY4ZLO30zZkhgyXhKowmX6_rJCSoQlUS7-WBs9LmoM2DtKEMOzxkwF1c1WVojeXdcn8eBI-XVRHeMP66UBgmlkgnY0gmlwhCKE1emKbXVsdGlhZGRyc7g6ADg2MW5vZGUtMDIuZ2MtdXMtY2VudHJhbDEtYS5zdGF0dXMucHJvZC5zdGF0dXNpbS5uZXQGAbveA4lzZWNwMjU2azGhAwtASPpiz5Gq3zuFuWF4MTviQVipi3HaQGytF-4wfbdvg3RjcIJ2X4N1ZHCCIyiFd2FrdTIP

But using the test.waku fleet, none of them contain the multiaddress field.

enrtree://AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM@test.waku.nodes.status.im

enr:-JK4QBcAt01zOZ-mu7JYvjNrYt2AGb2MZz_eH1ciG9vauuPbDwe1VkL5_XsnOyNYI_F-2iCtrPloeiG7e05q8dpm4bABgmlkgnY0gmlwhC_y0kmJc2VjcDI1NmsxoQIQJvj-KupljjSnBp2yz1PFl2SeELNwMSG68nL3Oz1SrIN0Y3CCdl-DdWRwgiMohXdha3UyDw

enr:-JK4QAmmBzjI8YAKKT-9pH_-a5y-9lSjNyY8dNd45AW1M6BhbhPtfGDQrLD73ZjgRKqqmRRAvKiUmSA-5hKoZfDhPqwBgmlkgnY0gmlwhGia74CJc2VjcDI1NmsxoQNYImLJllfa9siWWAIjBhyX4y5S3qgLkwKJFBjzf_6ZMYN0Y3CCdl-DdWRwgiMohXdha3UyDw

enr:-JK4QK3z-AuQU_QGpYjj3G8qRKzFjRo4xdezGdLkqr5cJ6OGVLTatuUHjK2qaYdOHlztrVvFHDfAxJRntoKMYQhqQAABgmlkgnY0gmlwhIbRi9KJc2VjcDI1NmsxoQOevTdO6jvv3fRruxguKR-3Ge4bcFsLeAIWEDjrfaigNoN0Y3CCdl-DdWRwgiMohXdha3UyDw

I can confirm what @fryorcraken said that wakuv2.test infra has websockets activates.

So I would say the enrs stored in enrtree://AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM@test.waku.nodes.status.im are wrong (perhaps outdated and were generated before enabling websockets)?

kaiserd commented 1 year ago

I have nothing to add to @alrevuelta 's answer. Thanks :). @jm-clius do you know why the testfleet enrtree ENRs do not contain multiaddresses?

danisharora099 commented 1 year ago

Summarising: the conclusion so far is that the ENRs stored in enrtree://AOGECG2SPND25EEFMAJ5WF3KSGJNSGV356DSTL2YVLLZWIV6SAYBM@test.waku.nodes.status.im are outdated.

jm-clius commented 1 year ago

Thanks for reporting this @danisharora099.

Note that this issue has been reported before, so I'm closing that one as a duplicate: https://github.com/waku-org/nwaku/issues/1323

I've also opened a relevant infra issue to update these lists: https://github.com/status-im/infra-nim-waku/issues/64

danisharora099 commented 1 year ago

Superseded by https://github.com/status-im/infra-nim-waku/issues/64

Thanks all!

waku-org / nwaku