Crowdloan max contributors limit to avoid unbounded work

pallets/crowdloan/Cargo.toml

@@ -21,6 +21,7 @@ frame-support.workspace = true
 frame-system.workspace = true
 sp-runtime.workspace = true
 sp-std.workspace = true
+log = { workspace = true }
 
 [dev-dependencies]
 pallet-balances = { default-features = true, workspace = true }
@@ -39,6 +40,7 @@ std = [
   "sp-runtime/std",
   "sp-std/std",
   "sp-io/std",
+  "log/std",
   "sp-core/std",
   "pallet-balances/std",
   "pallet-preimage/std",

pallets/crowdloan/src/benchmarking.rs

@@ -68,6 +68,7 @@ mod benchmarks {
                 target_address: Some(target_address.clone()),
                 call: Some(T::Preimages::bound(*call).unwrap()),
                 finalized: false,
+                contributors_count: 1,
             })
         );
         // ensure the creator has been deducted the deposit

pallets/crowdloan/src/lib.rs

@@ -7,7 +7,7 @@
 
 extern crate alloc;
 
-use alloc::{boxed::Box, vec, vec::Vec};
+use alloc::{boxed::Box, vec};
 use codec::{Decode, Encode};
 use frame_support::{
     PalletId,
@@ -25,6 +25,7 @@ use frame_support::{
 use frame_system::pallet_prelude::*;
 use scale_info::TypeInfo;
 use sp_runtime::traits::CheckedSub;
+use sp_std::vec::Vec;
 use weights::WeightInfo;
 
 pub use pallet::*;
@@ -33,6 +34,7 @@ use subtensor_macros::freeze_struct;
 pub type CrowdloanId = u32;
 
 mod benchmarking;
+mod migrations;
 mod mock;
 mod tests;
 pub mod weights;
@@ -42,11 +44,14 @@ pub type CurrencyOf<T> = <T as Config>::Currency;
 pub type BalanceOf<T> =
     <CurrencyOf<T> as fungible::Inspect<<T as frame_system::Config>::AccountId>>::Balance;
 
+// Define a maximum length for the migration key
+type MigrationKeyMaxLen = ConstU32<128>;
+
 pub type BoundedCallOf<T> =
     Bounded<<T as Config>::RuntimeCall, <T as frame_system::Config>::Hashing>;
 
 /// A struct containing the information about a crowdloan.
-#[freeze_struct("6b86ccf70fc1b8f1")]
+#[freeze_struct("5db9538284491545")]
 #[derive(Encode, Decode, Eq, PartialEq, Ord, PartialOrd, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 pub struct CrowdloanInfo<AccountId, Balance, BlockNumber, Call> {
     /// The creator of the crowdloan.
@@ -71,6 +76,8 @@ pub struct CrowdloanInfo<AccountId, Balance, BlockNumber, Call> {
     pub call: Option<Call>,
     /// Whether the crowdloan has been finalized.
     pub finalized: bool,
+    /// The number of contributors to the crowdloan.
+    pub contributors_count: u32,
 }
 
 pub type CrowdloanInfoOf<T> = CrowdloanInfo<
@@ -134,6 +141,10 @@ pub mod pallet {
         /// The maximum number of contributors that can be refunded in a single refund.
         #[pallet::constant]
         type RefundContributorsLimit: Get<u32>;
+
+        // The maximum number of contributors that can contribute to a crowdloan.
+        #[pallet::constant]
+        type MaxContributors: Get<u32>;
     }
 
     /// A map of crowdloan ids to their information.
@@ -162,6 +173,11 @@ pub mod pallet {
     #[pallet::storage]
     pub type CurrentCrowdloanId<T: Config> = StorageValue<_, CrowdloanId, OptionQuery>;
 
+    /// Storage for the migration run status.
+    #[pallet::storage]
+    pub type HasMigrationRun<T: Config> =
+        StorageMap<_, Identity, BoundedVec<u8, MigrationKeyMaxLen>, bool, ValueQuery>;
+
     #[pallet::event]
     #[pallet::generate_deposit(pub(super) fn deposit_event)]
     pub enum Event<T: Config> {
@@ -253,6 +269,21 @@ pub mod pallet {
         NotReadyToDissolve,
         /// The deposit cannot be withdrawn from the crowdloan.
         DepositCannotBeWithdrawn,
+        /// The maximum number of contributors has been reached.
+        MaxContributorsReached,
+    }
+
+    #[pallet::hooks]
+    impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+        fn on_runtime_upgrade() -> frame_support::weights::Weight {
+            let mut weight = frame_support::weights::Weight::from_parts(0, 0);
+
+            weight = weight
+                // Add the contributors count for each crowdloan
+                .saturating_add(migrations::migrate_add_contributors_count::<T>());
+
+            weight
+        }
     }
 
     #[pallet::call]
@@ -342,6 +373,7 @@ pub mod pallet {
                 target_address,
                 call,
                 finalized: false,
+                contributors_count: 1,
             };
             Crowdloans::<T>::insert(crowdloan_id, &crowdloan);
 
@@ -398,6 +430,12 @@ pub mod pallet {
                 Error::<T>::ContributionTooLow
             );
 
+            // Ensure the crowdloan has not reached the maximum number of contributors
+            ensure!(
+                crowdloan.contributors_count < T::MaxContributors::get(),
+                Error::<T>::MaxContributorsReached
+            );
+
             // Ensure contribution does not overflow the actual raised amount
             // and it does not exceed the cap
             let left_to_raise = crowdloan
@@ -415,11 +453,21 @@ pub mod pallet {
                 .checked_add(amount)
                 .ok_or(Error::<T>::Overflow)?;
 
-            // Compute the new total contribution and ensure it does not overflow.
-            let contribution = Contributions::<T>::get(crowdloan_id, &contributor)
-                .unwrap_or(Zero::zero())
-                .checked_add(amount)
-                .ok_or(Error::<T>::Overflow)?;
+            // Compute the new total contribution and ensure it does not overflow, we
+            // also increment the contributor count if the contribution is new.
+            let contribution =
+                if let Some(contribution) = Contributions::<T>::get(crowdloan_id, &contributor) {
+                    contribution
+                        .checked_add(amount)
+                        .ok_or(Error::<T>::Overflow)?
+                } else {
+                    // We have a new contribution
+                    crowdloan.contributors_count = crowdloan
+                        .contributors_count
+                        .checked_add(1)
+                        .ok_or(Error::<T>::Overflow)?;
+                    amount
+                };
 
             // Ensure contributor has enough balance to pay
             ensure!(
@@ -476,6 +524,10 @@ pub mod pallet {
                 Contributions::<T>::insert(crowdloan_id, &who, crowdloan.deposit);
             } else {
                 Contributions::<T>::remove(crowdloan_id, &who);
+                crowdloan.contributors_count = crowdloan
+                    .contributors_count
+                    .checked_sub(1)
+                    .ok_or(Error::<T>::Underflow)?;
             }
 
             CurrencyOf::<T>::transfer(
@@ -625,6 +677,10 @@ pub mod pallet {
                 refund_count = refund_count.checked_add(1).ok_or(Error::<T>::Overflow)?;
             }
 
+            crowdloan.contributors_count = crowdloan
+                .contributors_count
+                .checked_sub(refund_count)
+                .ok_or(Error::<T>::Underflow)?;
             Crowdloans::<T>::insert(crowdloan_id, &crowdloan);
 
             // Clear refunded contributors
@@ -682,6 +738,7 @@ pub mod pallet {
                 creator_contribution,
                 Preservation::Expendable,
             )?;
+            Contributions::<T>::remove(crowdloan_id, &crowdloan.creator);
 
             // Clear the call from the preimage storage
             if let Some(call) = crowdloan.call {

pallets/crowdloan/src/migrations/migrate_add_contributors_count.rs

@@ -0,0 +1,188 @@
+use alloc::string::String;
+use frame_support::{BoundedVec, migration::storage_key_iter, traits::Get, weights::Weight};
+use subtensor_macros::freeze_struct;
+
+use crate::*;
+
+mod old_storage {
+    use super::*;
+
+    #[freeze_struct("84bcbf9b8d3f0ddf")]
+    #[derive(Encode, Decode, Debug)]
+    pub struct OldCrowdloanInfo<AccountId, Balance, BlockNumber, Call> {
+        pub creator: AccountId,
+        pub deposit: Balance,
+        pub min_contribution: Balance,
+        pub end: BlockNumber,
+        pub cap: Balance,
+        pub funds_account: AccountId,
+        pub raised: Balance,
+        pub target_address: Option<AccountId>,
+        pub call: Option<Call>,
+        pub finalized: bool,
+    }
+}
+
+pub fn migrate_add_contributors_count<T: Config>() -> Weight {
+    let migration_name = BoundedVec::truncate_from(b"migrate_add_contributors_count".to_vec());
+    let mut weight = T::DbWeight::get().reads(1);
+
+    if HasMigrationRun::<T>::get(&migration_name) {
+        log::info!(
+            "Migration '{:?}' has already run. Skipping.",
+            migration_name
+        );
+        return weight;
+    }
+
+    log::info!(
+        "Running migration '{}'",
+        String::from_utf8_lossy(&migration_name)
+    );
+
+    let pallet_name = b"Crowdloan";
+    let item_name = b"Crowdloans";
+    let crowdloans = storage_key_iter::<
+        CrowdloanId,
+        old_storage::OldCrowdloanInfo<
+            T::AccountId,
+            BalanceOf<T>,
+            BlockNumberFor<T>,
+            BoundedCallOf<T>,
+        >,
+        Twox64Concat,
+    >(pallet_name, item_name)
+    .collect::<Vec<_>>();
+    weight = weight.saturating_add(T::DbWeight::get().reads(crowdloans.len() as u64));
+
+    for (id, crowdloan) in crowdloans {
+        let contributions = Contributions::<T>::iter_key_prefix(id)
+            .collect::<Vec<_>>()
+            .len();
+        weight = weight.saturating_add(T::DbWeight::get().reads(contributions as u64));
+
+        Crowdloans::<T>::insert(
+            id,
+            CrowdloanInfo {
+                creator: crowdloan.creator,
+                deposit: crowdloan.deposit,
+                min_contribution: crowdloan.min_contribution,
+                end: crowdloan.end,
+                cap: crowdloan.cap,
+                funds_account: crowdloan.funds_account,
+                raised: crowdloan.raised,
+                target_address: crowdloan.target_address,
+                call: crowdloan.call,
+                finalized: crowdloan.finalized,
+                contributors_count: contributions as u32,
+            },
+        );
+        weight = weight.saturating_add(T::DbWeight::get().writes(1));
+    }
+
+    HasMigrationRun::<T>::insert(&migration_name, true);
+    weight = weight.saturating_add(T::DbWeight::get().writes(1));
+
+    log::info!(
+        "Migration '{:?}' completed successfully.",
+        String::from_utf8_lossy(&migration_name)
+    );
+
+    weight
+}
+
+#[cfg(test)]
+mod tests {
+    use frame_support::{Hashable, storage::unhashed::put_raw};
+    use sp_core::U256;
+    use sp_io::hashing::twox_128;
+
+    use super::*;
+    use crate::mock::{Test, TestState};
+
+    #[test]
+    fn test_migrate_add_contributors_count_works() {
+        TestState::default().build_and_execute(|| {
+            let pallet_name = twox_128(b"Crowdloan");
+            let storage_name = twox_128(b"Crowdloans");
+            let prefix = [pallet_name, storage_name].concat();
+
+            let items = vec![
+                (
+                    old_storage::OldCrowdloanInfo {
+                        creator: U256::from(1),
+                        deposit: 100u64,
+                        min_contribution: 10u64,
+                        end: 100u64,
+                        cap: 1000u64,
+                        funds_account: U256::from(2),
+                        raised: 0u64,
+                        target_address: None,
+                        call: None::<BoundedCallOf<Test>>,
+                        finalized: false,
+                    },
+                    vec![(U256::from(1), 100)],
+                ),
+                (
+                    old_storage::OldCrowdloanInfo {
+                        creator: U256::from(1),
+                        deposit: 100u64,
+                        min_contribution: 10u64,
+                        end: 100u64,
+                        cap: 1000u64,
+                        funds_account: U256::from(2),
+                        raised: 0u64,
+                        target_address: None,
+                        call: None::<BoundedCallOf<Test>>,
+                        finalized: false,
+                    },
+                    vec![
+                        (U256::from(1), 100),
+                        (U256::from(2), 100),
+                        (U256::from(3), 100),
+                    ],
+                ),
+                (
+                    old_storage::OldCrowdloanInfo {
+                        creator: U256::from(1),
+                        deposit: 100u64,
+                        min_contribution: 10u64,
+                        end: 100u64,
+                        cap: 1000u64,
+                        funds_account: U256::from(2),
+                        raised: 0u64,
+                        target_address: None,
+                        call: None::<BoundedCallOf<Test>>,
+                        finalized: false,
+                    },
+                    vec![
+                        (U256::from(1), 100),
+                        (U256::from(2), 100),
+                        (U256::from(3), 100),
+                        (U256::from(4), 100),
+                        (U256::from(5), 100),
+                    ],
+                ),
+            ];
+
+            for (id, (crowdloan, contributions)) in items.into_iter().enumerate() {
+                let key = [prefix.clone(), (id as u32).twox_64_concat()].concat();
+                put_raw(&key, &crowdloan.encode());
+
+                for (contributor, amount) in contributions {
+                    Contributions::<Test>::insert(id as u32, contributor, amount);
+                }
+            }
+
+            migrate_add_contributors_count::<Test>();
+
+            assert!(Crowdloans::<Test>::get(0).is_some_and(|c| c.contributors_count == 1));
+            assert!(Crowdloans::<Test>::get(1).is_some_and(|c| c.contributors_count == 3));
+            assert!(Crowdloans::<Test>::get(2).is_some_and(|c| c.contributors_count == 5));
+
+            assert!(HasMigrationRun::<Test>::get(BoundedVec::truncate_from(
+                b"migrate_add_contributors_count".to_vec()
+            )));
+        });
+    }
+}

pallets/crowdloan/src/migrations/mod.rs

@@ -0,0 +1,2 @@
+mod migrate_add_contributors_count;
+pub use migrate_add_contributors_count::*;