From 9b87e162df96e199236386fd3622c530e819c4be Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 12 Oct 2023 03:34:16 +0200 Subject: [PATCH 1/2] blockchain: include total_stake in Epoch Having cached total stake value in Epoch structure helps with rewards calculation where we need give validators proportional payouts for signing blocks. --- common/blockchain/src/candidates.rs | 17 +-- common/blockchain/src/epoch.rs | 157 ++++++++++++++++++++++------ common/blockchain/src/manager.rs | 20 ++-- common/blockchain/src/validators.rs | 2 +- 4 files changed, 140 insertions(+), 56 deletions(-) diff --git a/common/blockchain/src/candidates.rs b/common/blockchain/src/candidates.rs index 31f2795d..830916bd 100644 --- a/common/blockchain/src/candidates.rs +++ b/common/blockchain/src/candidates.rs @@ -99,26 +99,19 @@ impl Candidates { .fold(0, |sum, c| sum.checked_add(c.stake.get()).unwrap()) } - /// Returns top validators together with their total stake if changed since - /// last call. - pub fn maybe_get_head(&mut self) -> Option<(Vec>, u128)> { + /// Returns top validators if changed since last call. + pub fn maybe_get_head(&mut self) -> Option>> { if !self.changed { return None; } - let mut total: u128 = 0; let validators = self .candidates .iter() .take(self.max_validators()) - .map(|candidate| { - total = total.checked_add(candidate.stake.get())?; - Some(Validator::from(candidate)) - }) - .collect::>>() - .unwrap(); + .map(Validator::from) + .collect::>(); self.changed = false; - self.debug_verify_state(); - Some((validators, total)) + Some(validators) } /// Adds a new candidates or updates existing candidate’s stake. diff --git a/common/blockchain/src/epoch.rs b/common/blockchain/src/epoch.rs index 6b6355ab..a3c8185f 100644 --- a/common/blockchain/src/epoch.rs +++ b/common/blockchain/src/epoch.rs @@ -1,16 +1,16 @@ use alloc::vec::Vec; use core::num::NonZeroU128; -use crate::validators::{PubKey, Validator}; +use borsh::maybestd::io; + +use crate::validators::Validator; /// An epoch describing configuration applying to all blocks within an epoch. /// /// An epoch is identified by hash of the block it was introduced in. As such, /// epoch’s identifier is unknown until block which defines it in /// [`crate::block::Block::next_blok`] field is created. -#[derive( - Clone, Debug, PartialEq, Eq, borsh::BorshSerialize, borsh::BorshDeserialize, -)] +#[derive(Clone, Debug, PartialEq, Eq, borsh::BorshSerialize)] pub struct Epoch { /// Version of the structure. Used to support forward-compatibility. At /// the moment this is always zero. @@ -21,9 +21,22 @@ pub struct Epoch { /// Minimum stake to consider block signed. quorum_stake: NonZeroU128, + + /// Total stake. + #[borsh_skip] + total_stake: NonZeroU128, +} + +impl borsh::BorshDeserialize for Epoch { + fn deserialize_reader(reader: &mut R) -> io::Result { + let _ = crate::common::VersionZero::deserialize_reader(reader)?; + let (validators, quorum_stake) = <_>::deserialize_reader(reader)?; + Self::new(validators, quorum_stake) + .ok_or_else(|| io::ErrorKind::InvalidData.into()) + } } -impl Epoch { +impl Epoch { /// Creates a new epoch. /// /// Returns `None` if the epoch is invalid, i.e. if quorum stake is greater @@ -34,37 +47,32 @@ impl Epoch { validators: Vec>, quorum_stake: NonZeroU128, ) -> Option { - let version = crate::common::VersionZero; - let this = Self { version, validators, quorum_stake }; - Some(this).filter(Self::is_valid) + Self::new_with(validators, |_| quorum_stake) } - /// Creates a new epoch without checking whether it’s valid. - /// - /// It’s caller’s responsibility to guarantee that total stake of all - /// validators is no more than quorum stake. + /// Creates a new epoch with function determining quorum. /// - /// In debug builds panics if the result is an invalid epoch. - pub(crate) fn new_unchecked( + /// The callback function is invoked with the total stake of all the + /// validators and must return positive number no greater than the argument. + /// If the returned value is greater, the epoch would be invalid and this + /// constructor returns `None`. Also returns `None` when total stake is + /// zero. + pub fn new_with( validators: Vec>, - quorum_stake: NonZeroU128, - ) -> Self { - let version = crate::common::VersionZero; - let this = Self { version, validators, quorum_stake }; - debug_assert!(this.is_valid()); - this - } - - /// Checks whether the epoch is valid. - fn is_valid(&self) -> bool { - let mut left = self.quorum_stake.get(); - for validator in self.validators.iter() { - left = left.saturating_sub(validator.stake().get()); - if left == 0 { - return true; - } + quorum_stake: impl FnOnce(NonZeroU128) -> NonZeroU128, + ) -> Option { + let mut total: u128 = 0; + for validator in validators.iter() { + total = total.checked_add(validator.stake().get())?; + } + let total_stake = NonZeroU128::new(total)?; + let quorum_stake = quorum_stake(total_stake); + if quorum_stake <= total_stake { + let version = crate::common::VersionZero; + Some(Self { version, validators, quorum_stake, total_stake }) + } else { + None } - false } /// Returns list of all validators in the epoch. @@ -74,7 +82,10 @@ impl Epoch { pub fn quorum_stake(&self) -> NonZeroU128 { self.quorum_stake } /// Finds a validator by their public key. - pub fn validator(&self, pk: &PK) -> Option<&Validator> { + pub fn validator(&self, pk: &PK) -> Option<&Validator> + where + PK: Eq, + { self.validators.iter().find(|validator| validator.pubkey() == pk) } } @@ -94,7 +105,10 @@ impl Epoch { Validator::new(pk.into(), NonZeroU128::new(stake).unwrap()) }) .collect(); - Self::new(validators, NonZeroU128::new(total / 2 + 1).unwrap()).unwrap() + Self::new_with(validators, |total| { + NonZeroU128::new(total.get() / 2 + 1).unwrap() + }) + .unwrap() } } @@ -118,3 +132,80 @@ fn test_creation() { assert_eq!(Some(&validators[0]), epoch.validator(&MockPubKey(0))); assert_eq!(None, epoch.validator(&MockPubKey(2))); } + +#[test] +fn test_borsh_success() { + let epoch = Epoch::test(&[(0, 10), (1, 10)]); + let encoded = borsh::to_vec(&epoch).unwrap(); + #[rustfmt::skip] + assert_eq!(&[ + /* version: */ 0, + /* length: */ 2, 0, 0, 0, + /* v[0].version: */ 0, + /* v[0].pubkey: */ 0, 0, 0, 0, + /* v[0].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* v[1].version: */ 0, + /* v[1].pubkey: */ 1, 0, 0, 0, + /* v[1].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* quorum: */ 11, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + ], encoded.as_slice()); + + let got = borsh::BorshDeserialize::try_from_slice(encoded.as_slice()); + assert_eq!(epoch, got.unwrap()); +} + +#[test] +#[rustfmt::skip] +fn test_borsh_failures() { + fn test(bytes: &[u8]) { + use borsh::BorshDeserialize; + let got = Epoch::::try_from_slice(bytes); + got.unwrap_err(); + } + + // No validators + test(&[ + /* version: */ 0, + /* length: */ 0, 0, 0, 0, + /* quorum: */ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + ]); + + // Validator with no stake. + test(&[ + /* version: */ 0, + /* length: */ 2, 0, 0, 0, + /* v[0].version: */ 0, + /* v[0].pubkey: */ 0, 0, 0, 0, + /* v[0].stake: */ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* v[1].version: */ 0, + /* v[1].pubkey: */ 1, 0, 0, 0, + /* v[1].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* quorum: */ 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + ]); + + // Zero quorum + test(&[ + /* version: */ 0, + /* length: */ 2, 0, 0, 0, + /* v[0].version: */ 0, + /* v[0].pubkey: */ 0, 0, 0, 0, + /* v[0].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* v[1].version: */ 0, + /* v[1].pubkey: */ 1, 0, 0, 0, + /* v[1].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* quorum: */ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + ]); + + // Quorum over total + test(&[ + /* version: */ 0, + /* length: */ 2, 0, 0, 0, + /* v[0].version: */ 0, + /* v[0].pubkey: */ 0, 0, 0, 0, + /* v[0].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* v[1].version: */ 0, + /* v[1].pubkey: */ 1, 0, 0, 0, + /* v[1].stake: */ 10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + /* quorum: */ 21, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + ]); +} diff --git a/common/blockchain/src/manager.rs b/common/blockchain/src/manager.rs index a0139d7c..d50d499c 100644 --- a/common/blockchain/src/manager.rs +++ b/common/blockchain/src/manager.rs @@ -168,16 +168,16 @@ impl ChainManager { { return None; } - let (validators, total) = self.candidates.maybe_get_head()?; - // 1. We validate that genesis has a valid epoch (at least 1 stake). - // 2. We never allow fewer than config.min_validators candidates. - // 3. We never allow candidates with zero stake. - // Therefore, total should always be positive. - let total = NonZeroU128::new(total).unwrap(); - // SAFETY: anything_unsigned + 1 > 0 - let quorum = unsafe { NonZeroU128::new_unchecked(total.get() / 2 + 1) } - .clamp(self.config.min_quorum_stake, total); - Some(epoch::Epoch::new_unchecked(validators, quorum)) + epoch::Epoch::new_with(self.candidates.maybe_get_head()?, |total| { + // SAFETY: 1. ‘total / 2 ≥ 0’ thus ‘total / 2 + 1 > 0’. + // 2. ‘total / 2 <= u128::MAX / 2’ thus ‘total / 2 + 1 < u128::MAX’. + let quorum = + unsafe { NonZeroU128::new_unchecked(total.get() / 2 + 1) }; + // min_quorum_stake may be greater than total_stake so we’re not + // using .clamp to make sure we never return value higher than + // total_stake. + quorum.max(self.config.min_quorum_stake).min(total) + }) } /// Adds a signature to pending block. diff --git a/common/blockchain/src/validators.rs b/common/blockchain/src/validators.rs index 845cabc3..d6624159 100644 --- a/common/blockchain/src/validators.rs +++ b/common/blockchain/src/validators.rs @@ -45,7 +45,7 @@ pub struct Validator { stake: NonZeroU128, } -impl Validator { +impl Validator { pub fn new(pubkey: PK, stake: NonZeroU128) -> Self { Self { version: crate::common::VersionZero, pubkey, stake } } From f5d452d5336d17fdd6d3ebdd086ddebaa4c04ddb Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 12 Oct 2023 15:49:50 +0200 Subject: [PATCH 2/2] moar docs --- common/blockchain/src/epoch.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/blockchain/src/epoch.rs b/common/blockchain/src/epoch.rs index a3c8185f..97dae879 100644 --- a/common/blockchain/src/epoch.rs +++ b/common/blockchain/src/epoch.rs @@ -20,9 +20,15 @@ pub struct Epoch { validators: Vec>, /// Minimum stake to consider block signed. + /// + /// Always no more than `total_stake`. quorum_stake: NonZeroU128, /// Total stake. + /// + /// This is always `sum(v.stake for v in validators)`. + // We don’t serialise it because we calculate it when deserializing to make + // sure that it’s always a correct value. #[borsh_skip] total_stake: NonZeroU128, }