Multi-phase elections solution resubmission (#8290)

* not climate

* explain the intent of the bool in the unsigned phase

* remove glob imports from unsigned.rs

* add OffchainRepeat parameter to ElectionProviderMultiPhase

* migrate core logic from #7976

This is a much smaller diff than that PR contained, but I think
it contains all the essentials.

* improve formatting

* fix test build failures

* cause test to pass

* Apply suggestions from code review

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* collapse imports

* threshold acquired directly within try_acquire_offchain_lock

* add test of resubmission after interval

* add test that ocw can regenerate a failed cache when resubmitting

* ensure that OCW solutions are of the correct round

This should help prevent stale cached solutions from persisting
past the election for which they are intended.

* add test of pre-dispatch round check

* use `RawSolution.round` instead of redundantly externally

* unpack imports

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* rename `OFFCHAIN_HEAD_DB` -> `OFFCHAIN_LOCK`

* rename `mine_call` -> `mine_checked_call`

* eliminate extraneous comma

* check cached call is current before submitting

* remove unused consts introduced by bad merge.

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

* resubmit when our solution beats queued solution

* clear call cache if solution fails to submit

* use local storage; clear on ElectionFinalized

* Revert "use local storage; clear on ElectionFinalized"

This reverts commit 4b46a9388532d0c09b337dc7c7edf76044a6cee8.

* BROKEN: try to filter local events in OCW

* use local storage; simplify score fetching

* fix event filter

* mutate storage instead of setting it

* StorageValueRef::local isn't actually implemented yet

* add logging for some events of interest in OCW miner

* rename kill_solution -> kill_ocw_solution to avoid ambiguity

* defensive err instead of unreachable given unreachable code

* doc punctuation

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* distinguish miner errors between "out of date" and "call invalid"

* downgrade info logs -> debug

* ensure encoded call decodes as a call

* fix semantics of validation of pre-dispatch failure for wrong round

* move score check within `and_then`

* add test that offchain workers clear their cache after election

* ensure that bad ocw submissions are not retained for resubmission

* simplify fn ocw_solution_exists

* add feasibility check when restoring cached solution

should address https://github.com/paritytech/substrate/pull/8290/files#r617533358

restructures how the checks are sequenced, which simplifies legibility.

* simplify checks again

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

substrate/bin/node/runtime/src/lib.rs

@@ -471,6 +471,7 @@ parameter_types! {
 	pub const SlashDeferDuration: pallet_staking::EraIndex = 24 * 7; // 1/4 the bonding duration.
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
 	pub const MaxNominatorRewardedPerValidator: u32 = 256;
+	pub OffchainRepeat: BlockNumber = 5;
 }
 
 impl pallet_staking::Config for Runtime {
@@ -542,6 +543,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
 	type SignedPhase = SignedPhase;
 	type UnsignedPhase = UnsignedPhase;
 	type SolutionImprovementThreshold = SolutionImprovementThreshold;
+	type OffchainRepeat = OffchainRepeat;
 	type MinerMaxIterations = MinerMaxIterations;
 	type MinerMaxWeight = MinerMaxWeight;
 	type MinerMaxLength = MinerMaxLength;

substrate/frame/election-provider-multi-phase/src/lib.rs

@@ -194,10 +194,6 @@
 //! **Score based on (byte) size**: We should always prioritize small solutions over bigger ones, if
 //! there is a tie. Even more harsh should be to enforce the bound of the `reduce` algorithm.
 //!
-//! **Offchain resubmit**: Essentially port <https://github.com/paritytech/substrate/pull/7976> to
-//! this pallet as well. The `OFFCHAIN_REPEAT` also needs to become an adjustable parameter of the
-//! pallet.
-//!
 //! **Make the number of nominators configurable from the runtime**. Remove `sp_npos_elections`
 //! dependency from staking and the compact solution type. It should be generated at runtime, there
 //! it should be encoded how many votes each nominators have. Essentially translate
@@ -224,7 +220,7 @@ use frame_support::{
 use frame_system::{ensure_none, offchain::SendTransactionTypes};
 use frame_election_provider_support::{ElectionDataProvider, ElectionProvider, onchain};
 use sp_npos_elections::{
-	assignment_ratio_to_staked_normalized, is_score_better, CompactSolution, ElectionScore,
+	assignment_ratio_to_staked_normalized, CompactSolution, ElectionScore,
 	EvaluateSupport, PerThing128, Supports, VoteWeight,
 };
 use sp_runtime::{
@@ -235,7 +231,10 @@ use sp_runtime::{
 	DispatchError, PerThing, Perbill, RuntimeDebug, SaturatedConversion,
 	traits::Bounded,
 };
-use sp_std::prelude::*;
+use sp_std::{
+	convert::TryInto,
+	prelude::*,
+};
 use sp_arithmetic::{
 	UpperOf,
 	traits::{Zero, CheckedAdd},
@@ -304,8 +303,16 @@ pub enum Phase<Bn> {
 	Off,
 	/// Signed phase is open.
 	Signed,
-	/// Unsigned phase. First element is whether it is open or not, second the starting block
+	/// Unsigned phase. First element is whether it is active or not, second the starting block
 	/// number.
+	///
+	/// We do not yet check whether the unsigned phase is active or passive. The intent is for the
+	/// blockchain to be able to declare: "I believe that there exists an adequate signed solution,"
+	/// advising validators not to bother running the unsigned offchain worker.
+	///
+	/// As validator nodes are free to edit their OCW code, they could simply ignore this advisory
+	/// and always compute their own solution. However, by default, when the unsigned phase is passive,
+	/// the offchain workers will not bother running.
 	Unsigned((bool, Bn)),
 }
 
@@ -316,27 +323,27 @@ impl<Bn> Default for Phase<Bn> {
 }
 
 impl<Bn: PartialEq + Eq> Phase<Bn> {
-	/// Weather the phase is signed or not.
+	/// Whether the phase is signed or not.
 	pub fn is_signed(&self) -> bool {
 		matches!(self, Phase::Signed)
 	}
 
-	/// Weather the phase is unsigned or not.
+	/// Whether the phase is unsigned or not.
 	pub fn is_unsigned(&self) -> bool {
 		matches!(self, Phase::Unsigned(_))
 	}
 
-	/// Weather the phase is unsigned and open or not, with specific start.
+	/// Whether the phase is unsigned and open or not, with specific start.
 	pub fn is_unsigned_open_at(&self, at: Bn) -> bool {
 		matches!(self, Phase::Unsigned((true, real)) if *real == at)
 	}
 
-	/// Weather the phase is unsigned and open or not.
+	/// Whether the phase is unsigned and open or not.
 	pub fn is_unsigned_open(&self) -> bool {
 		matches!(self, Phase::Unsigned((true, _)))
 	}
 
-	/// Weather the phase is off or not.
+	/// Whether the phase is off or not.
 	pub fn is_off(&self) -> bool {
 		matches!(self, Phase::Off)
 	}
@@ -512,7 +519,7 @@ pub mod pallet {
 
 	#[pallet::config]
 	pub trait Config: frame_system::Config + SendTransactionTypes<Call<Self>> {
-		type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
+		type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event> + TryInto<Event<Self>>;
 
 		/// Currency type.
 		type Currency: ReservableCurrency<Self::AccountId> + Currency<Self::AccountId>;
@@ -529,6 +536,13 @@ pub mod pallet {
 		#[pallet::constant]
 		type SolutionImprovementThreshold: Get<Perbill>;
 
+		/// The repeat threshold of the offchain worker.
+		///
+		/// For example, if it is 5, that means that at least 5 blocks will elapse between attempts
+		/// to submit the worker's solution.
+		#[pallet::constant]
+		type OffchainRepeat: Get<Self::BlockNumber>;
+
 		/// The priority of the unsigned transaction submitted in the unsigned-phase
 		type MinerTxPriority: Get<TransactionPriority>;
 		/// Maximum number of iteration of balancing that will be executed in the embedded miner of
@@ -638,16 +652,38 @@ pub mod pallet {
 			}
 		}
 
-		fn offchain_worker(n: T::BlockNumber) {
-			// We only run the OCW in the first block of the unsigned phase.
-			if Self::current_phase().is_unsigned_open_at(n) {
-				match Self::try_acquire_offchain_lock(n) {
-					Ok(_) => {
-						let outcome = Self::mine_check_and_submit().map_err(ElectionError::from);
-						log!(info, "mine_check_and_submit execution done: {:?}", outcome);
-					}
-					Err(why) => log!(warn, "denied offchain worker: {:?}", why),
+		fn offchain_worker(now: T::BlockNumber) {
+			match Self::current_phase() {
+				Phase::Unsigned((true, opened)) if opened == now => {
+					// mine a new solution, cache it, and attempt to submit it
+					let initial_output = Self::try_acquire_offchain_lock(now)
+						.and_then(|_| Self::mine_check_save_submit());
+					log!(info, "initial OCW output at {:?}: {:?}", now, initial_output);
+				}
+				Phase::Unsigned((true, opened)) if opened < now => {
+					// keep trying to submit solutions. worst case, we note that the stored solution
+					// is better than our cached/computed one, and decide not to submit after all.
+					//
+					// the offchain_lock prevents us from spamming submissions too often.
+					let resubmit_output = Self::try_acquire_offchain_lock(now)
+						.and_then(|_| Self::restore_or_compute_then_maybe_submit());
+					log!(info, "resubmit OCW output at {:?}: {:?}", now, resubmit_output);
 				}
+				_ => {}
+			}
+			// after election finalization, clear OCW solution storage
+			if <frame_system::Pallet<T>>::events()
+				.into_iter()
+				.filter_map(|event_record| {
+					let local_event = <T as Config>::Event::from(event_record.event);
+					local_event.try_into().ok()
+				})
+				.find(|event| {
+					matches!(event, Event::ElectionFinalized(_))
+				})
+				.is_some()
+			{
+				unsigned::kill_ocw_solution::<T>();
 			}
 		}
 
@@ -784,6 +820,8 @@ pub mod pallet {
 		PreDispatchWrongWinnerCount,
 		/// Submission was too weak, score-wise.
 		PreDispatchWeakSubmission,
+		/// OCW submitted solution for wrong round
+		OcwCallWrongEra,
 	}
 
 	#[pallet::origin]

substrate/frame/election-provider-multi-phase/src/mock.rs

@@ -61,6 +61,7 @@ frame_support::construct_runtime!(
 
 pub(crate) type Balance = u64;
 pub(crate) type AccountId = u64;
+pub(crate) type BlockNumber = u32;
 pub(crate) type VoterIndex = u32;
 pub(crate) type TargetIndex = u16;
 
@@ -262,11 +263,11 @@ parameter_types! {
 	pub static MinerMaxIterations: u32 = 5;
 	pub static MinerTxPriority: u64 = 100;
 	pub static SolutionImprovementThreshold: Perbill = Perbill::zero();
+	pub static OffchainRepeat: BlockNumber = 5;
 	pub static MinerMaxWeight: Weight = BlockWeights::get().max_block;
 	pub static MinerMaxLength: u32 = 256;
 	pub static MockWeightInfo: bool = false;
 
-
 	pub static EpochLength: u64 = 30;
 }
 
@@ -334,6 +335,7 @@ impl crate::Config for Runtime {
 	type SignedPhase = SignedPhase;
 	type UnsignedPhase = UnsignedPhase;
 	type SolutionImprovementThreshold = SolutionImprovementThreshold;
+	type OffchainRepeat = OffchainRepeat;
 	type MinerMaxIterations = MinerMaxIterations;
 	type MinerMaxWeight = MinerMaxWeight;
 	type MinerMaxLength = MinerMaxLength;

substrate/frame/election-provider-multi-phase/src/unsigned.rs

@@ -17,22 +17,27 @@
 
 //! The unsigned phase implementation.
 
-use crate::*;
-use frame_support::dispatch::DispatchResult;
+use crate::{
+	helpers, Call, CompactAccuracyOf, CompactOf, Config,
+	ElectionCompute, Error, FeasibilityError, Pallet, RawSolution, ReadySolution, RoundSnapshot,
+	SolutionOrSnapshotSize, Weight, WeightInfo,
+};
+use codec::{Encode, Decode};
+use frame_support::{dispatch::DispatchResult, ensure, traits::Get};
 use frame_system::offchain::SubmitTransaction;
+use sp_arithmetic::Perbill;
 use sp_npos_elections::{
-	seq_phragmen, CompactSolution, ElectionResult, assignment_ratio_to_staked_normalized,
-	assignment_staked_to_ratio_normalized,
+	CompactSolution, ElectionResult, ElectionScore, assignment_ratio_to_staked_normalized,
+	assignment_staked_to_ratio_normalized, is_score_better, seq_phragmen,
 };
-use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput};
-use sp_std::{cmp::Ordering, convert::TryFrom};
+use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput, SaturatedConversion};
+use sp_std::{cmp::Ordering, convert::TryFrom, vec::Vec};
 
 /// Storage key used to store the persistent offchain worker status.
-pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-election";
+pub(crate) const OFFCHAIN_LOCK: &[u8] = b"parity/multi-phase-unsigned-election";
 
-/// The repeat threshold of the offchain worker. This means we won't run the offchain worker twice
-/// within a window of 5 blocks.
-pub(crate) const OFFCHAIN_REPEAT: u32 = 5;
+/// Storage key used to cache the solution `call`.
+pub(crate) const OFFCHAIN_CACHED_CALL: &[u8] = b"parity/multi-phase-unsigned-election/call";
 
 /// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they
 /// voted.
@@ -63,6 +68,16 @@ pub enum MinerError {
 	PreDispatchChecksFailed,
 	/// The solution generated from the miner is not feasible.
 	Feasibility(FeasibilityError),
+	/// Something went wrong fetching the lock.
+	Lock(&'static str),
+	/// Cannot restore a solution that was not stored.
+	NoStoredSolution,
+	/// Cached solution does not match the current round.
+	SolutionOutOfDate,
+	/// Cached solution is not a `submit_unsigned` call.
+	SolutionCallInvalid,
+	/// Failed to store a solution.
+	FailedToStoreSolution,
 	/// There are no more voters to remove to trim the solution.
 	NoMoreVoters,
 }
@@ -79,25 +94,142 @@ impl From<FeasibilityError> for MinerError {
 	}
 }
 
+/// Save a given call into OCW storage.
+fn save_solution<T: Config>(call: &Call<T>) -> Result<(), MinerError> {
+	let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL);
+	match storage.mutate::<_, (), _>(|_| Ok(call.clone())) {
+		Ok(Ok(_)) => Ok(()),
+		Ok(Err(_)) => Err(MinerError::FailedToStoreSolution),
+		Err(_) => {
+			// this branch should be unreachable according to the definition of `StorageValueRef::mutate`:
+			// that function should only ever `Err` if the closure we pass it return an error.
+			// however, for safety in case the definition changes, we do not optimize the branch away
+			// or panic.
+			Err(MinerError::FailedToStoreSolution)
+		},
+	}
+}
+
+/// Get a saved solution from OCW storage if it exists.
+fn restore_solution<T: Config>() -> Result<Call<T>, MinerError> {
+	StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL)
+		.get()
+		.flatten()
+		.ok_or(MinerError::NoStoredSolution)
+}
+
+/// Clear a saved solution from OCW storage.
+pub(super) fn kill_ocw_solution<T: Config>() {
+	let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL);
+	storage.clear();
+}
+
+/// `true` when OCW storage contains a solution
+///
+/// More precise than `restore_solution::<T>().is_ok()`; that invocation will return `false`
+/// if a solution exists but cannot be decoded, whereas this just checks whether an item is present.
+#[cfg(test)]
+fn ocw_solution_exists<T: Config>() -> bool {
+	StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL).get::<Call<T>>().is_some()
+}
+
 impl<T: Config> Pallet<T> {
-	/// Mine a new solution, and submit it back to the chain as an unsigned transaction.
-	pub fn mine_check_and_submit() -> Result<(), MinerError> {
+	/// Attempt to restore a solution from cache. Otherwise, compute it fresh. Either way, submit
+	/// if our call's score is greater than that of the cached solution.
+	pub fn restore_or_compute_then_maybe_submit() -> Result<(), MinerError> {
+		log!(
+			debug,
+			"OCW attempting to restore or compute an unsigned solution for the current election"
+		);
+
+		let call = restore_solution::<T>()
+			.and_then(|call| {
+				// ensure the cached call is still current before submitting
+				if let Call::submit_unsigned(solution, _) = &call {
+					// prevent errors arising from state changes in a forkful chain
+					Self::basic_checks(solution, "restored")?;
+					Ok(call)
+				} else {
+					Err(MinerError::SolutionCallInvalid)
+				}
+			})
+			.or_else::<MinerError, _>(|_| {
+				// if not present or cache invalidated, regenerate
+				let (call, _) = Self::mine_checked_call()?;
+				save_solution(&call)?;
+				Ok(call)
+			})?;
+
+		// the runtime will catch it and reject the transaction if the phase is wrong, but it's
+		// cheap and easy to check it here to ease the workload on the runtime, so:
+		if !Self::current_phase().is_unsigned_open() {
+			// don't bother submitting; it's not an error, we're just too late.
+			return Ok(());
+		}
+
+		// in case submission fails for any reason, `submit_call` kills the stored solution
+		Self::submit_call(call)
+	}
+
+	/// Mine a new solution, cache it, and submit it back to the chain as an unsigned transaction.
+	pub fn mine_check_save_submit() -> Result<(), MinerError> {
+		log!(
+			debug,
+			"OCW attempting to compute an unsigned solution for the current election"
+		);
+
+		let (call, _) = Self::mine_checked_call()?;
+		save_solution(&call)?;
+		Self::submit_call(call)
+	}
+
+	/// Mine a new solution as a call. Performs all checks.
+	fn mine_checked_call() -> Result<(Call<T>, ElectionScore), MinerError> {
 		let iters = Self::get_balancing_iters();
 		// get the solution, with a load of checks to ensure if submitted, IT IS ABSOLUTELY VALID.
 		let (raw_solution, witness) = Self::mine_and_check(iters)?;
+
 		let score = raw_solution.score.clone();
+		let call: Call<T> = Call::submit_unsigned(raw_solution, witness).into();
 
-		let call: <T as frame_system::offchain::SendTransactionTypes<Call<T>>>::OverarchingCall =
-			Call::submit_unsigned(raw_solution, witness).into();
 		log!(
-			info,
-			"mined a solution with score {:?} and size {}",
+			debug,
+			"OCW mined a solution with score {:?} and size {}",
 			score,
 			call.using_encoded(|b| b.len())
 		);
 
-		SubmitTransaction::<T, Call<T>>::submit_unsigned_transaction(call)
-			.map_err(|_| MinerError::PoolSubmissionFailed)
+		Ok((call, score))
+	}
+
+	fn submit_call(call: Call<T>) -> Result<(), MinerError> {
+		log!(
+			debug,
+			"OCW submitting a solution as an unsigned transaction",
+		);
+
+		SubmitTransaction::<T, Call<T>>::submit_unsigned_transaction(call.into())
+			.map_err(|_| {
+				kill_ocw_solution::<T>();
+				MinerError::PoolSubmissionFailed
+			})
+	}
+
+	// perform basic checks of a solution's validity
+	//
+	// Performance: note that it internally clones the provided solution.
+	fn basic_checks(raw_solution: &RawSolution<CompactOf<T>>, solution_type: &str) -> Result<(), MinerError> {
+		Self::unsigned_pre_dispatch_checks(raw_solution).map_err(|err| {
+			log!(warn, "pre-dispatch checks fialed for {} solution: {:?}", solution_type, err);
+			MinerError::PreDispatchChecksFailed
+		})?;
+
+		Self::feasibility_check(raw_solution.clone(), ElectionCompute::Unsigned).map_err(|err| {
+			log!(warn, "feasibility check failed for {} solution: {:?}", solution_type, err);
+			err
+		})?;
+
+		Ok(())
 	}
 
 	/// Mine a new npos solution, with all the relevant checks to make sure that it will be accepted
@@ -105,26 +237,12 @@ impl<T: Config> Pallet<T> {
 	///
 	/// If you want an unchecked solution, use [`Pallet::mine_solution`].
 	/// If you want a checked solution and submit it at the same time, use
-	/// [`Pallet::mine_check_and_submit`].
+	/// [`Pallet::mine_check_save_submit`].
 	pub fn mine_and_check(
 		iters: usize,
 	) -> Result<(RawSolution<CompactOf<T>>, SolutionOrSnapshotSize), MinerError> {
 		let (raw_solution, witness) = Self::mine_solution(iters)?;
-
-		// ensure that this will pass the pre-dispatch checks
-		Self::unsigned_pre_dispatch_checks(&raw_solution).map_err(|e| {
-			log!(warn, "pre-dispatch-checks failed for mined solution: {:?}", e);
-			MinerError::PreDispatchChecksFailed
-		})?;
-
-		// ensure that this is a feasible solution
-		let _ = Self::feasibility_check(raw_solution.clone(), ElectionCompute::Unsigned).map_err(
-			|e| {
-				log!(warn, "feasibility-check failed for mined solution: {:?}", e);
-				MinerError::from(e)
-			},
-		)?;
-
+		Self::basic_checks(&raw_solution, "mined")?;
 		Ok((raw_solution, witness))
 	}
 
@@ -439,12 +557,14 @@ impl<T: Config> Pallet<T> {
 	/// not.
 	///
 	/// This essentially makes sure that we don't run on previous blocks in case of a re-org, and we
-	/// don't run twice within a window of length [`OFFCHAIN_REPEAT`].
+	/// don't run twice within a window of length `threshold`.
 	///
 	/// Returns `Ok(())` if offchain worker should happen, `Err(reason)` otherwise.
-	pub(crate) fn try_acquire_offchain_lock(now: T::BlockNumber) -> Result<(), &'static str> {
-		let storage = StorageValueRef::persistent(&OFFCHAIN_HEAD_DB);
-		let threshold = T::BlockNumber::from(OFFCHAIN_REPEAT);
+	pub(crate) fn try_acquire_offchain_lock(
+		now: T::BlockNumber,
+	) -> Result<(), MinerError> {
+		let threshold = T::OffchainRepeat::get();
+		let storage = StorageValueRef::persistent(&OFFCHAIN_LOCK);
 
 		let mutate_stat =
 			storage.mutate::<_, &'static str, _>(|maybe_head: Option<Option<T::BlockNumber>>| {
@@ -468,9 +588,9 @@ impl<T: Config> Pallet<T> {
 			// all good
 			Ok(Ok(_)) => Ok(()),
 			// failed to write.
-			Ok(Err(_)) => Err("failed to write to offchain db."),
+			Ok(Err(_)) => Err(MinerError::Lock("failed to write to offchain db.")),
 			// fork etc.
-			Err(why) => Err(why),
+			Err(why) => Err(MinerError::Lock(why)),
 		}
 	}
 
@@ -487,6 +607,9 @@ impl<T: Config> Pallet<T> {
 		// ensure solution is timely. Don't panic yet. This is a cheap check.
 		ensure!(Self::current_phase().is_unsigned_open(), Error::<T>::PreDispatchEarlySubmission);
 
+		// ensure round is current
+		ensure!(Self::round() == solution.round, Error::<T>::OcwCallWrongEra);
+
 		// ensure correct number of winners.
 		ensure!(
 			Self::desired_targets().unwrap_or_default()
@@ -511,7 +634,8 @@ impl<T: Config> Pallet<T> {
 #[cfg(test)]
 mod max_weight {
 	#![allow(unused_variables)]
-	use super::{mock::*, *};
+	use super::*;
+	use crate::mock::MultiPhase;
 
 	struct TestWeight;
 	impl crate::weights::WeightInfo for TestWeight {
@@ -597,13 +721,15 @@ mod max_weight {
 mod tests {
 	use super::*;
 	use crate::{
+		CurrentPhase, InvalidTransaction, Phase, QueuedSolution, TransactionSource,
+		TransactionValidityError,
 		mock::{
-			assert_noop, assert_ok, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin,
-			roll_to_with_ocw, roll_to, Runtime, TestCompact, TrimHelpers, trim_helpers, witness,
+			Call as OuterCall, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin, Runtime,
+			TestCompact, TrimHelpers, roll_to, roll_to_with_ocw, trim_helpers, witness,
 		},
 	};
-	use frame_support::{dispatch::Dispatchable, traits::OffchainWorker};
-	use mock::Call as OuterCall;
+	use frame_benchmarking::Zero;
+	use frame_support::{assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker};
 	use sp_npos_elections::IndexAssignment;
 	use sp_runtime::{traits::ValidateUnsigned, PerU16};
 
@@ -845,7 +971,7 @@ mod tests {
 			assert!(MultiPhase::current_phase().is_unsigned());
 
 			assert_eq!(
-				MultiPhase::mine_check_and_submit().unwrap_err(),
+				MultiPhase::mine_check_save_submit().unwrap_err(),
 				MinerError::PreDispatchChecksFailed,
 			);
 		})
@@ -924,6 +1050,8 @@ mod tests {
 	fn ocw_check_prevent_duplicate() {
 		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
 		ext.execute_with(|| {
+			let offchain_repeat = <Runtime as Config>::OffchainRepeat::get();
+
 			roll_to(25);
 			assert!(MultiPhase::current_phase().is_unsigned());
 
@@ -931,21 +1059,15 @@ mod tests {
 			assert!(MultiPhase::try_acquire_offchain_lock(25).is_ok());
 
 			// next block: rejected.
-			assert_noop!(MultiPhase::try_acquire_offchain_lock(26), "recently executed.");
+			assert_noop!(MultiPhase::try_acquire_offchain_lock(26), MinerError::Lock("recently executed."));
 
 			// allowed after `OFFCHAIN_REPEAT`
-			assert!(MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT).into()).is_ok());
+			assert!(MultiPhase::try_acquire_offchain_lock((26 + offchain_repeat).into()).is_ok());
 
 			// a fork like situation: re-execute last 3.
-			assert_noop!(
-				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 3).into()), "fork."
-			);
-			assert_noop!(
-				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 2).into()), "fork."
-			);
-			assert_noop!(
-				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 1).into()), "fork."
-			);
+			assert!(MultiPhase::try_acquire_offchain_lock((26 + offchain_repeat - 3).into()).is_err());
+			assert!(MultiPhase::try_acquire_offchain_lock((26 + offchain_repeat - 2).into()).is_err());
+			assert!(MultiPhase::try_acquire_offchain_lock((26 + offchain_repeat - 1).into()).is_err());
 		})
 	}
 
@@ -958,19 +1080,131 @@ mod tests {
 
 			// we must clear the offchain storage to ensure the offchain execution check doesn't get
 			// in the way.
-			let mut storage = StorageValueRef::persistent(&OFFCHAIN_HEAD_DB);
+			let mut storage = StorageValueRef::persistent(&OFFCHAIN_LOCK);
 
 			MultiPhase::offchain_worker(24);
 			assert!(pool.read().transactions.len().is_zero());
 			storage.clear();
 
+			// creates, caches, submits without expecting previous cache value
+			MultiPhase::offchain_worker(25);
+			assert_eq!(pool.read().transactions.len(), 1);
+			// assume that the tx has been processed
+			pool.try_write().unwrap().transactions.clear();
+
+			// locked, but also, has previously cached.
 			MultiPhase::offchain_worker(26);
 			assert!(pool.read().transactions.len().is_zero());
+		})
+	}
+
+	#[test]
+	fn ocw_clears_cache_after_election() {
+		let (mut ext, _pool) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			roll_to(25);
+			assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));
+
+			// we must clear the offchain storage to ensure the offchain execution check doesn't get
+			// in the way.
+			let mut storage = StorageValueRef::persistent(&OFFCHAIN_LOCK);
 			storage.clear();
 
-			// submits!
+			assert!(!ocw_solution_exists::<Runtime>(), "no solution should be present before we mine one");
+
+			// creates and cache a solution
 			MultiPhase::offchain_worker(25);
-			assert!(!pool.read().transactions.len().is_zero());
+			assert!(ocw_solution_exists::<Runtime>(), "a solution must be cached after running the worker");
+
+			// after an election, the solution must be cleared
+			// we don't actually care about the result of the election
+			roll_to(26);
+			let _ = MultiPhase::do_elect();
+			MultiPhase::offchain_worker(26);
+			assert!(!ocw_solution_exists::<Runtime>(), "elections must clear the ocw cache");
+		})
+	}
+
+	#[test]
+	fn ocw_resubmits_after_offchain_repeat() {
+		let (mut ext, pool) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			const BLOCK: u64 = 25;
+			let block_plus = |delta: i32| ((BLOCK as i32) + delta) as u64;
+			let offchain_repeat = <Runtime as Config>::OffchainRepeat::get();
+
+			roll_to(BLOCK);
+			assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, BLOCK)));
+
+			// we must clear the offchain storage to ensure the offchain execution check doesn't get
+			// in the way.
+			let mut storage = StorageValueRef::persistent(&OFFCHAIN_LOCK);
+
+			MultiPhase::offchain_worker(block_plus(-1));
+			assert!(pool.read().transactions.len().is_zero());
+			storage.clear();
+
+			// creates, caches, submits without expecting previous cache value
+			MultiPhase::offchain_worker(BLOCK);
+			assert_eq!(pool.read().transactions.len(), 1);
+			let tx_cache = pool.read().transactions[0].clone();
+			// assume that the tx has been processed
+			pool.try_write().unwrap().transactions.clear();
+
+			// attempts to resubmit the tx after the threshold has expired
+			// note that we have to add 1: the semantics forbid resubmission at
+			// BLOCK + offchain_repeat
+			MultiPhase::offchain_worker(block_plus(1 + offchain_repeat as i32));
+			assert_eq!(pool.read().transactions.len(), 1);
+
+			// resubmitted tx is identical to first submission
+			let tx = &pool.read().transactions[0];
+			assert_eq!(&tx_cache, tx);
+		})
+	}
+
+	#[test]
+	fn ocw_regenerates_and_resubmits_after_offchain_repeat() {
+		let (mut ext, pool) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			const BLOCK: u64 = 25;
+			let block_plus = |delta: i32| ((BLOCK as i32) + delta) as u64;
+			let offchain_repeat = <Runtime as Config>::OffchainRepeat::get();
+
+			roll_to(BLOCK);
+			assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, BLOCK)));
+
+			// we must clear the offchain storage to ensure the offchain execution check doesn't get
+			// in the way.
+			let mut storage = StorageValueRef::persistent(&OFFCHAIN_LOCK);
+
+			MultiPhase::offchain_worker(block_plus(-1));
+			assert!(pool.read().transactions.len().is_zero());
+			storage.clear();
+
+			// creates, caches, submits without expecting previous cache value
+			MultiPhase::offchain_worker(BLOCK);
+			assert_eq!(pool.read().transactions.len(), 1);
+			let tx_cache = pool.read().transactions[0].clone();
+			// assume that the tx has been processed
+			pool.try_write().unwrap().transactions.clear();
+
+			// remove the cached submitted tx
+			// this ensures that when the resubmit window rolls around, we're ready to regenerate
+			// from scratch if necessary
+			let mut call_cache = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL);
+			assert!(matches!(call_cache.get::<Call<Runtime>>(), Some(Some(_call))));
+			call_cache.clear();
+
+			// attempts to resubmit the tx after the threshold has expired
+			// note that we have to add 1: the semantics forbid resubmission at
+			// BLOCK + offchain_repeat
+			MultiPhase::offchain_worker(block_plus(1 + offchain_repeat as i32));
+			assert_eq!(pool.read().transactions.len(), 1);
+
+			// resubmitted tx is identical to first submission
+			let tx = &pool.read().transactions[0];
+			assert_eq!(&tx_cache, tx);
 		})
 	}
 
