pallet-aura: add feature-flagged explicit slot duration type #14649
Conversation
rphmeier
added
A0-please_review
rphmeier
changed the title
Yeah, specifically for the parts where asynchronous backing is actually enabled and more than one block needs to be made per slot. |
|
Can someone in @paritytech/sdk-node comment on whether it's better to just do the breaking change or to add the feature flag as proposed here? |
| /// | ||
| /// This was the default behavior of the Aura pallet and may be used for | ||
| /// backwards compatibility. | ||
| pub struct MinimumPeriodTimesTwo<T>(sp_std::marker::PhantomData<T>); |
There was a problem hiding this comment.
Should this be #[cfg(not(feature = "explicit-slot-duration"))]?
There was a problem hiding this comment.
No, it's strictly necessary with the explicit-slot-duration feature. I don't see a reason this should be feature flagged as it's strictly additive.
IMO the feature flag is a good way to gently propose a change. However should also be considered that this breaking change is not "silent". Maybe an explicit comment about the backward compatible default type would be useful and a matter of a very small change for the user... Even better would be to use |
| @@ -207,6 +207,7 @@ impl pallet_aura::Config for Runtime { | |||
| type DisabledValidators = (); | |||
| type MaxAuthorities = ConstU32<32>; | |||
| type AllowMultipleBlocksPerSlot = ConstBool<false>; | |||
| type SlotDuration = pallet_aura::MinimumPeriodTimesTwo<Runtime>; | |||
There was a problem hiding this comment.
I added this just to make the doc-CI happy (as it builds with all workspace features)
rphmeier
force-pushed
the
rh-aura-generalize-duration
branch
from
ece178d
to
9558faa
Compare
|
bot merge |
|
Waiting for commit status. |
|
Merge cancelled due to error. Error: Statuses failed for 9558faa |
bin/node-template/runtime/Cargo.toml
Outdated
| @@ -16,7 +16,7 @@ targets = ["x86_64-unknown-linux-gnu"] | |||
| codec = { package = "parity-scale-codec", version = "3.6.1", default-features = false, features = ["derive"] } | |||
| scale-info = { version = "2.5.0", default-features = false, features = ["derive"] } | |||
|
|
|||
| pallet-aura = { version = "4.0.0-dev", default-features = false, path = "../../../frame/aura" } | |||
| pallet-aura = { version = "4.0.0-dev", default-features = false, features = ["explicit-slot-duration"], path = "../../../frame/aura" } | |||
There was a problem hiding this comment.
fwiw if this is more the case of something that is experimental now but you see being useful for everyone, there is also feature = experimental already used in a few frame based crates.
There was a problem hiding this comment.
This is exactly what I was looking for. Thanks
rphmeier
force-pushed
the
rh-aura-generalize-duration
branch
from
3908288
to
aeef644
Compare
|
bot merge |
|
Waiting for commit status. |
This is feature-flagged for backwards compatibility - runtimes which are intending to upgrade to asynchronous backing should make use of this.
The main motivation of this change is that computing the slot duration with the
MinimumPerioddoesn't play nicely with asynchronous backing and authoring multiple blocks per slot.MinimumPeriodfor Cumulus chains doesn't really make a whole lot of sense, as the only clock that they can be compared to is the relay-chain slot number. We will recommend that parachains run withMinimumPeriod = 0, so this change is necessary to keep Aura working in that situation.I added this with the experimental feature flag with LTS in mind.