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

Fixing BABE epochs to change between blocks #3583

Merged
merged 78 commits into from
Sep 23, 2019
Merged

Fixing BABE epochs to change between blocks #3583

merged 78 commits into from
Sep 23, 2019

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Sep 10, 2019 •
edited
Loading

Closes #3586 -- detailed description within that issue.

This PR is rewriting a good portion of BABE to have well-defined behavior at bootstrap time and at epoch-change time.

This is an accumulation PR, with PRs made against it for individual review. More information can be found in those PR descriptions:

Commits from c15a2c3 were made directly to this branch and should bear additional review.

Remaining TODOs:

@rphmeier rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 10, 2019
@rphmeier rphmeier mentioned this pull request Sep 13, 2019
Merged
@Demi-Marie Demi-Marie self-requested a review September 13, 2019 14:15
Demi-Marie
Demi-Marie reviewed Sep 13, 2019
View reviewed changes
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

This needs TODOs for light client support, along with a corresponding issue being filed.

core/consensus/babe/src/lib.rs Outdated Show resolved Hide resolved
node/cli/src/chain_spec.rs
@@ -398,7 +399,11 @@ pub(crate) mod tests {
service_test::connectivity(
integration_test_config_with_two_authorities(),
|config| new_full(config),
|config| new_light(config),
|mut config| {
// light nodes are unsupported
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be a TODO. If this were not a test case, I would prefer to bail out and exit with an error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

disagree - we haven't added light client support as a feature yet. An issue would be fine; I don't see the need for a TODO

node/cli/src/service.rs
|config| new_light(config),
|mut config| {
// light nodes are unsupported
config.roles = Roles::FULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

node/cli/src/service.rs
|config| new_light(config),
|mut config| {
// light nodes are unsupported
config.roles = Roles::FULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

node/cli/src/service.rs
|config| new_light(config),
|mut config| {
// light nodes are unsupported
config.roles = Roles::FULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@rphmeier rphmeier mentioned this pull request Sep 18, 2019
Merged
Demi-Marie
Demi-Marie reviewed Sep 18, 2019
View reviewed changes
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

debug_assert! should be used for debug-mode asserts.

srml/babe/src/lib.rs
Self::do_initialize();
// PRECONDITION: `should_end_session` has done initialization and is guaranteed
// by the session module to be called before this.
#[cfg(debug_assertions)]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is what debug_assert! is for

@rphmeier rphmeier removed the request for review from pepyakin September 20, 2019 15:41
@rphmeier rphmeier added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 20, 2019
@rphmeier rphmeier mentioned this pull request Sep 21, 2019
Merged
@rphmeier rphmeier 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 Sep 23, 2019
andresilva
andresilva approved these changes Sep 23, 2019
View reviewed changes
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.

lgtm, just minor nits. most of the changes were already reviewed from previous PRs, I checked the latest changes on this PR and then gave it an overall review.

core/consensus/babe/src/epoch_changes.rs Outdated Show resolved Hide resolved
core/consensus/babe/src/lib.rs Show resolved Hide resolved
srml/babe/Cargo.toml Outdated Show resolved Hide resolved
srml/babe/src/lib.rs Outdated Show resolved Hide resolved
node/cli/src/service.rs Outdated Show resolved Hide resolved
srml/babe/src/mock.rs Show resolved Hide resolved
rphmeier and others added 2 commits September 23, 2019 14:25
@rphmeier @andresilva
finish srml-babe docs
2a8fff1 
Co-Authored-By: André Silva <andre.beat@gmail.com>
@rphmeier
address grumbles
c7d8ea6
@rphmeier rphmeier merged commit 426c26b into master Sep 23, 2019
@rphmeier rphmeier deleted the rh-fix-babe-epochs branch September 23, 2019 14:03
en pushed a commit to en/substrate that referenced this pull request Sep 24, 2019
@rphmeier @en
Fixing BABE epochs to change between blocks (paritytech#3583)
d6022ae 
* always fetch epoch from runtime

* node integration tests don't test light nodes

* give stand-in full node a FULL role

* rejig babe APIs

* introduce next-epoch-descriptor type

* overhaul srml-BABE epoch logic

* ensure VRF outputs end up in the right epoch-randomness

* rewrite `do_initialize` to remove unnecessary loop

* begin accounting for next epoch in epoch function

* slots passes header to epoch_data

* pass slot_number to SlotWorker::epoch_data

* begin extracting epoch-change logic into its own module

* aux methods for block weight

* aux methods for genesis configuration

* comment-out most, refactor header-check pipeline

* mostly flesh out verifier again

* reinstantiate babe BlockImport implementation

* reinstate import-queue instantiation

* reintroduce slot-worker implementation

* reinstate pretty much all the rest

* move fork-choice logic to BlockImport

* fix some, but not all errors

* patch test-runtime

* make is_descendent of slightly more generic

* get skeleton compiling when passing is_descendent_of

* make descendent-of-builder more succinct

* restore ordering of authority_index / slot_number

* start fiddling with tests

* fix warnings

* improve initialization architecture and handle genesis

* tests use correct block-import

* fix BABE tests

* fix some compiler errors

* fix node-cli compilation

* all crates compile

* bump runtime versions and fix some warnings

* tweak fork-tree search implementation

* do backtracking search in fork-tree

* node-cli integration tests now work

* fix broken assumption in test_connectivity

* babe tests fail for the right reasons.

* test genesis epoch logic for epoch_changes

* test that epochs can change between blocks

* First BABE SRML test

* Testing infrastructure for BABE

Also includes a trivial additional test.

* Apply suggestions from code review

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* A little more test progress

* More work on BABE testing

* Try to get the tests working

* Implement `UintAuthorityId`-based test mocks

* Fix compilation errors

* Adjust to upstream changes

* Block numbers are ignored in BABE epoch calculation

* authority_index() should ignore invalid authorities

* Fix compile error

* Add tests that session transitions happen

* Check if BABE produces logs

It currently does not.

* Fix test suite

This was really nasty, due to a type confusion that showed up as an
off-by-1 buffer error.

* Add additional tests

Most of these were derived from the current output, so they are only
useful to guard against regressions.

* Make the tests more readable

Also bump impl_version.

* Fix excessive line width

* Remove unused imports

* Update srml/babe/src/lib.rs

Co-Authored-By: André Silva <andre.beat@gmail.com>

* try to fix imports

* Fix build errors in test suite

* tests did not pass

* Try to get at least one digest to be output

Currently, the code emits either no digests (if I don’t call
`Session::rotate_session()` or two digests (if I do), which is wrong.

* More tests

They still don’t work, but this should help debugging.

* fix silly error

* Don’t even try to compile a broken test

* remove broken check_epoch test and add one for genesis epoch

* Check that the length of the pre-digests is correct

* Bump `impl_version`

* use epoch_for_descendent_of even for genesis

* account for competing block 1s

* finish srml-babe docs

Co-Authored-By: André Silva <andre.beat@gmail.com>

* address grumbles
@andresilva andresilva mentioned this pull request Sep 24, 2019
Merged
@svyatonik svyatonik mentioned this pull request Dec 12, 2019
Merged
@rphmeier rphmeier mentioned this pull request Sep 26, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Demi-Marie Demi-Marie Demi-Marie left review comments

@andresilva andresilva andresilva approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BABE epochs should change based on time-slot, not block number
4 participants
@rphmeier @andresilva @Demi-Marie @paritytech-ci