@@ -985,7 +1219,38 @@ mod tests {
 			let encoded = pool.read().transactions[0].clone();
 			let extrinsic: Extrinsic = Decode::decode(&mut &*encoded).unwrap();
 			let call = extrinsic.call;
-			assert!(matches!(call, OuterCall::MultiPhase(Call::submit_unsigned(_, _))));
+			assert!(matches!(call, OuterCall::MultiPhase(Call::submit_unsigned(..))));
+		})
+	}
+
+	#[test]
+	fn ocw_solution_must_have_correct_round() {
+		let (mut ext, pool) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			roll_to_with_ocw(25);
+			assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));
+			// OCW must have submitted now
+			// now, before we check the call, update the round
+			<crate::Round<Runtime>>::mutate(|round| *round += 1);
+
+			let encoded = pool.read().transactions[0].clone();
+			let extrinsic = Extrinsic::decode(&mut &*encoded).unwrap();
+			let call = match extrinsic.call {
+				OuterCall::MultiPhase(call @ Call::submit_unsigned(..)) => call,
+				_ => panic!("bad call: unexpected submission"),
+			};
+
+			// Custom(3) maps to PreDispatchChecksFailed
+			let pre_dispatch_check_error = TransactionValidityError::Invalid(InvalidTransaction::Custom(3));
+			assert_eq!(
+				<MultiPhase as ValidateUnsigned>::validate_unsigned(TransactionSource::Local, &call)
+					.unwrap_err(),
+				pre_dispatch_check_error,
+			);
+			assert_eq!(
+				<MultiPhase as ValidateUnsigned>::pre_dispatch(&call).unwrap_err(),
+				pre_dispatch_check_error,
+			);
 		})
 	}