Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement wrapping of EPM types #1633

Merged
merged 24 commits into from
Jan 22, 2024
Merged
Changes from 10 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
01e0c2a
Wrap ERP types with SnapshotWrapper
wirednkod Sep 19, 2023
01e4a26
fmt
wirednkod Sep 19, 2023
0390c85
Merge branch 'master' of github.com:paritytech/polkadot-sdk into nik-…
wirednkod Sep 21, 2023
5988d85
Merge branch 'master' into nik-wrap-epm-types-with-snapshotwrapper
wirednkod Sep 21, 2023
2838fee
Merge branch 'nik-wrap-epm-types-with-snapshotwrapper' of github.com:…
wirednkod Sep 21, 2023
78f0ca3
correct doc comments
wirednkod Sep 22, 2023
a6a80fe
Avoid cluttering imports with PhantomData
wirednkod Sep 22, 2023
3d03a8e
Address kill snapshot
wirednkod Sep 22, 2023
4167849
Merge branch 'master' of github.com:paritytech/polkadot-sdk into nik-…
wirednkod Sep 22, 2023
a559852
Gate is_consistent with try_runtime
wirednkod Sep 22, 2023
a198690
Address comments
wirednkod Oct 11, 2023
3e7bf1e
Add note mentioning that Snapshot, SnapshotMetadata and DesiredTarget…
wirednkod Oct 13, 2023
9c556be
Merge branch 'master' into nik-wrap-epm-types-with-snapshotwrapper
wirednkod Oct 16, 2023
95d0697
minor typos
wirednkod Oct 25, 2023
07a829f
Merge branch 'nik-wrap-epm-types-with-snapshotwrapper' of github.com:…
wirednkod Oct 25, 2023
944d49e
Merge branch 'master' into nik-wrap-epm-types-with-snapshotwrapper
gpestana Oct 26, 2023
63142fa
Merge branch 'master' into nik-wrap-epm-types-with-snapshotwrapper
kianenigma Nov 6, 2023
34c2f73
Update substrate/frame/election-provider-multi-phase/src/lib.rs
kianenigma Nov 6, 2023
105c52e
minor fix
wirednkod Nov 6, 2023
db4364d
Merge branch 'nik-wrap-epm-types-with-snapshotwrapper' of github.com:…
wirednkod Nov 6, 2023
a8c7cbd
Address comment
wirednkod Nov 7, 2023
487f4ff
minor fix
wirednkod Nov 21, 2023
f7068b9
Merge branch 'master' into nik-wrap-epm-types-with-snapshotwrapper
wirednkod Nov 23, 2023
52983ae
Merge branch 'master' into nik-wrap-epm-types-with-snapshotwrapper
bkchr Jan 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 40 additions & 31 deletions substrate/frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,14 @@ pub mod migrations;
pub mod signed;
pub mod unsigned;
pub mod weights;
use unsigned::VoterOf;
pub use weights::WeightInfo;

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<T> = <T as MinerConfig>::Solution;
Expand Down Expand Up @@ -1354,6 +1354,37 @@ pub mod pallet {
pub struct Pallet<T>(_);
}

gpestana marked this conversation as resolved.
Show resolved Hide resolved
/// This wrapper is created for handling the synchronization of [`Snapshot`], [`SnapshotMetadata`]
/// and [`DesiredTargets`] storage items.
pub struct SnapshotWrapper<T>(sp_std::marker::PhantomData<T>);

