[author-inherent] Move kick_off_authorship_validation() to post_inherents()

pallets/author-inherent/Cargo.toml

@@ -13,9 +13,8 @@ frame-system = { workspace = true }
 log = { workspace = true }
 nimbus-primitives = { workspace = true }
 scale-info = { workspace = true }
-sp-api = { workspace = true }
 sp-application-crypto = { workspace = true }
-sp-inherents = { workspace = true }
+sp-core = { workspace = true }
 sp-runtime = { workspace = true }
 sp-std = { workspace = true }
 
@@ -24,7 +23,6 @@ frame-benchmarking = { workspace = true, optional = true }
 
 [dev-dependencies]
 frame-support-test = { workspace = true }
-sp-core = { workspace = true }
 sp-io = { workspace = true }
 
 [features]
@@ -37,9 +35,8 @@ std = [
 	"nimbus-primitives/std",
 	"parity-scale-codec/std",
 	"scale-info/std",
-	"sp-api/std",
 	"sp-application-crypto/std",
-	"sp-inherents/std",
+	"sp-core/std",
 	"sp-runtime/std",
 	"sp-std/std",
 ]

pallets/author-inherent/src/lib.rs

@@ -14,32 +14,21 @@
 // You should have received a copy of the GNU General Public License
 // along with Moonkit.  If not, see <http://www.gnu.org/licenses/>.
 
-//! Pallet that allows block authors to include their identity in a block via an inherent.
-//! Currently the author does not _prove_ their identity, just states it. So it should not be used,
-//! for things like equivocation slashing that require authenticated authorship information.
+//! Pallet that extracts the block author identity from the pre-runtime digest and validates
+//! eligibility via a `PostInherents` hook. Currently the author does not _prove_ their identity,
+//! just states it. So it should not be used for things like equivocation slashing that require
+//! authenticated authorship information.
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-extern crate alloc;
-
-use alloc::string::String;
 use frame_support::traits::{FindAuthor, Get};
-use nimbus_primitives::{
-	AccountLookup, CanAuthor, NimbusId, SlotBeacon, INHERENT_IDENTIFIER, NIMBUS_ENGINE_ID,
-};
-use parity_scale_codec::{Decode, Encode, FullCodec};
-use sp_inherents::{InherentIdentifier, IsFatalError};
+use nimbus_primitives::{AccountLookup, CanAuthor, NimbusId, SlotBeacon, NIMBUS_ENGINE_ID};
+use parity_scale_codec::{Decode, FullCodec};
 use sp_runtime::ConsensusEngineId;
 
-pub use crate::weights::WeightInfo;
 pub use exec::BlockExecutor;
 pub use pallet::*;
 
-#[cfg(any(test, feature = "runtime-benchmarks"))]
-mod benchmarks;
-
-pub mod weights;
-
 mod exec;
 
 #[cfg(test)]
@@ -76,8 +65,6 @@ pub mod pallet {
 
 		/// Some way of determining the current slot for purposes of verifying the author's eligibility
 		type SlotBeacon: SlotBeacon;
-
-		type WeightInfo: WeightInfo;
 	}
 
 	impl<T> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
@@ -98,84 +85,17 @@ pub mod pallet {
 	#[pallet::storage]
 	pub type Author<T: Config> = StorageValue<_, T::AuthorId, OptionQuery>;
 
-	/// Check if the inherent was included
-	#[pallet::storage]
-	pub type InherentIncluded<T: Config> = StorageValue<_, bool, ValueQuery>;
-
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
-		fn on_initialize(_: BlockNumberFor<T>) -> Weight {
-			// Now extract the author from the digest
-			let digest = <frame_system::Pallet<T>>::digest();
-			let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime());
-			if let Some(author) = Self::find_author(pre_runtime_digests) {
-				// Store the author so we can confirm eligibility after the inherents have executed
-				<Author<T>>::put(&author);
-			}
-
-			// on_initialize: 1 write
-			// on_finalize: 1 read + 1 write
-			T::DbWeight::get().reads_writes(1, 2)
-		}
-		fn on_finalize(_: BlockNumberFor<T>) {
-			// According to parity, the only way to ensure that a mandatory inherent is included
-			// is by checking on block finalization that the inherent set a particular storage item:
-			// https://github.com/paritytech/polkadot-sdk/issues/2841#issuecomment-1876040854
-			assert!(
-				InherentIncluded::<T>::take(),
-				"Block invalid, missing inherent `kick_off_authorship_validation`"
-			);
-		}
-	}
-
-	#[pallet::call]
-	impl<T: Config> Pallet<T> {
-		/// This inherent is a workaround to run code after the "real" inherents have executed,
-		/// but before transactions are executed.
-		// This should go into on_post_inherents when it is ready https://github.com/paritytech/substrate/pull/10128
-		// TODO better weight. For now we just set a somewhat conservative fudge factor
-		#[pallet::call_index(0)]
-		#[pallet::weight((T::WeightInfo::kick_off_authorship_validation(), DispatchClass::Mandatory))]
-		pub fn kick_off_authorship_validation(origin: OriginFor<T>) -> DispatchResultWithPostInfo {
-			ensure_none(origin)?;
-
-			// First check that the slot number is valid (greater than the previous highest)
-			let new_slot = T::SlotBeacon::slot();
-
-			// Now check that the author is valid in this slot
-			assert!(
-				T::CanAuthor::can_author(&Self::get(), &new_slot),
-				"Block invalid, supplied author is not eligible."
-			);
-
-			InherentIncluded::<T>::put(true);
-
-			Ok(Pays::No.into())
-		}
-	}
-
-	#[pallet::inherent]
-	impl<T: Config> ProvideInherent for Pallet<T> {
-		type Call = Call<T>;
-		type Error = InherentError;
-		const INHERENT_IDENTIFIER: InherentIdentifier = INHERENT_IDENTIFIER;
-
-		fn is_inherent_required(_: &InherentData) -> Result<Option<Self::Error>, Self::Error> {
-			// Return Ok(Some(_)) unconditionally because this inherent is required in every block
-			// If it is not found, throw an AuthorInherentRequired error.
-			Ok(Some(InherentError::Other(String::from(
-				"Inherent required to manually initiate author validation",
-			))))
-		}
-
-		// Regardless of whether the client is still supplying the author id,
-		// we will create the new empty-payload inherent extrinsic.
-		fn create_inherent(_data: &InherentData) -> Option<Self::Call> {
-			Some(Call::kick_off_authorship_validation {})
-		}
-
-		fn is_inherent(call: &Self::Call) -> bool {
-			matches!(call, Call::kick_off_authorship_validation { .. })
+		fn on_initialize(_n: BlockNumberFor<T>) -> Weight {
+			// Clear author from previous block so that no pallet reads stale data
+			// before post_inherents() sets it for this block.
+			Author::<T>::kill();
+			// Pre-charge for `post_inherents()` which cannot return weight.
+			// This is mandatory per-block overhead so only DB ops matter:
+			//   reads:  AccountLookup + SlotBeacon + CanAuthor (1-3 reads)
+			//   writes: kill + Author::put (2 writes)
+			T::DbWeight::get().reads_writes(3, 2)
 		}
 	}
 
@@ -230,28 +150,21 @@ pub mod pallet {
 	}
 }
 
