PeerStore Merge Not Preserving Existing Protocols on Update

[danisharora099]

Version: libp2p@0.45.4

Platform: MacOS

Subsystem: Upgrader/PeerStore

Severity: High

Description:
While testing my code with upgraded libp2p@0.45.4., I encountered an issue where the protocol array in the peer store was not behaving as expected. While I expect that a new protocol should be appended to the existing list of protocols for a given peer, it appears that instead the new protocol is replacing the old entries.

Here is the test code:

nwaku = new NimGoNode(makeLogFileName(this));
    await nwaku.start({
      filter: true,
      lightpush: true,
      store: true,
      relay: true,
    });
    waku = await createLightNode({
      staticNoiseKey: NOISE_KEY_1,
      libp2p: { addresses: { listen: ["/ip4/0.0.0.0/tcp/0/ws"] } },
    });
    await waku.start();
    await waku.dial(await nwaku.getMultiaddrWithId());

    await waitForRemotePeer(waku, [
      Protocols.Filter,
      Protocols.LightPush,
      Protocols.Store,
    ]);

    console.log((await waku.libp2p.peerStore.all()).map((p) => p.protocols));

This can be run in an environment through this commit message: waku-org/js-waku@e0f7af3

This test code is throwing an error: Error: Failed to find known peer that registers protocols: /vac/waku/filter-subscribe/2.0.0-beta1. It seems that only one protocol is registered at a time according to the output of console.log((await waku.libp2p.peerStore.all()).map((p) => p.protocols));

