Refactor DigestItem (#2108)

* Add `start_aura2`.

* .gitignore patch conflict files

and remove one that accidentally got committed

* Fix build

The tests still don’t work.

* Fix compilation errors

* Fix compile errors (again)

* Try (and fail) to fix tests

* Properly deserialize data

Previously, `DigestItem::Consensus` had no separate `DigestItemType`,
so it did not get properly serialized and deserialized.

* Add extra debug logging.  Always allow old seals.

A `RUST_LOG=substrate_aura_consensus cargo test --all -- --nocapture \
tests::authoring_blocks` revealed that old seals were being and
rejected, causing the test to hang.  As a temporary debug measure, allow
old seals unconditionally, so that CI can test if this fixes the
problem.

* Forcibly disable rejection of old seals

* Use old trait, but newer serialization

The old trait for `CompatibleDigestItem` actually worked.  By changing
its implementation, one can ensure that all *new* seals have the modern
form, but *legacy* seals are still decoded correctly.

* Bump impl version

* Squash spurious deprecation warning

`rustc` should not be emitting a deprecation warning in deprecated
code, but it does, so silence it.

* Rip out unused Cargo feature

* Move AURA to aura_primitives

* Respond to code review

* Wrap overly-long line

* Reduce logging verbosity and add target

* Add dependency on `sr-primitives` to `aura_primitives`

* Fix build

It failed with a message about Cargo.lock being out of date.

* core: aura: rename aura engine id const

* core: aura: remove superfluous logging

* core: primitives: add removed semicolons

* core: aura: remove unused import

* core: network: style fix

* runtime: update wasm blobs

* runtime: bump impl_version

* core: primitives: tag all DigestItemType variants explicitly

substrate/.gitignore

@@ -18,3 +18,5 @@ polkadot.*
 .idea/
 nohup.out
 rls*.log
+*.orig
+*.rej

substrate/core/consensus/aura/primitives/Cargo.toml

@@ -7,6 +7,7 @@ edition = "2018"
 
 [dependencies]
 substrate-client = { path = "../../../client", default-features = false }
+runtime_primitives = { package = "sr-primitives", path = "../../../sr-primitives", default-features = false }
 
 [features]
 default = ["std"]

substrate/core/consensus/aura/primitives/src/lib.rs

@@ -19,6 +19,10 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 use substrate_client::decl_runtime_apis;
+use runtime_primitives::ConsensusEngineId;
+
+/// The `ConsensusEngineId` of AuRa.
+pub const AURA_ENGINE_ID: ConsensusEngineId = [b'a', b'u', b'r', b'a'];
 
 decl_runtime_apis! {
 	/// API necessary for block authorship with aura.

substrate/core/consensus/aura/src/lib.rs

@@ -25,7 +25,7 @@
 //!
 //! Blocks from future steps will be either deferred or rejected depending on how
 //! far in the future they are.
-
+#![deny(deprecated)]
 use std::{sync::Arc, time::Duration, thread, marker::PhantomData, hash::Hash, fmt::Debug};
 
 use parity_codec::{Encode, Decode};
@@ -37,6 +37,7 @@ use client::ChainHead;
 use client::block_builder::api::BlockBuilder as BlockBuilderApi;
 use client::runtime_api::ApiExt;
 use consensus_common::{ImportBlock, BlockOrigin};
+use aura_primitives::AURA_ENGINE_ID;
 use runtime_primitives::{generic, generic::BlockId, Justification};
 use runtime_primitives::traits::{
 	Block, Header, Digest, DigestItemFor, DigestItem, ProvideRuntimeApi
@@ -113,27 +114,42 @@ fn inherent_to_common_error(err: RuntimeString) -> consensus_common::Error {
 
 /// A digest item which is usable with aura consensus.
 pub trait CompatibleDigestItem<T: Pair>: Sized {
-	/// Construct a digest item which is a slot number and a signature on the
+	/// Construct a digest item which contains a slot number and a signature on the
 	/// hash.
-	fn aura_seal(slot_number: u64, signature: Signature<T>) -> Self;
+	fn aura_seal(slot_num: u64, signature: Signature<T>) -> Self;
 
 	/// If this item is an Aura seal, return the slot number and signature.
 	fn as_aura_seal(&self) -> Option<(u64, Signature<T>)>;
+
+	/// Return `true` if this seal type is deprecated.  Otherwise, return
+	/// `false`.
+	fn is_deprecated(&self) -> bool;
 }
 
-impl<T: Pair, Hash, AuthorityId> CompatibleDigestItem<T> for generic::DigestItem<Hash, AuthorityId, Signature<T>>
-	where T::Signature: Clone
+impl<P, Hash> CompatibleDigestItem<P> for generic::DigestItem<Hash, P::Public, P::Signature>
+	where P: Pair, P::Signature: Clone + Encode + Decode,
 {
 	/// Construct a digest item which is a slot number and a signature on the
 	/// hash.
-	fn aura_seal(slot_number: u64, signature: Signature<T>) -> Self {
-		generic::DigestItem::Seal(slot_number, signature)
+	fn aura_seal(slot_number: u64, signature: Signature<P>) -> Self {
+		generic::DigestItem::Consensus(AURA_ENGINE_ID, (slot_number, signature).encode())
 	}
+
 	/// If this item is an Aura seal, return the slot number and signature.
-	fn as_aura_seal(&self) -> Option<(u64, Signature<T>)> {
+	#[allow(deprecated)]
+	fn as_aura_seal(&self) -> Option<(u64, Signature<P>)> {
 		match self {
 			generic::DigestItem::Seal(slot, ref sig) => Some((*slot, (*sig).clone())),
-			_ => None
+			generic::DigestItem::Consensus(AURA_ENGINE_ID, seal) => Decode::decode(&mut &seal[..]),
+			_ => None,
+		}
+	}
+
+	#[allow(deprecated)]
+	fn is_deprecated(&self) -> bool {
+		match self {
+			generic::DigestItem::Seal(_, _) => true,
+			_ => false,
 		}
 	}
 }
@@ -171,6 +187,7 @@ pub fn start_aura_thread<B, C, E, I, P, SO, Error, OnExit>(
 	Error: From<C::Error> + From<I::Error> + 'static,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Encode + Decode + Eq + Clone + Debug + Hash + Send + Sync + 'static,
+	P::Signature: Encode,
 	SO: SyncOracle + Send + Sync + Clone + 'static,
 	OnExit: Future<Item=(), Error=()> + Send + 'static,
 	DigestItemFor<B>: CompatibleDigestItem<P> + DigestItem<AuthorityId=AuthorityId<P>> + 'static,
@@ -217,6 +234,7 @@ pub fn start_aura<B, C, E, I, P, SO, Error, OnExit>(
 	Error: From<C::Error> + From<I::Error>,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Hash + Eq + Send + Sync + Clone + Debug + Encode + Decode + 'static,
+	P::Signature: Encode,
 	SO: SyncOracle + Send + Sync + Clone,
 	DigestItemFor<B>: CompatibleDigestItem<P> + DigestItem<AuthorityId=AuthorityId<P>>,
 	Error: ::std::error::Error + Send + 'static + From<::consensus_common::Error>,
@@ -259,6 +277,7 @@ impl<B: Block, C, E, I, P, Error, SO> SlotWorker<B> for AuraWorker<C, E, I, P, S
 	I: BlockImport<B> + Send + Sync + 'static,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Hash + Eq + Send + Sync + Clone + Debug + Encode + Decode + 'static,
+	P::Signature: Encode,
 	Error: From<C::Error> + From<I::Error>,
 	SO: SyncOracle + Send + Clone,
 	DigestItemFor<B>: CompatibleDigestItem<P> + DigestItem<AuthorityId=AuthorityId<P>>,
@@ -416,27 +435,39 @@ impl<B: Block, C, E, I, P, Error, SO> SlotWorker<B> for AuraWorker<C, E, I, P, S
 /// if it's successful, returns the pre-header, the slot number, and the signat.
 //
 // FIXME #1018 needs misbehavior types
-fn check_header<B: Block, P: Pair>(slot_now: u64, mut header: B::Header, hash: B::Hash, authorities: &[AuthorityId<P>])
-	-> Result<CheckedHeader<B::Header, P::Signature>, String>
+#[forbid(deprecated)]
+fn check_header<B: Block, P: Pair>(
+	slot_now: u64,
+	mut header: B::Header,
+	hash: B::Hash,
+	authorities: &[AuthorityId<P>],
+	allow_old_seals: bool,
+) -> Result<CheckedHeader<B::Header, P::Signature>, String>
 	where DigestItemFor<B>: CompatibleDigestItem<P>,
 		P::Public: Clone + AsRef<P::Public>,
+		P::Signature: Decode,
 {
 	let digest_item = match header.digest_mut().pop() {
 		Some(x) => x,
 		None => return Err(format!("Header {:?} is unsealed", hash)),
 	};
-	let (slot_num, sig) = match digest_item.as_aura_seal() {
-		Some(x) => x,
-		None => return Err(format!("Header {:?} is unsealed", hash)),
-	};
+
+	if !allow_old_seals && digest_item.is_deprecated() {
+		debug!(target: "aura", "Header {:?} uses old seal format, rejecting", hash);
+		return Err(format!("Header {:?} uses old seal format, rejecting", hash))
+	}
+
+	let (slot_num, sig) = digest_item.as_aura_seal().ok_or_else(|| {
+		debug!(target: "aura", "Header {:?} is unsealed", hash);
+		format!("Header {:?} is unsealed", hash)
+	})?;
 
 	if slot_num > slot_now {
 		header.digest_mut().push(digest_item);
 		Ok(CheckedHeader::Deferred(header, slot_num))
 	} else {
 		// check the signature is valid under the expected authority and
 		// chain state.
-
 		let expected_author = match slot_author::<P>(slot_num, &authorities) {
 			None => return Err("Slot Author not found".to_string()),
 			Some(author) => author
@@ -473,6 +504,7 @@ pub struct AuraVerifier<C, E, P> {
 	extra: E,
 	phantom: PhantomData<P>,
 	inherent_data_providers: inherents::InherentDataProviders,
+	allow_old_seals: bool,
 }
 
 impl<C, E, P> AuraVerifier<C, E, P>
@@ -539,13 +571,15 @@ impl<B: Block> ExtraVerification<B> for NothingExtra {
 	}
 }
 
+#[forbid(deprecated)]
 impl<B: Block, C, E, P> Verifier<B> for AuraVerifier<C, E, P> where
 	C: Authorities<B> + ProvideRuntimeApi + Send + Sync,
 	C::Api: BlockBuilderApi<B>,
 	DigestItemFor<B>: CompatibleDigestItem<P> + DigestItem<AuthorityId=AuthorityId<P>>,
 	E: ExtraVerification<B>,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Send + Sync + Hash + Eq + Clone + Decode + Encode + Debug + AsRef<P::Public> + 'static,
+	P::Signature: Encode + Decode,
 {
 	fn verify(
 		&self,
@@ -569,7 +603,13 @@ impl<B: Block, C, E, P> Verifier<B> for AuraVerifier<C, E, P> where
 
 		// we add one to allow for some small drift.
 		// FIXME #1019 in the future, alter this queue to allow deferring of headers
-		let checked_header = check_header::<B, P>(slot_now + 1, header, hash, &authorities[..])?;
+		let checked_header = check_header::<B, P>(
+			slot_now + 1,
+			header,
+			hash,
+			&authorities[..],
+			self.allow_old_seals,
+		)?;
 		match checked_header {
 			CheckedHeader::Checked(pre_header, slot_num, sig) => {
 				let item = <DigestItemFor<B>>::aura_seal(slot_num, sig);
@@ -654,6 +694,7 @@ pub fn import_queue<B, C, E, P>(
 	client: Arc<C>,
 	extra: E,
 	inherent_data_providers: InherentDataProviders,
+	allow_old_seals: bool,
 ) -> Result<AuraImportQueue<B>, consensus_common::Error> where
 	B: Block,
 	C: 'static + Authorities<B> + ProvideRuntimeApi + Send + Sync,
@@ -662,6 +703,7 @@ pub fn import_queue<B, C, E, P>(
 	E: 'static + ExtraVerification<B>,
 	P: Pair + Send + Sync + 'static,
 	P::Public: Clone + Eq + Send + Sync + Hash + Debug + Encode + Decode + AsRef<P::Public>,
+	P::Signature: Encode + Decode,
 {
 	register_aura_inherent_data_provider(&inherent_data_providers, slot_duration.get())?;
 
@@ -671,6 +713,7 @@ pub fn import_queue<B, C, E, P>(
 			extra,
 			inherent_data_providers,
 			phantom: PhantomData,
+			allow_old_seals,
 		}
 	);
 	Ok(BasicQueue::new(verifier, block_import, justification_import))
@@ -756,6 +799,7 @@ mod tests {
 				extra: NothingExtra,
 				inherent_data_providers,
 				phantom: Default::default(),
+				allow_old_seals: false,
 			})
 		}
 

substrate/core/finality-grandpa/src/lib.rs

@@ -111,7 +111,7 @@ use ed25519::{Public as AuthorityId, Signature as AuthoritySignature};
 #[cfg(test)]
 mod tests;
 
-const GRANDPA_ENGINE_ID: network::ConsensusEngineId = [b'a', b'f', b'g', b'1'];
+const GRANDPA_ENGINE_ID: runtime_primitives::ConsensusEngineId = [b'a', b'f', b'g', b'1'];
 const MESSAGE_ROUND_TOLERANCE: u64 = 2;
 
 /// A GRANDPA message for a substrate chain.

substrate/core/network/src/consensus_gossip.rs

@@ -25,10 +25,10 @@ use rand::{self, seq::SliceRandom};
 use lru_cache::LruCache;
 use network_libp2p::{Severity, PeerId};
 use runtime_primitives::traits::{Block as BlockT, Hash, HashFor};
+use runtime_primitives::ConsensusEngineId;
 pub use crate::message::generic::{Message, ConsensusMessage};
 use crate::protocol::Context;
 use crate::config::Roles;
-use crate::ConsensusEngineId;
 
 // FIXME: Add additional spam/DoS attack protection: https://github.com/paritytech/substrate/issues/1115
 const KNOWN_MESSAGES_CACHE_SIZE: usize = 4096;

substrate/core/network/src/lib.rs

@@ -48,7 +48,7 @@ pub use network_libp2p::{
 	NodeKeyConfig, Secret, Secp256k1Secret, Ed25519Secret,
 	build_multiaddr, PeerId, PublicKey
 };
-pub use message::{generic as generic_message, RequestId, Status as StatusMessage, ConsensusEngineId};
+pub use message::{generic as generic_message, RequestId, Status as StatusMessage};
 pub use error::Error;
 pub use on_demand::{OnDemand, OnDemandService, RemoteResponse};
 #[doc(hidden)]

substrate/core/network/src/message.rs

@@ -17,7 +17,7 @@
 //! Network packet message types. These get serialized and put into the lower level protocol payload.
 
 use bitflags::bitflags;
-use runtime_primitives::traits::{Block as BlockT, Header as HeaderT};
+use runtime_primitives::{ConsensusEngineId, traits::{Block as BlockT, Header as HeaderT}};
 use parity_codec::{Encode, Decode, Input, Output};
 pub use self::generic::{
 	BlockAnnounce, RemoteCallRequest, RemoteReadRequest,
@@ -29,9 +29,6 @@ pub use self::generic::{
 /// A unique ID of a request.
 pub type RequestId = u64;
 
-/// Consensus engine unique ID.
-pub type ConsensusEngineId = [u8; 4];
-
 /// Type alias for using the message type using block type parameters.
 pub type Message<B> = generic::Message<
 	<B as BlockT>::Header,

substrate/core/network/src/protocol.rs

@@ -19,10 +19,10 @@ use futures::sync::mpsc;
 use parking_lot::Mutex;
 use network_libp2p::{PeerId, Severity};
 use primitives::storage::StorageKey;
-use runtime_primitives::generic::BlockId;
+use runtime_primitives::{generic::BlockId, ConsensusEngineId};
 use runtime_primitives::traits::{As, Block as BlockT, Header as HeaderT, NumberFor, Zero};
 use consensus::import_queue::ImportQueue;
-use crate::message::{self, Message, ConsensusEngineId};
+use crate::message::{self, Message};
 use crate::message::generic::{Message as GenericMessage, ConsensusMessage};
 use crate::consensus_gossip::ConsensusGossip;
 use crate::on_demand::OnDemandService;

substrate/core/network/src/service.rs

@@ -27,12 +27,12 @@ use network_libp2p::{multiaddr, RegisteredProtocol, NetworkState};
 use peerset::Peerset;
 use consensus::import_queue::{ImportQueue, Link};
 use crate::consensus_gossip::ConsensusGossip;
-use crate::message::{Message, ConsensusEngineId};
+use crate::message::Message;
 use crate::protocol::{self, Context, FromNetworkMsg, Protocol, ConnectedPeer, ProtocolMsg, ProtocolStatus, PeerInfo};
 use crate::config::Params;
 use crossbeam_channel::{self as channel, Receiver, Sender, TryRecvError};
 use crate::error::Error;
-use runtime_primitives::traits::{Block as BlockT, NumberFor};
+use runtime_primitives::{traits::{Block as BlockT, NumberFor}, ConsensusEngineId};
 use crate::specialization::NetworkSpecialization;
 
 use tokio::prelude::task::AtomicTask;
@@ -295,6 +295,7 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>> ::consensus::SyncOracle f
 	fn is_major_syncing(&self) -> bool {
 		self.is_major_syncing()
 	}
+
 	fn is_offline(&self) -> bool {
 		self.is_offline.load(Ordering::Relaxed)
 	}
@@ -315,6 +316,7 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>> SyncProvider<B> for Servi
 	fn is_major_syncing(&self) -> bool {
 		self.is_major_syncing()
 	}
+
 	/// Get sync status
 	fn status(&self) -> mpsc::UnboundedReceiver<ProtocolStatus<B>> {
 		let (sink, stream) = mpsc::unbounded();

substrate/core/network/src/test/mod.rs

@@ -39,15 +39,15 @@ use crate::consensus_gossip::ConsensusGossip;
 use crossbeam_channel::{self as channel, Sender, select};
 use futures::Future;
 use futures::sync::{mpsc, oneshot};
-use crate::message::{Message, ConsensusEngineId};
+use crate::message::Message;
 use network_libp2p::PeerId;
 use parity_codec::Encode;
 use parking_lot::{Mutex, RwLock};
 use primitives::{H256, ed25519::Public as AuthorityId};
 use crate::protocol::{ConnectedPeer, Context, FromNetworkMsg, Protocol, ProtocolMsg};
 use runtime_primitives::generic::BlockId;
 use runtime_primitives::traits::{AuthorityIdFor, Block as BlockT, Digest, DigestItem, Header, NumberFor};
-use runtime_primitives::Justification;
+use runtime_primitives::{Justification, ConsensusEngineId};
 use crate::service::{network_channel, NetworkChan, NetworkLink, NetworkMsg, NetworkPort, TransactionPool};
 use crate::specialization::NetworkSpecialization;
 use test_client::{self, AccountKeyring};
@@ -260,11 +260,13 @@ impl<D, S: NetworkSpecialization<Block> + Clone> Peer<D, S> {
 	}
 
 	// SyncOracle: are we connected to any peer?
+	#[cfg(test)]
 	fn is_offline(&self) -> bool {
 		self.is_offline.load(Ordering::Relaxed)
 	}
 
 	// SyncOracle: are we in the process of catching-up with the chain?
+	#[cfg(test)]
 	fn is_major_syncing(&self) -> bool {
 		self.is_major_syncing.load(Ordering::Relaxed)
 	}
@@ -644,7 +646,6 @@ pub trait TestNetFactory: Sized {
 					Some(NetworkMsg::ReportPeer(who, _)) => {
 						to_disconnect.insert(who);
 					}
-					Some(_msg) => continue,
 				}
 			}
 			for d in to_disconnect {

substrate/core/primitives/src/crypto.rs

@@ -414,7 +414,7 @@ mod tests {
 		fn derive<Iter: Iterator<Item=DeriveJunction>>(&self, _path: Iter) -> Result<Self, Self::DeriveError> {
 			Err(())
 		}
-		fn from_seed(_seed: Self::Seed) -> Self { TestPair::Seed(vec![]) }
+		fn from_seed(_seed: <TestPair as Pair>::Seed) -> Self { TestPair::Seed(vec![]) }
 		fn sign(&self, _message: &[u8]) -> Self::Signature { () }
 		fn verify<P: AsRef<Self::Public>, M: AsRef<[u8]>>(_sig: &Self::Signature, _message: M, _pubkey: P) -> bool { true }
 		fn verify_weak<P: AsRef<[u8]>, M: AsRef<[u8]>>(_sig: &[u8], _message: M, _pubkey: P) -> bool { true }

substrate/core/primitives/src/ed25519.rs

@@ -523,7 +523,7 @@ impl Pair {
 mod test {
 	use super::*;
 	use hex_literal::{hex, hex_impl};
-	use crate::{Pair as PairT, crypto::DEV_PHRASE};
+	use crate::crypto::DEV_PHRASE;
 
 	#[test]
 	fn default_phrase_should_be_used() {