substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Fetch Babe configuration from runtime state

Open davxy opened this issue 2 years ago • 2 comments

BabeApi::configuration method semantic was not clearly defined.

Almost everywhere, within the codebase, this API method is used to fetch the protocol configuration data as defined on genesis. But the runtime apis are generally defined to act on the state at an arbitrary block.

The (upstream) BabeApi::configuration implementation that ships with Substrate node was fetching some of the data from the current best block state and some other from a global constant containing a couple of genesis values (for c and allowed_slots params). In other words the configuration API doesn't return the genesis value nor the values at the requested block... but an unclear hybrid of the two.

This PR tries to clean-up the definition by:

  1. changing the BabeApi::configuration implementation within the substrate node to fetch the data at the requested block;
  2. remove some over-engineering by removing the sc_consensus_babe::Config newtype. We're going to directly use the configuration type defined within the Babe's primitives (sp_consensus_babe::BabeConfiguration).
  3. rename BabeGenesisConfiguration to BabeConfiguration. This type is not used only to store the genesis configuration, but it is used to store the protocol configuration in general.

The modification should not alter the current behavior since the configuration cached by the various Babe components is almost only used as a fallback to get genesis data. In other words is used on the very first run and in this case the values returned by the Babe's configuration Api are equal to the constant values used before this PR.


This section contains a summary of (previous and current) components holding a cached Babe configuration.

sc_consensus_babe::{BabeLink, BabeBlockImport, BabeSlotWorker}, sc_consensus_babe_rpc::Babe

Cached configuration used only on genesis. Thus the values read from the runtime are equal to the genesis values. In other words, should be functionally equivalent to the previous implementation.

sc_consensus_manual_seal::BabeConsensusDataProvider

Cached configuration used only on genesis (as prev cases).

But it is also used to get slot_duration and epoch_length values in the append_block_import implementation to set up the necessary import parameters. Here the end-result is still "correct" just because these values are not allowed to change between epoch changes.

We should keep an eye on this if in the future these are allowed to change between epochs. (A NOTE in the code has been added just as a reminder and make this explicit)

Migration of EpochChanges

The old version config is migrated to the current one by using the configuration read during startup using the BabeApi.

Previously the BabeApi::configuration was constructed using some constant values (in particular the c and allowed_slots). (see the BABE_GENESIS_EPOCH_CONFIG in node runtime)

This is not technically correct since the c and allowed_slots may have changed during an epoch change; Here we prefer to use the most up to date values on migration.


polkadot companion: https://github.com/paritytech/polkadot/pull/5842

davxy avatar Jun 30 '22 10:06 davxy

Could you please prepare a companion PR for polkadot to introduce the same runtime API changes?

andresilva avatar Jul 29 '22 09:07 andresilva

Could you please prepare a companion PR for polkadot to introduce the same runtime API changes?

@andresilva done

davxy avatar Jul 31 '22 07:07 davxy

bot merge

davxy avatar Sep 05 '22 10:09 davxy

Error: "Check reviews" status is not passing for https://github.com/paritytech/polkadot/pull/5842

bot merge

bkchr avatar Sep 05 '22 17:09 bkchr