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

Conversation

acatangiu
Copy link
Contributor

Deposit ConsensusLog on BEEFY consensus reset.

The log can be used by block import (or light clients) to detect any consensus reset without having to rely on state.

@acatangiu acatangiu added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 14, 2023
@acatangiu acatangiu self-assigned this Aug 14, 2023
frame_support::weights::constants::RocksDbWeight::get().reads(1)
}

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?

Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

LGTM, but I have a couple of questions:

  1. if this digest is emitted during BEEFY restart (not during the start), is it more similar to the GRANDPA forced change or to a regular change? In other words - do we expect light clients to be able to swallow this block and keep syncing? Because e.g. forced change in GRANDPA is currently not supported by (at least our, bridge) light client? Do we then at least generate a mandatory BEEFY justification for such a block?
  2. if we are treating reset as the forced change, which is not supported by regular light clients, then maybe we could emit ConsensusReset digest at current block, but the digesst shall specify the block at which reset will actually happen? I.e. ConsensusReset(<future-block-number>, ValidatorSet<AuthorityId>). IIUC there's not much difference for a light client - it needs to know the BEEFY starting block to be able to start. And if this starting block would be the block N', which in turn will point (using the digest) to the block M.

But to me the current approach (emitting from block hooks) is ok too.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants