Fix BABE epochs-transition logic on the core side #3620

rphmeier · 2019-09-16T08:15:03Z

This is the counterpart to the initial type-definition work done in #3611 on the core side.

Rough overview of changeset:

Extract SharedEpochChanges logic out to its own module in a way that can be more cleanly tested
Track chain total-weight offchain and handle it in the block-import worker
Define epoch 0 as starting at block 1. There might be multiple block 1s, so they might all kick off slightly competing epochs.
Allow epochs to change between blocks and grab epoch based on start slot as well as ancestry.
Detect that the first block in an epoch is such by comparing the epoch start slot to the parent block's slot. The first block in an epoch must issue a NextEpochDescriptor about the following epoch.
Does not define pruning of the epoch fork-tree at this point. That can come in a follow-up PR, since the logic is complex.
Fix BABE tests

andresilva · 2019-09-18T13:56:17Z

core/consensus/babe/src/aux_schema.rs

 	)
 }
+
+/// Write the cumulative chain-weight of a block ot aux storage.


Suggested change

/// Write the cumulative chain-weight of a block ot aux storage.

/// Write the cumulative chain-weight of a block to aux storage.

andresilva · 2019-09-18T13:57:30Z

core/consensus/babe/src/epoch_changes.rs

+
+	/// Build an `is_descendent_of` function.
+	///
+	/// The `current` parameter can be `Some` with the details a fresh block whose


Suggested change

/// The `current` parameter can be `Some` with the details a fresh block whose

/// The `current` parameter can be `Some` with the details of a fresh block whose

andresilva · 2019-09-18T14:06:31Z

core/consensus/babe/src/epoch_changes.rs

+		// prune any epochs which could not be _live_ as of the children of the
+		// finalized block.
+		// i.e. re-root the fork tree to the earliest ancestor of (hash, number)
+		// where epoch.start_slot + epoch.duration >= slot(hash)


self.inner.prune(hash, number, is_descendent_of) should work, but I'll verify again before merging this PR.

andresilva · 2019-09-18T14:10:30Z

core/consensus/babe/src/epoch_changes.rs

+// TODO: write tests within the scope of this module.
+//
+// 1. mock up an `IsDescendentOfBuilder`. Don't use the `Client` - these tests should be
+// minimal and focused.


https://github.com/paritytech/substrate/blob/master/core/utils/fork-tree/src/lib.rs#L649

We can mock an arbitrary tree like this.

Would love to share that code!

andresilva · 2019-09-18T14:17:16Z

core/consensus/babe/src/lib.rs