However, I expect the peer to have multiple protocols registered: Filter, LightPush, and Store.
I found that changing the upstream libp2p code with upgrader.ts (https://github.com/libp2p/js-libp2p/blob/41641f1a7656aa654234f6f849b1749786867121/src/upgrader.ts#LL465C11-L467C13) to the following fixed this issue.

await this.components.peerStore.merge(remotePeer, {
            protocols: [...protocols, protocol]
          })

Steps to reproduce the error:

• Run the above test code (through the commit message)
  • clone the repo
  • npm i && npm run build
  • cd packages/tests && npm run test:node
• Observe the mentioned error
• Check the registered protocols for a peer and observe that only the latest one is present.

Note: This is part of a larger migration PR for js-waku from v0.42 to 0.45.4: waku-org/js-waku#1385

cc @achingbrain

Tasks

Beta Give feedback

No tasks being tracked yet.

[danisharora099]

Happy to open a PR for this hotfix but I think this is some unintended behaviour for merge?

[achingbrain]

Thanks for spotting this issue. .peerStore.merge does exactly that, it deep-merges all passed fields, see the test for this here.

I've dug into it a little and it seems the remote peer does not send a signed peer record during identify, this hits this branch and so the sent protocols are never persisted.

This is a bug as we should persist the identify information, but replace any addresses with those from the signed peer record if it's present.

TBH I thought all libp2p implementations sent signed peer records by now - what is the remote peer type in this test?

[danisharora099]

Right. I am testing against nwaku as the remote peer - a nim implementation of waku (https://github.com/waku-org/nwaku/) which does not seem to have sending signed peer records by default but I can see some cases where it is implemented: https://github.com/waku-org/nwaku/blob/fcd7f33d9b7b1b7ff0857d8b5318a2dd3fe5e6aa/apps/wakunode2/app.nim#L458

I will check with the nwaku team re the same.

Judging by

This is a bug as we should persist the identify information, but replace any addresses with those from the signed peer record if it's present.

seems like in the interim, we can expect js-libp2p to still work when there is not a signedPeerRecord available since it is still optional at the API level?

Is this something we can expect a fixed release for, soon, as this is currently a blocker for our upgrade to 0.45.4

Thanks for looking into it @achingbrain

[achingbrain]

Yes, I'll get a fix out very soon.

[Menduist]

TBH I thought all libp2p implementations sent signed peer records by now

I'm pretty sure this is disabled on most, if not all eth consensus clients (which use a very small/safe subset of libp2p features)

[Marcel-G]

I was about to open an issue but I believe this covers my issue, I'll add the details here. Please let me know if it's unrelated.

I'm observing that rust-libp2p can no longer be used as a relay connection with js-libp2p since v0.45.0.

Steps to Reproduce

Run the rust-libp2p relay using any transport. Example using tcp will do https://github.com/libp2p/rust-libp2p/tree/master/examples/relay-server

Run the js-libp2p client which dials the relay node.

import { multiaddr } from '@multiformats/multiaddr' 
import { createLibp2p } from 'libp2p'
import { circuitRelayTransport } from 'libp2p/circuit-relay'
import { tcp } from '@libp2p/tcp'
import { mplex } from '@libp2p/mplex'
import { yamux } from '@chainsafe/libp2p-yamux'
import { noise } from '@chainsafe/libp2p-noise'
import { identifyService } from 'libp2p/identify'

export async function privateLibp2pNode () {
 const node = await createLibp2p({
   addresses: {
     listen: ['/ip4/0.0.0.0/tcp/0']
   },
   transports: [
     tcp(),
     circuitRelayTransport({
       discoverRelays: 1,
     }),
   ],
   streamMuxers: [yamux(), mplex()],
   connectionEncryption: [noise()],
   peerDiscovery: [],
   services: {
     identify: identifyService()
   }
 })

 return node
} 

(async () => {
 const node = await privateLibp2pNode()

 console.log(node.peerId)

 node.addEventListener("peer:identify", (event) => {
   const identify = event.detail;
   console.log('peer:identify', identify.peerId, identify.protocols)
 })

 node.addEventListener("self:peer:update", (event) => {
   console.log('self:peer:update', node.getMultiaddrs())
 })
 const ma = multiaddr('<addr of the rust-libp2p relay>')

 await node.dial(ma)
})()

Actual Result

Relayed address is not listed on peer:update

self:peer:update [
 Multiaddr(/ip4/127.0.0.1/tcp/58971/p2p/12D3KooWEcek3uKFuLasuAMcHTMzv5rxHqTm4W3YrQMjSjNFQtAs),
 Multiaddr(/ip4/192.168.178.106/tcp/58971/p2p/12D3KooWEcek3uKFuLasuAMcHTMzv5rxHqTm4W3YrQMjSjNFQtAs)
]

Expected Result

Peer update includes the relayed address on peer:update

self:peer:update [
 Multiaddr(/ip4/127.0.0.1/tcp/59802/p2p/12D3KooWFyhdyjdcxWyFeQtcVJCDW3mkja7jyRZ52cYNuhKPeM65),
 Multiaddr(/ip4/192.168.178.106/tcp/59802/p2p/12D3KooWFyhdyjdcxWyFeQtcVJCDW3mkja7jyRZ52cYNuhKPeM65),
 Multiaddr(/ip4/127.0.0.1/tcp/54336/p2p/12D3KooWCNibjstG4xDuh5ybRN8knVoYfGD7kuinJYEyaHVvob5d/p2p-circuit/p2p/12D3KooWFyhdyjdcxWyFeQtcVJCDW3mkja7jyRZ52cYNuhKPeM65),
 Multiaddr(/ip4/192.168.178.106/tcp/54336/p2p/12D3KooWCNibjstG4xDuh5ybRN8knVoYfGD7kuinJYEyaHVvob5d/p2p-circuit/p2p/12D3KooWFyhdyjdcxWyFeQtcVJCDW3mkja7jyRZ52cYNuhKPeM65)
]

rust-libp2p identify protocol does not implement signedPeerRecord libp2p/rust-libp2p#4017
There appears to be an undocumented breaking change between js-libp2p v0.44.0 & v0.45.0 which requires signedPeerRecord for this to work.

[maschad]

I was about to open an issue but I believe this covers my issue, I'll add the details here. Please let me know if it's unrelated.

I'm observing that rust-libp2p can no longer be used as a relay connection with js-libp2p since v0.45.0.

Steps to Reproduce
rust-libp2p identify protocol does not implement signedPeerRecord libp2p/rust-libp2p#4017 There appears to be an undocumented breaking change between js-libp2p v0.44.0 & v0.45.0 which requires signedPeerRecord for this to work.

Good catch @Marcel-G thanks for spotting this. I've merged @achingbrain 's fix #1790 which should be released in v0.45.5

[achingbrain]

v0.45.5 has been released so this should be fixed after deleting node_modules and any lock files and reinstalling your deps.