pallet-beefy: ensure mandatory block once per session (#11269)

* pallet-beefy: ensure mandatory block once per session

Signed-off-by: acatangiu <adrian@parity.io>

* pallet-beefy: fix tests with auth changes every session

Signed-off-by: acatangiu <adrian@parity.io>

* Apply suggestions from code review

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

* beefy: fix incorrect skip session metric on node restart

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

substrate/frame/beefy/src/lib.rs

@@ -105,10 +105,6 @@ impl<T: Config> Pallet<T> {
 	}
 
 	fn change_authorities(new: Vec<T::BeefyId>, queued: Vec<T::BeefyId>) {
-		// Always issue a change if `session` says that the validators have changed.
-		// Even if their session keys are the same as before, the underlying economic
-		// identities have changed. Furthermore, the digest below is used to signal
-		// BEEFY mandatory blocks.
 		<Authorities<T>>::put(&new);
 
 		let next_id = Self::validator_set_id() + 1u64;
@@ -153,16 +149,16 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
 		Self::initialize_authorities(&authorities);
 	}
 
-	fn on_new_session<'a, I: 'a>(changed: bool, validators: I, queued_validators: I)
+	fn on_new_session<'a, I: 'a>(_changed: bool, validators: I, queued_validators: I)
 	where
 		I: Iterator<Item = (&'a T::AccountId, T::BeefyId)>,
 	{
-		if changed {
-			let next_authorities = validators.map(|(_, k)| k).collect::<Vec<_>>();
-			let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::<Vec<_>>();
+		let next_authorities = validators.map(|(_, k)| k).collect::<Vec<_>>();
+		let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::<Vec<_>>();
 
-			Self::change_authorities(next_authorities, next_queued_authorities);
-		}
+		// Always issue a change on each `session`, even if validator set hasn't changed.
+		// We want to have at least one BEEFY mandatory block per session.
+		Self::change_authorities(next_authorities, next_queued_authorities);
 	}
 
 	fn on_disabled(i: u32) {

substrate/frame/beefy/src/mock.rs

@@ -145,8 +145,7 @@ pub fn new_test_ext_raw_authorities(authorities: Vec<(u64, BeefyId)>) -> TestExt
 
 	let session_keys: Vec<_> = authorities
 		.iter()
-		.enumerate()
-		.map(|(_, id)| (id.0 as u64, id.0 as u64, MockSessionKeys { dummy: id.1.clone() }))
+		.map(|id| (id.0 as u64, id.0 as u64, MockSessionKeys { dummy: id.1.clone() }))
 		.collect();
 
 	BasicExternalities::execute_with_storage(&mut t, || {

substrate/frame/beefy/src/tests.rs

@@ -59,23 +59,28 @@ fn genesis_session_initializes_authorities() {
 #[test]
 fn session_change_updates_authorities() {
 	new_test_ext(vec![1, 2, 3, 4]).execute_with(|| {
-		init_block(1);
-
 		assert!(0 == Beefy::validator_set_id());
 
-		// no change - no log
-		assert!(System::digest().logs.is_empty());
-
-		init_block(2);
+		init_block(1);
 
 		assert!(1 == Beefy::validator_set_id());
 
 		let want = beefy_log(ConsensusLog::AuthoritiesChange(
-			ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4)], 1).unwrap(),
+			ValidatorSet::new(vec![mock_beefy_id(1), mock_beefy_id(2)], 1).unwrap(),
 		));
 
 		let log = System::digest().logs[0].clone();
+		assert_eq!(want, log);
 
+		init_block(2);
+
+		assert!(2 == Beefy::validator_set_id());
+
+		let want = beefy_log(ConsensusLog::AuthoritiesChange(
+			ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4)], 2).unwrap(),
+		));
+
+		let log = System::digest().logs[1].clone();
 		assert_eq!(want, log);
 	});
 }
@@ -85,15 +90,13 @@ fn session_change_updates_next_authorities() {
 	let want = vec![mock_beefy_id(1), mock_beefy_id(2), mock_beefy_id(3), mock_beefy_id(4)];
 
 	new_test_ext(vec![1, 2, 3, 4]).execute_with(|| {
-		init_block(1);
-
 		let next_authorities = Beefy::next_authorities();
 
 		assert!(next_authorities.len() == 2);
 		assert_eq!(want[0], next_authorities[0]);
 		assert_eq!(want[1], next_authorities[1]);
 
-		init_block(2);
+		init_block(1);
 
 		let next_authorities = Beefy::next_authorities();
 
@@ -121,11 +124,14 @@ fn validator_set_updates_work() {
 	let want = vec![mock_beefy_id(1), mock_beefy_id(2), mock_beefy_id(3), mock_beefy_id(4)];
 
 	new_test_ext(vec![1, 2, 3, 4]).execute_with(|| {
+		let vs = Beefy::validator_set().unwrap();
+		assert_eq!(vs.id(), 0u64);
+
 		init_block(1);
 
 		let vs = Beefy::validator_set().unwrap();
 
-		assert_eq!(vs.id(), 0u64);
+		assert_eq!(vs.id(), 1u64);
 		assert_eq!(want[0], vs.validators()[0]);
 		assert_eq!(want[1], vs.validators()[1]);
 
@@ -133,7 +139,7 @@ fn validator_set_updates_work() {
 
 		let vs = Beefy::validator_set().unwrap();
 
-		assert_eq!(vs.id(), 1u64);
+		assert_eq!(vs.id(), 2u64);
 		assert_eq!(want[2], vs.validators()[0]);
 		assert_eq!(want[3], vs.validators()[1]);
 	});