Fix the BABE epoch logic in SRML #3611
Conversation
| @@ -52,17 +52,13 @@ pub enum BabePreDigest { | |||
| authority_index: super::AuthorityIndex, | |||
| /// Slot number | |||
| slot_number: SlotNumber, | |||
| /// Chain weight (measured in number of Primary blocks) | |||
| weight: BabeBlockWeight, | |||
There was a problem hiding this comment.
Where should this go then?
There was a problem hiding this comment.
it'll be maintained off-chain; it's a bit redundant to put it in the block header.
|
|
||
| /// Current slot number. | ||
| pub CurrentSlot get(current_slot): u64; | ||
|
|
||
| /// Whether secondary slots are enabled in case the VRF-based slot is |
There was a problem hiding this comment.
I am OK with removing support for dynamically enabling/disabling secondary slots since we can't change the slot duration anyway, so in practice this is not so useful.
There was a problem hiding this comment.
Right, that was my rationale. You'd never want to enable/disable secondary slots without tweaking the duration or the c parameter
| // from this block into the under-construction randomness. If we've determined | ||
| // that this block was the first in a new epoch, the changeover logic has | ||
| // already occurred at this point, so the under-construction randomness | ||
| // will only contain outputs from the right epoch. |
| // The exception is for block 1: the genesis has slot 0, so we treat | ||
| // epoch 0 as having started at the slot of block 1. We want to use | ||
| // the same randomness and validator set as signalled in the genesis, | ||
| // so we don't rotate the session. |
srml/babe/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| let mut guard = Guard::default(); | ||
| for digest in Self::get_inherent_digests() |
There was a problem hiding this comment.
I know this code was already written like this but in practice this loop should only have one iteration so maybe we could rewrite this to make that more explicit? Using find instead of filter_map or just doing a take(1) at the end should work. We can probably avoid using this Guard if it's explicitly just getting one value from the iterator.
| /// How many blocks to wait before running the median algorithm for relative time | ||
| /// This will not vary from chain to chain as it is not dependent on slot duration | ||
| /// or epoch length. | ||
| pub const MEDIAN_ALGORITHM_CARDINALITY: usize = 1200; // arbitrary suggestion by w3f-research. |
There was a problem hiding this comment.
It'll become some number of slots maybe, yes?
|
|
||
| /// Whether this chain should run with secondary slots, which are assigned | ||
| /// in round-robin manner. | ||
| pub secondary_slots: bool, |
There was a problem hiding this comment.
Another option might be pub secondary_slots: u16, or Option<..> that expresses how many VRF slots should elapse between secondary slots, with u16::MAX or None denoting not to bother with secondary slots.
This is a PR against #3583 to add the SRML functionality. It is not intended that BABE will build with this PR (hence no green checkmark). It's only for review of the SRML changes. I've checked that SRML-babe and node-runtime build with these changes.
Notable changes: