From d34642db1c2be39a74fe7cf21508eb17c19c8a22 Mon Sep 17 00:00:00 2001 From: Alex Potsides Date: Mon, 28 Oct 2024 08:07:36 +0000 Subject: [PATCH] fix: only update pending incoming connections if connection accepted (#2790) * fix: only update pending incoming connections if connection accepted The `afterUpgradeInbound` function decrements the count of pending (e.g. not upgraded yet) incoming connections, so only invoke that method if we actually accepted the connection, otherwise we decrement a counter that was never incremented. * chore: helpful to include implementation --- packages/libp2p/src/upgrader.ts | 14 ++++++++------ .../libp2p/test/upgrading/upgrader.spec.ts | 19 ++++++++++++++++++- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/packages/libp2p/src/upgrader.ts b/packages/libp2p/src/upgrader.ts index 753185f742..c263e11d5c 100644 --- a/packages/libp2p/src/upgrader.ts +++ b/packages/libp2p/src/upgrader.ts @@ -182,22 +182,22 @@ export class DefaultUpgrader implements Upgrader { * Upgrades an inbound connection */ async upgradeInbound (maConn: MultiaddrConnection, opts: UpgraderOptions = {}): Promise { + let accepted = false + try { this.metrics.dials?.increment({ inbound: true }) - const accept = await this.components.connectionManager.acceptIncomingConnection(maConn) + accepted = await this.components.connectionManager.acceptIncomingConnection(maConn) - if (!accept) { + if (!accepted) { throw new ConnectionDeniedError('connection denied') } await this.shouldBlockConnection('denyInboundConnection', maConn) - const conn = await this._performUpgrade(maConn, 'inbound', opts) - - return conn + return await this._performUpgrade(maConn, 'inbound', opts) } catch (err) { this.metrics.errors?.increment({ inbound: true @@ -205,7 +205,9 @@ export class DefaultUpgrader implements Upgrader { throw err } finally { - this.components.connectionManager.afterUpgradeInbound() + if (accepted) { + this.components.connectionManager.afterUpgradeInbound() + } } } diff --git a/packages/libp2p/test/upgrading/upgrader.spec.ts b/packages/libp2p/test/upgrading/upgrader.spec.ts index 7c55362f69..8f4c8d88bb 100644 --- a/packages/libp2p/test/upgrading/upgrader.spec.ts +++ b/packages/libp2p/test/upgrading/upgrader.spec.ts @@ -29,7 +29,8 @@ import { type Components, defaultComponents } from '../../src/components.js' import { createLibp2p } from '../../src/index.js' import { DEFAULT_MAX_OUTBOUND_STREAMS } from '../../src/registrar.js' import { DefaultUpgrader } from '../../src/upgrader.js' -import type { Libp2p, Connection, ConnectionProtector, Stream, ConnectionEncrypter, SecuredConnection, PeerId, StreamMuxer, StreamMuxerFactory, StreamMuxerInit, Upgrader, PrivateKey } from '@libp2p/interface' +import type { Libp2p, Connection, ConnectionProtector, Stream, ConnectionEncrypter, SecuredConnection, PeerId, StreamMuxer, StreamMuxerFactory, StreamMuxerInit, Upgrader, PrivateKey, MultiaddrConnection } from '@libp2p/interface' +import type { ConnectionManager } from '@libp2p/interface-internal' const addrs = [ multiaddr('/ip4/127.0.0.1/tcp/0'), @@ -596,6 +597,22 @@ describe('Upgrader', () => { expect(localConnectionProtector.protect.callCount).to.equal(0, 'used local connection protector') expect(remoteConnectionProtector.protect.callCount).to.equal(0, 'used remote connection protector') }) + + it('should not decrement inbound pending connection count if the connection is denied', async () => { + const connectionManager = stubInterface() + + // @ts-expect-error private field + localUpgrader.components.connectionManager = connectionManager + + const maConn = stubInterface() + + connectionManager.acceptIncomingConnection.resolves(false) + + await expect(localUpgrader.upgradeInbound(maConn)).to.eventually.be.rejected + .with.property('name', 'ConnectionDeniedError') + + expect(connectionManager.afterUpgradeInbound.called).to.be.false() + }) }) describe('libp2p.upgrader', () => {