From 5809de0852ae63f6962d76451f4757d7070eb1a7 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Mon, 13 Jan 2025 19:58:05 +1000 Subject: [PATCH 01/11] feat: Remove stale validator keys during reload --- .../commandline/CommandlineParserTest.java | 4 +- .../pegasys/web3signer/core/Eth1Runner.java | 1 + .../pegasys/web3signer/core/Eth2Runner.java | 51 ++++++------ .../service/http/handlers/ReloadHandler.java | 23 +++--- .../config/DefaultArtifactSignerProvider.java | 46 +++++++---- .../DefaultArtifactSignerProviderTest.java | 16 ++-- .../PostLoadingValidatorsProcessor.java | 80 +++++++++++++++++++ 7 files changed, 162 insertions(+), 59 deletions(-) create mode 100644 slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java diff --git a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java index 23bb35904..d658243f3 100644 --- a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java +++ b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java @@ -688,7 +688,9 @@ public void run() {} @Override protected List createArtifactSignerProvider( final Vertx vertx, final MetricsSystem metricsSystem) { - return List.of(new DefaultArtifactSignerProvider(Collections::emptyList, Optional.empty())); + return List.of( + new DefaultArtifactSignerProvider( + Collections::emptyList, Optional.empty(), Optional.empty())); } @Override diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java index 4a470d229..aab855979 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java @@ -116,6 +116,7 @@ protected List createArtifactSignerProvider( .getValues()); return signers; }, + Optional.empty(), Optional.empty()); // uses eth1 address as identifier diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java index eebd89ba7..2107efd47 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java @@ -58,16 +58,18 @@ import tech.pegasys.web3signer.signing.config.metadata.yubihsm.YubiHsmOpaqueDataProvider; import tech.pegasys.web3signer.slashingprotection.DbHealthCheck; import tech.pegasys.web3signer.slashingprotection.DbPrunerRunner; +import tech.pegasys.web3signer.slashingprotection.PostLoadingValidatorsProcessor; import tech.pegasys.web3signer.slashingprotection.SlashingProtectionContext; import tech.pegasys.web3signer.slashingprotection.SlashingProtectionContextFactory; import tech.pegasys.web3signer.slashingprotection.SlashingProtectionParameters; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; +import java.util.function.Supplier; import io.vertx.core.Vertx; import io.vertx.core.json.JsonObject; @@ -155,30 +157,25 @@ protected List createArtifactSignerProvider( final Vertx vertx, final MetricsSystem metricsSystem) { return List.of( new DefaultArtifactSignerProvider( - () -> { - try (final AzureKeyVaultFactory azureKeyVaultFactory = new AzureKeyVaultFactory()) { - final List signers = new ArrayList<>(); - signers.addAll( - loadSignersFromKeyConfigFiles(vertx, azureKeyVaultFactory, metricsSystem) - .getValues()); - signers.addAll(bulkLoadSigners(azureKeyVaultFactory).getValues()); + createArtifactSignerSupplier(vertx, metricsSystem), + slashingProtectionContext.map(PostLoadingValidatorsProcessor::new), + Optional.of(commitBoostApiParameters))); + } - final List validators = - signers.stream() - .map(ArtifactSigner::getIdentifier) - .map(Bytes::fromHexString) - .collect(Collectors.toList()); - if (validators.isEmpty()) { - LOG.warn("No BLS keys loaded. Check that the key store has BLS key config files"); - } else { - slashingProtectionContext.ifPresent( - context -> context.getRegisteredValidators().registerValidators(validators)); - } + private Supplier> createArtifactSignerSupplier( + final Vertx vertx, final MetricsSystem metricsSystem) { + return () -> { + try (final AzureKeyVaultFactory azureKeyVaultFactory = new AzureKeyVaultFactory()) { + final List signers = new ArrayList<>(); + // load keys from key config files + signers.addAll( + loadSignersFromKeyConfigFiles(vertx, azureKeyVaultFactory, metricsSystem).getValues()); + // bulk load keys + signers.addAll(bulkLoadSigners(azureKeyVaultFactory).getValues()); - return signers; - } - }, - Optional.of(commitBoostApiParameters))); + return signers; + } + }; } private MappedResults loadSignersFromKeyConfigFiles( @@ -302,9 +299,7 @@ private void registerSignerLoadingHealthCheck( @Override public void run() { super.run(); - if (pruningEnabled && slashingProtectionContext.isPresent()) { - scheduleAndExecuteInitialDbPruning(); - } + scheduleAndExecuteInitialDbPruning(); slashingProtectionContext.ifPresent(this::scheduleDbHealthCheck); } @@ -326,6 +321,10 @@ private void scheduleDbHealthCheck(final SlashingProtectionContext protectionCon } private void scheduleAndExecuteInitialDbPruning() { + if (!pruningEnabled || slashingProtectionContext.isEmpty()) { + return; + } + final DbPrunerRunner dbPrunerRunner = new DbPrunerRunner( slashingProtectionParameters, diff --git a/core/src/main/java/tech/pegasys/web3signer/core/service/http/handlers/ReloadHandler.java b/core/src/main/java/tech/pegasys/web3signer/core/service/http/handlers/ReloadHandler.java index 4c7277bdf..c41d82394 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/service/http/handlers/ReloadHandler.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/service/http/handlers/ReloadHandler.java @@ -22,27 +22,26 @@ import io.vertx.ext.web.RoutingContext; public class ReloadHandler implements Handler { - List orderedArtifactSignerProviders; + private final List orderedArtifactSignerProviders; - public ReloadHandler(List orderedArtifactSignerProviders) { + public ReloadHandler(final List orderedArtifactSignerProviders) { this.orderedArtifactSignerProviders = orderedArtifactSignerProviders; } @Override - public void handle(RoutingContext routingContext) { + public void handle(final RoutingContext routingContext) { Executors.newSingleThreadExecutor() .submit( () -> - orderedArtifactSignerProviders.stream() - .forEachOrdered( - signer -> { - try { - signer.load().get(); - } catch (InterruptedException | ExecutionException e) { - throw new RuntimeException(e); - } - })); + orderedArtifactSignerProviders.forEach( + signer -> { + try { + signer.load().get(); + } catch (InterruptedException | ExecutionException e) { + throw new RuntimeException(e); + } + })); routingContext.response().setStatusCode(200).end(); } } diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java index bfa2893b5..ca55f3388 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java @@ -33,6 +33,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Supplier; @@ -45,15 +46,19 @@ public class DefaultArtifactSignerProvider implements ArtifactSignerProvider { private static final Logger LOG = LogManager.getLogger(); private final Supplier> artifactSignerCollectionSupplier; + private final Optional, Set>> postLoadingCallback; + private final Optional commitBoostKeystoresParameters; + private final Map signers = new HashMap<>(); private final Map> proxySigners = new HashMap<>(); private final ExecutorService executorService = Executors.newSingleThreadExecutor(); - private final Optional commitBoostKeystoresParameters; public DefaultArtifactSignerProvider( final Supplier> artifactSignerCollectionSupplier, + final Optional, Set>> postLoadingCallback, final Optional commitBoostKeystoresParameters) { this.artifactSignerCollectionSupplier = artifactSignerCollectionSupplier; + this.postLoadingCallback = postLoadingCallback; this.commitBoostKeystoresParameters = commitBoostKeystoresParameters; } @@ -62,20 +67,31 @@ public Future load() { return executorService.submit( () -> { LOG.debug("Signer keys pre-loaded in memory {}", signers.size()); - - artifactSignerCollectionSupplier.get().stream() - .collect( - Collectors.toMap( - ArtifactSigner::getIdentifier, - Function.identity(), - (signer1, signer2) -> { - LOG.warn( - "Duplicate keys were found while loading. {}", Function.identity()); - return signer1; - })) - .forEach(signers::putIfAbsent); - - // for each loaded signer, load commit boost proxy signers (if any) + // step 1: Create copy of current signers + final Map oldSigners = new HashMap<>(signers); + // step 2: Clear current signers and then load them via ArtifactSignerCollectionSupplier + signers.clear(); + signers.putAll( + artifactSignerCollectionSupplier.get().stream() + .collect( + Collectors.toMap( + ArtifactSigner::getIdentifier, + Function.identity(), + (signer1, signer2) -> { + LOG.warn( + "Duplicate keys were found while loading. {}", Function.identity()); + return signer1; + }))); + + // step 3: Collect all stale keys that are no longer valid + final Set staleKeys = new HashSet<>(oldSigners.keySet()); + staleKeys.removeAll(signers.keySet()); + + // step 4: callback to register new keys and disable stale keys in slashing database + postLoadingCallback.ifPresent(callback -> callback.accept(signers.keySet(), staleKeys)); + + // step 5: for each loaded signer, load commit boost proxy signers (if any) + proxySigners.clear(); commitBoostKeystoresParameters .filter(KeystoresParameters::isEnabled) .ifPresent( diff --git a/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java b/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java index 962db1102..37e593f35 100644 --- a/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java +++ b/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java @@ -67,7 +67,9 @@ void signerReturnedForMatchingIdentifier() { final ArtifactSigner mockSigner = mock(ArtifactSigner.class); when(mockSigner.getIdentifier()).thenReturn(PUBLIC_KEY1); - signerProvider = new DefaultArtifactSignerProvider(() -> List.of(mockSigner), Optional.empty()); + signerProvider = + new DefaultArtifactSignerProvider( + () -> List.of(mockSigner), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); final Optional signer = signerProvider.getSigner(PUBLIC_KEY1); @@ -84,7 +86,7 @@ void signerProviderOnlyHasSingleEntryIfPassedInListHasMultipleMatchingSigners() signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner1, mockSigner2), Optional.empty()); + () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); assertThat(signerProvider.availableIdentifiers()).hasSize(1); @@ -100,7 +102,7 @@ void signerProviderCanMapInTwoSigners() { signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner1, mockSigner2), Optional.empty()); + () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); assertThat(signerProvider.availableIdentifiers()).hasSize(2); assertThat(signerProvider.availableIdentifiers()).containsOnly(PUBLIC_KEY1, PUBLIC_KEY2); @@ -129,7 +131,9 @@ void proxySignersAreLoadedCorrectly() throws IOException { signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner1, mockSigner2), Optional.of(commitBoostParameters)); + () -> List.of(mockSigner1, mockSigner2), + Optional.empty(), + Optional.of(commitBoostParameters)); // methods under test assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); @@ -167,7 +171,9 @@ void emptyProxySignersAreLoadedSuccessfully() { signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner1, mockSigner2), Optional.of(commitBoostParameters)); + () -> List.of(mockSigner1, mockSigner2), + Optional.empty(), + Optional.of(commitBoostParameters)); // methods under test assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); diff --git a/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java b/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java new file mode 100644 index 000000000..93c42bfef --- /dev/null +++ b/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java @@ -0,0 +1,80 @@ +/* + * Copyright 2025 ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package tech.pegasys.web3signer.slashingprotection; + +import tech.pegasys.web3signer.slashingprotection.dao.ValidatorsDao; + +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.function.BiConsumer; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.tuweni.bytes.Bytes; +import org.jdbi.v3.core.Jdbi; + +/** + * Process new validators by registering them with slashing database and disable stale validators. + */ +public class PostLoadingValidatorsProcessor implements BiConsumer, Set> { + private static final Logger LOG = LogManager.getLogger(); + private static final ValidatorsDao VALIDATORS_DAO = new ValidatorsDao(); + + private final SlashingProtectionContext slashingProtectionContext; + + public PostLoadingValidatorsProcessor(final SlashingProtectionContext slashingProtectionContext) { + this.slashingProtectionContext = slashingProtectionContext; + } + + @Override + public void accept(final Set newValidators, final Set staleValidators) { + registerNewValidators(newValidators); + disableStaleValidators(staleValidators); + } + + private void registerNewValidators(final Set newValidators) { + if (newValidators.isEmpty()) { + return; + } + + final List validatorsList = newValidators.stream().map(Bytes::fromHexString).toList(); + slashingProtectionContext.getRegisteredValidators().registerValidators(validatorsList); + } + + private void disableStaleValidators(final Set staleValidators) { + if (staleValidators.isEmpty()) { + return; + } + + final RegisteredValidators registeredValidators = + slashingProtectionContext.getRegisteredValidators(); + final Jdbi jdbi = slashingProtectionContext.getSlashingProtectionJdbi(); + jdbi.useTransaction( + handle -> { + // disable the validators in the database + staleValidators.forEach( + publicKey -> { + final Optional validatorId = + registeredValidators.getValidatorIdForPublicKey(Bytes.fromHexString(publicKey)); + if (validatorId.isPresent()) { + DbLocker.lockAllForValidator(handle, validatorId.get()); + VALIDATORS_DAO.setEnabled(handle, validatorId.get(), false); + } else { + LOG.trace( + "Validator with public key {} not found in database to disable", publicKey); + } + }); + }); + } +} From 1a12cc7805fd96f69dafaab5e9db070b464f4890 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Mon, 13 Jan 2025 23:06:59 +1000 Subject: [PATCH 02/11] Introduce backward compatibility flag --- CHANGELOG.md | 9 ++++++ .../KeyIdentifiersAcceptanceTest.java | 4 +-- .../commandline/Web3SignerBaseCommand.java | 30 ++++++++----------- .../commandline/CommandlineParserTest.java | 5 +++- .../jsonrpcproxy/support/TestBaseConfig.java | 5 ++++ .../pegasys/web3signer/core/Eth1Runner.java | 4 ++- .../pegasys/web3signer/core/Eth2Runner.java | 4 ++- .../web3signer/core/config/BaseConfig.java | 2 ++ .../config/DefaultArtifactSignerProvider.java | 21 ++++++++++--- .../signing/config/SignerLoader.java | 12 ++++---- .../DefaultArtifactSignerProviderTest.java | 8 +++-- .../signing/config/SignerLoaderTest.java | 2 +- 12 files changed, 69 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f27906c0..a0ae0cd42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## Next Release +### Breaking Changes +- The behavior of reload API endpoint has been changed. It will remove all in-memory keys before loading them again +using configuration files and bulk loading. This unfortunately results in behaviour change of local configuration files +pointing to encrypted keystores. Previously cached local configuration files were avoided during reload API call. +To keep the old behavior `--reload-keep-stale-keys=true` option can be used which will not remove stale keys during +reload API call. + +--- ## 24.12.0 ### Breaking Changes diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java index 843fb91c3..3a29a3bf5 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java @@ -147,7 +147,7 @@ public void healthCheckReportsKeysLoadedAfterReloadInEth2Mode() { @ParameterizedTest @EnumSource(value = KeyType.class) - public void alreadyLoadedPublicKeysAreNotRemovedAfterReload(final KeyType keyType) { + public void removedConfigFilePublicKeyIsRemovedAfterReload(final KeyType keyType) { final String[] prvKeys = privateKeys(keyType); final String[] keys = createKeys(keyType, true, prvKeys); @@ -161,7 +161,7 @@ public void alreadyLoadedPublicKeysAreNotRemovedAfterReload(final KeyType keyTyp // reload API call signer.callReload().then().statusCode(200); - validateApiResponse(signer.callApiPublicKeys(keyType), containsInAnyOrder(keys)); + validateApiResponse(signer.callApiPublicKeys(keyType), contains(keys[0])); } @ParameterizedTest diff --git a/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java b/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java index 70dbbe8f3..27702f720 100644 --- a/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java +++ b/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java @@ -213,9 +213,13 @@ public class Web3SignerBaseCommand implements BaseConfig, Runnable { paramLabel = INTEGER_FORMAT_HELP) private Integer vertxWorkerPoolSize = null; - @Deprecated(forRemoval = true) - @Option(names = "--Xworker-pool-size", hidden = true) - private Integer deprecatedWorkerPoolSize = null; + @Option( + names = "--reload-keep-stale-keys", + description = + "Set to true to keep stale keys loaded during reload API call. (default: ${DEFAULT-VALUE})", + paramLabel = "", + arity = "1") + private boolean reloadKeepStaleKeys = false; @CommandLine.Mixin private PicoCliTlsServerOptions picoCliTlsServerOptions; @@ -323,22 +327,18 @@ public boolean keystoreParallelProcessingEnabled() { @Override public int getVertxWorkerPoolSize() { - // both values are not allowed on cli, they will be verified in validateArgs() ... - if (vertxWorkerPoolSize != null && deprecatedWorkerPoolSize != null) { - return -1; - } - if (vertxWorkerPoolSize != null) { return vertxWorkerPoolSize; } - if (deprecatedWorkerPoolSize != null) { - return deprecatedWorkerPoolSize; - } - return VERTX_WORKER_POOL_SIZE_DEFAULT; } + @Override + public boolean reloadKeepStaleKeys() { + return reloadKeepStaleKeys; + } + @Override public String toString() { return MoreObjects.toStringHelper(this) @@ -386,12 +386,6 @@ public void validateArgs() { "--metrics-enabled option and --metrics-push-enabled option can't be used at the same " + "time. Please refer to CLI reference for more details about this constraint."); } - - if (vertxWorkerPoolSize != null && deprecatedWorkerPoolSize != null) { - throw new CommandLine.MutuallyExclusiveArgsException( - spec.commandLine(), - "--vertx-worker-pool-size option and --Xworker-pool-size option can't be used at the same time."); - } } public static class Web3signerMetricCategoryConverter extends MetricCategoryConverter { diff --git a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java index d658243f3..4e2af8edf 100644 --- a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java +++ b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java @@ -690,7 +690,10 @@ protected List createArtifactSignerProvider( final Vertx vertx, final MetricsSystem metricsSystem) { return List.of( new DefaultArtifactSignerProvider( - Collections::emptyList, Optional.empty(), Optional.empty())); + baseConfig.reloadKeepStaleKeys(), + Collections::emptyList, + Optional.empty(), + Optional.empty())); } @Override diff --git a/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java b/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java index a8ef1a848..6cd3cd195 100644 --- a/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java +++ b/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java @@ -141,4 +141,9 @@ public boolean keystoreParallelProcessingEnabled() { public int getVertxWorkerPoolSize() { return 20; } + + @Override + public boolean reloadKeepStaleKeys() { + return false; + } } diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java index aab855979..56b0f7d2f 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java @@ -92,6 +92,7 @@ protected List createArtifactSignerProvider( final ArtifactSignerProvider signerProvider = new DefaultArtifactSignerProvider( + baseConfig.reloadKeepStaleKeys(), () -> { final List signers = new ArrayList<>(); final AzureKeyVaultFactory azureKeyVaultFactory = new AzureKeyVaultFactory(); @@ -149,7 +150,8 @@ private MappedResults loadSignersFromKeyConfigFiles( awsKmsSignerFactory, true); - return new SignerLoader(baseConfig.keystoreParallelProcessingEnabled()) + return new SignerLoader( + baseConfig.keystoreParallelProcessingEnabled(), baseConfig.reloadKeepStaleKeys()) .load( baseConfig.getKeyConfigPath(), "yaml", diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java index 2107efd47..7b587fc21 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java @@ -157,6 +157,7 @@ protected List createArtifactSignerProvider( final Vertx vertx, final MetricsSystem metricsSystem) { return List.of( new DefaultArtifactSignerProvider( + baseConfig.reloadKeepStaleKeys(), createArtifactSignerSupplier(vertx, metricsSystem), slashingProtectionContext.map(PostLoadingValidatorsProcessor::new), Optional.of(commitBoostApiParameters))); @@ -201,7 +202,8 @@ private MappedResults loadSignersFromKeyConfigFiles( azureKeyVaultFactory); final MappedResults results = - new SignerLoader(baseConfig.keystoreParallelProcessingEnabled()) + new SignerLoader( + baseConfig.keystoreParallelProcessingEnabled(), baseConfig.reloadKeepStaleKeys()) .load( baseConfig.getKeyConfigPath(), "yaml", diff --git a/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java b/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java index 25384d827..04d4de8a3 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java @@ -64,4 +64,6 @@ public interface BaseConfig { boolean keystoreParallelProcessingEnabled(); int getVertxWorkerPoolSize(); + + boolean reloadKeepStaleKeys(); } diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java index ca55f3388..18201c203 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java @@ -45,6 +45,8 @@ public class DefaultArtifactSignerProvider implements ArtifactSignerProvider { private static final Logger LOG = LogManager.getLogger(); + + private final boolean reloadKeepStaleKeys; private final Supplier> artifactSignerCollectionSupplier; private final Optional, Set>> postLoadingCallback; private final Optional commitBoostKeystoresParameters; @@ -54,9 +56,11 @@ public class DefaultArtifactSignerProvider implements ArtifactSignerProvider { private final ExecutorService executorService = Executors.newSingleThreadExecutor(); public DefaultArtifactSignerProvider( + final boolean reloadKeepStaleKeys, final Supplier> artifactSignerCollectionSupplier, final Optional, Set>> postLoadingCallback, final Optional commitBoostKeystoresParameters) { + this.reloadKeepStaleKeys = reloadKeepStaleKeys; this.artifactSignerCollectionSupplier = artifactSignerCollectionSupplier; this.postLoadingCallback = postLoadingCallback; this.commitBoostKeystoresParameters = commitBoostKeystoresParameters; @@ -70,7 +74,9 @@ public Future load() { // step 1: Create copy of current signers final Map oldSigners = new HashMap<>(signers); // step 2: Clear current signers and then load them via ArtifactSignerCollectionSupplier - signers.clear(); + if (!reloadKeepStaleKeys) { + signers.clear(); + } signers.putAll( artifactSignerCollectionSupplier.get().stream() .collect( @@ -84,14 +90,21 @@ public Future load() { }))); // step 3: Collect all stale keys that are no longer valid - final Set staleKeys = new HashSet<>(oldSigners.keySet()); - staleKeys.removeAll(signers.keySet()); + final Set staleKeys; + if (reloadKeepStaleKeys) { + staleKeys = new HashSet<>(); + } else { + staleKeys = new HashSet<>(oldSigners.keySet()); + staleKeys.removeAll(signers.keySet()); + } // step 4: callback to register new keys and disable stale keys in slashing database postLoadingCallback.ifPresent(callback -> callback.accept(signers.keySet(), staleKeys)); // step 5: for each loaded signer, load commit boost proxy signers (if any) - proxySigners.clear(); + if (!reloadKeepStaleKeys) { + proxySigners.clear(); + } commitBoostKeystoresParameters .filter(KeystoresParameters::isEnabled) .ifPresent( diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java index e3f53378a..08a5da0b4 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java @@ -51,15 +51,14 @@ public class SignerLoader { private static final long FILES_PROCESSED_TO_REPORT = 10; // enable or disable parallel streams to convert and load private keys from metadata files private final boolean useParallelStreams; + // if false, reload metadata files even if they are not modified + private final boolean reloadKeepStaleFiles; private static final Map metadataConfigFilesPathCache = new HashMap<>(); - public SignerLoader(final boolean useParallelStreams) { + public SignerLoader(final boolean useParallelStreams, final boolean reloadKeepStaleFiles) { this.useParallelStreams = useParallelStreams; - } - - public SignerLoader() { - this(true); + this.reloadKeepStaleFiles = reloadKeepStaleFiles; } public MappedResults load( @@ -173,7 +172,8 @@ private boolean isNewOrModifiedMetadataFile(final Path path) { try { final FileTime lastModifiedTime = Files.getLastModifiedTime(path); if (metadataConfigFilesPathCache.containsKey(path)) { - if (metadataConfigFilesPathCache.get(path).compareTo(lastModifiedTime) == 0) { + if (reloadKeepStaleFiles + && metadataConfigFilesPathCache.get(path).compareTo(lastModifiedTime) == 0) { return false; } } diff --git a/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java b/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java index 37e593f35..c0d10bed7 100644 --- a/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java +++ b/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java @@ -69,7 +69,7 @@ void signerReturnedForMatchingIdentifier() { signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner), Optional.empty(), Optional.empty()); + false, () -> List.of(mockSigner), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); final Optional signer = signerProvider.getSigner(PUBLIC_KEY1); @@ -86,7 +86,7 @@ void signerProviderOnlyHasSingleEntryIfPassedInListHasMultipleMatchingSigners() signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); + false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); assertThat(signerProvider.availableIdentifiers()).hasSize(1); @@ -102,7 +102,7 @@ void signerProviderCanMapInTwoSigners() { signerProvider = new DefaultArtifactSignerProvider( - () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); + false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); assertThat(signerProvider.availableIdentifiers()).hasSize(2); assertThat(signerProvider.availableIdentifiers()).containsOnly(PUBLIC_KEY1, PUBLIC_KEY2); @@ -131,6 +131,7 @@ void proxySignersAreLoadedCorrectly() throws IOException { signerProvider = new DefaultArtifactSignerProvider( + false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.of(commitBoostParameters)); @@ -171,6 +172,7 @@ void emptyProxySignersAreLoadedSuccessfully() { signerProvider = new DefaultArtifactSignerProvider( + false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.of(commitBoostParameters)); diff --git a/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java b/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java index 521ef5e5a..57626f33d 100644 --- a/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java +++ b/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java @@ -108,7 +108,7 @@ public void setup() { signerParser = new YamlSignerParser( List.of(blsArtifactSignerFactory), YamlMapperFactory.createYamlMapper()); - signerLoader = new SignerLoader(); + signerLoader = new SignerLoader(true, true); } @ParameterizedTest(name = "{index} - Signer created for file name {0}") From b8c1ca03f220f5056f5ff08c2c2c1c6bc5fee371 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Tue, 14 Jan 2025 09:59:15 +1000 Subject: [PATCH 03/11] fixing unit test for deprecated cli option --- .../commandline/CommandlineParserTest.java | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java index 4e2af8edf..1020e7744 100644 --- a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java +++ b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java @@ -559,21 +559,6 @@ void awsWithoutModeDefaultsToSpecified() { .contains("v1", "v2", "v3"); } - @Test - void vertxWorkerPoolSizeWithWorkerPoolSizeFailsToParse() { - String cmdline = validBaseCommandOptions(); - cmdline += - "--vertx-worker-pool-size=30 --Xworker-pool-size=40 eth2 --slashing-protection-enabled=false"; - - parser.registerSubCommands(new MockEth2SubCommand()); - final int result = parser.parseCommandLine(cmdline.split(" ")); - - assertThat(result).isNotZero(); - assertThat(commandError.toString()) - .contains( - "Error parsing parameters: --vertx-worker-pool-size option and --Xworker-pool-size option can't be used at the same time."); - } - @Test void vertxWorkerPoolSizeDefaultParsesSuccessfully() { String cmdline = validBaseCommandOptions(); @@ -587,19 +572,6 @@ void vertxWorkerPoolSizeDefaultParsesSuccessfully() { assertThat(mockEth2SubCommand.getConfig().getVertxWorkerPoolSize()).isEqualTo(20); } - @Test - void vertxWorkerPoolSizeDeprecatedParsesSuccessfully() { - String cmdline = validBaseCommandOptions(); - cmdline += "--Xworker-pool-size=40 eth2 --slashing-protection-enabled=false"; - - MockEth2SubCommand mockEth2SubCommand = new MockEth2SubCommand(); - parser.registerSubCommands(mockEth2SubCommand); - final int result = parser.parseCommandLine(cmdline.split(" ")); - - assertThat(result).isZero(); - assertThat(mockEth2SubCommand.getConfig().getVertxWorkerPoolSize()).isEqualTo(40); - } - @Test void vertxWorkerPoolSizeParsesSuccessfully() { String cmdline = validBaseCommandOptions(); From dc13031ec0acdd81797ea8d68500b3b3e8f34d64 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Tue, 14 Jan 2025 12:15:24 +1000 Subject: [PATCH 04/11] Fix and add AT --- .../dsl/signer/SignerConfiguration.java | 9 ++++- .../signer/SignerConfigurationBuilder.java | 9 ++++- .../runner/CmdLineParamsConfigFileImpl.java | 8 +++++ .../runner/CmdLineParamsDefaultImpl.java | 5 +++ .../KeyIdentifiersAcceptanceTest.java | 33 +++++++++++++++++-- .../KeyIdentifiersAcceptanceTestBase.java | 9 +++++ 6 files changed, 69 insertions(+), 4 deletions(-) diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java index dc172c53e..9b129ee86 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java @@ -83,6 +83,7 @@ public class SignerConfiguration { private final boolean signingExtEnabled; private Optional> commitBoostParameters; + private final Optional reloadKeepStaleKeys; public SignerConfiguration( final String hostname, @@ -131,7 +132,8 @@ public SignerConfiguration( final ChainIdProvider chainIdProvider, final Optional v3KeystoresBulkloadParameters, final boolean signingExtEnabled, - final Optional> commitBoostParameters) { + final Optional> commitBoostParameters, + final Optional reloadKeepStaleKeys) { this.hostname = hostname; this.logLevel = logLevel; this.httpRpcPort = httpRpcPort; @@ -179,6 +181,7 @@ public SignerConfiguration( this.v3KeystoresBulkloadParameters = v3KeystoresBulkloadParameters; this.signingExtEnabled = signingExtEnabled; this.commitBoostParameters = commitBoostParameters; + this.reloadKeepStaleKeys = reloadKeepStaleKeys; } public String hostname() { @@ -376,4 +379,8 @@ public boolean isSigningExtEnabled() { public Optional> getCommitBoostParameters() { return commitBoostParameters; } + + public Optional getReloadKeepStaleKeys() { + return reloadKeepStaleKeys; + } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java index c7160ffc9..2eee742a6 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java @@ -87,6 +87,7 @@ public class SignerConfigurationBuilder { private boolean signingExtEnabled; private Pair commitBoostParameters; + private Optional reloadKeepStaleKeys = Optional.empty(); public SignerConfigurationBuilder withLogLevel(final Level logLevel) { this.logLevel = logLevel; @@ -339,6 +340,11 @@ public SignerConfigurationBuilder withCommitBoostParameters( return this; } + public SignerConfigurationBuilder withReloadKeepStaleKeys(final boolean reloadKeepStaleKeys) { + this.reloadKeepStaleKeys = Optional.of(reloadKeepStaleKeys); + return this; + } + public SignerConfiguration build() { if (mode == null) { throw new IllegalArgumentException("Mode cannot be null"); @@ -390,6 +396,7 @@ public SignerConfiguration build() { chainIdProvider, Optional.ofNullable(v3KeystoresBulkloadParameters), signingExtEnabled, - Optional.ofNullable(commitBoostParameters)); + Optional.ofNullable(commitBoostParameters), + reloadKeepStaleKeys); } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java index e9fcb2bd3..66acb01ea 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java @@ -123,6 +123,14 @@ public List createCmdLineParams() { yamlConfig.append(createServerTlsArgs()); + signerConfig + .getReloadKeepStaleKeys() + .ifPresent( + reloadKeepStaleKeys -> + yamlConfig.append( + String.format( + YAML_BOOLEAN_FMT, "reload-keep-stale-keys", reloadKeepStaleKeys))); + params.add(signerConfig.getMode()); // sub-command .. it can't go to config file if (signerConfig.getMode().equals("eth2")) { diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java index 4875a2932..713a8502e 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java @@ -107,6 +107,11 @@ public List createCmdLineParams() { params.addAll(createServerTlsArgs()); + signerConfig + .getReloadKeepStaleKeys() + .ifPresent( + reloadKeepStaleKeys -> params.add("--reload-keep-stale-keys=" + reloadKeepStaleKeys)); + params.add(signerConfig.getMode()); if (signerConfig.getMode().equals("eth2")) { diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java index 3a29a3bf5..7cc3afa3d 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java @@ -147,7 +147,7 @@ public void healthCheckReportsKeysLoadedAfterReloadInEth2Mode() { @ParameterizedTest @EnumSource(value = KeyType.class) - public void removedConfigFilePublicKeyIsRemovedAfterReload(final KeyType keyType) { + public void publicKeysAreRemovedAfterReloadDefault(final KeyType keyType) { final String[] prvKeys = privateKeys(keyType); final String[] keys = createKeys(keyType, true, prvKeys); @@ -161,7 +161,36 @@ public void removedConfigFilePublicKeyIsRemovedAfterReload(final KeyType keyType // reload API call signer.callReload().then().statusCode(200); - validateApiResponse(signer.callApiPublicKeys(keyType), contains(keys[0])); + // reload is async ... assert that the key is removed + Awaitility.await() + .atMost(5, SECONDS) + .until( + () -> signer.callApiPublicKeys(keyType).jsonPath().>get("."), + containsInAnyOrder(keys[0])); + } + + @ParameterizedTest + @EnumSource(value = KeyType.class) + public void publicKeysNotRemovedAfterReloadWithKeepStaleKeysTrue(final KeyType keyType) { + final String[] prvKeys = privateKeys(keyType); + final String[] keys = createKeys(keyType, true, prvKeys); + + initAndStartSignerWithReloadKeepStaleKeys(calculateMode(keyType)); + + validateApiResponse(signer.callApiPublicKeys(keyType), containsInAnyOrder(keys)); + + // remove one of the key config file + assertThat(testDirectory.resolve(keys[1] + ".yaml").toFile().delete()).isTrue(); + + // reload API call + signer.callReload().then().statusCode(200); + + // reload is async ... assert that the keys are not removed + Awaitility.await() + .atMost(5, SECONDS) + .until( + () -> signer.callApiPublicKeys(keyType).jsonPath().>get("."), + containsInAnyOrder(keys)); } @ParameterizedTest diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java index 0a7732343..081f788ff 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java @@ -141,6 +141,15 @@ protected void initAndStartSigner(final String mode) { startSigner(builder.build()); } + protected void initAndStartSignerWithReloadKeepStaleKeys(final String mode) { + startSigner( + new SignerConfigurationBuilder() + .withKeyStoreDirectory(testDirectory) + .withMode(mode) + .withReloadKeepStaleKeys(true) + .build()); + } + protected Response callApiPublicKeysWithoutOpenApiClientSideFilter(final KeyType keyType) { return given().baseUri(signer.getUrl()).accept("").get(Signer.publicKeysPath(keyType)); } From 76dbde106c281a28d0426f08c829cdd0e4d8411e Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Tue, 14 Jan 2025 12:38:32 +1000 Subject: [PATCH 05/11] Update AT assertion --- .../KeyIdentifiersAcceptanceTest.java | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java index 7cc3afa3d..d1073a3a4 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java @@ -164,9 +164,12 @@ public void publicKeysAreRemovedAfterReloadDefault(final KeyType keyType) { // reload is async ... assert that the key is removed Awaitility.await() .atMost(5, SECONDS) - .until( - () -> signer.callApiPublicKeys(keyType).jsonPath().>get("."), - containsInAnyOrder(keys[0])); + .untilAsserted( + () -> { + final List publicKeysList = + signer.callApiPublicKeys(keyType).jsonPath().getList("."); + assertThat(publicKeysList).containsOnly(keys[0]); + }); } @ParameterizedTest @@ -188,9 +191,12 @@ public void publicKeysNotRemovedAfterReloadWithKeepStaleKeysTrue(final KeyType k // reload is async ... assert that the keys are not removed Awaitility.await() .atMost(5, SECONDS) - .until( - () -> signer.callApiPublicKeys(keyType).jsonPath().>get("."), - containsInAnyOrder(keys)); + .untilAsserted( + () -> { + final List publicKeysList = + signer.callApiPublicKeys(keyType).jsonPath().getList("."); + assertThat(publicKeysList).containsExactlyInAnyOrder(keys); + }); } @ParameterizedTest From 3da805ed12a4c14999373a641d3f4d2e01e1feaa Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Tue, 14 Jan 2025 15:32:43 +1000 Subject: [PATCH 06/11] Update changelog --- CHANGELOG.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0ae0cd42..a7da46620 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,12 +1,20 @@ # Changelog ## Next Release + ### Breaking Changes -- The behavior of reload API endpoint has been changed. It will remove all in-memory keys before loading them again -using configuration files and bulk loading. This unfortunately results in behaviour change of local configuration files -pointing to encrypted keystores. Previously cached local configuration files were avoided during reload API call. -To keep the old behavior `--reload-keep-stale-keys=true` option can be used which will not remove stale keys during -reload API call. +- The behavior of reload API endpoint has been modified due to issue [#1018][issue_1018] implemented by PR [#1054][pr_1054]. +The reload API call will remove all in-memory keys before loading them again using configuration files and bulk loading. +This unfortunately results in behaviour change of local configuration files pointing to encrypted keystores. Previously +cached local configuration files were avoided being reloaded during reload API call, now they will be reloaded and +reparsed. To keep the old behavior `--reload-keep-stale-keys=true` option can be used which will not remove stale keys +during reload API call. + +[issue_1018]: https://github.com/Consensys/web3signer/issues/1018 +[pr_1054]: https://github.com/Consensys/web3signer/pull/1054 + +### Features Added +- Remove stale keys during reload API call. [#1018][issue_1018] [#1054][pr_1054] --- ## 24.12.0 From 8f6f0fd3a4e314f6bb567d4f694a060610c4e267 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Wed, 15 Jan 2025 10:35:33 +1000 Subject: [PATCH 07/11] Update comment --- .../signing/config/DefaultArtifactSignerProvider.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java index 18201c203..77e43716d 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java @@ -98,7 +98,8 @@ public Future load() { staleKeys.removeAll(signers.keySet()); } - // step 4: callback to register new keys and disable stale keys in slashing database + // step 4: callback to perform further actions specific to eth1/eth2 mode with loaded and + // stale keys. postLoadingCallback.ifPresent(callback -> callback.accept(signers.keySet(), staleKeys)); // step 5: for each loaded signer, load commit boost proxy signers (if any) From 338846aaed773647395d029d77046b899b684b7b Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Wed, 15 Jan 2025 12:35:04 +1000 Subject: [PATCH 08/11] trigger ci From 35680e92abc8e9d7b6ec62c9930e58b0f5ff6e41 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Thu, 16 Jan 2025 15:06:22 +1000 Subject: [PATCH 09/11] Remove disable validator logic in slashing db. Improved signers loading logic to avoid clearing the map. Also made sure maps are thread-safe by using concurrent maps --- .../config/DefaultArtifactSignerProvider.java | 114 ++++++++++-------- .../PostLoadingValidatorsProcessor.java | 55 +-------- 2 files changed, 70 insertions(+), 99 deletions(-) diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java index 77e43716d..d788150c2 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java @@ -25,16 +25,17 @@ import java.io.File; import java.nio.file.Path; import java.util.Collection; -import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.function.BiConsumer; import java.util.function.BiFunction; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -42,23 +43,33 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +/** + * Default implementation of {@link ArtifactSignerProvider} that loads signers and proxy signers. + * This class is designed to provide concurrent access to signers and proxy signers, ensuring thread + * safety and efficient read operations. The {@code load()} method is called infrequently, typically + * at startup or via a reload API call, while the getter methods are called frequently. + * + *

