From 0b516c2707c8b8e99fb85113175e21f31b9f3b05 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Mon, 13 Nov 2023 10:24:31 +0100 Subject: [PATCH] blockchain: extend return type of ChainManager::add_signature Add return values to distinguish a valid new signature and a valid duplicate signature. This is important to be able for the caller to determine whether a new signature has indeed been introduced or whether the signature submission can be ignored. --- common/blockchain/src/manager.rs | 58 +++++++++++++++++++------------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/common/blockchain/src/manager.rs b/common/blockchain/src/manager.rs index 6cd6d2e0..e2be70c4 100644 --- a/common/blockchain/src/manager.rs +++ b/common/blockchain/src/manager.rs @@ -81,6 +81,24 @@ pub enum AddSignatureError { BadValidator, } +/// Result of adding a signature to the pending block. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum AddSignatureEffect { + /// New signature has been accepted but quorum hasn’t been reached yet. + NoQuorumYet, + /// New signature has been accepted and quorum for the pending block has + /// been reached. + GotQuorum, + /// The signature has already been accepted previously. + Duplicate, +} + +impl AddSignatureEffect { + pub fn got_new_signature(self) -> bool { self != Self::Duplicate } + pub fn got_quorum(self) -> bool { self == Self::GotQuorum } +} + + impl ChainManager { pub fn new( config: crate::Config, @@ -116,13 +134,10 @@ impl ChainManager { /// Generates a new block and sets it as pending. /// - /// Returns an error if there’s already a pending block. Previous pending + /// 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. + /// generated) or conditions for creating a new block haven’t been met + /// (current block needs to be old enough, state needs to change etc.). pub fn generate_next( &mut self, host_height: crate::HostHeight, @@ -194,15 +209,12 @@ impl ChainManager { } /// Adds a signature to pending block. - /// - /// Returns `true` if quorum has been reached and the pending block has - /// graduated to the current block. pub fn add_signature( &mut self, pubkey: PK, signature: &PK::Signature, verifier: &impl crate::Verifier, - ) -> Result { + ) -> Result { let pending = self .pending_block .as_mut() @@ -218,12 +230,12 @@ impl ChainManager { } if !pending.signers.insert(pubkey) { - return Ok(false); + return Ok(AddSignatureEffect::Duplicate); } pending.signing_stake += validator_stake; if pending.signing_stake < self.next_epoch.quorum_stake().get() { - return Ok(false); + return Ok(AddSignatureEffect::NoQuorumYet); } self.block = self.pending_block.take().unwrap().next_block; @@ -231,7 +243,7 @@ impl ChainManager { self.next_epoch = epoch.clone(); self.epoch_height = self.block.host_height; } - Ok(true) + Ok(AddSignatureEffect::GotQuorum) } /// Updates validator candidate’s stake. @@ -305,7 +317,7 @@ fn test_generate() { fn sign_head( mgr: &mut ChainManager, validator: &crate::validators::Validator, - ) -> Result { + ) -> Result { let signature = mgr.head().1.sign(&validator.pubkey().make_signer()); mgr.add_signature(validator.pubkey().clone(), &signature, &()) } @@ -316,12 +328,12 @@ fn test_generate() { mgr.generate_next(10.into(), 3, CryptoHash::test(2), false) ); - assert_eq!(Ok(false), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(AddSignatureEffect::NoQuorumYet), 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!(Ok(AddSignatureEffect::Duplicate), sign_head(&mut mgr, &ali)); assert_eq!( Err(GenerateError::HasPendingBlock), mgr.generate_next(10.into(), 3, CryptoHash::test(2), false) @@ -345,11 +357,11 @@ fn test_generate() { ); - assert_eq!(Ok(true), sign_head(&mut mgr, &bob)); + assert_eq!(Ok(AddSignatureEffect::GotQuorum), 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)); + assert_eq!(Ok(AddSignatureEffect::NoQuorumYet), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(AddSignatureEffect::GotQuorum), sign_head(&mut mgr, &bob)); // State hasn’t changed, no need for new block. However, changing epoch can // trigger new block. @@ -359,8 +371,8 @@ fn test_generate() { ); 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)); + assert_eq!(Ok(AddSignatureEffect::NoQuorumYet), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(AddSignatureEffect::GotQuorum), 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. @@ -370,8 +382,8 @@ fn test_generate() { 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)); + assert_eq!(Ok(AddSignatureEffect::NoQuorumYet), sign_head(&mut mgr, &ali)); + assert_eq!(Ok(AddSignatureEffect::GotQuorum), 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.