pallet-migrations: fix index access for singluar migrations (#5695)

Discovered a bug in the migrations pallet while debugging
paritytech/try-runtime-cli#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

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: ggwpez <[email protected]>

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

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
 		};

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.

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>> {