fix(pallet-tfgrid): improve IP validation (#870)

Author: renauter

substrate-node/pallets/pallet-tfgrid/src/farm.rs

@@ -322,10 +322,13 @@ impl<T: Config> Pallet<T> {
         let mut public_ips_list: PublicIpListOf =
             vec![].try_into().map_err(|_| Error::<T>::InvalidPublicIP)?;
 
-        for ip in public_ips {
+        for ip4 in public_ips {
+            // Check if it's a valid IP4
+            ip4.is_valid().map_err(|_| Error::<T>::InvalidPublicIP)?;
+
             let pub_ip = PublicIP {
-                ip: ip.ip,
-                gateway: ip.gw,
+                ip: ip4.ip,
+                gateway: ip4.gw,
                 contract_id: 0,
             };
 

substrate-node/pallets/pallet-tfgrid/src/migrations/mod.rs

@@ -5,4 +5,5 @@ pub mod v12;
 pub mod v13;
 pub mod v14;
 //pub mod v15;
-pub mod v16;
+pub mod v16;
+pub mod v17;

substrate-node/pallets/pallet-tfgrid/src/migrations/v17.rs

@@ -0,0 +1,123 @@
+use crate::*;
+use frame_support::{traits::Get, traits::OnRuntimeUpgrade, weights::Weight};
+use log::{debug, info};
+use scale_info::prelude::string::String;
+use sp_core::ConstU32;
+use sp_runtime::{BoundedVec, Saturating};
+use sp_std::{marker::PhantomData, vec::Vec};
+use tfchain_support::types::{PublicIP, IP4};
+
+#[cfg(feature = "try-runtime")]
+use parity_scale_codec::{Decode, Encode};
+
+pub struct FixFarmPublicIps<T: Config>(PhantomData<T>);
+
+impl<T: Config> OnRuntimeUpgrade for FixFarmPublicIps<T> {
+    #[cfg(feature = "try-runtime")]
+    fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+        info!("current pallet version: {:?}", PalletVersion::<T>::get());
+        assert!(PalletVersion::<T>::get() >= types::StorageVersion::V16Struct);
+
+        let farms_count: u64 = Farms::<T>::iter().count() as u64;
+        info!(
+            "🔎 FixFarmPublicIps pre migration: Number of existing farms {:?}",
+            farms_count
+        );
+
+        info!("👥  TFGrid pallet to V17 passes PRE migrate checks ✅",);
+        Ok(farms_count.encode())
+    }
+
+    fn on_runtime_upgrade() -> Weight {
+        if PalletVersion::<T>::get() == types::StorageVersion::V16Struct {
+            fix_farm_public_ips::<T>()
+        } else {
+            info!(" >>> Unused TFGrid pallet V17 migration");
+            Weight::zero()
+        }
+    }
+
+    #[cfg(feature = "try-runtime")]
+    fn post_upgrade(pre_farms_count: Vec<u8>) -> Result<(), &'static str> {
+        info!("current pallet version: {:?}", PalletVersion::<T>::get());
+        assert!(PalletVersion::<T>::get() >= types::StorageVersion::V17Struct);
+
+        // Check number of farms against pre-check result
+        let pre_farms_count: u64 = Decode::decode(&mut pre_farms_count.as_slice())
+            .expect("the state parameter should be something that was generated by pre_upgrade");
+        assert_eq!(
+            Farms::<T>::iter().count() as u64,
+            pre_farms_count,
+            "Number of farms migrated does not match"
+        );
+
+        info!(
+            "👥  TFGrid pallet migration to {:?} passes POST migrate checks ✅",
+            Pallet::<T>::pallet_version()
+        );
+
+        Ok(())
+    }
+}
+
+pub fn fix_farm_public_ips<T: Config>() -> frame_support::weights::Weight {
+    info!(" >>> Migrating farms storage...");
+
+    let mut r = 0u64;
+    let mut w = 0u64;
+
+    let farms = Farms::<T>::iter().collect::<Vec<_>>();
+    r.saturating_accrue(farms.len() as u64);
+
+    for (_, mut farm) in Farms::<T>::iter() {
+        r.saturating_inc();
+        let size = farm.public_ips.len();
+
+        farm.public_ips.retain(|pubip| {
+            let ip4 = IP4 {
+                ip: pubip.ip.clone(),
+                gw: pubip.gateway.clone(),
+            };
+            ip4.is_valid().is_ok()
+        });
+
+        // Update farm only if some invalid IP was found
+        if farm.public_ips.len() < size {
+            debug!("Farm #{:?}: invalid IP found", farm.id);
+            debug!(
+                " public ips were: {:?}",
+                public_ips_to_string(Farms::<T>::get(farm.id).unwrap().public_ips)
+            );
+
+            Farms::<T>::insert(farm.id, &farm);
+
+            debug!(
+                " public ips now: {:?}",
+                public_ips_to_string(Farms::<T>::get(farm.id).unwrap().public_ips)
+            );
+            w.saturating_inc();
+        }
+    }
+
+    info!(" <<< Farms storage updated! Migrated {} Farms ✅", w);
+
+    // Update pallet storage version
+    PalletVersion::<T>::set(types::StorageVersion::V17Struct);
+    w.saturating_inc();
+    info!(" <<< Storage version upgraded");
+
+    // Return the weight consumed by the migration.
+    T::DbWeight::get().reads_writes(r, w)
+}
+
+fn public_ips_to_string(public_ips: BoundedVec<PublicIP, ConstU32<256>>) -> String {
+    let mut s = String::new();
+    for pub_ip in public_ips {
+        s.push_str("{ ip: ");
+        s.push_str(&String::from_utf8_lossy(&pub_ip.ip));
+        s.push_str(", gw: ");
+        s.push_str(&String::from_utf8_lossy(&pub_ip.gateway));
+        s.push_str("} ");
+    }
+    s
+}

substrate-node/pallets/pallet-tfgrid/src/tests.rs

@@ -411,7 +411,7 @@ fn test_update_farm_name_existing_name_fails() {
 }
 
 #[test]
-fn test_create_farm_with_double_ip_fails() {
+fn test_create_farm_duplicate_ip_fails() {
     ExternalityBuilder::build().execute_with(|| {
         create_entity();
         create_twin();
@@ -438,6 +438,26 @@ fn test_create_farm_with_double_ip_fails() {
     });
 }
 
+#[test]
+fn test_create_farm_not_valid_ip_fails() {
+    ExternalityBuilder::build().execute_with(|| {
+        create_entity();
+        create_twin();
+
+        let farm_name = get_farm_name_input(b"test_farm");
+
+        let mut pub_ips: PublicIpListInput<TestRuntime> = bounded_vec![];
+        let ip = get_public_ip_ip_input(b"185.206.122.33/24");
+        let gw = get_public_ip_gw_input(b"185.206.122.33");
+        pub_ips.try_push(IP4 { ip, gw }).unwrap();
+
+        assert_noop!(
+            TfgridModule::create_farm(RuntimeOrigin::signed(alice()), farm_name, pub_ips),
+            Error::<TestRuntime>::InvalidPublicIP
+        );
+    });
+}
+
 #[test]
 fn test_adding_ip_to_farm_works() {
     ExternalityBuilder::build().execute_with(|| {

substrate-node/pallets/pallet-tfgrid/src/types.rs

@@ -24,11 +24,12 @@ pub enum StorageVersion {
     V14Struct,
     V15Struct,
     V16Struct,
+    V17Struct,
 }
 
 impl Default for StorageVersion {
     fn default() -> StorageVersion {
-        StorageVersion::V16Struct
+        StorageVersion::V17Struct
     }
 }
 

substrate-node/runtime/src/lib.rs

@@ -771,10 +771,11 @@ pub type Executive = frame_executive::Executive<
 // All migrations executed on runtime upgrade as a nested tuple of types implementing
 // `OnRuntimeUpgrade`.
 type Migrations = (
-    pallet_tfgrid::migrations::v16::KillNodeGpuStatus<Runtime>,
-    pallet_smart_contract::migrations::v10::ReworkBillingLoopInsertion<Runtime>,
-    pallet_smart_contract::migrations::v11::ExtendContractLock<Runtime>,
-    migrations::remove_sudo::RemoveSudo<Runtime>,
+    // pallet_tfgrid::migrations::v16::KillNodeGpuStatus<Runtime>,
+    // pallet_smart_contract::migrations::v10::ReworkBillingLoopInsertion<Runtime>,
+    // pallet_smart_contract::migrations::v11::ExtendContractLock<Runtime>,
+    // migrations::remove_sudo::RemoveSudo<Runtime>,
+    pallet_tfgrid::migrations::v17::FixFarmPublicIps<Runtime>,
 );
 
 // follows Substrate's non destructive way of eliminating  otherwise required

substrate-node/support/src/lib.rs

@@ -1,6 +1,9 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
+pub mod constants;
+pub mod resources;
 pub mod traits;
 pub mod types;
-pub mod resources;
-pub mod constants;
+
+#[cfg(test)]
+mod tests;

substrate-node/support/src/resources.rs

@@ -114,150 +114,3 @@ impl Resources {
         return self.hru == 0 && self.sru == 0 && self.cru == 0 && self.mru == 0;
     }
 }
-
-#[cfg(test)]
-mod test {
-    use super::*;
-
-    #[test]
-    fn test_calc_cu_falsy_values() {
-        let resources = Resources {
-            hru: 0,
-            cru: 0,
-            mru: 0,
-            sru: 0,
-        };
-
-        let cu = resources.get_cu();
-        assert_eq!(cu, 0);
-    }
-
-    #[test]
-    fn test_calc_cu() {
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64 * GIGABYTE as u64 * 1024,
-            sru: 12 * GIGABYTE as u64 * 1024,
-        };
-
-        let cu = resources.get_cu();
-        assert_eq!(cu, 256);
-    }
-
-    #[test]
-    fn test_calc_cu_2() {
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 4,
-            mru: 8,
-            sru: 12 * GIGABYTE as u64 * 1024,
-        };
-
-        let cu = resources.get_cu();
-        assert_eq!(cu, 2);
-    }
-
-    #[test]
-    fn test_calc_su() {
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64,
-            sru: 12 * GIGABYTE as u64 * 1024,
-        };
-
-        let su = resources.get_su();
-        assert_eq!(su, 52);
-    }
-
-    #[test]
-    fn test_calc_su_2() {
-        let resources = Resources {
-            hru: 0,
-            cru: 64,
-            mru: 64,
-            sru: 12 * GIGABYTE as u64 * 1024,
-        };
-
-        let su = resources.get_su();
-        assert_eq!(su, 49);
-    }
-
-    #[test]
-    fn test_calc_su_3() {
-        let resources = Resources {
-            hru: 0,
-            cru: 64,
-            mru: 64,
-            sru: 0,
-        };
-
-        let su = resources.get_su();
-        assert_eq!(su, 0);
-    }
-
-    #[test]
-    fn test_calc_su_4() {
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64,
-            sru: 0,
-        };
-
-        let su = resources.get_su();
-        assert_eq!(su, 3);
-    }
-
-    #[test]
-    fn test_resources_diff() {
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64 * GIGABYTE as u64,
-            sru: 0,
-        };
-
-        let new_resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64 * GIGABYTE as u64,
-            sru: 0,
-        };
-
-        assert_eq!(Resources::has_changed(&resources, &new_resources, 1), false);
-
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64 * GIGABYTE as u64,
-            sru: 0,
-        };
-
-        let new_resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 40 * GIGABYTE as u64,
-            sru: 0,
-        };
-
-        assert_eq!(Resources::has_changed(&resources, &new_resources, 1), true);
-
-        let resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64 * GIGABYTE as u64,
-            sru: 1000 * GIGABYTE as u64,
-        };
-
-        let new_resources = Resources {
-            hru: 4 * GIGABYTE as u64 * 1024,
-            cru: 64,
-            mru: 64 * GIGABYTE as u64,
-            sru: 989 * GIGABYTE as u64,
-        };
-
-        assert_eq!(Resources::has_changed(&resources, &new_resources, 1), true);
-    }
-}