pallet-beefy: ensure mandatory block once per session (paritytech#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>

client/beefy/src/worker.rs

@@ -242,7 +242,10 @@ where
 		debug!(target: "beefy", "🥩 New active validator set: {:?}", active);
 		metric_set!(self, beefy_validator_set_id, active.id());
 		// BEEFY should produce a signed commitment for each session
-		if active.id() != self.last_signed_id + 1 && active.id() != GENESIS_AUTHORITY_SET_ID {
+		if active.id() != self.last_signed_id + 1 &&
+			active.id() != GENESIS_AUTHORITY_SET_ID &&
+			self.last_signed_id != 0
+		{
 			metric_inc!(self, beefy_skipped_sessions);
 		}
 

frame/beefy-mmr/src/tests.rs

@@ -70,9 +70,14 @@ fn should_contain_mmr_digest() {
 
 		assert_eq!(
 			System::digest().logs,
-			vec![beefy_log(ConsensusLog::MmrRoot(
-				hex!("fa0275b19b2565089f7e2377ee73b9050e8d53bce108ef722a3251fd9d371d4b").into()
-			))]
+			vec![
+				beefy_log(ConsensusLog::AuthoritiesChange(
+					ValidatorSet::new(vec![mock_beefy_id(1), mock_beefy_id(2)], 1).unwrap()
+				)),
+				beefy_log(ConsensusLog::MmrRoot(
+					hex!("95803defe6ea9f41e7ec6afa497064f21bfded027d8812efacbdf984e630cbdc").into()
+				))
+			]
 		);
 
 		// unique every time
@@ -81,14 +86,17 @@ fn should_contain_mmr_digest() {
 		assert_eq!(
 			System::digest().logs,
 			vec![
+				beefy_log(ConsensusLog::AuthoritiesChange(
+					ValidatorSet::new(vec![mock_beefy_id(1), mock_beefy_id(2)], 1).unwrap()
+				)),
 				beefy_log(ConsensusLog::MmrRoot(
-					hex!("fa0275b19b2565089f7e2377ee73b9050e8d53bce108ef722a3251fd9d371d4b").into()
+					hex!("95803defe6ea9f41e7ec6afa497064f21bfded027d8812efacbdf984e630cbdc").into()
 				)),
 				beefy_log(ConsensusLog::AuthoritiesChange(
-					ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4),], 1,).unwrap()
+					ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4)], 2).unwrap()
 				)),
 				beefy_log(ConsensusLog::MmrRoot(
-					hex!("85554fa7d4e863cce3cdce668c1ae82c0174ad37f8d1399284018bec9f9971c3").into()
+					hex!("a73271a0974f1e67d6e9b8dd58e506177a2e556519a330796721e98279a753e2").into()
 				)),
 			]
 		);
@@ -109,9 +117,9 @@ fn should_contain_valid_leaf_data() {
 			version: MmrLeafVersion::new(1, 5),
 			parent_number_and_hash: (0_u64, H256::repeat_byte(0x45)),
 			beefy_next_authority_set: BeefyNextAuthoritySet {
-				id: 1,
+				id: 2,
 				len: 2,
-				root: hex!("176e73f1bf656478b728e28dd1a7733c98621b8acf830bff585949763dca7a96")
+				root: hex!("9c6b2c1b0d0b25a008e6c882cc7b415f309965c72ad2b944ac0931048ca31cd5")
 					.into(),
 			},
 			leaf_extra: hex!("55b8e9e1cc9f0db7776fac0ca66318ef8acfb8ec26db11e373120583e07ee648")
@@ -131,7 +139,7 @@ fn should_contain_valid_leaf_data() {
 			version: MmrLeafVersion::new(1, 5),
 			parent_number_and_hash: (1_u64, H256::repeat_byte(0x45)),
 			beefy_next_authority_set: BeefyNextAuthoritySet {
-				id: 2,
+				id: 3,
 				len: 2,
 				root: hex!("9c6b2c1b0d0b25a008e6c882cc7b415f309965c72ad2b944ac0931048ca31cd5")
 					.into(),

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) {

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, || {

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,19 +124,22 @@ 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]);
 
 		init_block(2);
 
 		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]);
 	});