Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

BEEFY: add digest log on consensus reset #14765

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

BEEFY: add digest log on consensus reset #14765

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f602ef5
frame/beefy: add privileged call to reset BEEFY consensus
acatangiu Aug 14, 2023
9d36df5
frame/beefy: deposit log on consensus reset
acatangiu Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions frame/beefy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,30 @@ pub mod pallet {
Self::validate_unsigned(source, call)
}
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_initialize(_n: BlockNumberFor<T>) -> Weight {
// weight used by `on_finalize()` hook
frame_support::weights::constants::RocksDbWeight::get().reads(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this isn't technically correct, because at BEEFY genesis block we'll be doing 3 db reads (Authorities and ValidatorSetId in addition to GenesisBlock). So probably it should be frame_support::weights::constants::RocksDbWeight::get().reads(if GenesisBlock::<T>::get() == Some(n) { 3 } else { 1 }). Don't think it is critical here, though :)

}

fn on_finalize(n: BlockNumberFor<T>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this is needed because genesis_block can be a future block. But do we need to allow genesis_block to be a future block ? Could we just reset from the current block ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resetting from current block could also work and would simplify this code - no hook required anymore, just emit digest log when the reset call happens, BUT in practice we'd lose control of selecting the actual BEEFY genesis block number..

Imagine we need to start/reset BEEFY on Kusama or Polkadot: we need to go through governance and propose a genesis block number. It cannot be in the past because past blocks are immutable and it's practically impossible to time the governance enactment block number with the desired beefy genesis block number.

So in order to control beefy-genesis block M, what we do is propose in governance a beefy-genesis future block M > governance motion/proposal lifetime (estimated at block N) so that:

  • if motion fails, then it's noop
  • if it succeeds, the call to set beefy-genesis block will be mined at some block N' <= N < M - so it is valid and works

If we were to use current block then BEEFY genesis is simply the block when governance motion is enacted.

Regarding the need to control the number, I was talking to @andresilva that we'd likely want BEEFY genesis to be a session boundary block - although it's not really mandatory.

@bkchr wdyt? maybe we can use some other magic mechanism to delay a call to be executed on a predefined future block, and not have to rely on pallet-beefy to keep this logic?

// this check is only true when consensus is reset, so we can safely
// estimate the average weight is just one Db read.
if GenesisBlock::<T>::get() == Some(n) {
let validators = Authorities::<T>::get();
let set_id = ValidatorSetId::<T>::get();
if let Some(validator_set) = ValidatorSet::<T::BeefyId>::new(validators, set_id) {
let log = DigestItem::Consensus(
BEEFY_ENGINE_ID,
ConsensusLog::ConsensusReset(validator_set).encode(),
);
<frame_system::Pallet<T>>::deposit_log(log);
}
}
}
}
}

impl<T: Config> Pallet<T> {
Expand Down
47 changes: 25 additions & 22 deletions frame/beefy/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,31 +300,34 @@ pub fn new_test_ext_raw_authorities(authorities: Vec<BeefyId>) -> TestExternalit
t.into()
}

pub fn push_block(i: u32) {
System::on_finalize(System::block_number());
Session::on_finalize(System::block_number());
Staking::on_finalize(System::block_number());
Beefy::on_finalize(System::block_number());

let parent_hash = if System::block_number() > 1 {
let hdr = System::finalize();
hdr.hash()
} else {
System::parent_hash()
};

System::reset_events();
System::initialize(&(i as u64 + 1), &parent_hash, &Default::default());
System::set_block_number((i + 1).into());
Timestamp::set_timestamp(System::block_number() * 6000);

System::on_initialize(System::block_number());
Session::on_initialize(System::block_number());
Staking::on_initialize(System::block_number());
Beefy::on_initialize(System::block_number());
}

