diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 696bee9d7ee76..8ab18c58f9dd3 100644 --- a/client/beefy/src/worker.rs +++ b/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); } diff --git a/frame/beefy-mmr/src/tests.rs b/frame/beefy-mmr/src/tests.rs index 37f571cd842ee..fd3ecd5067155 100644 --- a/frame/beefy-mmr/src/tests.rs +++ b/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(), diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 744a06561e8c2..14e7ac26cdb6e 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -105,10 +105,6 @@ impl Pallet { } fn change_authorities(new: Vec, queued: Vec) { - // 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. >::put(&new); let next_id = Self::validator_set_id() + 1u64; @@ -153,16 +149,16 @@ impl OneSessionHandler for Pallet { 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, { - if changed { - let next_authorities = validators.map(|(_, k)| k).collect::>(); - let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::>(); + let next_authorities = validators.map(|(_, k)| k).collect::>(); + let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::>(); - 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) { diff --git a/frame/beefy/src/mock.rs b/frame/beefy/src/mock.rs index 5fc04f7cbd1d2..7a8f15cd51d29 100644 --- a/frame/beefy/src/mock.rs +++ b/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, || { diff --git a/frame/beefy/src/tests.rs b/frame/beefy/src/tests.rs index 7acb40f200dfd..4136b0c1f1ecf 100644 --- a/frame/beefy/src/tests.rs +++ b/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]); });