Add versioned migration to backfill commitment_nonce in pallet-storage-provider

pallet/src/lib.rs

@@ -22,6 +22,7 @@ extern crate alloc;
 pub use pallet::*;
 
 pub mod impls;
+pub mod migrations;
 pub mod runtime_api;
 pub mod weights;
 pub use weights::WeightInfo;
@@ -71,7 +72,12 @@ pub mod pallet {
         BlockNumberFor<T>,
     >;
 
+    /// In-code storage version. v1 backfills the `commitment_nonce` field added
+    /// to `BucketSnapshot` by #125; see [`crate::migrations::v1`].
+    const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
     #[pallet::pallet]
+    #[pallet::storage_version(STORAGE_VERSION)]
     pub struct Pallet<T>(_);
 
     #[pallet::hooks]

pallet/src/migrations.rs

@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: Apache-2.0
+
+//! Storage migrations for `pallet-storage-provider`.
+
+extern crate alloc;
+
+/// v0 -> v1: backfill the `commitment_nonce` field added to `BucketSnapshot` by
+/// the challenge flow overhaul (#125).
+///
+/// `BucketSnapshot` (nested in `Bucket::snapshot`) gained a trailing
+/// `commitment_nonce: u64` field, required by `extend_checkpoint` to verify a
+/// late-arriving signature against the payload the original signers signed.
+/// `Buckets` entries checkpointed before that change still encode the old
+/// (nonce-less) layout, so the current `Bucket`/`BucketSnapshot` decode treats
+/// them as undecodable — `try-runtime` reports the keys as undecodable. This
+/// re-stores every entry in the new layout, defaulting `commitment_nonce` to
+/// `0` (harmless: the nonce only matters to late-signature verification for a
+/// checkpoint still accepting signers, which no pre-existing snapshot has).
+pub mod v1 {
+    use crate::{Bucket, Buckets, Config, Member, Pallet};
+    use frame_support::{pallet_prelude::*, traits::UncheckedOnRuntimeUpgrade, weights::Weight};
+    use frame_system::pallet_prelude::BlockNumberFor;
+    use sp_core::H256;
+    use storage_primitives::{BucketSnapshot, Commitment};
+
+    /// The pre-migration `Bucket`/`BucketSnapshot`, used only to decode the old
+    /// `Buckets` values. Mirrors the pre-#125 layout, i.e. the current layout
+    /// minus the trailing `commitment_nonce` field.
+    mod old {
+        use super::*;
+        use alloc::vec::Vec;
+
+        #[derive(Decode)]
+        pub struct BucketSnapshot<BlockNumber> {
+            pub commitment: Commitment,
+            pub checkpoint_block: BlockNumber,
+            pub primary_signers: Vec<u8>,
+        }
+
+        #[derive(Decode)]
+        pub struct Bucket<T: Config> {
+            pub members: BoundedVec<Member<T>, T::MaxMembers>,
+            pub frozen_start_seq: Option<u64>,
+            pub min_providers: u32,
+            pub primary_providers: BoundedVec<T::AccountId, T::MaxPrimaryProviders>,
+            pub snapshot: Option<BucketSnapshot<BlockNumberFor<T>>>,
+            pub historical_roots: [(u32, H256); 6],
+            pub total_snapshots: u32,
+        }
+    }
+
+    pub struct InnerMigrateV0ToV1<T>(core::marker::PhantomData<T>);
+
+    impl<T: Config> UncheckedOnRuntimeUpgrade for InnerMigrateV0ToV1<T> {
+        fn on_runtime_upgrade() -> Weight {
+            let total = Buckets::<T>::iter_keys().count() as u64;
+            let mut translated = 0u64;
+            Buckets::<T>::translate::<old::Bucket<T>, _>(|_bucket_id, old| {
+                translated = translated.saturating_add(1);
+                Some(Bucket {
+                    members: old.members,
+                    frozen_start_seq: old.frozen_start_seq,
+                    min_providers: old.min_providers,
+                    primary_providers: old.primary_providers,
+                    snapshot: old.snapshot.map(|s| BucketSnapshot {
+                        commitment: s.commitment,
+                        checkpoint_block: s.checkpoint_block,
+                        primary_signers: s.primary_signers,
+                        commitment_nonce: 0,
+                    }),
+                    historical_roots: old.historical_roots,
+                    total_snapshots: old.total_snapshots,
+                })
+            });
+            // `translate` reads every key in the map (whether or not it
+            // decodes under the old layout) but only rewrites the ones that
+            // do, so `total` reads and `translated` writes is the true upper
+            // bound rather than under-counting reads on decode failures.
+            T::DbWeight::get().reads_writes(total, translated)
+        }
+
+        #[cfg(feature = "try-runtime")]
+        fn pre_upgrade() -> Result<alloc::vec::Vec<u8>, sp_runtime::TryRuntimeError> {
+            // Count existing keys before migration so post_upgrade can confirm
+            // none were dropped.
+            let count = Buckets::<T>::iter_keys().count() as u64;
+            Ok(count.encode())
+        }
+
+        #[cfg(feature = "try-runtime")]
+        fn post_upgrade(state: alloc::vec::Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
+            let before = u64::decode(&mut &state[..]).map_err(|_| "invalid pre_upgrade state")?;
+            // `iter_keys()` only enumerates keys; `iter()` fully decodes every
+            // value under the NEW layout. If any entry still failed to decode,
+            // `translate` would have left it as a raw, undecodable blob that
+            // `iter()` silently skips, so the two counts would diverge.
+            let after_keys = Buckets::<T>::iter_keys().count() as u64;
+            let after_values = Buckets::<T>::iter().count() as u64;
+            ensure!(
+                before == after_keys,
+                "Buckets entry count changed during migration"
+            );
+            ensure!(
+                after_keys == after_values,
+                "some Buckets entry failed to decode under the new layout"
+            );
+            Ok(())
+        }
+    }
+
+    /// Runs [`InnerMigrateV0ToV1`] only when the on-chain storage version is 0,
+    /// then bumps it to 1.
+    pub type MigrateV0ToV1<T> = frame_support::migrations::VersionedMigration<
+        0,
+        1,
+        InnerMigrateV0ToV1<T>,
+        Pallet<T>,
+        <T as frame_system::Config>::DbWeight,
+    >;
+}

pallet/src/tests/migrations.rs

@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: Apache-2.0
+
+//! Tests for the v0 -> v1 `commitment_nonce` backfill migration.
+
+use super::*;
+use crate::migrations::v1::InnerMigrateV0ToV1;
+use codec::Encode;
+use frame_support::{storage::unhashed, traits::UncheckedOnRuntimeUpgrade, BoundedVec};
+use sp_core::H256;
+use storage_primitives::{BucketId, Commitment};
+
+/// Mirrors the pre-#125 `BucketSnapshot` layout (no `commitment_nonce`), used
+/// only to encode a raw old-format value for this test.
+#[derive(Encode)]
+struct OldBucketSnapshot {
+    commitment: Commitment,
+    checkpoint_block: u64,
+    primary_signers: Vec<u8>,
+}
+
+/// Mirrors the pre-#125 `Bucket` layout, used only to encode a raw old-format
+/// value for this test.
+#[derive(Encode)]
+struct OldBucket {
+    members: BoundedVec<Member<Test>, <Test as Config>::MaxMembers>,
+    frozen_start_seq: Option<u64>,
+    min_providers: u32,
+    primary_providers: BoundedVec<u64, <Test as Config>::MaxPrimaryProviders>,
+    snapshot: Option<OldBucketSnapshot>,
+    historical_roots: [(u32, H256); 6],
+    total_snapshots: u32,
+}
+
+fn put_old_bucket(bucket_id: BucketId, old: OldBucket) {
+    unhashed::put_raw(&Buckets::<Test>::hashed_key_for(bucket_id), &old.encode());
+}
+
+#[test]
+fn migration_backfills_commitment_nonce_on_existing_snapshot() {
+    new_test_ext().execute_with(|| {
+        let bucket_id: BucketId = 1;
+        let member = Member {
+            account: 42u64,
+            role: Role::Admin,
+        };
+        put_old_bucket(
+            bucket_id,
+            OldBucket {
+                members: vec![member.clone()].try_into().unwrap(),
+                frozen_start_seq: None,
+                min_providers: 1,
+                primary_providers: vec![42u64].try_into().unwrap(),
+                snapshot: Some(OldBucketSnapshot {
+                    commitment: Commitment {
+                        mmr_root: H256::repeat_byte(0xAB),
+                        start_seq: 0,
+                        leaf_count: 2,
+                    },
+                    checkpoint_block: 100,
+                    primary_signers: vec![0b1],
+                }),
+                historical_roots: [(0, H256::default()); 6],
+                total_snapshots: 1,
+            },
+        );
+
+        InnerMigrateV0ToV1::<Test>::on_runtime_upgrade();
+
+        let migrated = Buckets::<Test>::get(bucket_id).expect("bucket must still decode");
+        assert_eq!(migrated.members.into_inner(), vec![member]);
+        assert_eq!(migrated.frozen_start_seq, None);
+        assert_eq!(migrated.min_providers, 1);
+        assert_eq!(migrated.primary_providers.into_inner(), vec![42u64]);
+        assert_eq!(migrated.total_snapshots, 1);
+        let snapshot = migrated.snapshot.expect("snapshot must survive migration");
+        assert_eq!(snapshot.checkpoint_block, 100);
+        assert_eq!(snapshot.primary_signers, vec![0b1]);
+        assert_eq!(snapshot.commitment.leaf_count, 2);
+        assert_eq!(snapshot.commitment_nonce, 0);
+    });
+}
+
+#[test]
+fn migration_preserves_bucket_with_no_snapshot() {
+    new_test_ext().execute_with(|| {
+        let bucket_id: BucketId = 2;
+        put_old_bucket(
+            bucket_id,
+            OldBucket {
+                members: BoundedVec::default(),
+                frozen_start_seq: Some(5),
+                min_providers: 0,
+                primary_providers: BoundedVec::default(),
+                snapshot: None,
+                historical_roots: [(0, H256::default()); 6],
+                total_snapshots: 0,
+            },
+        );
+
+        InnerMigrateV0ToV1::<Test>::on_runtime_upgrade();
+
+        let migrated = Buckets::<Test>::get(bucket_id).expect("bucket must still decode");
+        assert_eq!(migrated.frozen_start_seq, Some(5));
+        assert!(migrated.snapshot.is_none());
+    });
+}

pallet/src/tests/mod.rs

@@ -21,6 +21,7 @@ mod error_paths;
 mod extend_topup;
 mod genesis;
 mod member_buckets;
+mod migrations;
 mod misc;
 mod provider;
 mod replica;

runtimes/web3-storage-paseo/src/migrations.rs

@@ -29,4 +29,8 @@ pub type Migrations = (
     // Drop the `payment` field from `DriveInfo` (#105). A real data transform,
     // so it stays a `VersionedMigration` gated on the pallet's storage version.
     pallet_drive_registry::migrations::v1::MigrateV0ToV1<Runtime>,
+    // Backfill the `commitment_nonce` field added to `BucketSnapshot` by the
+    // challenge flow overhaul (#125). A real data transform, so it stays a
+    // `VersionedMigration` gated on the pallet's storage version.
+    pallet_storage_provider::migrations::v1::MigrateV0ToV1<Runtime>,
 );