Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
98 commits
Select commit Hold shift + click to select a range
dd4607b
Fix elections-phragmen and proxy issue
kianenigma Sep 7, 2020
6bd6221
remove TODO
kianenigma Sep 7, 2020
57de14e
Update bond to be per-vote
kianenigma Sep 8, 2020
684802e
Update frame/elections-phragmen/src/lib.rs
kianenigma Sep 9, 2020
70852fa
Fix benchmakrs
kianenigma Sep 9, 2020
4b7d0a6
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Sep 9, 2020
77caa5c
Fix weight as well.
kianenigma Sep 9, 2020
06a0aa0
Add license
kianenigma Sep 9, 2020
9943042
Make weight interpreted wasm! 🤦🏻‍♂️
kianenigma Sep 9, 2020
f9bd4a3
Merge branch 'master' into kiz-fix-proxy-election-drain
gavofyork Sep 14, 2020
f9ea73a
Remove a bunch of TODOs
kianenigma Sep 14, 2020
24c416b
Add migration
kianenigma Sep 14, 2020
10e3687
Better storage version.
kianenigma Sep 14, 2020
4495ead
Functionify.
kianenigma Sep 14, 2020
3c1a2ef
Fix deposit scheme.
kianenigma Sep 17, 2020
1b95224
remove legacy bond.
kianenigma Sep 17, 2020
2c677c2
Master.into()
kianenigma Sep 17, 2020
d2ec181
Master.into()
kianenigma Sep 17, 2020
a628850
better logging.
kianenigma Sep 17, 2020
f6189d5
Fix benchmarking test
kianenigma Sep 17, 2020
e812815
Fix confused deposit collection.
kianenigma Sep 18, 2020
498376c
Add fine
kianenigma Sep 21, 2020
d9b2204
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Sep 21, 2020
4691703
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Sep 21, 2020
88db61d
Master.into()
kianenigma Sep 23, 2020
b4e2a7c
Master.into()
kianenigma Oct 12, 2020
c871c81
Better name for storage item
kianenigma Oct 12, 2020
12c18d5
Fix name again.
kianenigma Oct 12, 2020
f900b68
remove unused
kianenigma Oct 12, 2020
6c942b4
Update frame/elections-phragmen/src/lib.rs
kianenigma Oct 13, 2020
8e3c2eb
Update frame/elections-phragmen/src/lib.rs
kianenigma Oct 13, 2020
e674a60
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Oct 13, 2020
a99bebc
Merge remote-tracking branch 'origin/master' into kiz-fix-proxy-elect…
shawntabrizi Oct 13, 2020
f5700cf
cargo run --release --features runtime-benchmarks --manifest-path bin…
shawntabrizi Oct 13, 2020
0564e59
new weight fns
kianenigma Oct 13, 2020
60d7d38
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Oct 13, 2020
1b48d0d
Fix build
kianenigma Oct 13, 2020
960a474
Fix line width
kianenigma Oct 13, 2020
19e9436
fix benchmakrs
kianenigma Oct 13, 2020
76cb36f
fix warning
kianenigma Oct 13, 2020
820d76f
cargo run --release --features runtime-benchmarks --manifest-path bin…
Oct 13, 2020
4caea9f
Tune the stake again
kianenigma Oct 13, 2020
0c1043f
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Oct 13, 2020
df267fa
cargo run --release --features runtime-benchmarks --manifest-path bin…
Oct 13, 2020
1621131
Merge branch 'master' of github.com:paritytech/substrate into kiz-fix…
kianenigma Oct 22, 2020
c79b522
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Oct 22, 2020
50fa257
Fix builds
kianenigma Nov 17, 2020
8bd074f
All tests work again.
kianenigma Nov 18, 2020
29880e4
A large number of fixes.
kianenigma Nov 18, 2020
d78f6bb
Fix event as well.
kianenigma Nov 18, 2020
5d09983
more fixes.
kianenigma Nov 19, 2020
6668ac1
Fix node build
kianenigma Nov 19, 2020
3dd01d8
Some fixes to benchmarks
kianenigma Nov 19, 2020
694e42a
Merge remote-tracking branch 'origin/master' into kiz-fix-proxy-elect…
Nov 19, 2020
cf94f49
Fix some warnings.
kianenigma Nov 19, 2020
ea69a54
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Nov 20, 2020
b2f36bd
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Nov 20, 2020
1fd2b08
Update frame/elections-phragmen/src/lib.rs
kianenigma Nov 25, 2020
585e599
a batch of review comments.
kianenigma Nov 25, 2020
4b9b207
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Nov 25, 2020
ea17535
Master.into()
kianenigma Nov 25, 2020
4b34e72
Fix a test.
kianenigma Nov 25, 2020
ecb0e4e
Fix some more tests.
kianenigma Nov 25, 2020
e810cc7
do migration with pallet version???
kianenigma Nov 25, 2020
e650c02
Master.into()
kianenigma Nov 25, 2020
8742865
Final touches.
kianenigma Nov 26, 2020
d6de615
Remove unused storage.
kianenigma Nov 27, 2020
6cae3da
another rounds of changes and fixes.
kianenigma Nov 30, 2020
3c0d341
Update frame/elections-phragmen/src/lib.rs
kianenigma Nov 30, 2020
3ca1590
Update frame/elections-phragmen/src/lib.rs
kianenigma Nov 30, 2020
9c9819b
Review grumbles.
kianenigma Nov 30, 2020
68ff142
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Nov 30, 2020
797d04c
Fix ocnflics
kianenigma Dec 2, 2020
73a3537
Fix a bit more.
kianenigma Dec 2, 2020
98f349e
Fix build
kianenigma Dec 2, 2020
9454a68
Experimental: independent migration.
kianenigma Dec 2, 2020
c750f74
WIP: isolated migration logics
gui1117 Dec 2, 2020
e11a216
clean up.
kianenigma Dec 2, 2020
1e2c8e9
make migration struct private and move migration to own file
gui1117 Dec 3, 2020
583aa82
add doc
gui1117 Dec 3, 2020
dca4bd5
Merge remote-tracking branch 'origin/master' into kiz-fix-proxy-elect…
gui1117 Dec 3, 2020
c90d25a
Merge remote-tracking branch 'origin/master' into kiz-fix-proxy-elect…
gui1117 Dec 3, 2020
586c61c
fix StorageInstance new syntax
gui1117 Dec 3, 2020
0a0ef71
Update frame/elections-phragmen/src/migrations_3_0_0.rs
gui1117 Dec 4, 2020
a205401
Master.into()
kianenigma Dec 17, 2020
63a4c56
another round of self-review.
kianenigma Dec 17, 2020
ec067c9
bit better formatting
kianenigma Dec 17, 2020
5dab8bc
Merge remote-tracking branch 'origin/master' into kiz-fix-proxy-elect…
Dec 17, 2020
32cb6c1
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Dec 17, 2020
38881e0
Fix tests.
kianenigma Dec 21, 2020
e4d7464
Master.into()
kianenigma Dec 21, 2020
79d1d5b
Master.into()
kianenigma Jan 19, 2021
7d95b15
Round of self-review
kianenigma Jan 20, 2021
ba12aec
Clean migrations
kianenigma Jan 20, 2021
673aed7
Merge remote-tracking branch 'origin/master' into kiz-fix-proxy-elect…
Jan 20, 2021
99f5fd2
cargo run --release --features=runtime-benchmarks --manifest-path=bin…
Jan 20, 2021
92dbabe
Revert unwanted change to construct-runtime
kianenigma Jan 20, 2021
c688a6f
Merge branch 'kiz-fix-proxy-election-drain' of github.com:paritytech/…
kianenigma Jan 20, 2021
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
Prev Previous commit
Next Next commit
do migration with pallet version???
  • Loading branch information
