From e484c01500323432cad4532fa76ea903300c1308 Mon Sep 17 00:00:00 2001
From: "paritytech-cmd-bot-polkadot-sdk[bot]"
 <179002856+paritytech-cmd-bot-polkadot-sdk[bot]@users.noreply.github.com>
Date: Tue, 17 Sep 2024 12:12:49 +0200
Subject: [PATCH] [stable2407] Backport #5695 (#5729)

Backport #5695 into `stable2407` from ggwpez.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
---
 prdoc/pr_5695.prdoc                       | 15 ++++++++++++++
 substrate/frame/migrations/src/lib.rs     |  7 ++++---
 substrate/frame/migrations/src/tests.rs   | 25 +++++++++++++++++++++++
 substrate/frame/support/src/migrations.rs |  9 +++++---
 4 files changed, 50 insertions(+), 6 deletions(-)
 create mode 100644 prdoc/pr_5695.prdoc

diff --git a/prdoc/pr_5695.prdoc b/prdoc/pr_5695.prdoc
new file mode 100644
index 000000000000..202eafa26ebd
--- /dev/null
+++ b/prdoc/pr_5695.prdoc
@@ -0,0 +1,15 @@
+title: 'pallet-migrations: fix index access for singluar migrations'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Discovered a bug in the migrations pallet while debugging https://github.com/paritytech/try-runtime-cli/pull/90.
+    It only occurs when a single MBM is configured - hence it did not happen when Ajuna Network tried it...
+
+    Changes:
+    - Check len of the tuple before accessing its nth_id
+    - Make nth_id return `None` on unary tuples and n>0
+crates:
+- name: pallet-migrations
+  bump: patch
+- name: frame-support
+  bump: patch
diff --git a/substrate/frame/migrations/src/lib.rs b/substrate/frame/migrations/src/lib.rs
index 68041a57eaa2..1823e5a2f952 100644
--- a/substrate/frame/migrations/src/lib.rs
+++ b/substrate/frame/migrations/src/lib.rs
@@ -678,7 +678,7 @@ impl<T: Config> Pallet<T> {
 			return Some(ControlFlow::Break(cursor))
 		}
 
-		let Some(id) = T::Migrations::nth_id(cursor.index) else {
+		if cursor.index >= T::Migrations::len() {
 			// No more migrations in the tuple - we are done.
 			defensive_assert!(cursor.index == T::Migrations::len(), "Inconsistent MBMs tuple");
 			Self::deposit_event(Event::UpgradeCompleted);
@@ -687,8 +687,9 @@ impl<T: Config> Pallet<T> {
 			return None;
 		};
 
-		let Ok(bounded_id): Result<IdentifierOf<T>, _> = id.try_into() else {
-			defensive!("integrity_test ensures that all identifiers' MEL bounds fit into CursorMaxLen; qed.");
+		let id = T::Migrations::nth_id(cursor.index).map(TryInto::try_into);
+		let Some(Ok(bounded_id)): Option<Result<IdentifierOf<T>, _>> = id else {
+			defensive!("integrity_test ensures that all identifiers are present and bounde; qed.");
 			Self::upgrade_failed(Some(cursor.index));
 			return None
 		};
diff --git a/substrate/frame/migrations/src/tests.rs b/substrate/frame/migrations/src/tests.rs
index 9c9043d37a62..73ca2a9a09cf 100644
--- a/substrate/frame/migrations/src/tests.rs
+++ b/substrate/frame/migrations/src/tests.rs
@@ -27,6 +27,31 @@ use frame_support::{pallet_prelude::Weight, traits::OnRuntimeUpgrade};
 #[docify::export]
 #[test]
 fn simple_works() {
+	use Event::*;
+	test_closure(|| {
+		// Add three migrations, each taking one block longer than the previous.
+		MockedMigrations::set(vec![(SucceedAfter, 2)]);
+
+		System::set_block_number(1);
+		Migrations::on_runtime_upgrade();
+		run_to_block(10);
+
+		// Check that the executed migrations are recorded in `Historical`.
+		assert_eq!(historic(), vec![mocked_id(SucceedAfter, 2),]);
+
+		// Check that we got all events.
+		assert_events(vec![
+			UpgradeStarted { migrations: 1 },
+			MigrationAdvanced { index: 0, took: 1 },
+			MigrationAdvanced { index: 0, took: 2 },
+			MigrationCompleted { index: 0, took: 3 },
+			UpgradeCompleted,
+		]);
+	});
+}
+
+#[test]
+fn simple_multiple_works() {
 	use Event::*;
 	test_closure(|| {
 		// Add three migrations, each taking one block longer than the previous.
diff --git a/substrate/frame/support/src/migrations.rs b/substrate/frame/support/src/migrations.rs
index 7f7461469564..0eabf9d0ee16 100644
--- a/substrate/frame/support/src/migrations.rs
+++ b/substrate/frame/support/src/migrations.rs
@@ -673,7 +673,8 @@ pub trait SteppedMigrations {
 
 	/// The `n`th [`SteppedMigration::id`].
 	///
-	/// Is guaranteed to return `Some` if `n < Self::len()`.
+	/// Is guaranteed to return `Some` if `n < Self::len()`. Calling this with any index larger or
+	/// equal to `Self::len()` MUST return `None`.
 	fn nth_id(n: u32) -> Option<Vec<u8>>;
 
 	/// The [`SteppedMigration::max_steps`] of the `n`th migration.
@@ -777,8 +778,10 @@ impl<T: SteppedMigration> SteppedMigrations for T {
 		1
 	}
 
-	fn nth_id(_n: u32) -> Option<Vec<u8>> {
-		Some(T::id().encode())
+	fn nth_id(n: u32) -> Option<Vec<u8>> {
+		n.is_zero()
+			.then_some(T::id().encode())
+			.defensive_proof("nth_id should only be called with n==0")
 	}
 
 	fn nth_max_steps(n: u32) -> Option<Option<u32>> {