From 73a45ccc08741f825f374185f742ff7059b919d3 Mon Sep 17 00:00:00 2001 From: brentstone Date: Mon, 13 Nov 2023 23:35:20 -0800 Subject: [PATCH 1/9] rename to `compute_and_store_total_consensus_stake` --- apps/src/lib/node/ledger/shell/finalize_block.rs | 2 +- apps/src/lib/node/ledger/shell/init_chain.rs | 2 +- ethereum_bridge/src/test_utils.rs | 6 +++--- proof_of_stake/src/lib.rs | 10 +++++----- proof_of_stake/src/tests.rs | 11 ++++++----- 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 35c23b4ebe..c4929de855 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -105,7 +105,7 @@ where current_epoch, current_epoch + pos_params.pipeline_len, )?; - namada_proof_of_stake::store_total_consensus_stake( + namada_proof_of_stake::compute_and_store_total_consensus_stake( &mut self.wl_storage, current_epoch, )?; diff --git a/apps/src/lib/node/ledger/shell/init_chain.rs b/apps/src/lib/node/ledger/shell/init_chain.rs index e14b74da2d..c875fd0c1f 100644 --- a/apps/src/lib/node/ledger/shell/init_chain.rs +++ b/apps/src/lib/node/ledger/shell/init_chain.rs @@ -157,7 +157,7 @@ where self.apply_genesis_txs_transfer(&genesis); self.apply_genesis_txs_bonds(&genesis); - pos::namada_proof_of_stake::store_total_consensus_stake( + pos::namada_proof_of_stake::compute_and_store_total_consensus_stake( &mut self.wl_storage, Default::default(), ) diff --git a/ethereum_bridge/src/test_utils.rs b/ethereum_bridge/src/test_utils.rs index b2e0bd613b..3552b27bf9 100644 --- a/ethereum_bridge/src/test_utils.rs +++ b/ethereum_bridge/src/test_utils.rs @@ -19,8 +19,8 @@ use namada_proof_of_stake::parameters::OwnedPosParams; use namada_proof_of_stake::pos_queries::PosQueries; use namada_proof_of_stake::types::GenesisValidator; use namada_proof_of_stake::{ - become_validator, bond_tokens, staking_token_address, - store_total_consensus_stake, BecomeValidator, + become_validator, bond_tokens, compute_and_store_total_consensus_stake, + staking_token_address, BecomeValidator, }; use crate::parameters::{ @@ -293,7 +293,7 @@ pub fn append_validators_to_storage( all_keys.insert(validator, keys); } - store_total_consensus_stake( + compute_and_store_total_consensus_stake( wl_storage, current_epoch + params.pipeline_len, ) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index d6d06e78ce..c19139d9aa 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -327,7 +327,7 @@ where current_epoch, epoch, )?; - store_total_consensus_stake(storage, epoch)?; + compute_and_store_total_consensus_stake(storage, epoch)?; } Ok(()) } @@ -1482,8 +1482,8 @@ where }) } -/// Store total consensus stake -pub fn store_total_consensus_stake( +/// Compute and then store the total consensus stake +pub fn compute_and_store_total_consensus_stake( storage: &mut S, epoch: Epoch, ) -> storage_api::Result<()> @@ -1492,7 +1492,7 @@ where { let total = compute_total_consensus_stake(storage, epoch)?; tracing::debug!( - "Computed total consensus stake for epoch {}: {}", + "Total consensus stake for epoch {}: {}", epoch, total.to_string_native() ); @@ -5670,7 +5670,7 @@ pub mod test_utils { )?; } // Store the total consensus validator stake to storage - store_total_consensus_stake(storage, current_epoch)?; + compute_and_store_total_consensus_stake(storage, current_epoch)?; // Copy validator sets and positions copy_genesis_validator_sets(storage, params, current_epoch)?; diff --git a/proof_of_stake/src/tests.rs b/proof_of_stake/src/tests.rs index 302ec31a00..aee5ace020 100644 --- a/proof_of_stake/src/tests.rs +++ b/proof_of_stake/src/tests.rs @@ -53,7 +53,8 @@ use crate::{ apply_list_slashes, become_validator, below_capacity_validator_set_handle, bond_handle, bond_tokens, bonds_and_unbonds, compute_amount_after_slashing_unbond, - compute_amount_after_slashing_withdraw, compute_bond_at_epoch, + compute_amount_after_slashing_withdraw, + compute_and_store_total_consensus_stake, compute_bond_at_epoch, compute_modified_redelegation, compute_new_redelegated_unbonds, compute_slash_bond_at_epoch, compute_slashable_amount, consensus_validator_set_handle, copy_validator_sets_and_positions, @@ -66,9 +67,9 @@ use crate::{ read_consensus_validator_set_addresses_with_stake, read_total_stake, read_validator_deltas_value, read_validator_stake, slash, slash_redelegation, slash_validator, slash_validator_redelegation, - staking_token_address, store_total_consensus_stake, total_bonded_handle, - total_deltas_handle, total_unbonded_handle, unbond_handle, unbond_tokens, - unjail_validator, update_validator_deltas, update_validator_set, + staking_token_address, total_bonded_handle, total_deltas_handle, + total_unbonded_handle, unbond_handle, unbond_tokens, unjail_validator, + update_validator_deltas, update_validator_set, validator_consensus_key_handle, validator_incoming_redelegations_handle, validator_outgoing_redelegations_handle, validator_set_positions_handle, validator_set_update_tendermint, validator_slashes_handle, @@ -2195,7 +2196,7 @@ fn get_tendermint_set_updates( fn advance_epoch(s: &mut TestWlStorage, params: &PosParams) -> Epoch { s.storage.block.epoch = s.storage.block.epoch.next(); let current_epoch = s.storage.block.epoch; - store_total_consensus_stake(s, current_epoch).unwrap(); + compute_and_store_total_consensus_stake(s, current_epoch).unwrap(); copy_validator_sets_and_positions( s, params, From 8103010b009426ea71743464ffad7a3edce4e56c Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 09:51:10 -0800 Subject: [PATCH 2/9] panic on slashing error dingus --- apps/src/lib/node/ledger/shell/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index 4b22542ced..8d63086be0 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -803,6 +803,7 @@ where current_epoch, err ); + panic!("Error while processing slashes"); } } From 3a8ddd0b5290d2013deb0fc9dea8d60563177e73 Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 09:51:55 -0800 Subject: [PATCH 3/9] fix FutureEpoch offsets of some data --- proof_of_stake/src/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proof_of_stake/src/types.rs b/proof_of_stake/src/types.rs index d72b599cdc..b948f5d8fd 100644 --- a/proof_of_stake/src/types.rs +++ b/proof_of_stake/src/types.rs @@ -103,14 +103,14 @@ pub type TotalConsensusStakes = crate::epoched::Epoched< /// Epoched validator's deltas. pub type ValidatorDeltas = crate::epoched::EpochedDelta< token::Change, - crate::epoched::OffsetUnbondingLen, + crate::epoched::OffsetPipelineLen, crate::epoched::OffsetMaxProposalPeriodOrSlashProcessingLenPlus, >; /// Epoched total deltas. pub type TotalDeltas = crate::epoched::EpochedDelta< token::Change, - crate::epoched::OffsetUnbondingLen, + crate::epoched::OffsetPipelineLen, crate::epoched::OffsetMaxProposalPeriodOrSlashProcessingLenPlus, >; @@ -146,7 +146,7 @@ pub type ValidatorSlashes = NestedMap; /// slashes earlier than `cubic_window_width` epochs behind the current pub type EpochedSlashes = crate::epoched::NestedEpoched< ValidatorSlashes, - crate::epoched::OffsetUnbondingLen, + crate::epoched::OffsetPipelineLen, crate::epoched::OffsetSlashProcessingLenPlus, >; From e0278060f4b917e5131a1ccc7d1892ac4746559a Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 09:52:55 -0800 Subject: [PATCH 4/9] remove unused queries fn --- proof_of_stake/src/pos_queries.rs | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/proof_of_stake/src/pos_queries.rs b/proof_of_stake/src/pos_queries.rs index b694897aa8..c0d0fbaf28 100644 --- a/proof_of_stake/src/pos_queries.rs +++ b/proof_of_stake/src/pos_queries.rs @@ -2,12 +2,9 @@ //! data. This includes validator and epoch related data. use namada_core::ledger::parameters::storage::get_max_proposal_bytes_key; -use namada_core::ledger::parameters::EpochDuration; use namada_core::ledger::storage::WlStorage; use namada_core::ledger::storage_api::collections::lazy_map::NestedSubKey; use namada_core::ledger::{storage, storage_api}; -use namada_core::tendermint_proto::google::protobuf; -use namada_core::tendermint_proto::types::EvidenceParams; use namada_core::types::address::Address; use namada_core::types::chain::ProposalBytes; use namada_core::types::storage::{BlockHeight, Epoch}; @@ -140,31 +137,6 @@ where .unwrap_or_default() } - /// Return evidence parameters. - // TODO: impove this docstring - pub fn get_evidence_params( - self, - epoch_duration: &EpochDuration, - pos_params: &PosParams, - ) -> EvidenceParams { - // Minimum number of epochs before tokens are unbonded and can be - // withdrawn - let len_before_unbonded = - std::cmp::max(pos_params.unbonding_len as i64 - 1, 0); - let max_age_num_blocks: i64 = - epoch_duration.min_num_of_blocks as i64 * len_before_unbonded; - let min_duration_secs = epoch_duration.min_duration.0 as i64; - let max_age_duration = Some(protobuf::Duration { - seconds: min_duration_secs * len_before_unbonded, - nanos: 0, - }); - EvidenceParams { - max_age_num_blocks, - max_age_duration, - ..EvidenceParams::default() - } - } - /// Lookup data about a validator from their protocol signing key. pub fn get_validator_from_protocol_pk( self, From b810e95491a117deb5036c2325add7b586f6c18c Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 09:53:47 -0800 Subject: [PATCH 5/9] clean up TODOs, docstrings, logging --- proof_of_stake/src/epoched.rs | 2 -- proof_of_stake/src/lib.rs | 60 +++++++++++++---------------------- proof_of_stake/src/rewards.rs | 1 - proof_of_stake/src/tests.rs | 1 - proof_of_stake/src/types.rs | 9 ++---- 5 files changed, 25 insertions(+), 48 deletions(-) diff --git a/proof_of_stake/src/epoched.rs b/proof_of_stake/src/epoched.rs index 48174a5746..06fdf148dc 100644 --- a/proof_of_stake/src/epoched.rs +++ b/proof_of_stake/src/epoched.rs @@ -402,8 +402,6 @@ where } /// Update data by removing old epochs - // TODO: should we consider more complex handling of empty epochs in the - // data below? pub fn update_data( &self, storage: &mut S, diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index c19139d9aa..8ed8bbbfa1 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -1,10 +1,4 @@ //! Proof of Stake system. -//! -//! TODO: We might need to storage both active and total validator set voting -//! power. For consensus, we only consider active validator set voting power, -//! but for other activities in which inactive validators can participate (e.g. -//! voting on a protocol parameter changes, upgrades, default VP changes) we -//! should use the total validator set voting power. #![doc(html_favicon_url = "https://dev.namada.net/master/favicon.png")] #![doc(html_logo_url = "https://dev.namada.net/master/rustdoc-logo.png")] @@ -308,7 +302,8 @@ where Ok(()) } -/// new init genesis +/// Copies the validator sets into all epochs up through the pipeline epoch at +/// genesis. Also computes the total pub fn copy_genesis_validator_sets( storage: &mut S, params: &OwnedPosParams, @@ -673,8 +668,6 @@ where } /// Read all validator addresses. -/// TODO: expand this to include the jailed validators as well, as it currently -/// only does consensus and bc pub fn read_all_validator_addresses( storage: &S, epoch: namada_core::types::storage::Epoch, @@ -716,6 +709,8 @@ pub fn is_validator( where S: StorageRead, { + // TODO: should this check be made different? I suppose it does work but + // feels weird... let rate = read_validator_max_commission_rate_change(storage, address)?; Ok(rate.is_some()) } @@ -1343,7 +1338,9 @@ where Ok(()) } -/// Validator sets and positions copying into a future epoch +/// Copy the consensus and below-capacity validator sets and positions into a +/// future epoch. Also copies the epoched set of all known validators in the +/// network. pub fn copy_validator_sets_and_positions( storage: &mut S, params: &PosParams, @@ -2361,7 +2358,6 @@ where /// - redelegation end epoch where redeleg stops contributing to src validator /// - src validator address /// - src bond start epoch where it started contributing to src validator -// TODO: refactor out type EagerRedelegatedUnbonds = BTreeMap; /// Computes a map of redelegated unbonds from a set of redelegated bonds. @@ -2378,7 +2374,6 @@ type EagerRedelegatedUnbonds = BTreeMap; /// 1. `modified.epoch` is not in the `epochs_to_remove` set. /// 2. `modified.validator_to_modify` is in `modified.vals_to_remove`. /// 3. `modified.epoch_to_modify` is in in `modified.epochs_to_remove`. -// TODO: try to optimize this by only writing to storage via Lazy! // `def computeNewRedelegatedUnbonds` from Quint fn compute_new_redelegated_unbonds( storage: &S, @@ -3366,7 +3361,6 @@ where ); return None; } - // TODO: maybe debug_assert that the new stake is >= threshold? } let consensus_key = validator_consensus_key_handle(&address) .get(storage, next_epoch, params) @@ -4044,10 +4038,6 @@ where address, ) = validator?; - // TODO: - // When below-threshold validator set is added, this shouldn't be needed - // anymore since some minimal stake will be required to be in at least - // the consensus set if stake.is_zero() { continue; } @@ -4108,8 +4098,6 @@ where { // Read the rewards accumulator and calculate the new rewards products // for the previous epoch - // - // TODO: think about changing the reward to Decimal let mut reward_tokens_remaining = inflation; let mut new_rewards_products: HashMap = HashMap::new(); let mut accumulators_sum = Dec::zero(); @@ -4218,7 +4206,10 @@ pub fn compute_cubic_slash_rate( where S: StorageRead, { - // tracing::debug!("COMPUTING CUBIC SLASH RATE"); + tracing::debug!( + "Computing the cubic slash rate for infraction epoch \ + {infraction_epoch}." + ); let mut sum_vp_fraction = Dec::zero(); let (start_epoch, end_epoch) = params.cubic_slash_epoch_window(infraction_epoch); @@ -4251,15 +4242,14 @@ where // validator_stake); Ok(acc + Dec::from(validator_stake)) - // TODO: does something more complex need to be done - // here in the event some of these slashes correspond to - // the same validator? }, )?; sum_vp_fraction += infracting_stake / consensus_stake; } - // tracing::debug!("sum_vp_fraction: {}", sum_vp_fraction); - Ok(Dec::new(9, 0).unwrap() * sum_vp_fraction * sum_vp_fraction) + let cubic_rate = + Dec::new(9, 0).unwrap() * sum_vp_fraction * sum_vp_fraction; + tracing::debug!("Cubic slash rate: {}", cubic_rate); + Ok(cubic_rate) } /// Record a slash for a misbehavior that has been received from Tendermint and @@ -4460,7 +4450,7 @@ where // Collect the enqueued slashes and update their rates let mut eager_validator_slashes: BTreeMap> = - BTreeMap::new(); // TODO: will need to update this in storage later + BTreeMap::new(); let mut eager_validator_slash_rates: HashMap = HashMap::new(); // `slashPerValidator` and `slashesMap` while also updating in storage @@ -4806,10 +4796,7 @@ where redel_bond_start, ) && bond_start <= slash.epoch && slash.epoch + params.slash_processing_epoch_offset() - // TODO this may need to be `<=` as in `fn compute_total_unbonded` - // - // NOTE(Tomas): Agreed and changed to `<=`. We're looking - // for slashes that were processed before or in the epoch + // We're looking for slashes that were processed before or in the epoch // in which slashes that are currently being processed // occurred. Because we're slashing in the beginning of an // epoch, we're also taking slashes that were processed in @@ -5007,7 +4994,6 @@ where .iter(storage)? .map(Result::unwrap) .filter(|slash| { - // TODO: check bounds on second arg start <= slash.epoch && slash.epoch + params.slash_processing_epoch_offset() <= epoch }) @@ -5119,7 +5105,6 @@ where ) .into()); } - // TODO: any other checks that are needed? (deltas, etc)? // Re-insert the validator into the validator set and update its state let pipeline_epoch = current_epoch + params.pipeline_len; @@ -5258,7 +5243,6 @@ where // started contributing to the src validator's voting power, these tokens // cannot be slashed anymore let is_not_chained = if let Some(end_epoch) = src_redel_end_epoch { - // TODO: check bounds for correctness (> and presence of cubic offset) let last_contrib_epoch = end_epoch.prev(); // If the source validator's slashes that would cause slash on // redelegation are now outdated (would have to be processed before or @@ -5409,7 +5393,8 @@ where Ok(()) } -/// De-activate a validator by removing it from any validator sets +/// Deactivate a validator by removing it from any validator sets. A validator +/// can only be deactivated if it is not jailed or already inactive. pub fn deactivate_validator( storage: &mut S, validator: &Address, @@ -5546,8 +5531,6 @@ pub fn reactivate_validator( where S: StorageRead + StorageWrite, { - // TODO: need to additionally check for past slashes, in case a jailed - // validator tries to deactivate and then reactivate let params = read_pos_params(storage)?; let pipeline_epoch = current_epoch + params.pipeline_len; @@ -5572,7 +5555,9 @@ where } } - // Check to see if the validator should still be jailed upon a reactivation + // Check to see if the validator should be jailed upon a reactivation. This + // may occur if a validator is deactivated but then an infraction is + // discovered later. let last_slash_epoch = read_validator_last_slash_epoch(storage, validator)?; if let Some(last_slash_epoch) = last_slash_epoch { let eligible_epoch = @@ -5585,7 +5570,6 @@ where pipeline_epoch, 0, )?; - // End execution here? return Ok(()); } } diff --git a/proof_of_stake/src/rewards.rs b/proof_of_stake/src/rewards.rs index 26d1b91442..427e08c50c 100644 --- a/proof_of_stake/src/rewards.rs +++ b/proof_of_stake/src/rewards.rs @@ -55,7 +55,6 @@ impl PosRewardsCalculator { /// the validator's signing behavior and stake to determine the fraction of /// the block rewards earned. pub fn get_reward_coeffs(&self) -> Result { - // TODO: think about possibility of u64 overflow let votes_needed = self.get_min_required_votes(); let Self { diff --git a/proof_of_stake/src/tests.rs b/proof_of_stake/src/tests.rs index aee5ace020..d4390ef17a 100644 --- a/proof_of_stake/src/tests.rs +++ b/proof_of_stake/src/tests.rs @@ -2501,7 +2501,6 @@ fn test_compute_modified_redelegation() { let mut bob = validator2.clone(); // Ensure a ranking order of alice > bob - // TODO: check why this needs to be > (am I just confusing myself?) if bob > alice { alice = validator2; bob = validator1; diff --git a/proof_of_stake/src/types.rs b/proof_of_stake/src/types.rs index b948f5d8fd..ee7f94292a 100644 --- a/proof_of_stake/src/types.rs +++ b/proof_of_stake/src/types.rs @@ -24,8 +24,6 @@ pub use rev_order::ReverseOrdTokenAmount; use crate::parameters::PosParams; -// TODO: review the offsets for each epoched type!! - /// Stored positions of validators in validator sets pub type ValidatorSetPositions = crate::epoched::NestedEpoched< LazyMap, @@ -71,11 +69,11 @@ pub type ValidatorStates = crate::epoched::Epoched< /// A map from a position to an address in a Validator Set pub type ValidatorPositionAddresses = LazyMap; -/// New validator set construction, keyed by staked token amount +/// Consensus validator set, keyed by staked token amount pub type ConsensusValidatorSet = NestedMap; -/// New validator set construction, keyed by staked token amount +/// Below-capacity validator set, keyed by staked token amount pub type BelowCapacityValidatorSet = NestedMap; @@ -128,8 +126,7 @@ pub type Bonds = crate::epoched::EpochedDelta< crate::epoched::OffsetMaxU64, >; -/// An epoched lazy set of all known active validator addresses (consensus, -/// below-capacity, below-threshold, jailed) +/// An epoched lazy set of all known validator addresses pub type ValidatorAddresses = crate::epoched::NestedEpoched< LazySet
, crate::epoched::OffsetPipelineLen, From 529c07124b5411210ab82ef1988167e7b8fc7a59 Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 09:54:32 -0800 Subject: [PATCH 6/9] remove unnecessary computation of total consensus stake --- proof_of_stake/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 8ed8bbbfa1..ee7bf4cfcb 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -322,7 +322,7 @@ where current_epoch, epoch, )?; - compute_and_store_total_consensus_stake(storage, epoch)?; + // compute_and_store_total_consensus_stake(storage, epoch)?; } Ok(()) } From 9ee4887475501b32c2db3c2ba190606bf134a3bf Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 09:54:53 -0800 Subject: [PATCH 7/9] checked arithmetic --- proof_of_stake/src/lib.rs | 25 +++++++++++++++++++++---- proof_of_stake/src/rewards.rs | 7 ++++++- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index ee7bf4cfcb..6ccea401f3 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -518,7 +518,13 @@ where let val = handle .get_delta_val(storage, current_epoch + offset)? .unwrap_or_default(); - handle.set(storage, val + delta, current_epoch, offset) + handle.set( + storage, + val.checked_add(&delta) + .expect("Validator deltas updated amount should not overflow"), + current_epoch, + offset, + ) } /// Read PoS total stake (sum of deltas). @@ -698,7 +704,13 @@ where let val = handle .get_delta_val(storage, current_epoch + offset)? .unwrap_or_default(); - handle.set(storage, val + delta, current_epoch, offset) + handle.set( + storage, + val.checked_add(&delta) + .expect("Total deltas updated amount should not overflow"), + current_epoch, + offset, + ) } /// Check if the provided address is a validator address @@ -987,7 +999,10 @@ where // tracing::debug!("VALIDATOR STAKE BEFORE UPDATE: {}", tokens_pre); - let tokens_post = tokens_pre.change() + token_change; + let tokens_post = tokens_pre + .change() + .checked_add(&token_change) + .expect("Post-validator set update token amount has overflowed"); debug_assert!(tokens_post.non_negative()); let tokens_post = token::Amount::from_change(tokens_post); @@ -1475,7 +1490,9 @@ where }, _validator, ) = entry?; - Ok(acc + amount) + Ok(acc.checked_add(amount).expect( + "Total consensus stake computation should not overflow.", + )) }) } diff --git a/proof_of_stake/src/rewards.rs b/proof_of_stake/src/rewards.rs index 427e08c50c..a42d40b67c 100644 --- a/proof_of_stake/src/rewards.rs +++ b/proof_of_stake/src/rewards.rs @@ -90,6 +90,11 @@ impl PosRewardsCalculator { /// Implement as ceiling of (2/3) * validator set stake fn get_min_required_votes(&self) -> Amount { - ((self.total_stake * 2u64) + (3u64 - 1u64)) / 3u64 + (self + .total_stake + .checked_mul(2.into()) + .expect("Amount overflow while computing minimum required votes") + + (3u64 - 1u64)) + / 3u64 } } From 887b9914634e49cbf5855c96fcbc8a01a7180ca8 Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 10:58:05 -0800 Subject: [PATCH 8/9] fix and test `is_delegator` --- proof_of_stake/src/lib.rs | 15 ++++- proof_of_stake/src/tests.rs | 122 ++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 3 deletions(-) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 6ccea401f3..03ce5d8af2 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -753,9 +753,18 @@ where } Ok(false) } - None => Ok(storage_api::iter_prefix_bytes(storage, &prefix)? - .next() - .is_some()), + None => { + let iter = storage_api::iter_prefix_bytes(storage, &prefix)?; + for res in iter { + let (key, _) = res?; + if let Some((bond_id, _epoch)) = is_bond_key(&key) { + if bond_id.source != bond_id.validator { + return Ok(true); + } + } + } + Ok(false) + } } } diff --git a/proof_of_stake/src/tests.rs b/proof_of_stake/src/tests.rs index d4390ef17a..9f74553f1e 100644 --- a/proof_of_stake/src/tests.rs +++ b/proof_of_stake/src/tests.rs @@ -293,6 +293,22 @@ proptest! { } } +proptest! { + // Generate arb valid input for `test_is_delegator` + #![proptest_config(Config { + cases: 100, + .. Config::default() + })] + #[test] + fn test_is_delegator( + + genesis_validators in arb_genesis_validators(2..3, None), + + ) { + test_is_delegator_aux(genesis_validators) + } +} + fn arb_params_and_genesis_validators( num_max_validator_slots: Option, val_size: Range, @@ -6560,3 +6576,109 @@ fn test_slashed_bond_amount_aux(validators: Vec) { let diff = val_stake - self_bond_amount - del_bond_amount; assert!(diff <= 2.into()); } + +fn test_is_delegator_aux(mut validators: Vec) { + validators.sort_by(|a, b| b.tokens.cmp(&a.tokens)); + + let validator1 = validators[0].address.clone(); + let validator2 = validators[1].address.clone(); + + let mut storage = TestWlStorage::default(); + let params = OwnedPosParams { + unbonding_len: 4, + ..Default::default() + }; + + // Genesis + let mut current_epoch = storage.storage.block.epoch; + let params = test_init_genesis( + &mut storage, + params, + validators.clone().into_iter(), + current_epoch, + ) + .unwrap(); + storage.commit_block().unwrap(); + + // Get delegators with some tokens + let staking_token = staking_token_address(&storage); + let delegator1 = address::testing::gen_implicit_address(); + let delegator2 = address::testing::gen_implicit_address(); + let del_balance = token::Amount::native_whole(1000); + credit_tokens(&mut storage, &staking_token, &delegator1, del_balance) + .unwrap(); + credit_tokens(&mut storage, &staking_token, &delegator2, del_balance) + .unwrap(); + + // Advance to epoch 1 + current_epoch = advance_epoch(&mut storage, ¶ms); + super::process_slashes(&mut storage, current_epoch).unwrap(); + + // Delegate in epoch 1 to validator1 + let del1_epoch = current_epoch; + super::bond_tokens( + &mut storage, + Some(&delegator1), + &validator1, + 1000.into(), + current_epoch, + None, + ) + .unwrap(); + + // Advance to epoch 2 + current_epoch = advance_epoch(&mut storage, ¶ms); + super::process_slashes(&mut storage, current_epoch).unwrap(); + + // Delegate in epoch 2 to validator2 + let del2_epoch = current_epoch; + super::bond_tokens( + &mut storage, + Some(&delegator2), + &validator2, + 1000.into(), + current_epoch, + None, + ) + .unwrap(); + + // Checks + assert!(super::is_validator(&storage, &validator1).unwrap()); + assert!(super::is_validator(&storage, &validator2).unwrap()); + assert!(!super::is_delegator(&storage, &validator1, None).unwrap()); + assert!(!super::is_delegator(&storage, &validator2, None).unwrap()); + + assert!(!super::is_validator(&storage, &delegator1).unwrap()); + assert!(!super::is_validator(&storage, &delegator2).unwrap()); + assert!(super::is_delegator(&storage, &delegator1, None).unwrap()); + assert!(super::is_delegator(&storage, &delegator2, None).unwrap()); + + for epoch in Epoch::default().iter_range(del1_epoch.0 + params.pipeline_len) + { + assert!( + !super::is_delegator(&storage, &delegator1, Some(epoch)).unwrap() + ); + } + assert!( + super::is_delegator( + &storage, + &delegator1, + Some(del1_epoch + params.pipeline_len) + ) + .unwrap() + ); + for epoch in Epoch::default().iter_range(del2_epoch.0 + params.pipeline_len) + { + assert!( + !super::is_delegator(&storage, &delegator2, Some(epoch)).unwrap() + ); + } + assert!( + super::is_delegator( + &storage, + &delegator2, + Some(del2_epoch + params.pipeline_len) + ) + .unwrap() + ); +} From 02f59354c817e793b7021e59ef490d13c1f505f7 Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 14 Nov 2023 11:11:09 -0800 Subject: [PATCH 9/9] changelog: add #2178 --- .changelog/unreleased/improvements/2178-misc-pos-tasks.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/improvements/2178-misc-pos-tasks.md diff --git a/.changelog/unreleased/improvements/2178-misc-pos-tasks.md b/.changelog/unreleased/improvements/2178-misc-pos-tasks.md new file mode 100644 index 0000000000..39f95b479b --- /dev/null +++ b/.changelog/unreleased/improvements/2178-misc-pos-tasks.md @@ -0,0 +1,3 @@ +- Various improvements to the PoS code, including adding a panic on a slashing + failure, some more checked arithmetics, aesthetic code cleanup, and fixing a + bug in is_delegator. ([\#2178](https://github.com/anoma/namada/pull/2178)) \ No newline at end of file