feat!: split identify and identify-push #2387

wemeetagain · 2024-02-01T22:42:41Z

Description

Add two options to the identify module to disable various parts of identify-push

runOnSelfUpdate - set to false to stop sending identify-push messages automatically on self:peer:update events
disableIdentifyPush - set to true to disable identify-push entirely, this protocol isn't registered and identify-push messages can't be sent

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation if necessary (this includes comments as well)
I have added tests that prove my fix is effective or that my feature works

achingbrain · 2024-02-02T08:14:50Z

Could these options be consolidated?

It would avoid the situation where a user sets both runOnSelfUpdate and disableIdentifyPush to true and expects something to happen.

Also, can you add some tests to ensure there are no regressions?

wemeetagain · 2024-02-02T15:51:59Z

IMO they are two separate features.
One stops the outbound messages that automatically happen (but still allows manual outbound messages and allows handling inbound messages).
One stops the handling of inbound messages.

could we split the identify module into two? Then we wouldn't need the disable feature.
like:

createLibp2p({
  services: {
    identify: identify({...}),
    identifyPush: identifyPush({...}),
  }
})

achingbrain · 2024-02-05T15:14:57Z

could we split the identify module into two?

This sounds like a good solution since it's a separate protocol, it could be exported from @libp2p/identify for convenience though since most people would want both most of the time.

BREAKING CHANGE: identifyPush functionality has been removed from the identify module. In order to get this functionality, the identifyPush module must be instantiated separately.

SgtPooki

some comments. Not sure how closely related these are but since the original proposal was a config, I assume they're both largely the same under the hood, in which case a base class they both extend could make the code much more manage-able, and less likely to fork in breaking ways when fixing bugs

SgtPooki · 2024-02-05T22:42:52Z

packages/protocol-identify/src/index.ts

+export interface IdentifyPushComponents {
+  peerId: PeerId
+  peerStore: PeerStore
+  connectionManager: ConnectionManager
+  registrar: Registrar
+  addressManager: AddressManager
+  events: TypedEventTarget<Libp2pEvents>
+  logger: ComponentLogger
+  nodeInfo: NodeInfo
 }


Is IdentifyPush expected to be a super-set of Identify?

Suggested change

export interface IdentifyPushComponents {

peerId: PeerId

peerStore: PeerStore

connectionManager: ConnectionManager

registrar: Registrar

addressManager: AddressManager

events: TypedEventTarget<Libp2pEvents>

logger: ComponentLogger

nodeInfo: NodeInfo

}

export interface IdentifyPushComponents extends IdentifyComponents {

connectionManager: ConnectionManager

}

SgtPooki · 2024-02-05T22:44:04Z

packages/protocol-identify/src/index.ts

+  /**
+   * How long we should wait for a remote peer to send their identify response
+   */
+  timeout?: number


this seems like an "identify-listen" capability. Does this belong in identifyPush? Maybe IdentifyPush needs a different name?

Identify/push is the name of the protocol - https://github.com/libp2p/specs/blob/master/identify/README.md#identifypush

SgtPooki · 2024-02-05T22:45:53Z

packages/protocol-identify/src/identify.ts

@@ -371,6 +252,180 @@ export class Identify implements Startable, IdentifyInterface {
      stream.abort(err)
    }
  }
