Companion for EPM duplicate submissions (#6115)

* make it work

* add migration

* fix

* Update utils/staking-miner/src/opts.rs

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* Update utils/staking-miner/src/monitor.rs

* small tweaks

* Update utils/staking-miner/src/opts.rs

Co-authored-by: Bastian Köcher <info@kchr.de>

* fmt

* fix print'

* fmt

* update lockfile for {"substrate"}

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
Co-authored-by: parity-processbot <>

polkadot/runtime/kusama/src/lib.rs

@@ -1478,6 +1478,7 @@ pub type Executive = frame_executive::Executive<
 		pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
 		// "Properly migrate weights to v2" <https://github.com/paritytech/polkadot/pull/6091>
 		parachains_configuration::migration::v3::MigrateToV3<Runtime>,
+		pallet_election_provider_multi_phase::migrations::v1::MigrateToV1<Runtime>,
 	),
 >;
 /// The payload being signed in the transactions.

polkadot/runtime/polkadot/src/lib.rs

@@ -1571,6 +1571,7 @@ pub type Executive = frame_executive::Executive<
 		pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
 		// "Properly migrate weights to v2" <https://github.com/paritytech/polkadot/pull/6091>
 		parachains_configuration::migration::v3::MigrateToV3<Runtime>,
+		pallet_election_provider_multi_phase::migrations::v1::MigrateToV1<Runtime>,
 	),
 >;
 

polkadot/runtime/westend/src/lib.rs

@@ -1222,6 +1222,7 @@ pub type Executive = frame_executive::Executive<
 		pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
 		// "Properly migrate weights to v2" <https://github.com/paritytech/polkadot/pull/6091>
 		parachains_configuration::migration::v3::MigrateToV3<Runtime>,
+		pallet_election_provider_multi_phase::migrations::v1::MigrateToV1<Runtime>,
 	),
 >;
 /// The payload being signed in transactions.

polkadot/utils/staking-miner/src/main.rs

@@ -253,6 +253,7 @@ enum Error<T: EPM::Config> {
 	AlreadySubmitted,
 	VersionMismatch,
 	StrategyNotSatisfied,
+	QueueFull,
 	Other(String),
 }
 
