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

Return babe configuration information in the babe api epoch functions #8072

Merged
26 commits merged into from
Mar 10, 2021

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Feb 8, 2021

polkadot companion: paritytech/polkadot#2467

This PR is an attempt to solve #8060. I'm not very familiar with decl_storage!, so I need to figure a few things out before I can continue.

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 8, 2021
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
@expenses expenses changed the title Return babe configuration information in the Return babe configuration information in the babe api epoch functions Feb 9, 2021
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

I think we need to have 3 storage entries:

  • EpochConfig: BabeEpochConfiguration - We need to make the module's genesis config (add_extra_genesis) take this as a parameter.
  • NextEpochConfig: Option<BabeEpochConfiguration> - This represents the config for epoch N+1 which is already set in stone (i.e. it was already previously announced). When implementing fn next_config() we read from here, if it's None then we return the value from EpochConfig since the config for the next epoch is the same as the current one.
  • PendingEpochConfigChange: Option<NextConfigDescriptor> - This is the current NextEpochConfig entry, it should be renamed to this. It represents a pending epoch config change for N+2, which will be announced once we finish the current epoch (N) and start the epoch N+1 by announcing the config for N+2.

frame/babe/src/lib.rs Outdated Show resolved Hide resolved
primitives/consensus/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
@expenses expenses marked this pull request as ready for review February 17, 2021 14:18
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 17, 2021
@expenses expenses added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Feb 17, 2021
@expenses
Copy link
Contributor Author

We could also remove c and allowed_slots from BabeGenesisConfiguration, but that would require releasing a new version of the babe api and creating migrations etc.

frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
primitives/consensus/babe/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@expenses expenses left a comment

Choose a reason for hiding this comment

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

.

@expenses expenses requested a review from andresilva March 3, 2021 08:49
bkchr
bkchr previously requested changes Mar 5, 2021