+}
+
+export class IdentifyPush implements Startable, IdentifyPushInterface {


Can we get a jsdoc explaining how this is different than identify? is this an extension of identify? can this be used in place of identify, or are both needed (and in what scenarios would they both be needed)?

The @packageDocumentation block links to the spec which should answer these questions. If not it should be expanded - please suggest an edit.

SgtPooki · 2024-02-05T22:51:42Z

packages/protocol-identify/src/identify.ts

  maxObservedAddresses: 10,
  maxIdentifyMessageSize: 8192,
  runOnConnectionOpen: true,
+  runOnSelfUpdate: true,


this type doesn't exist on IdentifyInit. Is that expected?

SgtPooki · 2024-02-05T22:54:25Z

packages/protocol-identify/src/index.ts

@@ -65,23 +59,60 @@ export interface IdentifyInit {
  runOnTransientConnection?: boolean
 }

+export interface IdentifyPushInit {


Suggested change

export interface IdentifyPushInit {

export interface IdentifyPushInit extends IdentifyInit {

SgtPooki · 2024-02-05T22:56:31Z

packages/protocol-identify/src/identify.ts


-    if (message.publicKey != null) {
-      peer.publicKey = message.publicKey
+async function consumeIdentifyMessage (peerStore: PeerStore, events: TypedEventTarget<Libp2pEvents>, log: Logger, connection: Connection, message: IdentifyMessage): Promise<IdentifyResult> {


Is this no longer private on purpose?

SgtPooki · 2024-02-05T23:04:23Z

packages/protocol-identify/src/identify.ts

+  constructor (components: IdentifyPushComponents, init: IdentifyPushInit = {}) {
+    this.started = false
+    this.peerId = components.peerId
+    this.peerStore = components.peerStore
+    this.registrar = components.registrar
+    this.addressManager = components.addressManager
+    this.connectionManager = components.connectionManager
+    this.events = components.events
+    this.log = components.logger.forComponent('libp2p:identify:push')
+
+    this.identifyPushProtocolStr = `/${init.protocolPrefix ?? defaultValues.protocolPrefix}/${MULTICODEC_IDENTIFY_PUSH_PROTOCOL_NAME}/${MULTICODEC_IDENTIFY_PUSH_PROTOCOL_VERSION}`
+    this.timeout = init.timeout ?? defaultValues.timeout
+    this.maxInboundStreams = init.maxInboundStreams ?? defaultValues.maxInboundStreams
+    this.maxOutboundStreams = init.maxOutboundStreams ?? defaultValues.maxOutboundStreams
+    this.maxIdentifyMessageSize = init.maxIdentifyMessageSize ?? defaultValues.maxIdentifyMessageSize
+    this.runOnTransientConnection = init.runOnTransientConnection ?? defaultValues.runOnTransientConnection
+
+    // Store self host metadata
+    this.host = {
+      protocolVersion: `${init.protocolPrefix ?? defaultValues.protocolPrefix}/${IDENTIFY_PROTOCOL_VERSION}`,
+      agentVersion: getAgentVersion(components.nodeInfo, init.agentVersion)
+    }


It would be nice to consolidate a lot of this into a base identify class that both extend from so they're more manageable.

SgtPooki · 2024-02-05T23:04:51Z

packages/protocol-identify/src/identify.ts

+      }
+    })
+
+    await this.registrar.handle(this.identifyPushProtocolStr, (data) => {


Suggested change

await this.registrar.handle(this.identifyPushProtocolStr, (data) => {

await this.registrar.handle(this.protocolStr, (data) => {

could help if we were to consolidate logic into a base class

wemeetagain added 2 commits February 1, 2024 17:32

feat: add runOnSelfUpdate option for identify

9d354ed

feat: add disableIdentifyPush for identify

c739ac4

wemeetagain requested a review from a team as a code owner February 1, 2024 22:42

This was referenced Feb 5, 2024

Flaky tests ChainSafe/lodestar#6358

Open

Too many unknown peers in test mainnet nodes ChainSafe/lodestar#6382

Open

feat!: split identifyPush into separate module

76cdbf3

BREAKING CHANGE: identifyPush functionality has been removed from the identify module. In order to get this functionality, the identifyPush module must be instantiated separately.

wemeetagain changed the title ~~feat: allow disabling of identify-push~~ feat!: split identify and identify-push Feb 5, 2024

wemeetagain mentioned this pull request Feb 5, 2024

chore: fix failing sim tests ChainSafe/lodestar#6389

Merged

SgtPooki reviewed Feb 5, 2024

View reviewed changes

achingbrain added 4 commits May 1, 2024 07:06

Merge remote-tracking branch 'origin/main' into feat/no-auto-id-push

1918650

chore: create superclass, add concurrency limit to push

c8b3244

chore: update test

f21cf45

chore: update test

7360589

achingbrain approved these changes May 1, 2024

View reviewed changes

achingbrain merged commit 9d13a2f into main May 1, 2024
22 checks passed

achingbrain deleted the feat/no-auto-id-push branch May 1, 2024 11:14

achingbrain mentioned this pull request May 1, 2024

chore: release main #2516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: split identify and identify-push #2387

feat!: split identify and identify-push #2387

wemeetagain commented Feb 1, 2024

achingbrain commented Feb 2, 2024 •

edited

Loading

wemeetagain commented Feb 2, 2024

achingbrain commented Feb 5, 2024

SgtPooki left a comment

SgtPooki Feb 5, 2024

SgtPooki Feb 5, 2024

achingbrain Feb 6, 2024

SgtPooki Feb 5, 2024

achingbrain Feb 6, 2024

SgtPooki Feb 5, 2024

SgtPooki Feb 5, 2024

SgtPooki Feb 5, 2024

SgtPooki Feb 5, 2024

SgtPooki Feb 5, 2024

	export interface IdentifyPushInit {
	export interface IdentifyPushInit extends IdentifyInit {

	await this.registrar.handle(this.identifyPushProtocolStr, (data) => {
	await this.registrar.handle(this.protocolStr, (data) => {

feat!: split identify and identify-push #2387

feat!: split identify and identify-push #2387

Conversation

wemeetagain commented Feb 1, 2024

Description

Change checklist

achingbrain commented Feb 2, 2024 • edited Loading

wemeetagain commented Feb 2, 2024

achingbrain commented Feb 5, 2024

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain commented Feb 2, 2024 •

edited

Loading