This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fetch Babe configuration from runtime state #11760
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
A3-in_progress
Pull request is in progress. No review needed at this stage.
label
Jun 30, 2022
davxy
added
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.
and removed
A3-in_progress
Pull request is in progress. No review needed at this stage.
labels
Jun 30, 2022
davxy
added
the
D3-trivial 🧸
PR contains trivial changes in a runtime directory that do not require an audit
label
Jun 30, 2022
davxy
changed the title
Fetch babe config data from runtime state
Fetch Babe configuration from runtime state
Jun 30, 2022
andresilva
approved these changes
Jul 29, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! 👍
Could you please prepare a companion PR for polkadot to introduce the same runtime API changes? |
Already specified over the 'PRIMARY_PROBABILITY' constant value
@andresilva done |
skunert
approved these changes
Aug 30, 2022
bot merge |
Error: "Check reviews" status is not passing for paritytech/polkadot#5842 |
bkchr
approved these changes
Sep 5, 2022
bot merge |
ark0f
pushed a commit
to gear-tech/substrate
that referenced
this pull request
Feb 27, 2023
* Fetch babe config data from runtime state * Some renaming * More renaming * Final nits * Fix tests and benches * Rename to in BabeConfiguration * Remove duplicate babe parameter description Already specified over the 'PRIMARY_PROBABILITY' constant value * trigger pipeline * trigger pipeline
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 (forc
andallowed_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:
BabeApi::configuration
implementation within the substrate node to fetch the data at the requested block;sc_consensus_babe::Config
newtype. We're going to directly use the configuration type defined within the Babe's primitives (sp_consensus_babe::BabeConfiguration
).BabeGenesisConfiguration
toBabeConfiguration
. 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'sconfiguration
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
andepoch_length
values in theappend_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 thec
andallowed_slots
).(see the
BABE_GENESIS_EPOCH_CONFIG
in node runtime)This is not technically correct since the
c
andallowed_slots
may have changed during an epoch change;Here we prefer to use the most up to date values on migration.
polkadot companion: paritytech/polkadot#5842