Better Parameterisation for Fee system (#3823)

* Better fee parameters

* Fix build

* Better runtime tests

* Price to Weight ratio as type parameter (#3856)

* Price to Weight ration as type parameter

* Kian feedback

* Some renames.

* Fix executor tests

* Getting Closer.

* Phantom Data

* Actually fix executor tests.

* Fix tests.

* Remove todo

* Fix build

substrate/node/runtime/src/constants.rs

@@ -66,19 +66,3 @@ pub mod time {
 	pub const HOURS: BlockNumber = MINUTES * 60;
 	pub const DAYS: BlockNumber = HOURS * 24;
 }
-
-// CRITICAL NOTE: The system module maintains two constants: a _maximum_ block weight and a _ratio_
-// of it yielding the portion which is accessible to normal transactions (reserving the rest for
-// operational ones). `TARGET_BLOCK_FULLNESS` is entirely independent and the system module is not
-// aware of if, nor should it care about it. This constant simply denotes on which ratio of the
-// _maximum_ block weight we tweak the fees. It does NOT care about the type of the dispatch.
-//
-// For the system to be configured in a sane way, `TARGET_BLOCK_FULLNESS` should always be less than
-// the ratio that `system` module uses to find normal transaction quota.
-/// Fee-related.
-pub mod fee {
-	pub use sr_primitives::Perbill;
-
-	/// The block saturation level. Fees will be updates based on this value.
-	pub const TARGET_BLOCK_FULLNESS: Perbill = Perbill::from_percent(25);
-}

substrate/node/runtime/src/impls.rs

@@ -19,10 +19,9 @@
 use node_primitives::Balance;
 use sr_primitives::weights::Weight;
 use sr_primitives::traits::{Convert, Saturating};
-use sr_primitives::Fixed64;
-use support::traits::{OnUnbalanced, Currency};
-use crate::{Balances, Authorship, MaximumBlockWeight, NegativeImbalance};
-use crate::constants::fee::TARGET_BLOCK_FULLNESS;
+use sr_primitives::{Fixed64, Perbill};
+use support::traits::{OnUnbalanced, Currency, Get};
+use crate::{Balances, System, Authorship, MaximumBlockWeight, NegativeImbalance};
 
 pub struct Author;
 impl OnUnbalanced<NegativeImbalance> for Author {
@@ -47,48 +46,34 @@ impl Convert<u128, Balance> for CurrencyToVoteHandler {
 	fn convert(x: u128) -> Balance { x * Self::factor() }
 }
 
-/// Handles converting a weight scalar to a fee value, based on the scale and granularity of the
-/// node's balance type.
-///
-/// This should typically create a mapping between the following ranges:
-///   - [0, system::MaximumBlockWeight]
-///   - [Balance::min, Balance::max]
-///
-/// Yet, it can be used for any other sort of change to weight-fee. Some examples being:
-///   - Setting it to `0` will essentially disable the weight fee.
-///   - Setting it to `1` will cause the literal `#[weight = x]` values to be charged.
-///
-/// By default, substrate node will have a weight range of [0, 1_000_000_000].
-pub struct WeightToFee;
-impl Convert<Weight, Balance> for WeightToFee {
-	fn convert(x: Weight) -> Balance {
+/// Convert from weight to balance via a simple coefficient multiplication
+/// The associated type C encapsulates a constant in units of balance per weight
+pub struct LinearWeightToFee<C>(rstd::marker::PhantomData<C>);
+
+impl<C: Get<Balance>> Convert<Weight, Balance> for LinearWeightToFee<C> {
+	fn convert(w: Weight) -> Balance {
 		// substrate-node a weight of 10_000 (smallest non-zero weight) to be mapped to 10^7 units of
 		// fees, hence:
-		Balance::from(x).saturating_mul(1_000)
+		let coefficient = C::get();
+		Balance::from(w).saturating_mul(coefficient)
 	}
 }
 
-/// A struct that updates the weight multiplier based on the saturation level of the previous block.
-/// This should typically be called once per-block.
+/// Update the given multiplier based on the following formula
 ///
-/// This assumes that weight is a numeric value in the u32 range.
-///
-/// Given `TARGET_BLOCK_FULLNESS = 1/2`, a block saturation greater than 1/2 will cause the system
-/// fees to slightly grow and the opposite for block saturations less than 1/2.
-///
-/// Formula:
-///   diff = (target_weight - current_block_weight)
+///   diff = (target_weight - previous_block_weight)
 ///   v = 0.00004
 ///   next_weight = weight * (1 + (v . diff) + (v . diff)^2 / 2)
 ///
+/// Where `target_weight` must be given as the `Get` implementation of the `T` generic type.
 /// https://research.web3.foundation/en/latest/polkadot/Token%20Economics/#relay-chain-transaction-fees
-pub struct FeeMultiplierUpdateHandler;
+pub struct TargetedFeeAdjustment<T>(rstd::marker::PhantomData<T>);
 
-impl Convert<(Weight, Fixed64), Fixed64> for FeeMultiplierUpdateHandler {
-	fn convert(previous_state: (Weight, Fixed64)) -> Fixed64 {
-		let (block_weight, multiplier) = previous_state;
+impl<T: Get<Perbill>> Convert<Fixed64, Fixed64> for TargetedFeeAdjustment<T> {
+	fn convert(multiplier: Fixed64) -> Fixed64 {
+		let block_weight = System::all_extrinsics_weight();
 		let max_weight = MaximumBlockWeight::get();
-		let target_weight = (TARGET_BLOCK_FULLNESS * max_weight) as u128;
+		let target_weight = (T::get() * max_weight) as u128;
 		let block_weight = block_weight as u128;
 
 		// determines if the first_term is positive
@@ -100,8 +85,8 @@ impl Convert<(Weight, Fixed64), Fixed64> for FeeMultiplierUpdateHandler {
 
 		// 0.00004 = 4/100_000 = 40_000/10^9
 		let v = Fixed64::from_rational(4, 100_000);
-		// 0.00004^2 = 16/10^10 ~= 2/10^9. Taking the future /2 into account, then it is just 1 parts
-		// from a billionth.
+		// 0.00004^2 = 16/10^10 ~= 2/10^9. Taking the future /2 into account, then it is just 1
+		// parts from a billionth.
 		let v_squared_2 = Fixed64::from_rational(1, 1_000_000_000);
 
 		let first_term = v.saturating_mul(diff);
@@ -132,15 +117,16 @@ impl Convert<(Weight, Fixed64), Fixed64> for FeeMultiplierUpdateHandler {
 mod tests {
 	use super::*;
 	use sr_primitives::weights::Weight;
+	use sr_primitives::assert_eq_error_rate;
 	use crate::{MaximumBlockWeight, AvailableBlockRatio, Runtime};
-	use crate::constants::currency::*;
+	use crate::{constants::currency::*, TransactionPayment, TargetBlockFullness};
 
 	fn max() -> Weight {
 		MaximumBlockWeight::get()
 	}
 
 	fn target() -> Weight {
-		TARGET_BLOCK_FULLNESS * max()
+		TargetBlockFullness::get() * max()
 	}
 
 	// poc reference implementation.
@@ -155,50 +141,68 @@ mod tests {
 		// Current saturation in terms of weight
 		let s = block_weight;
 
-		let fm = (v * (s/m - ss/m)) + (v.powi(2) * (s/m - ss/m).powi(2)) / 2.0;
-		let addition_fm = Fixed64::from_parts((fm * 1_000_000_000_f32) as i64);
+		let fm = v * (s/m - ss/m) + v.powi(2) * (s/m - ss/m).powi(2) / 2.0;
+		let addition_fm = Fixed64::from_parts((fm * 1_000_000_000_f32).round() as i64);
 		previous.saturating_add(addition_fm)
 	}
 
-	fn fm(parts: i64) -> Fixed64 {
+	fn feemul(parts: i64) -> Fixed64 {
 		Fixed64::from_parts(parts)
 	}
 
+	fn run_with_system_weight<F>(w: Weight, assertions: F) where F: Fn() -> () {
+		let mut t: runtime_io::TestExternalities =
+			system::GenesisConfig::default().build_storage::<Runtime>().unwrap().into();
+		t.execute_with(|| {
+			System::set_block_limits(w, 0);
+			assertions()
+		});
+	}
+
 	#[test]
 	fn fee_multiplier_update_poc_works() {
 		let fm = Fixed64::from_rational(0, 1);
 		let test_set = vec![
-			// TODO: this has a rounding error and fails.
-			// (0, fm.clone()),
+			(0, fm.clone()),
 			(100, fm.clone()),
 			(target(), fm.clone()),
 			(max() / 2, fm.clone()),
 			(max(), fm.clone()),
 		];
 		test_set.into_iter().for_each(|(w, fm)| {
-			assert_eq!(
-				fee_multiplier_update(w, fm),
-				FeeMultiplierUpdateHandler::convert((w, fm)),
-				"failed for weight {} and prev fm {:?}",
-				w,
-				fm,
-			);
+			run_with_system_weight(w, || {
+				assert_eq_error_rate!(
+					fee_multiplier_update(w, fm).into_inner(),
+					TargetedFeeAdjustment::<TargetBlockFullness>::convert(fm).into_inner(),
+					5,
+				);
+			})
 		})
 	}
 
 	#[test]
 	fn empty_chain_simulation() {
 		// just a few txs per_block.
-		let block_weight = 1000;
-		let mut fm = Fixed64::default();
-		let mut iterations: u64 = 0;
-		loop {
-			let next = FeeMultiplierUpdateHandler::convert((block_weight, fm));
-			fm = next;
-			if fm == Fixed64::from_rational(-1, 1) { break; }
-			iterations += 1;
-		}
-		println!("iteration {}, new fm = {:?}. Weight fee is now zero", iterations, fm);
+		let block_weight = 0;
+		run_with_system_weight(block_weight, || {
+			let mut fm = Fixed64::default();
+			let mut iterations: u64 = 0;
+			loop {
+				let next = TargetedFeeAdjustment::<TargetBlockFullness>::convert(fm);
+				fm = next;
+				if fm == Fixed64::from_rational(-1, 1) { break; }
+				iterations += 1;
+			}
+			println!("iteration {}, new fm = {:?}. Weight fee is now zero", iterations, fm);
+			assert!(iterations > 50_000, "This assertion is just a warning; Don't panic. \
+				Current substrate/polkadot node are configured with a _slow adjusting fee_ \
+				mechanism. Hence, it is really unlikely that fees collapse to zero even on an \
+				empty chain in less than at least of couple of thousands of empty blocks. But this \
+				simulation indicates that fees collapsed to zero after {} almost-empty blocks. \
+				Check it",
+				iterations,
+			);
+		})
 	}
 
 	#[test]
@@ -207,100 +211,116 @@ mod tests {
 		// `cargo test congested_chain_simulation -- --nocapture` to get some insight.
 
 		// almost full. The entire quota of normal transactions is taken.
-		let block_weight = AvailableBlockRatio::get() * max();
+		let block_weight = AvailableBlockRatio::get() * max() - 100;
 
-		// default minimum substrate weight
-		let tx_weight = 10_000u32;
+		// Default substrate minimum.
+		let tx_weight = 10_000;
 
-		// initial value of system
-		let mut fm = Fixed64::default();
-		assert_eq!(fm, Fixed64::from_parts(0));
+		run_with_system_weight(block_weight, || {
+			// initial value configured on module
+			let mut fm = Fixed64::default();
+			assert_eq!(fm, TransactionPayment::next_fee_multiplier());
 
-		let mut iterations: u64 = 0;
-		loop {
-			let next = FeeMultiplierUpdateHandler::convert((block_weight, fm));
-			if fm == next { break; }
-			fm = next;
-			iterations += 1;
-			let fee = <Runtime as transaction_payment::Trait>::WeightToFee::convert(tx_weight);
-			let adjusted_fee = fm.saturated_multiply_accumulate(fee);
-			println!(
-				"iteration {}, new fm = {:?}. Fee at this point is: \
-				{} units, {} millicents, {} cents, {} dollars",
-				iterations,
-				fm,
-				adjusted_fee,
-				adjusted_fee / MILLICENTS,
-				adjusted_fee / CENTS,
-				adjusted_fee / DOLLARS
-			);
-		}
+			let mut iterations: u64 = 0;
+			loop {
+				let next = TargetedFeeAdjustment::<TargetBlockFullness>::convert(fm);
+				// if no change, panic. This should never happen in this case.
+				if fm == next { panic!("The fee should ever increase"); }
+				fm = next;
+				iterations += 1;
+				let fee = <Runtime as transaction_payment::Trait>::WeightToFee::convert(tx_weight);
+				let adjusted_fee = fm.saturated_multiply_accumulate(fee);
+				println!(
+					"iteration {}, new fm = {:?}. Fee at this point is: {} units / {} millicents, \
+					{} cents, {} dollars",
+					iterations,
+					fm,
+					adjusted_fee,
+					adjusted_fee / MILLICENTS,
+					adjusted_fee / CENTS,
+					adjusted_fee / DOLLARS,
+				);
+			}
+		});
 	}
 
 	#[test]
 	fn stateless_weight_mul() {
-		// Light block. Fee is reduced a little.
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target() / 4, Fixed64::default())),
-			fm(-7500)
-		);
-		// a bit more. Fee is decreased less, meaning that the fee increases as the block grows.
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target() / 2, Fixed64::default())),
-			fm(-5000)
-		);
-		// ideal. Original fee. No changes.
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target(), Fixed64::default())),
-			fm(0)
-		);
-		// // More than ideal. Fee is increased.
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert(((target() * 2), Fixed64::default())),
-			fm(10000)
-		);
+		run_with_system_weight(target() / 4, || {
+			// Light block. Fee is reduced a little.
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default()),
+				feemul(-7500),
+			);
+		});
+		run_with_system_weight(target() / 2, || {
+			// a bit more. Fee is decreased less, meaning that the fee increases as the block grows.
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default()),
+				feemul(-5000),
+			);
+
+		});
+		run_with_system_weight(target(), || {
+			// ideal. Original fee. No changes.
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default()),
+				feemul(0),
+			);
+		});
+		run_with_system_weight(target() * 2, || {
+			// // More than ideal. Fee is increased.
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default()),
+				feemul(10000),
+			);
+		});
 	}
 
 	#[test]
 	fn stateful_weight_mul_grow_to_infinity() {
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target() * 2, Fixed64::default())),
-			fm(10000)
-		);
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target() * 2, fm(10000))),
-			fm(20000)
-		);
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target() * 2, fm(20000))),
-			fm(30000)
-		);
-		// ...
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((target() * 2, fm(1_000_000_000))),
-			fm(1_000_000_000 + 10000)
-		);
+		run_with_system_weight(target() * 2, || {
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default()),
+				feemul(10000)
+			);
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(feemul(10000)),
+				feemul(20000)
+			);
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(feemul(20000)),
+				feemul(30000)
+			);
+			// ...
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(feemul(1_000_000_000)),
+				feemul(1_000_000_000 + 10000)
+			);
+		});
 	}
 
 	#[test]
 	fn stateful_weight_mil_collapse_to_minus_one() {
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((0, Fixed64::default())),
-			fm(-10000)
-		);
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((0, fm(-10000))),
-			fm(-20000)
-		);
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((0, fm(-20000))),
-			fm(-30000)
-		);
-		// ...
-		assert_eq!(
-			FeeMultiplierUpdateHandler::convert((0, fm(1_000_000_000 * -1))),
-			fm(-1_000_000_000)
-		);
+		run_with_system_weight(0, || {
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default()),
+				feemul(-10000)
+			);
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(feemul(-10000)),
+				feemul(-20000)
+			);
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(feemul(-20000)),
+				feemul(-30000)
+			);
+			// ...
+			assert_eq!(
+				TargetedFeeAdjustment::<TargetBlockFullness>::convert(feemul(1_000_000_000 * -1)),
+				feemul(-1_000_000_000)
+			);
+		})
 	}
 
 	#[test]
