More tests, fix web3signer VC API client

common/eth2/src/lighthouse_vc/http_client.rs

@@ -437,7 +437,7 @@ impl ValidatorClientHttpClient {
     pub async fn post_lighthouse_validators_web3signer(
         &self,
         request: &[Web3SignerValidatorRequest],
-    ) -> Result<GenericResponse<ValidatorData>, Error> {
+    ) -> Result<(), Error> {
         let mut path = self.server.full.clone();
 
         path.path_segments_mut()

validator_client/src/http_api/keystores.rs

@@ -27,24 +27,24 @@ pub fn list<T: SlotClock + 'static, E: EthSpec>(
     let keystores = initialized_validators
         .validator_definitions()
         .iter()
-        .filter(|def| def.enabled && def.signing_definition.is_local_keystore())
+        .filter(|def| def.enabled)
         .map(|def| {
             let validating_pubkey = def.voting_public_key.compress();
 
-            let derivation_path = initialized_validators
+            let (derivation_path, readonly) = initialized_validators
                 .signing_method(&validating_pubkey)
-                .and_then(|signing_method| match *signing_method {
+                .map_or((None, None), |signing_method| match *signing_method {
                     SigningMethod::LocalKeystore {
                         ref voting_keystore,
                         ..
-                    } => voting_keystore.path(),
-                    SigningMethod::Web3Signer { .. } => None,
+                    } => (voting_keystore.path(), None),
+                    SigningMethod::Web3Signer { .. } => (None, Some(true)),
                 });
 
             SingleKeystoreResponse {
                 validating_pubkey,
                 derivation_path,
-                readonly: None,
+                readonly,
             }
         })
         .collect::<Vec<_>>();
@@ -59,6 +59,15 @@ pub fn import<T: SlotClock + 'static, E: EthSpec>(
     runtime: Weak<Runtime>,
     log: Logger,
 ) -> Result<ImportKeystoresResponse, Rejection> {
+    // Check request validity. This is the only cases in which we should return a 4xx code.
+    if request.keystores.len() != request.passwords.len() {
+        return Err(custom_bad_request(format!(
+            "mismatched numbers of keystores ({}) and passwords ({})",
+            request.keystores.len(),
+            request.passwords.len(),
+        )));
+    }
+
     info!(
         log,
         "Importing keystores via standard HTTP API";
@@ -98,7 +107,6 @@ pub fn import<T: SlotClock + 'static, E: EthSpec>(
     // Import each keystore. Some keystores may fail to be imported, so we record a status for each.
     let mut statuses = Vec::with_capacity(request.keystores.len());
 
-    // FIXME(sproul): check and test different length keystores vs passwords
     for (keystore, password) in request
         .keystores
         .into_iter()
@@ -157,6 +165,15 @@ fn import_single_keystore<T: SlotClock + 'static, E: EthSpec>(
         return Ok(ImportKeystoreStatus::Duplicate);
     }
 
+    // Check that the password is correct.
+    // In future we should re-structure to avoid the double decryption here. It's not as simple
+    // as removing this check because `add_validator_keystore` will break if provided with an
+    // invalid validator definition (`update_validators` will get stuck trying to decrypt with the
+    // wrong password indefinitely).
+    keystore
+        .decrypt_keypair(password.as_ref())
+        .map_err(|e| format!("incorrect password: {:?}", e))?;
+
     let validator_dir = ValidatorDirBuilder::new(validator_dir_path)
         .voting_keystore(keystore, password.as_ref())
         .store_withdrawal_keystore(false)

validator_client/src/http_api/tests.rs

@@ -460,7 +460,7 @@ impl ApiTester {
         self.client
             .post_lighthouse_validators_web3signer(&request)
             .await
-            .unwrap_err();
+            .unwrap();
 
         assert_eq!(self.vals_total(), initial_vals + s.count);
         if s.enabled {

validator_client/src/http_api/tests/keystores.rs

@@ -1,6 +1,9 @@
 use super::*;
 use account_utils::random_password_string;
-use eth2::lighthouse_vc::{http_client::ValidatorClientHttpClient as HttpClient, std_types::*};
+use eth2::lighthouse_vc::{
+    http_client::ValidatorClientHttpClient as HttpClient, std_types::*,
+    types::Web3SignerValidatorRequest,
+};
 use eth2_keystore::Keystore;
 use itertools::Itertools;
 use rand::{rngs::SmallRng, Rng, SeedableRng};
@@ -15,6 +18,27 @@ fn new_keystore(password: ZeroizeString) -> Keystore {
         .unwrap()
 }
 
+fn web3_signer_url() -> String {
+    "http://localhost:1/this-url-hopefully-doesnt-exist".into()
+}
+
+fn new_web3signer_validator() -> (Keypair, Web3SignerValidatorRequest) {
+    let keypair = Keypair::random();
+    let pk = keypair.pk.clone();
+    (
+        keypair,
+        Web3SignerValidatorRequest {
+            enable: true,
+            description: "".into(),
+            graffiti: None,
+            voting_public_key: pk,
+            url: web3_signer_url(),
+            root_certificate_path: None,
+            request_timeout_ms: None,
+        },
+    )
+}
+
 fn run_test<F, V>(f: F)
 where
     F: FnOnce(ApiTester) -> V,
@@ -62,13 +86,13 @@ fn check_get_response<'a>(
     }
 }
 
-fn check_import_response<'a>(
+fn check_import_response(
     response: &ImportKeystoresResponse,
     expected_statuses: impl IntoIterator<Item = ImportKeystoreStatus>,
 ) {
     for (status, expected_status) in response.data.iter().zip_eq(expected_statuses) {
         assert_eq!(
-            status.status, expected_status,
+            expected_status, status.status,
             "message: {:?}",
             status.message
         );
@@ -216,13 +240,159 @@ fn import_some_duplicate_keystores() {
     })
 }
 
-// FIXME(sproul): finish these test stubs
-// FIXME(sproul): add test involving web3signer validators
 #[test]
-fn import_keystores_wrong_password_all() {}
+fn import_wrong_number_of_passwords() {
+    run_test(|tester| async move {
+        let password = random_password_string();
+        let keystores = (0..3)
+            .map(|_| new_keystore(password.clone()))
+            .collect::<Vec<_>>();
+
+        let err = tester
+            .client
+            .post_keystores(&ImportKeystoresRequest {
+                keystores: keystores.clone(),
+                passwords: vec![password.clone()],
+                slashing_protection: None,
+            })
+            .await
+            .unwrap_err();
+        assert_eq!(err.status().unwrap(), 400);
+    })
+}
 
 #[test]
-fn import_keystores_wrong_password_some() {}
+fn get_web3_signer_keystores() {
+    run_test(|tester| async move {
+        let num_local = 3;
+        let num_remote = 2;
+
+        // Add some local validators.
+        let password = random_password_string();
+        let keystores = (0..num_local)
+            .map(|_| new_keystore(password.clone()))
+            .collect::<Vec<_>>();
+
+        let import_res = tester
+            .client
+            .post_keystores(&ImportKeystoresRequest {
+                keystores: keystores.clone(),
+                passwords: vec![password.clone(); keystores.len()],
+                slashing_protection: None,
+            })
+            .await
+            .unwrap();
+
+        // All keystores should be imported.
+        check_import_response(&import_res, all_imported(&keystores));
+
+        // Add some web3signer validators.
+        let remote_vals = (0..num_remote)
+            .map(|_| new_web3signer_validator().1)
+            .collect::<Vec<_>>();
+
+        tester
+            .client
+            .post_lighthouse_validators_web3signer(&remote_vals)
+            .await
+            .unwrap();
+
+        // Check that both local and remote validators are returned.
+        let get_res = tester.client.get_keystores().await.unwrap();
+
+        let expected_responses = keystores
+            .iter()
+            .map(|local_keystore| SingleKeystoreResponse {
+                validating_pubkey: keystore_pubkey(local_keystore),
+                derivation_path: local_keystore.path(),
+                readonly: None,
+            })
+            .chain(remote_vals.iter().map(|remote_val| SingleKeystoreResponse {
+                validating_pubkey: remote_val.voting_public_key.compress(),
+                derivation_path: None,
+                readonly: Some(true),
+            }))
+            .collect::<Vec<_>>();
+
+        for response in expected_responses {
+            assert!(get_res.data.contains(&response), "{:?}", response);
+        }
+    })
+}
+
+#[test]
+fn import_keystores_wrong_password() {
+    run_test(|tester| async move {
+        let num_keystores = 4;
+        let (keystores, correct_passwords): (Vec<_>, Vec<_>) = (0..num_keystores)
+            .map(|_| {
+                let password = random_password_string();
+                (new_keystore(password.clone()), password)
+            })
+            .unzip();
+
+        // First import with some incorrect passwords.
+        let incorrect_passwords = (0..num_keystores)
+            .map(|i| {
+                if i % 2 == 0 {
+                    random_password_string()
+                } else {
+                    correct_passwords[i].clone()
+                }
+            })
+            .collect::<Vec<_>>();
+
+        let import_res = tester
+            .client
+            .post_keystores(&ImportKeystoresRequest {
+                keystores: keystores.clone(),
+                passwords: incorrect_passwords.clone(),
+                slashing_protection: None,
+            })
+            .await
+            .unwrap();
+
+        let expected_statuses = (0..num_keystores).map(|i| {
+            if i % 2 == 0 {
+                ImportKeystoreStatus::Error
+            } else {
+                ImportKeystoreStatus::Imported
+            }
+        });
+        check_import_response(&import_res, expected_statuses);
+
+        // Import again with the correct passwords and check that the statuses are as expected.
+        let correct_import_req = ImportKeystoresRequest {
+            keystores: keystores.clone(),
+            passwords: correct_passwords.clone(),
+            slashing_protection: None,
+        };
+        let import_res = tester
+            .client
+            .post_keystores(&correct_import_req)
+            .await
+            .unwrap();
+        let expected_statuses = (0..num_keystores).map(|i| {
+            if i % 2 == 0 {
+                ImportKeystoreStatus::Imported
+            } else {
+                ImportKeystoreStatus::Duplicate
+            }
+        });
+        check_import_response(&import_res, expected_statuses);
+
+        // Import one final time, at which point all keys should be duplicates.
+        let import_res = tester
+            .client
+            .post_keystores(&correct_import_req)
+            .await
+            .unwrap();
+        check_import_response(
+            &import_res,
+            (0..num_keystores).map(|_| ImportKeystoreStatus::Duplicate),
+        );
+    });
+}
 
 #[test]
 fn import_keystores_full_slashing_protection() {}
@@ -237,10 +407,7 @@ fn delete_some_keystores() {}
 fn delete_all_keystores() {}
 
 #[test]
-fn delete_same_keystores_twice() {}
-
-#[test]
-fn delete_some_keystores_twice() {
+fn delete_keystores_twice() {
     run_test(|tester| async move {
         let password = random_password_string();
         let keystores = (0..2)
@@ -286,9 +453,6 @@ fn delete_nonexistent_keystores() {
     })
 }
 
-#[test]
-fn delete_some_nonexistent_keystores() {}
-
 fn make_attestation(source_epoch: u64, target_epoch: u64) -> Attestation<E> {
     Attestation {
         aggregation_bits: BitList::with_capacity(
@@ -360,19 +524,10 @@ fn delete_concurrent_with_signing() {
             let handle = runtime.spawn(async move {
                 for j in 0..num_attestations {
                     let mut att = make_attestation(j, j + 1);
-                    for (validator_id, public_key) in thread_pubkeys.iter().enumerate() {
-                        let res = validator_store
+                    for (_validator_id, public_key) in thread_pubkeys.iter().enumerate() {
+                        let _ = validator_store
                             .sign_attestation(*public_key, 0, &mut att, Epoch::new(j + 1))
                             .await;
-
-                        println!(
-                            "attestation {}->{} for validator t{}/v{}: {:?}",
-                            j,
-                            j + 1,
-                            thread_index,
-                            validator_id,
-                            res
-                        );
                     }
                 }
             });