-#[derive(Encode)]
-#[cfg_attr(feature = "std", derive(Debug, Decode))]
-pub enum InherentError {
-	Other(String),
-}
-
-impl IsFatalError for InherentError {
-	fn is_fatal_error(&self) -> bool {
-		match *self {
-			InherentError::Other(_) => true,
-		}
-	}
-}
-
-impl InherentError {
-	/// Try to create an instance ouf of the given identifier and data.
-	#[cfg(feature = "std")]
-	pub fn try_from(id: &InherentIdentifier, data: &[u8]) -> Option<Self> {
-		if id == &INHERENT_IDENTIFIER {
-			<InherentError as parity_scale_codec::Decode>::decode(&mut &data[..]).ok()
+impl<T: Config> frame_support::traits::PostInherents for Pallet<T> {
+	fn post_inherents() {
+		// Extract the author from the digest
+		let digest = <frame_system::Pallet<T>>::digest();
+		let pre_runtime_digests = digest.logs.iter().filter_map(|d| d.as_pre_runtime());
+		if let Some(author) = Self::find_author(pre_runtime_digests) {
+			let slot = T::SlotBeacon::slot();
+			// Check that the author is eligible for this slot
+			assert!(
+				T::CanAuthor::can_author(&author, &slot),
+				"Block invalid, supplied author is not eligible."
+			);
+			<Author<T>>::put(&author);
 		} else {
-			None
+			panic!("Block invalid, missing author in pre-runtime digest");
 		}
 	}
 }

pallets/author-inherent/src/mock.rs

@@ -30,7 +30,7 @@ frame_support::construct_runtime!(
 	pub enum Test
 	{
 		System: frame_system::{Pallet, Call, Config<T>, Storage, Event<T>},
-		AuthorInherent: pallet_testing::{Pallet, Call, Storage},
+		AuthorInherent: pallet_testing::{Pallet, Storage},
 	}
 );
 
@@ -72,38 +72,48 @@ impl frame_system::Config for Test {
 	type SingleBlockMigrations = ();
 	type MultiBlockMigrator = ();
 	type PreInherents = ();
-	type PostInherents = ();
+	type PostInherents = AuthorInherent;
 	type PostTransactions = ();
 }
 
 pub struct DummyBeacon {}
 impl nimbus_primitives::SlotBeacon for DummyBeacon {
 	fn slot() -> u32 {
-		0
+		System::block_number() as u32 + 1
 	}
 }
 
 pub const ALICE: u64 = 1;
 pub const ALICE_NIMBUS: [u8; 32] = [1; 32];
+pub const BOB: u64 = 99;
+pub const BOB_NIMBUS: [u8; 32] = [2; 32];
 pub struct MockAccountLookup;
 impl AccountLookup<u64> for MockAccountLookup {
 	fn lookup_account(nimbus_id: &NimbusId) -> Option<u64> {
 		let nimbus_id_bytes: &[u8] = nimbus_id.as_ref();
 
 		if nimbus_id_bytes == ALICE_NIMBUS {
 			Some(ALICE)
+		} else if nimbus_id_bytes == BOB_NIMBUS {
+			Some(BOB)
 		} else {
 			None
 		}
 	}
 }
 
+pub struct TestCanAuthor;
+impl nimbus_primitives::CanAuthor<u64> for TestCanAuthor {
+	fn can_author(author: &u64, slot: &u32) -> bool {
+		Authors::get().contains(author) && slot > &0
+	}
+}
+
 impl pallet_testing::Config for Test {
 	type AuthorId = u64;
 	type AccountLookup = MockAccountLookup;
-	type CanAuthor = ();
+	type CanAuthor = TestCanAuthor;
 	type SlotBeacon = DummyBeacon;
-	type WeightInfo = ();
 }
 
 /// Build genesis storage according to the mock runtime.