From 62e185456794b15425b791fa176a34325ab3b170 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 5 Mar 2021 10:39:41 +0100 Subject: [PATCH 01/45] not climate --- frame/election-provider-multi-phase/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index c4a5e0fa6936a..844a898bda113 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -303,27 +303,27 @@ impl Default for Phase { } impl Phase { - /// 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) } From 9a8c4a2b81f04fc93d097ab368d8ba010eab67aa Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 5 Mar 2021 11:17:22 +0100 Subject: [PATCH 02/45] explain the intent of the bool in the unsigned phase --- frame/election-provider-multi-phase/src/lib.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 844a898bda113..084c4ae00effa 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -291,8 +291,16 @@ pub enum Phase { 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)), } From 19d725104b4905d92a56a47c3b7ba89ca74ef88c Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 5 Mar 2021 15:00:47 +0100 Subject: [PATCH 03/45] remove glob imports from unsigned.rs --- .../election-provider-multi-phase/src/lib.rs | 2 +- .../src/unsigned.rs | 62 ++++++++++++++++--- 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 084c4ae00effa..66a2112eb1df6 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -213,7 +213,7 @@ use frame_support::{ use frame_system::{ensure_none, offchain::SendTransactionTypes}; use sp_election_providers::{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::{ diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 2039e5d9f0754..77f9337d3946f 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -17,15 +17,38 @@ //! The unsigned phase implementation. -use crate::*; -use frame_support::dispatch::DispatchResult; +use crate::{ + helpers, + Call, + CompactAccuracyOf, + CompactOf, + CompactVoterIndexOf, + Config, + ElectionCompute, + Error, + FeasibilityError, + Pallet, + RawSolution, + ReadySolution, + RoundSnapshot, + SolutionOrSnapshotSize, + Weight, + WeightInfo, +}; +use codec::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_ratio_to_staked_normalized, assignment_staked_to_ratio_normalized, + is_score_better, + seq_phragmen, + CompactSolution, + ElectionResult, }; use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; -use sp_std::cmp::Ordering; +use sp_std::{cmp::Ordering, 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"; @@ -400,7 +423,8 @@ impl Pallet { #[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 { @@ -481,12 +505,30 @@ mod max_weight { #[cfg(test)] mod tests { - use super::{ - mock::{Origin, *}, - Call, *, + use super::*; + use crate::{ + mock::{ + roll_to, + roll_to_with_ocw, + witness, + Call as OuterCall, + ExtBuilder, + Extrinsic, + MinerMaxWeight, + MultiPhase, + Origin, + Runtime, + TestCompact, + }, + CurrentPhase, + InvalidTransaction, + Phase, + QueuedSolution, + TransactionSource, + TransactionValidityError, }; - 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_election_providers::Assignment; use sp_runtime::{traits::ValidateUnsigned, PerU16}; From 02ad14b1c6caa6fcf8e44282db650ccc578d3441 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 5 Mar 2021 15:02:25 +0100 Subject: [PATCH 04/45] add OffchainRepeat parameter to ElectionProviderMultiPhase --- bin/node/runtime/src/lib.rs | 2 ++ frame/election-provider-multi-phase/src/lib.rs | 7 +++++++ frame/election-provider-multi-phase/src/mock.rs | 4 +++- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 20abb9b54ff08..58166b0ed2318 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -469,6 +469,7 @@ parameter_types! { .get(DispatchClass::Normal) .max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed") .saturating_sub(BlockExecutionWeight::get()); + pub OffchainRepeat: BlockNumber = 5; } impl pallet_staking::Config for Runtime { @@ -530,6 +531,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type SignedPhase = SignedPhase; type UnsignedPhase = UnsignedPhase; type SolutionImprovementThreshold = MinSolutionScoreBump; + type OffchainRepeat = OffchainRepeat; type MinerMaxIterations = MinerMaxIterations; type MinerMaxWeight = MinerMaxWeight; type MinerTxPriority = MultiPhaseUnsignedPriority; diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 66a2112eb1df6..06ac7fc83ea0d 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -522,6 +522,13 @@ pub mod pallet { #[pallet::constant] type SolutionImprovementThreshold: Get; + /// 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; + /// The priority of the unsigned transaction submitted in the unsigned-phase type MinerTxPriority: Get; /// Maximum number of iteration of balancing that will be executed in the embedded miner of diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index eb38a4cd52e95..e8c94309fe4fd 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -60,6 +60,7 @@ frame_support::construct_runtime!( pub(crate) type Balance = u64; pub(crate) type AccountId = u64; +pub(crate) type BlockNumber = u32; sp_npos_elections::generate_solution_type!( #[compact] @@ -201,10 +202,10 @@ 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 MockWeightInfo: bool = false; - pub static EpochLength: u64 = 30; } @@ -265,6 +266,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 MinerTxPriority = MinerTxPriority; From f6a0fd97a6b655e108adacd3e52d56fc7a58de87 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 8 Mar 2021 14:28:18 +0100 Subject: [PATCH 05/45] migrate core logic from #7976 This is a much smaller diff than that PR contained, but I think it contains all the essentials. --- .../election-provider-multi-phase/src/lib.rs | 26 +++++--- .../src/unsigned.rs | 59 +++++++++++++++---- 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 06ac7fc83ea0d..18bb4c9e77c7a 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -610,16 +610,26 @@ 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, "miner exeuction done: {:?}", outcome); + fn offchain_worker(now: T::BlockNumber) { + let threshold = T::OffchainRepeat::get(); + 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, threshold) + .and_then(|_| Self::mine_check_save_submit()); + log!(info, "initial OCW output at {:?}: {:?}", now, initial_output); + } + Phase::Unsigned((true, opened)) if opened < now => { + if !>::exists() { + // as long as there is no feasible solution, keep trying to submit ours + // + // the offchain_lock prevents us from spamming submissions too often. + let resubmit_output = Self::try_acquire_offchain_lock(now, threshold) + .and_then(|_| Self::restore_or_compute_then_submit()); + log!(info, "resubmit OCW output at {:?}: {:?}", now, resubmit_output); } - Err(why) => log!(warn, "denied offchain worker: {:?}", why), } + _ => {}, } } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 77f9337d3946f..f57298f73baf2 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -53,9 +53,8 @@ use sp_std::{cmp::Ordering, 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"; -/// 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"; #[derive(Debug, Eq, PartialEq)] pub enum MinerError { @@ -69,6 +68,8 @@ pub enum MinerError { PreDispatchChecksFailed, /// The solution generated from the miner is not feasible. Feasibility(FeasibilityError), + /// Something went wrong fetching the lock. + Lock(&'static str), } impl From for MinerError { @@ -83,15 +84,48 @@ impl From for MinerError { } } +/// Save a given call into OCW storage. +fn save_solution(call: &Call) { + let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); + storage.set(&call); +} + +/// Get a saved solution from OCW storage if it exists. +fn restore_solution() -> Option> { + StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL).get().flatten() +} + impl Pallet { - /// 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. + pub fn restore_or_compute_then_submit() -> Result<(), MinerError> { + let call = match restore_solution() { + Some(call) => call, + None => { + let call = Self::mine_call()?; + save_solution(&call); + call + }, + }; + 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> { + let call = Self::mine_call()?; + save_solution(&call); + Self::submit_call(call) + } + + /// Mine a new solution as a call. Performs all checks. + fn mine_call() -> Result, 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)?; + Ok(Call::submit_unsigned(raw_solution, witness)) + } - let call = Call::submit_unsigned(raw_solution, witness).into(); - SubmitTransaction::>::submit_unsigned_transaction(call) + fn submit_call(call: Call) -> Result<(), MinerError> { + SubmitTransaction::>::submit_unsigned_transaction(call.into()) .map_err(|_| MinerError::PoolSubmissionFailed) } @@ -171,7 +205,7 @@ impl Pallet { .map_err::(Into::into)?; sp_npos_elections::reduce(&mut staked); - // convert back to ration and make compact. + // convert back to ratio and make compact. let ratio = assignment_staked_to_ratio_normalized(staked)?; let compact = >::from_assignment(ratio, &voter_index, &target_index)?; @@ -351,12 +385,11 @@ impl Pallet { /// 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> { + pub(crate) fn try_acquire_offchain_lock(now: T::BlockNumber, threshold: T::BlockNumber) -> Result<(), MinerError> { let storage = StorageValueRef::persistent(&OFFCHAIN_HEAD_DB); - let threshold = T::BlockNumber::from(OFFCHAIN_REPEAT); let mutate_stat = storage.mutate::<_, &'static str, _>(|maybe_head: Option>| { @@ -380,9 +413,9 @@ impl Pallet { // 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)), } } From 59a0fb4668918fd0a8f2381845f13e700ee93c45 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 8 Mar 2021 15:07:17 +0100 Subject: [PATCH 06/45] improve formatting --- frame/election-provider-multi-phase/src/lib.rs | 2 +- frame/election-provider-multi-phase/src/unsigned.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 18bb4c9e77c7a..7eafec3fa4c80 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -629,7 +629,7 @@ pub mod pallet { log!(info, "resubmit OCW output at {:?}: {:?}", now, resubmit_output); } } - _ => {}, + _ => {} } } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index f57298f73baf2..1742122b2adc3 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -104,7 +104,7 @@ impl Pallet { let call = Self::mine_call()?; save_solution(&call); call - }, + } }; Self::submit_call(call) } @@ -388,7 +388,10 @@ impl Pallet { /// 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, threshold: T::BlockNumber) -> Result<(), MinerError> { + pub(crate) fn try_acquire_offchain_lock( + now: T::BlockNumber, + threshold: T::BlockNumber, + ) -> Result<(), MinerError> { let storage = StorageValueRef::persistent(&OFFCHAIN_HEAD_DB); let mutate_stat = From 4612d07669eda93ca57ae5dbfa9b2f6b639e7274 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 8 Mar 2021 16:13:14 +0100 Subject: [PATCH 07/45] fix test build failures --- .../election-provider-multi-phase/src/lib.rs | 4 -- .../src/unsigned.rs | 61 +++++++++---------- 2 files changed, 30 insertions(+), 35 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 7eafec3fa4c80..148c6cdea94ef 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -192,10 +192,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 diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 1742122b2adc3..8dfb941c9934a 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -134,7 +134,7 @@ impl Pallet { /// /// 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>, SolutionOrSnapshotSize), MinerError> { @@ -543,25 +543,12 @@ mod max_weight { mod tests { use super::*; use crate::{ + CurrentPhase, InvalidTransaction, Phase, QueuedSolution, TransactionSource, + TransactionValidityError, mock::{ - roll_to, - roll_to_with_ocw, - witness, - Call as OuterCall, - ExtBuilder, - Extrinsic, - MinerMaxWeight, - MultiPhase, - Origin, - Runtime, - TestCompact, + BlockNumber, Call as OuterCall, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, + Origin, Runtime, TestCompact, roll_to, roll_to_with_ocw, witness, }, - CurrentPhase, - InvalidTransaction, - Phase, - QueuedSolution, - TransactionSource, - TransactionValidityError, }; use frame_benchmarking::Zero; use frame_support::{assert_noop, assert_ok, dispatch::Dispatchable, traits::OffchainWorker}; @@ -805,7 +792,7 @@ mod tests { // mine seq_phragmen solution with 2 iters. assert_eq!( - MultiPhase::mine_check_and_submit().unwrap_err(), + MultiPhase::mine_check_save_submit().unwrap_err(), MinerError::PreDispatchChecksFailed, ); }) @@ -882,30 +869,42 @@ mod tests { #[test] fn ocw_check_prevent_duplicate() { + const OFFCHAIN_REPEAT: BlockNumber = 5; + let (mut ext, _) = ExtBuilder::default().build_offchainify(0); ext.execute_with(|| { roll_to(25); assert!(MultiPhase::current_phase().is_unsigned()); // first execution -- okay. - assert!(MultiPhase::try_acquire_offchain_lock(25).is_ok()); + assert!(MultiPhase::try_acquire_offchain_lock(25, OFFCHAIN_REPEAT.into()).is_ok()); // next block: rejected. - assert!(MultiPhase::try_acquire_offchain_lock(26).is_err()); + assert!(MultiPhase::try_acquire_offchain_lock(26, OFFCHAIN_REPEAT.into()).is_err()); // 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(), + OFFCHAIN_REPEAT.into() + ) + .is_ok()); // a fork like situation: re-execute last 3. - 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() - ); + assert!(MultiPhase::try_acquire_offchain_lock( + (26 + OFFCHAIN_REPEAT - 3).into(), + OFFCHAIN_REPEAT.into() + ) + .is_err()); + assert!(MultiPhase::try_acquire_offchain_lock( + (26 + OFFCHAIN_REPEAT - 2).into(), + OFFCHAIN_REPEAT.into() + ) + .is_err()); + assert!(MultiPhase::try_acquire_offchain_lock( + (26 + OFFCHAIN_REPEAT - 1).into(), + OFFCHAIN_REPEAT.into() + ) + .is_err()); }) } From c8ccbe9e1a44504b94d994b645140c250aec2294 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 10 Mar 2021 10:05:20 +0100 Subject: [PATCH 08/45] cause test to pass --- .../src/unsigned.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 8dfb941c9934a..7f52bc440b643 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -909,7 +909,7 @@ mod tests { } #[test] - fn ocw_only_runs_when_signed_open_now() { + fn ocw_runs_when_signed_open_now() { let (mut ext, pool) = ExtBuilder::default().build_offchainify(0); ext.execute_with(|| { roll_to(25); @@ -923,13 +923,18 @@ mod tests { assert!(pool.read().transactions.len().is_zero()); storage.clear(); + // creates, caches, submits without expecting previous cache value + MultiPhase::offchain_worker(25); + dbg!(&pool.read().transactions.len()); + 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); + dbg!(&pool.read().transactions.len()); assert!(pool.read().transactions.len().is_zero()); - storage.clear(); - - // submits! - MultiPhase::offchain_worker(25); - assert!(!pool.read().transactions.len().is_zero()); }) } From f4b50fc746446ee92b349c94d24da7ba7f7e5d57 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 16 Mar 2021 13:49:14 +0100 Subject: [PATCH 09/45] Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/election-provider-multi-phase/src/lib.rs | 3 +-- frame/election-provider-multi-phase/src/unsigned.rs | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 5c25f78eeba69..61c66d391bca7 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -617,8 +617,7 @@ pub mod pallet { } Phase::Unsigned((true, opened)) if opened < now => { if !>::exists() { - // as long as there is no feasible solution, keep trying to submit ours - // + // as long as there is no feasible solution, keep trying to submit ours. // the offchain_lock prevents us from spamming submissions too often. let resubmit_output = Self::try_acquire_offchain_lock(now, threshold) .and_then(|_| Self::restore_or_compute_then_submit()); diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 7f52bc440b643..27164ae982098 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -925,7 +925,6 @@ mod tests { // creates, caches, submits without expecting previous cache value MultiPhase::offchain_worker(25); - dbg!(&pool.read().transactions.len()); assert_eq!(pool.read().transactions.len(), 1); // assume that the tx has been processed pool.try_write().unwrap().transactions.clear(); @@ -933,7 +932,6 @@ mod tests { // locked, but also, has previously cached. MultiPhase::offchain_worker(26); - dbg!(&pool.read().transactions.len()); assert!(pool.read().transactions.len().is_zero()); }) } From a192ba3a0302f74ffcab9e3263241d591a69f50b Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 16 Mar 2021 14:44:41 +0100 Subject: [PATCH 10/45] collapse imports --- .../src/unsigned.rs | 29 ++++--------------- 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 27164ae982098..ae2b926a97837 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -17,35 +17,16 @@ //! The unsigned phase implementation. -use crate::{ - helpers, - Call, - CompactAccuracyOf, - CompactOf, - CompactVoterIndexOf, - Config, - ElectionCompute, - Error, - FeasibilityError, - Pallet, - RawSolution, - ReadySolution, - RoundSnapshot, - SolutionOrSnapshotSize, - Weight, - WeightInfo, +use crate::{helpers, Call, CompactAccuracyOf, CompactOf, CompactVoterIndexOf, Config, + ElectionCompute, Error, FeasibilityError, Pallet, RawSolution, ReadySolution, RoundSnapshot, + SolutionOrSnapshotSize, Weight, WeightInfo, }; use codec::Decode; use frame_support::{dispatch::DispatchResult, ensure, traits::Get}; use frame_system::offchain::SubmitTransaction; use sp_arithmetic::Perbill; -use sp_npos_elections::{ - assignment_ratio_to_staked_normalized, - assignment_staked_to_ratio_normalized, - is_score_better, - seq_phragmen, - CompactSolution, - ElectionResult, +use sp_npos_elections::{assignment_ratio_to_staked_normalized, assignment_staked_to_ratio_normalized, + is_score_better, seq_phragmen, CompactSolution, ElectionResult, }; use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; use sp_std::{cmp::Ordering, vec::Vec}; From 81ceca1744beddedb3cde29b7ccfc3700f736aed Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 16 Mar 2021 14:52:38 +0100 Subject: [PATCH 11/45] threshold acquired directly within try_acquire_offchain_lock --- .../election-provider-multi-phase/src/lib.rs | 5 ++- .../src/unsigned.rs | 36 ++++++------------- 2 files changed, 12 insertions(+), 29 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 61c66d391bca7..dd559f8a51a4e 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -607,11 +607,10 @@ pub mod pallet { } fn offchain_worker(now: T::BlockNumber) { - let threshold = T::OffchainRepeat::get(); 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, threshold) + 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); } @@ -619,7 +618,7 @@ pub mod pallet { if !>::exists() { // as long as there is no feasible solution, keep trying to submit ours. // the offchain_lock prevents us from spamming submissions too often. - let resubmit_output = Self::try_acquire_offchain_lock(now, threshold) + let resubmit_output = Self::try_acquire_offchain_lock(now) .and_then(|_| Self::restore_or_compute_then_submit()); log!(info, "resubmit OCW output at {:?}: {:?}", now, resubmit_output); } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index ae2b926a97837..35888528e787b 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -371,8 +371,8 @@ impl Pallet { /// Returns `Ok(())` if offchain worker should happen, `Err(reason)` otherwise. pub(crate) fn try_acquire_offchain_lock( now: T::BlockNumber, - threshold: T::BlockNumber, ) -> Result<(), MinerError> { + let threshold = T::OffchainRepeat::get(); let storage = StorageValueRef::persistent(&OFFCHAIN_HEAD_DB); let mutate_stat = @@ -527,7 +527,7 @@ mod tests { CurrentPhase, InvalidTransaction, Phase, QueuedSolution, TransactionSource, TransactionValidityError, mock::{ - BlockNumber, Call as OuterCall, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, + Call as OuterCall, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin, Runtime, TestCompact, roll_to, roll_to_with_ocw, witness, }, }; @@ -850,42 +850,26 @@ mod tests { #[test] fn ocw_check_prevent_duplicate() { - const OFFCHAIN_REPEAT: BlockNumber = 5; - let (mut ext, _) = ExtBuilder::default().build_offchainify(0); ext.execute_with(|| { + let offchain_repeat = ::OffchainRepeat::get(); + roll_to(25); assert!(MultiPhase::current_phase().is_unsigned()); // first execution -- okay. - assert!(MultiPhase::try_acquire_offchain_lock(25, OFFCHAIN_REPEAT.into()).is_ok()); + assert!(MultiPhase::try_acquire_offchain_lock(25).is_ok()); // next block: rejected. - assert!(MultiPhase::try_acquire_offchain_lock(26, OFFCHAIN_REPEAT.into()).is_err()); + assert!(MultiPhase::try_acquire_offchain_lock(26).is_err()); // allowed after `OFFCHAIN_REPEAT` - assert!(MultiPhase::try_acquire_offchain_lock( - (26 + OFFCHAIN_REPEAT).into(), - 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!(MultiPhase::try_acquire_offchain_lock( - (26 + OFFCHAIN_REPEAT - 3).into(), - OFFCHAIN_REPEAT.into() - ) - .is_err()); - assert!(MultiPhase::try_acquire_offchain_lock( - (26 + OFFCHAIN_REPEAT - 2).into(), - OFFCHAIN_REPEAT.into() - ) - .is_err()); - assert!(MultiPhase::try_acquire_offchain_lock( - (26 + OFFCHAIN_REPEAT - 1).into(), - OFFCHAIN_REPEAT.into() - ) - .is_err()); + 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()); }) } From 449b799331684d3c79f240fe462a8198dfd65f85 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 17 Mar 2021 10:06:23 +0100 Subject: [PATCH 12/45] add test of resubmission after interval --- .../src/unsigned.rs | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index c315ee6abce32..fc81826d69e87 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -910,13 +910,50 @@ mod tests { // 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_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 = ::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_HEAD_DB); + + 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_can_submit_to_pool() { let (mut ext, pool) = ExtBuilder::default().build_offchainify(0); From 85f715e847c8e4ad8ab2ea65515cb5c09e90114f Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 17 Mar 2021 10:18:09 +0100 Subject: [PATCH 13/45] add test that ocw can regenerate a failed cache when resubmitting --- .../src/unsigned.rs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index fc81826d69e87..8f14acab70329 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -954,6 +954,51 @@ mod tests { }) } + #[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 = ::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_HEAD_DB); + + 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::>(), 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); + }) + } + #[test] fn ocw_can_submit_to_pool() { let (mut ext, pool) = ExtBuilder::default().build_offchainify(0); From c6ecab84eeb2f94ebc44c0387f18c4429c5b4731 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Mar 2021 09:20:16 +0100 Subject: [PATCH 14/45] 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. --- .../src/benchmarking.rs | 6 ++- .../election-provider-multi-phase/src/lib.rs | 13 +++--- .../src/unsigned.rs | 41 +++++++++++-------- 3 files changed, 36 insertions(+), 24 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 3b1b7bd7a2290..70f43e7ee8b8e 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -243,11 +243,13 @@ frame_benchmarking::benchmarks! { assert!(>::queued_solution().is_none()); >::put(Phase::Unsigned((true, 1u32.into()))); + let round = >::round(); + // encode the most significant storage item that needs to be decoded in the dispatch. let encoded_snapshot = >::snapshot().unwrap().encode(); - let encoded_call = >::submit_unsigned(raw_solution.clone(), witness).encode(); + let encoded_call = >::submit_unsigned(raw_solution.clone(), round, witness).encode(); }: { - assert_ok!(>::submit_unsigned(RawOrigin::None.into(), raw_solution, witness)); + assert_ok!(>::submit_unsigned(RawOrigin::None.into(), raw_solution, round, witness)); let _decoded_snap = as Decode>::decode(&mut &*encoded_snapshot).unwrap(); let _decoded_call = as Decode>::decode(&mut &*encoded_call).unwrap(); } verify { diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 81cbe625d05b2..25479ff6a297f 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -725,6 +725,7 @@ pub mod pallet { pub fn submit_unsigned( origin: OriginFor, solution: RawSolution>, + round: u32, witness: SolutionOrSnapshotSize, ) -> DispatchResultWithPostInfo { ensure_none(origin)?; @@ -733,7 +734,7 @@ pub mod pallet { deprive validator from their authoring reward."; // Check score being an improvement, phase, and desired targets. - Self::unsigned_pre_dispatch_checks(&solution).expect(error_message); + Self::unsigned_pre_dispatch_checks(&solution, round).expect(error_message); // ensure witness was correct. let SolutionOrSnapshotSize { voters, targets } = @@ -786,6 +787,8 @@ pub mod pallet { PreDispatchWrongWinnerCount, /// Submission was too weak, score-wise. PreDispatchWeakSubmission, + /// OCW submitted solution for wrong round + OcwCallWrongEra, } #[pallet::origin] @@ -794,7 +797,7 @@ pub mod pallet { impl ValidateUnsigned for Pallet { type Call = Call; fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity { - if let Call::submit_unsigned(solution, _) = call { + if let Call::submit_unsigned(solution, round, _) = call { // discard solution not coming from the local OCW. match source { TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ } @@ -803,7 +806,7 @@ pub mod pallet { } } - let _ = Self::unsigned_pre_dispatch_checks(solution) + let _ = Self::unsigned_pre_dispatch_checks(solution, *round) .map_err(|err| { log!(error, "unsigned transaction validation failed due to {:?}", err); err @@ -831,8 +834,8 @@ pub mod pallet { } fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> { - if let Call::submit_unsigned(solution, _) = call { - Self::unsigned_pre_dispatch_checks(solution) + if let Call::submit_unsigned(solution, round, _) = call { + Self::unsigned_pre_dispatch_checks(solution, *round) .map_err(dispatch_error_to_invalid) .map_err(Into::into) } else { diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 8f14acab70329..51dd160c872f6 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -104,9 +104,10 @@ impl Pallet { 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 round = Self::round(); let score = raw_solution.score.clone(); - let call: Call = Call::submit_unsigned(raw_solution, witness).into(); + let call: Call = Call::submit_unsigned(raw_solution, round, witness).into(); log!( info, @@ -133,9 +134,10 @@ impl Pallet { iters: usize, ) -> Result<(RawSolution>, SolutionOrSnapshotSize), MinerError> { let (raw_solution, witness) = Self::mine_solution(iters)?; + let current_round = Self::round(); // ensure that this will pass the pre-dispatch checks - Self::unsigned_pre_dispatch_checks(&raw_solution).map_err(|e| { + Self::unsigned_pre_dispatch_checks(&raw_solution, current_round).map_err(|e| { log!(warn, "pre-dispatch-checks failed for mined solution: {:?}", e); MinerError::PreDispatchChecksFailed })?; @@ -425,10 +427,14 @@ impl Pallet { /// code, so that we do less and less storage reads here. pub(crate) fn unsigned_pre_dispatch_checks( solution: &RawSolution>, + round: u32, ) -> DispatchResult { // ensure solution is timely. Don't panic yet. This is a cheap check. ensure!(Self::current_phase().is_unsigned_open(), Error::::PreDispatchEarlySubmission); + // ensure round is current + ensure!(Self::round() == round, Error::::OcwCallWrongEra,); + // ensure correct number of winners. ensure!( Self::desired_targets().unwrap_or_default() @@ -543,8 +549,8 @@ mod tests { CurrentPhase, InvalidTransaction, Phase, QueuedSolution, TransactionSource, TransactionValidityError, mock::{ - Call as OuterCall, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, - Origin, Runtime, TestCompact, roll_to, roll_to_with_ocw, witness, + Call as OuterCall, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin, Runtime, + TestCompact, roll_to, roll_to_with_ocw, witness, }, }; use frame_benchmarking::Zero; @@ -556,7 +562,7 @@ mod tests { fn validate_unsigned_retracts_wrong_phase() { ExtBuilder::default().desired_targets(0).build_and_execute(|| { let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), witness()); + let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); // initial assert_eq!(MultiPhase::current_phase(), Phase::Off); @@ -616,7 +622,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), witness()); + let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); // initial assert!(::validate_unsigned( @@ -653,7 +659,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), witness()); + let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); assert_eq!(solution.compact.unique_targets().len(), 0); // won't work anymore. @@ -675,7 +681,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), witness()); + let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); assert_eq!( ::validate_unsigned( @@ -701,7 +707,7 @@ mod tests { // This is in itself an invalid BS solution. let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), witness()); + let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); let outer_call: OuterCall = call.into(); let _ = outer_call.dispatch(Origin::none()); }) @@ -721,7 +727,7 @@ mod tests { let mut correct_witness = witness(); correct_witness.voters += 1; correct_witness.targets -= 1; - let call = Call::submit_unsigned(solution.clone(), correct_witness); + let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), correct_witness); let outer_call: OuterCall = call.into(); let _ = outer_call.dispatch(Origin::none()); }) @@ -742,7 +748,7 @@ mod tests { // ensure this solution is valid. assert!(MultiPhase::queued_solution().is_none()); - assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness)); + assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, MultiPhase::round(), witness)); assert!(MultiPhase::queued_solution().is_some()); }) } @@ -817,8 +823,9 @@ mod tests { }], }; let (solution, witness) = MultiPhase::prepare_election_result(result).unwrap(); - assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution)); - assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness)); + let round = MultiPhase::round(); + assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution, round)); + assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, round, witness)); assert_eq!(MultiPhase::queued_solution().unwrap().score[0], 10); // trial 1: a solution who's score is only 2, i.e. 20% better in the first element. @@ -837,7 +844,7 @@ mod tests { // 12 is not 50% more than 10 assert_eq!(solution.score[0], 12); assert_noop!( - MultiPhase::unsigned_pre_dispatch_checks(&solution), + MultiPhase::unsigned_pre_dispatch_checks(&solution, round), Error::::PreDispatchWeakSubmission, ); // submitting this will actually panic. @@ -859,8 +866,8 @@ mod tests { assert_eq!(solution.score[0], 17); // and it is fine - assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution)); - assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness)); + assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution, round)); + assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, round, witness)); }) } @@ -1010,7 +1017,7 @@ 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(..)))); }) } } From ad3b786488844f9ac7b7149519ff00dec5421881 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Mar 2021 10:07:35 +0100 Subject: [PATCH 15/45] add test of pre-dispatch round check --- .../src/unsigned.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 51dd160c872f6..3da31645c5172 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1020,4 +1020,28 @@ mod tests { 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 + >::mutate(|round| *round += 1); + + let encoded = pool.read().transactions[0].clone(); + let extrinsic = Extrinsic::decode(&mut &*encoded).unwrap(); + let (solution, round) = match extrinsic.call { + OuterCall::MultiPhase(Call::submit_unsigned(solution, round, _)) => (solution, round), + _ => panic!("bad call: unexpected submission"), + }; + + assert_noop!( + MultiPhase::unsigned_pre_dispatch_checks(&solution, round), + Error::::OcwCallWrongEra, + ); + }) + } } From 89cf445cdd48e0afad20cba2b694af34a999085f Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Mar 2021 13:59:20 +0100 Subject: [PATCH 16/45] use `RawSolution.round` instead of redundantly externally --- .../src/benchmarking.rs | 6 +-- .../election-provider-multi-phase/src/lib.rs | 11 +++-- .../src/unsigned.rs | 40 +++++++++---------- 3 files changed, 25 insertions(+), 32 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index efd9322e0eee1..40c7e801ae78d 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -243,13 +243,11 @@ frame_benchmarking::benchmarks! { assert!(>::queued_solution().is_none()); >::put(Phase::Unsigned((true, 1u32.into()))); - let round = >::round(); - // encode the most significant storage item that needs to be decoded in the dispatch. let encoded_snapshot = >::snapshot().unwrap().encode(); - let encoded_call = >::submit_unsigned(raw_solution.clone(), round, witness).encode(); + let encoded_call = >::submit_unsigned(raw_solution.clone(), witness).encode(); }: { - assert_ok!(>::submit_unsigned(RawOrigin::None.into(), raw_solution, round, witness)); + assert_ok!(>::submit_unsigned(RawOrigin::None.into(), raw_solution, witness)); let _decoded_snap = as Decode>::decode(&mut &*encoded_snapshot).unwrap(); let _decoded_call = as Decode>::decode(&mut &*encoded_call).unwrap(); } verify { diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 25479ff6a297f..1652dd25c25b2 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -725,7 +725,6 @@ pub mod pallet { pub fn submit_unsigned( origin: OriginFor, solution: RawSolution>, - round: u32, witness: SolutionOrSnapshotSize, ) -> DispatchResultWithPostInfo { ensure_none(origin)?; @@ -734,7 +733,7 @@ pub mod pallet { deprive validator from their authoring reward."; // Check score being an improvement, phase, and desired targets. - Self::unsigned_pre_dispatch_checks(&solution, round).expect(error_message); + Self::unsigned_pre_dispatch_checks(&solution).expect(error_message); // ensure witness was correct. let SolutionOrSnapshotSize { voters, targets } = @@ -797,7 +796,7 @@ pub mod pallet { impl ValidateUnsigned for Pallet { type Call = Call; fn validate_unsigned(source: TransactionSource, call: &Self::Call) -> TransactionValidity { - if let Call::submit_unsigned(solution, round, _) = call { + if let Call::submit_unsigned(solution, _) = call { // discard solution not coming from the local OCW. match source { TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ } @@ -806,7 +805,7 @@ pub mod pallet { } } - let _ = Self::unsigned_pre_dispatch_checks(solution, *round) + let _ = Self::unsigned_pre_dispatch_checks(solution) .map_err(|err| { log!(error, "unsigned transaction validation failed due to {:?}", err); err @@ -834,8 +833,8 @@ pub mod pallet { } fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> { - if let Call::submit_unsigned(solution, round, _) = call { - Self::unsigned_pre_dispatch_checks(solution, *round) + if let Call::submit_unsigned(solution, _) = call { + Self::unsigned_pre_dispatch_checks(solution) .map_err(dispatch_error_to_invalid) .map_err(Into::into) } else { diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index dafb406187faa..2686bf7b8bd22 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -104,10 +104,9 @@ impl Pallet { 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 round = Self::round(); let score = raw_solution.score.clone(); - let call: Call = Call::submit_unsigned(raw_solution, round, witness).into(); + let call: Call = Call::submit_unsigned(raw_solution, witness).into(); log!( info, @@ -134,10 +133,9 @@ impl Pallet { iters: usize, ) -> Result<(RawSolution>, SolutionOrSnapshotSize), MinerError> { let (raw_solution, witness) = Self::mine_solution(iters)?; - let current_round = Self::round(); // ensure that this will pass the pre-dispatch checks - Self::unsigned_pre_dispatch_checks(&raw_solution, current_round).map_err(|e| { + Self::unsigned_pre_dispatch_checks(&raw_solution).map_err(|e| { log!(warn, "pre-dispatch-checks failed for mined solution: {:?}", e); MinerError::PreDispatchChecksFailed })?; @@ -427,13 +425,12 @@ impl Pallet { /// code, so that we do less and less storage reads here. pub(crate) fn unsigned_pre_dispatch_checks( solution: &RawSolution>, - round: u32, ) -> DispatchResult { // ensure solution is timely. Don't panic yet. This is a cheap check. ensure!(Self::current_phase().is_unsigned_open(), Error::::PreDispatchEarlySubmission); // ensure round is current - ensure!(Self::round() == round, Error::::OcwCallWrongEra,); + ensure!(Self::round() == solution.round, Error::::OcwCallWrongEra,); // ensure correct number of winners. ensure!( @@ -562,7 +559,7 @@ mod tests { fn validate_unsigned_retracts_wrong_phase() { ExtBuilder::default().desired_targets(0).build_and_execute(|| { let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); + let call = Call::submit_unsigned(solution.clone(), witness()); // initial assert_eq!(MultiPhase::current_phase(), Phase::Off); @@ -622,7 +619,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); + let call = Call::submit_unsigned(solution.clone(), witness()); // initial assert!(::validate_unsigned( @@ -659,7 +656,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); + let call = Call::submit_unsigned(solution.clone(), witness()); assert_eq!(solution.compact.unique_targets().len(), 0); // won't work anymore. @@ -681,7 +678,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); + let call = Call::submit_unsigned(solution.clone(), witness()); assert_eq!( ::validate_unsigned( @@ -707,7 +704,7 @@ mod tests { // This is in itself an invalid BS solution. let solution = RawSolution:: { score: [5, 0, 0], ..Default::default() }; - let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), witness()); + let call = Call::submit_unsigned(solution.clone(), witness()); let outer_call: OuterCall = call.into(); let _ = outer_call.dispatch(Origin::none()); }) @@ -727,7 +724,7 @@ mod tests { let mut correct_witness = witness(); correct_witness.voters += 1; correct_witness.targets -= 1; - let call = Call::submit_unsigned(solution.clone(), MultiPhase::round(), correct_witness); + let call = Call::submit_unsigned(solution.clone(), correct_witness); let outer_call: OuterCall = call.into(); let _ = outer_call.dispatch(Origin::none()); }) @@ -748,7 +745,7 @@ mod tests { // ensure this solution is valid. assert!(MultiPhase::queued_solution().is_none()); - assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, MultiPhase::round(), witness)); + assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness)); assert!(MultiPhase::queued_solution().is_some()); }) } @@ -823,9 +820,8 @@ mod tests { }], }; let (solution, witness) = MultiPhase::prepare_election_result(result).unwrap(); - let round = MultiPhase::round(); - assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution, round)); - assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, round, witness)); + assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution)); + assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness)); assert_eq!(MultiPhase::queued_solution().unwrap().score[0], 10); // trial 1: a solution who's score is only 2, i.e. 20% better in the first element. @@ -844,7 +840,7 @@ mod tests { // 12 is not 50% more than 10 assert_eq!(solution.score[0], 12); assert_noop!( - MultiPhase::unsigned_pre_dispatch_checks(&solution, round), + MultiPhase::unsigned_pre_dispatch_checks(&solution), Error::::PreDispatchWeakSubmission, ); // submitting this will actually panic. @@ -866,8 +862,8 @@ mod tests { assert_eq!(solution.score[0], 17); // and it is fine - assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution, round)); - assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, round, witness)); + assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution)); + assert_ok!(MultiPhase::submit_unsigned(Origin::none(), solution, witness)); }) } @@ -1033,13 +1029,13 @@ mod tests { let encoded = pool.read().transactions[0].clone(); let extrinsic = Extrinsic::decode(&mut &*encoded).unwrap(); - let (solution, round) = match extrinsic.call { - OuterCall::MultiPhase(Call::submit_unsigned(solution, round, _)) => (solution, round), + let solution = match extrinsic.call { + OuterCall::MultiPhase(Call::submit_unsigned(solution, _)) => solution, _ => panic!("bad call: unexpected submission"), }; assert_noop!( - MultiPhase::unsigned_pre_dispatch_checks(&solution, round), + MultiPhase::unsigned_pre_dispatch_checks(&solution), Error::::OcwCallWrongEra, ); }) From 458682becc6617887f5ca0140db9e68c9a59d6de Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Mar 2021 14:01:29 +0100 Subject: [PATCH 17/45] unpack imports Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/election-provider-multi-phase/src/unsigned.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 2686bf7b8bd22..8d1e9aaf107da 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -17,7 +17,8 @@ //! The unsigned phase implementation. -use crate::{helpers, Call, CompactAccuracyOf, CompactOf, CompactVoterIndexOf, Config, +use crate::{ + helpers, Call, CompactAccuracyOf, CompactOf, CompactVoterIndexOf, Config, ElectionCompute, Error, FeasibilityError, Pallet, RawSolution, ReadySolution, RoundSnapshot, SolutionOrSnapshotSize, Weight, WeightInfo, }; @@ -25,7 +26,8 @@ use codec::Decode; use frame_support::{dispatch::DispatchResult, ensure, traits::Get}; use frame_system::offchain::SubmitTransaction; use sp_arithmetic::Perbill; -use sp_npos_elections::{assignment_ratio_to_staked_normalized, assignment_staked_to_ratio_normalized, +use sp_npos_elections::{ + assignment_ratio_to_staked_normalized, assignment_staked_to_ratio_normalized, is_score_better, seq_phragmen, CompactSolution, ElectionResult, }; use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; From d7dbcc0f6dec75cba45fd20156968535b9e8631e Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Mar 2021 14:03:14 +0100 Subject: [PATCH 18/45] rename `OFFCHAIN_HEAD_DB` -> `OFFCHAIN_LOCK` --- frame/election-provider-multi-phase/src/unsigned.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 8d1e9aaf107da..61c4ecece7ebe 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -34,7 +34,7 @@ use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; use sp_std::{cmp::Ordering, 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"; /// Storage key used to cache the solution `call`. pub(crate) const OFFCHAIN_CACHED_CALL: &[u8] = b"parity/multi-phase-unsigned-election/call"; @@ -388,7 +388,7 @@ impl Pallet { now: T::BlockNumber, ) -> Result<(), MinerError> { let threshold = T::OffchainRepeat::get(); - let storage = StorageValueRef::persistent(&OFFCHAIN_HEAD_DB); + let storage = StorageValueRef::persistent(&OFFCHAIN_LOCK); let mutate_stat = storage.mutate::<_, &'static str, _>(|maybe_head: Option>| { @@ -903,7 +903,7 @@ 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()); @@ -934,7 +934,7 @@ 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(block_plus(-1)); assert!(pool.read().transactions.len().is_zero()); @@ -972,7 +972,7 @@ 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(block_plus(-1)); assert!(pool.read().transactions.len().is_zero()); From b3e49500c735308773d3df9360d3525a7bf80caa Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Mar 2021 14:04:21 +0100 Subject: [PATCH 19/45] rename `mine_call` -> `mine_checked_call` --- frame/election-provider-multi-phase/src/unsigned.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 61c4ecece7ebe..95579aa110f4f 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -84,7 +84,7 @@ impl Pallet { let call = match restore_solution() { Some(call) => call, None => { - let call = Self::mine_call()?; + let call = Self::mine_checked_call()?; save_solution(&call); call } @@ -94,13 +94,13 @@ impl Pallet { /// 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> { - let call = Self::mine_call()?; + 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_call() -> Result, MinerError> { + fn mine_checked_call() -> Result, MinerError> { use codec::Encode; let iters = Self::get_balancing_iters(); From 0b772686b176812414a77b6dafd7233cb1e64833 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Mar 2021 14:05:34 +0100 Subject: [PATCH 20/45] eliminate extraneous comma --- frame/election-provider-multi-phase/src/unsigned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 95579aa110f4f..9de16be57759e 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -432,7 +432,7 @@ impl Pallet { ensure!(Self::current_phase().is_unsigned_open(), Error::::PreDispatchEarlySubmission); // ensure round is current - ensure!(Self::round() == solution.round, Error::::OcwCallWrongEra,); + ensure!(Self::round() == solution.round, Error::::OcwCallWrongEra); // ensure correct number of winners. ensure!( From 576f87cfef4ca3b1dc6b8cb2832d019b5fa8845b Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Mar 2021 14:28:24 +0100 Subject: [PATCH 21/45] check cached call is current before submitting --- .../src/unsigned.rs | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 9de16be57759e..c200aa0acfeec 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -53,6 +53,10 @@ pub enum MinerError { 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, } impl From for MinerError { @@ -74,21 +78,33 @@ fn save_solution(call: &Call) { } /// Get a saved solution from OCW storage if it exists. -fn restore_solution() -> Option> { - StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL).get().flatten() +fn restore_solution() -> Result, MinerError> { + StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL) + .get() + .flatten() + .ok_or(MinerError::NoStoredSolution) } impl Pallet { /// Attempt to restore a solution from cache. Otherwise, compute it fresh. Either way, submit. pub fn restore_or_compute_then_submit() -> Result<(), MinerError> { - let call = match restore_solution() { - Some(call) => call, - None => { + let call = restore_solution::() + .and_then(|call| { + // ensure the cached call is still current before submitting + let current_round = Self::round(); + match &call { + Call::submit_unsigned(solution, _) if solution.round == current_round => { + Ok(call) + } + _ => Err(MinerError::SolutionOutOfDate), + } + }) + .or_else::(|_| { + // if not present or cache invalidated, regenerate let call = Self::mine_checked_call()?; save_solution(&call); - call - } - }; + Ok(call) + })?; Self::submit_call(call) } From 681e6f0940cc6c3a1e309291921f1074fe3dd7a1 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 24 Mar 2021 12:35:11 +0100 Subject: [PATCH 22/45] remove unused consts introduced by bad merge. Co-authored-by: Guillaume Thiolliere --- bin/node/runtime/src/lib.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 2c6bdcc92c061..394d27c714fd6 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -468,14 +468,6 @@ 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 const ElectionLookahead: BlockNumber = EPOCH_DURATION_IN_BLOCKS / 4; - pub const MaxIterations: u32 = 10; - // 0.05%. The higher the value, the more strict solution acceptance becomes. - pub MinSolutionScoreBump: Perbill = Perbill::from_rational(5u32, 10_000); - pub OffchainSolutionWeightLimit: Weight = RuntimeBlockWeights::get() - .get(DispatchClass::Normal) - .max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed") - .saturating_sub(BlockExecutionWeight::get()); pub OffchainRepeat: BlockNumber = 5; } From ea3fc29087cde44618933746fb71036ec3662078 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 11:08:44 +0200 Subject: [PATCH 23/45] resubmit when our solution beats queued solution --- .../election-provider-multi-phase/src/lib.rs | 14 ++++++------- .../src/unsigned.rs | 21 ++++++++++++++++--- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 0e024ae4e749b..2eadbb781f66c 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -651,13 +651,13 @@ pub mod pallet { log!(info, "initial OCW output at {:?}: {:?}", now, initial_output); } Phase::Unsigned((true, opened)) if opened < now => { - if !>::exists() { - // as long as there is no feasible solution, keep trying to submit ours. - // 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_submit()); - log!(info, "resubmit OCW output at {:?}: {:?}", now, resubmit_output); - } + // 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); } _ => {} } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index fcea07d495e1c..84249f3bde010 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -86,8 +86,9 @@ fn restore_solution() -> Result, MinerError> { } impl Pallet { - /// Attempt to restore a solution from cache. Otherwise, compute it fresh. Either way, submit. - pub fn restore_or_compute_then_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> { let call = restore_solution::() .and_then(|call| { // ensure the cached call is still current before submitting @@ -105,7 +106,21 @@ impl Pallet { save_solution(&call); Ok(call) })?; - Self::submit_call(call) + + let call_score = match &call { + Call::submit_unsigned(solution, _) => solution.score.clone(), + _ => Default::default(), + }; + + if Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( + call_score, + q.score, + T::SolutionImprovementThreshold::get() + )) { + Self::submit_call(call) + } else { + Ok(()) + } } /// Mine a new solution, cache it, and submit it back to the chain as an unsigned transaction. From cc70d37673fcc82a31304c2b7c9ac42ddb13c489 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 11:17:13 +0200 Subject: [PATCH 24/45] clear call cache if solution fails to submit --- frame/election-provider-multi-phase/src/unsigned.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 84249f3bde010..c01a84a4bc511 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -85,6 +85,12 @@ fn restore_solution() -> Result, MinerError> { .ok_or(MinerError::NoStoredSolution) } +/// Clear a saved solution from OCW storage. +fn kill_solution() { + let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); + storage.clear(); +} + impl Pallet { /// 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. @@ -153,7 +159,10 @@ impl Pallet { fn submit_call(call: Call) -> Result<(), MinerError> { SubmitTransaction::>::submit_unsigned_transaction(call.into()) - .map_err(|_| MinerError::PoolSubmissionFailed) + .map_err(|_| { + kill_solution::(); + MinerError::PoolSubmissionFailed + }) } /// Mine a new npos solution, with all the relevant checks to make sure that it will be accepted From 4b46a9388532d0c09b337dc7c7edf76044a6cee8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 12:46:38 +0200 Subject: [PATCH 25/45] use local storage; clear on ElectionFinalized --- .../src/benchmarking.rs | 2 +- .../election-provider-multi-phase/src/lib.rs | 22 ++++++++++--------- .../src/unsigned.rs | 10 ++++----- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 40c7e801ae78d..c472a75869334 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -282,6 +282,6 @@ frame_benchmarking::benchmarks! { impl_benchmark_test_suite!( MultiPhase, - crate::mock::ExtBuilder::default().build(), + crate::mock::ExtBuilder::default().build_offchainify(0).0, crate::mock::Runtime, ); diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 2eadbb781f66c..d17a7f1ed6294 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1098,7 +1098,7 @@ impl Pallet { } fn do_elect() -> Result<(Supports, Weight), ElectionError> { - >::take() + let result = >::take() .map_or_else( || match T::Fallback::get() { FallbackStrategy::OnChain => Self::onchain_fallback() @@ -1125,7 +1125,9 @@ impl Pallet { log!(warn, "Failed to finalize election round. reason {:?}", err); } err - }) + }); + unsigned::kill_solution::(); + result } } @@ -1330,7 +1332,7 @@ mod tests { #[test] fn phase_rotation_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().build_offchainify(0).0.execute_with(|| { // 0 ------- 15 ------- 25 ------- 30 ------- ------- 45 ------- 55 ------- 60 // | | | | // Signed Unsigned Signed Unsigned @@ -1395,7 +1397,7 @@ mod tests { #[test] fn signed_phase_void() { - ExtBuilder::default().phases(0, 10).build_and_execute(|| { + ExtBuilder::default().phases(0, 10).build_offchainify(0).0.execute_with(|| { roll_to(15); assert!(MultiPhase::current_phase().is_off()); @@ -1418,7 +1420,7 @@ mod tests { #[test] fn unsigned_phase_void() { - ExtBuilder::default().phases(10, 0).build_and_execute(|| { + ExtBuilder::default().phases(10, 0).build_offchainify(0).0.execute_with(|| { roll_to(15); assert!(MultiPhase::current_phase().is_off()); @@ -1441,7 +1443,7 @@ mod tests { #[test] fn both_phases_void() { - ExtBuilder::default().phases(0, 0).build_and_execute(|| { + ExtBuilder::default().phases(0, 0).build_offchainify(0).0.execute_with(|| { roll_to(15); assert!(MultiPhase::current_phase().is_off()); @@ -1464,7 +1466,7 @@ mod tests { #[test] fn early_termination() { // an early termination in the signed phase, with no queued solution. - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().build_offchainify(0).0.execute_with(|| { // signed phase started at block 15 and will end at 25. roll_to(14); assert_eq!(MultiPhase::current_phase(), Phase::Off); @@ -1497,7 +1499,7 @@ mod tests { #[test] fn fallback_strategy_works() { - ExtBuilder::default().fallback(FallbackStrategy::OnChain).build_and_execute(|| { + ExtBuilder::default().fallback(FallbackStrategy::OnChain).build_offchainify(0).0.execute_with(|| { roll_to(15); assert_eq!(MultiPhase::current_phase(), Phase::Signed); @@ -1516,7 +1518,7 @@ mod tests { ) }); - ExtBuilder::default().fallback(FallbackStrategy::Nothing).build_and_execute(|| { + ExtBuilder::default().fallback(FallbackStrategy::Nothing).build_offchainify(0).0.execute_with(|| { roll_to(15); assert_eq!(MultiPhase::current_phase(), Phase::Signed); @@ -1530,7 +1532,7 @@ mod tests { #[test] fn snapshot_creation_fails_if_too_big() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().build_offchainify(0).0.execute_with(|| { Targets::set((0..(TargetIndex::max_value() as AccountId) + 1).collect::>()); // signed phase failed to open. diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index c01a84a4bc511..6388d55a2b053 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -73,21 +73,21 @@ impl From for MinerError { /// Save a given call into OCW storage. fn save_solution(call: &Call) { - let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); + let storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); storage.set(&call); } /// Get a saved solution from OCW storage if it exists. fn restore_solution() -> Result, MinerError> { - StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL) + StorageValueRef::local(&OFFCHAIN_CACHED_CALL) .get() .flatten() .ok_or(MinerError::NoStoredSolution) } /// Clear a saved solution from OCW storage. -fn kill_solution() { - let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); +pub(super) fn kill_solution() { + let mut storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); storage.clear(); } @@ -1027,7 +1027,7 @@ mod tests { // 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); + let mut call_cache = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); assert!(matches!(call_cache.get::>(), Some(Some(_call)))); call_cache.clear(); From 224d5e1289d800d69b5898a303ea392e85016495 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 13:11:48 +0200 Subject: [PATCH 26/45] Revert "use local storage; clear on ElectionFinalized" This reverts commit 4b46a9388532d0c09b337dc7c7edf76044a6cee8. --- .../src/benchmarking.rs | 2 +- .../election-provider-multi-phase/src/lib.rs | 22 +++++++++---------- .../src/unsigned.rs | 10 ++++----- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index c472a75869334..40c7e801ae78d 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -282,6 +282,6 @@ frame_benchmarking::benchmarks! { impl_benchmark_test_suite!( MultiPhase, - crate::mock::ExtBuilder::default().build_offchainify(0).0, + crate::mock::ExtBuilder::default().build(), crate::mock::Runtime, ); diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index d17a7f1ed6294..2eadbb781f66c 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1098,7 +1098,7 @@ impl Pallet { } fn do_elect() -> Result<(Supports, Weight), ElectionError> { - let result = >::take() + >::take() .map_or_else( || match T::Fallback::get() { FallbackStrategy::OnChain => Self::onchain_fallback() @@ -1125,9 +1125,7 @@ impl Pallet { log!(warn, "Failed to finalize election round. reason {:?}", err); } err - }); - unsigned::kill_solution::(); - result + }) } } @@ -1332,7 +1330,7 @@ mod tests { #[test] fn phase_rotation_works() { - ExtBuilder::default().build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // 0 ------- 15 ------- 25 ------- 30 ------- ------- 45 ------- 55 ------- 60 // | | | | // Signed Unsigned Signed Unsigned @@ -1397,7 +1395,7 @@ mod tests { #[test] fn signed_phase_void() { - ExtBuilder::default().phases(0, 10).build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().phases(0, 10).build_and_execute(|| { roll_to(15); assert!(MultiPhase::current_phase().is_off()); @@ -1420,7 +1418,7 @@ mod tests { #[test] fn unsigned_phase_void() { - ExtBuilder::default().phases(10, 0).build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().phases(10, 0).build_and_execute(|| { roll_to(15); assert!(MultiPhase::current_phase().is_off()); @@ -1443,7 +1441,7 @@ mod tests { #[test] fn both_phases_void() { - ExtBuilder::default().phases(0, 0).build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().phases(0, 0).build_and_execute(|| { roll_to(15); assert!(MultiPhase::current_phase().is_off()); @@ -1466,7 +1464,7 @@ mod tests { #[test] fn early_termination() { // an early termination in the signed phase, with no queued solution. - ExtBuilder::default().build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().build_and_execute(|| { // signed phase started at block 15 and will end at 25. roll_to(14); assert_eq!(MultiPhase::current_phase(), Phase::Off); @@ -1499,7 +1497,7 @@ mod tests { #[test] fn fallback_strategy_works() { - ExtBuilder::default().fallback(FallbackStrategy::OnChain).build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().fallback(FallbackStrategy::OnChain).build_and_execute(|| { roll_to(15); assert_eq!(MultiPhase::current_phase(), Phase::Signed); @@ -1518,7 +1516,7 @@ mod tests { ) }); - ExtBuilder::default().fallback(FallbackStrategy::Nothing).build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().fallback(FallbackStrategy::Nothing).build_and_execute(|| { roll_to(15); assert_eq!(MultiPhase::current_phase(), Phase::Signed); @@ -1532,7 +1530,7 @@ mod tests { #[test] fn snapshot_creation_fails_if_too_big() { - ExtBuilder::default().build_offchainify(0).0.execute_with(|| { + ExtBuilder::default().build_and_execute(|| { Targets::set((0..(TargetIndex::max_value() as AccountId) + 1).collect::>()); // signed phase failed to open. diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 6388d55a2b053..c01a84a4bc511 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -73,21 +73,21 @@ impl From for MinerError { /// Save a given call into OCW storage. fn save_solution(call: &Call) { - let storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); + let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); storage.set(&call); } /// Get a saved solution from OCW storage if it exists. fn restore_solution() -> Result, MinerError> { - StorageValueRef::local(&OFFCHAIN_CACHED_CALL) + StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL) .get() .flatten() .ok_or(MinerError::NoStoredSolution) } /// Clear a saved solution from OCW storage. -pub(super) fn kill_solution() { - let mut storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); +fn kill_solution() { + let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); storage.clear(); } @@ -1027,7 +1027,7 @@ mod tests { // 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::local(&OFFCHAIN_CACHED_CALL); + let mut call_cache = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); assert!(matches!(call_cache.get::>(), Some(Some(_call)))); call_cache.clear(); From c5668e9ecc2865f1bc9eb0e0f8a71c4afc78ece2 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 15:08:18 +0200 Subject: [PATCH 27/45] BROKEN: try to filter local events in OCW --- .../election-provider-multi-phase/src/lib.rs | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 2eadbb781f66c..5001406cc52bf 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -231,7 +231,10 @@ use sp_runtime::{ DispatchError, PerThing, Perbill, RuntimeDebug, SaturatedConversion, traits::Bounded, }; -use sp_std::prelude::*; +use sp_std::{ + convert::{TryFrom, TryInto}, + prelude::*, +}; use sp_arithmetic::{ UpperOf, traits::{Zero, CheckedAdd}, @@ -516,7 +519,9 @@ pub mod pallet { #[pallet::config] pub trait Config: frame_system::Config + SendTransactionTypes> { - type Event: From> + IsType<::Event>; + type Event: From> + + IsType<::Event> + + TryFrom<::Event>; /// Currency type. type Currency: ReservableCurrency + Currency; @@ -661,6 +666,17 @@ pub mod pallet { } _ => {} } + // after election finalization, clear OCW solution storage + if >::events() + .into_iter() + .filter_map(|event_record| event_record.event.try_into().ok()) + .find(|event| { + matches!(event, Event::ElectionFinalized(_)) + }) + .is_some() + { + unsigned::kill_solution(); + } } fn integrity_test() { From e9305be22c7e7bbb7ab8581a553b4f85c55e1365 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 15:25:40 +0200 Subject: [PATCH 28/45] use local storage; simplify score fetching --- .../src/unsigned.rs | 35 ++++++++----------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index c01a84a4bc511..39bc1111922cd 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -27,8 +27,8 @@ use frame_support::{dispatch::DispatchResult, ensure, traits::Get}; use frame_system::offchain::SubmitTransaction; use sp_arithmetic::Perbill; use sp_npos_elections::{ - assignment_ratio_to_staked_normalized, assignment_staked_to_ratio_normalized, - is_score_better, seq_phragmen, CompactSolution, ElectionResult, + 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, vec::Vec}; @@ -73,21 +73,21 @@ impl From for MinerError { /// Save a given call into OCW storage. fn save_solution(call: &Call) { - let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); + let storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); storage.set(&call); } /// Get a saved solution from OCW storage if it exists. fn restore_solution() -> Result, MinerError> { - StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL) + StorageValueRef::local(&OFFCHAIN_CACHED_CALL) .get() .flatten() .ok_or(MinerError::NoStoredSolution) } /// Clear a saved solution from OCW storage. -fn kill_solution() { - let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); +pub(super) fn kill_solution() { + let mut storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); storage.clear(); } @@ -95,31 +95,26 @@ impl Pallet { /// 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> { - let call = restore_solution::() + let (call, score) = restore_solution::() .and_then(|call| { // ensure the cached call is still current before submitting let current_round = Self::round(); match &call { Call::submit_unsigned(solution, _) if solution.round == current_round => { - Ok(call) + Ok((call, solution.score.clone())) } _ => Err(MinerError::SolutionOutOfDate), } }) .or_else::(|_| { // if not present or cache invalidated, regenerate - let call = Self::mine_checked_call()?; + let (call, score) = Self::mine_checked_call()?; save_solution(&call); - Ok(call) + Ok((call, score)) })?; - let call_score = match &call { - Call::submit_unsigned(solution, _) => solution.score.clone(), - _ => Default::default(), - }; - if Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( - call_score, + score, q.score, T::SolutionImprovementThreshold::get() )) { @@ -131,13 +126,13 @@ impl Pallet { /// 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> { - let call = Self::mine_checked_call()?; + 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, MinerError> { + fn mine_checked_call() -> Result<(Call, ElectionScore), MinerError> { use codec::Encode; let iters = Self::get_balancing_iters(); @@ -154,7 +149,7 @@ impl Pallet { call.using_encoded(|b| b.len()) ); - Ok(call) + Ok((call, score)) } fn submit_call(call: Call) -> Result<(), MinerError> { @@ -1027,7 +1022,7 @@ mod tests { // 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); + let mut call_cache = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); assert!(matches!(call_cache.get::>(), Some(Some(_call)))); call_cache.clear(); From fe841416e857073b648dc9ff08f532cb28bf89a3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 15:42:15 +0200 Subject: [PATCH 29/45] fix event filter --- frame/election-provider-multi-phase/src/lib.rs | 13 +++++++------ frame/election-provider-multi-phase/src/unsigned.rs | 3 ++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 5001406cc52bf..6f6103ba181d3 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -232,7 +232,7 @@ use sp_runtime::{ traits::Bounded, }; use sp_std::{ - convert::{TryFrom, TryInto}, + convert::TryInto, prelude::*, }; use sp_arithmetic::{ @@ -519,9 +519,7 @@ pub mod pallet { #[pallet::config] pub trait Config: frame_system::Config + SendTransactionTypes> { - type Event: From> + - IsType<::Event> + - TryFrom<::Event>; + type Event: From> + IsType<::Event> + TryInto>; /// Currency type. type Currency: ReservableCurrency + Currency; @@ -669,13 +667,16 @@ pub mod pallet { // after election finalization, clear OCW solution storage if >::events() .into_iter() - .filter_map(|event_record| event_record.event.try_into().ok()) + .filter_map(|event_record| { + let local_event = ::Event::from(event_record.event); + local_event.try_into().ok() + }) .find(|event| { matches!(event, Event::ElectionFinalized(_)) }) .is_some() { - unsigned::kill_solution(); + unsigned::kill_solution::(); } } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 39bc1111922cd..63848aac11130 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -101,7 +101,8 @@ impl Pallet { let current_round = Self::round(); match &call { Call::submit_unsigned(solution, _) if solution.round == current_round => { - Ok((call, solution.score.clone())) + let score = solution.score.clone(); + Ok((call, score)) } _ => Err(MinerError::SolutionOutOfDate), } From 8b31773fdaced855005c21bc115908dafc13f27e Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 29 Mar 2021 16:40:19 +0200 Subject: [PATCH 30/45] mutate storage instead of setting it --- .../election-provider-multi-phase/src/unsigned.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 63848aac11130..7637121f389eb 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -57,6 +57,8 @@ pub enum MinerError { NoStoredSolution, /// Cached solution does not match the current round SolutionOutOfDate, + /// Failed to store a solution + FailedToStoreSolution, } impl From for MinerError { @@ -72,9 +74,13 @@ impl From for MinerError { } /// Save a given call into OCW storage. -fn save_solution(call: &Call) { +fn save_solution(call: &Call) -> Result<(), MinerError> { let storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); - storage.set(&call); + match storage.mutate::<_, (), _>(|_| Ok(call.clone())) { + Ok(Ok(_)) => Ok(()), + Ok(Err(_)) => Err(MinerError::FailedToStoreSolution), + Err(_) => unreachable!("mutation function cannot fail; qed"), + } } /// Get a saved solution from OCW storage if it exists. @@ -110,7 +116,7 @@ impl Pallet { .or_else::(|_| { // if not present or cache invalidated, regenerate let (call, score) = Self::mine_checked_call()?; - save_solution(&call); + save_solution(&call)?; Ok((call, score)) })?; @@ -128,7 +134,7 @@ impl Pallet { /// 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> { let (call, _) = Self::mine_checked_call()?; - save_solution(&call); + save_solution(&call)?; Self::submit_call(call) } From 9978693a9c9b86ae72b41738d70db87c68646ec5 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 30 Mar 2021 09:17:03 +0200 Subject: [PATCH 31/45] StorageValueRef::local isn't actually implemented yet --- frame/election-provider-multi-phase/src/unsigned.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 7637121f389eb..e0be4083aa720 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -75,7 +75,7 @@ impl From for MinerError { /// Save a given call into OCW storage. fn save_solution(call: &Call) -> Result<(), MinerError> { - let storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); + let storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); match storage.mutate::<_, (), _>(|_| Ok(call.clone())) { Ok(Ok(_)) => Ok(()), Ok(Err(_)) => Err(MinerError::FailedToStoreSolution), @@ -85,7 +85,7 @@ fn save_solution(call: &Call) -> Result<(), MinerError> { /// Get a saved solution from OCW storage if it exists. fn restore_solution() -> Result, MinerError> { - StorageValueRef::local(&OFFCHAIN_CACHED_CALL) + StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL) .get() .flatten() .ok_or(MinerError::NoStoredSolution) @@ -93,7 +93,7 @@ fn restore_solution() -> Result, MinerError> { /// Clear a saved solution from OCW storage. pub(super) fn kill_solution() { - let mut storage = StorageValueRef::local(&OFFCHAIN_CACHED_CALL); + let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); storage.clear(); } @@ -1029,7 +1029,7 @@ mod tests { // 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::local(&OFFCHAIN_CACHED_CALL); + let mut call_cache = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); assert!(matches!(call_cache.get::>(), Some(Some(_call)))); call_cache.clear(); From ebcdeaa24107a06c545624f0a3e08b4436240246 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 30 Mar 2021 09:38:36 +0200 Subject: [PATCH 32/45] add logging for some events of interest in OCW miner --- .../src/unsigned.rs | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index e0be4083aa720..cddc9c3492ef5 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -101,6 +101,11 @@ impl Pallet { /// 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!( + info, + "OCW attempting to restore or compute an unsigned solution for the current election" + ); + let (call, score) = restore_solution::() .and_then(|call| { // ensure the cached call is still current before submitting @@ -108,6 +113,14 @@ impl Pallet { match &call { Call::submit_unsigned(solution, _) if solution.round == current_round => { let score = solution.score.clone(); + + log!( + debug, + "restored a cached call for round {} with score {:?}", + current_round, + score, + ); + Ok((call, score)) } _ => Err(MinerError::SolutionOutOfDate), @@ -127,12 +140,22 @@ impl Pallet { )) { Self::submit_call(call) } else { + log!( + info, + "queued solution has better score than OCW mined solution; skipping submission" + ); + Ok(()) } } /// 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!( + info, + "OCW attempting to compute an unsigned solution for the current election" + ); + let (call, _) = Self::mine_checked_call()?; save_solution(&call)?; Self::submit_call(call) @@ -151,7 +174,7 @@ impl Pallet { log!( info, - "mined a solution with score {:?} and size {}", + "OCW mined a solution with score {:?} and size {}", score, call.using_encoded(|b| b.len()) ); @@ -160,6 +183,11 @@ impl Pallet { } fn submit_call(call: Call) -> Result<(), MinerError> { + log!( + debug, + "OCW submitting a solution as an unsigned transaction", + ); + SubmitTransaction::>::submit_unsigned_transaction(call.into()) .map_err(|_| { kill_solution::(); From bd7a19533265c11cf3fcf319f1f9b4c068a7f80c Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 1 Apr 2021 16:21:22 +0200 Subject: [PATCH 33/45] rename kill_solution -> kill_ocw_solution to avoid ambiguity --- frame/election-provider-multi-phase/src/lib.rs | 2 +- frame/election-provider-multi-phase/src/unsigned.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 963d027c59b66..165e462566b54 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -676,7 +676,7 @@ pub mod pallet { }) .is_some() { - unsigned::kill_solution::(); + unsigned::kill_ocw_solution::(); } } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 730ff934a8fbc..117a3595f0634 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -92,7 +92,7 @@ fn restore_solution() -> Result, MinerError> { } /// Clear a saved solution from OCW storage. -pub(super) fn kill_solution() { +pub(super) fn kill_ocw_solution() { let mut storage = StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL); storage.clear(); } @@ -190,7 +190,7 @@ impl Pallet { SubmitTransaction::>::submit_unsigned_transaction(call.into()) .map_err(|_| { - kill_solution::(); + kill_ocw_solution::(); MinerError::PoolSubmissionFailed }) } From 1c896bed7576b2f5c448664bfc07660cb51e4d78 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 1 Apr 2021 16:25:38 +0200 Subject: [PATCH 34/45] defensive err instead of unreachable given unreachable code --- frame/election-provider-multi-phase/src/unsigned.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 117a3595f0634..8090380176e45 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -79,7 +79,13 @@ fn save_solution(call: &Call) -> Result<(), MinerError> { match storage.mutate::<_, (), _>(|_| Ok(call.clone())) { Ok(Ok(_)) => Ok(()), Ok(Err(_)) => Err(MinerError::FailedToStoreSolution), - Err(_) => unreachable!("mutation function cannot fail; qed"), + 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) + }, } } From 2243501d3d2d50a5aea6b0ff390f49923ffcd401 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 1 Apr 2021 16:27:05 +0200 Subject: [PATCH 35/45] doc punctuation Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/election-provider-multi-phase/src/unsigned.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 8090380176e45..75f2bd2a74fd5 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -53,11 +53,11 @@ pub enum MinerError { Feasibility(FeasibilityError), /// Something went wrong fetching the lock. Lock(&'static str), - /// Cannot restore a solution that was not stored + /// Cannot restore a solution that was not stored. NoStoredSolution, - /// Cached solution does not match the current round + /// Cached solution does not match the current round. SolutionOutOfDate, - /// Failed to store a solution + /// Failed to store a solution. FailedToStoreSolution, } From 2b5d0505aca55e73e0cff3bd0c9e4770aa122303 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 1 Apr 2021 16:32:29 +0200 Subject: [PATCH 36/45] distinguish miner errors between "out of date" and "call invalid" --- frame/election-provider-multi-phase/src/unsigned.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 75f2bd2a74fd5..e9fd9abc5d4aa 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -57,6 +57,8 @@ pub enum MinerError { 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, } @@ -116,8 +118,8 @@ impl Pallet { .and_then(|call| { // ensure the cached call is still current before submitting let current_round = Self::round(); - match &call { - Call::submit_unsigned(solution, _) if solution.round == current_round => { + if let Call::submit_unsigned(solution, _) = &call { + if solution.round == current_round { let score = solution.score.clone(); log!( @@ -128,8 +130,11 @@ impl Pallet { ); Ok((call, score)) + } else { + Err(MinerError::SolutionOutOfDate) } - _ => Err(MinerError::SolutionOutOfDate), + } else { + Err(MinerError::SolutionCallInvalid) } }) .or_else::(|_| { From 5eb2673a7a2b06806a44f87c09080192b5687cd3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 1 Apr 2021 16:40:13 +0200 Subject: [PATCH 37/45] downgrade info logs -> debug --- frame/election-provider-multi-phase/src/unsigned.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index e9fd9abc5d4aa..dc7af06dfb06f 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -110,7 +110,7 @@ impl Pallet { /// if our call's score is greater than that of the cached solution. pub fn restore_or_compute_then_maybe_submit() -> Result<(), MinerError> { log!( - info, + debug, "OCW attempting to restore or compute an unsigned solution for the current election" ); @@ -152,7 +152,7 @@ impl Pallet { Self::submit_call(call) } else { log!( - info, + debug, "queued solution has better score than OCW mined solution; skipping submission" ); @@ -163,7 +163,7 @@ impl Pallet { /// 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!( - info, + debug, "OCW attempting to compute an unsigned solution for the current election" ); @@ -184,7 +184,7 @@ impl Pallet { let call: Call = Call::submit_unsigned(raw_solution, witness).into(); log!( - info, + debug, "OCW mined a solution with score {:?} and size {}", score, call.using_encoded(|b| b.len()) From 70b4f71cbbf4767c621783b828a55196822697d5 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 20 Apr 2021 10:27:55 +0200 Subject: [PATCH 38/45] ensure encoded call decodes as a call --- frame/election-provider-multi-phase/src/unsigned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index d8fd37c15ad1f..c5f372d2d348b 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1127,7 +1127,7 @@ mod tests { // 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::>(), Some(Some(_call)))); + assert!(matches!(call_cache.get::>(), Some(Some(_call)))); call_cache.clear(); // attempts to resubmit the tx after the threshold has expired From 9c280487819296d39b6c3eb2f8d23362ddbc3607 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 20 Apr 2021 10:42:23 +0200 Subject: [PATCH 39/45] fix semantics of validation of pre-dispatch failure for wrong round --- .../src/unsigned.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index c5f372d2d348b..a02e9e5925469 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1169,14 +1169,21 @@ mod tests { let encoded = pool.read().transactions[0].clone(); let extrinsic = Extrinsic::decode(&mut &*encoded).unwrap(); - let solution = match extrinsic.call { - OuterCall::MultiPhase(Call::submit_unsigned(solution, _)) => solution, + let call = match extrinsic.call { + OuterCall::MultiPhase(call @ Call::submit_unsigned(..)) => call, _ => panic!("bad call: unexpected submission"), }; - assert_noop!( - MultiPhase::unsigned_pre_dispatch_checks(&solution), - Error::::OcwCallWrongEra, + // Custom(3) maps to PreDispatchChecksFailed + let pre_dispatch_check_error = TransactionValidityError::Invalid(InvalidTransaction::Custom(3)); + assert_eq!( + ::validate_unsigned(TransactionSource::Local, &call) + .unwrap_err(), + pre_dispatch_check_error, + ); + assert_eq!( + ::pre_dispatch(&call).unwrap_err(), + pre_dispatch_check_error, ); }) } From 3f1109b676deb1d079dee7392ae81d146208aba3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 20 Apr 2021 11:28:35 +0200 Subject: [PATCH 40/45] move score check within `and_then` --- .../src/unsigned.rs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index a02e9e5925469..f92c3612a0efe 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -116,7 +116,7 @@ impl Pallet { "OCW attempting to restore or compute an unsigned solution for the current election" ); - let (call, score) = restore_solution::() + let (call, _score) = restore_solution::() .and_then(|call| { // ensure the cached call is still current before submitting let current_round = Self::round(); @@ -131,7 +131,18 @@ impl Pallet { score, ); - Ok((call, score)) + if Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( + score, + q.score, + T::SolutionImprovementThreshold::get() + )) { + Ok((call, score)) + } else { + // not technically accurate; it's valid, just scores too low. + // Still, this will cause a re-mine in the `or_else` clause; + // we might get lucky and generate a higher-scoring solution this time. + Err(MinerError::SolutionCallInvalid) + } } else { Err(MinerError::SolutionOutOfDate) } @@ -146,20 +157,7 @@ impl Pallet { Ok((call, score)) })?; - if Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( - score, - q.score, - T::SolutionImprovementThreshold::get() - )) { - Self::submit_call(call) - } else { - log!( - debug, - "queued solution has better score than OCW mined solution; skipping submission" - ); - - Ok(()) - } + Self::submit_call(call) } /// Mine a new solution, cache it, and submit it back to the chain as an unsigned transaction. From af2d2c60393026ca95354e1033bbdd8e182293a7 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 20 Apr 2021 11:53:38 +0200 Subject: [PATCH 41/45] add test that offchain workers clear their cache after election --- .../src/unsigned.rs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index f92c3612a0efe..17de249508b4c 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -107,6 +107,18 @@ pub(super) fn kill_ocw_solution() { storage.clear(); } +/// `true` when OCW storage contains a solution +/// +/// More precise than `restore_solution::().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() -> bool { + matches!( + StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL).get::>(), + Some(_), + ) +} + impl Pallet { /// 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. @@ -1057,6 +1069,33 @@ mod tests { }) } + #[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(); + + assert!(!ocw_solution_exists::(), "no solution should be present before we mine one"); + + // creates and cache a solution + MultiPhase::offchain_worker(25); + assert!(ocw_solution_exists::(), "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::(), "elections must clear the ocw cache"); + }) + } + #[test] fn ocw_resubmits_after_offchain_repeat() { let (mut ext, pool) = ExtBuilder::default().build_offchainify(0); From fc71d3dc1b6fa820a885b04346bf0b6cd89628d8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 21 Apr 2021 16:59:21 +0200 Subject: [PATCH 42/45] ensure that bad ocw submissions are not retained for resubmission --- frame/election-provider-multi-phase/src/unsigned.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 17de249508b4c..0ed7c50dba1c6 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -169,7 +169,17 @@ impl Pallet { Ok((call, score)) })?; - Self::submit_call(call) + Self::submit_call(call).map_err(|err| { + // in case submission failed because of a bad solution, ensure that next time, + // we mine a new one. + match err { + MinerError::PreDispatchChecksFailed | MinerError::PoolSubmissionFailed => { + kill_ocw_solution::() + } + _ => {} + } + err + }) } /// Mine a new solution, cache it, and submit it back to the chain as an unsigned transaction. From 840bad33b7ac9d7d7f8dd4e71cd96f792700b799 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 23 Apr 2021 13:16:35 +0200 Subject: [PATCH 43/45] simplify fn ocw_solution_exists --- frame/election-provider-multi-phase/src/unsigned.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 0ed7c50dba1c6..fd2b65661e1af 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -113,10 +113,7 @@ pub(super) fn kill_ocw_solution() { /// if a solution exists but cannot be decoded, whereas this just checks whether an item is present. #[cfg(test)] fn ocw_solution_exists() -> bool { - matches!( - StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL).get::>(), - Some(_), - ) + StorageValueRef::persistent(&OFFCHAIN_CACHED_CALL).get::>().is_some() } impl Pallet { From 8eaf4811e1d169d0774357e5de4d19321eb41edb Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 23 Apr 2021 14:46:52 +0200 Subject: [PATCH 44/45] 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. --- .../src/unsigned.rs | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index fd2b65661e1af..e39917acee465 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -128,33 +128,37 @@ impl Pallet { let (call, _score) = restore_solution::() .and_then(|call| { // ensure the cached call is still current before submitting - let current_round = Self::round(); if let Call::submit_unsigned(solution, _) = &call { - if solution.round == current_round { - let score = solution.score.clone(); - - log!( - debug, - "restored a cached call for round {} with score {:?}", - current_round, - score, - ); - - if Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( - score, - q.score, - T::SolutionImprovementThreshold::get() - )) { - Ok((call, score)) - } else { - // not technically accurate; it's valid, just scores too low. - // Still, this will cause a re-mine in the `or_else` clause; - // we might get lucky and generate a higher-scoring solution this time. - Err(MinerError::SolutionCallInvalid) - } - } else { - Err(MinerError::SolutionOutOfDate) + // not out of date by round + let current_round = Self::round(); + if solution.round != current_round { + return Err(MinerError::SolutionOutOfDate); } + + // higher score than what's currently queued + let score = solution.score.clone(); + if !Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( + score, + q.score, + T::SolutionImprovementThreshold::get() + )) { + // not technically accurate; it's valid, just scores too low. + // Still, this will cause a re-mine in the `or_else` clause; + // we might get lucky and generate a higher-scoring solution this time. + return Err(MinerError::SolutionCallInvalid); + } + + // prevent errors arising from state changes in a forkful chain + Self::feasibility_check(solution.clone(), ElectionCompute::Unsigned)?; + + // checks complete + log!( + debug, + "restored a cached call for round {} with score {:?}", + current_round, + score, + ); + Ok((call, score)) } else { Err(MinerError::SolutionCallInvalid) } From f4b52f0367cd258bc157d5a707d6b65eb9e159e9 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 23 Apr 2021 15:42:32 +0200 Subject: [PATCH 45/45] simplify checks again --- .../src/unsigned.rs | 90 +++++++------------ 1 file changed, 32 insertions(+), 58 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index e39917acee465..ab9a0f610f7bb 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -125,62 +125,33 @@ impl Pallet { "OCW attempting to restore or compute an unsigned solution for the current election" ); - let (call, _score) = restore_solution::() + let call = restore_solution::() .and_then(|call| { // ensure the cached call is still current before submitting if let Call::submit_unsigned(solution, _) = &call { - // not out of date by round - let current_round = Self::round(); - if solution.round != current_round { - return Err(MinerError::SolutionOutOfDate); - } - - // higher score than what's currently queued - let score = solution.score.clone(); - if !Self::queued_solution().map_or(true, |q: ReadySolution<_>| is_score_better::( - score, - q.score, - T::SolutionImprovementThreshold::get() - )) { - // not technically accurate; it's valid, just scores too low. - // Still, this will cause a re-mine in the `or_else` clause; - // we might get lucky and generate a higher-scoring solution this time. - return Err(MinerError::SolutionCallInvalid); - } - // prevent errors arising from state changes in a forkful chain - Self::feasibility_check(solution.clone(), ElectionCompute::Unsigned)?; - - // checks complete - log!( - debug, - "restored a cached call for round {} with score {:?}", - current_round, - score, - ); - Ok((call, score)) + Self::basic_checks(solution, "restored")?; + Ok(call) } else { Err(MinerError::SolutionCallInvalid) } }) .or_else::(|_| { // if not present or cache invalidated, regenerate - let (call, score) = Self::mine_checked_call()?; + let (call, _) = Self::mine_checked_call()?; save_solution(&call)?; - Ok((call, score)) + Ok(call) })?; - Self::submit_call(call).map_err(|err| { - // in case submission failed because of a bad solution, ensure that next time, - // we mine a new one. - match err { - MinerError::PreDispatchChecksFailed | MinerError::PoolSubmissionFailed => { - kill_ocw_solution::() - } - _ => {} - } - err - }) + // 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. @@ -227,6 +198,23 @@ impl Pallet { }) } + // perform basic checks of a solution's validity + // + // Performance: note that it internally clones the provided solution. + fn basic_checks(raw_solution: &RawSolution>, 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 /// to the chain. /// @@ -237,21 +225,7 @@ impl Pallet { iters: usize, ) -> Result<(RawSolution>, 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)) }