Merge branch 'brent/improve-slash-queue' (#2574)

* origin/brent/improve-slash-queue:
  test
  refactor: drop the LazyVec
  changelog: add #2574
  fix testing
  ensure that only 1 slash per block is enqueued

.changelog/unreleased/improvements/2574-imrpove-slash-queue.md

@@ -0,0 +1,2 @@
+- Only process 1 slash per validator per block height.
+  ([\#2574](https://github.com/anoma/namada/pull/2574))

crates/apps/src/lib/node/ledger/shell/finalize_block.rs

@@ -3626,7 +3626,7 @@ mod test_finalize_block {
         let enqueued_slash = enqueued_slashes_handle()
             .at(&processing_epoch)
             .at(&val1.address)
-            .front(&shell.state)
+            .get(&shell.state, &height.0)
             .unwrap()
             .unwrap();
         assert_eq!(enqueued_slash.epoch, misbehavior_epoch);
@@ -3663,7 +3663,7 @@ mod test_finalize_block {
                     address: pkh1,
                     power: Default::default(),
                 },
-                height: height.try_into().unwrap(),
+                height: height.next_height().try_into().unwrap(),
                 time: tendermint::Time::unix_epoch(),
                 total_voting_power: Default::default(),
             },
@@ -3696,8 +3696,13 @@ mod test_finalize_block {
             .at(&processing_epoch.next())
             .at(&val1.address);
 
-        assert_eq!(enqueued_slashes_8.len(&shell.state).unwrap(), 2_u64);
-        assert_eq!(enqueued_slashes_9.len(&shell.state).unwrap(), 1_u64);
+        let num_enqueued_8 =
+            enqueued_slashes_8.iter(&shell.state).unwrap().count();
+        let num_enqueued_9 =
+            enqueued_slashes_9.iter(&shell.state).unwrap().count();
+
+        assert_eq!(num_enqueued_8, 2);
+        assert_eq!(num_enqueued_9, 1);
         let last_slash =
             namada_proof_of_stake::storage::read_validator_last_slash_epoch(
                 &shell.state,

crates/proof_of_stake/src/slashing.rs

@@ -160,12 +160,16 @@ where
         evidence_epoch + params.slash_processing_epoch_offset();
 
     // Add the slash to the list of enqueued slashes to be processed at a later
-    // epoch
-    enqueued_slashes_handle()
+    // epoch. If a slash at the same block height already exists, return early.
+    let enqueued = enqueued_slashes_handle()
         .get_data_handler()
         .at(&processing_epoch)
-        .at(validator)
-        .push(storage, slash)?;
+        .at(validator);
+    if enqueued.contains(storage, &evidence_block_height)? {
+        return Ok(());
+    } else {
+        enqueued.insert(storage, evidence_block_height, slash)?;
+    }
 
     // Update the most recent slash (infraction) epoch for the validator
     let last_slash_epoch = read_validator_last_slash_epoch(storage, validator)?;

crates/proof_of_stake/src/tests/state_machine.rs

@@ -738,6 +738,7 @@ impl StateMachineTest for ConcretePosState {
                     &params,
                     current_epoch,
                     infraction_epoch,
+                    height,
                     slash_type,
                     &address,
                 );
@@ -1356,6 +1357,7 @@ impl ConcretePosState {
         params: &PosParams,
         current_epoch: Epoch,
         infraction_epoch: Epoch,
+        infraction_height: u64,
         slash_type: SlashType,
         validator: &Address,
     ) {
@@ -1392,7 +1394,7 @@ impl ConcretePosState {
         let slash = enqueued_slashes_handle()
             .at(&processing_epoch)
             .at(validator)
-            .back(&self.s)
+            .get(&self.s, &infraction_height)
             .unwrap();
         if let Some(slash) = slash {
             assert_eq!(slash.epoch, infraction_epoch);
@@ -5212,12 +5214,16 @@ fn arb_slash(state: &AbstractPosState) -> impl Strategy<Value = Transition> {
         .checked_sub(state.params.unbonding_len)
         .unwrap_or_default()..=current_epoch)
         .prop_map(Epoch::from);
-    (arb_validator, arb_type, arb_epoch).prop_map(
-        |(validator, slash_type, infraction_epoch)| Transition::Misbehavior {
-            address: validator,
-            slash_type,
-            infraction_epoch,
-            height: 0,
+    let arb_height = 0_u64..10_000_u64;
+
+    (arb_validator, arb_type, arb_epoch, arb_height).prop_map(
+        |(validator, slash_type, infraction_epoch, height)| {
+            Transition::Misbehavior {
+                address: validator,
+                slash_type,
+                infraction_epoch,
+                height,
+            }
         },
     )
 }

crates/proof_of_stake/src/tests/state_machine_v2.rs

@@ -2691,6 +2691,7 @@ impl StateMachineTest for ConcretePosState {
                     &params,
                     current_epoch,
                     infraction_epoch,
+                    height,
                     slash_type,
                     &address,
                 );
@@ -3058,6 +3059,7 @@ impl ConcretePosState {
         params: &PosParams,
         current_epoch: Epoch,
         infraction_epoch: Epoch,
+        infraction_height: u64,
         slash_type: SlashType,
         validator: &Address,
     ) {
@@ -3102,7 +3104,7 @@ impl ConcretePosState {
         let slash = enqueued_slashes_handle()
             .at(&processing_epoch)
             .at(validator)
-            .back(&self.s)
+            .get(&self.s, &infraction_height)
             .unwrap();
         if let Some(slash) = slash {
             assert_eq!(slash.epoch, infraction_epoch);
@@ -4589,12 +4591,16 @@ fn arb_slash(state: &AbstractPosState) -> impl Strategy<Value = Transition> {
         .checked_sub(state.params.unbonding_len)
         .unwrap_or_default()..=current_epoch)
         .prop_map(Epoch::from);
-    (arb_validator, arb_type, arb_epoch).prop_map(
-        |(validator, slash_type, infraction_epoch)| Transition::Misbehavior {
-            address: validator,
-            slash_type,
-            infraction_epoch,
-            height: 0,
+    let arb_height = 0_u64..10_000_u64;
+
+    (arb_validator, arb_type, arb_epoch, arb_height).prop_map(
+        |(validator, slash_type, infraction_epoch, height)| {
+            Transition::Misbehavior {
+                address: validator,
+                slash_type,
+                infraction_epoch,
+                height,
+            }
         },
     )
 }

crates/proof_of_stake/src/tests/test_slash_and_redel.rs

@@ -1,9 +1,15 @@
+use std::collections::BTreeMap;
 use std::ops::Deref;
 use std::str::FromStr;
 
 use assert_matches::assert_matches;
-use namada_core::address;
+use namada_core::address::testing::{
+    established_address_1, established_address_2,
+};
+use namada_core::address::{self, Address};
 use namada_core::dec::Dec;
+use namada_core::key::testing::{keypair_1, keypair_2, keypair_3};
+use namada_core::key::RefTo;
 use namada_core::storage::{BlockHeight, Epoch};
 use namada_core::token::NATIVE_MAX_DECIMAL_PLACES;
 use namada_state::testing::TestState;
@@ -19,9 +25,10 @@ use crate::queries::bonds_and_unbonds;
 use crate::slashing::{process_slashes, slash};
 use crate::storage::{
     bond_handle, delegator_redelegated_bonds_handle,
-    delegator_redelegated_unbonds_handle, read_total_stake,
-    read_validator_stake, total_bonded_handle, total_unbonded_handle,
-    unbond_handle, validator_incoming_redelegations_handle,
+    delegator_redelegated_unbonds_handle, enqueued_slashes_handle,
+    read_total_stake, read_validator_stake, total_bonded_handle,
+    total_unbonded_handle, unbond_handle,
+    validator_incoming_redelegations_handle,
     validator_outgoing_redelegations_handle, validator_slashes_handle,
     validator_total_redelegated_bonded_handle,
     validator_total_redelegated_unbonded_handle,
@@ -32,7 +39,7 @@ use crate::tests::helpers::{
     test_slashes_with_unbonding_params,
 };
 use crate::token::{credit_tokens, read_balance};
-use crate::types::{BondId, GenesisValidator, SlashType};
+use crate::types::{BondId, GenesisValidator, Slash, SlashType};
 use crate::{
     bond_tokens, redelegate_tokens, staking_token_address, token,
     unbond_tokens, withdraw_tokens, OwnedPosParams, RedelegationError,
@@ -1500,3 +1507,144 @@ fn test_slashed_bond_amount_aux(validators: Vec<GenesisValidator>) {
     let diff = val_stake - self_bond_amount - del_bond_amount;
     assert!(diff <= 2.into());
 }
+
+#[test]
+fn test_one_slash_per_block_height() {
+    let mut storage = TestState::default();
+    let params = OwnedPosParams {
+        unbonding_len: 4,
+        validator_stake_threshold: token::Amount::zero(),
+        ..Default::default()
+    };
+
+    let validator1 = established_address_1();
+    let validator2 = established_address_2();
+
+    let gen_validators = [
+        GenesisValidator {
+            address: validator1.clone(),
+            tokens: 100.into(),
+            consensus_key: keypair_1().ref_to(),
+            protocol_key: keypair_3().ref_to(),
+            eth_cold_key: keypair_3().ref_to(),
+            eth_hot_key: keypair_3().ref_to(),
+            commission_rate: Default::default(),
+            max_commission_rate_change: Default::default(),
+            metadata: Default::default(),
+        },
+        GenesisValidator {
+            address: validator2.clone(),
+            tokens: 100.into(),
+            consensus_key: keypair_2().ref_to(),
+            protocol_key: keypair_3().ref_to(),
+            eth_cold_key: keypair_3().ref_to(),
+            eth_hot_key: keypair_3().ref_to(),
+            commission_rate: Default::default(),
+            max_commission_rate_change: Default::default(),
+            metadata: Default::default(),
+        },
+    ];
+
+    // Genesis
+    let current_epoch = storage.in_mem().block.epoch;
+    let params = test_init_genesis(
+        &mut storage,
+        params,
+        gen_validators.clone().into_iter(),
+        current_epoch,
+    )
+    .unwrap();
+    storage.commit_block().unwrap();
+
+    let enqueued_slashes = enqueued_slashes_handle();
+
+    let slash11 = Slash {
+        block_height: 0,
+        epoch: 0.into(),
+        r#type: SlashType::DuplicateVote,
+        rate: Dec::zero(),
+    };
+    let slash12 = Slash {
+        block_height: 0,
+        epoch: 0.into(),
+        r#type: SlashType::LightClientAttack,
+        rate: Dec::zero(),
+    };
+    let slash13 = Slash {
+        block_height: 1,
+        epoch: 0.into(),
+        r#type: SlashType::DuplicateVote,
+        rate: Dec::zero(),
+    };
+    let slash21 = Slash {
+        block_height: 0,
+        epoch: 0.into(),
+        r#type: SlashType::LightClientAttack,
+        rate: Dec::zero(),
+    };
+    let slash22 = Slash {
+        block_height: 0,
+        epoch: 0.into(),
+        r#type: SlashType::DuplicateVote,
+        rate: Dec::zero(),
+    };
+    let slash23 = Slash {
+        block_height: 1,
+        epoch: 0.into(),
+        r#type: SlashType::DuplicateVote,
+        rate: Dec::zero(),
+    };
+
+    let processing_epoch =
+        current_epoch + params.slash_processing_epoch_offset();
+    let enqueue = |stg: &mut TestState, slash: &Slash, validator: &Address| {
+        crate::slashing::slash(
+            stg,
+            &params,
+            current_epoch,
+            slash.epoch,
+            slash.block_height,
+            slash.r#type,
+            validator,
+            current_epoch.next(),
+        )
+        .unwrap();
+    };
+
+    // Enqueue some of the slashes
+    enqueue(&mut storage, &slash11, &validator1);
+    enqueue(&mut storage, &slash21, &validator2);
+    enqueue(&mut storage, &slash13, &validator1);
+    enqueue(&mut storage, &slash23, &validator2);
+
+    // Check
+    let res = enqueued_slashes
+        .get_data_handler()
+        .collect_map(&storage)
+        .unwrap();
+    let exp = BTreeMap::from_iter([(
+        processing_epoch,
+        BTreeMap::from_iter([
+            (
+                validator1.clone(),
+                BTreeMap::from_iter([(0, slash11), (1, slash13)]),
+            ),
+            (
+                validator2.clone(),
+                BTreeMap::from_iter([(0, slash21), (1, slash23)]),
+            ),
+        ]),
+    )]);
+    assert_eq!(res, exp);
+
+    // Enqueue new slashes
+    enqueue(&mut storage, &slash12, &validator1);
+    enqueue(&mut storage, &slash22, &validator2);
+
+    // Check that the slashes are still the same now
+    let res = enqueued_slashes
+        .get_data_handler()
+        .collect_map(&storage)
+        .unwrap();
+    assert_eq!(res, exp);
+}

crates/proof_of_stake/src/types/mod.rs

@@ -137,7 +137,7 @@ pub type ValidatorAddresses = crate::epoched::NestedEpoched<
 
 /// Slashes indexed by validator address and then block height (for easier
 /// retrieval and iteration when processing)
-pub type ValidatorSlashes = NestedMap<Address, Slashes>;
+pub type ValidatorSlashes = NestedMap<Address, LazyMap<u64, Slash>>;
 
 /// Epoched slashes, where the outer epoch key is the epoch in which the slash
 /// is processed