pub fn start_session(session_index: SessionIndex) {
for i in Session::current_index()..session_index {
System::on_finalize(System::block_number());
Session::on_finalize(System::block_number());
Staking::on_finalize(System::block_number());
Beefy::on_finalize(System::block_number());

let parent_hash = if System::block_number() > 1 {
let hdr = System::finalize();
hdr.hash()
} else {
System::parent_hash()
};

System::reset_events();
System::initialize(&(i as u64 + 1), &parent_hash, &Default::default());
System::set_block_number((i + 1).into());
Timestamp::set_timestamp(System::block_number() * 6000);

System::on_initialize(System::block_number());
Session::on_initialize(System::block_number());
Staking::on_initialize(System::block_number());
Beefy::on_initialize(System::block_number());
push_block(i);
}

assert_eq!(Session::current_index(), session_index);
}

Expand Down
61 changes: 57 additions & 4 deletions frame/beefy/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

use std::vec;

use codec::Encode;
use codec::{Decode, Encode};
use sp_consensus_beefy::{
check_equivocation_proof, generate_equivocation_proof, known_payloads::MMR_ROOT_ID,
Keyring as BeefyKeyring, Payload, ValidatorSet, KEY_TYPE as BEEFY_KEY_TYPE,
Expand All @@ -28,7 +28,7 @@ use sp_runtime::DigestItem;
use frame_support::{
assert_err, assert_ok,
dispatch::{GetDispatchInfo, Pays},
traits::{Currency, KeyOwnerProofSystem, OnInitialize},
traits::{Currency, KeyOwnerProofSystem, OnFinalize, OnInitialize},
};

use crate::{mock::*, Call, Config, Error, Weight, WeightInfo};
Expand Down Expand Up @@ -801,14 +801,67 @@ fn set_new_genesis_works() {

let new_genesis = 10u64;
// the call for setting new genesis should work
assert_ok!(Beefy::set_new_genesis(RuntimeOrigin::root(), new_genesis,));
assert_ok!(Beefy::set_new_genesis(RuntimeOrigin::root(), new_genesis));
// verify new genesis was set
assert_eq!(Beefy::genesis_block(), Some(new_genesis));

// setting to old block should fail
assert_err!(
Beefy::set_new_genesis(RuntimeOrigin::root(), 1u64,),
Beefy::set_new_genesis(RuntimeOrigin::root(), 1u64),
Error::<Test>::InvalidConfiguration,
);
});
}

#[test]
fn deposit_consensus_reset_log_works() {
let authorities = test_authorities();

new_test_ext_raw_authorities(authorities).execute_with(|| {
fn find_consensus_reset_log() -> Option<ConsensusLog<BeefyId>> {
for log in System::digest().logs.iter_mut() {
if let DigestItem::Consensus(BEEFY_ENGINE_ID, inner_log) = log {
let inner_decoded =
ConsensusLog::<BeefyId>::decode(&mut &inner_log[..]).unwrap();
if matches!(inner_decoded, ConsensusLog::ConsensusReset(_)) {
return Some(inner_decoded)
}
}
}
None
}

start_era(1);
push_block(4);

// no consensus reset log
assert!(find_consensus_reset_log().is_none());

let new_genesis = 6u64;
// the call for setting new genesis should work
assert_ok!(Beefy::set_new_genesis(RuntimeOrigin::root(), new_genesis));
// verify new genesis was set
assert_eq!(Beefy::genesis_block(), Some(new_genesis));

// there should still be no digest (yet)
assert!(find_consensus_reset_log().is_none());

push_block(5);

// there should still be no digest (yet)
assert!(find_consensus_reset_log().is_none());

assert_eq!(System::block_number(), new_genesis);
Beefy::on_finalize(System::block_number());

// consensus reset log should be there now
let log = find_consensus_reset_log().unwrap();

// validate logged authorities
let validators = Beefy::authorities();
let set_id = Beefy::validator_set_id();
let want_validator_set = ValidatorSet::<BeefyId>::new(validators, set_id).unwrap();
let want = ConsensusLog::ConsensusReset(want_validator_set);
assert_eq!(want.encode(), log.encode());
});
}
3 changes: 3 additions & 0 deletions primitives/consensus/beefy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ pub enum ConsensusLog<AuthorityId: Codec> {
/// MMR root hash.
#[codec(index = 3)]
MmrRoot(MmrRootHash),
/// BEEFY consensus has been reset.
#[codec(index = 4)]
ConsensusReset(ValidatorSet<AuthorityId>),
}

/// BEEFY vote message.
Expand Down