Fixing BABE epochs to change between blocks (#3583)

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* address grumbles

Author: rphmeier

Cargo.lock

@@ -1482,6 +1482,9 @@ impl<B, E, Block, RA> CallRuntimeAt<Block> for Client<B, E, Block, RA> where
 	}
 }
 
+/// NOTE: only use this implementation when you are sure there are NO consensus-level BlockImport
+/// objects. Otherwise, importing blocks directly into the client would be bypassing
+/// important verification work.
 impl<'a, B, E, Block, RA> consensus::BlockImport<Block> for &'a Client<B, E, Block, RA> where
 	B: backend::Backend<Block, Blake2Hasher>,
 	E: CallExecutor<Block, Blake2Hasher> + Clone + Send + Sync,
@@ -1491,6 +1494,13 @@ impl<'a, B, E, Block, RA> consensus::BlockImport<Block> for &'a Client<B, E, Blo
 
 	/// Import a checked and validated block. If a justification is provided in
 	/// `BlockImportParams` then `finalized` *must* be true.
+	///
+	/// NOTE: only use this implementation when there are NO consensus-level BlockImport
+	/// objects. Otherwise, importing blocks directly into the client would be bypassing
+	/// important verification work.
+	///
+	/// If you are not sure that there are no BlockImport objects provided by the consensus
+	/// algorithm, don't use this function.
 	fn import_block(
 		&mut self,
 		import_block: BlockImportParams<Block>,
@@ -1899,25 +1909,27 @@ where
 /// Utility methods for the client.
 pub mod utils {
 	use super::*;
-	use crate::{backend::Backend, blockchain, error};
+	use crate::{blockchain, error};
 	use primitives::H256;
+	use std::borrow::Borrow;
 
 	/// Returns a function for checking block ancestry, the returned function will
 	/// return `true` if the given hash (second parameter) is a descendent of the
 	/// base (first parameter). If the `current` parameter is defined, it should
 	/// represent the current block `hash` and its `parent hash`, if given the
 	/// function that's returned will assume that `hash` isn't part of the local DB
 	/// yet, and all searches in the DB will instead reference the parent.
-	pub fn is_descendent_of<'a, B, E, Block: BlockT<Hash=H256>, RA>(
-		client: &'a Client<B, E, Block, RA>,
-		current: Option<(&'a H256, &'a H256)>,
+	pub fn is_descendent_of<'a, Block: BlockT<Hash=H256>, T, H: Borrow<H256> + 'a>(
+		client: &'a T,
+		current: Option<(H, H)>,
 	) -> impl Fn(&H256, &H256) -> Result<bool, error::Error> + 'a
-		where B: Backend<Block, Blake2Hasher>,
-			  E: CallExecutor<Block, Blake2Hasher> + Send + Sync,
+		where T: ChainHeaderBackend<Block>,
 	{
 		move |base, hash| {
 			if base == hash { return Ok(false); }
 
+			let current = current.as_ref().map(|(c, p)| (c.borrow(), p.borrow()));
+
 			let mut hash = hash;
 			if let Some((current_hash, current_parent_hash)) = current {
 				if base == current_hash { return Ok(false); }
@@ -1931,7 +1943,7 @@ pub mod utils {
 			}
 
 			let tree_route = blockchain::tree_route(
-				|id| client.header(&id)?.ok_or_else(|| Error::UnknownBlock(format!("{:?}", id))),
+				|id| client.header(id)?.ok_or_else(|| Error::UnknownBlock(format!("{:?}", id))),
 				BlockId::Hash(*hash),
 				BlockId::Hash(*base),
 			)?;

core/client/src/client.rs

@@ -218,8 +218,8 @@ impl<H, B, C, E, I, P, Error, SO> slots::SimpleSlotWorker<B> for AuraWorker<C, E
 		self.block_import.clone()
 	}
 
-	fn epoch_data(&self, block: &B::Hash) -> Result<Self::EpochData, consensus_common::Error> {
-		authorities(self.client.as_ref(), &BlockId::Hash(*block))
+	fn epoch_data(&self, header: &B::Header, _slot_number: u64) -> Result<Self::EpochData, consensus_common::Error> {
+		authorities(self.client.as_ref(), &BlockId::Hash(header.hash()))
 	}
 
 	fn authorities_len(&self, epoch_data: &Self::EpochData) -> usize {
@@ -741,7 +741,7 @@ mod tests {
 			}
 		}
 
-		fn make_verifier(&self, client: PeersClient, _cfg: &ProtocolConfig)
+		fn make_verifier(&self, client: PeersClient, _cfg: &ProtocolConfig, _peer_data: &())
 			-> Self::Verifier
 		{
 			match client {

core/consensus/aura/src/lib.rs

@@ -17,12 +17,10 @@
 //! Private implementation details of BABE digests.
 
 #[cfg(feature = "std")]
-use super::AuthoritySignature;
-#[cfg(feature = "std")]
-use super::{BABE_ENGINE_ID, Epoch};
+use super::{BABE_ENGINE_ID, AuthoritySignature};
 #[cfg(not(feature = "std"))]
 use super::{VRF_OUTPUT_LENGTH, VRF_PROOF_LENGTH};
-use super::{AuthorityIndex, BabeBlockWeight, SlotNumber};
+use super::{AuthorityId, AuthorityIndex, SlotNumber, BabeAuthorityWeight};
 #[cfg(feature = "std")]
 use sr_primitives::{DigestItem, generic::OpaqueDigestItemId};
 #[cfg(feature = "std")]
@@ -35,6 +33,8 @@ use schnorrkel::{
 	SignatureError, errors::MultiSignatureStage,
 	vrf::{VRFProof, VRFOutput, VRF_OUTPUT_LENGTH, VRF_PROOF_LENGTH}
 };
+use rstd::vec::Vec;
+
 
 /// A BABE pre-runtime digest. This contains all data required to validate a
 /// block and for the BABE runtime module. Slots can be assigned to a primary
@@ -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,
 	},
 	/// A secondary deterministic slot assignment.
 	Secondary {
 		/// Authority index
 		authority_index: super::AuthorityIndex,
 		/// Slot number
 		slot_number: SlotNumber,
-		/// Chain weight (measured in number of Primary blocks)
-		weight: BabeBlockWeight,
 	},
 }
 
@@ -84,11 +80,12 @@ impl BabePreDigest {
 		}
 	}
 
-	/// Returns the weight of the pre digest.
-	pub fn weight(&self) -> BabeBlockWeight {
+	/// Returns the weight _added_ by this digest, not the cumulative weight
+	/// of the chain.
+	pub fn added_weight(&self) -> crate::BabeBlockWeight {
 		match self {
-			BabePreDigest::Primary { weight, .. } => *weight,
-			BabePreDigest::Secondary { weight, .. } => *weight,
+			BabePreDigest::Primary { .. } => 1,
+			BabePreDigest::Secondary { .. } => 0,
 		}
 	}
 }
@@ -100,26 +97,29 @@ pub const BABE_VRF_PREFIX: &'static [u8] = b"substrate-babe-vrf";
 #[derive(Copy, Clone, Encode, Decode)]
 pub enum RawBabePreDigest {
 	/// A primary VRF-based slot assignment.
+	#[codec(index = "1")]
 	Primary {
 		/// Authority index
 		authority_index: AuthorityIndex,
 		/// Slot number
 		slot_number: SlotNumber,
-		/// Chain weight (measured in number of Primary blocks)
-		weight: BabeBlockWeight,
 		/// VRF output
 		vrf_output: [u8; VRF_OUTPUT_LENGTH],
 		/// VRF proof
 		vrf_proof: [u8; VRF_PROOF_LENGTH],
 	},
 	/// A secondary deterministic slot assignment.
+	#[codec(index = "2")]
 	Secondary {
 		/// Authority index
+		///
+		/// This is not strictly-speaking necessary, since the secondary slots
+		/// are assigned based on slot number and epoch randomness. But including
+		/// it makes things easier for higher-level users of the chain data to
+		/// be aware of the author of a secondary-slot block.
 		authority_index: AuthorityIndex,
 		/// Slot number
 		slot_number: SlotNumber,
-		/// Chain weight (measured in number of Primary blocks)
-		weight: BabeBlockWeight,
 	},
 }
 
@@ -142,25 +142,21 @@ impl Encode for BabePreDigest {
 				vrf_proof,
 				authority_index,
 				slot_number,
-				weight,
 			} => {
 				RawBabePreDigest::Primary {
 					vrf_output: *vrf_output.as_bytes(),
 					vrf_proof: vrf_proof.to_bytes(),
 					authority_index: *authority_index,
 					slot_number: *slot_number,
-					weight: *weight,
 				}
 			},
 			BabePreDigest::Secondary {
 				authority_index,
 				slot_number,
-				weight,
 			} => {
 				RawBabePreDigest::Secondary {
 					authority_index: *authority_index,
 					slot_number: *slot_number,
-					weight: *weight,
 				}
 			},
 		};
@@ -176,7 +172,7 @@ impl codec::EncodeLike for BabePreDigest {}
 impl Decode for BabePreDigest {
 	fn decode<R: Input>(i: &mut R) -> Result<Self, Error> {
 		let pre_digest = match Decode::decode(i)? {
-			RawBabePreDigest::Primary { vrf_output, vrf_proof, authority_index, slot_number, weight } => {
+			RawBabePreDigest::Primary { vrf_output, vrf_proof, authority_index, slot_number } => {
 				// Verify (at compile time) that the sizes in babe_primitives are correct
 				let _: [u8; super::VRF_OUTPUT_LENGTH] = vrf_output;
 				let _: [u8; super::VRF_PROOF_LENGTH] = vrf_proof;
@@ -186,18 +182,29 @@ impl Decode for BabePreDigest {
 					vrf_output: VRFOutput::from_bytes(&vrf_output).map_err(convert_error)?,
 					authority_index,
 					slot_number,
-					weight,
 				}
 			},
-			RawBabePreDigest::Secondary { authority_index, slot_number, weight } => {
-				BabePreDigest::Secondary { authority_index, slot_number, weight }
+			RawBabePreDigest::Secondary { authority_index, slot_number } => {
+				BabePreDigest::Secondary { authority_index, slot_number }
 			},
 		};
 
 		Ok(pre_digest)
 	}
 }
 
+/// Information about the next epoch. This is broadcast in the first block
+/// of the epoch.
+#[derive(Decode, Encode, Default, PartialEq, Eq, Clone)]
+#[cfg_attr(any(feature = "std", test), derive(Debug))]
+pub struct NextEpochDescriptor {
+	/// The authorities.
+	pub authorities: Vec<(AuthorityId, BabeAuthorityWeight)>,
+
+	/// The value of randomness to use for the slot-assignment.
+	pub randomness: [u8; VRF_OUTPUT_LENGTH],
+}
+
 /// A digest item which is usable with BABE consensus.
 #[cfg(feature = "std")]
 pub trait CompatibleDigestItem: Sized {
@@ -214,7 +221,7 @@ pub trait CompatibleDigestItem: Sized {
 	fn as_babe_seal(&self) -> Option<AuthoritySignature>;
 
 	/// If this item is a BABE epoch, return it.
-	fn as_babe_epoch(&self) -> Option<Epoch>;
+	fn as_next_epoch_descriptor(&self) -> Option<NextEpochDescriptor>;
 }
 
 #[cfg(feature = "std")]
@@ -237,8 +244,12 @@ impl<Hash> CompatibleDigestItem for DigestItem<Hash> where
 		self.try_to(OpaqueDigestItemId::Seal(&BABE_ENGINE_ID))
 	}
 
-	fn as_babe_epoch(&self) -> Option<Epoch> {
+	fn as_next_epoch_descriptor(&self) -> Option<NextEpochDescriptor> {
 		self.try_to(OpaqueDigestItemId::Consensus(&BABE_ENGINE_ID))
+			.and_then(|x: super::ConsensusLog| match x {
+				super::ConsensusLog::NextEpochData(n) => Some(n),
+				_ => None,
+			})
 	}
 }