struct __OldNextEpochConfig;
impl frame_support::traits::StorageInstance for __OldNextEpochConfig {
fn pallet_prefix() -> &'static str { "BabeApi" }
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong.

Currently this is Babe and not BabeApi.

Also this should probably be configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made it configurable, but I think it still is BabeApi. See paritytech/polkadot#2467 (comment).

@expenses expenses requested a review from bkchr March 9, 2021 08:18
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
frame/babe/src/lib.rs Outdated Show resolved Hide resolved
@expenses
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 10, 2021

The PR is currently unmergeable.

@expenses
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 10, 2021

Trying merge.

@ghost ghost merged commit 2461349 into master Mar 10, 2021
@ghost ghost deleted the ashley-standalone-epoch branch March 10, 2021 08:40
@jordy25519 jordy25519 mentioned this pull request Mar 29, 2021
jordy25519 pushed a commit to cennznet/substrate that referenced this pull request Sep 20, 2021
…paritytech#8072)

* Make changes

* Add serialize/deserialize, copy babe epoch config defaults from node runtime

* Fix line widths and turn default features off for serde

* Remove ser/deser from Epoch, fix node-cli

* Apply suggestions

* Add comment to BABE_GENESIS_EPOCH_CONFIG in bin

* Apply suggestions

* Add a sketchy migration function

* Add a migration test

* Check for PendingEpochConfigChange as well

* Make epoch_config in node-cli

* Move updating EpochConfig out of the if

* Fix executor tests

* Calculate weight for add_epoch_configurations

* Fix babe test

* Apply suggestions from code review

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

* Add more asserts to tests, remove unused changes to primitives/slots

* Allow setting the migration pallet prefix

* Rename to BabePalletPrefix

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
icodezjb added a commit to chainx-org/ChainX that referenced this pull request Feb 8, 2022
gguoss pushed a commit to chainx-org/ChainX that referenced this pull request Feb 25, 2022
* Remove VestingAccount

* Fix chain specs

* Update rust-toolchain

* Update make test and benchmark

* Update CI

* Run `make format` and `make clippy`

* Update CI

* Quick fix

* Support try-runtime

* Remove pallet-randomness-collective-flip

* Migrate elections-phragmen
(paritytech/substrate#7040)

* Migrate PalletVersion to StorageVersion
(paritytech/substrate#9165)

* Migrate pallet-babe epoch config
(paritytech/substrate#8072)

* Migrate frame-system AccountInfo to AccountInfoWithTripleRefCount
(paritytech/substrate#8221)

* Migrate prefix `GrandpaFinality` -> `Grandpa`

* Migrate prefix `Instance1Collective` -> `Council`

* Migrate prefix `Instance2Collective` -> `TechnicalCommittee`

* Migrate prefix `Instance1Membership` -> `TechnicalMembership`

* Migrate pallet-tips prefix from `Treasury` -> `Tips`

* Migrate pallet-bounties prefix from `Treasury` -> `Bounties`

* Migrate prefix from `PhragmenElection` -> `Elections`

* Revert `dev` spec_name

* Confirm migrations order

* Use ChainX substrate patch for system migration

* Run `make format`

* Reset spec_name `dev` -> `chainx`

* Add migrations.rs for mainnet and testnet

* Bump ChainX version to `4.0.0`

* Bump ChainX version to `4.0.0`

* Update governance parameters for dev and malan

* Update malan chainspec

* Quick fix

* Quick fix

* Update generate_keys.sh

* Adjust malan parameters

* Update malan chainspec

* Update malan chainspec

* Update malan chainspec

* Rename malan testnet name

* Add bootnodes url

* Run `make format`

* Regenerate weights

* Disable pre_release.yml CI

* Regenerate benchmark weights

* Run `make clippy`

* Run `make format`

Co-authored-by: icodezjb <icodezjb@users.noreply.github.com>
gguoss pushed a commit to chainx-org/ChainX that referenced this pull request Feb 25, 2022
* Remove VestingAccount

* Fix chain specs

* Update rust-toolchain

* Update make test and benchmark

* Update CI

* Run `make format` and `make clippy`

* Update CI

* Quick fix

* Support try-runtime

* Remove pallet-randomness-collective-flip

* Migrate elections-phragmen
(paritytech/substrate#7040)

* Migrate PalletVersion to StorageVersion
(paritytech/substrate#9165)

* Migrate pallet-babe epoch config
(paritytech/substrate#8072)

* Migrate frame-system AccountInfo to AccountInfoWithTripleRefCount
(paritytech/substrate#8221)

* Migrate prefix `GrandpaFinality` -> `Grandpa`

* Migrate prefix `Instance1Collective` -> `Council`

* Migrate prefix `Instance2Collective` -> `TechnicalCommittee`

* Migrate prefix `Instance1Membership` -> `TechnicalMembership`

* Migrate pallet-tips prefix from `Treasury` -> `Tips`

* Migrate pallet-bounties prefix from `Treasury` -> `Bounties`

* Migrate prefix from `PhragmenElection` -> `Elections`

* Revert `dev` spec_name

* Confirm migrations order

* Use ChainX substrate patch for system migration

* Run `make format`

* Reset spec_name `dev` -> `chainx`

* Add migrations.rs for mainnet and testnet

* Bump ChainX version to `4.0.0`

* Bump ChainX version to `4.0.0`

* Update governance parameters for dev and malan

* Update malan chainspec

* Quick fix

* Quick fix

* Update generate_keys.sh

* Adjust malan parameters

* Update malan chainspec

* Update malan chainspec

* Update malan chainspec

* Rename malan testnet name

* Add bootnodes url

* Run `make format`

* Regenerate weights

* Disable pre_release.yml CI

* Regenerate benchmark weights

* Run `make clippy`

* Run `make format`

Co-authored-by: icodezjb <icodezjb@users.noreply.github.com>
This pull request was closed.
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. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants