diff --git a/common/blockchain/Cargo.toml b/common/blockchain/Cargo.toml index b3233a9c..722adc9a 100644 --- a/common/blockchain/Cargo.toml +++ b/common/blockchain/Cargo.toml @@ -11,7 +11,7 @@ derive_more.workspace = true lib = { workspace = true, features = ["borsh"] } [dev-dependencies] -lib = { workspace = true, features = ["borsh", "test_utils"] } +lib = { workspace = true, features = ["test_utils"] } rand.workspace = true stdx.workspace = true diff --git a/common/blockchain/src/candidates.rs b/common/blockchain/src/candidates.rs index 830916bd..294c9117 100644 --- a/common/blockchain/src/candidates.rs +++ b/common/blockchain/src/candidates.rs @@ -99,32 +99,51 @@ impl Candidates { .fold(0, |sum, c| sum.checked_add(c.stake.get()).unwrap()) } - /// Returns top validators if changed since last call. - pub fn maybe_get_head(&mut self) -> Option>> { - if !self.changed { - return None; - } - let validators = self - .candidates - .iter() - .take(self.max_validators()) - .map(Validator::from) - .collect::>(); - self.changed = false; - Some(validators) + /// Returns top validators if changed since last time changed flag was + /// cleared. + /// + /// To clear changed flag, use [`Self::clear_changed_flag`]. + pub fn maybe_get_head(&self) -> Option>> { + self.changed.then(|| { + self.candidates + .iter() + .take(self.max_validators()) + .map(Validator::from) + .collect::>() + }) } + /// Clears the changed flag. + /// + /// Changed flag is set automatically whenever head of the candidates list + /// is modified (note that changes outside of the head of candidates list do + /// not affect the flag). + pub fn clear_changed_flag(&mut self) { self.changed = false; } + /// Adds a new candidates or updates existing candidate’s stake. + /// + /// If `stake` is zero, removes the candidate from the set. pub fn update( &mut self, cfg: &chain::Config, pubkey: PK, stake: u128, ) -> Result<(), UpdateCandidateError> { - let stake = NonZeroU128::new(stake) - .filter(|stake| *stake >= cfg.min_validator_stake) - .ok_or(UpdateCandidateError::NotEnoughValidatorStake)?; - let candidate = Candidate { pubkey, stake }; + match NonZeroU128::new(stake) { + None => self.do_remove(cfg, &pubkey), + Some(stake) if stake < cfg.min_validator_stake => { + Err(UpdateCandidateError::NotEnoughValidatorStake) + } + Some(stake) => self.do_update(cfg, Candidate { pubkey, stake }), + } + } + + /// Adds a new candidates or updates existing candidate’s stake. + fn do_update( + &mut self, + cfg: &chain::Config, + candidate: Candidate, + ) -> Result<(), UpdateCandidateError> { let old_pos = self.candidates.iter().position(|el| el.pubkey == candidate.pubkey); let mut new_pos = @@ -143,7 +162,7 @@ impl Candidates { } /// Removes an existing candidate. - pub fn remove( + fn do_remove( &mut self, cfg: &chain::Config, pubkey: &PK, diff --git a/common/blockchain/src/candidates/tests.rs b/common/blockchain/src/candidates/tests.rs index 7078781c..62eab225 100644 --- a/common/blockchain/src/candidates/tests.rs +++ b/common/blockchain/src/candidates/tests.rs @@ -121,23 +121,23 @@ fn test_candidates_0() { // Check minimum total stake and count are checked assert_eq!( Err(NotEnoughTotalStake), - candidates.remove(&cfg_with_min_total_stake(10), &pk('E')), + candidates.update(&cfg_with_min_total_stake(10), pk('E'), 0), ); assert_eq!( Err(NotEnoughValidators), - candidates.remove(&cfg_with_min_validators(5), &pk('E')), + candidates.update(&cfg_with_min_validators(5), pk('E'), 0), ); // Removal is idempotent for _ in 0..2 { - candidates.remove(&cfg_with_min_validators(2), &pk('E')).unwrap(); + candidates.update(&cfg_with_min_validators(2), pk('E'), 0).unwrap(); check([('D', 4), ('C', 3), ('B', 2), ('A', 1)], &candidates); } // Go below max_validators of candidates. - candidates.remove(&cfg_with_min_validators(1), &pk('C')).unwrap(); - candidates.remove(&cfg_with_min_validators(1), &pk('B')).unwrap(); - candidates.remove(&cfg_with_min_validators(1), &pk('A')).unwrap(); + candidates.update(&cfg_with_min_validators(1), pk('C'), 0).unwrap(); + candidates.update(&cfg_with_min_validators(1), pk('B'), 0).unwrap(); + candidates.update(&cfg_with_min_validators(1), pk('A'), 0).unwrap(); check([('D', 4)], &candidates); // Minimum validator stake is checked @@ -261,7 +261,7 @@ impl TestCtx { let count = self.candidates.candidates.len(); let head_stake = self.candidates.head_stake; - let res = self.candidates.remove(&self.config, &pubkey); + let res = self.candidates.update(&self.config, pubkey.clone(), 0); self.check(); if let Err(err) = res { @@ -375,16 +375,12 @@ impl TestCtx { ); } - /// Performs a random test. `data` must be a three-element slice. The - /// random test is determined from values in the slice. - fn test(&mut self, data: &[u8]) { + /// Performs a random test. `data` must be a two-element slice. The random + /// test is determined from values in the slice. + fn test(&mut self, pubkey: u8, stake: u8) { let old_state = self.candidates.clone(); - let pubkey = MockPubKey((data[0]).into()); - let op = if data[2] % 2 == 0 { - Ok(pubkey) - } else { - Err((pubkey, u128::from(data[1]))) - }; + let pubkey = MockPubKey(u32::from(pubkey)); + let stake = u128::from(stake); let this = self as *mut TestCtx; let res = std::panic::catch_unwind(|| { @@ -392,21 +388,17 @@ impl TestCtx { // self.candidates may be in inconsistent state. This is fine since // we’re panicking anyway. let this = unsafe { &mut *this }; - match &op { - Ok(pubkey) => this.test_remove(pubkey.clone()), - Err((pubkey, stake)) => { - this.test_update(pubkey.clone(), *stake) - } + match u128::from(stake) { + 0 => this.test_remove(pubkey), + _ => this.test_update(pubkey.clone(), stake), } }); if let Err(err) = res { std::eprintln!("{:?}", old_state); - match op { - Ok(pubkey) => std::eprintln!(" Remove {pubkey:?}"), - Err((pubkey, stake)) => { - std::eprintln!(" Update {pubkey:?} staking {stake}") - } + match stake { + 0 => std::eprintln!(" Remove {pubkey:?}"), + _ => std::eprintln!(" Update {pubkey:?} staking {stake}"), } std::eprintln!("{:?}", self.candidates); std::panic::resume_unwind(err); @@ -421,11 +413,11 @@ fn stress_test() { let mut rng = rand::thread_rng(); let mut ctx = TestCtx::new(&mut rng); let mut n = lib::test_utils::get_iteration_count(1); - let mut buf = [0u8; 3 * 1024]; + let mut buf = [0u8; 2 * 1024]; while n > 0 { rng.fill(&mut buf[..]); - for data in buf.chunks_exact(3).take(n) { - ctx.test(data); + for data in buf.chunks_exact(2).take(n) { + ctx.test(data[0], data[1]); } n = n.saturating_sub(1024); } diff --git a/common/blockchain/src/manager.rs b/common/blockchain/src/manager.rs index 155e21c8..ba15e7ad 100644 --- a/common/blockchain/src/manager.rs +++ b/common/blockchain/src/manager.rs @@ -32,9 +32,6 @@ pub struct ChainManager { /// Height at which current epoch was defined. epoch_height: HostHeight, - /// Current state root. - state_root: CryptoHash, - /// Set of validator candidates to consider for the next epoch. candidates: Candidates, } @@ -58,20 +55,25 @@ struct PendingBlock { } /// Provided genesis block is invalid. -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct BadGenesis; /// Error while generating a new block. -#[derive(derive_more::From)] +#[derive(Clone, Debug, PartialEq, Eq, derive_more::From)] pub enum GenerateError { /// Last block hasn’t been signed by enough validators yet. HasPendingBlock, /// Block isn’t old enough (see [`chain::config::min_block_length`] field). BlockTooYoung, + /// Block’s state root hasen’t changed and thus there’s no need to create + /// a new block. + UnchangedState, + /// An error while generating block. Inner(block::GenerateError), } /// Error while accepting a signature from a validator. +#[derive(Clone, Debug, PartialEq, Eq)] pub enum AddSignatureError { /// There’s no pending block. NoPendingBlock, @@ -92,7 +94,6 @@ impl ChainManager { let next_epoch = genesis.next_epoch.clone().ok_or(BadGenesis)?; let candidates = Candidates::new(config.max_validators, next_epoch.validators()); - let state_root = genesis.state_root.clone(); let epoch_height = genesis.host_height; Ok(Self { config, @@ -100,14 +101,17 @@ impl ChainManager { next_epoch, pending_block: None, epoch_height, - state_root, candidates, }) } - /// Sets value of state root to use in the next block. - pub fn update_state_root(&mut self, state_root: CryptoHash) { - self.state_root = state_root; + /// Returns the head of the chain as a `(finalised, block)` pair where + /// `finalised` indicates whether the block has been finalised. + pub fn head(&self) -> (bool, &block::Block) { + match self.pending_block { + None => (true, &self.block), + Some(ref pending) => (false, &pending.next_block), + } } /// Generates a new block and sets it as pending. @@ -115,10 +119,16 @@ impl ChainManager { /// Returns an error if there’s already a pending block. Previous pending /// block must first be signed by quorum of validators before next block is /// generated. + /// + /// Otherwise, returns whether the new block has been generated. Doesn’t + /// generate a block if the `state_root` is the same as the one in current + /// head of the blockchain and `force` is not set. pub fn generate_next( &mut self, host_height: HostHeight, host_timestamp: u64, + state_root: CryptoHash, + force: bool, ) -> Result<(), GenerateError> { if self.pending_block.is_some() { return Err(GenerateError::HasPendingBlock); @@ -131,10 +141,15 @@ impl ChainManager { } let next_epoch = self.maybe_generate_next_epoch(host_height); + if next_epoch.is_none() && !force && state_root == self.block.state_root + { + return Err(GenerateError::UnchangedState); + } + let next_block = self.block.generate_next( host_height, host_timestamp, - self.state_root.clone(), + state_root, next_epoch, )?; self.pending_block = Some(PendingBlock { @@ -143,6 +158,7 @@ impl ChainManager { signers: Set::new(), signing_stake: 0, }); + self.candidates.clear_changed_flag(); Ok(()) } @@ -220,11 +236,15 @@ impl ChainManager { Ok(true) } - /// Adds a new validator candidate or updates existing candidate’s stake. + /// Updates validator candidate’s stake. /// - /// Reducing candidates stake may fail if that would result in quorum or - /// total stake among the top `self.config.max_validators` to drop below - /// limits configured in `self.config`. + /// If `stake` is zero, removes the candidate if it exists on the list. + /// Otherwise, updates stake of an existing candidate or adds a new one. + /// + /// Note that removing a candidate or reducing existing candidate’s stake + /// may fail if that would result in quorum or total stake among the top + /// `self.config.max_validators` to drop below limits configured in + /// `self.config`. pub fn update_candidate( &mut self, pubkey: PK, @@ -232,18 +252,134 @@ impl ChainManager { ) -> Result<(), UpdateCandidateError> { self.candidates.update(&self.config, pubkey, stake) } +} - /// Removes an existing validator candidate. - /// - /// Note that removing a candidate may fail if the result candidate set - /// would no longer satisfy minimums in the chain configuration. See also - /// [`Self::update_candidate`]. - /// - /// Does nothing if the candidate is not found. - pub fn remove_candidate( - &mut self, - pubkey: &PK, - ) -> Result<(), UpdateCandidateError> { - self.candidates.remove(&self.config, pubkey) +#[test] +fn test_generate() { + use crate::validators::MockPubKey; + + let epoch = epoch::Epoch::test(&[(1, 2), (2, 2), (3, 2)]); + assert_eq!(4, epoch.quorum_stake().get()); + + let ali = epoch.validators()[0].clone(); + let bob = epoch.validators()[1].clone(); + let eve = epoch.validators()[2].clone(); + + let genesis = block::Block::generate_genesis( + 1.into(), + 1.into(), + 1, + CryptoHash::default(), + epoch, + ) + .unwrap(); + let config = chain::Config { + min_validators: core::num::NonZeroU16::MIN, + max_validators: core::num::NonZeroU16::new(3).unwrap(), + min_validator_stake: core::num::NonZeroU128::MIN, + min_total_stake: core::num::NonZeroU128::MIN, + min_quorum_stake: core::num::NonZeroU128::MIN, + min_block_length: 4.into(), + min_epoch_length: 8.into(), + }; + let mut mgr = ChainManager::new(config, genesis).unwrap(); + + // min_block_length not reached + assert_eq!( + Err(GenerateError::BlockTooYoung), + mgr.generate_next(4.into(), 2, CryptoHash::default(), false) + ); + // No change to the state so no need for a new block. + assert_eq!( + Err(GenerateError::UnchangedState), + mgr.generate_next(5.into(), 2, CryptoHash::default(), false) + ); + // Inner error. + assert_eq!( + Err(GenerateError::Inner(block::GenerateError::BadHostTimestamp)), + mgr.generate_next(5.into(), 1, CryptoHash::test(1), false) + ); + // Force create even if state hasn’t changed. + mgr.generate_next(5.into(), 2, CryptoHash::default(), true).unwrap(); + + fn sign_head( + mgr: &mut ChainManager, + validator: &crate::validators::Validator, + ) -> Result { + let signature = mgr.head().1.sign(&validator.pubkey().make_signer()); + mgr.add_signature(validator.pubkey().clone(), &signature) } + + // The head hasn’t been fully signed yet. + assert_eq!( + Err(GenerateError::HasPendingBlock), + mgr.generate_next(10.into(), 3, CryptoHash::test(2), false) + ); + + assert_eq!(Ok(false), sign_head(&mut mgr, &ali)); + assert_eq!( + Err(GenerateError::HasPendingBlock), + mgr.generate_next(10.into(), 3, CryptoHash::test(2), false) + ); + assert_eq!(Ok(false), sign_head(&mut mgr, &ali)); + assert_eq!( + Err(GenerateError::HasPendingBlock), + mgr.generate_next(10.into(), 3, CryptoHash::test(2), false) + ); + + // Signatures are verified + let pubkey = MockPubKey(42); + let signature = mgr.head().1.sign(&pubkey.make_signer()); + assert_eq!( + Err(AddSignatureError::BadValidator), + mgr.add_signature(pubkey, &signature) + ); + assert_eq!( + Err(AddSignatureError::BadSignature), + mgr.add_signature(bob.pubkey().clone(), &signature) + ); + + assert_eq!( + Err(GenerateError::HasPendingBlock), + mgr.generate_next(10.into(), 3, CryptoHash::test(2), false) + ); + + + assert_eq!(Ok(true), sign_head(&mut mgr, &bob)); + mgr.generate_next(10.into(), 3, CryptoHash::test(2), false).unwrap(); + + assert_eq!(Ok(false), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(true), sign_head(&mut mgr, &bob)); + + // State hasn’t changed, no need for new block. However, changing epoch can + // trigger new block. + assert_eq!( + Err(GenerateError::UnchangedState), + mgr.generate_next(15.into(), 4, CryptoHash::test(2), false) + ); + mgr.update_candidate(*eve.pubkey(), 1).unwrap(); + mgr.generate_next(15.into(), 4, CryptoHash::test(2), false).unwrap(); + assert_eq!(Ok(false), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(true), sign_head(&mut mgr, &bob)); + + // Epoch has minimum length. Even if the head of candidates changes but not + // enough host blockchain passed, the epoch won’t be changed. + mgr.update_candidate(*eve.pubkey(), 2).unwrap(); + assert_eq!( + Err(GenerateError::UnchangedState), + mgr.generate_next(20.into(), 5, CryptoHash::test(2), false) + ); + mgr.generate_next(30.into(), 5, CryptoHash::test(2), false).unwrap(); + assert_eq!(Ok(false), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(true), sign_head(&mut mgr, &bob)); + + // Lastly, adding candidates past the head (i.e. in a way which wouldn’t + // affect the epoch) doesn’t change the state. + mgr.update_candidate(MockPubKey(4), 1).unwrap(); + assert_eq!( + Err(GenerateError::UnchangedState), + mgr.generate_next(40.into(), 5, CryptoHash::test(2), false) + ); + mgr.update_candidate(*eve.pubkey(), 0).unwrap(); + mgr.generate_next(40.into(), 6, CryptoHash::test(2), false).unwrap(); } diff --git a/common/blockchain/src/validators.rs b/common/blockchain/src/validators.rs index c67310de..80f18d70 100644 --- a/common/blockchain/src/validators.rs +++ b/common/blockchain/src/validators.rs @@ -68,6 +68,10 @@ pub(crate) mod test_utils { )] pub struct MockPubKey(pub u32); + impl MockPubKey { + pub fn make_signer(&self) -> MockSigner { MockSigner(*self) } + } + /// A mock implementation of a Signer. Offers no security; intended for /// tests only. #[derive(Clone, Copy, PartialEq, Eq)]