bug: canary fails on go-waku nodes due to libp2p upgrade

[jakubgs]

Problem

Recent the was and upgrade of go-libp2p upgrade in go-waku:

• chore: bump libp2p go-waku#556

Which appears to have broken compatibility with `wakucanary:

 > ./build/wakucanary --address=/dns4/node-01.do-ams3.go-waku.test.statusim.net/tcp/30303/p2p/16Uiu2HAm9vnvCQgCDrynDK1h7GJoEZVGvnuzq84RyDQ3DEdXmcX7 --log-level=WARN
ERR 2023-05-29 16:05:48.039+02:00 Could not connect                          tid=94576 file=wakucanary.nim:181 peerId=16U*dXmcX7
ERR 2023-05-29 16:05:48.040+02:00 The node has some problems (see logs)      tid=94576 file=wakucanary.nim:191

Which according to @richard-ramos is most probably due to multiaddress parsing incomapatibility:

DBG 2023-05-29 15:39:58.666+02:00 Not supported MultiAddress in blob         topics="libp2p multiaddress" tid=47452 file=multiaddress.nim:1138 ma="@[4, 172, 17, 1, 2, 6, 1, 187, 192, 3, 221, 3]"
DBG 2023-05-29 15:39:58.666+02:00 Not supported MultiAddress in blob         topics="libp2p multiaddress" tid=47452 file=multiaddress.nim:1138 ma="@[4, 134, 209, 134, 63, 6, 1, 187, 192, 3, 221, 3]"

Impact

All go-waku.test fleet canaries are red due to this issue:

https://canary.infra.status.im/service/156/

Making them useless.

To reproduce

If you can reproduce the behavior, steps to reproduce:

1. Build canary with make wakucanary
2. Run:

   ./build/wakucanary --address=/dns4/node-01.do-ams3.go-waku.test.statusim.net/tcp/30303/p2p/16Uiu2HAm9vnvCQgCDrynDK1h7GJoEZVGvnuzq84RyDQ3DEdXmcX7
   

3. See error
4. PROFIT

Expected behavior

It works.

[richard-ramos]

go-waku node is advertising the following addresses:

 "listenAddresses": [
      "/ip4/8.218.2.110/tcp/30303/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/127.0.0.1/tcp/443/tls/ws/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/127.0.0.1/tcp/30303/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/172.17.1.2/tcp/443/tls/ws/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/172.17.1.2/tcp/30303/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/dns4/node-01.ac-cn-hongkong-c.go-waku.test.statusim.net/tcp/30303/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/dns4/node-01.ac-cn-hongkong-c.go-waku.test.statusim.net/tcp/443/wss/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/8.218.2.110/tcp/443/wss/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/8.218.2.110/tcp/30303/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD",
      "/ip4/8.218.2.110/tcp/443/tls/ws/p2p/16Uiu2HAmBDbMWFiG9ki8sDw6fYtraSxo4oHU9HbuN43S2HVyq1FD"
    ]

I wonder if the problem is related to the addresses containing tls/ws, because tls is not part of the list of codecs in https://github.com/status-im/nim-libp2p/blob/unstable/libp2p/multiaddress.nim#L364

cc: @Menduist @diegomrsantos

[diegomrsantos]

Just to clarify. An upgrade in go-waku (which uses go-libp2p) makes nwaku nodes to have problems when parsing multi addresses?

[diegomrsantos]

@richard-ramos indeed seems tls isn't supported, @Menduist may talk more about it. But the current code returns an error only if all addresses can't be parsed. I believe the previous code was even more restrictive and would return an error if any address couldn't be parsed.

Does this help to understand the issue better somehow?

[richard-ramos]

Yes, it happened once go-libp2p was upgraded. (I had not see those tls/ws addresses being used before)

If current version is less restrictive, we probably need upgrade nwaku's nim-libp2p version. I did notice that if i returned true here: https://github.com/status-im/nim-libp2p/blob/d40d32416030e82947a0889c5467236805e5cbc6/libp2p/multiaddress.nim#L814-L816 i would connect succesfully to my node.

Thank you, @diegomrsantos!

[richard-ramos]

I believe the previous code was even more restrictive and would return an error if any address couldn't be parsed.

@diegomrsantos do you know in which commit/PR was this change done?

[diegomrsantos]

@richard-ramos this one vacp2p/nim-libp2p@0221aff

[richard-ramos]

I see. nwaku is using a version containing that change and fails. I ended up tracking the issue to these lines:
https://github.com/status-im/nim-libp2p/blob/unstable/libp2p/routing_record.nim#L54-L55

If we replace them by:

      let f = subProto.getField(1, addressInfo.address)
      if f.isOk() and f.get() == false:
          return err(ProtoError.RequiredFieldMissing)

then It works correctly, however, I do not know if this is the right way to fix this problem?

[diegomrsantos]

We could also use the code below to not add empty addresses:

if f.isOk():
  if f.get():
    record.addresses &= addressInfo
  else:
    return err(ProtoError.RequiredFieldMissing)

But in both versions, we silently ignore Result errors. What do you think @Menduist?

[Menduist]

@diegomrsantos we should be consistent between peer record & identify decoding (fail if all parsing failed, otherwise succeed)

[diegomrsantos]

vacp2p/nim-libp2p#898

[jm-clius]

I see the underlying issue has been closed. Do we consider this issue fixed if the submodule is updated on nwaku side? cc @diegomrsantos

[diegomrsantos]

@jm-clius could it be tested to see if it's enough to fix the issue in nwaku?

[SionoiS]

My comment didn't saved for some reason....

I could not reproduce with latest master.

[jm-clius]

Thanks @SionoiS