Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve KeyManager API import operation #968

Merged
merged 10 commits into from
Feb 16, 2024
18 changes: 8 additions & 10 deletions core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java
Original file line number Diff line number Diff line change
Expand Up @@ -199,16 +199,14 @@ private void registerEth2Routes(

router
.route(HttpMethod.POST, KEYSTORES_PATH)
.handler(
new BlockingHandlerDecorator(
new ImportKeystoresHandler(
objectMapper,
baseConfig.getKeyConfigPath(),
slashingProtectionContext.map(
SlashingProtectionContext::getSlashingProtection),
blsSignerProvider,
validatorManager),
false))
.blockingHandler(
new ImportKeystoresHandler(
objectMapper,
baseConfig.getKeyConfigPath(),
slashingProtectionContext.map(SlashingProtectionContext::getSlashingProtection),
blsSignerProvider,
validatorManager),
false)
.failureHandler(errorHandler);

router
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright 2024 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.core.service.http.handlers.keymanager.imports;

import org.apache.tuweni.bytes.Bytes;

/** Keep the index of the keystore import json and password */
public class ImportKeystoreData implements Comparable<ImportKeystoreData> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a record instead of a class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be able to convert this one to record as we need to modify the "ImportKeystoreResult" state to ERROR which is a member of this class.

private final int index;
private final String pubKey;
private final Bytes pubKeyBytes;
private final String keystoreJson;
private final String password;
private ImportKeystoreResult importKeystoreResult;

public ImportKeystoreData(
final int index,
String pubKey,
final String keystoreJson,
final String password,
final ImportKeystoreResult importKeystoreResult) {
this.index = index;
this.pubKey = pubKey;
this.pubKeyBytes = Bytes.fromHexString(pubKey);
this.keystoreJson = keystoreJson;
this.password = password;
this.importKeystoreResult = importKeystoreResult;
}

public int getIndex() {
return index;
}

public String getPubKey() {
return pubKey;
}

public Bytes getPubKeyBytes() {
return pubKeyBytes;
}

public String getKeystoreJson() {
return keystoreJson;
}

public String getPassword() {
return password;
}

public void setImportKeystoreResult(ImportKeystoreResult importKeystoreResult) {
this.importKeystoreResult = importKeystoreResult;
}

public ImportKeystoreResult getImportKeystoreResult() {
return importKeystoreResult;
}

@Override
public int compareTo(final ImportKeystoreData other) {
return Integer.compare(this.index, other.index);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ public ImportKeystoreResult(
this.message = message;
}

public ImportKeystoreResult(final ImportKeystoreStatus status) {
this(status, null);
}

@JsonProperty("status")
public ImportKeystoreStatus getStatus() {
return status;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

import static io.vertx.core.http.HttpHeaders.CONTENT_TYPE;
import static tech.pegasys.web3signer.core.service.http.handlers.ContentTypes.JSON_UTF_8;
import static tech.pegasys.web3signer.core.service.http.handlers.keymanager.imports.ImportKeystoreStatus.DUPLICATE;
import static tech.pegasys.web3signer.core.service.http.handlers.keymanager.imports.ImportKeystoreStatus.IMPORTED;
import static tech.pegasys.web3signer.signing.KeystoreFileManager.KEYSTORE_JSON_EXTENSION;
import static tech.pegasys.web3signer.signing.KeystoreFileManager.KEYSTORE_PASSWORD_EXTENSION;
import static tech.pegasys.web3signer.signing.KeystoreFileManager.METADATA_YAML_EXTENSION;
Expand All @@ -36,6 +38,7 @@
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand All @@ -46,7 +49,6 @@
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.tuweni.bytes.Bytes;

public class ImportKeystoresHandler implements Handler<RoutingContext> {

Expand Down Expand Up @@ -110,12 +112,8 @@ public void handle(final RoutingContext context) {
// extract pubkeys to import first
final List<String> pubkeysToImport;
try {
pubkeysToImport =
parsedBody.getKeystores().stream()
.map(json -> new JsonObject(json).getString("pubkey"))
.map(IdentifierUtils::normaliseIdentifier)
.collect(Collectors.toList());
} catch (Exception e) {
pubkeysToImport = parsedBody.getKeystores().stream().map(this::getNormalizedPubKey).toList();
} catch (final Exception e) {
context.fail(BAD_REQUEST, e);
return;
}
Expand All @@ -128,9 +126,7 @@ public void handle(final RoutingContext context) {

// filter out already loaded keys for slashing data import
final List<String> nonLoadedPubkeys =
pubkeysToImport.stream()
.filter(key -> !existingPubkeys.contains(key))
.collect(Collectors.toList());
pubkeysToImport.stream().filter(key -> !existingPubkeys.contains(key)).toList();

// read slashing protection data if present and import data matching non-loaded keys to import
// only
Expand All @@ -141,35 +137,63 @@ public void handle(final RoutingContext context) {
new ByteArrayInputStream(
parsedBody.getSlashingProtection().getBytes(StandardCharsets.UTF_8));
slashingProtection.get().importDataWithFilter(slashingProtectionData, nonLoadedPubkeys);
} catch (Exception e) {
} catch (final Exception e) {
context.fail(BAD_REQUEST, e);
return;
}
}

final List<ImportKeystoreResult> results = new ArrayList<>();
final List<ImportKeystoreData> duplicateData = new ArrayList<>();
final List<ImportKeystoreData> toBeImportedData = new ArrayList<>();
for (int i = 0; i < parsedBody.getKeystores().size(); i++) {
final String pubkey = pubkeysToImport.get(i);
try {
final String jsonKeystoreData = parsedBody.getKeystores().get(i);
final String password = parsedBody.getPasswords().get(i);

if (existingPubkeys.contains(pubkey)) {
// keystore already loaded
results.add(new ImportKeystoreResult(ImportKeystoreStatus.DUPLICATE, null));
} else {
validatorManager.addValidator(Bytes.fromHexString(pubkey), jsonKeystoreData, password);
results.add(new ImportKeystoreResult(ImportKeystoreStatus.IMPORTED, null));
}
} catch (final Exception e) {
// cleanup the current key being processed and continue
removeSignersAndCleanupImportedKeystoreFiles(List.of(pubkey));
results.add(
new ImportKeystoreResult(
ImportKeystoreStatus.ERROR, "Error importing keystore: " + e.getMessage()));
final String jsonKeystoreData = parsedBody.getKeystores().get(i);
final String password = parsedBody.getPasswords().get(i);
final String pubkey = getNormalizedPubKey(jsonKeystoreData);
if (existingPubkeys.contains(pubkey)) {
// keystore already loaded
duplicateData.add(
new ImportKeystoreData(
i, pubkey, jsonKeystoreData, password, new ImportKeystoreResult(DUPLICATE)));
} else {
toBeImportedData.add(
new ImportKeystoreData(
i, pubkey, jsonKeystoreData, password, new ImportKeystoreResult(IMPORTED)));
}
}

// process TO_BE_IMPORTED in parallel and add to validator manager. Change status to ERROR if
// any error happens.
final List<ImportKeystoreData> importedData =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shame we need to iterate over the list multiple times. You could probably do a zip using mapToObj and an IntStream to just over the stream once.

i.e. something like

IntStream.range(0, parsedBody.getKeystores().size())
            .mapToObj(i -> { create import keystore result; })

toBeImportedData.parallelStream()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will using the parallelStream impact the performance on signing requests?

.peek(
data -> {
try {
validatorManager.addValidator(
data.getPubKeyBytes(), data.getKeystoreJson(), data.getPassword());
} catch (final Exception e) {
data.setImportKeystoreResult(
new ImportKeystoreResult(
ImportKeystoreStatus.ERROR,
"Error importing keystore: " + e.getMessage()));
}
})
.toList();

// clean out any error results
removeSignersAndCleanupImportedKeystoreFiles(
importedData.stream()
.filter(
data -> data.getImportKeystoreResult().getStatus() == ImportKeystoreStatus.ERROR)
.map(ImportKeystoreData::getPubKey)
.toList());

// merge duplicate and imported data and sort
final List<ImportKeystoreResult> results =
Stream.concat(duplicateData.stream(), importedData.stream())
.sorted()
.map(ImportKeystoreData::getImportKeystoreResult)
.toList();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block of code in the ImportKeystoresHandler.handle is getting very long and hard to read can this be broken up?

try {
context
.response()
Expand All @@ -182,6 +206,10 @@ public void handle(final RoutingContext context) {
}
}

private String getNormalizedPubKey(final String json) {
return IdentifierUtils.normaliseIdentifier(new JsonObject(json).getString("pubkey"));
}

private ImportKeystoresRequestBody parseRequestBody(final RequestBody requestBody)
throws JsonProcessingException {
final String body = requestBody.asString();
Expand Down
Loading