feat: Remove stale validator keys during reload (Consensys#1054)

* feat: Remove stale validator keys during reload
* Fix and add AT
* Update changelog
* 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
* Remove stale artifact signers from SignerLoader.

CHANGELOG.md

@@ -1,5 +1,20 @@
 # Changelog
 
+## Next Release
+
+### 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 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
+
+### Features Added
+- Remove stale keys during reload API call. [#1018][issue_1018] [#1054][pr_1054]
+
+---
 ## 24.12.0
 
 ### Breaking Changes

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 publicKeysAreRemovedAfterReloadDefault(final KeyType keyType) {
     final String[] prvKeys = privateKeys(keyType);
     final String[] keys = createKeys(keyType, true, prvKeys);
 
@@ -161,7 +161,15 @@ public void alreadyLoadedPublicKeysAreNotRemovedAfterReload(final KeyType keyTyp
     // reload API call
     signer.callReload().then().statusCode(200);
 
-    validateApiResponse(signer.callApiPublicKeys(keyType), containsInAnyOrder(keys));
+    // reload is async ... assert that the key is removed
+    Awaitility.await()
+        .atMost(5, SECONDS)
+        .untilAsserted(
+            () -> {
+              final List<String> publicKeysList =
+                  signer.callApiPublicKeys(keyType).jsonPath().getList(".");
+              assertThat(publicKeysList).containsOnly(keys[0]);
+            });
   }
 
   @ParameterizedTest

commandline/src/main/java/tech/pegasys/web3signer/commandline/Web3SignerBaseCommand.java

@@ -213,10 +213,6 @@ 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;
-
   @CommandLine.Mixin private PicoCliTlsServerOptions picoCliTlsServerOptions;
 
   @Override
@@ -323,19 +319,10 @@ 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;
   }
 
@@ -386,12 +373,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 {

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();
@@ -688,7 +660,9 @@ public void run() {}
     @Override
     protected List<ArtifactSignerProvider> 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

core/src/main/java/tech/pegasys/web3signer/core/Eth1Runner.java

@@ -116,6 +116,7 @@ protected List<ArtifactSignerProvider> createArtifactSignerProvider(
                       .getValues());
               return signers;
             },
+            Optional.empty(),
             Optional.empty());
 
     // uses eth1 address as identifier
@@ -148,13 +149,12 @@ private MappedResults<ArtifactSigner> loadSignersFromKeyConfigFiles(
               awsKmsSignerFactory,
               true);
 
-      return new SignerLoader(baseConfig.keystoreParallelProcessingEnabled())
-          .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());
     }
   }
 

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<ArtifactSignerProvider> createArtifactSignerProvider(
       final Vertx vertx, final MetricsSystem metricsSystem) {
     return List.of(
         new DefaultArtifactSignerProvider(
-            () -> {
-              try (final AzureKeyVaultFactory azureKeyVaultFactory = new AzureKeyVaultFactory()) {
-                final List<ArtifactSigner> 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<Bytes> 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<Collection<ArtifactSigner>> createArtifactSignerSupplier(
+      final Vertx vertx, final MetricsSystem metricsSystem) {
+    return () -> {
+      try (final AzureKeyVaultFactory azureKeyVaultFactory = new AzureKeyVaultFactory()) {
+        final List<ArtifactSigner> 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<ArtifactSigner> loadSignersFromKeyConfigFiles(
@@ -204,14 +201,12 @@ private MappedResults<ArtifactSigner> loadSignersFromKeyConfigFiles(
               azureKeyVaultFactory);
 
       final MappedResults<ArtifactSigner> results =
-          new SignerLoader(baseConfig.keystoreParallelProcessingEnabled())
-              .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;
@@ -302,9 +297,7 @@ private void registerSignerLoadingHealthCheck(
   @Override
   public void run() {
     super.run();
-    if (pruningEnabled && slashingProtectionContext.isPresent()) {
-      scheduleAndExecuteInitialDbPruning();
-    }
+    scheduleAndExecuteInitialDbPruning();
     slashingProtectionContext.ifPresent(this::scheduleDbHealthCheck);
   }
 
@@ -326,6 +319,10 @@ private void scheduleDbHealthCheck(final SlashingProtectionContext protectionCon
   }
 
   private void scheduleAndExecuteInitialDbPruning() {
+    if (!pruningEnabled || slashingProtectionContext.isEmpty()) {
+      return;
+    }
+
     final DbPrunerRunner dbPrunerRunner =
         new DbPrunerRunner(
             slashingProtectionParameters,

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<RoutingContext> {
-  List<ArtifactSignerProvider> orderedArtifactSignerProviders;
+  private final List<ArtifactSignerProvider> orderedArtifactSignerProviders;
 
-  public ReloadHandler(List<ArtifactSignerProvider> orderedArtifactSignerProviders) {
+  public ReloadHandler(final List<ArtifactSignerProvider> 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();
   }
 }

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();
+  }
 }