The class uses a {@link ConcurrentHashMap} for storing signers and proxy signers to allow + * efficient concurrent read access. It also uses a single-threaded executor to ensure that the + * {@code load()} method and other write operations are executed sequentially. + */ public class DefaultArtifactSignerProvider implements ArtifactSignerProvider { private static final Logger LOG = LogManager.getLogger(); private final boolean reloadKeepStaleKeys; private final Supplier> artifactSignerCollectionSupplier; - private final Optional, Set>> postLoadingCallback; + private final Optional>> postLoadingCallback; private final Optional commitBoostKeystoresParameters; - private final Map signers = new HashMap<>(); - private final Map> proxySigners = new HashMap<>(); + private final ConcurrentMap signers = new ConcurrentHashMap<>(); + private final ConcurrentMap> proxySigners = new ConcurrentHashMap<>(); private final ExecutorService executorService = Executors.newSingleThreadExecutor(); public DefaultArtifactSignerProvider( final boolean reloadKeepStaleKeys, final Supplier> artifactSignerCollectionSupplier, - final Optional, Set>> postLoadingCallback, + final Optional>> postLoadingCallback, final Optional commitBoostKeystoresParameters) { this.reloadKeepStaleKeys = reloadKeepStaleKeys; this.artifactSignerCollectionSupplier = artifactSignerCollectionSupplier; @@ -71,13 +82,9 @@ public Future load() { return executorService.submit( () -> { LOG.debug("Signer keys pre-loaded in memory {}", signers.size()); - // step 1: Create copy of current signers - final Map oldSigners = new HashMap<>(signers); - // step 2: Clear current signers and then load them via ArtifactSignerCollectionSupplier - if (!reloadKeepStaleKeys) { - signers.clear(); - } - signers.putAll( + + // Load new signers into a temporary map - this is time-consuming logic + final Map newSigners = artifactSignerCollectionSupplier.get().stream() .collect( Collectors.toMap( @@ -87,47 +94,50 @@ public Future load() { LOG.warn( "Duplicate keys were found while loading. {}", Function.identity()); return signer1; - }))); - - // step 3: Collect all stale keys that are no longer valid - final Set staleKeys; - if (reloadKeepStaleKeys) { - staleKeys = new HashSet<>(); - } else { - staleKeys = new HashSet<>(oldSigners.keySet()); - staleKeys.removeAll(signers.keySet()); - } + })); - // step 4: callback to perform further actions specific to eth1/eth2 mode with loaded and - // stale keys. - postLoadingCallback.ifPresent(callback -> callback.accept(signers.keySet(), staleKeys)); + // Collect all stale keys that are no longer valid + final Set staleKeys = new HashSet<>(signers.keySet()); + staleKeys.removeAll(newSigners.keySet()); - // step 5: for each loaded signer, load commit boost proxy signers (if any) + // Update the signers map with new signers + signers.putAll(newSigners); + // Conditionally remove stale keys from signers map if (!reloadKeepStaleKeys) { - proxySigners.clear(); + staleKeys.forEach(signers::remove); } + + // Callback to perform further actions specific to eth1/eth2 mode (if any) + postLoadingCallback.ifPresent(callback -> callback.accept(signers.keySet())); + + // For each loaded signer, load commit boost proxy signers (if any) commitBoostKeystoresParameters .filter(KeystoresParameters::isEnabled) .ifPresent( - keystoreParameter -> - signers - .keySet() - .forEach( - consensusPubKey -> { - LOG.trace( - "Loading proxy signers for signer '{}' ...", consensusPubKey); - loadProxySigners( - keystoreParameter, - consensusPubKey, - SECP256K1.name(), - SecpV3KeystoresBulkLoader::loadECDSAProxyKeystores); - - loadProxySigners( - keystoreParameter, - consensusPubKey, - BLS.name(), - BlsKeystoreBulkLoader::loadKeystoresUsingPasswordFile); - })); + keystoreParameter -> { + signers + .keySet() + .forEach( + consensusPubKey -> { + LOG.trace( + "Loading proxy signers for signer '{}' ...", consensusPubKey); + loadProxySigners( + keystoreParameter, + consensusPubKey, + SECP256K1.name(), + SecpV3KeystoresBulkLoader::loadECDSAProxyKeystores); + + loadProxySigners( + keystoreParameter, + consensusPubKey, + BLS.name(), + BlsKeystoreBulkLoader::loadKeystoresUsingPasswordFile); + }); + // Conditionally remove stale proxy signers + if (!reloadKeepStaleKeys) { + staleKeys.forEach(proxySigners::remove); + } + }); LOG.info("Total signers (keys) currently loaded in memory: {}", signers.size()); return null; @@ -160,7 +170,7 @@ public Set availableIdentifiers() { @Override public Map> getProxyIdentifiers(final String consensusPubKey) { final Set artifactSigners = - proxySigners.computeIfAbsent(consensusPubKey, k -> Set.of()); + proxySigners.computeIfAbsent(consensusPubKey, k -> ConcurrentHashMap.newKeySet()); return artifactSigners.stream() .collect( Collectors.groupingBy( @@ -194,7 +204,9 @@ public Future addProxySigner( final ArtifactSigner proxySigner, final String consensusPubKey) { return executorService.submit( () -> { - proxySigners.computeIfAbsent(consensusPubKey, k -> new HashSet<>()).add(proxySigner); + proxySigners + .computeIfAbsent(consensusPubKey, k -> ConcurrentHashMap.newKeySet()) + .add(proxySigner); LOG.info( "Loaded new proxy signer {} for consensus public key '{}'", proxySigner.getIdentifier(), @@ -235,7 +247,9 @@ private void loadProxySigners( final MappedResults signersResult = loaderFunction.apply(proxyDir, keystoreParameter.getKeystoresPasswordFile()); final Collection signers = signersResult.getValues(); - proxySigners.computeIfAbsent(consensusPubKey, k -> new HashSet<>()).addAll(signers); + proxySigners + .computeIfAbsent(consensusPubKey, k -> ConcurrentHashMap.newKeySet()) + .addAll(signers); } } } diff --git a/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java b/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java index 93c42bfef..5d168031c 100644 --- a/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java +++ b/slashing-protection/src/main/java/tech/pegasys/web3signer/slashingprotection/PostLoadingValidatorsProcessor.java @@ -12,69 +12,26 @@ */ package tech.pegasys.web3signer.slashingprotection; -import tech.pegasys.web3signer.slashingprotection.dao.ValidatorsDao; - import java.util.List; -import java.util.Optional; import java.util.Set; -import java.util.function.BiConsumer; +import java.util.function.Consumer; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.apache.tuweni.bytes.Bytes; -import org.jdbi.v3.core.Jdbi; - -/** - * Process new validators by registering them with slashing database and disable stale validators. - */ -public class PostLoadingValidatorsProcessor implements BiConsumer, Set> { - private static final Logger LOG = LogManager.getLogger(); - private static final ValidatorsDao VALIDATORS_DAO = new ValidatorsDao(); - - private final SlashingProtectionContext slashingProtectionContext; - - public PostLoadingValidatorsProcessor(final SlashingProtectionContext slashingProtectionContext) { - this.slashingProtectionContext = slashingProtectionContext; - } +/** Process new validators by registering them with slashing database. */ +public record PostLoadingValidatorsProcessor(SlashingProtectionContext slashingProtectionContext) + implements Consumer> { @Override - public void accept(final Set newValidators, final Set staleValidators) { + public void accept(final Set newValidators) { registerNewValidators(newValidators); - disableStaleValidators(staleValidators); } private void registerNewValidators(final Set newValidators) { - if (newValidators.isEmpty()) { + if (newValidators == null || newValidators.isEmpty()) { return; } final List validatorsList = newValidators.stream().map(Bytes::fromHexString).toList(); slashingProtectionContext.getRegisteredValidators().registerValidators(validatorsList); } - - private void disableStaleValidators(final Set staleValidators) { - if (staleValidators.isEmpty()) { - return; - } - - final RegisteredValidators registeredValidators = - slashingProtectionContext.getRegisteredValidators(); - final Jdbi jdbi = slashingProtectionContext.getSlashingProtectionJdbi(); - jdbi.useTransaction( - handle -> { - // disable the validators in the database - staleValidators.forEach( - publicKey -> { - final Optional validatorId = - registeredValidators.getValidatorIdForPublicKey(Bytes.fromHexString(publicKey)); - if (validatorId.isPresent()) { - DbLocker.lockAllForValidator(handle, validatorId.get()); - VALIDATORS_DAO.setEnabled(handle, validatorId.get(), false); - } else { - LOG.trace( - "Validator with public key {} not found in database to disable", publicKey); - } - }); - }); - } } From 17dda5ee707022e5295f3e3f73868644fdc677cc Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Mon, 20 Jan 2025 23:33:34 +1000 Subject: [PATCH 10/11] Remove stale artifact signers from SignerLoader. Update unit tests --- CHANGELOG.md | 8 +- .../pegasys/web3signer/core/Eth1Runner.java | 14 +- .../pegasys/web3signer/core/Eth2Runner.java | 15 +- .../web3signer/signing/BlsArtifactSigner.java | 9 + .../signing/config/ConfigFileContent.java | 39 -- .../signing/config/SignerLoader.java | 340 +++++++++--------- .../signing/config/SignerLoaderTest.java | 57 +-- 7 files changed, 220 insertions(+), 262 deletions(-) delete mode 100644 signing/src/main/java/tech/pegasys/web3signer/signing/config/ConfigFileContent.java diff --git a/CHANGELOG.md b/CHANGELOG.md index a7da46620..1e3e1d490 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,9 @@ ### Breaking Changes - The behavior of reload API endpoint has been modified due to issue [#1018][issue_1018] implemented by PR [#1054][pr_1054]. -The reload API call will remove all in-memory keys before loading them again using configuration files and bulk loading. -This unfortunately results in behaviour change of local configuration files pointing to encrypted keystores. Previously -cached local configuration files were avoided being reloaded during reload API call, now they will be reloaded and -reparsed. To keep the old behavior `--reload-keep-stale-keys=true` option can be used which will not remove stale keys -during reload API call. +The reload API call will remove stale keys therefore they will not be return in public_keys endpoint neither will be +able to perform any signing requests. +- `--Xworker-pool-size` deprecated cli option has been removed. Use `--vertx-worker-pool-size` instead. [issue_1018]: https://github.com/Consensys/web3signer/issues/1018 [pr_1054]: https://github.com/Consensys/web3signer/pull/1054 diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java index 56b0f7d2f..385693f5c 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java @@ -150,14 +150,12 @@ private MappedResults loadSignersFromKeyConfigFiles( awsKmsSignerFactory, true); - return new SignerLoader( - baseConfig.keystoreParallelProcessingEnabled(), baseConfig.reloadKeepStaleKeys()) - .load( - baseConfig.getKeyConfigPath(), - "yaml", - new YamlSignerParser( - List.of(ethSecpArtifactSignerFactory), - YamlMapperFactory.createYamlMapper(baseConfig.getKeyStoreConfigFileMaxSize()))); + return SignerLoader.load( + baseConfig.getKeyConfigPath(), + new YamlSignerParser( + List.of(ethSecpArtifactSignerFactory), + YamlMapperFactory.createYamlMapper(baseConfig.getKeyStoreConfigFileMaxSize())), + baseConfig.keystoreParallelProcessingEnabled()); } } diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java index 7b587fc21..726ffc3f0 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java @@ -202,15 +202,12 @@ private MappedResults loadSignersFromKeyConfigFiles( azureKeyVaultFactory); final MappedResults results = - new SignerLoader( - baseConfig.keystoreParallelProcessingEnabled(), baseConfig.reloadKeepStaleKeys()) - .load( - baseConfig.getKeyConfigPath(), - "yaml", - new YamlSignerParser( - List.of(artifactSignerFactory), - YamlMapperFactory.createYamlMapper( - baseConfig.getKeyStoreConfigFileMaxSize()))); + SignerLoader.load( + baseConfig.getKeyConfigPath(), + new YamlSignerParser( + List.of(artifactSignerFactory), + YamlMapperFactory.createYamlMapper(baseConfig.getKeyStoreConfigFileMaxSize())), + baseConfig.keystoreParallelProcessingEnabled()); registerSignerLoadingHealthCheck(KEYS_CHECK_CONFIG_FILE_LOADING, results); return results; diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/BlsArtifactSigner.java b/signing/src/main/java/tech/pegasys/web3signer/signing/BlsArtifactSigner.java index c7a04b131..594eba2f2 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/BlsArtifactSigner.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/BlsArtifactSigner.java @@ -20,6 +20,7 @@ import java.util.Objects; import java.util.Optional; +import com.google.common.base.MoreObjects; import org.apache.tuweni.bytes.Bytes; public class BlsArtifactSigner implements ArtifactSigner { @@ -77,4 +78,12 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(keyPair); } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("keyPair", keyPair.getPublicKey()) + .add("origin", origin) + .toString(); + } } diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/ConfigFileContent.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/ConfigFileContent.java deleted file mode 100644 index 86e3c6959..000000000 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/ConfigFileContent.java +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Copyright 2023 ConsenSys AG. - * - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on - * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the - * specific language governing permissions and limitations under the License. - */ -package tech.pegasys.web3signer.signing.config; - -import java.nio.file.Path; -import java.util.Collections; -import java.util.Map; - -class ConfigFileContent { - private final Map contentMap; - private final int errorCount; - - public ConfigFileContent(final Map contentMap, final int errorCount) { - this.contentMap = contentMap; - this.errorCount = errorCount; - } - - static ConfigFileContent withSingleErrorCount() { - return new ConfigFileContent(Collections.emptyMap(), 1); - } - - public Map getContentMap() { - return contentMap; - } - - public int getErrorCount() { - return errorCount; - } -} diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java index 08a5da0b4..984b68aa2 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/SignerLoader.java @@ -15,6 +15,7 @@ import tech.pegasys.web3signer.keystorage.common.MappedResults; import tech.pegasys.web3signer.signing.ArtifactSigner; import tech.pegasys.web3signer.signing.config.metadata.SigningMetadata; +import tech.pegasys.web3signer.signing.config.metadata.SigningMetadataException; import tech.pegasys.web3signer.signing.config.metadata.parser.SignerParser; import java.io.IOException; @@ -26,13 +27,14 @@ import java.time.Instant; import java.util.AbstractMap.SimpleEntry; import java.util.Collection; -import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; @@ -45,216 +47,204 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +/** + * The SignerLoaded loads the metadata files and converts them to ArtifactSigners. This class keeps + * track of the metadata files and ArtifactSigners that have been read and only reads them again if + * they have been modified. It also removes the cached ArtifactSigners if the metadata file has been + * removed. + */ public class SignerLoader { private static final Logger LOG = LogManager.getLogger(); private static final long FILES_PROCESSED_TO_REPORT = 10; - // enable or disable parallel streams to convert and load private keys from metadata files - private final boolean useParallelStreams; - // if false, reload metadata files even if they are not modified - private final boolean reloadKeepStaleFiles; - - private static final Map metadataConfigFilesPathCache = new HashMap<>(); - - public SignerLoader(final boolean useParallelStreams, final boolean reloadKeepStaleFiles) { - this.useParallelStreams = useParallelStreams; - this.reloadKeepStaleFiles = reloadKeepStaleFiles; - } - - public MappedResults load( - final Path configsDirectory, final String fileExtension, final SignerParser signerParser) { - final Instant start = Instant.now(); + private static final Set CONFIG_FILE_EXTENSIONS = Set.of("yaml", "yml"); + + record CachedArtifactSigners( + Path metadataFile, FileTime lastModifiedTime, Set artifactSigners) {} + + private static final Map cachedArtifactSigners = + new ConcurrentHashMap<>(); + + /** + * Load ArtifactSigners for new or modified metadata files. Return cached ArtifactSigners if + * metadata files have not been modified. Remove cached ArtifactSigner if metadata file has been + * removed. + * + * @param configsDirectory Location of the metadata files + * @param signerParser An implementation of SignerParser to parse the metadata files + * @return A MappedResults of ArtifactSigners and error count + */ + public static MappedResults load( + final Path configsDirectory, + final SignerParser signerParser, + final boolean useParallelStreams) { LOG.info("Loading signer configuration metadata files from {}", configsDirectory); + final Instant loadStartTime = Instant.now(); + // get all metadata file paths from the config directory. + final Map filteredPaths; + try { + filteredPaths = getMetadataConfigFiles(configsDirectory); + } catch (final IOException e) { + LOG.error("Unable to access the supplied key directory", e); + return MappedResults.errorResult(); + } - final ConfigFileContent configFileContent = - getNewOrModifiedConfigFilesContents(configsDirectory, fileExtension); - - LOG.info( - "Signer configuration metadata files read in memory {} in {}", - configFileContent.getContentMap().size(), - calculateTimeTaken(start).orElse("unknown duration")); - - final Instant conversionStartInstant = Instant.now(); - // Step 1: convert yaml file content to list of SigningMetadata - final MappedResults signingMetadataResults = - convertConfigFileContent(configFileContent.getContentMap(), signerParser); - - LOG.debug( - "Signing configuration metadata files converted to signing metadata {}", - signingMetadataResults.getValues().size()); - - // Step 2: Convert SigningMetadata to ArtifactSigners. This involves connecting to remote - // Hashicorp vault, AWS, Azure or decrypting local keystore files. - final MappedResults artifactSigners = - mapMetadataToArtifactSigner(signingMetadataResults.getValues(), signerParser); - - // merge error counts of config file parsing errors ... - artifactSigners.mergeErrorCount(signingMetadataResults.getErrorCount()); - artifactSigners.mergeErrorCount(configFileContent.getErrorCount()); + // remove cached metadata file mappings that have been deleted or not loaded + var cachedArtifactSignerSize = cachedArtifactSigners.size(); + cachedArtifactSigners.keySet().removeIf(path -> !filteredPaths.containsKey(path)); + if (cachedArtifactSignerSize != cachedArtifactSigners.size()) { + LOG.info( + "Removed {} cached metadata files that has not been loaded.", + cachedArtifactSignerSize - cachedArtifactSigners.size()); + } + // reload the metadata files that are either new or modified LOG.info( - "Total Artifact Signer loaded via configuration files: {}\nError count {}\nTime Taken: {}.", - artifactSigners.getValues().size(), - artifactSigners.getErrorCount(), - calculateTimeTaken(conversionStartInstant).orElse("unknown duration")); - - return artifactSigners; - } + "Loading and converting SigningMetadata to ArtifactSigner using {} streams ...", + useParallelStreams ? "parallel" : "sequential"); + final Stream> pathStream = + useParallelStreams + ? filteredPaths.entrySet().parallelStream() + : filteredPaths.entrySet().stream(); - private MappedResults convertConfigFileContent( - final Map contentMap, final SignerParser signerParser) { + final AtomicLong configFilesHandled = new AtomicLong(0); final AtomicInteger errorCount = new AtomicInteger(0); - final List signingMetadataList = - contentMap.entrySet().parallelStream() + final Map> loadedArtSigners = + pathStream + .filter(SignerLoader::isModifiedOrNew) .flatMap( entry -> { + if (configFilesHandled.incrementAndGet() % FILES_PROCESSED_TO_REPORT == 0) { + LOG.info("{} signing metadata processed", configFilesHandled.get()); + } try { - return signerParser.readSigningMetadata(entry.getValue()).stream(); - } catch (final Exception e) { - renderException(e, entry.getKey().toString()); + return Stream.of( + new SimpleEntry<>( + entry.getKey(), + Files.readString(entry.getKey(), StandardCharsets.UTF_8))); + } catch (IOException e) { + LOG.error("Error reading metadata config file: {}", entry.getKey(), e); errorCount.incrementAndGet(); return Stream.empty(); } }) - .collect(Collectors.toList()); - return MappedResults.newInstance(signingMetadataList, errorCount.get()); - } - - @VisibleForTesting - static Optional calculateTimeTaken(final Instant start) { - final Instant now = Instant.now(); - final long timeTaken = Duration.between(start, now).toMillis(); - if (timeTaken < 0) { - LOG.warn("System Clock returned time in past. Start: {}, Now: {}.", start, now); - return Optional.empty(); - } - return Optional.of(DurationFormatUtils.formatDurationHMS(timeTaken)); - } - - private ConfigFileContent getNewOrModifiedConfigFilesContents( - final Path configsDirectory, final String fileExtension) { - // Step 1, read Paths in config directory without reading the file content since Files.list does - // not use parallel stream - final List filteredPaths; - try (final Stream fileStream = Files.list(configsDirectory)) { - filteredPaths = - fileStream - .filter(path -> matchesFileExtension(fileExtension, path)) - .filter(this::isNewOrModifiedMetadataFile) - .collect(Collectors.toList()); - } catch (final IOException e) { - LOG.error("Unable to access the supplied key directory", e); - return ConfigFileContent.withSingleErrorCount(); - } - - final AtomicInteger errorCount = new AtomicInteger(0); - // step 2, read file contents in parallel stream - final Map configFileMap = - filteredPaths.parallelStream() - .map( - path -> { + .flatMap( + entry -> { + try { + final List signingMetadata = + signerParser.readSigningMetadata(entry.getValue()); + return Stream.of(new SimpleEntry<>(entry.getKey(), signingMetadata)); + } catch (final SigningMetadataException e) { + LOG.error( + "Error parsing metadata file {} to signing metadata: {}", + entry.getKey(), + ExceptionUtils.getRootCauseMessage(e)); + errorCount.incrementAndGet(); + return Stream.empty(); + } + }) + .flatMap( + entry -> { try { - return getMetadataFileContent(path); - } catch (final IOException e) { + final Set artifactSigners = + new HashSet<>(signerParser.parse(entry.getValue())); + return Stream.of(new SimpleEntry<>(entry.getKey(), artifactSigners)); + } catch (final SigningMetadataException e) { + LOG.error( + "Error converting signing metadata to Artifact Signer: {}", + ExceptionUtils.getRootCauseMessage(e)); errorCount.incrementAndGet(); - LOG.error("Error reading config file: {}", path, e); - return null; + return Stream.empty(); } }) - .filter(Objects::nonNull) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - return new ConfigFileContent(configFileMap, errorCount.get()); - } + // merge the new loaded ArtifactSigners with the cached ones + loadedArtSigners.forEach( + (path, artifactSigners) -> { + cachedArtifactSigners.put( + path, + new CachedArtifactSigners( + path, filteredPaths.get(path).lastModifiedTime, artifactSigners)); + }); + + // return all ArtifactSigners from the cache. If same ArtifactSigner is loaded from multiple + // paths, only one will + // be returned. + final Collection allArtifactSigners = + cachedArtifactSigners.values().stream() + .flatMap(cachedArtifactSigners -> cachedArtifactSigners.artifactSigners.stream()) + .collect(Collectors.toSet()); - private boolean isNewOrModifiedMetadataFile(final Path path) { - // only read file if is not previously read or has been since modified - try { - final FileTime lastModifiedTime = Files.getLastModifiedTime(path); - if (metadataConfigFilesPathCache.containsKey(path)) { - if (reloadKeepStaleFiles - && metadataConfigFilesPathCache.get(path).compareTo(lastModifiedTime) == 0) { - return false; - } - } + LOG.info( + "Total Artifact Signers loaded via configuration files: {}\nTotal Paths cached: {}, Error count: {}\nTime Taken: {}.", + allArtifactSigners.size(), + cachedArtifactSigners.size(), + errorCount.get(), + calculateTimeTaken(loadStartTime).orElse("unknown duration")); - // keep the path and last modified time in local cache - metadataConfigFilesPathCache.put(path, lastModifiedTime); - return true; - } catch (final IOException e) { - LOG.error("Error reading config file: {}", path, e); - return false; - } + return MappedResults.newInstance(allArtifactSigners, errorCount.get()); } - private SimpleEntry getMetadataFileContent(final Path path) throws IOException { - return new SimpleEntry<>(path, Files.readString(path, StandardCharsets.UTF_8)); + @VisibleForTesting + static void clearCache() { + cachedArtifactSigners.clear(); } - private MappedResults mapMetadataToArtifactSigner( - final Collection signingMetadataCollection, - final SignerParser signerParser) { - - if (signingMetadataCollection.isEmpty()) { - return MappedResults.newSetInstance(); - } - - LOG.info( - "Converting signing metadata to Artifact Signer using {} streams ...", - useParallelStreams ? "parallel" : "sequential"); - - try { - if (useParallelStreams) { - return mapToArtifactSigner(signingMetadataCollection.parallelStream(), signerParser); - } else { - return mapToArtifactSigner(signingMetadataCollection.stream(), signerParser); - } - } catch (final Exception e) { - LOG.error("Unexpected error in processing configuration files: {}", e.getMessage(), e); - return MappedResults.errorResult(); + /** + * Load Metadata config file paths and their timestamps. + * + * @param configsDirectory Path to the directory containing the metadata files + * @return A map of metadata file paths and their last modified timestamps + * @throws IOException If there is an error reading the config directory. + */ + private static Map getMetadataConfigFiles( + final Path configsDirectory) throws IOException { + try (final Stream fileStream = Files.list(configsDirectory)) { + return fileStream + .filter(SignerLoader::validFileExtension) + .map( + path -> { + try { + return new SimpleEntry<>( + path, + new CachedArtifactSigners(path, Files.getLastModifiedTime(path), Set.of())); + } catch (final IOException e) { + // very unlikely to happen as Files.list is already successful. + LOG.error("Error getting last modified time for config file: {}", path, e); + return null; + } + }) + .filter(Objects::nonNull) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); } } - private MappedResults mapToArtifactSigner( - final Stream signingMetadataStream, final SignerParser signerParser) { - final AtomicLong configFilesHandled = new AtomicLong(); - final AtomicInteger errorCount = new AtomicInteger(0); + private static boolean isModifiedOrNew(final Map.Entry entry) { + var metadataFilePath = entry.getKey(); + var metadataFile = entry.getValue(); + return !cachedArtifactSigners.containsKey(metadataFilePath) + || cachedArtifactSigners + .get(metadataFilePath) + .lastModifiedTime + .compareTo(metadataFile.lastModifiedTime) + != 0; + } - final Set artifactSigners = - signingMetadataStream - .flatMap( - metadataContent -> { - final long filesProcessed = configFilesHandled.incrementAndGet(); - if (filesProcessed % FILES_PROCESSED_TO_REPORT == 0) { - LOG.info("{} signing metadata processed", filesProcessed); - } - try { - return signerParser.parse(List.of(metadataContent)).stream(); - } catch (final Exception e) { - LOG.error( - "Error converting signing metadata to Artifact Signer: {}", e.getMessage()); - LOG.debug(ExceptionUtils.getStackTrace(e)); - errorCount.incrementAndGet(); - return Stream.empty(); - } - }) - .collect(Collectors.toSet()); - LOG.debug("Signing metadata mapped to Artifact Signer: {}", artifactSigners.size()); - return MappedResults.newInstance(artifactSigners, errorCount.get()); + @VisibleForTesting + static Optional calculateTimeTaken(final Instant start) { + final Instant now = Instant.now(); + final long timeTaken = Duration.between(start, now).toMillis(); + if (timeTaken < 0) { + LOG.warn("System Clock returned time in past. Start: {}, Now: {}.", start, now); + return Optional.empty(); + } + return Optional.of(DurationFormatUtils.formatDurationHMS(timeTaken)); } - private boolean matchesFileExtension(final String validFileExtension, final Path filename) { + private static boolean validFileExtension(final Path filename) { final boolean isHidden = filename.toFile().isHidden(); final String extension = FilenameUtils.getExtension(filename.toString()); - return !isHidden - && extension.toLowerCase(Locale.ROOT).endsWith(validFileExtension.toLowerCase(Locale.ROOT)); - } - - private void renderException(final Throwable t, final String filename) { - LOG.error( - "Error parsing signing metadata file {}: {}", - filename, - ExceptionUtils.getRootCauseMessage(t)); - LOG.debug(ExceptionUtils.getStackTrace(t)); + return !isHidden && CONFIG_FILE_EXTENSIONS.contains(extension.toLowerCase(Locale.ROOT)); } } diff --git a/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java b/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java index 57626f33d..1c48cfc09 100644 --- a/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java +++ b/signing/src/test/java/tech/pegasys/web3signer/signing/config/SignerLoaderTest.java @@ -62,7 +62,6 @@ @ExtendWith(MockitoExtension.class) class SignerLoaderTest { private static final YAMLMapper YAML_MAPPER = YamlMapperFactory.createYamlMapper(); - private static final String FILE_EXTENSION = "yaml"; @TempDir Path configsDirectory; @Mock private MetricsSystem metricsSystem; @Mock private HashicorpConnectionFactory hashicorpConnectionFactory; @@ -77,7 +76,6 @@ class SignerLoaderTest { private static final BLSKeyPair blsKeyPair2 = BLSTestUtil.randomKeyPair(2); private static final BLSKeyPair blsKeyPair3 = BLSTestUtil.randomKeyPair(3); - private SignerLoader signerLoader; private SignerParser signerParser; @BeforeEach @@ -108,7 +106,8 @@ public void setup() { signerParser = new YamlSignerParser( List.of(blsArtifactSignerFactory), YamlMapperFactory.createYamlMapper()); - signerLoader = new SignerLoader(true, true); + + SignerLoader.clearCache(); } @ParameterizedTest(name = "{index} - Signer created for file name {0}") @@ -117,8 +116,9 @@ void signerReturnedForValidMetadataFile(final String fileName) throws IOExceptio final String privateKeyHex = blsKeyPair1.getSecretKey().toBytes().toHexString(); createFileInConfigsDirectory(fileName, privateKeyHex); + SignerLoader.load(configsDirectory, signerParser, true); final Collection signerList = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser).getValues(); + SignerLoader.load(configsDirectory, signerParser, true).getValues(); assertThat(signerList.size()).isOne(); assertThat(signerList.stream().findFirst().orElseThrow().getIdentifier()) @@ -130,7 +130,9 @@ static Stream validMetadataFileNameProvider() { configFileName(blsKeyPair1), "prefix_" + configFileName(blsKeyPair1), "test.yaml", - "test.YAML"); + "test.YAML", + "test.yml", + "test.YML"); } @Test @@ -140,7 +142,7 @@ void wrongFileExtensionReturnsEmptySigner() throws IOException { createFileInConfigsDirectory(filename, privateKeyHex); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).isEmpty(); assertThat(result.getErrorCount()).isZero(); @@ -151,7 +153,7 @@ void failedParserReturnsEmptySigner() throws IOException { createFileInConfigsDirectory(configFileName(blsKeyPair1), "NOT_A_VALID_KEY"); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).isEmpty(); assertThat(result.getErrorCount()).isOne(); @@ -161,7 +163,7 @@ void failedParserReturnsEmptySigner() throws IOException { void failedWithDirectoryErrorReturnEmptySigner() throws IOException { final Path missingConfigDir = configsDirectory.resolve("idontexist"); final MappedResults result = - signerLoader.load(missingConfigDir, FILE_EXTENSION, signerParser); + SignerLoader.load(missingConfigDir, signerParser, true); assertThat(result.getValues()).isEmpty(); assertThat(result.getErrorCount()).isOne(); @@ -176,7 +178,7 @@ void multipleMatchesForSameIdentifierReturnsSameSigners() throws IOException { createFileInConfigsDirectory(filename2, privateKeyHex); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).hasSize(1); assertThat(result.getErrorCount()).isZero(); @@ -187,7 +189,7 @@ void signerIdentifiersNotReturnedInvalidMetadataFile() throws IOException { createEmptyFileInConfigsDirectory(configFileName(blsKeyPair1)); createEmptyFileInConfigsDirectory(configFileName(blsKeyPair2)); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).isEmpty(); assertThat(result.getErrorCount()).isEqualTo(2); @@ -203,7 +205,7 @@ void signerIdentifiersNotReturnedForHiddenFiles() throws IOException { createFileInConfigsDirectory(configFileName(blsKeyPair2), privateKeyHex2); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).hasSize(1); assertThat(result.getValues().stream().findFirst().orElseThrow().getIdentifier()) @@ -221,8 +223,7 @@ void signerIdentifiersReturnedForAllValidMetadataFilesInDirectory() throws IOExc createFileInConfigsDirectory(configFileName(blsKeyPair2), privateKeyHex2); createFileInConfigsDirectory(configFileName(blsKeyPair3), privateKeyHex3); - MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + MappedResults result = SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).hasSize(3); assertThat( @@ -236,7 +237,7 @@ void signerIdentifiersReturnedForAllValidMetadataFilesInDirectory() throws IOExc } @Test - void callingLoadTwiceDoesNotReloadUnmodifiedConfigFiles() throws IOException { + void callingLoadTwiceReturnedSameNumberOfArtifacts() throws IOException { final String privateKeyHex1 = blsKeyPair1.getSecretKey().toBytes().toHexString(); final String privateKeyHex2 = blsKeyPair2.getSecretKey().toBytes().toHexString(); final String privateKeyHex3 = blsKeyPair3.getSecretKey().toBytes().toHexString(); @@ -246,7 +247,7 @@ void callingLoadTwiceDoesNotReloadUnmodifiedConfigFiles() throws IOException { createFileInConfigsDirectory(configFileName(blsKeyPair3), privateKeyHex3); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).hasSize(3); assertThat( @@ -259,12 +260,12 @@ void callingLoadTwiceDoesNotReloadUnmodifiedConfigFiles() throws IOException { blsKeyPair3.getPublicKey().toHexString()); final MappedResults reloadedResult = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); - assertThat(reloadedResult.getValues()).isEmpty(); + SignerLoader.load(configsDirectory, signerParser, true); + assertThat(reloadedResult.getValues()).hasSize(3); } @Test - void callingLoadTwiceOnlyLoadSignersFromModifiedConfigFiles() throws IOException { + void callingLoadTwiceWithRemovedFiles() throws IOException { final String privateKeyHex1 = blsKeyPair1.getSecretKey().toBytes().toHexString(); final String privateKeyHex2 = blsKeyPair2.getSecretKey().toBytes().toHexString(); final String privateKeyHex3 = blsKeyPair3.getSecretKey().toBytes().toHexString(); @@ -274,7 +275,7 @@ void callingLoadTwiceOnlyLoadSignersFromModifiedConfigFiles() throws IOException createFileInConfigsDirectory(configFileName(blsKeyPair3), privateKeyHex3); final MappedResults result = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser); + SignerLoader.load(configsDirectory, signerParser, true); assertThat(result.getValues()).hasSize(3); assertThat( @@ -286,14 +287,18 @@ void callingLoadTwiceOnlyLoadSignersFromModifiedConfigFiles() throws IOException blsKeyPair2.getPublicKey().toHexString(), blsKeyPair3.getPublicKey().toHexString()); - // recreate file - which would change the last modified time - createFileInConfigsDirectory(configFileName(blsKeyPair3), privateKeyHex3); + // remove file + Files.delete(configsDirectory.resolve(configFileName(blsKeyPair3))); final Collection reloadedArtifactSigner = - signerLoader.load(configsDirectory, FILE_EXTENSION, signerParser).getValues(); - assertThat(reloadedArtifactSigner).hasSize(1); - assertThat(reloadedArtifactSigner.stream().findFirst().get().getIdentifier()) - .isEqualTo(blsKeyPair3.getPublicKey().toHexString()); + SignerLoader.load(configsDirectory, signerParser, true).getValues(); + assertThat(reloadedArtifactSigner).hasSize(2); + assertThat( + reloadedArtifactSigner.stream() + .map(ArtifactSigner::getIdentifier) + .collect(Collectors.toList())) + .containsOnly( + blsKeyPair1.getPublicKey().toHexString(), blsKeyPair2.getPublicKey().toHexString()); } @Test @@ -325,7 +330,7 @@ private Path createFileInConfigsDirectory(final String fileName, final String pr } private static String configFileName(final BLSKeyPair blsKeyPair) { - return String.format("%s.%s", blsKeyPair.getPublicKey().toHexString(), FILE_EXTENSION); + return blsKeyPair.getPublicKey().toHexString() + ".yaml"; } private void createEmptyFileInConfigsDirectory(final String filename) throws IOException { From 9602ac166fa7c1b170c6f637dffe21d15fffa786 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Tue, 21 Jan 2025 11:32:37 +1000 Subject: [PATCH 11/11] revert keep stale keys logic --- .../dsl/signer/SignerConfiguration.java | 9 +------ .../signer/SignerConfigurationBuilder.java | 9 +------ .../runner/CmdLineParamsConfigFileImpl.java | 8 ------ .../runner/CmdLineParamsDefaultImpl.java | 5 ---- .../KeyIdentifiersAcceptanceTest.java | 27 ------------------- .../KeyIdentifiersAcceptanceTestBase.java | 9 ------- .../commandline/Web3SignerBaseCommand.java | 13 --------- .../commandline/CommandlineParserTest.java | 5 +--- .../jsonrpcproxy/support/TestBaseConfig.java | 5 ---- .../pegasys/web3signer/core/Eth1Runner.java | 1 - .../pegasys/web3signer/core/Eth2Runner.java | 1 - .../web3signer/core/config/BaseConfig.java | 2 -- .../config/DefaultArtifactSignerProvider.java | 16 ++++------- .../DefaultArtifactSignerProviderTest.java | 8 +++--- 14 files changed, 11 insertions(+), 107 deletions(-) diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java index 9b129ee86..dc172c53e 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfiguration.java @@ -83,7 +83,6 @@ public class SignerConfiguration { private final boolean signingExtEnabled; private Optional> commitBoostParameters; - private final Optional reloadKeepStaleKeys; public SignerConfiguration( final String hostname, @@ -132,8 +131,7 @@ public SignerConfiguration( final ChainIdProvider chainIdProvider, final Optional v3KeystoresBulkloadParameters, final boolean signingExtEnabled, - final Optional> commitBoostParameters, - final Optional reloadKeepStaleKeys) { + final Optional> commitBoostParameters) { this.hostname = hostname; this.logLevel = logLevel; this.httpRpcPort = httpRpcPort; @@ -181,7 +179,6 @@ public SignerConfiguration( this.v3KeystoresBulkloadParameters = v3KeystoresBulkloadParameters; this.signingExtEnabled = signingExtEnabled; this.commitBoostParameters = commitBoostParameters; - this.reloadKeepStaleKeys = reloadKeepStaleKeys; } public String hostname() { @@ -379,8 +376,4 @@ public boolean isSigningExtEnabled() { public Optional> getCommitBoostParameters() { return commitBoostParameters; } - - public Optional getReloadKeepStaleKeys() { - return reloadKeepStaleKeys; - } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java index 2eee742a6..c7160ffc9 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/SignerConfigurationBuilder.java @@ -87,7 +87,6 @@ public class SignerConfigurationBuilder { private boolean signingExtEnabled; private Pair commitBoostParameters; - private Optional reloadKeepStaleKeys = Optional.empty(); public SignerConfigurationBuilder withLogLevel(final Level logLevel) { this.logLevel = logLevel; @@ -340,11 +339,6 @@ public SignerConfigurationBuilder withCommitBoostParameters( return this; } - public SignerConfigurationBuilder withReloadKeepStaleKeys(final boolean reloadKeepStaleKeys) { - this.reloadKeepStaleKeys = Optional.of(reloadKeepStaleKeys); - return this; - } - public SignerConfiguration build() { if (mode == null) { throw new IllegalArgumentException("Mode cannot be null"); @@ -396,7 +390,6 @@ public SignerConfiguration build() { chainIdProvider, Optional.ofNullable(v3KeystoresBulkloadParameters), signingExtEnabled, - Optional.ofNullable(commitBoostParameters), - reloadKeepStaleKeys); + Optional.ofNullable(commitBoostParameters)); } } diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java index 66acb01ea..e9fcb2bd3 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsConfigFileImpl.java @@ -123,14 +123,6 @@ public List createCmdLineParams() { yamlConfig.append(createServerTlsArgs()); - signerConfig - .getReloadKeepStaleKeys() - .ifPresent( - reloadKeepStaleKeys -> - yamlConfig.append( - String.format( - YAML_BOOLEAN_FMT, "reload-keep-stale-keys", reloadKeepStaleKeys))); - params.add(signerConfig.getMode()); // sub-command .. it can't go to config file if (signerConfig.getMode().equals("eth2")) { diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java index 713a8502e..4875a2932 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/dsl/signer/runner/CmdLineParamsDefaultImpl.java @@ -107,11 +107,6 @@ public List createCmdLineParams() { params.addAll(createServerTlsArgs()); - signerConfig - .getReloadKeepStaleKeys() - .ifPresent( - reloadKeepStaleKeys -> params.add("--reload-keep-stale-keys=" + reloadKeepStaleKeys)); - params.add(signerConfig.getMode()); if (signerConfig.getMode().equals("eth2")) { diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java index d1073a3a4..1aa01d6d6 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTest.java @@ -172,33 +172,6 @@ public void publicKeysAreRemovedAfterReloadDefault(final KeyType keyType) { }); } - @ParameterizedTest - @EnumSource(value = KeyType.class) - public void publicKeysNotRemovedAfterReloadWithKeepStaleKeysTrue(final KeyType keyType) { - final String[] prvKeys = privateKeys(keyType); - final String[] keys = createKeys(keyType, true, prvKeys); - - initAndStartSignerWithReloadKeepStaleKeys(calculateMode(keyType)); - - validateApiResponse(signer.callApiPublicKeys(keyType), containsInAnyOrder(keys)); - - // remove one of the key config file - assertThat(testDirectory.resolve(keys[1] + ".yaml").toFile().delete()).isTrue(); - - // reload API call - signer.callReload().then().statusCode(200); - - // reload is async ... assert that the keys are not removed - Awaitility.await() - .atMost(5, SECONDS) - .untilAsserted( - () -> { - final List publicKeysList = - signer.callApiPublicKeys(keyType).jsonPath().getList("."); - assertThat(publicKeysList).containsExactlyInAnyOrder(keys); - }); - } - @ParameterizedTest @EnumSource(value = KeyType.class) public void allLoadedKeysAreReturnedInPublicKeyResponse(final KeyType keyType) { diff --git a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java index 081f788ff..0a7732343 100644 --- a/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java +++ b/acceptance-tests/src/test/java/tech/pegasys/web3signer/tests/publickeys/KeyIdentifiersAcceptanceTestBase.java @@ -141,15 +141,6 @@ protected void initAndStartSigner(final String mode) { startSigner(builder.build()); } - protected void initAndStartSignerWithReloadKeepStaleKeys(final String mode) { - startSigner( - new SignerConfigurationBuilder() - .withKeyStoreDirectory(testDirectory) - .withMode(mode) - .withReloadKeepStaleKeys(true) - .build()); - } - protected Response callApiPublicKeysWithoutOpenApiClientSideFilter(final KeyType keyType) { return given().baseUri(signer.getUrl()).accept("").get(Signer.publicKeysPath(keyType)); } diff --git a/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java b/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java index 27702f720..915d5ca32 100644 --- a/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java +++ b/commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java @@ -213,14 +213,6 @@ public class Web3SignerBaseCommand implements BaseConfig, Runnable { paramLabel = INTEGER_FORMAT_HELP) private Integer vertxWorkerPoolSize = null; - @Option( - names = "--reload-keep-stale-keys", - description = - "Set to true to keep stale keys loaded during reload API call. (default: ${DEFAULT-VALUE})", - paramLabel = "", - arity = "1") - private boolean reloadKeepStaleKeys = false; - @CommandLine.Mixin private PicoCliTlsServerOptions picoCliTlsServerOptions; @Override @@ -334,11 +326,6 @@ public int getVertxWorkerPoolSize() { return VERTX_WORKER_POOL_SIZE_DEFAULT; } - @Override - public boolean reloadKeepStaleKeys() { - return reloadKeepStaleKeys; - } - @Override public String toString() { return MoreObjects.toStringHelper(this) diff --git a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java index 1020e7744..f0127abc3 100644 --- a/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java +++ b/commandline/src/test/java/tech/pegasys/web3signer/commandline/CommandlineParserTest.java @@ -662,10 +662,7 @@ protected List createArtifactSignerProvider( final Vertx vertx, final MetricsSystem metricsSystem) { return List.of( new DefaultArtifactSignerProvider( - baseConfig.reloadKeepStaleKeys(), - Collections::emptyList, - Optional.empty(), - Optional.empty())); + Collections::emptyList, Optional.empty(), Optional.empty())); } @Override diff --git a/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java b/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java index 6cd3cd195..a8ef1a848 100644 --- a/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java +++ b/core/src/integrationTest/java/tech/pegasys/web3signer/core/jsonrpcproxy/support/TestBaseConfig.java @@ -141,9 +141,4 @@ public boolean keystoreParallelProcessingEnabled() { public int getVertxWorkerPoolSize() { return 20; } - - @Override - public boolean reloadKeepStaleKeys() { - return false; - } } diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java index 385693f5c..e92111ee3 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java @@ -92,7 +92,6 @@ protected List createArtifactSignerProvider( final ArtifactSignerProvider signerProvider = new DefaultArtifactSignerProvider( - baseConfig.reloadKeepStaleKeys(), () -> { final List signers = new ArrayList<>(); final AzureKeyVaultFactory azureKeyVaultFactory = new AzureKeyVaultFactory(); diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java index 726ffc3f0..210526fd7 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java @@ -157,7 +157,6 @@ protected List createArtifactSignerProvider( final Vertx vertx, final MetricsSystem metricsSystem) { return List.of( new DefaultArtifactSignerProvider( - baseConfig.reloadKeepStaleKeys(), createArtifactSignerSupplier(vertx, metricsSystem), slashingProtectionContext.map(PostLoadingValidatorsProcessor::new), Optional.of(commitBoostApiParameters))); diff --git a/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java b/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java index 04d4de8a3..25384d827 100644 --- a/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java +++ b/core/src/main/java/tech/pegasys/web3signer/core/config/BaseConfig.java @@ -64,6 +64,4 @@ public interface BaseConfig { boolean keystoreParallelProcessingEnabled(); int getVertxWorkerPoolSize(); - - boolean reloadKeepStaleKeys(); } diff --git a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java index d788150c2..beb580fdd 100644 --- a/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java +++ b/signing/src/main/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProvider.java @@ -57,7 +57,6 @@ public class DefaultArtifactSignerProvider implements ArtifactSignerProvider { private static final Logger LOG = LogManager.getLogger(); - private final boolean reloadKeepStaleKeys; private final Supplier> artifactSignerCollectionSupplier; private final Optional>> postLoadingCallback; private final Optional commitBoostKeystoresParameters; @@ -67,11 +66,9 @@ public class DefaultArtifactSignerProvider implements ArtifactSignerProvider { private final ExecutorService executorService = Executors.newSingleThreadExecutor(); public DefaultArtifactSignerProvider( - final boolean reloadKeepStaleKeys, final Supplier> artifactSignerCollectionSupplier, final Optional>> postLoadingCallback, final Optional commitBoostKeystoresParameters) { - this.reloadKeepStaleKeys = reloadKeepStaleKeys; this.artifactSignerCollectionSupplier = artifactSignerCollectionSupplier; this.postLoadingCallback = postLoadingCallback; this.commitBoostKeystoresParameters = commitBoostKeystoresParameters; @@ -102,10 +99,9 @@ public Future load() { // Update the signers map with new signers signers.putAll(newSigners); - // Conditionally remove stale keys from signers map - if (!reloadKeepStaleKeys) { - staleKeys.forEach(signers::remove); - } + + // remove stale keys from signers map + staleKeys.forEach(signers::remove); // Callback to perform further actions specific to eth1/eth2 mode (if any) postLoadingCallback.ifPresent(callback -> callback.accept(signers.keySet())); @@ -133,10 +129,8 @@ public Future load() { BLS.name(), BlsKeystoreBulkLoader::loadKeystoresUsingPasswordFile); }); - // Conditionally remove stale proxy signers - if (!reloadKeepStaleKeys) { - staleKeys.forEach(proxySigners::remove); - } + // Remove stale proxy signers + staleKeys.forEach(proxySigners::remove); }); LOG.info("Total signers (keys) currently loaded in memory: {}", signers.size()); diff --git a/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java b/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java index c0d10bed7..37e593f35 100644 --- a/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java +++ b/signing/src/test/java/tech/pegasys/web3signer/signing/config/DefaultArtifactSignerProviderTest.java @@ -69,7 +69,7 @@ void signerReturnedForMatchingIdentifier() { signerProvider = new DefaultArtifactSignerProvider( - false, () -> List.of(mockSigner), Optional.empty(), Optional.empty()); + () -> List.of(mockSigner), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); final Optional signer = signerProvider.getSigner(PUBLIC_KEY1); @@ -86,7 +86,7 @@ void signerProviderOnlyHasSingleEntryIfPassedInListHasMultipleMatchingSigners() signerProvider = new DefaultArtifactSignerProvider( - false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); + () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); assertThat(signerProvider.availableIdentifiers()).hasSize(1); @@ -102,7 +102,7 @@ void signerProviderCanMapInTwoSigners() { signerProvider = new DefaultArtifactSignerProvider( - false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); + () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.empty()); assertThatCode(() -> signerProvider.load().get()).doesNotThrowAnyException(); assertThat(signerProvider.availableIdentifiers()).hasSize(2); assertThat(signerProvider.availableIdentifiers()).containsOnly(PUBLIC_KEY1, PUBLIC_KEY2); @@ -131,7 +131,6 @@ void proxySignersAreLoadedCorrectly() throws IOException { signerProvider = new DefaultArtifactSignerProvider( - false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.of(commitBoostParameters)); @@ -172,7 +171,6 @@ void emptyProxySignersAreLoadedSuccessfully() { signerProvider = new DefaultArtifactSignerProvider( - false, () -> List.of(mockSigner1, mockSigner2), Optional.empty(), Optional.of(commitBoostParameters));