staking: add delay to reenable a validator (#4068)

Sketch solution to #4067, this gives a starting point to provide a
justification why this solve epoch grinding and TCT index overflows.

crates/core/component/stake/src/component/validator_handler/validator_manager.rs

@@ -1,24 +1,27 @@
 use {
-    super::{validator_store::ValidatorPoolTracker, ValidatorDataRead, ValidatorDataWrite},
     crate::{
         component::{
             metrics,
             stake::{ConsensusIndexRead, ConsensusIndexWrite, RateDataWrite},
+            validator_handler::{
+                validator_store::ValidatorPoolTracker, ValidatorDataRead, ValidatorDataWrite,
+            },
             StateReadExt as _, StateWriteExt as _,
         },
         rate::{BaseRateData, RateData},
         state_key,
         validator::{self, BondingState::*, State, State::*, Validator},
         DelegationToken, IdentityKey, Penalty, Uptime,
     },
-    anyhow::Result,
+    anyhow::{ensure, Result},
     async_trait::async_trait,
     cnidarium::StateWrite,
     futures::StreamExt as _,
     penumbra_asset::asset,
     penumbra_num::Amount,
     penumbra_proto::StateWriteProto,
     penumbra_sct::component::clock::{EpochManager, EpochRead},
+    penumbra_sct::component::StateReadExt as _,
     penumbra_shielded_pool::component::AssetRegistry,
     sha2::{Digest as _, Sha256},
     std::collections::BTreeMap,
@@ -123,13 +126,11 @@ pub trait ValidatorManager: StateWrite {
     ) -> Result<()> {
         let validator_state_path = state_key::validators::state::by_id(identity_key);
 
+        let current_height = self.get_block_height().await?;
+
         // Using the start height of the current epoch let us do block based unbonding delays without
         // requiring to bind actions to a specific block height (instead they bind to a whole epoch).
-        let unbonding_start_height = {
-            // We scope it strictly to avoid accidentally using the wrong height.
-            let current_height = self.get_block_height().await?;
-            self.get_epoch_by_height(current_height).await?.start_height
-        };
+        let unbonding_start_height = self.get_epoch_by_height(current_height).await?.start_height;
 
         tracing::debug!("trying to execute a state transition");
 
@@ -154,6 +155,7 @@ pub trait ValidatorManager: StateWrite {
             (Inactive | Jailed | Defined, Disabled) => {
                 // The validator was disabled by its operator.
                 // If necessary, the epoch-handler will deindex this validator after processing it.
+                // We record the height at which the validator was disabled outside of the `match`.
             }
             (Inactive, Active) => {
                 // The delegator has been promoted into the active set, we initialize its uptime tracker,
@@ -177,6 +179,7 @@ pub trait ValidatorManager: StateWrite {
                 // When a validator is honorably discharged from the active set, we begin unbonding
                 // its delegation pool. The epoch-handler will decide whether it wants to keep it in
                 // the consensus set index or not.
+                // In the special case of a validator being disabled, we record the height at which it was disabled.
                 self.set_validator_bonding_state(
                     identity_key,
                     Unbonding {
@@ -284,6 +287,12 @@ pub trait ValidatorManager: StateWrite {
             }
         }
 
+        // Now that we have validated the state transition, we can record the last disabled height.
+        // Doing this here lets us keep the match statement clean and focused on the critical transitions.
+        if new_state == Disabled {
+            self.set_last_disabled_height(identity_key, current_height)
+        }
+
         // At this point, we are guaranteed that this state transition is valid.
         tracing::info!("successful state transition");
         self.put(validator_state_path, new_state);
@@ -489,7 +498,10 @@ pub trait ValidatorManager: StateWrite {
         Ok(())
     }
 
-    /// Update a validator definition
+    /// Create a new validator definition or update an existing one.
+    /// # Errors
+    /// This method errors if the validator state is not found in the state,
+    /// or if the validator definition has been disabled too recently.
     #[tracing::instrument(skip(self, validator), fields(id = ?validator.identity_key))]
     async fn update_validator_definition(&mut self, validator: Validator) -> Result<()> {
         tracing::debug!(definition = ?validator, "updating validator definition");
@@ -507,6 +519,27 @@ pub trait ValidatorManager: StateWrite {
                 self.set_validator_state(id, Disabled).await?;
             }
             (Disabled, true) => {
+                let last_disabled_height = self.get_last_disabled_height(id).await;
+                if let Some(last_disabled) = last_disabled_height {
+                    let current_height = self.get_block_height().await?;
+                    let epoch_duration = self.get_sct_params().await?.epoch_duration;
+
+                    // The actual delay is not too load-bearing, what we want here is to make sure that
+                    // there is a buffer between the last disabled height and the current height.
+                    // See #4067 for details about epoch-grinding.
+                    let allowed_enabled_height = last_disabled.saturating_add(epoch_duration);
+                    let wait_duration = current_height.saturating_sub(allowed_enabled_height);
+                    ensure!(
+                        current_height >= allowed_enabled_height,
+                        "validator has been disabled too recently (last_disabled={}, current_height={}, epoch_duration={}), wait {} blocks",
+                        last_disabled,
+                        current_height,
+                        epoch_duration,
+                        wait_duration
+                    );
+                } else {
+                    tracing::warn!(validator_identity = %id, "validator has no recorded last_disabled_height but is disabled");
+                }
                 // The operator has re-enabled their validator, if it has enough stake it will become
                 // inactive, otherwise it will become defined.
                 let min_validator_stake = self.get_stake_params().await?.min_validator_stake;

crates/core/component/stake/src/component/validator_handler/validator_store.rs

@@ -101,6 +101,17 @@ pub trait ValidatorDataRead: StateRead {
         self.get(&state_key::validators::power::by_id(validator))
     }
 
+    /// Returns the block height at which the validator was last disabled.
+    /// If the validator was never disabled, returns `None`.
+    async fn get_last_disabled_height(&self, identity_key: &IdentityKey) -> Option<u64> {
+        self.nonverifiable_get_raw(
+            state_key::validators::last_disabled::by_id(identity_key).as_bytes(),
+        )
+        .await
+        .expect("no deserialization error expected")
+        .map(|bytes| u64::from_be_bytes(bytes.try_into().expect("we only write 8 bytes")))
+    }
+
     async fn get_validator_definition(
         &self,
         identity_key: &IdentityKey,
@@ -289,6 +300,19 @@ pub(crate) trait ValidatorDataWrite: StateWrite {
         let path = state_key::validators::rate::previous_by_id(identity_key);
         self.put(path, rate_data)
     }
+
+    #[instrument(skip(self))]
+    /// Set the block height at which the validator was last disabled.
+    /// This is useful to make sure that the validator is not re-enabled too soon.
+    /// See #4067 for details about epoch-grinding.
+    fn set_last_disabled_height(&mut self, identity_key: &IdentityKey, height: u64) {
+        self.nonverifiable_put_raw(
+            state_key::validators::last_disabled::by_id(identity_key)
+                .as_bytes()
+                .to_vec(),
+            height.to_be_bytes().to_vec(),
+        );
+    }
 }
 
 impl<T: StateWrite + ?Sized> ValidatorDataWrite for T {}

crates/core/component/stake/src/state_key.rs

@@ -84,6 +84,12 @@ pub mod validators {
         }
     }
 
+    pub mod last_disabled {
+        pub fn by_id(id: &crate::IdentityKey) -> String {
+            format!("staking/validators/data/last_disabled/{id}")
+        }
+    }
+
     /// Tracks the funding rewards of the previously active validator set
     /// in object storage. Consumed by the funding component.
     pub mod rewards {