Improve KeyManager API import operation #968

usmansaleem · 2024-02-06T07:48:01Z

PR Description

Improve KeyManager API import operation. Process new validator in parallel instead of serial stream

Fixed Issue(s)

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

Testing

I thought about testing these changes in a realistic/non-local environment.

-- process new validator in parallel instead of serial stream

...ech/pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoreData.java

jframe · 2024-02-07T04:16:45Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

      }
    }

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


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

jframe · 2024-02-07T04:17:32Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

+      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 =
+        toBeImportedData.parallelStream()
+            .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();
+


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

jframe · 2024-02-07T04:18:36Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

+    // process TO_BE_IMPORTED in parallel and add to validator manager. Change status to ERROR if
+    // any error happens.
+    final List<ImportKeystoreData> importedData =
+        toBeImportedData.parallelStream()


Will using the parallelStream impact the performance on signing requests?

jframe · 2024-02-07T04:22:16Z

...ech/pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoreData.java

+import org.apache.tuweni.bytes.Bytes;
+
+/** Keep the index of the keystore import json and password */
+public class ImportKeystoreData implements Comparable<ImportKeystoreData> {


Could this be a record instead of a class?

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.

siladu · 2024-02-07T07:21:43Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### Bugs fixed
 - Ensure that Web3Signer stops the http server when a sigterm is received
+- Improve Key Manager API import operation to use parallel processing instead of serial processing.


think this is more of an improvement than a bug but you might disagree :)

@usmansaleem

will move it to Improvements section

jframe

LGTM

siladu

Just a few minor comments, nothing blocking

siladu · 2024-02-15T05:19:45Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

-            .sorted()
-            .map(ImportKeystoreData::getImportKeystoreResult)
-            .toList();
+    // step 4: add validators to be imported in parallel stream


aren't we just importing here rather than "adding to be imported"?
Don't think we should mention the parallel stream implementation detail

siladu · 2024-02-15T05:21:41Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

      context.fail(SERVER_ERROR, e);
    }
  }

-  private String getNormalizedPubKey(final String json) {
+  /**
+   * Import validators in parallel stream


Do we need javadoc for a private method? If parallel stream is important here maybe rename the method?

you are right, good observation, lets me rename the method.

siladu · 2024-02-15T05:42:33Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

+        getKeystoreDataToProcess(parsedBody, activePubKeys);
+
+    // Step 3: import slashing protection data for all to-be-IMPORTED keys
+    final List<String> importedPubKeys = getToBeImportedPubKeys(importKeystoreDataList);


s/importedPubKeys/pubKeysToBeImported ?

siladu · 2024-02-15T05:47:10Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### Bugs fixed
 - Ensure that Web3Signer stops the http server when a sigterm is received
+- Improve Key Manager API import operation to use parallel processing instead of serial processing.


@usmansaleem

siladu · 2024-02-15T05:50:11Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

-    // load existing keys
-    final Set<String> existingPubkeys =
+    // load "active" keys
+    final Set<String> activePubKeys =


Not clear to me what "active" means here and why that's significant for this code. Why is this better than "existingPubKeys"?

The "active keys" term is used in the specs i.e https://github.com/ethereum/keymanager-APIs/tree/master/flows#import. For us, "active" means keys which are already loaded into memory before this API is called.

I've renamed it back to prior one.

CHANGELOG.md

siladu · 2024-02-16T02:39:40Z

...pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoresHandler.java

@@ -103,26 +103,26 @@ public void handle(final RoutingContext context) {
      return;
    }

-    // load "active" keys
-    final Set<String> activePubKeys =
+    // "active" keys which are already loaded by Web3Signer before this import call.


Co-authored-by: Simon Dudley <simon.dudley@consensys.net>

usmansaleem added 2 commits February 6, 2024 17:45

Improve KeyManager API import operation

72fbd05

-- process new validator in parallel instead of serial stream

Merge remote-tracking branch 'upstream/master' into import_api_fix

2c12847

github-advanced-security bot found potential problems Feb 6, 2024

View reviewed changes

...ech/pegasys/web3signer/core/service/http/handlers/keymanager/imports/ImportKeystoreData.java Fixed Show fixed Hide fixed

usmansaleem added 3 commits February 6, 2024 17:54

changelog

4132aa9

Add equals and hashbode for ImportKeystoreData

8c13c56

Merge remote-tracking branch 'upstream/master' into import_api_fix

a5d2e2f

jframe reviewed Feb 7, 2024

View reviewed changes

siladu reviewed Feb 7, 2024

View reviewed changes

usmansaleem added 3 commits February 15, 2024 09:52

refactoring the import logic

299f9bd

refactoring to record

ac66178

Merge remote-tracking branch 'upstream/master' into import_api_fix

76a7520

jframe approved these changes Feb 15, 2024

View reviewed changes

siladu approved these changes Feb 15, 2024

View reviewed changes

review suggestions

0cc227f

usmansaleem requested a review from siladu February 15, 2024 10:55

siladu approved these changes Feb 16, 2024

View reviewed changes

Update CHANGELOG.md

f269599

Co-authored-by: Simon Dudley <simon.dudley@consensys.net>

usmansaleem enabled auto-merge (squash) February 16, 2024 04:15

usmansaleem merged commit 371a5dd into Consensys:master Feb 16, 2024
7 checks passed

usmansaleem deleted the import_api_fix branch March 18, 2024 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve KeyManager API import operation #968

Improve KeyManager API import operation #968

usmansaleem commented Feb 6, 2024 •

edited

Loading

jframe Feb 7, 2024

jframe Feb 7, 2024

jframe Feb 7, 2024

jframe Feb 7, 2024

usmansaleem Feb 14, 2024

siladu Feb 7, 2024

siladu Feb 15, 2024

usmansaleem Feb 15, 2024

jframe left a comment

siladu left a comment

siladu Feb 15, 2024

siladu Feb 15, 2024

usmansaleem Feb 15, 2024

siladu Feb 15, 2024

siladu Feb 15, 2024

siladu Feb 15, 2024

usmansaleem Feb 15, 2024

usmansaleem Feb 15, 2024

siladu Feb 16, 2024

Improve KeyManager API import operation #968

Improve KeyManager API import operation #968

Conversation

usmansaleem commented Feb 6, 2024 • edited Loading

PR Description

Fixed Issue(s)

Documentation

Changelog

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jframe left a comment

Choose a reason for hiding this comment

siladu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usmansaleem commented Feb 6, 2024 •

edited

Loading