impl<T: Config> SnapshotWrapper<T> {
/// Kill all snapshot related storage items at the same time.
pub fn kill() {
<Snapshot<T>>::kill();
<SnapshotMetadata<T>>::kill();
<DesiredTargets<T>>::kill();
}
/// Set all snapshot related storage items at the same time.
pub fn set(metadata: SolutionOrSnapshotSize, desired_targets: u32, buffer: Vec<u8>) {
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
<SnapshotMetadata<T>>::put(metadata);
<DesiredTargets<T>>::put(desired_targets);
sp_io::storage::set(&<Snapshot<T>>::hashed_key(), &buffer);
}

/// 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 {
(<Snapshot<T>>::exists() &&
<SnapshotMetadata<T>>::exists() &&
<DesiredTargets<T>>::exists()) ||
(!<Snapshot<T>>::exists() &&
!<SnapshotMetadata<T>>::exists() &&
!<DesiredTargets<T>>::exists())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe can do something like this for improved readability

Suggested change
pub fn is_consistent() -> bool {
(<Snapshot<T>>::exists() &&
<SnapshotMetadata<T>>::exists() &&
<DesiredTargets<T>>::exists()) ||
(!<Snapshot<T>>::exists() &&
!<SnapshotMetadata<T>>::exists() &&
!<DesiredTargets<T>>::exists())
}
pub fn is_consistent() -> bool {
let snapshots = [
<Snapshot<T>>::exists(),
<SnapshotMetadata<T>>::exists(),
<DesiredTargets<T>>::exists(),
];
// All should either exist or not exist
snapshots.iter().all(|&exists| exists) || snapshots.iter().all(|&exists| !exists)
}

Copy link
Contributor

@kianenigma kianenigma Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapshots.iter().skip(1).all(|v| snapshots[0] == v) has the same effect.

}

impl<T: Config> Pallet<T> {
/// Internal logic of the offchain worker, to be executed only when the offchain lock is
/// acquired with success.
Expand Down Expand Up @@ -1405,9 +1436,6 @@ impl<T: Config> Pallet<T> {
SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32 };
log!(info, "creating a snapshot with metadata {:?}", metadata);

<SnapshotMetadata<T>>::put(metadata);
<DesiredTargets<T>>::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.
Expand All @@ -1422,7 +1450,7 @@ impl<T: Config> Pallet<T> {
// buffer should have not re-allocated since.
debug_assert!(buffer.len() == size && size == buffer.capacity());

sp_io::storage::set(&<Snapshot<T>>::hashed_key(), &buffer);
SnapshotWrapper::<T>::set(metadata, desired_targets, buffer);
}

/// Parts of [`create_snapshot`] that happen outside of this pallet.
Expand Down Expand Up @@ -1503,13 +1531,6 @@ impl<T: Config> Pallet<T> {
);
}

/// Kill everything created by [`Pallet::create_snapshot`].
pub fn kill_snapshot() {
<Snapshot<T>>::kill();
<SnapshotMetadata<T>>::kill();
<DesiredTargets<T>>::kill();
}

/// Checks the feasibility of a solution.
pub fn feasibility_check(
raw_solution: RawSolution<SolutionOf<T::MinerConfig>>,
Expand Down Expand Up @@ -1544,8 +1565,8 @@ impl<T: Config> Pallet<T> {
// Phase is off now.
Self::phase_transition(Phase::Off);

// Kill snapshots.
Self::kill_snapshot();
// Kill snapshots (everything created by [`Pallet::create_snapshot`]).
wirednkod marked this conversation as resolved.
Show resolved Hide resolved
SnapshotWrapper::<T>::kill();
}

fn do_elect() -> Result<BoundedSupportsOf<Self>, ElectionError<T>> {
Expand Down Expand Up @@ -1616,15 +1637,7 @@ impl<T: Config> Pallet<T> {
// - [`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 <Snapshot<T>>::exists() &&
<SnapshotMetadata<T>>::exists() &&
<DesiredTargets<T>>::exists()
{
Ok(())
} else if !<Snapshot<T>>::exists() &&
!<SnapshotMetadata<T>>::exists() &&
!<DesiredTargets<T>>::exists()
{
if SnapshotWrapper::<T>::is_consistent() {
Ok(())
} else {
Err("If snapshot exists, metadata and desired targets should be set too. Otherwise, none should be set.".into())
Expand Down Expand Up @@ -1759,18 +1772,14 @@ mod feasibility_check {
assert!(MultiPhase::current_phase().is_signed());
let solution = raw_solution();

// For whatever reason it might be:
<Snapshot<Runtime>>::kill();
// kill all `Snapshot, `SnapshotMetadata` and `DesiredTargets` for the storage state to
wirednkod marked this conversation as resolved.
Show resolved Hide resolved
// be consistent, by using the `SnapshotWrapper` for the try_state checks to pass.
<SnapshotWrapper<Runtime>>::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.
<SnapshotMetadata<Runtime>>::kill();
<DesiredTargets<Runtime>>::kill();
})
}

Expand Down
Loading