From 22ed8b389a622b0286a486b84b862a1a8dff24ab Mon Sep 17 00:00:00 2001 From: david Date: Mon, 24 Jan 2022 13:56:25 +0100 Subject: [PATCH 1/3] improve rounds selection calculation --- client/beefy/src/lib.rs | 10 +-- client/beefy/src/worker.rs | 145 +++++++++++++++++------------------- primitives/beefy/src/lib.rs | 4 +- 3 files changed, 73 insertions(+), 86 deletions(-) diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 9b2bf383df8ef..273ee569b0b7e 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -24,7 +24,7 @@ use prometheus::Registry; use sc_client_api::{Backend, BlockchainEvents, Finalizer}; use sc_network_gossip::{GossipEngine, Network as GossipNetwork}; -use sp_api::ProvideRuntimeApi; +use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_blockchain::HeaderBackend; use sp_keystore::SyncCryptoStorePtr; use sp_runtime::traits::Block; @@ -111,7 +111,7 @@ where B: Block, BE: Backend, C: Client, - C::Api: BeefyApi, + C::Api: BeefyApi>, N: GossipNetwork + Clone + Send + 'static, { /// BEEFY client @@ -126,8 +126,6 @@ where pub signed_commitment_sender: BeefySignedCommitmentSender, /// BEEFY best block sender pub beefy_best_block_sender: BeefyBestBlockSender, - /// Minimal delta between blocks, BEEFY should vote for - pub min_block_delta: u32, /// Prometheus metric registry pub prometheus_registry: Option, /// Chain specific GRANDPA protocol name. See [`beefy_protocol_name::standard_name`]. @@ -142,7 +140,7 @@ where B: Block, BE: Backend, C: Client, - C::Api: BeefyApi, + C::Api: BeefyApi>, N: GossipNetwork + Clone + Send + 'static, { let BeefyParams { @@ -152,7 +150,6 @@ where network, signed_commitment_sender, beefy_best_block_sender, - min_block_delta, prometheus_registry, protocol_name, } = beefy_params; @@ -182,7 +179,6 @@ where beefy_best_block_sender, gossip_engine, gossip_validator, - min_block_delta, metrics, }; diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 0c7d8d4ffdc9c..08334c1ce4a94 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -61,7 +61,6 @@ where pub beefy_best_block_sender: BeefyBestBlockSender, pub gossip_engine: GossipEngine, pub gossip_validator: Arc>, - pub min_block_delta: u32, pub metrics: Option, } @@ -78,8 +77,6 @@ where signed_commitment_sender: BeefySignedCommitmentSender, gossip_engine: Arc>>, gossip_validator: Arc>, - /// Min delta in block numbers between two blocks, BEEFY should vote on - min_block_delta: u32, metrics: Option, rounds: Option>>, finality_notifications: FinalityNotifications, @@ -100,7 +97,7 @@ where B: Block + Codec, BE: Backend, C: Client, - C::Api: BeefyApi, + C::Api: BeefyApi>, { /// Return a new BEEFY worker instance. /// @@ -117,7 +114,6 @@ where beefy_best_block_sender, gossip_engine, gossip_validator, - min_block_delta, metrics, } = worker_params; @@ -128,7 +124,6 @@ where signed_commitment_sender, gossip_engine: Arc::new(Mutex::new(gossip_engine)), gossip_validator, - min_block_delta, metrics, rounds: None, finality_notifications: client.finality_notification_stream(), @@ -146,10 +141,20 @@ where B: Block, BE: Backend, C: Client, - C::Api: BeefyApi, + C::Api: BeefyApi>, { /// Return `true`, if we should vote on block `number` - fn should_vote_on(&self, number: NumberFor) -> bool { + fn should_vote_on(&self, header: &B::Header) -> bool { + let number = *header.number(); + let at = BlockId::hash(header.hash()); + let session_boundary = self.client.runtime_api().get_session_boundary(&at).ok(); + let session_start = if let Some(session_boundary) = session_boundary { + session_boundary + } else { + debug!(target: "beefy", "🥩 Could not retrieve session boundary- won't vote for: {:?}", number); + return false + }; + let best_beefy_block = if let Some(block) = self.best_beefy_block { block } else { @@ -157,7 +162,7 @@ where return false }; - let target = vote_target(self.best_grandpa_block, best_beefy_block, self.min_block_delta); + let target = vote_target(self.best_grandpa_block, best_beefy_block, session_start); trace!(target: "beefy", "🥩 should_vote_on: #{:?}, next_block_to_vote_on: #{:?}", number, target); @@ -258,7 +263,7 @@ where } } - if self.should_vote_on(*notification.header.number()) { + if self.should_vote_on(¬ification.header) { let (validators, validator_set_id) = if let Some(rounds) = &self.rounds { (rounds.validators(), rounds.validator_set_id()) } else { @@ -461,13 +466,18 @@ where } /// Calculate next block number to vote on -fn vote_target(best_grandpa: N, best_beefy: N, min_delta: u32) -> N +fn vote_target(best_grandpa: N, best_beefy: N, session_start: N) -> N where N: AtLeast32Bit + Copy + Debug, { + // if the session_start as a mandatory block does not have a beefy justification yet, we vote on + // it + let m: u32 = if session_start <= best_beefy { 1 } else { 0 }; + let diff = best_grandpa.saturating_sub(best_beefy); - let diff = diff.saturated_into::(); - let target = best_beefy + min_delta.max(diff.next_power_of_two()).into(); + let diff = diff.saturated_into::() / 2; + let target = (session_start * (1u32 - m).into()) + + ((best_beefy + diff.next_power_of_two().into()) * m.into()); trace!( target: "beefy", @@ -485,90 +495,69 @@ mod tests { use super::vote_target; #[test] - fn vote_on_min_block_delta() { - let t = vote_target(1u32, 0, 4); - assert_eq!(4, t); - let t = vote_target(2u32, 0, 4); - assert_eq!(4, t); - let t = vote_target(3u32, 0, 4); - assert_eq!(4, t); - let t = vote_target(4u32, 0, 4); - assert_eq!(4, t); - - let t = vote_target(4u32, 4, 4); - assert_eq!(8, t); - - let t = vote_target(10u32, 10, 4); - assert_eq!(14, t); - let t = vote_target(11u32, 10, 4); - assert_eq!(14, t); - let t = vote_target(12u32, 10, 4); - assert_eq!(14, t); - let t = vote_target(13u32, 10, 4); - assert_eq!(14, t); - - let t = vote_target(10u32, 10, 8); - assert_eq!(18, t); - let t = vote_target(11u32, 10, 8); - assert_eq!(18, t); - let t = vote_target(12u32, 10, 8); - assert_eq!(18, t); - let t = vote_target(13u32, 10, 8); - assert_eq!(18, t); + fn vote_on_mandatory_block() { + let t = vote_target(1008u32, 1000, 1001); + assert_eq!(1001, t); + + let t = vote_target(1016u32, 1000, 1008); + assert_eq!(1008, t); + + let t = vote_target(105u32, 100, 101); + assert_eq!(101, t); } #[test] fn vote_on_power_of_two() { - let t = vote_target(1008u32, 1000, 4); + let t = vote_target(1008u32, 1000, 999); + assert_eq!(1004, t); + + let t = vote_target(1016u32, 1000, 999); assert_eq!(1008, t); - let t = vote_target(1016u32, 1000, 4); + let t = vote_target(1032u32, 1000, 999); assert_eq!(1016, t); - let t = vote_target(1032u32, 1000, 4); + let t = vote_target(1064u32, 1000, 999); assert_eq!(1032, t); - let t = vote_target(1064u32, 1000, 4); + let t = vote_target(1128u32, 1000, 999); assert_eq!(1064, t); - let t = vote_target(1128u32, 1000, 4); + let t = vote_target(1256u32, 1000, 999); assert_eq!(1128, t); - let t = vote_target(1256u32, 1000, 4); + let t = vote_target(1512u32, 1000, 999); assert_eq!(1256, t); - let t = vote_target(1512u32, 1000, 4); - assert_eq!(1512, t); - - let t = vote_target(1024u32, 0, 4); - assert_eq!(1024, t); + let t = vote_target(1024u32, 0, 0); + assert_eq!(512, t); } #[test] fn vote_on_target_block() { - let t = vote_target(1008u32, 1002, 4); - assert_eq!(1010, t); - let t = vote_target(1010u32, 1002, 4); - assert_eq!(1010, t); - - let t = vote_target(1016u32, 1006, 4); - assert_eq!(1022, t); - let t = vote_target(1022u32, 1006, 4); - assert_eq!(1022, t); - - let t = vote_target(1032u32, 1012, 4); - assert_eq!(1044, t); - let t = vote_target(1044u32, 1012, 4); - assert_eq!(1044, t); - - let t = vote_target(1064u32, 1014, 4); - assert_eq!(1078, t); - let t = vote_target(1078u32, 1014, 4); - assert_eq!(1078, t); - - let t = vote_target(1128u32, 1008, 4); - assert_eq!(1136, t); - let t = vote_target(1136u32, 1008, 4); - assert_eq!(1136, t); + let t = vote_target(1008u32, 1002, 1001); + assert_eq!(1006, t); + let t = vote_target(1010u32, 1002, 1001); + assert_eq!(1006, t); + + let t = vote_target(1016u32, 1006, 1004); + assert_eq!(1014, t); + let t = vote_target(1022u32, 1006, 1004); + assert_eq!(1014, t); + + let t = vote_target(1032u32, 1012, 1010); + assert_eq!(1028, t); + let t = vote_target(1044u32, 1012, 1010); + assert_eq!(1028, t); + + let t = vote_target(1064u32, 1014, 1012); + assert_eq!(1046, t); + let t = vote_target(1078u32, 1014, 1012); + assert_eq!(1046, t); + + let t = vote_target(1128u32, 1008, 1004); + assert_eq!(1072, t); + let t = vote_target(1136u32, 1008, 1004); + assert_eq!(1072, t); } } diff --git a/primitives/beefy/src/lib.rs b/primitives/beefy/src/lib.rs index 8dbdd66f3559b..7ce07b75997e8 100644 --- a/primitives/beefy/src/lib.rs +++ b/primitives/beefy/src/lib.rs @@ -156,10 +156,12 @@ pub struct VoteMessage { sp_api::decl_runtime_apis! { /// API necessary for BEEFY voters. - pub trait BeefyApi + pub trait BeefyApi { /// Return the current active BEEFY validator set fn validator_set() -> Option>; + /// Returns the block number at which the current session began + fn get_session_boundary() -> BlockNumber; } } From 4d9209543b0904981dfe9877852729c9a406bdcc Mon Sep 17 00:00:00 2001 From: david Date: Tue, 25 Jan 2022 15:33:31 +0100 Subject: [PATCH 2/3] restore min block delta --- client/beefy/src/lib.rs | 4 +++ client/beefy/src/worker.rs | 66 ++++++++++++++++++++++---------------- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 273ee569b0b7e..56c9c360d3d13 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -126,6 +126,8 @@ where pub signed_commitment_sender: BeefySignedCommitmentSender, /// BEEFY best block sender pub beefy_best_block_sender: BeefyBestBlockSender, + /// Minimal delta between blocks, BEEFY should vote for + pub min_block_delta: u32, /// Prometheus metric registry pub prometheus_registry: Option, /// Chain specific GRANDPA protocol name. See [`beefy_protocol_name::standard_name`]. @@ -150,6 +152,7 @@ where network, signed_commitment_sender, beefy_best_block_sender, + min_block_delta, prometheus_registry, protocol_name, } = beefy_params; @@ -179,6 +182,7 @@ where beefy_best_block_sender, gossip_engine, gossip_validator, + min_block_delta, metrics, }; diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 08334c1ce4a94..30f9105b892a7 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -61,6 +61,7 @@ where pub beefy_best_block_sender: BeefyBestBlockSender, pub gossip_engine: GossipEngine, pub gossip_validator: Arc>, + pub min_block_delta: u32, pub metrics: Option, } @@ -77,6 +78,8 @@ where signed_commitment_sender: BeefySignedCommitmentSender, gossip_engine: Arc>>, gossip_validator: Arc>, + /// Minimal delta between blocks, BEEFY should vote for + min_block_delta: u32, metrics: Option, rounds: Option>>, finality_notifications: FinalityNotifications, @@ -114,6 +117,7 @@ where beefy_best_block_sender, gossip_engine, gossip_validator, + min_block_delta, metrics, } = worker_params; @@ -124,6 +128,7 @@ where signed_commitment_sender, gossip_engine: Arc::new(Mutex::new(gossip_engine)), gossip_validator, + min_block_delta, metrics, rounds: None, finality_notifications: client.finality_notification_stream(), @@ -143,7 +148,7 @@ where C: Client, C::Api: BeefyApi>, { - /// Return `true`, if we should vote on block `number` + /// Return `true`, if we should vote on block `header` fn should_vote_on(&self, header: &B::Header) -> bool { let number = *header.number(); let at = BlockId::hash(header.hash()); @@ -151,7 +156,7 @@ where let session_start = if let Some(session_boundary) = session_boundary { session_boundary } else { - debug!(target: "beefy", "🥩 Could not retrieve session boundary- won't vote for: {:?}", number); + warn!(target: "beefy", "🥩 Could not retrieve session boundary- won't vote for: {:?}", number); return false }; @@ -162,7 +167,12 @@ where return false }; - let target = vote_target(self.best_grandpa_block, best_beefy_block, session_start); + let target = vote_target( + self.best_grandpa_block, + best_beefy_block, + session_start, + self.min_block_delta, + ); trace!(target: "beefy", "🥩 should_vote_on: #{:?}, next_block_to_vote_on: #{:?}", number, target); @@ -466,18 +476,18 @@ where } /// Calculate next block number to vote on -fn vote_target(best_grandpa: N, best_beefy: N, session_start: N) -> N +fn vote_target(best_grandpa: N, best_beefy: N, session_start: N, min_delta: u32) -> N where N: AtLeast32Bit + Copy + Debug, { - // if the session_start as a mandatory block does not have a beefy justification yet, we vote on - // it + // if the session_start as a mandatory block does not have a beefy justification yet, + // we vote on it let m: u32 = if session_start <= best_beefy { 1 } else { 0 }; let diff = best_grandpa.saturating_sub(best_beefy); let diff = diff.saturated_into::() / 2; let target = (session_start * (1u32 - m).into()) + - ((best_beefy + diff.next_power_of_two().into()) * m.into()); + ((best_beefy + min_delta.max(diff.next_power_of_two().into()).into()) * m.into()); trace!( target: "beefy", @@ -496,68 +506,68 @@ mod tests { #[test] fn vote_on_mandatory_block() { - let t = vote_target(1008u32, 1000, 1001); + let t = vote_target(1008u32, 1000, 1001, 4); assert_eq!(1001, t); - let t = vote_target(1016u32, 1000, 1008); + let t = vote_target(1016u32, 1000, 1008, 4); assert_eq!(1008, t); - let t = vote_target(105u32, 100, 101); + let t = vote_target(105u32, 100, 101, 4); assert_eq!(101, t); } #[test] fn vote_on_power_of_two() { - let t = vote_target(1008u32, 1000, 999); + let t = vote_target(1008u32, 1000, 999, 4); assert_eq!(1004, t); - let t = vote_target(1016u32, 1000, 999); + let t = vote_target(1016u32, 1000, 999, 4); assert_eq!(1008, t); - let t = vote_target(1032u32, 1000, 999); + let t = vote_target(1032u32, 1000, 999, 4); assert_eq!(1016, t); - let t = vote_target(1064u32, 1000, 999); + let t = vote_target(1064u32, 1000, 999, 4); assert_eq!(1032, t); - let t = vote_target(1128u32, 1000, 999); + let t = vote_target(1128u32, 1000, 999, 4); assert_eq!(1064, t); - let t = vote_target(1256u32, 1000, 999); + let t = vote_target(1256u32, 1000, 999, 4); assert_eq!(1128, t); - let t = vote_target(1512u32, 1000, 999); + let t = vote_target(1512u32, 1000, 999, 4); assert_eq!(1256, t); - let t = vote_target(1024u32, 0, 0); + let t = vote_target(1024u32, 0, 0, 4); assert_eq!(512, t); } #[test] fn vote_on_target_block() { - let t = vote_target(1008u32, 1002, 1001); + let t = vote_target(1008u32, 1002, 1001, 4); assert_eq!(1006, t); - let t = vote_target(1010u32, 1002, 1001); + let t = vote_target(1010u32, 1002, 1001, 4); assert_eq!(1006, t); - let t = vote_target(1016u32, 1006, 1004); + let t = vote_target(1016u32, 1006, 1004, 4); assert_eq!(1014, t); - let t = vote_target(1022u32, 1006, 1004); + let t = vote_target(1022u32, 1006, 1004, 4); assert_eq!(1014, t); - let t = vote_target(1032u32, 1012, 1010); + let t = vote_target(1032u32, 1012, 1010, 4); assert_eq!(1028, t); - let t = vote_target(1044u32, 1012, 1010); + let t = vote_target(1044u32, 1012, 1010, 4); assert_eq!(1028, t); - let t = vote_target(1064u32, 1014, 1012); + let t = vote_target(1064u32, 1014, 1012, 4); assert_eq!(1046, t); - let t = vote_target(1078u32, 1014, 1012); + let t = vote_target(1078u32, 1014, 1012, 4); assert_eq!(1046, t); - let t = vote_target(1128u32, 1008, 1004); + let t = vote_target(1128u32, 1008, 1004, 4); assert_eq!(1072, t); - let t = vote_target(1136u32, 1008, 1004); + let t = vote_target(1136u32, 1008, 1004, 4); assert_eq!(1072, t); } } From 83f2d41d85fe6a00f8677cf4ec5ba8fbb83a136d Mon Sep 17 00:00:00 2001 From: david Date: Tue, 25 Jan 2022 15:35:25 +0100 Subject: [PATCH 3/3] update comment --- client/beefy/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 30f9105b892a7..c574d5ba1f892 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -78,7 +78,7 @@ where signed_commitment_sender: BeefySignedCommitmentSender, gossip_engine: Arc>>, gossip_validator: Arc>, - /// Minimal delta between blocks, BEEFY should vote for + /// Min delta in block numbers between two blocks, BEEFY should vote on min_block_delta: u32, metrics: Option, rounds: Option>>,