From ed7300b4068552095f05da03adf41ba35d8beeaf Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Fri, 13 Oct 2023 00:31:23 -0400
Subject: [PATCH] Explicitly provide a pre_dispatch which calls
 validate_unsigned

pre_dispatch is guaranteed by documentation to be called and persisted.
validate_unsigned is not, though the provided pre_dispatch does by default call
validate_unsigned. By explicitly providing our own pre_dispatch, we accomplish
the bounds we require and expect, only being invalidated on Substrate
redefining their API.

We should still test this, yet since we call retire_session in
validate_unsigned, any test of rotation will test it's being properly called.
---
 substrate/in-instructions/pallet/src/lib.rs |  7 +++++--
 substrate/validator-sets/pallet/src/lib.rs  | 11 +++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/substrate/in-instructions/pallet/src/lib.rs b/substrate/in-instructions/pallet/src/lib.rs
index 4c1adb2b..4bb943ac 100644
--- a/substrate/in-instructions/pallet/src/lib.rs
+++ b/substrate/in-instructions/pallet/src/lib.rs
@@ -108,8 +108,6 @@ pub mod pallet {
 
       let batch = batch.batch;
 
-      // TODO: Test validate_unsigned is actually called prior to execution, which is required for
-      // this to be safe
       LastBatchBlock::<T>::insert(batch.network, frame_system::Pallet::<T>::block_number());
 
       LastBatch::<T>::insert(batch.network, batch.id);
@@ -204,6 +202,11 @@ pub mod pallet {
         .propagate(true)
         .build()
     }
+
+    // Explicitly provide a pre-dispatch which calls validate_unsigned
+    fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
+      Self::validate_unsigned(TransactionSource::InBlock, call).map(|_| ()).map_err(Into::into)
+    }
   }
 }
 
diff --git a/substrate/validator-sets/pallet/src/lib.rs b/substrate/validator-sets/pallet/src/lib.rs
index 90563944..1d38bbb5 100644
--- a/substrate/validator-sets/pallet/src/lib.rs
+++ b/substrate/validator-sets/pallet/src/lib.rs
@@ -364,11 +364,13 @@ pub mod pallet {
     ) -> DispatchResult {
       ensure_none(origin)?;
 
+      // signature isn't checked as this is an unsigned transaction, and validate_unsigned
+      // (called by pre_dispatch) checks it
+      let _ = signature;
+
       let session = Session(pallet_session::Pallet::<T>::current_index());
 
       let set = ValidatorSet { session, network };
-      // TODO: Is this needed? validate_unsigned should be called before this and ensure it's Ok
-      Self::verify_signature(set, &key_pair, &signature)?;
 
       Keys::<T>::set(set, Some(key_pair.clone()));
       Self::deposit_event(Event::KeyGen { set, key_pair });
@@ -412,6 +414,11 @@ pub mod pallet {
         .propagate(true)
         .build()
     }
+
+    // Explicitly provide a pre-dispatch which calls validate_unsigned
+    fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
+      Self::validate_unsigned(TransactionSource::InBlock, call).map(|_| ()).map_err(Into::into)
+    }
   }
 
   impl<T: Config> Pallet<T> {