From afaee4c3e11c5c66da52afc1c0c40f1d55f02928 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <oss@bajtos.net>
Date: Thu, 9 Mar 2023 15:36:35 +0100
Subject: [PATCH] fix: accept two incoming PING streams per peer (#1617)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Modify the default configuration for the PING protocol to accept at most
two streams per peer, as recommended in the PING protocol spec.

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
---
 src/config.ts          |  8 +++++++-
 test/ping/ping.node.ts | 30 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/src/config.ts b/src/config.ts
index 925b2f105f..95ebd242a6 100644
--- a/src/config.ts
+++ b/src/config.ts
@@ -79,7 +79,13 @@ const DefaultConfig: Partial<Libp2pInit> = {
   },
   ping: {
     protocolPrefix: 'ipfs',
-    maxInboundStreams: 1,
+    // See https://github.com/libp2p/specs/blob/d4b5fb0152a6bb86cfd9ea/ping/ping.md?plain=1#L38-L43
+    // The dialing peer MUST NOT keep more than one outbound stream for the ping protocol per peer.
+    // The listening peer SHOULD accept at most two streams per peer since cross-stream behavior is
+    // non-linear and stream writes occur asynchronously. The listening peer may perceive the
+    // dialing peer closing and opening the wrong streams (for instance, closing stream B and
+    // opening stream A even though the dialing peer is opening stream B and closing stream A).
+    maxInboundStreams: 2,
     maxOutboundStreams: 1,
     timeout: 10000
   },
diff --git a/test/ping/ping.node.ts b/test/ping/ping.node.ts
index a92479ea16..c54b3a2b07 100644
--- a/test/ping/ping.node.ts
+++ b/test/ping/ping.node.ts
@@ -75,4 +75,34 @@ describe('ping', () => {
 
     defer.resolve()
   })
+
+  it('allows two incoming streams from the same peer', async () => {
+    const remote = nodes[0]
+    const client = await createNode({
+      config: createBaseOptions({
+        ping: {
+        // Allow two outbound ping streams.
+        // It is not allowed by the spec, but this test needs to open two concurrent streams.
+          maxOutboundStreams: 2
+        }
+      })
+    })
+    await client.components.peerStore.addressBook.set(remote.peerId, remote.getMultiaddrs())
+    // register our new node for shutdown after the test finishes
+    // otherwise the Mocha/Node.js process never finishes
+    nodes.push(client)
+
+    // Send two ping requests in parallel, this should open two concurrent streams
+    const results = await Promise.allSettled([
+      client.ping(remote.peerId),
+      client.ping(remote.peerId)
+    ])
+
+    // Verify that the remote peer accepted both inbound streams
+    expect(results.map(describe)).to.deep.equal(['fulfilled', 'fulfilled'])
+
+    function describe (result: PromiseSettledResult<number>): string {
+      return result.status === 'fulfilled' ? result.status : result.reason ?? result.status
+    }
+  })
 })