@@ -309,6 +329,7 @@ mod tests {
 		let mb = kb * kb;
 		let max_fm = Fixed64::from_natural(i64::max_value());
 
+		// check that for all values it can compute, correctly.
 		vec![
 			0,
 			1,
@@ -322,7 +343,11 @@ mod tests {
 			Weight::max_value() / 2,
 			Weight::max_value()
 		].into_iter().for_each(|i| {
-			FeeMultiplierUpdateHandler::convert((i, Fixed64::default()));
+			run_with_system_weight(i, || {
+				let next = TargetedFeeAdjustment::<TargetBlockFullness>::convert(Fixed64::default());
+				let truth = fee_multiplier_update(i, Fixed64::default());
+				assert_eq_error_rate!(truth.into_inner(), next.into_inner(), 5);
+			});
 		});
 
 		// Some values that are all above the target and will cause an increase.
@@ -330,12 +355,11 @@ mod tests {
 		vec![t + 100, t * 2, t * 4]
 			.into_iter()
 			.for_each(|i| {
-				let fm = FeeMultiplierUpdateHandler::convert((
-					i,
-					max_fm
-				));
-				// won't grow. The convert saturates everything.
-				assert_eq!(fm, max_fm);
+				run_with_system_weight(i, || {
+					let fm = TargetedFeeAdjustment::<TargetBlockFullness>::convert(max_fm);
+					// won't grow. The convert saturates everything.
+					assert_eq!(fm, max_fm);
+				})
 			});
 	}
 }

substrate/node/runtime/src/lib.rs

@@ -64,7 +64,7 @@ pub use staking::StakerStatus;
 
 /// Implementations of some helper traits passed into runtime modules as associated types.
 pub mod impls;
-use impls::{CurrencyToVoteHandler, FeeMultiplierUpdateHandler, Author, WeightToFee};
+use impls::{CurrencyToVoteHandler, Author, LinearWeightToFee, TargetedFeeAdjustment};
 
 /// Constant values used within the runtime.
 pub mod constants;
@@ -109,9 +109,9 @@ pub type DealWithFees = SplitTwoWays<
 parameter_types! {
 	pub const BlockHashCount: BlockNumber = 250;
 	pub const MaximumBlockWeight: Weight = 1_000_000_000;
-	pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75);
 	pub const MaximumBlockLength: u32 = 5 * 1024 * 1024;
 	pub const Version: RuntimeVersion = VERSION;
+	pub const AvailableBlockRatio: Perbill = Perbill::from_percent(75);
 }
 
 impl system::Trait for Runtime {
@@ -176,6 +176,10 @@ impl balances::Trait for Runtime {
 parameter_types! {
 	pub const TransactionBaseFee: Balance = 1 * CENTS;
 	pub const TransactionByteFee: Balance = 10 * MILLICENTS;
+	// setting this to zero will disable the weight fee.
+	pub const WeightFeeCoefficient: Balance = 1_000;
+	// for a sane configuration, this should always be less than `AvailableBlockRatio`.
+	pub const TargetBlockFullness: Perbill = Perbill::from_percent(25);
 }
 
 impl transaction_payment::Trait for Runtime {
@@ -183,8 +187,8 @@ impl transaction_payment::Trait for Runtime {
 	type OnTransactionPayment = DealWithFees;
 	type TransactionBaseFee = TransactionBaseFee;
 	type TransactionByteFee = TransactionByteFee;
-	type WeightToFee = WeightToFee;
-	type FeeMultiplierUpdate = FeeMultiplierUpdateHandler;
+	type WeightToFee = LinearWeightToFee<WeightFeeCoefficient>;
+	type FeeMultiplierUpdate = TargetedFeeAdjustment<TargetBlockFullness>;
 }
 
 parameter_types! {