Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: manually use identify protocol #6400

Merged
merged 1 commit into from
Feb 10, 2024
Merged

feat: manually use identify protocol #6400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/beacon-node/src/network/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from "@libp2p/interface";
import type {AddressManager, ConnectionManager, Registrar, TransportManager} from "@libp2p/interface-internal";
import type {Datastore} from "interface-datastore";
import {Identify} from "@chainsafe/libp2p-identify";
import {Slot, SlotRootHex, allForks, altair, capella, deneb, phase0} from "@lodestar/types";
import {PeerIdStr} from "../util/peerId.js";
import {INetworkEventBus} from "./events.js";
Expand Down Expand Up @@ -98,4 +99,4 @@ export type LodestarComponents = {
metrics?: Metrics;
};

export type Libp2p = ILibp2p<{components: LodestarComponents}>;
export type Libp2p = ILibp2p<{components: LodestarComponents; identify: Identify}>;
1 change: 1 addition & 0 deletions packages/beacon-node/src/network/libp2p/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ export async function createNodeJsLibp2p(
services: {
identify: identify({
agentVersion: networkOpts.private ? "" : networkOpts.version ? `lodestar/${networkOpts.version}` : "lodestar",
runOnConnectionOpen: false,
}),
// individual components are specified because the components object is a Proxy
// and passing it here directly causes problems downstream, not to mention is slowwww
Expand Down
24 changes: 10 additions & 14 deletions packages/beacon-node/src/network/peers/peerManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {BitArray} from "@chainsafe/ssz";
import {SYNC_COMMITTEE_SUBNET_COUNT} from "@lodestar/params";
import {BeaconConfig} from "@lodestar/config";
import {allForks, altair, phase0} from "@lodestar/types";
import {retry, withTimeout} from "@lodestar/utils";
import {withTimeout} from "@lodestar/utils";
import {LoggerNode} from "@lodestar/logger/node";
import {GoodByeReasonCode, GOODBYE_KNOWN_CODES, Libp2pEvent} from "../../constants/index.js";
import {IClock} from "../../util/clock.js";
Expand Down Expand Up @@ -606,22 +606,18 @@ export class PeerManager {
void this.requestStatus(remotePeer, this.statusCache.get());
}

// AgentVersion was set in libp2p IdentifyService, 'peer:connect' event handler
// since it's not possible to handle it async, we have to wait for a while to set AgentVersion
// See https://github.com/libp2p/js-libp2p/pull/1168
retry(
async () => {
const agentVersionBytes = (await this.libp2p.peerStore.get(peerData.peerId)).metadata.get("AgentVersion");
if (agentVersionBytes) {
const agentVersion = new TextDecoder().decode(agentVersionBytes) || "N/A";
this.libp2p.services.identify
.identify(evt.detail)
.then((result) => {
const agentVersion = result.agentVersion;
if (agentVersion) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curios about the different cases, i.e. when do we not get an error but also an empty agentVersion? and should we add a fallback case like "N/A" as previously done?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protobuf sent to us can have a null entry.

I thought about keeping the fallback "N/A" but it didn't seem particularly useful? Happy to add it back if anyone feels different.

Copy link
Member

@nflaig nflaig Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about keeping the fallback "N/A" but it didn't seem particularly useful?

The only case I see where this makes a difference is in the case we error as it would be undefined vs "N/A" otherwise, although I don't see that this value is used anywhere other than dumping peer information.

I remember Tuyen did some changes in known client version but that is just for metrics afaik and we already handle the case if agentClient is not set

lodestar/packages/beacon-node/src/network/peers/peerManager.ts

Line 697 in 3adf2c2

const client = peerData?.agentClient ?? ClientKind.Unknown;

peerData.agentVersion = agentVersion;
peerData.agentClient = getKnownClientFromAgentVersion(agentVersion);
}
},
{retries: 3, retryDelay: 1000}
).catch((err) => {
this.logger.debug("Error setting agentVersion for the peer", {peerId: peerData.peerId.toString()}, err);
});
})
.catch((err) => {
this.logger.debug("Error setting agentVersion for the peer", {peerId: peerData.peerId.toString()}, err);
});
};

/**
Expand Down
Loading