From efc2132fa2ece419d36af03c935b3c2c60440eb5 Mon Sep 17 00:00:00 2001
From: Dastan <88332432+dastansam@users.noreply.github.com>
Date: Mon, 13 May 2024 00:35:53 +0200
Subject: [PATCH] migrations: `take()`should consume read and write operation
 weight (#4302)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

#### 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 <git@kchr.de>
---
 prdoc/pr_4302.prdoc                               | 15 +++++++++++++++
 substrate/frame/balances/src/migration.rs         |  2 +-
 substrate/frame/democracy/src/migrations/v1.rs    |  4 ++--
 .../single-block-migrations/src/migrations/v1.rs  |  8 ++++----
 substrate/frame/staking/src/migrations.rs         |  2 +-
 5 files changed, 23 insertions(+), 8 deletions(-)
 create mode 100644 prdoc/pr_4302.prdoc

diff --git a/prdoc/pr_4302.prdoc b/prdoc/pr_4302.prdoc
new file mode 100644
index 000000000000..bb4331f28023
--- /dev/null
+++ b/prdoc/pr_4302.prdoc
@@ -0,0 +1,15 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "migrations: take() should consume read and write operation weight"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      `take()` consumes only 1 read worth of weight in `single-block-migrations` example, while `take()` is `get() + kill()`, 
+      i.e should be 1 read + 1 write. Since this could mislead developers writing migrations following the example, 
+      this PR fixes the weight calculation.
+
+crates: 
+  - name: pallet-example-single-block-migrations
+    bump: minor
diff --git a/substrate/frame/balances/src/migration.rs b/substrate/frame/balances/src/migration.rs
index 38d9c07ff7e0..568c3fbb7cf0 100644
--- a/substrate/frame/balances/src/migration.rs
+++ b/substrate/frame/balances/src/migration.rs
@@ -91,7 +91,7 @@ impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for ResetInactive<T, I> {
 			StorageVersion::new(0).put::<Pallet<T, I>>();
 
 			log::info!(target: LOG_TARGET, "Storage to version 0");
-			T::DbWeight::get().reads_writes(1, 2)
+			T::DbWeight::get().reads_writes(1, 3)
 		} else {
 			log::info!(
 				target: LOG_TARGET,
diff --git a/substrate/frame/democracy/src/migrations/v1.rs b/substrate/frame/democracy/src/migrations/v1.rs
index 5e423b9ab6ef..47f8df017f1e 100644
--- a/substrate/frame/democracy/src/migrations/v1.rs
+++ b/substrate/frame/democracy/src/migrations/v1.rs
@@ -108,7 +108,7 @@ pub mod v1 {
 				.collect::<Vec<_>>();
 			let bounded = BoundedVec::<_, T::MaxProposals>::truncate_from(props.clone());
 			PublicProps::<T>::put(bounded);
-			weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1));
+			weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 2));
 
 			if props.len() as u32 > T::MaxProposals::get() {
 				log::error!(
@@ -126,7 +126,7 @@ pub mod v1 {
 
 			StorageVersion::new(1).put::<Pallet<T>>();
 
-			weight.saturating_add(T::DbWeight::get().reads_writes(1, 2))
+			weight.saturating_add(T::DbWeight::get().reads_writes(1, 3))
 		}
 
 		#[cfg(feature = "try-runtime")]
diff --git a/substrate/frame/examples/single-block-migrations/src/migrations/v1.rs b/substrate/frame/examples/single-block-migrations/src/migrations/v1.rs
index 18ef4e72cc4f..7b543d72c984 100644
--- a/substrate/frame/examples/single-block-migrations/src/migrations/v1.rs
+++ b/substrate/frame/examples/single-block-migrations/src/migrations/v1.rs
@@ -67,10 +67,10 @@ impl<T: crate::Config> UncheckedOnRuntimeUpgrade for InnerMigrateV0ToV1<T> {
 			// Write the new value to storage
 			let new = crate::CurrentAndPreviousValue { current: old_value, previous: None };
 			crate::Value::<T>::put(new);
-			// One read for the old value, one write for the new value
-			T::DbWeight::get().reads_writes(1, 1)
+			// One read + write for taking the old value, and one write for setting the new value
+			T::DbWeight::get().reads_writes(1, 2)
 		} else {
-			// One read for trying to access the old value
+			// No writes since there was no old value, just one read for checking
 			T::DbWeight::get().reads(1)
 		}
 	}
@@ -184,7 +184,7 @@ mod test {
 			// value.
 			assert_eq!(
 				weight,
-				<MockRuntime as frame_system::Config>::DbWeight::get().reads_writes(1, 1)
+				<MockRuntime as frame_system::Config>::DbWeight::get().reads_writes(1, 2)
 			);
 
 			// After the migration, the new value should be set as the `current` value.
diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs
index 510252be26c9..b2ddf77004f9 100644
--- a/substrate/frame/staking/src/migrations.rs
+++ b/substrate/frame/staking/src/migrations.rs
@@ -370,7 +370,7 @@ pub mod v10 {
 				StorageVersion::<T>::put(ObsoleteReleases::V10_0_0);
 
 				log!(info, "MigrateToV10 executed successfully");
-				T::DbWeight::get().reads_writes(1, 1)
+				T::DbWeight::get().reads_writes(1, 2)
 			} else {
 				log!(warn, "MigrateToV10 should be removed.");
 				T::DbWeight::get().reads(1)