fix(validator): lock keystores when loading from cache file (#5474)

* Lock keystores when loading from cache file * Update tests to run with and without existing keystore cache
ChainSafe · May 14, 2023 · 6c10dbe · 6c10dbe
1 parent a59f231
commit 6c10dbe
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 33 deletions.
diff --git a/packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts b/packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
@@ -17,10 +17,17 @@ export async function decryptKeystoreDefinitions(
   if (opts.cacheFilePath) {
     try {
       const signers = await loadKeystoreCache(opts.cacheFilePath, keystoreDefinitions);
+
+      for (const {keystorePath} of keystoreDefinitions) {
+        lockKeystore(keystorePath, opts);
+      }
+
       if (opts?.onDecrypt) {
         opts?.onDecrypt(signers.length - 1);
       }
+
       opts.logger.debug("Loaded keystores via keystore cache");
+
       return signers;
     } catch {
       // Some error loading the cache, ignore and invalidate cache
@@ -41,17 +48,7 @@ export async function decryptKeystoreDefinitions(
     defaultPoolSize
   );
   for (const [index, definition] of keystoreDefinitions.entries()) {
-    try {
-      lockFilepath(definition.keystorePath);
-    } catch (e) {
-      if (opts.ignoreLockFile) {
-        opts.logger.warn("Keystore forcefully loaded even though lockfile exists", {
-          path: definition.keystorePath,
-        });
-      } else {
-        throw e;
-      }
-    }
+    lockKeystore(definition.keystorePath, opts);
 
     const task = pool.queue((thread) => thread.decryptKeystoreDefinition(definition));
     tasks.push(task);
@@ -102,3 +99,17 @@ export async function decryptKeystoreDefinitions(
 
   return signers;
 }
+
+function lockKeystore(keystorePath: string, opts: KeystoreDecryptOptions): void {
+  try {
+    lockFilepath(keystorePath);
+  } catch (e) {
+    if (opts.ignoreLockFile) {
+      opts.logger.warn("Keystore forcefully loaded even though lockfile exists", {
+        path: keystorePath,
+      });
+    } else {
+      throw e;
+    }
+  }
+}
diff --git a/packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts b/packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts
@@ -38,31 +38,52 @@ describe("decryptKeystoreDefinitions", function () {
     }
   });
 
-  it("decrypt keystores", async () => {
-    const signers = await decryptKeystoreDefinitions(definitions, {logger: console});
-    expect(signers.length).to.equal(secretKeys.length);
-    for (const signer of signers) {
-      const hexSecret = signer.secretKey.toHex();
-      expect(secretKeys.includes(hexSecret), `secretKeys doesn't include ${hexSecret}`).to.be.true;
-    }
-  });
+  context("with keystore cache", () => {
+    const cacheFilePath = path.join(dataDir, "cache", "keystores.cache");
 
-  it("fail to decrypt keystores if lockfiles already exist", async () => {
-    await decryptKeystoreDefinitions(definitions, {logger: console});
-    // lockfiles should exist after the first run
+    beforeEach(async () => {
+      // create cache file to ensure keystores are loaded from cache during tests
+      await decryptKeystoreDefinitions(definitions, {logger: console, cacheFilePath});
+      expect(fs.existsSync(cacheFilePath)).to.be.true;
 
-    try {
-      await decryptKeystoreDefinitions(definitions, {logger: console});
-      expect.fail("Second decrypt should fail due to failure to get lockfile");
-    } catch (e) {
-      expect((e as Error).message.startsWith("EEXIST: file already exists"), "Wrong error is thrown").to.be.true;
-    }
-  });
+      // remove lockfiles created during cache file preparation
+      rimraf.sync(path.join(importFromDir, "*.lock"), {glob: true});
+    });
 
-  it("decrypt keystores if lockfiles already exist if ignoreLockFile=true", async () => {
-    await decryptKeystoreDefinitions(definitions, {logger: console});
-    // lockfiles should exist after the first run
+    testDecryptKeystoreDefinitions(cacheFilePath);
+  });
 
-    await decryptKeystoreDefinitions(definitions, {logger: console, ignoreLockFile: true});
+  context("without keystore cache", () => {
+    testDecryptKeystoreDefinitions();
   });
+
+  function testDecryptKeystoreDefinitions(cacheFilePath?: string): void {
+    it("decrypt keystores", async () => {
+      const signers = await decryptKeystoreDefinitions(definitions, {logger: console, cacheFilePath});
+      expect(signers.length).to.equal(secretKeys.length);
+      for (const signer of signers) {
+        const hexSecret = signer.secretKey.toHex();
+        expect(secretKeys.includes(hexSecret), `secretKeys doesn't include ${hexSecret}`).to.be.true;
+      }
+    });
+
+    it("fail to decrypt keystores if lockfiles already exist", async () => {
+      await decryptKeystoreDefinitions(definitions, {logger: console, cacheFilePath});
+      // lockfiles should exist after the first run
+
+      try {
+        await decryptKeystoreDefinitions(definitions, {logger: console, cacheFilePath});
+        expect.fail("Second decrypt should fail due to failure to get lockfile");
+      } catch (e) {
+        expect((e as Error).message.startsWith("EEXIST: file already exists"), "Wrong error is thrown").to.be.true;
+      }
+    });
+
+    it("decrypt keystores if lockfiles already exist if ignoreLockFile=true", async () => {
+      await decryptKeystoreDefinitions(definitions, {logger: console, cacheFilePath});
+      // lockfiles should exist after the first run
+
+      await decryptKeystoreDefinitions(definitions, {logger: console, cacheFilePath, ignoreLockFile: true});
+    });
+  }
 });