Skip to content

Commit efc2132

Browse files
dastansambkchr
andauthored
migrations: take()should consume read and write operation weight (#4302)
#### Problem `take()` consumes only 1 read worth of weight in `single-block-migrations` example, while `take()` [is](https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/storage/unhashed.rs#L63) `get() + kill()`, i.e should be 1 read + 1 write. I think this could mislead developers who follow this example to write their migrations --------- Co-authored-by: Bastian Köcher <[email protected]>
1 parent 5f31981 commit efc2132

File tree

5 files changed

+23
-8
lines changed

5 files changed

+23
-8
lines changed

prdoc/pr_4302.prdoc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
2+
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
3+
4+
title: "migrations: take() should consume read and write operation weight"
5+
6+
doc:
7+
- audience: Runtime Dev
8+
description: |
9+
`take()` consumes only 1 read worth of weight in `single-block-migrations` example, while `take()` is `get() + kill()`,
10+
i.e should be 1 read + 1 write. Since this could mislead developers writing migrations following the example,
11+
this PR fixes the weight calculation.
12+
13+
crates:
14+
- name: pallet-example-single-block-migrations
15+
bump: minor

substrate/frame/balances/src/migration.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for ResetInactive<T, I> {
9191
StorageVersion::new(0).put::<Pallet<T, I>>();
9292

9393
log::info!(target: LOG_TARGET, "Storage to version 0");
94-
T::DbWeight::get().reads_writes(1, 2)
94+
T::DbWeight::get().reads_writes(1, 3)
9595
} else {
9696
log::info!(
9797
target: LOG_TARGET,

substrate/frame/democracy/src/migrations/v1.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ pub mod v1 {
108108
.collect::<Vec<_>>();
109109
let bounded = BoundedVec::<_, T::MaxProposals>::truncate_from(props.clone());
110110
PublicProps::<T>::put(bounded);
111-
weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1));
111+
weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2));
112112

113113
if props.len() as u32 > T::MaxProposals::get() {
114114
log::error!(
@@ -126,7 +126,7 @@ pub mod v1 {
126126

127127
StorageVersion::new(1).put::<Pallet<T>>();
128128

129-
weight.saturating_add(T::DbWeight::get().reads_writes(1, 2))
129+
weight.saturating_add(T::DbWeight::get().reads_writes(1, 3))
130130
}
131131

132132
#[cfg(feature = "try-runtime")]

substrate/frame/examples/single-block-migrations/src/migrations/v1.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,10 @@ impl<T: crate::Config> UncheckedOnRuntimeUpgrade for InnerMigrateV0ToV1<T> {
6767
// Write the new value to storage
6868
let new = crate::CurrentAndPreviousValue { current: old_value, previous: None };
6969
crate::Value::<T>::put(new);
70-
// One read for the old value, one write for the new value
71-
T::DbWeight::get().reads_writes(1, 1)
70+
// One read + write for taking the old value, and one write for setting the new value
71+
T::DbWeight::get().reads_writes(1, 2)
7272
} else {
73-
// One read for trying to access the old value
73+
// No writes since there was no old value, just one read for checking
7474
T::DbWeight::get().reads(1)
7575
}
7676
}
@@ -184,7 +184,7 @@ mod test {
184184
// value.
185185
assert_eq!(
186186
weight,
187-
<MockRuntime as frame_system::Config>::DbWeight::get().reads_writes(1, 1)
187+
<MockRuntime as frame_system::Config>::DbWeight::get().reads_writes(1, 2)
188188
);
189189

190190
// After the migration, the new value should be set as the `current` value.

substrate/frame/staking/src/migrations.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ pub mod v10 {
370370
StorageVersion::<T>::put(ObsoleteReleases::V10_0_0);
371371

372372
log!(info, "MigrateToV10 executed successfully");
373-
T::DbWeight::get().reads_writes(1, 1)
373+
T::DbWeight::get().reads_writes(1, 2)
374374
} else {
375375
log!(warn, "MigrateToV10 should be removed.");
376376
T::DbWeight::get().reads(1)

0 commit comments

Comments
 (0)