From cc77b92d25b4e67447de65014fad00d31125f74f Mon Sep 17 00:00:00 2001
From: bgallois <benjamin@gallois.cc>
Date: Tue, 4 Jun 2024 17:43:42 +0200
Subject: [PATCH] fix #236

---
 docs/user/fees.md               |  18 ++---
 resources/metadata.scale        | Bin 148283 -> 148323 bytes
 runtime/common/src/fees.rs      |  31 +++++--
 runtime/gdev/tests/fee_tests.rs | 139 ++++++++++++++++++++++++++++----
 4 files changed, 158 insertions(+), 30 deletions(-)

diff --git a/docs/user/fees.md b/docs/user/fees.md
index bec58eccc..4b79c2a5e 100644
--- a/docs/user/fees.md
+++ b/docs/user/fees.md
@@ -10,26 +10,26 @@ Transaction weight measures the computational resources required to process a tr
 
 ### Transaction Fees
 
-Transaction fees in Substrate-based blockchains are crucial for efficiently managing network resources and sustaining economic viability. They regulate resource allocation by ensuring transactions consuming more computational resources incur higher fees, discouraging spam, and promoting fair use of network capacity.  
-The fees are computed as follows:  
+Transaction fees in Substrate-based blockchains are crucial for efficiently managing network resources and sustaining economic viability. They regulate resource allocation by ensuring transactions consuming more computational resources incur higher fees, discouraging spam, and promoting fair use of network capacity.
+The fees are computed as follows:
 `fee = base_fee + weight2fee * fee_multiplier + length2fee + tip`
 
 ## Fees in Duniter
 
 ### Fees Implementation Details
 
-Implementing a zero-fee chain in Duniter involves configuring the blockchain to waive transaction fees when the current block weight is below a specified target. This approach aims to promote accessibility and encourage participation by eliminating fees during periods of lower network activity.   
-However, transaction fees are applied when the block weight surpasses the defined target to ensure network security and stability during increased usage. Additionally, leveraging the fee multiplier mechanism helps deter potential prolonged network attacks by dynamically adjusting fee levels based on previous network conditions.  
+Implementing a zero-fee chain in Duniter involves configuring the blockchain to waive transaction fees when the current block weight is below a specified target. This approach aims to promote accessibility and encourage participation by eliminating fees during periods of lower network activity.
+However, transaction fees are applied when the block weight or length surpasses the defined targets to ensure network security and stability during increased usage. Additionally, leveraging the fee multiplier mechanism helps deter potential prolonged network attacks by dynamically adjusting fee levels based on previous network conditions.
 Duniter members benefit from the quota system, which refunds transaction fees during high network activity periods.
 
 Fees are computed as follows:
-* If `current_weight < 0.25 * max_weight` and `fee_multiplier = 1`, ie. normal load:  
+* If `current_weight < 0.25 * max_weight` and `current_length < 0.25 * max_length` and `fee_multiplier = 1`, ie. normal load:
 `fee = 0`
-* If `current_weight > 0.25 * max_weight`, ie. heavy usage (approximately more than 25 transactions per second):  
-`fee = `5cÄž1 + extrinsic_weight * (5cÄž1/base_extrinsic_weight)* fee_multiplier + extrinsic_length/100 + tip`  
+* If `current_weight > 0.25 * max_weight` or `current_length > 0.25 * max_length` or `fee_multiplier > 1`, ie. heavy usage (approximately more than 135 transactions per second):
+`fee = `5cÄž1 + extrinsic_weight * (5cÄž1/base_extrinsic_weight)* fee_multiplier + extrinsic_length/100 + tip`
 
 The multiplier is updated as follows:
-* If `current_weight > 0.25 * max_weight`:  
+* If `current_weight > 0.25 * max_weight` or `current_length > 0.25 * max_length`:
 `Min(fee_multiplier += 1, 10)`
-* If `current_weight < 0.25 * max_weight`:  
+* If `current_weight < 0.25 * max_weight` and `current_length < 0.25 * max_length`:
 `Max(fee_multiplier -= 1, 1)`
