feat: simplify bootnode enr initialization (#5945)

* feat: simplify enr initialization

* chore: fix tests

* chore: more cleanup

packages/cli/src/cmds/beacon/initPeerIdAndEnr.ts

@@ -53,14 +53,35 @@ 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<T extends "ip" | "tcp" | "udp" | "ip6" | "tcp6" | "udp6">(
+  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,
+  opts?: {newEnr?: boolean; bootnode?: boolean}
+): void {
+  const preSeq = enr.seq;
   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 (!opts?.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);
@@ -93,6 +114,19 @@ export function overwriteEnrWithCliArgs(enr: SignableENR, args: BeaconArgs, logg
   if (udpMultiaddr6) {
     testMultiaddrForLocal(udpMultiaddr6, false);
   }
+
+  if (enr.seq !== preSeq) {
+    // 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
+    // Otherwise, we can increment the sequence number as little as possible
+    if (opts?.newEnr) {
+      enr.seq = BigInt(1);
+    } else {
+      enr.seq = preSeq + BigInt(1);
+    }
+    // invalidate cached signature
+    delete enr["_signature"];
+  }
 }
 
 /**
@@ -101,7 +135,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;
 
@@ -114,7 +149,7 @@ export async function initPeerIdAndEnr(
   const readPersistedPeerIdAndENR = async (
     peerIdFile: string,
     enrFile: string
-  ): Promise<{peerId: PeerId; enr: SignableENR}> => {
+  ): Promise<{peerId: PeerId; enr: SignableENR; newEnr: boolean}> => {
     let peerId: PeerId;
     let enr: SignableENR;
 
@@ -123,37 +158,37 @@ 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()), newEnr: true};
     }
     // attempt to read stored enr
     try {
       enr = SignableENR.decodeTxt(fs.readFileSync(enrFile, "utf-8"), createKeypairFromPeerId(peerId));
     } 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, 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};
+      return {peerId, enr, newEnr: true};
     }
-    return {peerId, enr};
+    return {peerId, enr, newEnr: false};
   };
 
   if (persistNetworkIdentity) {
     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);
+    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);
+    overwriteEnrWithCliArgs(enr, args, logger, {newEnr: true, bootnode});
     return {peerId, enr};
   }
 }

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};
 }

packages/cli/test/unit/cmds/initPeerIdAndEnr.test.ts

@@ -0,0 +1,52 @@
+import fs from "node:fs";
+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(() => {
+    fs.rmSync(tmpDir.name, {recursive: true});
+  });
+
+  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()
+    );
+    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 () => {
+    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());
+  });
+});