From 01e0c2a3eda4d233c9224a33d863f9724eff4791 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Tue, 19 Sep 2023 14:21:05 +0300 Subject: [PATCH 01/13] Wrap ERP types with SnapshotWrapper --- .../election-provider-multi-phase/src/lib.rs | 66 ++++++++++++------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 8b6e0827c715..3d9a0a78fd12 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -275,14 +275,16 @@ pub mod migrations; pub mod signed; pub mod unsigned; pub mod weights; -use unsigned::VoterOf; -pub use weights::WeightInfo; + +use frame_support::pallet_prelude::PhantomData; pub use signed::{ BalanceOf, GeometricDepositBase, NegativeImbalanceOf, PositiveImbalanceOf, SignedSubmission, SignedSubmissionOf, SignedSubmissions, SubmissionIndicesOf, }; +use unsigned::VoterOf; pub use unsigned::{Miner, MinerConfig}; +pub use weights::WeightInfo; /// The solution type used by this crate. pub type SolutionOf = ::Solution; @@ -1354,6 +1356,37 @@ pub mod pallet { pub struct Pallet(_); } +/// This Wrapper is created for handling the synchronization of `Snapshot`, `SnapshotMetadata` and +/// `DesiredTargets` storage items +pub struct SnapshotWrapper(PhantomData); + +impl SnapshotWrapper { + /// kill all snapshot related storage items at the same time + pub fn kill() { + >::kill(); + >::kill(); + >::kill(); + } + /// Set all snapshot related storage items at the same time + pub fn set(metadata: SolutionOrSnapshotSize, desired_targets: u32, buffer: Vec) { + >::put(metadata); + >::put(desired_targets); + sp_io::storage::set(&>::hashed_key(), &buffer); + } + + /// Check if all of the storage items exist + pub fn exists() -> bool { + >::exists() && >::exists() && >::exists() + } + + /// Check if all of the storage items do not exist + pub fn not_exists() -> bool { + !>::exists() + && !>::exists() + && !>::exists() + } +} + impl Pallet { /// Internal logic of the offchain worker, to be executed only when the offchain lock is /// acquired with success. @@ -1405,9 +1438,6 @@ impl Pallet { SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32 }; log!(info, "creating a snapshot with metadata {:?}", metadata); - >::put(metadata); - >::put(desired_targets); - // instead of using storage APIs, we do a manual encoding into a fixed-size buffer. // `encoded_size` encodes it without storing it anywhere, this should not cause any // allocation. @@ -1422,7 +1452,7 @@ impl Pallet { // buffer should have not re-allocated since. debug_assert!(buffer.len() == size && size == buffer.capacity()); - sp_io::storage::set(&>::hashed_key(), &buffer); + SnapshotWrapper::::set(metadata, desired_targets, buffer); } /// Parts of [`create_snapshot`] that happen outside of this pallet. @@ -1505,9 +1535,7 @@ impl Pallet { /// Kill everything created by [`Pallet::create_snapshot`]. pub fn kill_snapshot() { - >::kill(); - >::kill(); - >::kill(); + SnapshotWrapper::::kill(); } /// Checks the feasibility of a solution. @@ -1616,15 +1644,7 @@ impl Pallet { // - [`DesiredTargets`] exists if and only if [`Snapshot`] is present. // - [`SnapshotMetadata`] exist if and only if [`Snapshot`] is present. fn try_state_snapshot() -> Result<(), TryRuntimeError> { - if >::exists() && - >::exists() && - >::exists() - { - Ok(()) - } else if !>::exists() && - !>::exists() && - !>::exists() - { + if SnapshotWrapper::::exists() || SnapshotWrapper::::not_exists() { Ok(()) } else { Err("If snapshot exists, metadata and desired targets should be set too. Otherwise, none should be set.".into()) @@ -1759,18 +1779,14 @@ mod feasibility_check { assert!(MultiPhase::current_phase().is_signed()); let solution = raw_solution(); - // For whatever reason it might be: - >::kill(); + // kill all `Snapshot, `SnapshotMetadata` and `DesiredTargets` for the storage state to be + // consistent, by using the `SnapshotWrapper` for the try_state checks to pass. + >::kill(); assert_noop!( MultiPhase::feasibility_check(solution, COMPUTE), FeasibilityError::SnapshotUnavailable ); - - // kill also `SnapshotMetadata` and `DesiredTargets` for the storage state to be - // consistent for the try_state checks to pass. - >::kill(); - >::kill(); }) } From 01e4a264d6528734bdcfb84f3d66e58ccdf57bd9 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Tue, 19 Sep 2023 14:28:16 +0300 Subject: [PATCH 02/13] fmt --- polkadot/xcm/xcm-builder/src/universal_exports.rs | 8 ++++++-- .../frame/election-provider-multi-phase/src/lib.rs | 10 +++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/universal_exports.rs b/polkadot/xcm/xcm-builder/src/universal_exports.rs index d1b8c8c721fe..a24631ffa942 100644 --- a/polkadot/xcm/xcm-builder/src/universal_exports.rs +++ b/polkadot/xcm/xcm-builder/src/universal_exports.rs @@ -171,7 +171,9 @@ impl SnapshotWrapper { /// Check if all of the storage items do not exist pub fn not_exists() -> bool { - !>::exists() - && !>::exists() - && !>::exists() + !>::exists() && + !>::exists() && + !>::exists() } } @@ -1779,8 +1779,8 @@ mod feasibility_check { assert!(MultiPhase::current_phase().is_signed()); let solution = raw_solution(); - // kill all `Snapshot, `SnapshotMetadata` and `DesiredTargets` for the storage state to be - // consistent, by using the `SnapshotWrapper` for the try_state checks to pass. + // kill all `Snapshot, `SnapshotMetadata` and `DesiredTargets` for the storage state to + // be consistent, by using the `SnapshotWrapper` for the try_state checks to pass. >::kill(); assert_noop!( From 78f0ca366ab6ebd81198515eca23ec60fc7bbefb Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Fri, 22 Sep 2023 12:56:13 +0300 Subject: [PATCH 03/13] correct doc comments --- .../frame/election-provider-multi-phase/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 776ad8b2982d..c0fab3900169 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1356,30 +1356,30 @@ pub mod pallet { pub struct Pallet(_); } -/// This Wrapper is created for handling the synchronization of `Snapshot`, `SnapshotMetadata` and -/// `DesiredTargets` storage items +/// This wrapper is created for handling the synchronization of [`Snapshot`], [`SnapshotMetadata`] +/// and [`DesiredTargets`] storage items. pub struct SnapshotWrapper(PhantomData); impl SnapshotWrapper { - /// kill all snapshot related storage items at the same time + /// Kill all snapshot related storage items at the same time. pub fn kill() { >::kill(); >::kill(); >::kill(); } - /// Set all snapshot related storage items at the same time + /// Set all snapshot related storage items at the same time. pub fn set(metadata: SolutionOrSnapshotSize, desired_targets: u32, buffer: Vec) { >::put(metadata); >::put(desired_targets); sp_io::storage::set(&>::hashed_key(), &buffer); } - /// Check if all of the storage items exist + /// Check if all of the storage items exist. pub fn exists() -> bool { >::exists() && >::exists() && >::exists() } - /// Check if all of the storage items do not exist + /// Check if all of the storage items do not exist. pub fn not_exists() -> bool { !>::exists() && !>::exists() && From a6a80feb0bd825c3164800d9e94e393d8317c031 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Fri, 22 Sep 2023 12:57:22 +0300 Subject: [PATCH 04/13] Avoid cluttering imports with PhantomData --- substrate/frame/election-provider-multi-phase/src/lib.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index c0fab3900169..0ebf748affc7 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1358,7 +1358,7 @@ pub mod pallet { /// This wrapper is created for handling the synchronization of [`Snapshot`], [`SnapshotMetadata`] /// and [`DesiredTargets`] storage items. -pub struct SnapshotWrapper(PhantomData); +pub struct SnapshotWrapper(sp_std::marker::PhantomData); impl SnapshotWrapper { /// Kill all snapshot related storage items at the same time. @@ -1533,11 +1533,6 @@ impl Pallet { ); } - /// Kill everything created by [`Pallet::create_snapshot`]. - pub fn kill_snapshot() { - SnapshotWrapper::::kill(); - } - /// Checks the feasibility of a solution. pub fn feasibility_check( raw_solution: RawSolution>, From 3d03a8ea230813b6ad565ff9ec068216a74c6a10 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Fri, 22 Sep 2023 12:59:21 +0300 Subject: [PATCH 05/13] Address kill snapshot --- substrate/frame/election-provider-multi-phase/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 0ebf748affc7..6d2f44c30a21 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -276,8 +276,6 @@ pub mod signed; pub mod unsigned; pub mod weights; -use frame_support::pallet_prelude::PhantomData; - pub use signed::{ BalanceOf, GeometricDepositBase, NegativeImbalanceOf, PositiveImbalanceOf, SignedSubmission, SignedSubmissionOf, SignedSubmissions, SubmissionIndicesOf, @@ -1567,8 +1565,8 @@ impl Pallet { // Phase is off now. Self::phase_transition(Phase::Off); - // Kill snapshots. - Self::kill_snapshot(); + // Kill snapshots (everything created by [`Pallet::create_snapshot`]). + SnapshotWrapper::::kill(); } fn do_elect() -> Result, ElectionError> { From a559852b54aa3a38f2559e4d9995005bff974a84 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Fri, 22 Sep 2023 14:09:36 +0300 Subject: [PATCH 06/13] Gate is_consistent with try_runtime --- .../election-provider-multi-phase/src/lib.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 6d2f44c30a21..576b016bca47 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1372,16 +1372,16 @@ impl SnapshotWrapper { sp_io::storage::set(&>::hashed_key(), &buffer); } - /// Check if all of the storage items exist. - pub fn exists() -> bool { - >::exists() && >::exists() && >::exists() - } - - /// Check if all of the storage items do not exist. - pub fn not_exists() -> bool { - !>::exists() && - !>::exists() && - !>::exists() + /// Check if all of the storage items exist at the same time or all of the storage items do not + /// exist. + #[cfg(feature = "try-runtime")] + pub fn is_consistent() -> bool { + (>::exists() && + >::exists() && + >::exists()) || + (!>::exists() && + !>::exists() && + !>::exists()) } } @@ -1637,7 +1637,7 @@ impl Pallet { // - [`DesiredTargets`] exists if and only if [`Snapshot`] is present. // - [`SnapshotMetadata`] exist if and only if [`Snapshot`] is present. fn try_state_snapshot() -> Result<(), TryRuntimeError> { - if SnapshotWrapper::::exists() || SnapshotWrapper::::not_exists() { + if SnapshotWrapper::::is_consistent() { Ok(()) } else { Err("If snapshot exists, metadata and desired targets should be set too. Otherwise, none should be set.".into()) From a198690e902e43aebec10f06f003564a130e3136 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Wed, 11 Oct 2023 18:46:48 +0200 Subject: [PATCH 07/13] Address comments --- substrate/frame/election-provider-multi-phase/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 576b016bca47..3fba03d07c77 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1565,7 +1565,7 @@ impl Pallet { // Phase is off now. Self::phase_transition(Phase::Off); - // Kill snapshots (everything created by [`Pallet::create_snapshot`]). + // Kill snapshot and relevant metadata (everything created by [`SnapshotMetadata::set`]). SnapshotWrapper::::kill(); } @@ -1772,7 +1772,7 @@ mod feasibility_check { assert!(MultiPhase::current_phase().is_signed()); let solution = raw_solution(); - // kill all `Snapshot, `SnapshotMetadata` and `DesiredTargets` for the storage state to + // kill `Snapshot`, `SnapshotMetadata` and `DesiredTargets` for the storage state to // be consistent, by using the `SnapshotWrapper` for the try_state checks to pass. >::kill(); From 3e7bf1e09fc054af1b96f012cd09f3f765e46d27 Mon Sep 17 00:00:00 2001 From: Nikos Kontakis Date: Fri, 13 Oct 2023 22:37:24 +0300 Subject: [PATCH 08/13] Add note mentioning that Snapshot, SnapshotMetadata and DesiredTargets must only be mutated throught the SnapshotWrapper --- substrate/frame/election-provider-multi-phase/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 3fba03d07c77..531a6c80d3f1 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1278,6 +1278,7 @@ pub mod pallet { /// Snapshot data of the round. /// /// This is created at the beginning of the signed phase and cleared upon calling `elect`. + /// Note: This storage type must only be mutated through[`SnapshotWrapper`]. #[pallet::storage] #[pallet::getter(fn snapshot)] pub type Snapshot = StorageValue<_, RoundSnapshot>>; @@ -1285,6 +1286,7 @@ pub mod pallet { /// Desired number of targets to elect for this round. /// /// Only exists when [`Snapshot`] is present. + /// Note: This storage type must only be mutated through[`SnapshotWrapper`]. #[pallet::storage] #[pallet::getter(fn desired_targets)] pub type DesiredTargets = StorageValue<_, u32>; @@ -1292,6 +1294,7 @@ pub mod pallet { /// The metadata of the [`RoundSnapshot`] /// /// Only exists when [`Snapshot`] is present. + /// Note: This storage type must only be mutated through[`SnapshotWrapper`]. #[pallet::storage] #[pallet::getter(fn snapshot_metadata)] pub type SnapshotMetadata = StorageValue<_, SolutionOrSnapshotSize>; From 95d0697ba264ea5131bff9fb14b353b937889a71 Mon Sep 17 00:00:00 2001 From: wirednkod Date: Wed, 25 Oct 2023 16:42:56 +0300 Subject: [PATCH 09/13] minor typos --- substrate/frame/election-provider-multi-phase/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 531a6c80d3f1..2a5d446f2ba9 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1278,7 +1278,7 @@ pub mod pallet { /// Snapshot data of the round. /// /// This is created at the beginning of the signed phase and cleared upon calling `elect`. - /// Note: This storage type must only be mutated through[`SnapshotWrapper`]. + /// Note: This storage type must only be mutated through [`SnapshotWrapper`]. #[pallet::storage] #[pallet::getter(fn snapshot)] pub type Snapshot = StorageValue<_, RoundSnapshot>>; @@ -1286,7 +1286,7 @@ pub mod pallet { /// Desired number of targets to elect for this round. /// /// Only exists when [`Snapshot`] is present. - /// Note: This storage type must only be mutated through[`SnapshotWrapper`]. + /// Note: This storage type must only be mutated through [`SnapshotWrapper`]. #[pallet::storage] #[pallet::getter(fn desired_targets)] pub type DesiredTargets = StorageValue<_, u32>; @@ -1294,7 +1294,7 @@ pub mod pallet { /// The metadata of the [`RoundSnapshot`] /// /// Only exists when [`Snapshot`] is present. - /// Note: This storage type must only be mutated through[`SnapshotWrapper`]. + /// Note: This storage type must only be mutated through [`SnapshotWrapper`]. #[pallet::storage] #[pallet::getter(fn snapshot_metadata)] pub type SnapshotMetadata = StorageValue<_, SolutionOrSnapshotSize>; From 34c2f73447387b6b3e44216e54c8afc028c906f7 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 6 Nov 2023 08:57:11 +0000 Subject: [PATCH 10/13] Update substrate/frame/election-provider-multi-phase/src/lib.rs --- substrate/frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index d101eeb354fd..980a1c40b6fc 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1369,7 +1369,7 @@ impl SnapshotWrapper { >::kill(); } /// Set all snapshot related storage items at the same time. - pub fn set(metadata: SolutionOrSnapshotSize, desired_targets: u32, buffer: Vec) { + pub fn set(metadata: SolutionOrSnapshotSize, desired_targets: u32, buffer: &[u8]) { >::put(metadata); >::put(desired_targets); sp_io::storage::set(&>::hashed_key(), &buffer); From 105c52e2f67af2644adf6ec47d46357f2243eda5 Mon Sep 17 00:00:00 2001 From: wirednkod Date: Mon, 6 Nov 2023 14:35:47 +0200 Subject: [PATCH 11/13] minor fix --- .../frame/election-provider-multi-phase/src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index d101eeb354fd..a1632f33582e 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1379,12 +1379,14 @@ impl SnapshotWrapper { /// exist. #[cfg(feature = "try-runtime")] pub fn is_consistent() -> bool { - (>::exists() && - >::exists() && - >::exists()) || - (!>::exists() && - !>::exists() && - !>::exists()) + let snapshots = [ + >::exists(), + >::exists(), + >::exists(), + ]; + + // All should either exist or not exist + snapshots.iter().skip(1).all(|v| snapshots[0] == v) } } From a8c7cbd95e261638036df753741a16a7fc86b56f Mon Sep 17 00:00:00 2001 From: wirednkod Date: Tue, 7 Nov 2023 11:35:32 +0200 Subject: [PATCH 12/13] Address comment --- substrate/frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index f9030d9f9de6..3fb4bb822f75 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1455,7 +1455,7 @@ impl Pallet { // buffer should have not re-allocated since. debug_assert!(buffer.len() == size && size == buffer.capacity()); - SnapshotWrapper::::set(metadata, desired_targets, buffer); + SnapshotWrapper::::set(metadata, desired_targets, &buffer); } /// Parts of [`create_snapshot`] that happen outside of this pallet. From 487f4ff9dff9c884fa786cf87ef3ed57f45da2eb Mon Sep 17 00:00:00 2001 From: wirednkod Date: Tue, 21 Nov 2023 16:03:50 +0200 Subject: [PATCH 13/13] minor fix --- substrate/frame/election-provider-multi-phase/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 3fb4bb822f75..fef1e7313769 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1386,7 +1386,7 @@ impl SnapshotWrapper { ]; // All should either exist or not exist - snapshots.iter().skip(1).all(|v| snapshots[0] == v) + snapshots.iter().skip(1).all(|v| snapshots[0] == *v) } }