Moved DigestItem impl to impl_outer_log (#932)

* moved DigestItem impl to impl_outer_log

* Enable backtrace collecting on the CI

* internal module is not required in impl_outer_log

* Revert "internal module is not required in impl_outer_log"

This reverts commit 5430446971cdf5fd5735863b86cd08a11d35d1dd.

* Revert "moved DigestItem impl to impl_outer_log"

This reverts commit 8872bbee963dbb4316251222f5c0757303fb3f52.

* Revert "Revert "moved DigestItem impl to impl_outer_log""

This reverts commit 47afd59682f8828665bd9cfb293b40ce4c4787af.

* Revert "Revert "internal module is not required in impl_outer_log""

This reverts commit 9ae640010c00ef134099d5b6a4430d330be134b2.

* More diagnostics

* check if only 1 test fails

* test is back + some "traces"

* removed some traces

* removed "traces"

substrate/.gitlab-ci.yml

@@ -32,7 +32,7 @@ variables:
     when:                          on_success
     expire_in:                     1 mos
     paths:
-    - target/release/polkadot
+    - target/release/substrate
 
 .determine_version: &determine_version |
   export VERSION=$(grep -m 1 "version =" Cargo.toml | awk '{print $3}' | tr -d '"' | tr -d "\n")
@@ -49,6 +49,7 @@ test:rust:stable:                  &test
   script:
     - ./scripts/init.sh
     - export PATH="${CI_PROJECT_DIR}/cargo/bin/:$PATH"
+    - export RUST_BACKTRACE=1
     - ./scripts/build.sh
     - time cargo test --all --release --locked
   tags:

substrate/core/service/test/src/lib.rs

@@ -185,7 +185,7 @@ pub fn connectivity<F: ServiceFactory>(spec: FactoryChainSpec<F>) {
 		{
 			let mut network = TestNet::<F>::new(&temp, spec.clone(), NUM_NODES, 0, vec![], 30400);
 			info!("Checking star topology");
-			let first_address = network.full_nodes[0].1.network().node_id().unwrap();
+			let first_address = network.full_nodes[0].1.network().node_id().expect("No node address");
 			for (_, service) in network.full_nodes.iter().skip(1) {
 				service.network().add_reserved_peer(first_address.clone()).expect("Error adding reserved peer");
 			}
@@ -200,10 +200,10 @@ pub fn connectivity<F: ServiceFactory>(spec: FactoryChainSpec<F>) {
 		{
 			let mut network = TestNet::<F>::new(&temp, spec, NUM_NODES as u32, 0, vec![], 30400);
 			info!("Checking linked topology");
-			let mut address = network.full_nodes[0].1.network().node_id().unwrap();
+			let mut address = network.full_nodes[0].1.network().node_id().expect("No node address");
 			for (_, service) in network.full_nodes.iter().skip(1) {
 				service.network().add_reserved_peer(address.clone()).expect("Error adding reserved peer");
-				address = service.network().node_id().unwrap();
+				address = service.network().node_id().expect("No node address");
 			}
 			network.run_until_all_full(|_index, service| {
 				service.network().status().num_peers == NUM_NODES as usize - 1

substrate/core/sr-primitives/src/generic/digest.rs

@@ -113,17 +113,11 @@ impl<Hash: Codec + Member, AuthorityId: Codec + Member> traits::DigestItem for D
 	type AuthorityId = AuthorityId;
 
 	fn as_authorities_change(&self) -> Option<&[Self::AuthorityId]> {
-		match *self {
-			DigestItem::AuthoritiesChange(ref authorities) => Some(authorities),
-			_ => None,
-		}
+		self.dref().as_authorities_change()
 	}
 
 	fn as_changes_trie_root(&self) -> Option<&Hash> {
-		match *self {
-			DigestItem::ChangesTrieRoot(ref changes_trie_root) => Some(changes_trie_root),
-			_ => None,
-		}
+		self.dref().as_changes_trie_root()
 	}
 }
 
@@ -150,6 +144,22 @@ impl<Hash: Decode, AuthorityId: Decode> Decode for DigestItem<Hash, AuthorityId>
 	}
 }
 
+impl<'a, Hash: Codec + Member, AuthorityId: Codec + Member> DigestItemRef<'a, Hash, AuthorityId> {
+	pub fn as_authorities_change(&self) -> Option<&'a [AuthorityId]> {
+		match *self {
+			DigestItemRef::AuthoritiesChange(ref authorities) => Some(authorities),
+			_ => None,
+		}
+	}
+
+	pub fn as_changes_trie_root(&self) -> Option<&'a Hash> {
+		match *self {
+			DigestItemRef::ChangesTrieRoot(ref changes_trie_root) => Some(changes_trie_root),
+			_ => None,
+		}
+	}
+}
+
 impl<'a, Hash: Encode, AuthorityId: Encode> Encode for DigestItemRef<'a, Hash, AuthorityId> {
 	fn encode(&self) -> Vec<u8> {
 		let mut v = Vec::new();

substrate/core/sr-primitives/src/lib.rs

@@ -326,7 +326,7 @@ macro_rules! impl_outer_log {
 	(
 		$(#[$attr:meta])*
 		pub enum $name:ident ($internal:ident: DigestItem<$( $genarg:ty ),*>) for $trait:ident {
-			$( $module:ident($( $item:ident ),*) ),*
+			$( $module:ident( $( $sitem:ident ),* ) ),*
 		}
 	) => {
 		/// Wrapper for all possible log entries for the `$trait` runtime. Provides binary-compatible
@@ -343,7 +343,7 @@ macro_rules! impl_outer_log {
 		#[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))]
 		$(#[$attr])*
 		#[allow(non_camel_case_types)]
-		enum $internal {
+		pub enum InternalLog {
 			$(
 				$module($module::Log<$trait>),
 			)*
@@ -357,14 +357,27 @@ macro_rules! impl_outer_log {
 			fn dref<'a>(&'a self) -> Option<$crate::generic::DigestItemRef<'a, $($genarg),*>> {
 				match self.0 {
 					$($(
-					$internal::$module($module::RawLog::$item(ref v)) =>
-						Some($crate::generic::DigestItemRef::$item(v)),
+					$internal::$module($module::RawLog::$sitem(ref v)) =>
+						Some($crate::generic::DigestItemRef::$sitem(v)),
 					)*)*
 					_ => None,
 				}
 			}
 		}
 
+		impl $crate::traits::DigestItem for $name {
+			type Hash = <$crate::generic::DigestItem<$($genarg),*> as $crate::traits::DigestItem>::Hash;
+			type AuthorityId = <$crate::generic::DigestItem<$($genarg),*> as $crate::traits::DigestItem>::AuthorityId;
+
+			fn as_authorities_change(&self) -> Option<&[Self::AuthorityId]> {
+				self.dref().and_then(|dref| dref.as_authorities_change())
+			}
+
+			fn as_changes_trie_root(&self) -> Option<&Self::Hash> {
+				self.dref().and_then(|dref| dref.as_changes_trie_root())
+			}
+		}
+
 		impl From<$crate::generic::DigestItem<$($genarg),*>> for $name {
 			/// Converts `generic::DigestItem` into `$name`. If `generic::DigestItem` represents
 			/// a system item which is supported by the runtime, it is returned.
@@ -375,8 +388,8 @@ macro_rules! impl_outer_log {
 			fn from(gen: $crate::generic::DigestItem<$($genarg),*>) -> Self {
 				match gen {
 					$($(
-					$crate::generic::DigestItem::$item(value) =>
-						$name($internal::$module($module::RawLog::$item(value))),
+					$crate::generic::DigestItem::$sitem(value) =>
+						$name($internal::$module($module::RawLog::$sitem(value))),
 					)*)*
 					_ => gen.as_other()
 						.and_then(|value| $crate::codec::Decode::decode(&mut &value[..]))
@@ -417,10 +430,10 @@ macro_rules! impl_outer_log {
 				}
 			}
 
-			impl From<$module::Log<$trait>> for $internal {
+			impl From<$module::Log<$trait>> for InternalLog {
 				/// Converts single module log item into `$internal`.
 				fn from(x: $module::Log<$trait>) -> Self {
-					$internal::$module(x)
+					InternalLog::$module(x)
 				}
 			}
 		)*
@@ -431,6 +444,7 @@ macro_rules! impl_outer_log {
 mod tests {
 	use substrate_primitives::hash::H256;
 	use codec::{Encode as EncodeHidden, Decode as DecodeHidden};
+	use traits::DigestItem;
 
 	pub trait RuntimeT {
 		type AuthorityId;
@@ -442,31 +456,31 @@ mod tests {
 		type AuthorityId = u64;
 	}
 
+	mod a {
+		use super::RuntimeT;
+		pub type Log<R> = RawLog<<R as RuntimeT>::AuthorityId>;
+
+		#[derive(Serialize, Deserialize, Debug, Encode, Decode, PartialEq, Eq, Clone)]
+		pub enum RawLog<AuthorityId> { A1(AuthorityId), AuthoritiesChange(Vec<AuthorityId>), A3(AuthorityId) }
+	}
+
+	mod b {
+		use super::RuntimeT;
+		pub type Log<R> = RawLog<<R as RuntimeT>::AuthorityId>;
+
+		#[derive(Serialize, Deserialize, Debug, Encode, Decode, PartialEq, Eq, Clone)]
+		pub enum RawLog<AuthorityId> { B1(AuthorityId), B2(AuthorityId) }
+	}
+
+	// TODO try to avoid redundant brackets: a(AuthoritiesChange), b
+	impl_outer_log! {
+		pub enum Log(InternalLog: DigestItem<H256, u64>) for Runtime {
+			a(AuthoritiesChange), b()
+		}
+	}
+
 	#[test]
 	fn impl_outer_log_works() {
-		mod a {
-			use super::RuntimeT;
-			pub type Log<R> = RawLog<<R as RuntimeT>::AuthorityId>;
-
-			#[derive(Serialize, Deserialize, Debug, Encode, Decode, PartialEq, Eq, Clone)]
-			pub enum RawLog<AuthorityId> { A1(AuthorityId), AuthoritiesChange(Vec<AuthorityId>), A3(AuthorityId) }
-		}
-
-		mod b {
-			use super::RuntimeT;
-			pub type Log<R> = RawLog<<R as RuntimeT>::AuthorityId>;
-
-			#[derive(Serialize, Deserialize, Debug, Encode, Decode, PartialEq, Eq, Clone)]
-			pub enum RawLog<AuthorityId> { B1(AuthorityId), B2(AuthorityId) }
-		}
-
-		// TODO try to avoid redundant brackets: a(AuthoritiesChange), b
-		impl_outer_log! {
-			pub enum Log(InternalLog: DigestItem<H256, u64>) for Runtime {
-				a(AuthoritiesChange), b()
-			}
-		}
-
 		// encode/decode regular item
 		let b1: Log = b::RawLog::B1::<u64>(777).into();
 		let encoded_b1 = b1.encode();
@@ -492,5 +506,11 @@ mod tests {
 			super::generic::DigestItem::AuthoritiesChange::<H256, u64>(authorities) => assert_eq!(authorities, vec![100, 200, 300]),
 			_ => panic!("unexpected generic_auth_change: {:?}", generic_auth_change),
 		}
+
+		// check that as-style methods are working with system items
+		assert!(auth_change.as_authorities_change().is_some());
+
+		// check that as-style methods are not working with regular items
+		assert!(b1.as_authorities_change().is_none());
 	}
 }

substrate/core/sr-primitives/src/traits.rs

@@ -534,14 +534,10 @@ pub trait DigestItem: Codec + Member {
 	type AuthorityId: Member;
 
 	/// Returns Some if the entry is the `AuthoritiesChange` entry.
-	fn as_authorities_change(&self) -> Option<&[Self::AuthorityId]> {
-		None
-	}
+	fn as_authorities_change(&self) -> Option<&[Self::AuthorityId]>;
 
 	/// Returns Some if the entry is the `ChangesTrieRoot` entry.
-	fn as_changes_trie_root(&self) -> Option<&Self::Hash> {
-		None
-	}
+	fn as_changes_trie_root(&self) -> Option<&Self::Hash>;
 }
 
 /// Something that provides an inherent for a runtime.

substrate/node/runtime/src/lib.rs

@@ -40,7 +40,6 @@ extern crate substrate_primitives;
 #[macro_use]
 extern crate parity_codec_derive;
 
-#[cfg_attr(not(feature = "std"), macro_use)]
 extern crate sr_std as rstd;
 extern crate srml_balances as balances;
 extern crate srml_consensus as consensus;
@@ -67,7 +66,7 @@ use runtime_api::runtime::*;
 use runtime_primitives::ApplyResult;
 use runtime_primitives::transaction_validity::TransactionValidity;
 use runtime_primitives::generic;
-use runtime_primitives::traits::{Convert, BlakeTwo256, DigestItem, Block as BlockT};
+use runtime_primitives::traits::{Convert, BlakeTwo256, Block as BlockT};
 use version::{RuntimeVersion, ApiId};
 use council::{motions as council_motions, voting as council_voting};
 #[cfg(feature = "std")]
@@ -192,25 +191,6 @@ impl contract::Trait for Runtime {
 	type Event = Event;
 }
 
-impl DigestItem for Log {
-	type Hash = Hash;
-	type AuthorityId = SessionKey;
-
-	fn as_authorities_change(&self) -> Option<&[Self::AuthorityId]> {
-		match self.0 {
-			InternalLog::consensus(ref item) => item.as_authorities_change(),
-			_ => None,
-		}
-	}
-
-	fn as_changes_trie_root(&self) -> Option<&Self::Hash> {
-		match self.0 {
-			InternalLog::system(ref item) => item.as_changes_trie_root(),
-			_ => None,
-		}
-	}
-}
-
 construct_runtime!(
 	pub enum Runtime with Log(InternalLog: DigestItem<Hash, SessionKey>) where
 		Block = Block,