remove unnecessary storage item in length fee adjustment

Author: rphmeier

sugondat/chain/pallets/length-fee-adjustment/src/lib.rs

@@ -54,12 +54,25 @@ pub mod pallet {
     use frame_support::pallet_prelude::*;
     use frame_system::pallet_prelude::*;
     use pallet_transaction_payment::{Multiplier, OnChargeTransaction};
-    use polkadot_primitives::v6::PersistedValidationData;
+    use polkadot_primitives::v6::{BlockNumber as RelayChainBlockNumber, PersistedValidationData};
     use sp_runtime::{
         traits::{Get, One, Zero},
         FixedPointNumber, Perquintill, SaturatedConversion, Saturating,
     };
 
+    /// A provider for the last relay-chain block number, i.e. the relay-parent number of the
+    /// previous block from _this_ chain.
+    pub trait LastRelayBlockNumberProvider {
+        /// Get the last relay chain block number.
+        fn last_relay_block_number() -> RelayChainBlockNumber;
+    }
+
+    impl<T: cumulus_pallet_parachain_system::Config> LastRelayBlockNumberProvider for T {
+        fn last_relay_block_number() -> RelayChainBlockNumber {
+            cumulus_pallet_parachain_system::Pallet::<T>::last_relay_block_number()
+        }
+    }
+
     /// Configure the pallet by specifying the parameters and types on which it depends.
     #[pallet::config]
     pub trait Config: frame_system::Config + pallet_transaction_payment::Config {
@@ -92,6 +105,8 @@ pub mod pallet {
         /// This means that producing a block at most every n skipped blocks should be enforced to avoid falling into this error.
         #[pallet::constant]
         type SkippedBlocksNumberTerms: Get<u32>;
+
+        type LastRelayBlockNumberProvider: LastRelayBlockNumberProvider;
     }
 
     #[pallet::pallet]
@@ -119,9 +134,6 @@ pub mod pallet {
     pub type TargetBlockSize<T: Config> =
         StorageValue<_, Perquintill, ValueQuery, TargetBlockSizeDefault>;
 
-    #[pallet::storage]
-    pub type PrevRelayBlockNumber<T: Config> = StorageValue<_, u32, OptionQuery>;
-
     #[pallet::hooks]
     impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
         fn on_initialize(_: BlockNumberFor<T>) -> Weight {
@@ -217,16 +229,14 @@ pub mod pallet {
     impl<T: Config> OnSystemEvent for Pallet<T> {
         fn on_validation_data(data: &PersistedValidationData) {
             let relay_block_number = data.relay_parent_number;
+            let prev_relay_block_number =
+                T::LastRelayBlockNumberProvider::last_relay_block_number();
 
-            let prev_relay_block_number = PrevRelayBlockNumber::<T>::get();
-            PrevRelayBlockNumber::<T>::put(relay_block_number);
-
-            // if nothing was already present then this is the first block to
-            // be executed with the fee adjustment so nothing should be done
-            let prev_relay_block_number = match prev_relay_block_number {
-                Some(prev) => prev,
-                None => return,
-            };
+            // a value of zero here implies this is the first block of the parachain. no need
+            // to do a massive fee update.
+            if prev_relay_block_number == RelayChainBlockNumber::zero() {
+                return;
+            }
 
             // It should never be negative because the relay_block_number is surely
             // greater than the para_block_number.

sugondat/chain/pallets/length-fee-adjustment/src/mock.rs

@@ -11,6 +11,9 @@ use sp_runtime::{
     FixedPointNumber, SaturatedConversion,
 };
 
+use pallet_sugondat_length_fee_adjustment::LastRelayBlockNumberProvider;
+use polkadot_primitives::v6::BlockNumber as RelayChainBlockNumber;
+
 type Block = frame_system::mocking::MockBlock<Test>;
 type Balance = u64;
 
@@ -87,6 +90,19 @@ parameter_types! {
 
     pub static WeightToFee: u64 = 1;
     pub static OperationalFeeMultiplier: u8 = 5;
+    pub static LastRelayBlockNumber: RelayChainBlockNumber = 0;
+}
+
+pub struct MockLastRelayBlockNumberProvider;
+
+impl LastRelayBlockNumberProvider for MockLastRelayBlockNumberProvider {
+    fn last_relay_block_number() -> RelayChainBlockNumber {
+        LastRelayBlockNumber::get()
+    }
+}
+
+pub fn set_last_relay_block_number(n: RelayChainBlockNumber) {
+    LastRelayBlockNumber::mutate(|x| *x = n);
 }
 
 impl WeightToFeeT for WeightToFee {
@@ -114,4 +130,5 @@ impl pallet_sugondat_length_fee_adjustment::Config for Test {
     type MinimumMultiplierBlockSize = MinimumMultiplierBlockSize;
     type MaximumMultiplierBlockSize = MaximumMultiplierBlockSize;
     type SkippedBlocksNumberTerms = SkippedBlocksNumberTerms;
+    type LastRelayBlockNumberProvider = MockLastRelayBlockNumberProvider;
 }

sugondat/chain/pallets/length-fee-adjustment/src/tests.rs

@@ -1,5 +1,5 @@
 use crate::{
-    mock::{LengthFeeAdjustment, Test},
+    mock::{self, LengthFeeAdjustment, Test},
     *,
 };
 use cumulus_pallet_parachain_system::OnSystemEvent;
@@ -66,6 +66,27 @@ fn test_default_target_block_size() {
     });
 }
 
+#[test]
+fn test_no_update_when_prev_is_zero() {
+    new_test_ext().execute_with(|| {
+        // using Multiplier::one() only e^(-vnt) is tested
+        NextLengthMultiplier::<Test>::put(Multiplier::one());
+        mock::set_last_relay_block_number(0);
+
+        let relay_data = PersistedValidationData {
+            parent_head: HeadData(vec![]),
+            relay_parent_number: 100_000_000,
+            relay_parent_storage_root: sp_core::H256::zero(),
+            max_pov_size: 0,
+        };
+
+        LengthFeeAdjustment::on_validation_data(&relay_data);
+
+        let mul = NextLengthMultiplier::<Test>::get();
+        assert_eq!(mul, Multiplier::one());
+    });
+}
+
 #[test]
 fn test_skipped_block_multiplier_update() {
     // TODO: change max_skipped_blocks to 7200
@@ -76,14 +97,14 @@ fn test_skipped_block_multiplier_update() {
         for d in (0..max_skipped_blocks).step_by(max_skipped_blocks as usize / 1000) {
             // using Multiplier::one() only e^(-vnt) is tested
             NextLengthMultiplier::<Test>::put(Multiplier::one());
-            PrevRelayBlockNumber::<Test>::put(0);
+            mock::set_last_relay_block_number(1);
 
             // TODO: update with `1 + d`
             // when updating to asynchronous backing
             // https://github.com/thrumdev/blobs/issues/166
             let relay_data = PersistedValidationData {
                 parent_head: HeadData(vec![]),
-                relay_parent_number: 2 + d * 2,
+                relay_parent_number: 1 + 2 + d * 2, // extra 1 is because last rp was 1
                 relay_parent_storage_root: sp_core::H256::zero(),
                 max_pov_size: 0,
             };
@@ -119,13 +140,6 @@ fn test_skipped_block_no_prev_data() {
         };
 
         LengthFeeAdjustment::on_validation_data(&relay_data);
-
-        // PrevRelayBlockNumber should be set to what passed in the argument
-        // as relay_parent_number and NextLengthMultiplier should remain the same
-        assert_eq!(
-            PrevRelayBlockNumber::<Test>::get(),
-            Some(relay_parent_number)
-        );
         assert_eq!(NextLengthMultiplier::<Test>::get(), prev_multiplier);
     });
 }

sugondat/chain/runtimes/sugondat-kusama/src/lib.rs

@@ -312,6 +312,7 @@ impl pallet_sugondat_length_fee_adjustment::Config for Runtime {
     type MaximumMultiplierBlockSize = MaximumMultiplierBlockSize;
     type MinimumMultiplierBlockSize = MinimumMultiplierBlockSize;
     type SkippedBlocksNumberTerms = SkippedBlocksNumberTerms;
+    type LastRelayBlockNumberProvider = Runtime;
 }
 
 pub type SlowAdjustingFeeUpdate<R> = TargetedFeeAdjustment<

sugondat/chain/runtimes/test/src/lib.rs

@@ -379,6 +379,7 @@ impl pallet_sugondat_length_fee_adjustment::Config for Runtime {
     type MaximumMultiplierBlockSize = MaximumMultiplierBlockSize;
     type MinimumMultiplierBlockSize = MinimumMultiplierBlockSize;
     type SkippedBlocksNumberTerms = SkippedBlocksNumberTerms;
+    type LastRelayBlockNumberProvider = Runtime;
 }
 
 pub type SlowAdjustingFeeUpdate<R> = TargetedFeeAdjustment<