@@ -425,6 +426,7 @@ fn mine_dpos<T: EPM::Config>(ext: &mut Ext) -> Result<(), Error<T>> {
 
 pub(crate) async fn check_versions<T: frame_system::Config + EPM::Config>(
 	rpc: &SharedRpcClient,
+	print: bool,
 ) -> Result<(), Error<T>> {
 	let linked_version = T::Version::get();
 	let on_chain_version = rpc
@@ -432,10 +434,31 @@ pub(crate) async fn check_versions<T: frame_system::Config + EPM::Config>(
 		.await
 		.expect("runtime version RPC should always work; qed");
 
-	log::debug!(target: LOG_TARGET, "linked version {:?}", linked_version);
-	log::debug!(target: LOG_TARGET, "on-chain version {:?}", on_chain_version);
+	let do_print = || {
+		log::info!(
+			target: LOG_TARGET,
+			"linked version {:?}",
+			(&linked_version.spec_name, &linked_version.spec_version)
+		);
+		log::info!(
+			target: LOG_TARGET,
+			"on-chain version {:?}",
+			(&on_chain_version.spec_name, &on_chain_version.spec_version)
+		);
+	};
 
-	if linked_version != on_chain_version {
+	if print {
+		do_print();
+	}
+
+	// we relax the checking here a bit, which should not cause any issues in production (a chain
+	// that messes up its spec name is highly unlikely), but it allows us to do easier testing.
+	if linked_version.spec_name != on_chain_version.spec_name ||
+		linked_version.spec_version != on_chain_version.spec_version
+	{
+		if !print {
+			do_print();
+		}
 		log::error!(
 			target: LOG_TARGET,
 			"VERSION MISMATCH: any transaction will fail with bad-proof"
@@ -563,7 +586,7 @@ async fn main() {
 	log::info!(target: LOG_TARGET, "connected to chain {:?}", chain);
 
 	any_runtime_unit! {
-		check_versions::<Runtime>(&rpc).await
+		check_versions::<Runtime>(&rpc, true).await
 	};
 
 	let outcome = any_runtime! {

polkadot/utils/staking-miner/src/monitor.rs

@@ -64,7 +64,7 @@ where
 		.map_err::<Error<T>, _>(Into::into)?
 		.unwrap_or_default();
 
-	for (_score, idx) in indices {
+	for (_score, _bn, idx) in indices {
 		let key = StorageKey(EPM::SignedSubmissionsMap::<T>::hashed_key_for(idx));
 
 		if let Some(submission) = rpc
@@ -81,20 +81,36 @@ where
 	Ok(())
 }
 
+/// `true` if `our_score` should pass the onchain `best_score` with the given strategy.
+pub(crate) fn score_passes_strategy(
+	our_score: sp_npos_elections::ElectionScore,
+	best_score: sp_npos_elections::ElectionScore,
+	strategy: SubmissionStrategy,
+) -> bool {
+	match strategy {
+		SubmissionStrategy::Always => true,
+		SubmissionStrategy::IfLeading =>
+			our_score == best_score ||
+				our_score.strict_threshold_better(best_score, Perbill::zero()),
+		SubmissionStrategy::ClaimBetterThan(epsilon) =>
+			our_score.strict_threshold_better(best_score, epsilon),
+		SubmissionStrategy::ClaimNoWorseThan(epsilon) =>
+			!best_score.strict_threshold_better(our_score, epsilon),
+	}
+}
+
 /// Reads all current solutions and checks the scores according to the `SubmissionStrategy`.
-async fn ensure_no_better_solution<T: EPM::Config, B: BlockT>(
+async fn ensure_strategy_met<T: EPM::Config, B: BlockT>(
 	rpc: &SharedRpcClient,
 	at: Hash,
 	score: sp_npos_elections::ElectionScore,
 	strategy: SubmissionStrategy,
 	max_submissions: u32,
 ) -> Result<(), Error<T>> {
-	let epsilon = match strategy {
-		// don't care about current scores.
-		SubmissionStrategy::Always => return Ok(()),
-		SubmissionStrategy::IfLeading => Perbill::zero(),
-		SubmissionStrategy::ClaimBetterThan(epsilon) => epsilon,
-	};
+	// don't care about current scores.
+	if matches!(strategy, SubmissionStrategy::Always) {
+		return Ok(())
+	}
 
 	let indices_key = StorageKey(EPM::SignedSubmissionIndices::<T>::hashed_key().to_vec());
 
@@ -104,34 +120,16 @@ async fn ensure_no_better_solution<T: EPM::Config, B: BlockT>(
 		.map_err::<Error<T>, _>(Into::into)?
 		.unwrap_or_default();
 
-	let mut is_best_score = true;
-	let mut scores = 0;
-
-	log::debug!(target: LOG_TARGET, "submitted solutions on chain: {:?}", indices);
-
-	// BTreeMap is ordered, take last to get the max score.
-	for (curr_max_score, _) in indices.into_iter() {
-		if !score.strict_threshold_better(curr_max_score, epsilon) {
-			log::warn!(target: LOG_TARGET, "mined score is not better; skipping to submit");
-			is_best_score = false;
-		}
-
-		if score == curr_max_score {
-			log::warn!(
-				target: LOG_TARGET,
-				"mined score has the same score as already submitted score"
-			);
-		}
-
-		// Indices can't be bigger than u32::MAX so can't overflow.
-		scores += 1;
+	// we check the queue here as well. Could be checked elsewhere.
+	if indices.len() as u32 >= max_submissions {
+		return Err(Error::<T>::QueueFull)
 	}
 
-	if scores == max_submissions {
-		log::warn!(target: LOG_TARGET, "The submissions queue is full");
-	}
+	// default score is all zeros, any score is better than it.
+	let best_score = indices.last().map(|(score, _, _)| *score).unwrap_or_default();
+	log::debug!(target: LOG_TARGET, "best onchain score is {:?}", best_score);
 
-	if is_best_score {
+	if score_passes_strategy(score, best_score, strategy) {
 		Ok(())
 	} else {
 		Err(Error::StrategyNotSatisfied)
@@ -233,7 +231,7 @@ macro_rules! monitor_cmd_for { ($runtime:tt) => { paste::paste! {
 
 			// block on this because if this fails there is no way to recover from
 			// that error i.e, upgrade/downgrade required.
-			if let Err(err) = crate::check_versions::<Runtime>(&rpc).await {
+			if let Err(err) = crate::check_versions::<Runtime>(&rpc, false).await {
 				let _ = tx.send(err.into());
 				return;
 			}
@@ -314,9 +312,14 @@ macro_rules! monitor_cmd_for { ($runtime:tt) => { paste::paste! {
 				}
 			};
 
-			let ensure_no_better_fut = tokio::spawn(async move {
-				ensure_no_better_solution::<Runtime, Block>(&rpc1, latest_head, score, config.submission_strategy,
-					SignedMaxSubmissions::get()).await
+			let ensure_strategy_met_fut = tokio::spawn(async move {
+				ensure_strategy_met::<Runtime, Block>(
+					&rpc1,
+					latest_head,
+					score,
+					config.submission_strategy,
+					SignedMaxSubmissions::get()
+				).await
 			});
 
 			let ensure_signed_phase_fut = tokio::spawn(async move {
@@ -325,7 +328,7 @@ macro_rules! monitor_cmd_for { ($runtime:tt) => { paste::paste! {
 
 			// Run the calls in parallel and return once all has completed or any failed.
 			if let Err(err) = tokio::try_join!(
-				flatten(ensure_no_better_fut),
+				flatten(ensure_strategy_met_fut),
 				flatten(ensure_signed_phase_fut),
 			) {
 				log::debug!(target: LOG_TARGET, "Skipping to submit at block {}; {}", at.number, err);
@@ -420,3 +423,46 @@ macro_rules! monitor_cmd_for { ($runtime:tt) => { paste::paste! {
 monitor_cmd_for!(polkadot);
 monitor_cmd_for!(kusama);
 monitor_cmd_for!(westend);
+
+#[cfg(test)]
+pub mod tests {
+	use super::*;
+
+	#[test]
+	fn score_passes_strategy_works() {
+		let s = |x| sp_npos_elections::ElectionScore { minimal_stake: x, ..Default::default() };
+		let two = Perbill::from_percent(2);
+
+		// anything passes Always
+		assert!(score_passes_strategy(s(0), s(0), SubmissionStrategy::Always));
+		assert!(score_passes_strategy(s(5), s(0), SubmissionStrategy::Always));
+		assert!(score_passes_strategy(s(5), s(10), SubmissionStrategy::Always));
+
+		// if leading
+		assert!(score_passes_strategy(s(0), s(0), SubmissionStrategy::IfLeading));
+		assert!(score_passes_strategy(s(1), s(0), SubmissionStrategy::IfLeading));
+		assert!(score_passes_strategy(s(2), s(0), SubmissionStrategy::IfLeading));
+		assert!(!score_passes_strategy(s(5), s(10), SubmissionStrategy::IfLeading));
+		assert!(!score_passes_strategy(s(9), s(10), SubmissionStrategy::IfLeading));
+		assert!(score_passes_strategy(s(10), s(10), SubmissionStrategy::IfLeading));
+
+		// if better by 2%
+		assert!(!score_passes_strategy(s(50), s(100), SubmissionStrategy::ClaimBetterThan(two)));
+		assert!(!score_passes_strategy(s(100), s(100), SubmissionStrategy::ClaimBetterThan(two)));
+		assert!(!score_passes_strategy(s(101), s(100), SubmissionStrategy::ClaimBetterThan(two)));
+		assert!(!score_passes_strategy(s(102), s(100), SubmissionStrategy::ClaimBetterThan(two)));
+		assert!(score_passes_strategy(s(103), s(100), SubmissionStrategy::ClaimBetterThan(two)));
+		assert!(score_passes_strategy(s(150), s(100), SubmissionStrategy::ClaimBetterThan(two)));
+
+		// if no less than 2% worse
+		assert!(!score_passes_strategy(s(50), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(!score_passes_strategy(s(97), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(98), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(99), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(100), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(101), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(102), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(103), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+		assert!(score_passes_strategy(s(150), s(100), SubmissionStrategy::ClaimNoWorseThan(two)));
+	}
+}

polkadot/utils/staking-miner/src/opts.rs

@@ -89,7 +89,9 @@ pub(crate) struct MonitorConfig {
 	/// `--submission-strategy always`: always submit.
 	///
 	/// `--submission-strategy "percent-better <percent>"`: submit if the submission is `n` percent better.
-	#[arg(long, default_value = "if-leading")]
+	///
+	/// `--submission-strategy "no-worse-than  <percent>"`: submit if submission is no more than `n` percent worse.
+	#[clap(long, default_value = "if-leading")]
 	pub submission_strategy: SubmissionStrategy,
 
 	/// Delay in number seconds to wait until starting mining a solution.
@@ -157,12 +159,14 @@ pub(crate) struct InfoOpts {
 #[derive(Debug, Copy, Clone)]
 #[cfg_attr(test, derive(PartialEq))]
 pub enum SubmissionStrategy {
-	// Only submit if at the time, we are the best.
-	IfLeading,
-	// Always submit.
+	/// Always submit.
 	Always,
-	// Submit if we are leading, or if the solution that's leading is more that the given `Perbill`
-	// better than us. This helps detect obviously fake solutions and still combat them.
+	/// Only submit if at the time, we are the best (or equal to it).
+	IfLeading,
+	/// Submit if we are no worse than `Perbill` worse than the best.
+	ClaimNoWorseThan(Perbill),
+	/// Submit if we are leading, or if the solution that's leading is more that the given `Perbill`
+	/// better than us. This helps detect obviously fake solutions and still combat them.
 	ClaimBetterThan(Perbill),
 }
 
@@ -185,6 +189,7 @@ pub(crate) enum Solver {
 /// * --submission-strategy if-leading: only submit if leading
 /// * --submission-strategy always: always submit
 /// * --submission-strategy "percent-better <percent>": submit if submission is `n` percent better.
+/// * --submission-strategy "no-worse-than<percent>": submit if submission is no more than `n` percent worse.
 ///
 impl FromStr for SubmissionStrategy {
 	type Err = String;
@@ -196,8 +201,11 @@ impl FromStr for SubmissionStrategy {
 			Self::IfLeading
 		} else if s == "always" {
 			Self::Always
-		} else if s.starts_with("percent-better ") {
-			let percent: u32 = s[15..].parse().map_err(|e| format!("{:?}", e))?;
+		} else if let Some(percent) = s.strip_prefix("no-worse-than ") {
+			let percent: u32 = percent.parse().map_err(|e| format!("{:?}", e))?;
+			Self::ClaimNoWorseThan(Perbill::from_percent(percent))
+		} else if let Some(percent) = s.strip_prefix("percent-better ") {
+			let percent: u32 = percent.parse().map_err(|e| format!("{:?}", e))?;
 			Self::ClaimBetterThan(Perbill::from_percent(percent))
 		} else {
 			return Err(s.into())