This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Relative slots #2820
Merged
Relative slots #2820
Conversation
This file contains hidden or 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
Demi-Marie
added
the
A3-in_progress
tomaka
reviewed
Jun 7, 2019
Contributor
|
I'm interested, could you please add a description or link an issue? @demimarie-parity |
`Aura` → `Babe` Co-Authored-By: Pierre Krieger <[email protected]>
Contributor
Author
|
@marcio-diaz I just included a description of this feature, along with the addition of import queues. |
Somehow, those broke the build
Demi-Marie
added
A0-please_review
niklasad1
reviewed
Jun 13, 2019
It is more useful
niklasad1
reviewed
Jun 14, 2019
Also: `1000_000_000` → `1_000_000_000` Suggested-by: Niklas Adolfsson <[email protected]>
andresilva
reviewed
Jun 17, 2019
Contributor
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
minor grumble, overall lgtm.
core/consensus/babe/src/lib.rs
Outdated
| auxiliary: Vec::new(), | ||
| fork_choice: ForkChoiceStrategy::LongestChain, | ||
| }; | ||
| const NANOS_PER_SEC: u32 = 1_000_000_000; |
Contributor
There was a problem hiding this comment.
Can we move this block of code to some aux function?
Contributor
Author
There was a problem hiding this comment.
We should. We also need to get the information to the block-authoring logic somehow ― currently, it is never used by it. However, I could not think of a better way to do it than to stuff it in a global variable (yuck!!!!).
bkchr
suggested changes
Jun 19, 2019
Member
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Looks good so far, just some minor issues.
Otherwise it would panic.
Co-Authored-By: Bastian Köcher <[email protected]>
… into demi-relative-slots
bkchr
reviewed
Jun 19, 2019
for signed `Duration` values. Suggested-by: Bastian Köcher
Contributor
Author
|
@bkchr can you do another review? |
bkchr
approved these changes
Jun 21, 2019
Member
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
Overall looking good, some last nitpicks.
MTDK1
pushed a commit
to bdevux/substrate
that referenced
this pull request
Jul 10, 2019
* Initial work on relative slots for BABE * More work * Update core/consensus/babe/src/lib.rs `Aura` → `Babe` Co-Authored-By: Pierre Krieger <[email protected]> * More work on relative slots * Add missing field in test-runtime * Bump `impl_version` and `authoring_version` * Fix compile errors and warnings * Upgrade dependencies * Update dependencies more * Revert some updates to dependencies Somehow, those broke the build * Fix compilation errors * `Duration` → `u128` in calculations * `slot_duration` is in milleseconds, not seconds * Median algorithm: ignore blocks with slot_num < sl * Fix silly compile error * Store a duration, rather than an instant It is more useful * Fix compilation errors * `INVERSE_NANO` → `NANOS_PER_SEC` Also: `1000_000_000` → `1_000_000_000` Suggested-by: Niklas Adolfsson <[email protected]> * Un-bump `authoring_version` * Disable median algorithm when `median_required_blocks` is 0 Otherwise it would panic. * Apply suggestions from code review Co-Authored-By: Bastian Köcher <[email protected]> * Simplify panic * Fix build error * Create `SignedDuration` struct for signed `Duration` values. Suggested-by: Bastian Köcher * Refactor median algorithm into separate function * Add issues for FIXMEs and respond to code review * Fix minor warnings
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
BABE uses relative time to determine slots, which avoids AuRa’s requirement that each node have a synchronized clock. However, this has not been implemented until now. Future plans, which will follow this PR, include an on-chain, VRF-based randomness beacon, as well as general improvements to the consensus infrastructure.
This also implements BABE import queues, which are required to use BABE for anything other than its own test suite. This makes using BABE in an actual blockchain possible, although it would be insecure, as equivocation reporting is not yet implemented. That said, I do not believe equivocation reporting has been implemented for AuRa or GRANDPA either (but it will be before 2.0).