From 5043220146361b7a733ede82f1286d8620857aa4 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 29 Sep 2021 13:34:39 +0200 Subject: [PATCH 1/3] dont read events in elections anymore. --- .../election-provider-multi-phase/src/lib.rs | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 6b0329afc0d77..b4a5dbcc162f4 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1201,8 +1201,12 @@ impl Pallet { match current_phase { Phase::Unsigned((true, opened)) if opened == now => { // Mine a new solution, cache it, and attempt to submit it - let initial_output = Self::ensure_offchain_repeat_frequency(now) - .and_then(|_| Self::mine_check_save_submit()); + let initial_output = Self::ensure_offchain_repeat_frequency(now).and_then(|_| { + // This is executed at the beginning of each round. Any cache is now invalid. + // Clear it. + kill_ocw_solution::(); + Self::mine_check_save_submit() + }); log!(debug, "initial offchain thread output: {:?}", initial_output); }, Phase::Unsigned((true, opened)) if opened < now => { @@ -1214,20 +1218,6 @@ impl Pallet { }, _ => {}, } - - // After election finalization, clear OCW solution storage. - // - // We can read the events here because offchain worker doesn't affect PoV. - if >::read_events_no_consensus() - .into_iter() - .filter_map(|event_record| { - let local_event = ::Event::from(event_record.event); - local_event.try_into().ok() - }) - .any(|event| matches!(event, Event::ElectionFinalized(_))) - { - unsigned::kill_ocw_solution::(); - } } /// Logic for [`::on_initialize`] when signed phase is being opened. From ca69b725b043cbc6d0bb15fec51bae41e03ce84f Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 30 Sep 2021 23:28:21 -0400 Subject: [PATCH 2/3] Update frame/election-provider-multi-phase/src/lib.rs --- frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index b4a5dbcc162f4..9508b3a7c83fe 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1204,7 +1204,7 @@ impl Pallet { let initial_output = Self::ensure_offchain_repeat_frequency(now).and_then(|_| { // This is executed at the beginning of each round. Any cache is now invalid. // Clear it. - kill_ocw_solution::(); + unsigned::kill_ocw_solution::(); Self::mine_check_save_submit() }); log!(debug, "initial offchain thread output: {:?}", initial_output); From 32b69da50729f5e1a5e5ad952820ff3e95ee5c2e Mon Sep 17 00:00:00 2001 From: Zeke Mostov <32168567+emostov@users.noreply.github.com> Date: Thu, 30 Sep 2021 23:11:23 -0700 Subject: [PATCH 3/3] Fix test for Substrate#9898 (#9907) --- .../src/unsigned.rs | 55 ++++++++++++++----- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 31ad502ac076e..0ed9b5427b1ec 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1241,35 +1241,62 @@ mod tests { } #[test] - fn ocw_clears_cache_after_election() { - let (mut ext, _pool) = ExtBuilder::default().build_offchainify(0); + fn ocw_clears_cache_on_unsigned_phase_open() { + let (mut ext, pool) = ExtBuilder::default().build_offchainify(0); ext.execute_with(|| { - roll_to(25); - assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25))); + const BLOCK: u64 = 25; + let block_plus = |delta: u64| BLOCK + delta; + let offchain_repeat = ::OffchainRepeat::get(); - // we must clear the offchain storage to ensure the offchain execution check doesn't get - // in the way. - let mut storage = StorageValueRef::persistent(&OFFCHAIN_LAST_BLOCK); - storage.clear(); + roll_to(BLOCK); + // we are on the first block of the unsigned phase + assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, BLOCK))); assert!( !ocw_solution_exists::(), "no solution should be present before we mine one", ); - // creates and cache a solution - MultiPhase::offchain_worker(25); + // create and cache a solution on the first block of the unsigned phase + MultiPhase::offchain_worker(BLOCK); assert!( ocw_solution_exists::(), "a solution must be cached after running the worker", ); - // after an election, the solution must be cleared + // record the submitted tx, + let tx_cache_1 = pool.read().transactions[0].clone(); + // and assume it has been processed. + pool.try_write().unwrap().transactions.clear(); + + // after an election, the solution is not 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"); + MultiPhase::offchain_worker(block_plus(1)); + assert!(ocw_solution_exists::(), "elections does not clear the ocw cache"); + + // submit a solution with the offchain worker after the repeat interval + MultiPhase::offchain_worker(block_plus(offchain_repeat + 1)); + + // record the submitted tx, + let tx_cache_2 = pool.read().transactions[0].clone(); + // and assume it has been processed. + pool.try_write().unwrap().transactions.clear(); + + // the OCW submitted the same solution twice since the cache was not cleared. + assert_eq!(tx_cache_1, tx_cache_2); + + let current_block = block_plus(offchain_repeat * 2 + 2); + // force the unsigned phase to start on the current block. + CurrentPhase::::set(Phase::Unsigned((true, current_block))); + + // clear the cache and create a solution since we are on the first block of the unsigned + // phase. + MultiPhase::offchain_worker(current_block); + let tx_cache_3 = pool.read().transactions[0].clone(); + + // the submitted solution changes because the cache was cleared. + assert_eq!(tx_cache_1, tx_cache_3); }) }