diff --git a/resources/metadata.scale b/resources/metadata.scale
index fef41d8bafe615f1ce210c92a47aa4f81e05496d..bf88b3ce9a6e05e2a98b8c43493369f717191571 100644
GIT binary patch
delta 192
zcmdnp#`(C7vtbKk*}nRpj7h!{ISkwk3SKKOG7A5P0w?!&Nd`u4gKx{1>oE$aLBKLb
zZiX4(?PB4|_A@dtz*N@#5?l;d`3$HM)j&pehK``D5@7}iZ~$v#VAy_rALBO`K?iPz
lhP?I?M&UD1u>Iy?#tW7LHtY-nORv0uIjesAmcNYct^ml+RG9z(

delta 149
zcmaFd#<{zVvtbKk*}k|&#-#Lp@(kPz3SKKOGBPnZFfyq<7hquIZuob8r5;e=0+7ux
z<GWofko|#yk%0jw-=-w87$~N|2$Y9wW@KmR2+As9Vh8}M+x}@E<4+bI9d3q(y!H~H
hY6iybe-1NVu;kWYXAoF=<pmSN0*38({xY_^0suH^Dt!O|

diff --git a/runtime/common/src/fees.rs b/runtime/common/src/fees.rs
index bc41727b2..22a4d247e 100644
--- a/runtime/common/src/fees.rs
+++ b/runtime/common/src/fees.rs
@@ -62,7 +62,7 @@ where
     /// Function to convert weight to fee when "constant-fees" feature is not enabled.
     ///
     /// This function calculates the fee based on the length of the transaction in bytes.
-    /// If the current block weight is less than a fraction of the max block weight and the fee multiplier is one,
+    /// If the current block weight and length are less than a fraction of the max block weight and length and the fee multiplier is one,
     /// it returns a zero fee. Otherwise, it calculates the fee based on the length in bytes.
     #[cfg(not(feature = "constant-fees"))]
     fn weight_to_fee(length_in_bytes: &Weight) -> Self::Balance {
@@ -73,9 +73,15 @@ where
             .max_total
             .unwrap_or(weights.max_block);
         let current_block_weight = <frame_system::Pallet<U>>::block_weight();
+
+        let length = U::BlockLength::get();
+        let normal_max_length = *length.max.get(DispatchClass::Normal) as u64;
+        let current_block_length = <frame_system::Pallet<U>>::all_extrinsics_len() as u64;
+
         if current_block_weight
             .get(DispatchClass::Normal)
             .all_lt(X::get() * normal_max_weight)
+            && current_block_length < (X::get() * normal_max_length)
             && fee_multiplier.is_one()
         {
             0u32.into()
@@ -118,7 +124,7 @@ where
     /// Function to get the polynomial coefficients for weight to fee conversion.
     ///
     /// This function calculates the polynomial coefficients for converting transaction weight to fee.
-    /// If the current block weight is less than a fraction of the block max weight and the fee multiplier is one,
+    /// If the current block weight and length are less than a fraction of the block max weight and length, and the fee multiplier is one,
     /// it returns zero. Otherwise, it calculates the coefficients based on the extrinsic base weight mapped to 5 cents.
     fn polynomial() -> WeightToFeeCoefficients<Self::Balance> {
         let weights = U::BlockWeights::get();
@@ -128,9 +134,15 @@ where
             .max_total
             .unwrap_or(weights.max_block);
         let current_block_weight = <frame_system::Pallet<U>>::block_weight();
+
+        let length = U::BlockLength::get();
+        let normal_max_length = *length.max.get(DispatchClass::Normal) as u64;
+        let current_block_length = <frame_system::Pallet<U>>::all_extrinsics_len() as u64;
+
         if current_block_weight
             .get(DispatchClass::Normal)
             .all_lt(X::get() * normal_max_weight)
+            && current_block_length < (X::get() * normal_max_length)
             && fee_multiplier.is_one()
         {
             smallvec![WeightToFeeCoefficient {
@@ -212,9 +224,9 @@ where
 {
     /// Function to convert the previous fee multiplier to a new fee multiplier.
     ///
-    /// This function adjusts the fee multiplier based on the current block weight and target block fullness.
-    /// - If the current block weight is less than the target, it decreases the multiplier by one, with a minimum of one.
-    /// - If the current block weight is more than the target, it increases the multiplier by one, up to the maximum multiplier.
+    /// This function adjusts the fee multiplier based on the current block weight, length and target block fullness.
+    /// - If the current block weight and length are less than the target, it decreases the multiplier by one, with a minimum of one.
+    /// - If the current block weight or length is more than the target, it increases the multiplier by one, up to the maximum multiplier.
     #[cfg(not(feature = "constant-fees"))]
     fn convert(previous: Multiplier) -> Multiplier {
         let max_multiplier = X::get();
@@ -226,16 +238,21 @@ where
             .max_total
             .unwrap_or(weights.max_block);
 
+        let length = T::BlockLength::get();
+        let normal_max_length = *length.max.get(DispatchClass::Normal) as u64;
+        let current_block_length = <frame_system::Pallet<T>>::all_extrinsics_len() as u64;
+
         if <frame_system::Pallet<T>>::block_weight()
             .get(DispatchClass::Normal)
             .all_lt(target_block_fullness * normal_max_weight)
+            && current_block_length < (target_block_fullness * normal_max_length)
         {
-            // If the current block weight is less than the target, keep the
+            // If the current block weight and length are less than the target, keep the
             // multiplier at the minimum or decrease it by one to slowly
             // return to the minimum.
             previous.saturating_sub(1.into()).max(1.into())
         } else {
-            // If the current block weight is more than the target, increase
+            // If the current block weight or length is more than the target, increase
             // the multiplier by one.
             previous.saturating_add(1.into()).min(max_multiplier)
         }
diff --git a/runtime/gdev/tests/fee_tests.rs b/runtime/gdev/tests/fee_tests.rs
index 99d7b8338..45983d7e9 100644
--- a/runtime/gdev/tests/fee_tests.rs
+++ b/runtime/gdev/tests/fee_tests.rs
@@ -22,6 +22,7 @@ use common::*;
 use frame_support::{assert_ok, pallet_prelude::DispatchClass};
 use gdev_runtime::*;
 use sp_keyring::AccountKeyring;
+use sp_runtime::Perquintill;
 
 /// This test checks that an almost empty block incurs no fees for an extrinsic.
 #[test]
@@ -53,7 +54,7 @@ fn test_fees_empty() {
 /// - Multiple extrinsics are applied successfully without incurring fees until the block is under target weight.
 /// - The last extrinsic incurs additional fees as the block reaches its target, verifying fee calculation under high load conditions.
 #[test]
-fn test_fees_full() {
+fn test_fees_weight() {
     ExtBuilder::new(1, 3, 4)
         .with_initial_balances(vec![
             (AccountKeyring::Alice.to_account_id(), 10_000),
@@ -72,9 +73,8 @@ fn test_fees_full() {
                 .get(DispatchClass::Normal)
                 .all_lt(Target::get() * normal_max_weight * Perbill::from_percent(99))
             {
-                let call = RuntimeCall::Balances(BalancesCall::transfer_allow_death {
-                    dest: AccountKeyring::Eve.to_account_id().into(),
-                    value: 5,
+                let call = RuntimeCall::System(SystemCall::remark {
+                    remark: vec![255u8; 1],
                 });
                 let xt = get_unchecked_extrinsic(
                     call,
@@ -89,14 +89,13 @@ fn test_fees_full() {
             }
             assert_eq!(
                 Balances::free_balance(AccountKeyring::Alice.to_account_id()),
-                10_000 - 5 * transactions
+                10_000
             );
             // Ensure that the next extrinsic exceeds the limit.
-            System::set_block_consumed_resources(Target::get() * normal_max_weight, 100_usize);
+            System::set_block_consumed_resources(Target::get() * normal_max_weight, 0_usize);
             // The block will reach the fee limit, so the next extrinsic should start incurring fees.
-            let call = RuntimeCall::Balances(BalancesCall::transfer_allow_death {
-                dest: AccountKeyring::Eve.to_account_id().into(),
-                value: 5,
+            let call = RuntimeCall::System(SystemCall::remark {
+                remark: vec![255u8; 1],
             });
 
             let xt = get_unchecked_extrinsic(
@@ -108,9 +107,73 @@ fn test_fees_full() {
                 transactions as u32,
             );
             assert_ok!(Executive::apply_extrinsic(xt));
-            assert!(
-                Balances::free_balance(AccountKeyring::Alice.to_account_id())
-                    < 10_000 - 5 * transactions - 5
+            assert_ne!(
+                Balances::free_balance(AccountKeyring::Alice.to_account_id()),
+                10_000
+            );
+        })
+}
+
+/// This test checks the fee behavior when the block is almost full.
+/// - Multiple extrinsics are applied successfully without incurring fees until the block is under target length.
+/// - The last extrinsic incurs additional fees as the block reaches its target, verifying fee calculation under high load conditions.
+#[test]
+fn test_fees_length() {
+    ExtBuilder::new(1, 3, 4)
+        .with_initial_balances(vec![
+            (AccountKeyring::Alice.to_account_id(), 10_000),
+            (AccountKeyring::Eve.to_account_id(), 10_000),
+        ])
+        .build()
+        .execute_with(|| {
+            let mut transactions = 0u64;
+            let length = BlockLength::get();
+            let normal_max_length = *length.max.get(DispatchClass::Normal) as u64;
+
+            // Stopping just below the limit
+            while u64::from(System::all_extrinsics_len())
+                < (Target::get() * Perquintill::from_percent(99) * normal_max_length)
+            {
+                let call = RuntimeCall::System(SystemCall::remark {
+                    remark: vec![255u8; 1_000],
+                });
+                let xt = get_unchecked_extrinsic(
+                    call,
+                    4u64,
+                    8u64,
+                    AccountKeyring::Alice,
+                    0u64,
+                    transactions as u32,
+                );
+                assert_ok!(Executive::apply_extrinsic(xt));
+                transactions += 1;
+            }
+            assert_eq!(
+                Balances::free_balance(AccountKeyring::Alice.to_account_id()),
+                10_000
+            );
+            // Ensure that the next extrinsic exceeds the limit.
+            System::set_block_consumed_resources(
+                Weight::zero(),
+                (Target::get() * normal_max_length).try_into().unwrap(),
+            );
+            // The block will reach the fee limit, so the next extrinsic should start incurring fees.
+            let call = RuntimeCall::System(SystemCall::remark {
+                remark: vec![255u8; 1],
+            });
+
+            let xt = get_unchecked_extrinsic(
+                call,
+                4u64,
+                8u64,
+                AccountKeyring::Alice,
+                0u64,
+                transactions as u32,
+            );
+            assert_ok!(Executive::apply_extrinsic(xt));
+            assert_ne!(
+                Balances::free_balance(AccountKeyring::Alice.to_account_id()),
+                10_000
             );
         })
 }
@@ -118,7 +181,7 @@ fn test_fees_full() {
 /// This test checks the behavior of the fee multiplier based on block weight
 /// and previous block weight.
 #[test]
-fn test_fees_multiplier() {
+fn test_fees_multiplier_weight() {
     ExtBuilder::new(1, 3, 4)
         .with_initial_balances(vec![
             (AccountKeyring::Alice.to_account_id(), 10_000),
@@ -140,7 +203,55 @@ fn test_fees_multiplier() {
             // the fee multiplier is increased by one, up to the MaxMultiplier.
             let mut current = 0u128;
             for i in 1..20u128 {
-                System::set_block_consumed_resources(Target::get() * normal_max_weight, 100_usize);
+                System::set_block_consumed_resources(Target::get() * normal_max_weight, 0_usize);
+                run_to_block(i as u32);
+                current += 1;
+                assert_eq!(
+                    pallet_transaction_payment::Pallet::<Runtime>::next_fee_multiplier(),
+                    core::cmp::min(current.into(), MaxMultiplier::get())
+                );
+            }
+
+            // If the block weight is under the target and the previous block was also under the target,
+            // the fee multiplier is decreased by one, down to the one.
+            let mut current = 10u128;
+            for i in 20..50u32 {
+                run_to_block(i);
+                current = current.saturating_sub(1).max(1u128);
+                assert_eq!(
+                    pallet_transaction_payment::Pallet::<Runtime>::next_fee_multiplier(),
+                    current.into()
+                );
+            }
+        })
+}
+
+/// This test checks the behavior of the fee multiplier based on block length
+/// and previous block length.
+#[test]
+fn test_fees_multiplier_length() {
+    ExtBuilder::new(1, 3, 4)
+        .with_initial_balances(vec![
+            (AccountKeyring::Alice.to_account_id(), 10_000),
+            (AccountKeyring::Eve.to_account_id(), 10_000),
+        ])
+        .build()
+        .execute_with(|| {
+            let length = BlockLength::get();
+            let normal_max_length = *length.max.get(DispatchClass::Normal) as u64;
+
+            assert_eq!(
+                pallet_transaction_payment::Pallet::<Runtime>::next_fee_multiplier(),
+                1.into()
+            );
+            // If the block weight is over the target and the previous block was also over the target,
+            // the fee multiplier is increased by one, up to the MaxMultiplier.
+            let mut current = 0u128;
+            for i in 1..20u128 {
+                System::set_block_consumed_resources(
+                    Weight::zero(),
+                    (Target::get() * normal_max_length).try_into().unwrap(),
+                );
                 run_to_block(i as u32);
                 current += 1;
                 assert_eq!(
-- 
GitLab