Skip to content

Commit ca68d9a

Browse files
authored
feat: override libp2p handler when registering protocol for new fork (#7755)
**Motivation** Right now, we do [not register a handler if it already is already registered](https://github.com/ChainSafe/lodestar/blob/78a5e72989d10426b77bd408673fe191bbf33f8a/packages/reqresp/src/ReqResp.ts#L102-L105) but even though we update some parts now such as rate limit at the hard fork, we do not update all of them like `requestSizes`, we can either pass the fork down in all the code paths where it is needed and use getter functions or we override the handler once during the hard fork transition. This PR proposes the later solution as it's much simpler and seems cleaner as well. **Description** Override libp2p handler when registering protocol for new fork, this is possible now due to libp2p/js-libp2p#2945 since we updated to libp2p 2.0 in #7359. More context libp2p/js-libp2p#2928 Closes #7557
1 parent 2f79f78 commit ca68d9a

File tree

3 files changed

+17
-24
lines changed

3 files changed

+17
-24
lines changed

packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,9 @@ export class ReqRespBeaconNode extends ReqResp {
137137
}
138138
}
139139

140-
// Subscribe required protocols, prevent libp2p for throwing if already registered
140+
// Subscribe required protocols
141141
for (const [protocol, handler] of mustSubscribeProtocols) {
142-
this.registerProtocol({...protocol, handler}, {ignoreIfDuplicate: true}).catch((e) => {
142+
this.registerProtocol({...protocol, handler}).catch((e) => {
143143
this.logger.error("Error on ReqResp.registerProtocol", {protocolID: this.formatProtocolID(protocol)}, e);
144144
});
145145
}

packages/reqresp/src/ReqResp.ts

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,6 @@ export interface ReqRespOpts extends SendRequestOpts, ReqRespRateLimiterOpts {
3333
getPeerLogMetadata?: (peerId: string) => string;
3434
}
3535

36-
export interface ReqRespRegisterOpts {
37-
ignoreIfDuplicate?: boolean;
38-
}
39-
4036
/**
4137
* Implementation of Ethereum Consensus p2p Req/Resp domain.
4238
* For the spec that this code is based on, see:
@@ -87,27 +83,21 @@ export class ReqResp {
8783
/**
8884
* Register protocol as supported and to libp2p.
8985
* async because libp2p registrar persists the new protocol list in the peer-store.
90-
* Throws if the same protocol is registered twice.
86+
* Overrides handler and rate limits in case protocol is already registered.
9187
* Can be called at any time, no concept of started / stopped
9288
*/
93-
async registerProtocol(protocol: Protocol, opts?: ReqRespRegisterOpts): Promise<void> {
89+
async registerProtocol(protocol: Protocol): Promise<void> {
9490
const protocolID = this.formatProtocolID(protocol);
9591
const {handler: _handler, inboundRateLimits, ...rest} = protocol;
9692

93+
this.registerDialOnlyProtocol(rest);
94+
this.dialOnlyProtocols.set(protocolID, false);
95+
9796
if (inboundRateLimits) {
98-
// Rate limits can change across hard forks and must always be updated
9997
this.rateLimiter.setRateLimits(protocolID, inboundRateLimits);
10098
}
10199

102-
// libp2p will throw if handler for protocol is already registered, allow to overwrite behavior
103-
if (opts?.ignoreIfDuplicate && this.registeredProtocols.has(protocolID)) {
104-
return;
105-
}
106-
107-
this.registerDialOnlyProtocol(rest);
108-
this.dialOnlyProtocols.set(protocolID, false);
109-
110-
return this.libp2p.handle(protocolID, this.getRequestHandler(protocol, protocolID));
100+
return this.libp2p.handle(protocolID, this.getRequestHandler(protocol, protocolID), {force: true});
111101
}
112102

113103
/**

packages/reqresp/test/unit/ReqResp.test.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,19 +66,22 @@ describe("ResResp", () => {
6666
expect(libp2p.handle).toHaveBeenCalledOnce();
6767
});
6868

69-
it("should not register handler twice for same protocol if ignoreIfDuplicate=true", async () => {
70-
await reqresp.registerProtocol(numberToStringProtocol, {ignoreIfDuplicate: true});
69+
it("should override existing handler if same protocol is registered multiple times", async () => {
70+
await reqresp.registerProtocol(numberToStringProtocol);
7171
expect(libp2p.handle).toHaveBeenCalledOnce();
7272

73-
await reqresp.registerProtocol(numberToStringProtocol, {ignoreIfDuplicate: true});
74-
expect(libp2p.handle).toHaveBeenCalledOnce();
73+
await reqresp.registerProtocol(numberToStringProtocol);
74+
expect(libp2p.handle).toHaveBeenCalledTimes(2);
75+
76+
await reqresp.registerProtocol(numberToStringProtocol);
77+
expect(libp2p.handle).toHaveBeenCalledTimes(3);
7578
});
7679

7780
it("should apply new rate limits if same protocol is registered with different limits", async () => {
7881
// Initial registration of protocol
7982
const {quota, quotaTimeMs} = numberToStringProtocol.inboundRateLimits?.byPeer as RateLimiterQuota;
8083
const initialMsPerToken = quotaTimeMs / quota;
81-
await reqresp.registerProtocol(numberToStringProtocol, {ignoreIfDuplicate: true});
84+
await reqresp.registerProtocol(numberToStringProtocol);
8285
const initialLimit = reqresp["rateLimiter"]["rateLimitersPerPeer"].get(
8386
"/eth2/beacon_chain/req/number_to_string/1/ssz_snappy"
8487
);
@@ -92,7 +95,7 @@ describe("ResResp", () => {
9295
inboundRateLimits: {byPeer: updatedQuota},
9396
};
9497
const updatedMsPerToken = updatedQuota.quotaTimeMs / updatedQuota.quota;
95-
await reqresp.registerProtocol(updatedProtocol, {ignoreIfDuplicate: true});
98+
await reqresp.registerProtocol(updatedProtocol);
9699
const updatedLimit = reqresp["rateLimiter"]["rateLimitersPerPeer"].get(
97100
"/eth2/beacon_chain/req/number_to_string/1/ssz_snappy"
98101
);

0 commit comments

Comments
 (0)