Implement standard keymanager API - review

packages/cli/src/cmds/validator/handler.ts

@@ -105,10 +105,17 @@ export async function validatorHandler(args: IValidatorCliArgs & IGlobalArgs): P
   await validator.start();
 
   // Start keymanager API backend
-  // Only if keymanagerEnabled flag is set to true and at least one keystore path is supplied
-  const firstImportKeystorePath = args.importKeystoresPath?.[0];
-  if (args.keymanagerEnabled && firstImportKeystorePath) {
-    const keymanagerApi = new KeymanagerApi(logger, validator, firstImportKeystorePath);
+  // Only if keymanagerEnabled flag is set to true
+  if (args.keymanagerEnabled) {
+    if (!args.importKeystoresPath || args.importKeystoresPath.length === 0) {
+      throw new YargsError("For keymanagerEnabled must set importKeystoresPath to at least 1 path");
+    }
+
+    // Use the first path in importKeystoresPath as directory to write keystores
+    // KeymanagerApi must ensure that the path is a directory and not a file
+    const firstImportKeystorePath = args.importKeystoresPath[0];
+
+    const keymanagerApi = new KeymanagerApi(validator, firstImportKeystorePath);
 
     const keymanagerServer = new KeymanagerServer(
       {

packages/cli/src/cmds/validator/keys.ts

@@ -5,7 +5,7 @@ import {CoordType, PublicKey, SecretKey} from "@chainsafe/bls";
 import {deriveEth2ValidatorKeys, deriveKeyFromMnemonic} from "@chainsafe/bls-keygen";
 import {interopSecretKey} from "@chainsafe/lodestar-beacon-state-transition";
 import {externalSignerGetKeys} from "@chainsafe/lodestar-validator";
-import {LOCK_FILE_EXT, getLockFile} from "@chainsafe/lodestar-keymanager-server";
+import {lockFilepath, unlockFilepath} from "@chainsafe/lodestar-keymanager-server";
 import {defaultNetwork, IGlobalArgs} from "../../options";
 import {parseRange, stripOffNewlines, YargsError} from "../../util";
 import {ValidatorDirManager} from "../../validatorDir";
@@ -53,13 +53,9 @@ export async function getLocalSecretKeys(
 
     const keystorePaths = args.importKeystoresPath.map((filepath) => resolveKeystorePaths(filepath)).flat(1);
 
-    // Create lock files for all keystores
-    const lockFile = getLockFile();
-    const lockFilePaths = keystorePaths.map((keystorePath) => keystorePath + LOCK_FILE_EXT);
-
     // Lock all keystores first
-    for (const lockFilePath of lockFilePaths) {
-      lockFile.lockSync(lockFilePath);
+    for (const keystorePath of keystorePaths) {
+      lockFilepath(keystorePath);
     }
 
     const secretKeys = await Promise.all(
@@ -71,8 +67,9 @@ export async function getLocalSecretKeys(
     return {
       secretKeys,
       unlockSecretKeys: () => {
-        for (const lockFilePath of lockFilePaths) {
-          lockFile.unlockSync(lockFilePath);
+        for (const keystorePath of keystorePaths) {
+          // Should not throw if lock file is already deleted
+          unlockFilepath(keystorePath);
         }
       },
     };

packages/cli/src/validatorDir/ValidatorDir.ts

@@ -3,14 +3,13 @@ import path from "node:path";
 import bls, {SecretKey} from "@chainsafe/bls";
 import {Keystore} from "@chainsafe/bls-keystore";
 import {phase0} from "@chainsafe/lodestar-types";
-import {getLockFile} from "@chainsafe/lodestar-keymanager-server";
+import {lockFilepath, unlockFilepath} from "@chainsafe/lodestar-keymanager-server";
 import {YargsError, readValidatorPassphrase} from "../util";
 import {decodeEth1TxData} from "../depositContract/depositData";
 import {add0xPrefix} from "../util/format";
 import {
   VOTING_KEYSTORE_FILE,
   WITHDRAWAL_KEYSTORE_FILE,
-  LOCK_FILE,
   ETH1_DEPOSIT_DATA_FILE,
   ETH1_DEPOSIT_AMOUNT_FILE,
   ETH1_DEPOSIT_TX_HASH_FILE,
@@ -50,23 +49,21 @@ export interface IEth1DepositData {
  * ```
  */
 export class ValidatorDir {
-  dir: string;
-  private lockfilePath: string;
+  private readonly dirpath: string;
 
   /**
    * Open `dir`, creating a lockfile to prevent concurrent access.
    * Errors if there is a filesystem error or if a lockfile already exists
-   * @param dir
    */
   constructor(baseDir: string, pubkey: string, options?: IValidatorDirOptions) {
-    this.dir = path.join(baseDir, add0xPrefix(pubkey));
-    this.lockfilePath = path.join(this.dir, LOCK_FILE);
+    this.dirpath = path.join(baseDir, add0xPrefix(pubkey));
 
-    if (!fs.existsSync(this.dir)) throw new YargsError(`Validator directory ${this.dir} does not exist`);
+    if (!fs.existsSync(this.dirpath)) {
+      throw new YargsError(`Validator directory ${this.dirpath} does not exist`);
+    }
 
-    const lockFile = getLockFile();
     try {
-      lockFile.lockSync(this.lockfilePath);
+      lockFilepath(this.dirpath);
     } catch (e) {
       if (options && options.force) {
         // Ignore error, maybe log?
@@ -80,7 +77,7 @@ export class ValidatorDir {
    * Removes the lockfile associated with this validator dir
    */
   close(): void {
-    getLockFile().unlockSync(this.lockfilePath);
+    unlockFilepath(this.dirpath);
   }
 
   /**
@@ -91,7 +88,7 @@ export class ValidatorDir {
    * @param secretsDir
    */
   async votingKeypair(secretsDir: string): Promise<SecretKey> {
-    const keystorePath = path.join(this.dir, VOTING_KEYSTORE_FILE);
+    const keystorePath = path.join(this.dirpath, VOTING_KEYSTORE_FILE);
     return await this.unlockKeypair(keystorePath, secretsDir);
   }
 
@@ -103,7 +100,7 @@ export class ValidatorDir {
    * @param secretsDir
    */
   async withdrawalKeypair(secretsDir: string): Promise<SecretKey> {
-    const keystorePath = path.join(this.dir, WITHDRAWAL_KEYSTORE_FILE);
+    const keystorePath = path.join(this.dirpath, WITHDRAWAL_KEYSTORE_FILE);
     return await this.unlockKeypair(keystorePath, secretsDir);
   }
 
@@ -127,14 +124,14 @@ export class ValidatorDir {
    * when relying upon this value.
    */
   eth1DepositTxHashExists(): boolean {
-    return fs.existsSync(path.join(this.dir, ETH1_DEPOSIT_TX_HASH_FILE));
+    return fs.existsSync(path.join(this.dirpath, ETH1_DEPOSIT_TX_HASH_FILE));
   }
 
   /**
    * Saves the `tx_hash` to a file in `this.dir`.
    */
   saveEth1DepositTxHash(txHash: string): void {
-    const filepath = path.join(this.dir, ETH1_DEPOSIT_TX_HASH_FILE);
+    const filepath = path.join(this.dirpath, ETH1_DEPOSIT_TX_HASH_FILE);
 
     if (fs.existsSync(filepath)) throw new YargsError(`ETH1_DEPOSIT_TX_HASH_FILE ${filepath} already exists`);
 
@@ -146,8 +143,8 @@ export class ValidatorDir {
    * submitting an Eth1 deposit.
    */
   eth1DepositData(): IEth1DepositData {
-    const depositDataPath = path.join(this.dir, ETH1_DEPOSIT_DATA_FILE);
-    const depositAmountPath = path.join(this.dir, ETH1_DEPOSIT_AMOUNT_FILE);
+    const depositDataPath = path.join(this.dirpath, ETH1_DEPOSIT_DATA_FILE);
+    const depositAmountPath = path.join(this.dirpath, ETH1_DEPOSIT_AMOUNT_FILE);
     const depositDataRlp = fs.readFileSync(depositDataPath, "utf8");
     const depositAmount = fs.readFileSync(depositAmountPath, "utf8");
 

packages/cli/src/validatorDir/paths.ts

@@ -6,10 +6,6 @@ export const WITHDRAWAL_KEYSTORE_FILE = "withdrawal-keystore.json";
 export const ETH1_DEPOSIT_DATA_FILE = "eth1-deposit-data.rlp";
 export const ETH1_DEPOSIT_AMOUNT_FILE = "eth1-deposit-gwei.txt";
 export const ETH1_DEPOSIT_TX_HASH_FILE = "eth1-deposit-tx-hash.txt";
-/**
- * The file used for indicating if a directory is in-use by another process.
- */
-export const LOCK_FILE = ".lock";
 
 // Dynamic paths computed from the validator pubkey
 

packages/keymanager-server/src/impl.ts

@@ -12,26 +12,22 @@ import {
 import {fromHexString} from "@chainsafe/ssz";
 import {Interchange, SignerType, Validator} from "@chainsafe/lodestar-validator";
 import {PubkeyHex} from "@chainsafe/lodestar-validator/src/types";
-import {ILogger} from "@chainsafe/lodestar-utils";
-import {LOCK_FILE_EXT, getLockFile} from "./util/lockfile";
+import {lockFilepath, unlockFilepath} from "./util/lockfile";
 
 export const KEY_IMPORTED_PREFIX = "key_imported";
 
 export class KeymanagerApi implements Api {
-  constructor(
-    private readonly logger: ILogger,
-    private readonly validator: Validator,
-    private readonly importKeystoresPath: string
-  ) {}
-
-  getKeystorePathInfoForKey = (pubkey: string): {keystoreFilePath: string; lockFilePath: string} => {
-    const keystoreFilename = `${KEY_IMPORTED_PREFIX}_${pubkey}.json`;
-    const keystoreFilePath = path.join(this.importKeystoresPath, keystoreFilename);
-    return {
-      keystoreFilePath,
-      lockFilePath: `${keystoreFilePath}${LOCK_FILE_EXT}`,
-    };
-  };
+  constructor(private readonly validator: Validator, private readonly importKeystoresDirpath: string) {
+    if (fs.existsSync(importKeystoresDirpath)) {
+      // Ensure is directory
+      if (!fs.statSync(importKeystoresDirpath).isDirectory()) {
+        throw Error("importKeystoresPath is not a directory");
+      }
+    } else {
+      // Or, create empty directory
+      fs.mkdirSync(importKeystoresDirpath, {recursive: true});
+    }
+  }
 
   /**
    * List all validating pubkeys known to and decrypted by this keymanager binary
@@ -63,9 +59,9 @@ export class KeymanagerApi implements Api {
    * Users SHOULD send slashing_protection data associated with the imported pubkeys. MUST follow the format defined in
    * EIP-3076: Slashing Protection Interchange Format.
    *
-   * @param keystores JSON-encoded keystore files generated with the Launchpad
+   * @param keystoresStr JSON-encoded keystore files generated with the Launchpad
    * @param passwords Passwords to unlock imported keystore files. `passwords[i]` must unlock `keystores[i]`
-   * @param slashingProtection Slashing protection data for some of the keys of `keystores`
+   * @param slashingProtectionStr Slashing protection data for some of the keys of `keystores`
    * @returns Status result of each `request.keystores` with same length and order of `request.keystores`
    *
    * https://github.com/ethereum/keymanager-APIs/blob/0c975dae2ac6053c8245ebdb6a9f27c2f114f407/keymanager-oapi.yaml
@@ -80,6 +76,11 @@ export class KeymanagerApi implements Api {
       message?: string;
     }[];
   }> {
+    // The arguments to this function is passed in within the body of an HTTP request
+    // hence fastify will parse it into an object before this function is called.
+    // Even though the slashingProtectionStr is typed as SlashingProtectionData,
+    // at runtime, when the handler for the request is selected, it would see slashingProtectionStr
+    // as an object, hence trying to parse it using JSON.parse won't work. Instead, we cast straight to Interchange
     const interchange = (slashingProtectionStr as unknown) as Interchange;
     await this.validator.validatorStore.importInterchange(interchange);
 
@@ -105,12 +106,13 @@ export class KeymanagerApi implements Api {
         const pubKey = secretKey.toPublicKey().toHex();
         this.validator.validatorStore.addSigner({type: SignerType.Local, secretKey});
 
-        const keystorePathInfo = this.getKeystorePathInfoForKey(pubKey);
+        const keystoreFilepath = this.getKeystoreFilepath(pubKey);
+
+        // Lock before writing keystore
+        lockFilepath(keystoreFilepath);
 
-        // Persist keys for latter restarts
-        await fs.promises.writeFile(keystorePathInfo.keystoreFilePath, keystoreStr, {encoding: "utf8"});
-        const lockFile = getLockFile();
-        lockFile.lockSync(keystorePathInfo.lockFilePath);
+        // Persist keys for latter restarts, directory of `keystoreFilepath` is created in the constructor
+        await fs.promises.writeFile(keystoreFilepath, keystoreStr, {encoding: "utf8"});
 
         statuses[i] = {status: ImportStatus.imported};
       } catch (e) {
@@ -137,7 +139,7 @@ export class KeymanagerApi implements Api {
    * Slashing protection data must only be returned for keys from `request.pubkeys` for which a
    * `deleted` or `not_active` status is returned.
    *
-   * @param pubkeys List of public keys to delete.
+   * @param pubkeysHex List of public keys to delete.
    * @returns Deletion status of all keys in `request.pubkeys` in the same order.
    *
    * https://github.com/ethereum/keymanager-APIs/blob/0c975dae2ac6053c8245ebdb6a9f27c2f114f407/keymanager-oapi.yaml
@@ -172,14 +174,14 @@ export class KeymanagerApi implements Api {
         // Remove from Sync committee duties
         // Remove from indices
         this.validator.removeDutiesForKey(pubkeyHex);
-        const keystorePathInfo = this.getKeystorePathInfoForKey(pubkeyHex);
+
+        const keystoreFilepath = this.getKeystoreFilepath(pubkeyHex);
+
         // Remove key from persistent storage
-        for (const keystoreFile of await fs.promises.readdir(this.importKeystoresPath)) {
-          if (keystoreFile.indexOf(pubkeyHex) !== -1) {
-            await fs.promises.unlink(keystorePathInfo.keystoreFilePath);
-            await fs.promises.unlink(keystorePathInfo.lockFilePath);
-          }
-        }
+        fs.unlinkSync(keystoreFilepath);
+
+        // Unlock last
+        unlockFilepath(keystoreFilepath);
       } catch (e) {
         statuses[i] = {status: DeletionStatus.error, message: (e as Error).message};
       }
@@ -210,4 +212,8 @@ export class KeymanagerApi implements Api {
       slashingProtection: JSON.stringify(interchangeV5),
     };
   }
+
+  private getKeystoreFilepath(pubkeyHex: string): string {
+    return path.join(this.importKeystoresDirpath, `${KEY_IMPORTED_PREFIX}_${pubkeyHex}.json`);
+  }
 }

packages/keymanager-server/src/server.ts

@@ -47,9 +47,12 @@ export class KeymanagerServer {
 
   constructor(optsArg: Partial<RestApiOptions>, modules: IRestApiModules) {
     this.logger = modules.logger;
+
     // Apply opts defaults
     const opts = {
       ...restApiOptionsDefault,
+      // optsArg is a Partial type, any of its properties can be undefined. If port is set to undefined,
+      // it overrides the default port value in restApiOptionsDefault to be undefined.
       ...Object.fromEntries(Object.entries(optsArg).filter(([_, v]) => v != null)),
     };
 

packages/keymanager-server/src/util/lockfile.ts

@@ -4,16 +4,38 @@ type Lockfile = {
 };
 
 let lockFile: Lockfile | null = null;
-export const LOCK_FILE_EXT = ".lock";
+
+function getLockFilepath(filepath: string): string {
+  return `${filepath}.lock`;
+}
+
 /**
  * When lockfile it's required it registers listeners to process
  * Since it's only used by the validator client, require lazily to not pollute
  * beacon_node client context
  */
-export function getLockFile(): Lockfile {
+function getLockFile(): Lockfile {
   if (!lockFile) {
     // eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports
     lockFile = require("lockfile") as Lockfile;
   }
   return lockFile;
 }
+
+/**
+ * Creates a .lock file for `filepath`, argument passed must not be the lock path
+ * @param filepath File to lock, i.e. `keystore_0001.json`
+ */
+export function lockFilepath(filepath: string): void {
+  getLockFile().lockSync(getLockFilepath(filepath));
+}
+
+/**
+ * Deletes a .lock file for `filepath`, argument passed must not be the lock path
+ * @param filepath File to unlock, i.e. `keystore_0001.json`
+ */
+export function unlockFilepath(filepath: string): void {
+  // Does not throw if the lock file is already deleted
+  // https://github.com/npm/lockfile/blob/6590779867ee9bdc5dbebddc962640759892bb91/lockfile.js#L68
+  getLockFile().unlockSync(getLockFilepath(filepath));
+}