From 58ef346aed459e214e4811961ae92dc42948a9df Mon Sep 17 00:00:00 2001 From: Cayman Date: Thu, 7 Sep 2023 15:07:44 -0400 Subject: [PATCH 1/3] feat: simplify enr initialization --- .../cli/src/cmds/beacon/initPeerIdAndEnr.ts | 39 +++++++++++---- packages/cli/src/cmds/bootnode/handler.ts | 2 +- .../test/unit/cmds/initPeerIdAndEnr.test.ts | 48 +++++++++++++++++++ 3 files changed, 78 insertions(+), 11 deletions(-) create mode 100644 packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts diff --git a/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts b/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts index 1bc6f8e5fd00..9509d3ad4148 100644 --- a/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts +++ b/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts @@ -53,14 +53,29 @@ export function isLocalMultiAddr(multiaddr: Multiaddr | undefined): boolean { return false; } -export function overwriteEnrWithCliArgs(enr: SignableENR, args: BeaconArgs, logger: Logger): void { +/** + * Only update the enr if the value has changed + */ +function maybeUpdateEnr( + enr: SignableENR, + key: T, + value: SignableENR[T] | undefined +): void { + if (enr[key] !== value) { + enr[key] = value; + } +} + +export function overwriteEnrWithCliArgs(enr: SignableENR, args: BeaconArgs, logger: Logger, bootnode?: boolean): void { const {port, discoveryPort, port6, discoveryPort6} = parseListenArgs(args); - enr.ip = args["enr.ip"] ?? enr.ip; - enr.tcp = args["enr.tcp"] ?? port ?? enr.tcp; - enr.udp = args["enr.udp"] ?? discoveryPort ?? enr.udp; - enr.ip6 = args["enr.ip6"] ?? enr.ip6; - enr.tcp6 = args["enr.tcp6"] ?? port6 ?? enr.tcp6; - enr.udp6 = args["enr.udp6"] ?? discoveryPort6 ?? enr.udp6; + maybeUpdateEnr(enr, "ip", args["enr.ip"] ?? enr.ip); + maybeUpdateEnr(enr, "ip6", args["enr.ip6"] ?? enr.ip6); + maybeUpdateEnr(enr, "udp", args["enr.udp"] ?? discoveryPort ?? enr.udp); + maybeUpdateEnr(enr, "udp6", args["enr.udp6"] ?? discoveryPort6 ?? enr.udp6); + if (!bootnode) { + maybeUpdateEnr(enr, "tcp", args["enr.tcp"] ?? port ?? enr.tcp); + maybeUpdateEnr(enr, "tcp6", args["enr.tcp6"] ?? port6 ?? enr.tcp6); + } function testMultiaddrForLocal(mu: Multiaddr, ip4: boolean): void { const isLocal = isLocalMultiAddr(mu); @@ -101,7 +116,8 @@ export function overwriteEnrWithCliArgs(enr: SignableENR, args: BeaconArgs, logg export async function initPeerIdAndEnr( args: BeaconArgs, beaconDir: string, - logger: Logger + logger: Logger, + bootnode?: boolean ): Promise<{peerId: PeerId; enr: SignableENR}> { const {persistNetworkIdentity} = args; @@ -146,14 +162,17 @@ export async function initPeerIdAndEnr( const enrFile = path.join(beaconDir, "enr"); const peerIdFile = path.join(beaconDir, "peer-id.json"); const {peerId, enr} = await readPersistedPeerIdAndENR(peerIdFile, enrFile); - overwriteEnrWithCliArgs(enr, args, logger); + overwriteEnrWithCliArgs(enr, args, logger, bootnode); // Re-persist peer-id and enr writeFile600Perm(peerIdFile, exportToJSON(peerId)); writeFile600Perm(enrFile, enr.encodeTxt()); return {peerId, enr}; } else { const {peerId, enr} = await newPeerIdAndENR(); - overwriteEnrWithCliArgs(enr, args, logger); + overwriteEnrWithCliArgs(enr, args, logger, bootnode); + // Since the enr is newly created, its sequence number can be set to 1 + // It's especially clean for fully configured bootnodes whose enrs never change + enr.seq = BigInt(1); return {peerId, enr}; } } diff --git a/packages/cli/src/cmds/bootnode/handler.ts b/packages/cli/src/cmds/bootnode/handler.ts index 0623d67bda59..be639eb1bf4b 100644 --- a/packages/cli/src/cmds/bootnode/handler.ts +++ b/packages/cli/src/cmds/bootnode/handler.ts @@ -180,7 +180,7 @@ export async function bootnodeHandlerInit(args: BootnodeArgs & GlobalArgs) { ); const logger = initLogger(args, beaconPaths.dataDir, config, "bootnode.log"); - const {peerId, enr} = await initPeerIdAndEnr(args as unknown as BeaconArgs, bootnodeDir, logger); + const {peerId, enr} = await initPeerIdAndEnr(args as unknown as BeaconArgs, bootnodeDir, logger, true); return {discv5Args, metricsArgs, bootnodeDir, network, version, commit, peerId, enr, logger}; } diff --git a/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts b/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts new file mode 100644 index 000000000000..edf4c350a469 --- /dev/null +++ b/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts @@ -0,0 +1,48 @@ +import tmp from "tmp"; +import {expect} from "chai"; +import {initPeerIdAndEnr} from "../../../src/cmds/beacon/initPeerIdAndEnr.js"; +import {BeaconArgs} from "../../../src/cmds/beacon/options.js"; +import {testLogger} from "../../utils.js"; + +describe("initPeerIdAndEnr", () => { + let tmpDir: tmp.DirResult; + + beforeEach(() => { + tmpDir = tmp.dirSync(); + }); + + afterEach(() => { + tmpDir.removeCallback(); + }); + + it("first time should create a new enr and peer id", async () => { + const {enr, peerId} = await initPeerIdAndEnr( + {persistNetworkIdentity: true} as unknown as BeaconArgs, + tmpDir.name, + testLogger(), + true + ); + expect((await enr.peerId()).toString(), "enr peer id doesn't equal the returned peer id").to.equal( + peerId.toString() + ); + }); + + it("second time should use ths existing enr and peer id", async () => { + const run1 = await initPeerIdAndEnr( + {persistNetworkIdentity: true} as unknown as BeaconArgs, + tmpDir.name, + testLogger(), + true + ); + + const run2 = await initPeerIdAndEnr( + {persistNetworkIdentity: true} as unknown as BeaconArgs, + tmpDir.name, + testLogger(), + true + ); + + expect(run1.peerId.toString()).to.equal(run2.peerId.toString()); + expect(run1.enr.encodeTxt()).to.equal(run2.enr.encodeTxt()); + }); +}); From 6f2c3242b53b129a76d312ff8a90458f08dbef42 Mon Sep 17 00:00:00 2001 From: Cayman Date: Fri, 8 Sep 2023 14:55:10 -0400 Subject: [PATCH 2/3] chore: fix tests --- .../cli/src/cmds/beacon/initPeerIdAndEnr.ts | 17 +++++++++++------ .../cli/test/unit/cmds/initPeerIdAndEnr.test.ts | 6 +++++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts b/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts index 9509d3ad4148..65c040e88c72 100644 --- a/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts +++ b/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts @@ -130,7 +130,7 @@ export async function initPeerIdAndEnr( const readPersistedPeerIdAndENR = async ( peerIdFile: string, enrFile: string - ): Promise<{peerId: PeerId; enr: SignableENR}> => { + ): Promise<{peerId: PeerId; enr: SignableENR; usePersisted: boolean}> => { let peerId: PeerId; let enr: SignableENR; @@ -139,7 +139,7 @@ export async function initPeerIdAndEnr( peerId = await readPeerId(peerIdFile); } catch (e) { logger.warn("Unable to read peerIdFile, creating a new peer id"); - return newPeerIdAndENR(); + return {...(await newPeerIdAndENR()), usePersisted: false}; } // attempt to read stored enr try { @@ -147,22 +147,27 @@ export async function initPeerIdAndEnr( } catch (e) { logger.warn("Unable to decode stored local ENR, creating a new ENR"); enr = SignableENR.createV4(createKeypairFromPeerId(peerId)); - return {peerId, enr}; + return {peerId, enr, usePersisted: false}; } // check stored peer id against stored enr if (!peerId.equals(await enr.peerId())) { logger.warn("Stored local ENR doesn't match peerIdFile, creating a new ENR"); enr = SignableENR.createV4(createKeypairFromPeerId(peerId)); - return {peerId, enr}; + return {peerId, enr, usePersisted: false}; } - return {peerId, enr}; + return {peerId, enr, usePersisted: true}; }; if (persistNetworkIdentity) { const enrFile = path.join(beaconDir, "enr"); const peerIdFile = path.join(beaconDir, "peer-id.json"); - const {peerId, enr} = await readPersistedPeerIdAndENR(peerIdFile, enrFile); + const {peerId, enr, usePersisted} = await readPersistedPeerIdAndENR(peerIdFile, enrFile); overwriteEnrWithCliArgs(enr, args, logger, bootnode); + // If the enr is newly created, its sequence number can be set to 1 + // It's especially clean for fully configured bootnodes whose enrs never change + if (!usePersisted) { + enr.seq = BigInt(1); + } // Re-persist peer-id and enr writeFile600Perm(peerIdFile, exportToJSON(peerId)); writeFile600Perm(enrFile, enr.encodeTxt()); diff --git a/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts b/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts index edf4c350a469..4bdfedf64b95 100644 --- a/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts +++ b/packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts @@ -1,3 +1,4 @@ +import fs from "node:fs"; import tmp from "tmp"; import {expect} from "chai"; import {initPeerIdAndEnr} from "../../../src/cmds/beacon/initPeerIdAndEnr.js"; @@ -12,7 +13,7 @@ describe("initPeerIdAndEnr", () => { }); afterEach(() => { - tmpDir.removeCallback(); + fs.rmSync(tmpDir.name, {recursive: true}); }); it("first time should create a new enr and peer id", async () => { @@ -25,6 +26,9 @@ describe("initPeerIdAndEnr", () => { expect((await enr.peerId()).toString(), "enr peer id doesn't equal the returned peer id").to.equal( peerId.toString() ); + expect(enr.seq).to.equal(BigInt(1)); + expect(enr.tcp).to.equal(undefined); + expect(enr.tcp6).to.equal(undefined); }); it("second time should use ths existing enr and peer id", async () => { From 29c8bc4d935a191776fe00052c1a4bbea93cffda Mon Sep 17 00:00:00 2001 From: Cayman Date: Fri, 8 Sep 2023 15:19:55 -0400 Subject: [PATCH 3/3] chore: more cleanup --- .../cli/src/cmds/beacon/initPeerIdAndEnr.ts | 47 ++++++++++++------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts b/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts index 65c040e88c72..37c5d766d7a3 100644 --- a/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts +++ b/packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts @@ -66,13 +66,19 @@ function maybeUpdateEnr => { + ): Promise<{peerId: PeerId; enr: SignableENR; newEnr: boolean}> => { let peerId: PeerId; let enr: SignableENR; @@ -139,7 +158,7 @@ export async function initPeerIdAndEnr( peerId = await readPeerId(peerIdFile); } catch (e) { logger.warn("Unable to read peerIdFile, creating a new peer id"); - return {...(await newPeerIdAndENR()), usePersisted: false}; + return {...(await newPeerIdAndENR()), newEnr: true}; } // attempt to read stored enr try { @@ -147,37 +166,29 @@ export async function initPeerIdAndEnr( } catch (e) { logger.warn("Unable to decode stored local ENR, creating a new ENR"); enr = SignableENR.createV4(createKeypairFromPeerId(peerId)); - return {peerId, enr, usePersisted: false}; + return {peerId, enr, newEnr: true}; } // check stored peer id against stored enr if (!peerId.equals(await enr.peerId())) { logger.warn("Stored local ENR doesn't match peerIdFile, creating a new ENR"); enr = SignableENR.createV4(createKeypairFromPeerId(peerId)); - return {peerId, enr, usePersisted: false}; + return {peerId, enr, newEnr: true}; } - return {peerId, enr, usePersisted: true}; + return {peerId, enr, newEnr: false}; }; if (persistNetworkIdentity) { const enrFile = path.join(beaconDir, "enr"); const peerIdFile = path.join(beaconDir, "peer-id.json"); - const {peerId, enr, usePersisted} = await readPersistedPeerIdAndENR(peerIdFile, enrFile); - overwriteEnrWithCliArgs(enr, args, logger, bootnode); - // If the enr is newly created, its sequence number can be set to 1 - // It's especially clean for fully configured bootnodes whose enrs never change - if (!usePersisted) { - enr.seq = BigInt(1); - } + const {peerId, enr, newEnr} = await readPersistedPeerIdAndENR(peerIdFile, enrFile); + overwriteEnrWithCliArgs(enr, args, logger, {newEnr, bootnode}); // Re-persist peer-id and enr writeFile600Perm(peerIdFile, exportToJSON(peerId)); writeFile600Perm(enrFile, enr.encodeTxt()); return {peerId, enr}; } else { const {peerId, enr} = await newPeerIdAndENR(); - overwriteEnrWithCliArgs(enr, args, logger, bootnode); - // Since the enr is newly created, its sequence number can be set to 1 - // It's especially clean for fully configured bootnodes whose enrs never change - enr.seq = BigInt(1); + overwriteEnrWithCliArgs(enr, args, logger, {newEnr: true, bootnode}); return {peerId, enr}; } }