-	Ok(slots::start_slot_worker(
+
+	let epoch_changes = babe_link.epoch_changes.clone();
+	let pruning_task = client.finality_notification_stream()


This will only be started by validators this way and it must be run by all full nodes. That's why it was returned with the import queue.

Yeah, I realized that earlier today, which a bit too late. Still, I don't think that forcing the user to spawn a background task is friendly at all.

I'd rather have us prune implicitly every time we add an epoch, based on the finalized block.

core/consensus/babe/src/lib.rs

andresilva · 2019-09-18T14:23:31Z

core/consensus/babe/src/lib.rs

+				// TODO: block-import handles fork choice and this shouldn't even have the
+				// option to specify one.
+				// https://github.com/paritytech/substrate/issues/3623
+				fork_choice: ForkChoiceStrategy::LongestChain,


When we are authoring (before import), i.e. by this exact point, do we guarantee that nothing else gets imported in the meantime? I'm worried that we can author a primary block and won't reorg to it because in the meantime we might have imported a secondary block (with longest chain we would keep the secondary at the same height). The previous code was also susceptible to this kind of race condition if it exists.

In the meantime I read the code below and figured out that this logic was moved to BabeBlockImport which also takes care of the race condition issue.

andresilva · 2019-09-18T14:30:20Z

core/consensus/babe/src/lib.rs


 			old_epoch_changes = Some(epoch_changes.clone());

+			babe_info!("New epoch {} launching at block {} (block slot {} >= start slot {}).",


As above, should these be downgraded?

I'd prefer these to go in the info logs since we won't get them that often (although due to forks we might get a couple ~duplicates at a time).

rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 16, 2019

rphmeier requested review from Demi-Marie and andresilva as code owners September 16, 2019 08:15

rphmeier force-pushed the rh-fix-epochs-core branch from ee2d7dc to 95846ca Compare September 17, 2019 21:41

rphmeier requested review from Robbepop, gui1117, jimpo, kianenigma, pepyakin and tomusdrw as code owners September 17, 2019 21:41

rphmeier force-pushed the rh-fix-babe-epochs branch from d4cef45 to d2d3942 Compare September 17, 2019 21:41

rphmeier removed request for Robbepop, gui1117, jimpo, kianenigma, pepyakin and tomusdrw September 17, 2019 21:42

rphmeier added 13 commits September 18, 2019 12:14

begin accounting for next epoch in epoch function

d95c733

slots passes header to epoch_data

aa429f5

pass slot_number to SlotWorker::epoch_data

73d9221

begin extracting epoch-change logic into its own module

4269d9d

aux methods for block weight

4289869

aux methods for genesis configuration

abf57ca

comment-out most, refactor header-check pipeline

2173879

mostly flesh out verifier again

5c321ef

reinstantiate babe BlockImport implementation

81aaab7

reinstate import-queue instantiation

f22846a

reintroduce slot-worker implementation

bb0ef7b

reinstate pretty much all the rest

4b74154

move fork-choice logic to BlockImport

b6dba9a

rphmeier added 12 commits September 18, 2019 12:16

fix some, but not all errors

35e3c89

patch test-runtime

e7d7545

make is_descendent of slightly more generic

9f1cbe7

get skeleton compiling when passing is_descendent_of

c6c46a5

make descendent-of-builder more succinct

ad87669

restore ordering of authority_index / slot_number

0d8d5b2

start fiddling with tests

1a4f223

fix warnings

3a655d8

improve initialization architecture and handle genesis

144fb6f

tests use correct block-import

1cfbde0

fix BABE tests

9454ea1

fix some compiler errors

6d8a151

rphmeier force-pushed the rh-fix-epochs-core branch from a5a226e to 6d8a151 Compare September 18, 2019 10:47

fix node-cli compilation

975afdd

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 18, 2019

rphmeier added 2 commits September 18, 2019 16:14

all crates compile

582f558

bump runtime versions and fix some warnings

b147ebe

andresilva reviewed Sep 18, 2019

View reviewed changes

rphmeier mentioned this pull request Sep 18, 2019

Split BABE verification and authorship code into own modules #3642

Closed

tweak fork-tree search implementation

d8ffc2d

paritytech-ci added the B2-breaksapi label Sep 18, 2019

rphmeier added 3 commits September 18, 2019 19:48

do backtracking search in fork-tree

aa37321

node-cli integration tests now work

20ec772

fix broken assumption in test_connectivity

e13983a

rphmeier merged commit a82f7f6 into rh-fix-babe-epochs Sep 19, 2019

rphmeier deleted the rh-fix-epochs-core branch September 19, 2019 13:14

rphmeier mentioned this pull request Sep 19, 2019

Fixing BABE epochs to change between blocks #3583

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix BABE epochs-transition logic on the core side #3620

Fix BABE epochs-transition logic on the core side #3620

Uh oh!

rphmeier commented Sep 16, 2019 •

edited

Loading

Uh oh!

andresilva Sep 18, 2019

Uh oh!

andresilva Sep 18, 2019

Uh oh!

andresilva Sep 18, 2019

Uh oh!

andresilva Sep 18, 2019

Uh oh!

rphmeier Sep 18, 2019

Uh oh!

andresilva Sep 18, 2019

Uh oh!

rphmeier Sep 18, 2019

Uh oh!

Uh oh!

andresilva Sep 18, 2019

Uh oh!

andresilva Sep 18, 2019

Uh oh!

rphmeier Sep 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/// Write the cumulative chain-weight of a block ot aux storage.
	/// Write the cumulative chain-weight of a block to aux storage.

	/// The `current` parameter can be `Some` with the details a fresh block whose
	/// The `current` parameter can be `Some` with the details of a fresh block whose


		old_epoch_changes = Some(epoch_changes.clone());

		babe_info!("New epoch {} launching at block {} (block slot {} >= start slot {}).",

Fix BABE epochs-transition logic on the core side #3620

Fix BABE epochs-transition logic on the core side #3620

Uh oh!

Conversation

rphmeier commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rphmeier commented Sep 16, 2019 •

edited

Loading