Strict slashing protection by default (#1750)

## Proposed Changes

Replace `--strict-slashing-protection` by `--init-slashing-protection` and remove mentions of `--auto-register`

scripts/local_testnet/validator_client.sh

@@ -15,4 +15,4 @@ exec lighthouse \
 	--datadir $VALIDATORS_DIR \
 	--secrets-dir $SECRETS_DIR \
 	--testnet-dir $TESTNET_DIR \
-	--auto-register
+	--init-slashing-protection

testing/node_test_rig/src/lib.rs

@@ -106,6 +106,14 @@ pub fn testing_client_config() -> ClientConfig {
     client_config
 }
 
+pub fn testing_validator_config() -> ValidatorConfig {
+    ValidatorConfig {
+        init_slashing_protection: true,
+        disable_auto_discover: false,
+        ..ValidatorConfig::default()
+    }
+}
+
 /// Contains the directories for a `LocalValidatorClient`.
 ///
 /// This struct is separate to `LocalValidatorClient` to allow for pre-computation of validator

testing/simulator/src/eth1_sim.rs

@@ -4,8 +4,8 @@ use eth1::http::Eth1NetworkId;
 use eth1_test_rig::GanacheEth1Instance;
 use futures::prelude::*;
 use node_test_rig::{
-    environment::EnvironmentBuilder, testing_client_config, ClientGenesis, ValidatorConfig,
+    environment::EnvironmentBuilder, testing_client_config, testing_validator_config,
-    ValidatorFiles,
+    ClientGenesis, ValidatorFiles,
 };
 use rayon::prelude::*;
 use std::net::{IpAddr, Ipv4Addr};
@@ -128,14 +128,7 @@ pub fn run_eth1_sim(matches: &ArgMatches) -> Result<(), String> {
          */
         for (i, files) in validator_files.into_iter().enumerate() {
             network
-                .add_validator_client(
+                .add_validator_client(testing_validator_config(), i, files)
-                    ValidatorConfig {
-                        disable_auto_discover: false,
-                        ..ValidatorConfig::default()
-                    },
-                    i,
-                    files,
-                )
                 .await?;
         }
 

testing/simulator/src/no_eth1_sim.rs

@@ -2,8 +2,8 @@ use crate::{checks, LocalNetwork};
 use clap::ArgMatches;
 use futures::prelude::*;
 use node_test_rig::{
-    environment::EnvironmentBuilder, testing_client_config, ClientGenesis, ValidatorConfig,
+    environment::EnvironmentBuilder, testing_client_config, testing_validator_config,
-    ValidatorFiles,
+    ClientGenesis, ValidatorFiles,
 };
 use rayon::prelude::*;
 use std::net::{IpAddr, Ipv4Addr};
@@ -99,14 +99,7 @@ pub fn run_no_eth1_sim(matches: &ArgMatches) -> Result<(), String> {
         let add_validators_fut = async {
             for (i, files) in validator_files.into_iter().enumerate() {
                 network
-                    .add_validator_client(
+                    .add_validator_client(testing_validator_config(), i, files)
-                        ValidatorConfig {
-                            disable_auto_discover: false,
-                            ..ValidatorConfig::default()
-                        },
-                        i,
-                        files,
-                    )
                     .await?;
             }
 

validator_client/slashing_protection/src/slashing_database.rs

@@ -173,6 +173,17 @@ impl SlashingDatabase {
         Ok(())
     }
 
+    /// Check that all of the given validators are registered.
+    pub fn check_validator_registrations<'a>(
+        &self,
+        mut public_keys: impl Iterator<Item = &'a PublicKey>,
+    ) -> Result<(), NotSafe> {
+        let mut conn = self.conn_pool.get()?;
+        let txn = conn.transaction()?;
+        public_keys
+            .try_for_each(|public_key| self.get_validator_id_in_txn(&txn, public_key).map(|_| ()))
+    }
+
     /// Get the database-internal ID for a validator.
     ///
     /// This is NOT the same as a validator index, and depends on the ordering that validators

validator_client/src/cli.rs

@@ -52,14 +52,6 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .conflicts_with("datadir")
                 .requires("validators-dir"),
         )
-        .arg(Arg::with_name("auto-register").long("auto-register").help(
-            "If present, the validator client will register any new signing keys with \
-                       the slashing protection database so that they may be used. WARNING: \
-                       enabling the same signing key on multiple validator clients WILL lead to \
-                       that validator getting slashed. Only use this flag the first time you run \
-                       the validator client, or if you're certain there are no other \
-                       nodes using the same key. Automatically enabled unless `--strict` is specified",
-        ))
         .arg(
             Arg::with_name("delete-lockfiles")
             .long("delete-lockfiles")
@@ -73,13 +65,14 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
             )
         )
         .arg(
-            Arg::with_name("strict-slashing-protection")
+            Arg::with_name("init-slashing-protection")
-            .long("strict-slashing-protection")
+                .long("init-slashing-protection")
                 .help(
-                "If present, do not create a new slashing database. This is to ensure that users \
+                    "If present, do not require the slashing protection database to exist before \
-                do not accidentally get slashed in case their slashing protection db ends up in the \
+                     running. You SHOULD NOT use this flag unless you're certain that a new \
-                wrong directory during directory restructure and vc creates a new empty db and \
+                     slashing protection database is required. Usually, your database \
-                re-registers all validators."
+                     will have been initialized when you imported your validator keys. If you \
+                     misplace your database and then run with this flag you risk being slashed."
                 )
         )
         .arg(

validator_client/src/config.rs

@@ -32,8 +32,8 @@ pub struct Config {
     pub delete_lockfiles: bool,
     /// If true, don't scan the validators dir for new keystores.
     pub disable_auto_discover: bool,
-    /// If true, don't re-register existing validators in definitions.yml for slashing protection.
+    /// If true, re-register existing validators in definitions.yml for slashing protection.
-    pub strict_slashing_protection: bool,
+    pub init_slashing_protection: bool,
     /// Graffiti to be inserted everytime we create a block.
     pub graffiti: Option<Graffiti>,
     /// Configuration for the HTTP REST API.
@@ -58,7 +58,7 @@ impl Default for Config {
             allow_unsynced_beacon_node: false,
             delete_lockfiles: false,
             disable_auto_discover: false,
-            strict_slashing_protection: false,
+            init_slashing_protection: false,
             graffiti: None,
             http_api: <_>::default(),
         }
@@ -122,7 +122,7 @@ impl Config {
         config.allow_unsynced_beacon_node = cli_args.is_present("allow-unsynced");
         config.delete_lockfiles = cli_args.is_present("delete-lockfiles");
         config.disable_auto_discover = cli_args.is_present("disable-auto-discover");
-        config.strict_slashing_protection = cli_args.is_present("strict-slashing-protection");
+        config.init_slashing_protection = cli_args.is_present("init-slashing-protection");
 
         if let Some(input_graffiti) = cli_args.value_of("graffiti") {
             let graffiti_bytes = input_graffiti.as_bytes();

validator_client/src/http_api/tests.rs

@@ -17,6 +17,7 @@ use eth2::{
 };
 use eth2_keystore::KeystoreBuilder;
 use parking_lot::RwLock;
+use slashing_protection::{SlashingDatabase, SLASHING_PROTECTION_FILENAME};
 use slot_clock::TestingSlotClock;
 use std::marker::PhantomData;
 use std::net::Ipv4Addr;
@@ -65,15 +66,17 @@ impl ApiTester {
             .build()
             .unwrap();
 
+        let slashing_db_path = config.validator_dir.join(SLASHING_PROTECTION_FILENAME);
+        let slashing_protection = SlashingDatabase::open_or_create(&slashing_db_path).unwrap();
+
         let validator_store: ValidatorStore<TestingSlotClock, E> = ValidatorStore::new(
             initialized_validators,
-            &config,
+            slashing_protection,
             Hash256::repeat_byte(42),
             E::default_spec(),
             fork_service.clone(),
             log.clone(),
-        )
+        );
-        .unwrap();
 
         let initialized_validators = validator_store.initialized_validators();
 

validator_client/src/lib.rs

@@ -28,6 +28,7 @@ use futures::channel::mpsc;
 use http_api::ApiSecret;
 use initialized_validators::InitializedValidators;
 use notifier::spawn_notifier;
+use slashing_protection::{SlashingDatabase, SLASHING_PROTECTION_FILENAME};
 use slog::{error, info, Logger};
 use slot_clock::SlotClock;
 use slot_clock::SystemTimeSlotClock;
@@ -106,6 +107,44 @@ impl<T: EthSpec> ProductionValidatorClient<T> {
         .await
         .map_err(|e| format!("Unable to initialize validators: {:?}", e))?;
 
+        // Initialize slashing protection.
+        let slashing_db_path = config.validator_dir.join(SLASHING_PROTECTION_FILENAME);
+        let slashing_protection = if config.init_slashing_protection {
+            SlashingDatabase::open_or_create(&slashing_db_path).map_err(|e| {
+                format!(
+                    "Failed to open or create slashing protection database: {:?}",
+                    e
+                )
+            })
+        } else {
+            SlashingDatabase::open(&slashing_db_path).map_err(|e| {
+                format!(
+                    "Failed to open slashing protection database: {:?}.\n\
+                     Ensure that `slashing_protection.sqlite` is in {:?} folder",
+                    e, config.validator_dir
+                )
+            })
+        }?;
+
+        // Check validator registration with slashing protection, or auto-register all validators.
+        if config.init_slashing_protection {
+            slashing_protection
+                .register_validators(validators.iter_voting_pubkeys())
+                .map_err(|e| format!("Error while registering slashing protection: {:?}", e))?;
+        } else {
+            slashing_protection
+                .check_validator_registrations(validators.iter_voting_pubkeys())
+                .map_err(|e| {
+                    format!(
+                        "One or more validators not found in slashing protection database.\n\
+                         Ensure you haven't misplaced your slashing protection database, or \
+                         carefully consider running with --init-slashing-protection (see --help). \
+                         Error: {:?}",
+                        e
+                    )
+                })?;
+        }
+
         info!(
             log,
             "Initialized validators";
@@ -157,12 +196,12 @@ impl<T: EthSpec> ProductionValidatorClient<T> {
 
         let validator_store: ValidatorStore<SystemTimeSlotClock, T> = ValidatorStore::new(
             validators,
-            &config,
+            slashing_protection,
             genesis_validators_root,
             context.eth2_config.spec.clone(),
             fork_service.clone(),
             log.clone(),
-        )?;
+        );
 
         info!(
             log,
@@ -170,8 +209,6 @@ impl<T: EthSpec> ProductionValidatorClient<T> {
             "voting_validators" => validator_store.num_voting_validators()
         );
 
-        validator_store.register_all_validators_for_slashing_protection()?;
-
         let duties_service = DutiesServiceBuilder::new()
             .slot_clock(slot_clock.clone())
             .validator_store(validator_store.clone())

validator_client/src/validator_store.rs

@@ -1,9 +1,7 @@
-use crate::{
+use crate::{fork_service::ForkService, initialized_validators::InitializedValidators};
-    config::Config, fork_service::ForkService, initialized_validators::InitializedValidators,
-};
 use account_utils::{validator_definitions::ValidatorDefinition, ZeroizeString};
 use parking_lot::RwLock;
-use slashing_protection::{NotSafe, Safe, SlashingDatabase, SLASHING_PROTECTION_FILENAME};
+use slashing_protection::{NotSafe, Safe, SlashingDatabase};
 use slog::{crit, error, warn, Logger};
 use slot_clock::SlotClock;
 use std::marker::PhantomData;
@@ -56,32 +54,13 @@ pub struct ValidatorStore<T, E: EthSpec> {
 impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
     pub fn new(
         validators: InitializedValidators,
-        config: &Config,
+        slashing_protection: SlashingDatabase,
         genesis_validators_root: Hash256,
         spec: ChainSpec,
         fork_service: ForkService<T>,
         log: Logger,
-    ) -> Result<Self, String> {
+    ) -> Self {
-        let slashing_db_path = config.validator_dir.join(SLASHING_PROTECTION_FILENAME);
+        Self {
-        let slashing_protection = if config.strict_slashing_protection {
-            // Don't create a new slashing database if `strict_slashing_protection` is turned on.
-            SlashingDatabase::open(&slashing_db_path).map_err(|e| {
-                format!(
-                    "Failed to open slashing protection database: {:?}.
-                    Ensure that `slashing_protection.sqlite` is in {:?} folder",
-                    e, config.validator_dir
-                )
-            })?
-        } else {
-            SlashingDatabase::open_or_create(&slashing_db_path).map_err(|e| {
-                format!(
-                    "Failed to open or create slashing protection database: {:?}",
-                    e
-                )
-            })?
-        };
-
-        Ok(Self {
             validators: Arc::new(RwLock::new(validators)),
             slashing_protection,
             genesis_validators_root,
@@ -90,7 +69,7 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
             temp_dir: None,
             fork_service,
             _phantom: PhantomData,
-        })
+        }
     }
 
     pub fn initialized_validators(&self) -> Arc<RwLock<InitializedValidators>> {
@@ -130,16 +109,6 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
         Ok(validator_def)
     }
 
-    /// Register all known validators with the slashing protection database.
-    ///
-    /// Registration is required to protect against a lost or missing slashing database,
-    /// such as when relocating validator keys to a new machine.
-    pub fn register_all_validators_for_slashing_protection(&self) -> Result<(), String> {
-        self.slashing_protection
-            .register_validators(self.validators.read().iter_voting_pubkeys())
-            .map_err(|e| format!("Error while registering validators: {:?}", e))
-    }
-
     pub fn voting_pubkeys(&self) -> Vec<PublicKey> {
         self.validators
             .read()
@@ -235,7 +204,7 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
                 warn!(
                     self.log,
                     "Not signing block for unregistered validator";
-                    "msg" => "Carefully consider running with --auto-register (see --help)",
+                    "msg" => "Carefully consider running with --init-slashing-protection (see --help)",
                     "public_key" => format!("{:?}", pk)
                 );
                 None
@@ -314,7 +283,7 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore<T, E> {
                 warn!(
                     self.log,
                     "Not signing attestation for unregistered validator";
-                    "msg" => "Carefully consider running with --auto-register (see --help)",
+                    "msg" => "Carefully consider running with --init-slashing-protection (see --help)",
                     "public_key" => format!("{:?}", pk)
                 );
                 None