Improved weight guards + tests

Also reduce the maximum storage proof size required, as it previously was 1/8 of the maximum PoV size per block on Polkadot

zrml/asset-router/src/lib.rs

@@ -70,6 +70,9 @@ pub mod pallet {
     pub(crate) use zeitgeist_primitives::traits::CheckedDivPerComponent;
 
     pub(crate) const LOG_TARGET: &str = "runtime::asset-router";
+    const MAX_ASSET_DESTRUCTIONS_PER_BLOCK: u8 = 128;
+    pub(crate) const MAX_ASSETS_IN_DESTRUCTION: u32 = 2048;
+    const MAX_INDESTRUCTIBLE_ASSETS: u32 = 256;
 
     pub trait AssetTraits<T: Config, A>:
         Create<T::AccountId, AssetId = A, Balance = T::Balance>
@@ -191,7 +194,7 @@ pub mod pallet {
     /// Keeps track of assets that can't be destroyed.
     #[pallet::storage]
     pub(crate) type IndestructibleAssets<T: Config> =
-        StorageValue<_, BoundedVec<T::AssetType, ConstU32<8192>>, ValueQuery>;
+        StorageValue<_, BoundedVec<T::AssetType, ConstU32<MAX_ASSETS_IN_DESTRUCTION>>, ValueQuery>;
 
     #[pallet::error]
     pub enum Error<T> {
@@ -215,7 +218,26 @@ pub mod pallet {
     #[pallet::hooks]
     impl<T: Config> Hooks<T::BlockNumber> for Pallet<T> {
         fn on_idle(_: T::BlockNumber, mut remaining_weight: Weight) -> Weight {
-            if !remaining_weight.all_gte(T::DbWeight::get().reads(1)) {
+            let max_proof_size_destructibles: u64 =
+                AssetInDestruction::<T::AssetType>::max_encoded_len()
+                    .saturating_mul(MAX_ASSETS_IN_DESTRUCTION.saturated_into())
+                    .saturated_into();
+            let max_proof_size_indestructibles: u64 = T::AssetType::max_encoded_len()
+                .saturating_mul(MAX_INDESTRUCTIBLE_ASSETS.saturated_into())
+                .saturated_into();
+            let max_proof_size_total =
+                max_proof_size_destructibles.saturating_add(max_proof_size_indestructibles);
+
+            let max_extra_weight = Weight::from_parts(
+                // 1ms extra execution time
+                1_000_000_000,
+                // Maximum proof size assuming writes on full storage
+                max_proof_size_total,
+            );
+
+            if !remaining_weight
+                .all_gte(max_extra_weight.saturating_add(T::DbWeight::get().reads(1)))
+            {
                 return remaining_weight;
             };
 
@@ -224,18 +246,19 @@ pub mod pallet {
                 return remaining_weight.saturating_sub(T::DbWeight::get().reads(1));
             }
 
-            remaining_weight =
-                remaining_weight.saturating_sub(T::DbWeight::get().reads_writes(1, 1));
-            let saftey_counter_outer_max = 128u8;
+            remaining_weight = remaining_weight
+                .saturating_sub(T::DbWeight::get().reads_writes(1, 1))
+                .saturating_sub(max_extra_weight);
+            let saftey_counter_outer_max = MAX_ASSET_DESTRUCTIONS_PER_BLOCK;
             let mut saftey_counter_outer;
 
             'outer: while let Some(mut asset) = destroy_assets.pop() {
-                let safety_counter_inner_max = 6u8;
+                let safety_counter_inner_max = DESTRUCTION_STATES;
                 let mut safety_counter_inner = 0u8;
 
                 while (*asset.state() != DestructionState::Destroyed
                     && *asset.state() != DestructionState::Indestructible)
-                        && safety_counter_inner < safety_counter_inner_max
+                    && safety_counter_inner < safety_counter_inner_max
                 {
                     match asset.state() {
                         DestructionState::Accounts => {

zrml/asset-router/src/mock.rs

@@ -66,7 +66,7 @@ pub(super) const CUSTOM_ASSET_INITIAL_AMOUNT: Balance = 20;
 pub(super) const MARKET_ASSET_INITIAL_AMOUNT: Balance = 30;
 pub(super) const CURRENCY_INITIAL_AMOUNT: Balance = 40;
 
-pub(super) const DESTROY_WEIGHT: Weight = Weight::from_all(1000);
+pub(super) const DESTROY_WEIGHT: Weight = Weight::from_parts(1000, 0);
 
 pub(super) type AccountId = <Runtime as frame_system::Config>::AccountId;
 pub(super) type CustomAssetsInstance = pallet_assets::Instance1;

zrml/asset-router/src/tests/managed_destroy.rs

@@ -18,8 +18,12 @@
 #![cfg(test)]
 
 use super::*;
-use crate::AssetInDestruction;
-use frame_support::{traits::tokens::fungibles::Inspect, BoundedVec};
+use crate::{AssetInDestruction, Weight};
+use frame_support::{
+    traits::{tokens::fungibles::Inspect, Get},
+    weights::RuntimeDbWeight,
+    BoundedVec,
+};
 use pallet_assets::ManagedDestroy;
 
 #[test]
@@ -121,15 +125,17 @@ fn destroys_assets_fully_works_properly() {
         assert_ok!(managed_destroy_multi_transactional(assets.clone()));
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 3);
 
-        let available_weight = 1_000_000_000.into();
+        let available_weight = 10_000_000_000.into();
         let remaining_weight = AssetRouter::on_idle(0, available_weight);
         assert!(!AssetRouter::asset_exists(CAMPAIGN_ASSET));
         assert!(!AssetRouter::asset_exists(CUSTOM_ASSET));
         assert!(!AssetRouter::asset_exists(MARKET_ASSET));
         assert_eq!(crate::IndestructibleAssets::<Runtime>::get(), vec![]);
         assert_eq!(crate::DestroyAssets::<Runtime>::get(), vec![]);
 
-        let consumed_weight = available_weight - 3u64 * 3u64 * DESTROY_WEIGHT;
+        let mut consumed_weight = available_weight - 3u64 * 3u64 * DESTROY_WEIGHT;
+        // Consider safety buffer for extra execution time and storage proof size
+        consumed_weight = consumed_weight.saturating_sub(Weight::from_parts(1_000_000_000, 45_824));
         assert_eq!(remaining_weight, consumed_weight);
     })
 }
@@ -147,15 +153,17 @@ fn destroys_assets_partially_properly() {
         assert_ok!(managed_destroy_multi_transactional(assets.clone()));
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 3);
 
-        let available_weight = DESTROY_WEIGHT * 3;
+        let mut available_weight: Weight = Weight::from_all(1_000_000_000) + 2u64 * DESTROY_WEIGHT;
         // Make on_idle only partially delete the first asset
-        let _ = AssetRouter::on_idle(0, available_weight - 2u32 * DESTROY_WEIGHT);
+        let _ = AssetRouter::on_idle(0, available_weight);
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 3);
 
         // Now delete each asset one by one by supplying exactly the required weight
+        available_weight = Weight::from_all(1_000_000_000) + DESTROY_WEIGHT;
         let _ = AssetRouter::on_idle(0, available_weight);
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 2);
 
+        available_weight = Weight::from_all(1_000_000_000) + 3u64 * DESTROY_WEIGHT;
         let _ = AssetRouter::on_idle(0, available_weight);
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 1);
 
@@ -173,7 +181,7 @@ fn properly_handles_indestructible_assets() {
     ExtBuilder::default().build().execute_with(|| {
         let assets_raw = vec![CAMPAIGN_ASSET, CUSTOM_ASSET, MARKET_ASSET];
         let mut destroy_assets = crate::DestroyAssets::<Runtime>::get();
-        let available_weight = 1_000_000_000.into();
+        let available_weight = 10_000_000_000.into();
 
         for asset in assets_raw {
             destroy_assets.force_push(AssetInDestruction::new(asset));
@@ -226,7 +234,9 @@ fn properly_handles_indestructible_assets() {
         crate::DestroyAssets::<Runtime>::put(destroy_assets);
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 3);
         let remaining_weight = AssetRouter::on_idle(0, available_weight);
-        let consumed_weight = available_weight - 2u32 * 3u32 * DESTROY_WEIGHT - DESTROY_WEIGHT;
+        let mut consumed_weight = available_weight - 2u32 * 3u32 * DESTROY_WEIGHT - DESTROY_WEIGHT;
+        // Consider safety buffer for extra execution time and storage proof size
+        consumed_weight = consumed_weight.saturating_sub(Weight::from_parts(1_000_000_000, 45_824));
         assert_eq!(remaining_weight, consumed_weight);
         assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 0);
         assert_eq!(crate::IndestructibleAssets::<Runtime>::get().len(), 1);
@@ -236,3 +246,31 @@ fn properly_handles_indestructible_assets() {
         assert!(!AssetRouter::asset_exists(MARKET_ASSET));
     })
 }
+
+#[test]
+fn does_not_execute_on_insufficient_weight() {
+    ExtBuilder::default().build().execute_with(|| {
+        assert_ok!(AssetRouter::create(CAMPAIGN_ASSET, ALICE, true, CAMPAIGN_ASSET_MIN_BALANCE));
+        assert_ok!(AssetRouter::managed_destroy(CAMPAIGN_ASSET, None));
+        assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 1);
+
+        let db_weight: RuntimeDbWeight = <Runtime as frame_system::Config>::DbWeight::get();
+        let mut available_weight =
+            Weight::from_parts(1_000_000_000, 45_824) + db_weight.reads(1) - 1u64.into();
+        let mut remaining_weight = AssetRouter::on_idle(0, available_weight);
+        assert_eq!(available_weight, remaining_weight);
+        assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 1);
+
+        available_weight += 1u64.into();
+        let mut remaining_weight_expected: Weight = 0u64.into();
+        remaining_weight = AssetRouter::on_idle(0, available_weight);
+        assert_eq!(remaining_weight_expected, remaining_weight);
+        assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 1);
+
+        remaining_weight_expected = 1u64.into();
+        available_weight += 3u64 * DESTROY_WEIGHT + remaining_weight_expected;
+        remaining_weight = AssetRouter::on_idle(0, available_weight);
+        assert_eq!(remaining_weight_expected, remaining_weight);
+        assert_eq!(crate::DestroyAssets::<Runtime>::get().len(), 0);
+    })
+}

zrml/asset-router/src/types.rs

@@ -15,13 +15,15 @@
 // You should have received a copy of the GNU General Public License
 // along with Zeitgeist. If not, see <https://www.gnu.org/licenses/>.
 
-use crate::{BoundedVec, Config, ConstU32, Weight};
+use crate::{BoundedVec, Config, ConstU32, Weight, MAX_ASSETS_IN_DESTRUCTION};
 use core::cmp::Ordering;
 use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
 
-pub(crate) type DestroyAssetsT<T> =
-    BoundedVec<AssetInDestruction<<T as Config>::AssetType>, ConstU32<8192>>;
+pub(crate) type DestroyAssetsT<T> = BoundedVec<
+    AssetInDestruction<<T as Config>::AssetType>,
+    ConstU32<{ MAX_ASSETS_IN_DESTRUCTION }>,
+>;
 
 pub(crate) enum DestructionOk {
     Complete(Weight),
@@ -45,6 +47,7 @@ pub(crate) enum DestructionState {
     Destroyed,
     Indestructible,
 }
+pub(crate) const DESTRUCTION_STATES: u8 = 5;
 
 #[derive(Clone, Copy, Encode, Eq, Debug, Decode, MaxEncodedLen, PartialEq, TypeInfo)]
 pub(crate) struct AssetInDestruction<A> {