kianenigma committed Nov 25, 2020
commit e810cc72156b462585fc197174d6ca3d279643cd
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion bin/node/runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pallet-contracts = { version = "2.0.0", default-features = false, path = "../../
pallet-contracts-primitives = { version = "2.0.0", default-features = false, path = "../../../frame/contracts/common/" }
pallet-contracts-rpc-runtime-api = { version = "0.8.0", default-features = false, path = "../../../frame/contracts/rpc/runtime-api/" }
pallet-democracy = { version = "2.0.0", default-features = false, path = "../../../frame/democracy" }
pallet-elections-phragmen = { version = "2.0.0", default-features = false, path = "../../../frame/elections-phragmen" }
pallet-elections-phragmen = { version = "3.0.0", default-features = false, path = "../../../frame/elections-phragmen" }
pallet-grandpa = { version = "2.0.0", default-features = false, path = "../../../frame/grandpa" }
pallet-im-online = { version = "2.0.0", default-features = false, path = "../../../frame/im-online" }
pallet-indices = { version = "2.0.0", default-features = false, path = "../../../frame/indices" }
Expand Down
24 changes: 24 additions & 0 deletions frame/elections-phragmen/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Changelog
All notable changes to this crate will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Added
[Add slashing events to elections-phragmen](https://github.com/paritytech/substrate/pull/7543)

### Changed

### Fixed
[Don't slash all outgoing members](https://github.com/paritytech/substrate/pull/7394)
[Fix wrong outgoing calculation in election](https://github.com/paritytech/substrate/pull/7384)

### Security
\[**Needs Migration**\] [Fix elections-phragmen and proxy issue + Record deposits on-chain](https://github.com/paritytech/substrate/pull/7040)

## [2.0.0] - 2020-09-2020

Initial version from which version tracking has begun.

2 changes: 1 addition & 1 deletion frame/elections-phragmen/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "pallet-elections-phragmen"
version = "2.0.0"
version = "3.0.0"
authors = ["Parity Technologies <[email protected]>"]
edition = "2018"
license = "Apache-2.0"
Expand Down
151 changes: 72 additions & 79 deletions frame/elections-phragmen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,95 +127,88 @@ type NegativeImbalanceOf<T> =
/// Helper functions for migrations of this module.
pub mod migrations {
use super::*;
use frame_support::{migration::StorageKeyIterator, Twox64Concat};

/// Migrate from the old legacy voting bond (fixed) to the new one (per-vote dynamic).
///
/// Will only be triggered if storage version is V1.
pub fn migrate_voters_to_recorded_deposit<T: Trait>(old_deposit: BalanceOf<T>) -> Weight {
if <Module<T>>::pallet_storage_version() == StorageVersion::V1 {
let mut count = 0;
<StorageKeyIterator<T::AccountId, (BalanceOf<T>, Vec<T::AccountId>), Twox64Concat>>::new(
<Voting<T>>::module_prefix(),
b"Voting",
)
.for_each(|(who, (stake, votes))| {
// Insert a new value into the same location, thus no need to do `.drain()`.
let deposit = old_deposit;
let voter = Voter { votes, stake, deposit };
<Voting<T>>::insert(who, voter);
count += 1;
});
use frame_support::{
traits::{PalletVersion, GetPalletVersion},
Twox64Concat,
migration::StorageKeyIterator,
};

PalletStorageVersion::put(StorageVersion::V2RecordedDeposit);
frame_support::debug::info!(
"🏛 pallet-elections-phragmen: {} voters migrated to V2PerVoterDeposit.",
count,
);
Weight::max_value()
pub fn migrate_to_3_0_0<T: Trait>(old_voter_bond: BalanceOf<T>, old_candidacy_bond: BalanceOf<T>) -> Weight {
let current_version = <Module<T> as GetPalletVersion>::current_version();
let maybe_storage_version = <Module<T> as GetPalletVersion>::storage_version();

if current_version == PalletVersion::new(3, 0, 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will disallow running this migration for future versions meaning that people that don't upgrade before the next change to the phragmen pallet will not be able to run this.
I think you should only match on the storage version here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm good point, but I wonder if that can create any sort of footgun in an edge case where we accidentally execute a migration twice?

Copy link
Contributor Author

@kianenigma kianenigma Nov 26, 2020

Choose a reason for hiding this comment

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

What I am worried about for example is if someone is trying to do multiple upgrades at once. In my restricted scheme, they cannot shoot themselves in the foot. In yours, I think they can?

Copy link
Contributor

Choose a reason for hiding this comment

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

For that we could set the version that you're intending to migrate to after the migration was successful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we set the version? The version will always be set to what is noted in Cargo.toml after a runtime upgrade afaik.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that the main dilemma here is:

  1. Should this migration be executed as a fn on_runtime_upgrade() inside the pallet?
  2. Should this migration be executed as an additional upgrade directly given to executive?

I assumed that in all cases the automatic write happens, while it is not the case. Automatic write happens only in case 1.

Based on the assumption that the upgrade will be called from executive I can remove the additional if and fo a manual write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly though, I think this migration should live inside the fn on_runtime_upgrade() of the pallet, and have all the needed if-conditions to make sure it is executed only when it is a jump from 2_0_0 to 3_0_0. Given this, the code can always live in substrate and allows chains that are multiple versions behind to also update them one by one.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes ideally it is better for it to live inside the on_runtime_upgrade, but how can we give the information about old deposits ?
Maybe as associated type on Config ? then this is bloating Config :-/ so I don't know what is best.

OnRuntimeUpgrade executes first CustomOnRuntimeUpgrade define at runtime level then pallets OnRuntimeUpgrade which writes into the storage the pallet version after their execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe as associated type on Config ? then this is bloating Config :-/ so I don't know what is best.

Even worse about this is that now we bump to 3_0_0 because the change is breaking, and if we add new configs just for migration, we only can clean them if we bump to 4_0_0 because the removal itself will be a breaking change (I am doing my best here to adhere to semver, which is kinda hard to do in pallets.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the main dilemma here is:

1. Should this migration be executed as a `fn on_runtime_upgrade()` inside the pallet?

2. Should this migration be executed as an additional upgrade directly given to executive?

I assumed that in all cases the automatic write happens, while it is not the case. Automatic write happens only in case 1.

I don't think that's right. I thought this code means that the version is set for all pallets.

match maybe_storage_version {
Some(storage_version) if storage_version == PalletVersion::new(2, 0, 0) => {
migrate_voters_to_recorded_deposit::<T>(old_voter_bond);
migrate_candidates_to_recorded_deposit::<T>(old_candidacy_bond);
migrate_runners_up_to_recorded_deposit::<T>(old_candidacy_bond);
migrate_members_to_recorded_deposit::<T>(old_candidacy_bond);
Weight::max_value()
},
_ => {
0
}
}
} else {
frame_support::debug::warn!(
"🏛 pallet-elections-phragmen: Tried to run migration but PalletStorageVersion is \
updated to V2. This code probably needs to be removed now.",
);
0
}
}

/// Migrate from the old legacy voting bond (fixed) to the new one (per-vote dynamic).
///
/// Will only be triggered if storage version is V1.
pub fn migrate_voters_to_recorded_deposit<T: Trait>(old_deposit: BalanceOf<T>) {
let mut count = 0;
<StorageKeyIterator<T::AccountId, (BalanceOf<T>, Vec<T::AccountId>), Twox64Concat>>::new(
<Voting<T>>::module_prefix(),
b"Voting",
)
.for_each(|(who, (stake, votes))| {
// Insert a new value into the same location, thus no need to do `.drain()`.
let deposit = old_deposit;
let voter = Voter { votes, stake, deposit };
<Voting<T>>::insert(who, voter);
count += 1;
});
}

/// Migrate all candidates to recorded deposit.
///
/// Will only be triggered if storage version is V1.
pub fn migrate_candidates_to_recorded_deposit<T: Trait>(old_deposit: BalanceOf<T>) -> Weight {
if <Module<T>>::pallet_storage_version() == StorageVersion::V1 {
let old_candidates = frame_support::migration::take_storage_value::<Vec<T::AccountId>>(
<Voting<T>>::module_prefix(),
b"Candidates",
&[],
)
.unwrap_or_default();
let new_candidates = old_candidates
.into_iter()
.map(|c| (c, old_deposit))
.collect::<Vec<_>>();
<Candidates<T>>::put(new_candidates);
Weight::max_value()
} else {
frame_support::debug::warn!(
"🏛 pallet-elections-phragmen: Tried to run migration but PalletStorageVersion is \
updated. This code probably needs to be removed now.",
);
0
}
pub fn migrate_candidates_to_recorded_deposit<T: Trait>(old_deposit: BalanceOf<T>) {
let old_candidates = frame_support::migration::take_storage_value::<Vec<T::AccountId>>(
<Voting<T>>::module_prefix(),
b"Candidates",
&[],
)
.unwrap_or_default();
let new_candidates = old_candidates
.into_iter()
.map(|c| (c, old_deposit))
.collect::<Vec<_>>();
<Candidates<T>>::put(new_candidates);
}

pub fn migrate_members_to_recorded_deposit<T: Trait>(deposit: BalanceOf<T>) -> Weight {
if <Module<T>>::pallet_storage_version() == StorageVersion::V1 {
let _ = <Members<T>>::translate::<Vec<(T::AccountId, BalanceOf<T>)>, _>(
|maybe_old_members| {
maybe_old_members.map(|old_members| {
frame_support::debug::info!(
"migrated {} member accounts.",
old_members.len()
);
old_members
.into_iter()
.map(|(who, stake)| SeatHolder {
who,
stake,
deposit,
})
.collect::<Vec<_>>()
})
},
);
Weight::max_value()
} else {
frame_support::debug::warn!(
"🏛 pallet-elections-phragmen: Tried to run migration but PalletStorageVersion is \
updated. This code probably needs to be removed now.",
);
0
}
pub fn migrate_members_to_recorded_deposit<T: Trait>(deposit: BalanceOf<T>) {
let _ = <Members<T>>::translate::<Vec<(T::AccountId, BalanceOf<T>)>, _>(
|maybe_old_members| {
maybe_old_members.map(|old_members| {
frame_support::debug::info!(
"migrated {} member accounts.",
old_members.len()
);
old_members
.into_iter()
.map(|(who, stake)| SeatHolder {
who,
stake,
deposit,
})
.collect::<Vec<_>>()
})
},
);
}

pub fn migrate_runners_up_to_recorded_deposit<T: Trait>(deposit: BalanceOf<T>) -> Weight {
Expand Down Expand Up @@ -2705,7 +2698,7 @@ mod tests {
assert_err_with_weight!(
Elections::remove_member(Origin::root(), 4, false),
Error::<Test>::InvalidReplacement,
Some(33489000) // only thing that matters for now is that it is NOT the full block.
Some(34042000) // only thing that matters for now is that it is NOT the full block.
);
});
}
Expand Down