From fd975b9e1a28c1d3b54f64dcdc1984d1eaf99ed3 Mon Sep 17 00:00:00 2001 From: dandanlen <3168260+dandanlen@users.noreply.github.com> Date: Mon, 23 Oct 2023 16:13:06 +0200 Subject: [PATCH] chore: revise-reputation-constants (#4117) - deletes online credits - updates the values of chainspec constants - updates signer nomination criteria - revises and clarifies naming Co-authored-by: Martin Rieke --- .../cf-integration-tests/src/account.rs | 4 +- .../cf-integration-tests/src/mock_runtime.rs | 2 + .../src/signer_nomination.rs | 34 +------- state-chain/custom-rpc/src/lib.rs | 6 -- state-chain/node/src/chain_spec/common.rs | 26 +++--- .../node/src/chain_spec/perseverance.rs | 4 - state-chain/pallets/cf-broadcast/src/lib.rs | 10 +-- state-chain/pallets/cf-reputation/README.md | 6 +- .../pallets/cf-reputation/src/benchmarking.rs | 2 +- state-chain/pallets/cf-reputation/src/lib.rs | 78 ++++++++---------- .../pallets/cf-reputation/src/reputation.rs | 73 ++++++----------- .../pallets/cf-reputation/src/tests.rs | 81 ++++++++++++------- .../src/chainflip/signer_nomination.rs | 17 ++-- state-chain/runtime/src/constants.rs | 6 -- state-chain/runtime/src/lib.rs | 1 - state-chain/runtime/src/runtime_apis.rs | 1 - state-chain/traits/src/lib.rs | 17 ++-- .../traits/src/mocks/signer_nomination.rs | 14 ++-- 18 files changed, 162 insertions(+), 220 deletions(-) diff --git a/state-chain/cf-integration-tests/src/account.rs b/state-chain/cf-integration-tests/src/account.rs index 535615e201..9716a84cd5 100644 --- a/state-chain/cf-integration-tests/src/account.rs +++ b/state-chain/cf-integration-tests/src/account.rs @@ -37,7 +37,7 @@ fn account_deletion_removes_relevant_storage_items() { network::Cli::start_bidding(&backup_node); Reputation::heartbeat(state_chain_runtime::RuntimeOrigin::signed(backup_node.clone())) .unwrap(); - assert!(Reputations::::get(backup_node.clone()).online_credits > 0); + assert!(Reputations::::contains_key(backup_node.clone())); let elon_vanity_name = "ElonShibaMoonInu"; network::Cli::set_vanity_name(&backup_node, elon_vanity_name); @@ -70,6 +70,6 @@ fn account_deletion_removes_relevant_storage_items() { let vanity_names = VanityNames::::get(); assert!(vanity_names.get(&backup_node).is_none()); assert_eq!(pallet_cf_account_roles::AccountRoles::::get(&backup_node), None); - assert_eq!(Reputations::::get(backup_node).online_credits, 0); + assert_eq!(Reputations::::get(backup_node).online_blocks, 0); }); } diff --git a/state-chain/cf-integration-tests/src/mock_runtime.rs b/state-chain/cf-integration-tests/src/mock_runtime.rs index bee02a5d54..f385f16615 100644 --- a/state-chain/cf-integration-tests/src/mock_runtime.rs +++ b/state-chain/cf-integration-tests/src/mock_runtime.rs @@ -23,6 +23,8 @@ pub const BACKUP_NODE_EMISSION_INFLATION_PERBILL: u32 = 6; pub const SUPPLY_UPDATE_INTERVAL_DEFAULT: u32 = 14_400; pub const MIN_FUNDING: FlipBalance = 10 * FLIPPERINOS_PER_FLIP; +pub const ACCRUAL_RATIO: (i32, u32) = (1, 1); + /// The offences committable within the protocol and their respective reputation penalty and /// suspension durations. pub const PENALTIES: &[(Offence, (i32, BlockNumber))] = &[ diff --git a/state-chain/cf-integration-tests/src/signer_nomination.rs b/state-chain/cf-integration-tests/src/signer_nomination.rs index 5646049775..693cb0853f 100644 --- a/state-chain/cf-integration-tests/src/signer_nomination.rs +++ b/state-chain/cf-integration-tests/src/signer_nomination.rs @@ -1,9 +1,6 @@ use std::collections::BTreeSet; -use cf_traits::{ - offence_reporting::OffenceReporter, EpochInfo, SingleSignerNomination, - ThresholdSignerNomination, -}; +use cf_traits::{offence_reporting::OffenceReporter, EpochInfo, ThresholdSignerNomination}; use pallet_cf_threshold_signature::PalletOffence; use pallet_cf_validator::{CurrentAuthorities, CurrentEpoch, HistoricalAuthorities}; use sp_runtime::AccountId32; @@ -12,9 +9,6 @@ use state_chain_runtime::{EthereumInstance, Reputation, Runtime, Validator}; type RuntimeThresholdSignerNomination = >::ThresholdSignerNomination; -type RuntimeBroadcastsignerNomination = - >::BroadcastSignerNomination; - #[test] fn threshold_signer_nomination_respects_epoch() { super::genesis::default().build().execute_with(|| { @@ -80,15 +74,6 @@ fn test_not_nominated_for_offence(penalise: F) { } } -#[test] -fn offline_nodes_are_not_nominated_for_threshold_signing() { - super::genesis::default().build().execute_with(|| { - test_not_nominated_for_offence(|node_id| { - Reputation::penalise_offline_authorities(vec![node_id]) - }); - }); -} - #[test] fn nodes_who_failed_to_sign_excluded_from_threshold_nomination() { super::genesis::default().build().execute_with(|| { @@ -97,20 +82,3 @@ fn nodes_who_failed_to_sign_excluded_from_threshold_nomination() { }); }); } - -#[test] -fn offline_nodes_are_not_nominated_transaction_signing() { - super::genesis::default().build().execute_with(|| { - let node1 = Validator::current_authorities().first().unwrap().clone(); - - Reputation::penalise_offline_authorities(vec![node1.clone()]); - - for seed in 0..20 { - // no extra ids, excluded. - assert_ne!( - RuntimeBroadcastsignerNomination::nomination_with_seed(seed, &[]).unwrap(), - node1 - ); - } - }); -} diff --git a/state-chain/custom-rpc/src/lib.rs b/state-chain/custom-rpc/src/lib.rs index 6b55f36bda..c84f498663 100644 --- a/state-chain/custom-rpc/src/lib.rs +++ b/state-chain/custom-rpc/src/lib.rs @@ -50,7 +50,6 @@ pub enum RpcAccountInfo { flip_balance: NumberOrHex, bond: NumberOrHex, last_heartbeat: u32, - online_credits: u32, reputation_points: i32, keyholder_epochs: Vec, is_current_authority: bool, @@ -106,7 +105,6 @@ impl RpcAccountInfo { flip_balance: info.balance.into(), bond: info.bond.into(), last_heartbeat: info.last_heartbeat, - online_credits: info.online_credits, reputation_points: info.reputation_points, keyholder_epochs: info.keyholder_epochs, is_current_authority: info.is_current_authority, @@ -130,7 +128,6 @@ pub struct RpcAccountInfoV2 { pub balance: NumberOrHex, pub bond: NumberOrHex, pub last_heartbeat: u32, - pub online_credits: u32, pub reputation_points: i32, pub keyholder_epochs: Vec, pub is_current_authority: bool, @@ -582,7 +579,6 @@ where balance: account_info.balance.into(), bond: account_info.bond.into(), last_heartbeat: account_info.last_heartbeat, - online_credits: account_info.online_credits, reputation_points: account_info.reputation_points, keyholder_epochs: account_info.keyholder_epochs, is_current_authority: account_info.is_current_authority, @@ -1005,7 +1001,6 @@ mod test { balance: 10u128.pow(18), bond: 10u128.pow(18), last_heartbeat: 0, - online_credits: 0, reputation_points: 0, keyholder_epochs: vec![123], is_current_authority: true, @@ -1030,7 +1025,6 @@ mod test { "is_qualified": true, "keyholder_epochs": [123], "last_heartbeat": 0, - "online_credits": 0, "reputation_points": 0, "role": "validator", "apy_bp": 100, diff --git a/state-chain/node/src/chain_spec/common.rs b/state-chain/node/src/chain_spec/common.rs index d5d925bb02..0167c955a5 100644 --- a/state-chain/node/src/chain_spec/common.rs +++ b/state-chain/node/src/chain_spec/common.rs @@ -21,23 +21,23 @@ pub const CURRENT_AUTHORITY_EMISSION_INFLATION_PERBILL: u32 = 28; pub const BACKUP_NODE_EMISSION_INFLATION_PERBILL: u32 = 6; pub const SUPPLY_UPDATE_INTERVAL: u32 = 24 * HOURS; -// Number of online credits required to get `ACCRUAL_REPUTATION_POINTS` of reputation -const ACCRUAL_ONLINE_CREDITS: u32 = 2500; -// Number of reputation points received for having `ACCRUAL_ONLINE_CREDITS` -const ACCRUAL_REPUTATION_POINTS: i32 = 1; -pub const ACCRUAL_RATIO: (i32, u32) = (ACCRUAL_REPUTATION_POINTS, ACCRUAL_ONLINE_CREDITS); +// This is equivalent to one reputation point for every minute of online time. +pub const REPUTATION_PER_HEARTBEAT: i32 = 15; +pub const ACCRUAL_RATIO: (i32, u32) = (REPUTATION_PER_HEARTBEAT, HEARTBEAT_BLOCK_INTERVAL); + +const REPUTATION_PENALTY_SMALL: i32 = REPUTATION_PER_HEARTBEAT; // 15 minutes to recover reputation +const REPUTATION_PENALTY_MEDIUM: i32 = REPUTATION_PER_HEARTBEAT * 4; // One hour to recover reputation +const REPUTATION_PENALTY_LARGE: i32 = REPUTATION_PER_HEARTBEAT * 8; // Two hours to recover reputation /// The offences committable within the protocol and their respective reputation penalty and /// suspension durations. pub const PENALTIES: &[(Offence, (i32, BlockNumber))] = &[ - (Offence::ParticipateKeygenFailed, (15, HEARTBEAT_BLOCK_INTERVAL)), - (Offence::ParticipateSigningFailed, (15, HEARTBEAT_BLOCK_INTERVAL)), - (Offence::MissedAuthorshipSlot, (15, HEARTBEAT_BLOCK_INTERVAL)), - (Offence::MissedHeartbeat, (15, HEARTBEAT_BLOCK_INTERVAL)), - // We exclude them from the nomination pool of the next attempt, - // so there is no need to suspend them further. - (Offence::FailedToBroadcastTransaction, (10, 0)), - (Offence::GrandpaEquivocation, (50, HEARTBEAT_BLOCK_INTERVAL * 5)), + (Offence::MissedHeartbeat, (REPUTATION_PENALTY_SMALL, 0)), + (Offence::ParticipateKeygenFailed, (REPUTATION_PENALTY_MEDIUM, HEARTBEAT_BLOCK_INTERVAL)), + (Offence::ParticipateSigningFailed, (REPUTATION_PENALTY_MEDIUM, HEARTBEAT_BLOCK_INTERVAL)), + (Offence::MissedAuthorshipSlot, (REPUTATION_PENALTY_LARGE, HEARTBEAT_BLOCK_INTERVAL)), + (Offence::FailedToBroadcastTransaction, (REPUTATION_PENALTY_MEDIUM, HEARTBEAT_BLOCK_INTERVAL)), + (Offence::GrandpaEquivocation, (REPUTATION_PENALTY_LARGE, HEARTBEAT_BLOCK_INTERVAL * 5)), ]; pub const MINIMUM_SWAP_AMOUNTS: &[(Asset, AssetAmount)] = &[ diff --git a/state-chain/node/src/chain_spec/perseverance.rs b/state-chain/node/src/chain_spec/perseverance.rs index e8f99ac836..90a4d58d28 100644 --- a/state-chain/node/src/chain_spec/perseverance.rs +++ b/state-chain/node/src/chain_spec/perseverance.rs @@ -8,10 +8,6 @@ use cf_primitives::{AccountId, AccountRole, BlockNumber, FlipBalance, NetworkEnv use sc_service::ChainType; use sp_core::H256; -// *** Overrides from common -pub const ACCRUAL_RATIO: (i32, u32) = (10, 10); -// *** - pub struct Config; pub const NETWORK_NAME: &str = "Chainflip-Perseverance"; diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 5712e36f3e..eef21ca59b 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -16,8 +16,8 @@ impl_pallet_safe_mode!(PalletSafeMode; retry_enabled); use cf_chains::{ApiCall, Chain, ChainCrypto, FeeRefundCalculator, TransactionBuilder}; use cf_traits::{ - offence_reporting::OffenceReporter, Broadcaster, Chainflip, EpochInfo, EpochKey, - OnBroadcastReady, SingleSignerNomination, ThresholdSigner, + offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, Chainflip, EpochInfo, + EpochKey, OnBroadcastReady, ThresholdSigner, }; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ @@ -73,7 +73,7 @@ pub const PALLET_VERSION: StorageVersion = StorageVersion::new(1); pub mod pallet { use super::*; use cf_chains::benchmarking_value::BenchmarkValue; - use cf_traits::{AccountRoleRegistry, KeyProvider, OnBroadcastReady, SingleSignerNomination}; + use cf_traits::{AccountRoleRegistry, BroadcastNomination, KeyProvider, OnBroadcastReady}; use frame_support::{ensure, pallet_prelude::*, traits::EnsureOrigin}; use frame_system::pallet_prelude::*; @@ -169,7 +169,7 @@ pub mod pallet { >; /// Signer nomination. - type BroadcastSignerNomination: SingleSignerNomination; + type BroadcastSignerNomination: BroadcastNomination; /// For reporting bad actors. type OffenceReporter: OffenceReporter< @@ -759,7 +759,7 @@ impl, I: 'static> Pallet { let seed = (broadcast_attempt.broadcast_attempt_id, broadcast_attempt.transaction_payload.clone()) .encode(); - if let Some(nominated_signer) = T::BroadcastSignerNomination::nomination_with_seed( + if let Some(nominated_signer) = T::BroadcastSignerNomination::nominate_broadcaster( seed, &FailedBroadcasters::::get(broadcast_attempt.broadcast_attempt_id.broadcast_id) .unwrap_or_default(), diff --git a/state-chain/pallets/cf-reputation/README.md b/state-chain/pallets/cf-reputation/README.md index 35c2ccaf4f..d4ba53074e 100644 --- a/state-chain/pallets/cf-reputation/README.md +++ b/state-chain/pallets/cf-reputation/README.md @@ -4,7 +4,7 @@ A module to manage offences, reputation and suspensions of our nodes for the Sta ## Overview -Nodes earn reputations points for remaining online. For every block of online time, they receive an online credit. At regular intervals dictated by the heartbeat duration, these credits are exchanged for reputation points according to an *accrual rate*. +Nodes earn reputation points for remaining online. We count the number of blocks between heartbeat submissions and, at regular intervals dictated by the heartbeat duration, these are converted to reputation points according to an *accrual rate*. If a node is reported for committing an offence, the matching penalty is resolved. A penalty consists of a reputation penalty and a suspension duration measured in blocks. Note both the penalty and suspension can be zero. @@ -22,9 +22,9 @@ Once every heartbeat interval, this pallet divides nodes into nodes that are 'on - Heartbeat interval: The duration, measured in blocks, after which we consider a node to be offline if no heartbeat is received. - Online: A node is considered online if its most recent hearbeat was at most `heartbeat_interval` blocks ago. - Offline: A node is considered offline if its most recent heartbeat was more than `heartbeat_interval` blocks ago. -- Online credits: Online credits increase for every heartbeat interval in which a node submitted their heartbeat. +- Online Blocks: We count the number of blocks for which a validator has been online, based on heartbeat submissions. - Reputation points: A measure of how diligently a node has been fulfilling its duties. - Suspension: A suspension is served for a given offence and lasts for a number of blocks. The consequences of suspensions are not defined by this pallet - rather the currently suspended nodes for any collection of offences can be queried in order to act - Offences: any event that can be reported and might incur a reputation penalty and/or suspension. - Slashing: The process of confiscating and burning FLIP tokens from an authority. -- Accrual Ratio: A ratio of reputation points earned per number of offline credits +- Accrual Ratio: A ratio of reputation points earned per number of online blocks. diff --git a/state-chain/pallets/cf-reputation/src/benchmarking.rs b/state-chain/pallets/cf-reputation/src/benchmarking.rs index c7774862d8..18ac55f9ed 100644 --- a/state-chain/pallets/cf-reputation/src/benchmarking.rs +++ b/state-chain/pallets/cf-reputation/src/benchmarking.rs @@ -12,7 +12,7 @@ const MAX_VALIDATOR_COUNT: u32 = 150; benchmarks! { update_accrual_ratio { - let call = Call::::update_accrual_ratio{ reputation_points: 2, online_credits: 151u32.into() }; + let call = Call::::update_accrual_ratio{ reputation_points: 2, number_of_blocks: 151u32.into() }; } : { let _ = call.dispatch_bypass_filter(T::EnsureGovernance::try_successful_origin().unwrap()); } set_penalty { let call = Call::::set_penalty { offence: PalletOffence::MissedHeartbeat.into(), new_penalty: Default::default() }; diff --git a/state-chain/pallets/cf-reputation/src/lib.rs b/state-chain/pallets/cf-reputation/src/lib.rs index d92cbdc957..bf99faab83 100644 --- a/state-chain/pallets/cf-reputation/src/lib.rs +++ b/state-chain/pallets/cf-reputation/src/lib.rs @@ -38,13 +38,13 @@ use sp_std::{ impl_pallet_safe_mode!(PalletSafeMode; reporting_enabled); impl ReputationParameters for T { - type OnlineCredits = BlockNumberFor; + type BlockNumber = BlockNumberFor; fn bounds() -> (ReputationPoints, ReputationPoints) { T::ReputationPointFloorAndCeiling::get() } - fn accrual_rate() -> (ReputationPoints, Self::OnlineCredits) { + fn accrual_rate() -> (ReputationPoints, Self::BlockNumber) { AccrualRatio::::get() } } @@ -137,7 +137,7 @@ pub mod pallet { impl Hooks> for Pallet { fn on_initialize(current_block: BlockNumberFor) -> Weight { if T::SafeMode::get().reporting_enabled && - Self::blocks_since_new_interval(current_block) == Zero::zero() + current_block % T::HeartbeatBlockInterval::get() == Zero::zero() { // Reputation depends on heartbeats Self::penalise_offline_authorities(Self::current_network_state().offline); @@ -148,7 +148,7 @@ pub mod pallet { } } - /// The ratio at which one accrues Reputation points in exchange for online credits + /// The ratio at which one accrues Reputation points for online blocks. #[pallet::storage] #[pallet::getter(fn accrual_ratio)] pub type AccrualRatio = @@ -196,7 +196,7 @@ pub mod pallet { /// The accrual rate for our reputation points has been updated. AccrualRateUpdated { reputation_points: ReputationPoints, - online_credits: BlockNumberFor, + number_of_blocks: BlockNumberFor, }, /// The penalty for missing a heartbeat has been updated. MissedHeartbeatPenaltyUpdated { new_reputation_penalty: ReputationPoints }, @@ -212,8 +212,9 @@ pub mod pallet { #[pallet::call] impl Pallet { - /// The accrual ratio can be updated and would come into play in the current heartbeat - /// interval. This is gated with governance. + /// Updates the rate at which reputation points are accrued. + /// + /// For every `number_of_blocks` blocks, `reputation_points` points are accrued. /// /// ## Events /// @@ -227,20 +228,20 @@ pub mod pallet { pub fn update_accrual_ratio( origin: OriginFor, reputation_points: ReputationPoints, - online_credits: BlockNumberFor, - ) -> DispatchResultWithPostInfo { + number_of_blocks: BlockNumberFor, + ) -> DispatchResult { T::EnsureGovernance::ensure_origin(origin)?; ensure!( reputation_points <= T::MaximumAccruableReputation::get() && - online_credits > Zero::zero(), + number_of_blocks > Zero::zero(), Error::::InvalidAccrualRatio ); - AccrualRatio::::set((reputation_points, online_credits)); - Self::deposit_event(Event::AccrualRateUpdated { reputation_points, online_credits }); + AccrualRatio::::set((reputation_points, number_of_blocks)); + Self::deposit_event(Event::AccrualRateUpdated { reputation_points, number_of_blocks }); - Ok(().into()) + Ok(()) } /// Updates the penalty for missing a heartbeat. @@ -253,7 +254,7 @@ pub mod pallet { pub fn update_missed_heartbeat_penalty( origin: OriginFor, new_reputation_penalty: ReputationPoints, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { T::EnsureGovernance::ensure_origin(origin)?; Penalties::::insert( @@ -265,7 +266,7 @@ pub mod pallet { ); Self::deposit_event(Event::MissedHeartbeatPenaltyUpdated { new_reputation_penalty }); - Ok(().into()) + Ok(()) } /// Set the [Penalty] for an [Offence]. @@ -275,7 +276,7 @@ pub mod pallet { origin: OriginFor, offence: T::Offence, new_penalty: Penalty, - ) -> DispatchResultWithPostInfo { + ) -> DispatchResult { T::EnsureGovernance::ensure_origin(origin)?; let old_penalty = Penalties::::mutate(offence, |penalty| { @@ -286,7 +287,7 @@ pub mod pallet { Self::deposit_event(Event::::PenaltyUpdated { offence, old_penalty, new_penalty }); - Ok(().into()) + Ok(()) } /// A heartbeat is used to measure the liveness of a node. It is measured in blocks. @@ -304,29 +305,22 @@ pub mod pallet { /// - [BadOrigin](frame_support::error::BadOrigin) #[pallet::call_index(3)] #[pallet::weight(T::WeightInfo::heartbeat())] - pub fn heartbeat(origin: OriginFor) -> DispatchResultWithPostInfo { + pub fn heartbeat(origin: OriginFor) -> DispatchResult { let validator_id: T::ValidatorId = T::AccountRoleRegistry::ensure_validator(origin)?.into(); let current_block_number = frame_system::Pallet::::current_block_number(); - let start_of_this_interval = - current_block_number - Self::blocks_since_new_interval(current_block_number); - - // Heartbeat intervals range is (start, end] - match LastHeartbeat::::get(&validator_id) { - Some(last_heartbeat) if last_heartbeat > start_of_this_interval => { - // we have already submitted a heartbeat for this interval - }, - _ => { - Reputations::::mutate(&validator_id, |rep| { - rep.boost_reputation(T::HeartbeatBlockInterval::get()); - }); - }, - }; - - LastHeartbeat::::insert(&validator_id, current_block_number); + Reputations::::mutate(&validator_id, |rep| { + rep.boost_reputation(sp_std::cmp::min( + T::HeartbeatBlockInterval::get(), + current_block_number - + LastHeartbeat::::mutate(&validator_id, |last_heartbeat| { + last_heartbeat.replace(current_block_number).unwrap_or_default() + }), + )); + }); - Ok(().into()) + Ok(()) } } @@ -345,11 +339,6 @@ pub mod pallet { } impl Pallet { - /// Returns the number of blocks that have elapsed since the new HeartbeatBlockInterval - pub fn blocks_since_new_interval(block_number: BlockNumberFor) -> BlockNumberFor { - block_number % T::HeartbeatBlockInterval::get() - } - /// Partitions the authorities based on whether they are considered online or offline. pub fn current_network_state() -> NetworkState { let (online, offline) = @@ -460,7 +449,7 @@ impl Pallet { ); for validator_id in offline_authorities { let reputation_points = Reputations::::mutate(&validator_id, |rep| { - rep.reset_online_credits(); + rep.online_blocks = Zero::zero(); rep.reputation_points }); @@ -514,11 +503,10 @@ impl Pallet { impl ReputationResetter for Pallet { type ValidatorId = T::ValidatorId; - /// Reset both the online credits and the reputation points of a validator to zero. + /// Reset both the reputation of a validator to the default. fn reset_reputation(validator: &Self::ValidatorId) { - Reputations::::mutate(validator, |rep| { - rep.reset_reputation(); - rep.reset_online_credits(); + Reputations::::mutate(validator, |rep: &mut RuntimeReputationTracker<_>| { + *rep = Default::default() }); } } diff --git a/state-chain/pallets/cf-reputation/src/reputation.rs b/state-chain/pallets/cf-reputation/src/reputation.rs index 5a010d1ca2..f2af3cb7ac 100644 --- a/state-chain/pallets/cf-reputation/src/reputation.rs +++ b/state-chain/pallets/cf-reputation/src/reputation.rs @@ -1,8 +1,8 @@ use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ pallet_prelude::Member, - sp_runtime::traits::{AtLeast32BitUnsigned, Saturating, Zero}, - Parameter, + sp_runtime::traits::{AtLeast32BitUnsigned, Saturating}, + DebugNoBound, DefaultNoBound, Parameter, }; use scale_info::TypeInfo; use sp_std::fmt::Debug; @@ -10,31 +10,18 @@ use sp_std::fmt::Debug; pub type ReputationPoints = i32; /// Reputation of a node -#[derive(Encode, Decode, TypeInfo, MaxEncodedLen, Clone, PartialEq, Eq)] +#[derive( + Encode, Decode, DebugNoBound, DefaultNoBound, TypeInfo, MaxEncodedLen, Clone, PartialEq, Eq, +)] #[scale_info(skip_type_params(P))] #[codec(mel_bound(P: ReputationParameters))] pub struct ReputationTracker { - pub online_credits: P::OnlineCredits, + pub online_blocks: P::BlockNumber, pub reputation_points: ReputationPoints, } -impl Default for ReputationTracker

{ - fn default() -> Self { - Self { online_credits: Default::default(), reputation_points: Default::default() } - } -} - -impl Debug for ReputationTracker

{ - fn fmt(&self, f: &mut sp_std::fmt::Formatter<'_>) -> sp_std::fmt::Result { - f.debug_struct("ReputationTracker") - .field("online_credits", &self.online_credits) - .field("reputation_points", &self.reputation_points) - .finish() - } -} - pub trait ReputationParameters { - type OnlineCredits: Member + type BlockNumber: Member + Parameter + MaxEncodedLen + AtLeast32BitUnsigned @@ -44,18 +31,18 @@ pub trait ReputationParameters { // This is an on-chain constant fn bounds() -> (ReputationPoints, ReputationPoints); - fn accrual_rate() -> (ReputationPoints, Self::OnlineCredits); + fn accrual_rate() -> (ReputationPoints, Self::BlockNumber); } impl ReputationTracker

{ - /// Validators are rewarded with [OnlineCreditsFor] for remaining online. The credits are - /// automatically converted to reputation points according to - /// some conversion ratio (ie. the [AccrualRatio]). - pub fn boost_reputation(&mut self, online_credit_reward: P::OnlineCredits) { - self.online_credits.saturating_accrue(online_credit_reward); - let (reward, per_credits) = P::accrual_rate(); - while self.online_credits >= per_credits { - self.online_credits.saturating_reduce(per_credits); + /// Validators are rewarded for remaining online. We count the number of blocks they + /// have been online for, and periodically convert this into reputation according to + /// the accrual rate. + pub fn boost_reputation(&mut self, block_since_last_hearbeat: P::BlockNumber) { + self.online_blocks.saturating_accrue(block_since_last_hearbeat); + let (reward, per_blocks) = P::accrual_rate(); + while self.online_blocks >= per_blocks { + self.online_blocks.saturating_reduce(per_blocks); self.reputation_points.saturating_accrue(reward); self.clamp(); } @@ -72,16 +59,6 @@ impl ReputationTracker

{ let (floor, ceiling) = P::bounds(); self.reputation_points = self.reputation_points.clamp(floor, ceiling); } - - /// Reset online credits to zero. - pub fn reset_online_credits(&mut self) { - self.online_credits = Zero::zero(); - } - - /// Reset Reputation to zero. - pub fn reset_reputation(&mut self) { - self.reputation_points = Zero::zero(); - } } #[cfg(test)] @@ -96,13 +73,13 @@ mod test_reputation { impl ReputationParameters for TestParams { - type OnlineCredits = u32; + type BlockNumber = u32; fn bounds() -> (ReputationPoints, ReputationPoints) { BOUNDS } - fn accrual_rate() -> (ReputationPoints, Self::OnlineCredits) { + fn accrual_rate() -> (ReputationPoints, Self::BlockNumber) { (REWARD, RATE) } } @@ -111,27 +88,27 @@ mod test_reputation { fn test_reputation_accrual_bounds() { let mut rep = ReputationTracker::>::default(); assert_eq!(rep.reputation_points, 0); - assert_eq!(rep.online_credits, 0); + assert_eq!(rep.online_blocks, 0); rep.boost_reputation(70); assert_eq!(rep.reputation_points, 3); - assert_eq!(rep.online_credits, 10); + assert_eq!(rep.online_blocks, 10); rep.boost_reputation(10); assert_eq!(rep.reputation_points, 4); - assert_eq!(rep.online_credits, 0); + assert_eq!(rep.online_blocks, 0); rep.boost_reputation(10); assert_eq!(rep.reputation_points, 4); - assert_eq!(rep.online_credits, 10); + assert_eq!(rep.online_blocks, 10); rep.boost_reputation(1000); assert_eq!(rep.reputation_points, 5); - assert_eq!(rep.online_credits, 10); + assert_eq!(rep.online_blocks, 10); rep.deduct_reputation(1); assert_eq!(rep.reputation_points, 4); - assert_eq!(rep.online_credits, 10); + assert_eq!(rep.online_blocks, 10); rep.deduct_reputation(100); assert_eq!(rep.reputation_points, -5); - assert_eq!(rep.online_credits, 10); + assert_eq!(rep.online_blocks, 10); } } diff --git a/state-chain/pallets/cf-reputation/src/tests.rs b/state-chain/pallets/cf-reputation/src/tests.rs index d8649df196..040263392d 100644 --- a/state-chain/pallets/cf-reputation/src/tests.rs +++ b/state-chain/pallets/cf-reputation/src/tests.rs @@ -19,19 +19,11 @@ fn advance_by_block() { } // Move forward one heartbeat interval sending the heartbeat extrinsic for nodes -fn submit_heartbeat_and_move_forward_heartbeat_interval( +fn move_forward_hearbeat_interval_and_submit_heartbeat( node: ::AccountId, ) { - assert_ok!(ReputationPallet::heartbeat(RuntimeOrigin::signed(node))); advance_by_hearbeat_intervals(1); -} - -#[test] -fn submitting_heartbeat_for_heartbeat_block_interval_should_reward_reputation_points() { - new_test_ext().execute_with(|| { - ReputationPallet::heartbeat(RuntimeOrigin::signed(ALICE)).unwrap(); - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT,); - }); + assert_ok!(ReputationPallet::heartbeat(RuntimeOrigin::signed(node))); } #[test] @@ -51,20 +43,51 @@ fn offline_nodes_get_slashed_if_reputation_is_negative() { }); } +macro_rules! assert_reputation { + ( $id:expr, $rep:expr ) => { + assert_eq!( + reputation_points(&$id), + $rep, + "Expected reputation of {}, got {:?}", + $rep, + ReputationPallet::reputation(&$id) + ); + }; +} + #[test] -fn only_one_heartbeat_per_interval_earns_reputation() { +fn number_of_submissions_doesnt_affect_reputation_increase() { new_test_ext().execute_with(|| { + // Disable reporting to prevent reputation points from being deducted. + >::set_code_red(); + assert_reputation!(ALICE, 0); + + // Submit twice per block. + for _ in 0..HEARTBEAT_BLOCK_INTERVAL { + advance_by_block(); + ReputationPallet::heartbeat(RuntimeOrigin::signed(ALICE)).unwrap(); + ReputationPallet::heartbeat(RuntimeOrigin::signed(ALICE)).unwrap(); + } + assert_reputation!(ALICE, REPUTATION_PER_HEARTBEAT); + + // Submit every other block. + Pallet::::reset_reputation(&ALICE); + for i in 0..HEARTBEAT_BLOCK_INTERVAL { + advance_by_block(); + if i % 2 == 0 { + ReputationPallet::heartbeat(RuntimeOrigin::signed(ALICE)).unwrap(); + } + } ReputationPallet::heartbeat(RuntimeOrigin::signed(ALICE)).unwrap(); - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT,); - // submit again, then move forward - advance_by_block(); - submit_heartbeat_and_move_forward_heartbeat_interval(ALICE); - // no change in reputation, because we were on the same block, therefore we were in the same - // heartbeat block interval - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT,); - // we've moved forward a block interval, so now we should have the extra rep + assert_reputation!(ALICE, REPUTATION_PER_HEARTBEAT); + + // Submit after the heartbeat interval has elapsed. + Pallet::::reset_reputation(&ALICE); + for _ in 0..(HEARTBEAT_BLOCK_INTERVAL * 2) { + advance_by_block(); + } ReputationPallet::heartbeat(RuntimeOrigin::signed(ALICE)).unwrap(); - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT * 2,); + assert_reputation!(ALICE, REPUTATION_PER_HEARTBEAT); }); } @@ -82,6 +105,8 @@ fn update_last_heartbeat_each_submission() { #[test] fn updating_accrual_rate_should_affect_reputation_points() { new_test_ext().execute_with(|| { + // Disable reporting to prevent reputation points from being deducted. + >::set_code_red(); // Fails due to too high a reputation points assert_noop!( ReputationPallet::update_accrual_ratio( @@ -110,8 +135,8 @@ fn updating_accrual_rate_should_affect_reputation_points() { assert_eq!(ReputationPallet::accrual_ratio(), ACCRUAL_RATIO); - submit_heartbeat_and_move_forward_heartbeat_interval(ALICE); - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT); + move_forward_hearbeat_interval_and_submit_heartbeat(ALICE); + assert_reputation!(ALICE, REPUTATION_PER_HEARTBEAT); // Double the accrual rate. assert_ok!(ReputationPallet::update_accrual_ratio( @@ -120,8 +145,8 @@ fn updating_accrual_rate_should_affect_reputation_points() { ACCRUAL_RATIO.1, )); - submit_heartbeat_and_move_forward_heartbeat_interval(ALICE); - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT * 3); + move_forward_hearbeat_interval_and_submit_heartbeat(ALICE); + assert_reputation!(ALICE, REPUTATION_PER_HEARTBEAT * 3); // Halve the divisor, equivalent to double the initial rate. assert_ok!(ReputationPallet::update_accrual_ratio( @@ -130,8 +155,8 @@ fn updating_accrual_rate_should_affect_reputation_points() { ACCRUAL_RATIO.1 / 2, )); - submit_heartbeat_and_move_forward_heartbeat_interval(ALICE); - assert_eq!(reputation_points(&ALICE), REPUTATION_PER_HEARTBEAT * 5); + move_forward_hearbeat_interval_and_submit_heartbeat(ALICE); + assert_reputation!(ALICE, REPUTATION_PER_HEARTBEAT * 5); }); } @@ -149,7 +174,7 @@ fn reporting_any_offence_should_penalise_reputation_points_and_suspend() { let points_before = who.iter().map(reputation_points).collect::>(); ::report_many(offence, who); for (id, points) in who.iter().zip(points_before) { - assert_eq!(reputation_points(id), points - penalty.reputation,); + assert_reputation!(id, points - penalty.reputation); } assert_eq!( ReputationPallet::validators_suspended_for(&[offence]), @@ -270,7 +295,7 @@ fn dont_report_in_safe_mode() { reputation: crate::PalletSafeMode { reporting_enabled: false }, }); ReputationPallet::report_many(AllOffences::NotLockingYourComputer, &[marcello]); - assert_eq!(ReputationPallet::reputation(marcello).reputation_points, 0); + assert_reputation!(marcello, 0); MockRuntimeSafeMode::set_safe_mode(MockRuntimeSafeMode { reputation: crate::PalletSafeMode { reporting_enabled: true }, }); diff --git a/state-chain/runtime/src/chainflip/signer_nomination.rs b/state-chain/runtime/src/chainflip/signer_nomination.rs index a391640cb4..2f6ac4841b 100644 --- a/state-chain/runtime/src/chainflip/signer_nomination.rs +++ b/state-chain/runtime/src/chainflip/signer_nomination.rs @@ -65,15 +65,15 @@ fn eligible_authorities( /// excluded from being nominated. pub struct RandomSignerNomination; -impl cf_traits::SingleSignerNomination for RandomSignerNomination { - type SignerId = ::ValidatorId; +impl cf_traits::BroadcastNomination for RandomSignerNomination { + type BroadcasterId = ::ValidatorId; - fn nomination_with_seed( + fn nominate_broadcaster( seed: H, - exclude_ids: &[Self::SignerId], - ) -> Option { + exclude_ids: &[Self::BroadcasterId], + ) -> Option { let mut all_excludes = Reputation::validators_suspended_for(&[ - Offence::MissedAuthorshipSlot, + Offence::FailedToBroadcastTransaction, Offence::MissedHeartbeat, ]); all_excludes.extend(exclude_ids.iter().cloned()); @@ -99,9 +99,10 @@ impl cf_traits::ThresholdSignerNomination for RandomSignerNomination { eligible_authorities( epoch_index, &Reputation::validators_suspended_for(&[ - Offence::ParticipateSigningFailed, - Offence::MissedAuthorshipSlot, Offence::MissedHeartbeat, + Offence::ParticipateSigningFailed, + Offence::ParticipateKeygenFailed, + Offence::ParticipateKeyHandoverFailed, ]), ), ) diff --git a/state-chain/runtime/src/constants.rs b/state-chain/runtime/src/constants.rs index 4ff6af7dc4..039c0c08af 100644 --- a/state-chain/runtime/src/constants.rs +++ b/state-chain/runtime/src/constants.rs @@ -15,12 +15,6 @@ pub mod common { pub const MAX_AUTHORITIES: AuthorityCount = 150; - // Number of online credits required to get `ACCRUAL_REPUTATION_POINTS` of reputation - const ACCRUAL_ONLINE_CREDITS: u32 = 2500; - // Number of reputation points received for having `ACCRUAL_ONLINE_CREDITS` - const ACCRUAL_REPUTATION_POINTS: i32 = 1; - pub const ACCRUAL_RATIO: (i32, u32) = (ACCRUAL_REPUTATION_POINTS, ACCRUAL_ONLINE_CREDITS); - // ======= Keygen and signing ======= /// Maximum duration a ceremony stage can last diff --git a/state-chain/runtime/src/lib.rs b/state-chain/runtime/src/lib.rs index 3452885b4e..7b0068f22b 100644 --- a/state-chain/runtime/src/lib.rs +++ b/state-chain/runtime/src/lib.rs @@ -931,7 +931,6 @@ impl_runtime_apis! { balance: account_info.total(), bond: account_info.bond(), last_heartbeat: pallet_cf_reputation::LastHeartbeat::::get(account_id).unwrap_or(0), - online_credits: reputation_info.online_credits, reputation_points: reputation_info.reputation_points, keyholder_epochs: key_holder_epochs, is_current_authority, diff --git a/state-chain/runtime/src/runtime_apis.rs b/state-chain/runtime/src/runtime_apis.rs index 69fd5a98bb..57cb57e347 100644 --- a/state-chain/runtime/src/runtime_apis.rs +++ b/state-chain/runtime/src/runtime_apis.rs @@ -40,7 +40,6 @@ pub struct RuntimeApiAccountInfoV2 { pub balance: u128, pub bond: u128, pub last_heartbeat: u32, // can *maybe* remove this - check with Andrew - pub online_credits: u32, pub reputation_points: i32, pub keyholder_epochs: Vec, pub is_current_authority: bool, diff --git a/state-chain/traits/src/lib.rs b/state-chain/traits/src/lib.rs index 6462dc843c..cbc0bb330e 100644 --- a/state-chain/traits/src/lib.rs +++ b/state-chain/traits/src/lib.rs @@ -187,7 +187,6 @@ pub trait EpochTransitionHandler { fn on_expired_epoch(_expired: EpochIndex) {} } -/// Resetter for Reputation Points and Online Credits of a Validator pub trait ReputationResetter { type ValidatorId; @@ -330,17 +329,17 @@ pub trait Slashing { fn slash_balance(account_id: &Self::AccountId, amount: Percent); } -/// Can nominate a single account. -pub trait SingleSignerNomination { - /// The id type of signer - type SignerId; +/// Nominate a single account for transaction broadcasting. +pub trait BroadcastNomination { + /// The id type of the broadcaster. + type BroadcasterId; - /// Returns a random live signer, excluding particular provided signers. The seed value is used + /// Returns a random broadcaster id, excluding particular provided ids. The seed value is used /// as a source of randomness. Returns None if no signers are live. - fn nomination_with_seed( + fn nominate_broadcaster( seed: H, - exclude_ids: &[Self::SignerId], - ) -> Option; + exclude_ids: &[Self::BroadcasterId], + ) -> Option; } pub trait ThresholdSignerNomination { diff --git a/state-chain/traits/src/mocks/signer_nomination.rs b/state-chain/traits/src/mocks/signer_nomination.rs index bc832bb401..9c92e7d490 100644 --- a/state-chain/traits/src/mocks/signer_nomination.rs +++ b/state-chain/traits/src/mocks/signer_nomination.rs @@ -1,6 +1,6 @@ use sp_std::collections::btree_set::BTreeSet; -use crate::{EpochIndex, EpochInfo, SingleSignerNomination, ThresholdSignerNomination}; +use crate::{BroadcastNomination, EpochIndex, EpochInfo, ThresholdSignerNomination}; thread_local! { pub static THRESHOLD_NOMINEES: std::cell::RefCell>> = Default::default(); @@ -9,13 +9,13 @@ thread_local! { pub struct MockNominator; -impl SingleSignerNomination for MockNominator { - type SignerId = u64; +impl BroadcastNomination for MockNominator { + type BroadcasterId = u64; - fn nomination_with_seed( + fn nominate_broadcaster( _seed: S, - _exclude_ids: &[Self::SignerId], - ) -> Option { + _exclude_ids: &[Self::BroadcasterId], + ) -> Option { let next_nomination_index = LAST_NOMINATED_INDEX.with(|cell| { let mut last_nomination = cell.borrow_mut(); let next_nomination_index = @@ -58,7 +58,7 @@ impl MockNominator { } pub fn use_current_authorities_as_nominees< - E: EpochInfo::SignerId>, + E: EpochInfo::BroadcasterId>, >() { Self::set_nominees(Some(BTreeSet::from_iter(E::current_authorities()))); }