diff --git a/.changelog/unreleased/improvements/2574-imrpove-slash-queue.md b/.changelog/unreleased/improvements/2574-imrpove-slash-queue.md new file mode 100644 index 0000000000..ac35c5d5a9 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs b/crates/apps/src/lib/node/ledger/shell/finalize_block.rs index 5df448c0fa..21574dc971 100644 --- a/crates/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/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, diff --git a/crates/proof_of_stake/src/slashing.rs b/crates/proof_of_stake/src/slashing.rs index fe5fd5f018..388bf3cd7f 100644 --- a/crates/proof_of_stake/src/slashing.rs +++ b/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)?; diff --git a/crates/proof_of_stake/src/tests/state_machine.rs b/crates/proof_of_stake/src/tests/state_machine.rs index aef82ded92..16172f8458 100644 --- a/crates/proof_of_stake/src/tests/state_machine.rs +++ b/crates/proof_of_stake/src/tests/state_machine.rs @@ -738,6 +738,7 @@ impl StateMachineTest for ConcretePosState { ¶ms, 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 { .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, + } }, ) } diff --git a/crates/proof_of_stake/src/tests/state_machine_v2.rs b/crates/proof_of_stake/src/tests/state_machine_v2.rs index f7253586d2..5c89bc6d98 100644 --- a/crates/proof_of_stake/src/tests/state_machine_v2.rs +++ b/crates/proof_of_stake/src/tests/state_machine_v2.rs @@ -2691,6 +2691,7 @@ impl StateMachineTest for ConcretePosState { ¶ms, 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 { .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, + } }, ) } diff --git a/crates/proof_of_stake/src/tests/test_slash_and_redel.rs b/crates/proof_of_stake/src/tests/test_slash_and_redel.rs index 34a9c574d8..9df89eeef4 100644 --- a/crates/proof_of_stake/src/tests/test_slash_and_redel.rs +++ b/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) { 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, + ¶ms, + 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); +} diff --git a/crates/proof_of_stake/src/types/mod.rs b/crates/proof_of_stake/src/types/mod.rs index 900be57030..8c6259e414 100644 --- a/crates/proof_of_stake/src/types/mod.rs +++ b/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; +pub type ValidatorSlashes = NestedMap>; /// Epoched slashes, where the outer epoch key is the epoch in which the slash /// is processed