Add runtime support for PreRuntime and Consensus digests (#2757)

* Try to fix runtime panic

Does not work

* Fix trivial typo

* Add runtime support for `PreRuntime` and `Consensus` digests

Fixes c7d1204ce5

* Fix silly compile error.

* Fix overly-long lines

Also remove some in-progress code that would not wind up being useful
anyway.

* Respond to review comments

* delete `unset RUSTC_WRAPPER` from scripts/common.sh
* delete unnecessary `use aura::AURA_ENGINE_ID` from
  `node/runtime/src/lib.rs`
* add comments explaining why `PreRuntime` and `Consensus` must be
  special-cased in `core/sr-primitives/lib.rs`
* switch to using `$crate::rstd::marker::PhantomData` in
  `impl_outer_log!`
* improve documentation of `DigestItem::Seal`

* Fix compilation and add proof that we do not panic

Also fix some warnings.

* Apply suggestions from code review

Mostly for readability

Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>

* Apply suggestions from code review

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

* $crate::rstd::marker::PhantomData → Default::default()

The import is still needed, as `Default::default()` can’t be used in
patterns.

* Bump `spec_version`

Also do some reformatting.

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

@@ -74,13 +74,14 @@ pub enum DigestItem<Hash, AuthorityId, SealSignature> {
 	/// be generated by the native code of any consensus engine, but this is not
 	/// checked (yet).
 	Consensus(ConsensusEngineId, Vec<u8>),
-	/// Put a Seal on it.
+	/// Put a Seal on it. This is only used by native code, and is never seen
+	/// by runtimes.
 	Seal(ConsensusEngineId, SealSignature),
 	/// A pre-runtime digest.
 	///
 	/// These are messages from the consensus engine to the runtime, although
 	/// the consensus engine can (and should) read them itself to avoid
-	/// code and state duplication.  It is erroneous for a runtime to produce
+	/// code and state duplication. It is erroneous for a runtime to produce
 	/// these, but this is not (yet) checked.
 	PreRuntime(ConsensusEngineId, Vec<u8>),
 	/// Any 'non-system' digest item, opaque to the native code.
@@ -110,7 +111,8 @@ pub enum DigestItemRef<'a, Hash: 'a, AuthorityId: 'a, SealSignature: 'a> {
 	/// be generated by the native code of any consensus engine, but this is not
 	/// checked (yet).
 	Consensus(&'a ConsensusEngineId, &'a Vec<u8>),
-	/// Put a Seal on it.
+	/// Put a Seal on it. This is only used by native code, and is never seen
+	/// by runtimes.
 	Seal(&'a ConsensusEngineId, &'a SealSignature),
 	/// A pre-runtime digest.
 	///

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

@@ -25,6 +25,8 @@ pub use parity_codec as codec;
 #[cfg(feature = "std")]
 #[doc(hidden)]
 pub use serde;
+#[doc(hidden)]
+pub use rstd;
 
 #[cfg(feature = "std")]
 pub use runtime_io::{StorageOverlay, ChildrenStorageOverlay};
@@ -341,8 +343,8 @@ impl ::rstd::ops::Deref for PerU128 {
 	type Target = u128;
 
 	fn deref(&self) -> &u128 {
-        &self.0
+		&self.0
-    }
+	}
 }
 
 impl codec::CompactAs for PerU128 {
@@ -611,6 +613,69 @@ macro_rules! impl_outer_config {
 	}
 }
 
+// NOTE [`PreRuntime` and `Consensus` are special]
+//
+// We MUST treat `PreRuntime` and `Consensus` variants specially, as they:
+//
+// * have more parameters (both in `generic::DigestItem` and in runtimes)
+// * have a `PhantomData` parameter in the runtime, but not in `generic::DigestItem`
+
+#[macro_export]
+#[doc(hidden)]
+macro_rules! __parse_pattern_2 {
+	(PreRuntime $module:ident $internal:ident $v1:ident $v2:ident) => {
+		$internal::$module($module::RawLog::PreRuntime(ref $v1, ref $v2, $crate::rstd::marker::PhantomData))
+	};
+	(Consensus $module:ident $internal:ident $v1:ident $v2:ident) => {
+		$internal::$module($module::RawLog::Consensus(ref $v1, ref $v2, $crate::rstd::marker::PhantomData))
+	};
+	($name:ident $module:ident $internal:ident $v1:ident $v2:ident) => {
+		$internal::$module($module::RawLog::$name(ref $v1))
+	};
+}
+
+#[macro_export]
+#[doc(hidden)]
+macro_rules! __parse_pattern {
+	(PreRuntime $engine_id:pat, $binder:pat) => {
+		$crate::generic::DigestItem::PreRuntime($engine_id, $binder)
+	};
+	(Consensus $engine_id:pat, $binder:pat) => {
+		$crate::generic::DigestItem::Consensus($engine_id, $binder)
+	};
+	($name:ident $engine_id:pat, $binder:pat) => {
+		$crate::generic::DigestItem::$name($binder)
+	};
+}
+
+#[macro_export]
+#[doc(hidden)]
+macro_rules! __parse_expr {
+	(PreRuntime $engine_id:expr, $module:ident $internal:ident $binder:expr) => {
+		$internal::$module($module::RawLog::PreRuntime($engine_id, $binder, Default::default()))
+	};
+	(Consensus $engine_id:expr, $module:ident $internal:ident $binder:expr) => {
+		$internal::$module($module::RawLog::Consensus($engine_id, $binder, Default::default()))
+	};
+	($name:ident $engine_id:expr, $module:ident $internal:ident $binder:expr) => {
+		$internal::$module($module::RawLog::$name($binder))
+	};
+}
+
+#[macro_export]
+#[doc(hidden)]
+macro_rules! __parse_expr_2 {
+	(PreRuntime $module:ident $internal:ident $v1:ident $v2:ident) => {
+		$crate::generic::DigestItemRef::PreRuntime($v1, $v2)
+	};
+	(Consensus $module:ident $internal:ident $v1:ident $v2:ident) => {
+		$crate::generic::DigestItemRef::Consensus($v1, $v2)
+	};
+	($name:ident $module:ident $internal:ident $v1:ident $v2:ident) => {
+		$crate::generic::DigestItemRef::$name($v1)
+	};
+}
+
 /// Generates enum that contains all possible log entries for the runtime.
 /// Every individual module of the runtime that is mentioned, must
 /// expose a `Log` and `RawLog` enums.
@@ -631,7 +696,7 @@ macro_rules! impl_outer_log {
 	(
 		$(#[$attr:meta])*
 		pub enum $name:ident ($internal:ident: DigestItem<$( $genarg:ty ),*>) for $trait:ident {
-			$( $module:ident $(<$instance:path>)? ( $( $sitem:ident ),* ) ),*
+			$( $module:ident $(<$instance:path>)? ( $( $sitem:tt ),* ) ),*
 		}
 	) => {
 		/// Wrapper for all possible log entries for the `$trait` runtime. Provides binary-compatible
@@ -650,7 +715,7 @@ macro_rules! impl_outer_log {
 		#[allow(non_camel_case_types)]
 		pub enum InternalLog {
 			$(
-				$module($module::Log<$trait $(, $instance)? >),
+				$module($module::Log <$trait $(, $instance)?>),
 			)*
 		}
 
@@ -662,8 +727,8 @@ macro_rules! impl_outer_log {
 			fn dref<'a>(&'a self) -> Option<$crate::generic::DigestItemRef<'a, $($genarg),*>> {
 				match self.0 {
 					$($(
-					$internal::$module($module::RawLog::$sitem(ref v)) =>
+					$crate::__parse_pattern_2!($sitem $module $internal a b) =>
-						Some($crate::generic::DigestItemRef::$sitem(v)),
+						Some($crate::__parse_expr_2!($sitem $module $internal a b)),
 					)*)*
 					_ => None,
 				}
@@ -688,22 +753,31 @@ macro_rules! impl_outer_log {
 		}
 
 		impl From<$crate::generic::DigestItem<$($genarg),*>> for $name {
-			/// Converts `generic::DigestItem` into `$name`. If `generic::DigestItem` represents
+			/// Converts `generic::DigestItem` into `$name`. If
-			/// a system item which is supported by the runtime, it is returned.
+			/// `generic::DigestItem` represents a system item which is
-			/// Otherwise we expect a `Other` log item. Trying to convert from anything other
+			/// supported by the runtime, it is returned. Otherwise we expect a
-			/// will lead to panic in runtime, since the runtime does not supports this 'system'
+			/// `Other`, `PreDigest`, or `Consensus` log item. Trying to convert
-			/// log item.
+			/// from anything else will lead to panic at runtime, since the
+			/// runtime does not supports this 'system' log item.
 			#[allow(unreachable_patterns)]
 			fn from(gen: $crate::generic::DigestItem<$($genarg),*>) -> Self {
 				match gen {
 					$($(
-					$crate::generic::DigestItem::$sitem(value) =>
+					$crate::__parse_pattern!($sitem b, a) =>
-						$name($internal::$module($module::RawLog::$sitem(value))),
+						$name($crate::__parse_expr!($sitem b, $module $internal a)),
 					)*)*
-					_ => gen.as_other()
+					_ => {
-						.and_then(|value| $crate::codec::Decode::decode(&mut &value[..]))
+						if let Some(s) = gen.as_other()
-						.map($name)
+							.and_then(|value| $crate::codec::Decode::decode(&mut &value[..]))
-						.expect("not allowed to fail in runtime"),
+							.map($name)
+						{
+							s
+						} else {
+							panic!("we only reach here if the runtime did not handle a digest; \
+									runtimes are required to handle all digests they receive; qed"
+									)
+						}
+					}
 				}
 			}
 		}
@@ -732,16 +806,16 @@ macro_rules! impl_outer_log {
 		}
 
 		$(
-			impl From<$module::Log<$trait $(, $instance)? >> for $name {
+			impl From<$module::Log<$trait $(, $instance)?>> for $name {
 				/// Converts single module log item into `$name`.
 				fn from(x: $module::Log<$trait $(, $instance)? >) -> Self {
 					$name(x.into())
 				}
 			}
 
-			impl From<$module::Log<$trait $(, $instance)? >> for InternalLog {
+			impl From<$module::Log<$trait $(, $instance)?>> for InternalLog {
 				/// Converts single module log item into `$internal`.
-				fn from(x: $module::Log<$trait $(, $instance)? >) -> Self {
+				fn from(x: $module::Log<$trait $(, $instance)?>) -> Self {
 					InternalLog::$module(x)
 				}
 			}

substrate/core/sr-sandbox/without_std.rs

@@ -15,7 +15,7 @@
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
 use rstd::prelude::*;
-use rstd::{slice, marker, mem};
+use rstd::{slice, marker, mem, vec};
 use rstd::rc::Rc;
 use codec::{Decode, Encode};
 use primitives::sandbox as sandbox_primitives;

substrate/node-template/runtime/src/lib.rs

@@ -200,7 +200,7 @@ construct_runtime!(
 		System: system::{default, Log(ChangesTrieRoot)},
 		Timestamp: timestamp::{Module, Call, Storage, Config<T>, Inherent},
 		Consensus: consensus::{Module, Call, Storage, Config<T>, Log(AuthoritiesChange), Inherent},
-		Aura: aura::{Module},
+		Aura: aura::{Module, Log(PreRuntime)},
 		Indices: indices,
 		Balances: balances,
 		Sudo: sudo,

substrate/node/runtime/src/lib.rs

@@ -58,8 +58,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("node"),
 	impl_name: create_runtime_str!("substrate-node"),
 	authoring_version: 10,
-	spec_version: 90,
+	spec_version: 91,
-	impl_version: 91,
+	impl_version: 92,
 	apis: RUNTIME_API_VERSIONS,
 };
 
@@ -227,7 +227,7 @@ construct_runtime!(
 		UncheckedExtrinsic = UncheckedExtrinsic
 	{
 		System: system::{default, Log(ChangesTrieRoot)},
-		Aura: aura::{Module, Inherent(Timestamp)},
+		Aura: aura::{Module, Inherent(Timestamp), Log(PreRuntime)},
 		Timestamp: timestamp::{Module, Call, Storage, Config<T>, Inherent},
 		Consensus: consensus::{Module, Call, Storage, Config<T>, Log(AuthoritiesChange), Inherent},
 		Indices: indices,

substrate/srml/aura/src/lib.rs

@@ -51,15 +51,19 @@
 pub use timestamp;
 
 use rstd::{result, prelude::*};
+use parity_codec::{Encode, Decode};
 use srml_support::storage::StorageValue;
 use srml_support::{decl_storage, decl_module};
 use primitives::traits::{SaturatedConversion, Saturating, Zero, One};
 use timestamp::OnTimestampSet;
+use rstd::marker::PhantomData;
 #[cfg(feature = "std")]
 use timestamp::TimestampInherentData;
 use inherents::{RuntimeString, InherentIdentifier, InherentData, ProvideInherent, MakeFatalError};
 #[cfg(feature = "std")]
 use inherents::{InherentDataProviders, ProvideInherentData};
+#[cfg(feature = "std")]
+use serde::Serialize;
 
 mod mock;
 mod tests;
@@ -89,6 +93,20 @@ impl AuraInherentData for InherentData {
 	}
 }
 
+/// Logs in this module.
+pub type Log<T> = RawLog<T>;
+
+/// Logs in this module.
+///
+/// The type parameter distinguishes logs belonging to two different runtimes,
+/// which should not be mixed.
+#[cfg_attr(feature = "std", derive(Serialize, Debug))]
+#[derive(Encode, Decode, PartialEq, Eq, Clone)]
+pub enum RawLog<T> {
+	/// AuRa inherent digests
+	PreRuntime([u8; 4], Vec<u8>, PhantomData<T>),
+}
+
 /// Provides the slot duration inherent data for `Aura`.
 #[cfg(feature = "std")]
 pub struct InherentDataProvider {

substrate/srml/babe/src/lib.rs

@@ -17,19 +17,21 @@
 //! Consensus extension module for BABE consensus.
 
 #![cfg_attr(not(feature = "std"), no_std)]
-#![forbid(unsafe_code, warnings)]
+#![forbid(unsafe_code)]
 pub use timestamp;
 
-use rstd::{result, prelude::*};
+use rstd::{result, prelude::*, marker::PhantomData};
 use srml_support::{decl_storage, decl_module};
 use timestamp::{OnTimestampSet, Trait};
 use primitives::traits::{SaturatedConversion, Saturating};
 #[cfg(feature = "std")]
 use timestamp::TimestampInherentData;
-use parity_codec::Decode;
+use parity_codec::{Encode, Decode};
 use inherents::{RuntimeString, InherentIdentifier, InherentData, ProvideInherent, MakeFatalError};
 #[cfg(feature = "std")]
 use inherents::{InherentDataProviders, ProvideInherentData};
+#[cfg(feature = "std")]
+use serde::Serialize;
 
 /// The BABE inherent identifier.
 pub const INHERENT_IDENTIFIER: InherentIdentifier = *b"babeslot";
@@ -56,6 +58,20 @@ impl BabeInherentData for InherentData {
 	}
 }
 
+/// Logs in this module.
+pub type Log<T> = RawLog<T>;
+
+/// Logs in this module.
+///
+/// The type parameter distinguishes logs belonging to two different runtimes,
+/// which should not be mixed.
+#[cfg_attr(feature = "std", derive(Serialize, Debug))]
+#[derive(Encode, Decode, PartialEq, Eq, Clone)]
+pub enum RawLog<T> {
+	/// BABE inherent digests
+	PreRuntime([u8; 4], Vec<u8>, PhantomData<T>),
+}
+
 /// Provides the slot duration inherent data for BABE.
 #[cfg(feature = "std")]
 pub struct InherentDataProvider {

substrate/srml/consensus/src/lib.rs

@@ -221,6 +221,7 @@ impl<T: OnOfflineReport<Vec<u32>>> InherentOfflineReport for InstantFinalityRepo
 	}
 }
 
+/// Logs in this module.
 pub type Log<T> = RawLog<
 	<T as Trait>::SessionKey,
 >;

substrate/srml/contract/src/account_db.rs

@@ -177,10 +177,10 @@ impl<T: Trait> AccountDb<T> for DirectAccountDb {
 }
 pub struct OverlayAccountDb<'a, T: Trait + 'a> {
 	local: RefCell<ChangeSet<T>>,
-	underlying: &'a AccountDb<T>,
+	underlying: &'a dyn AccountDb<T>,
 }
 impl<'a, T: Trait> OverlayAccountDb<'a, T> {
-	pub fn new(underlying: &'a AccountDb<T>) -> OverlayAccountDb<'a, T> {
+	pub fn new(underlying: &'a dyn AccountDb<T>) -> OverlayAccountDb<'a, T> {
 		OverlayAccountDb {
 			local: RefCell::new(ChangeSet::new()),
 			underlying,