From dc12406677f1906993dd64d39e92439be3678e4a Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Tue, 9 May 2023 15:22:26 +0200 Subject: [PATCH 1/2] Lock keystores when loading from cache file --- .../decryptKeystoreDefinitions/index.ts | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts b/packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts index ceb3323fb6aa..f1507d70e77f 100644 --- 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; + } + } +} From c0e9629611dcf0571be373ffc17545e126a99621 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Tue, 9 May 2023 15:24:21 +0200 Subject: [PATCH 2/2] Update tests to run with and without existing keystore cache --- .../decryptKeystoreDefinitions.test.ts | 65 ++++++++++++------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts b/packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts index 1d373bf49485..c4b69d2ba77c 100644 --- 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}); + }); + } });