Fix the broken weight multiplier update function (#6334)

* Initial draft, has some todos left * remove ununsed import * Apply suggestions from code review * Some refactors with migration * Fix more test and cleanup * Fix for companion * Apply suggestions from code review Co-authored-by: Alexander Popiak <alexander.popiak@parity.io> * Update bin/node/runtime/src/impls.rs * Fix weight * Add integrity test * length is not affected. Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
2026-06-18 15:21:05 +00:00 · 2020-06-17 15:20:17 +02:00
parent 67c9ac9393
commit 82bdf1a891
10 changed files with 458 additions and 253 deletions
@@ -44,7 +44,7 @@ use frame_support::{
 	dispatch::DispatchResult,
 };
 use sp_runtime::{
-	FixedI128, FixedPointNumber, FixedPointOperand,
+	FixedU128, FixedPointNumber, FixedPointOperand, Perquintill, RuntimeDebug,
 	transaction_validity::{
 		TransactionPriority, ValidTransaction, InvalidTransaction, TransactionValidityError,
 		TransactionValidity,
@@ -57,13 +57,125 @@ use sp_runtime::{
 use pallet_transaction_payment_rpc_runtime_api::RuntimeDispatchInfo;

 /// Fee multiplier.
-pub type Multiplier = FixedI128;
+pub type Multiplier = FixedU128;

 type BalanceOf<T> =
 	<<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
 type NegativeImbalanceOf<T> =
 	<<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::NegativeImbalance;

+/// A struct to update the weight multiplier per block. It implements `Convert<Multiplier,
+/// Multiplier>`, meaning that it can convert the previous multiplier to the next one. This should
+/// be called on `on_finalize` of a block, prior to potentially cleaning the weight data from the
+/// system module.
+///
+/// given:
+/// 	s = previous block weight
+/// 	s'= ideal block weight
+/// 	m = maximum block weight
+///		diff = (s - s')/m
+///		v = 0.00001
+///		t1 = (v * diff)
+///		t2 = (v * diff)^2 / 2
+///	then:
+/// 	next_multiplier = prev_multiplier * (1 + t1 + t2)
+///
+/// Where `(s', v)` must be given as the `Get` implementation of the `T` generic type. Moreover, `M`
+/// must provide the minimum allowed value for the multiplier. Note that a runtime should ensure
+/// with tests that the combination of this `M` and `V` is not such that the multiplier can drop to
+/// zero and never recover.
+///
+/// note that `s'` is interpreted as a portion in the _normal transaction_ capacity of the block.
+/// For example, given `s' == 0.25` and `AvailableBlockRatio = 0.75`, then the target fullness is
+/// _0.25 of the normal capacity_ and _0.1875 of the entire block_.
+///
+/// This implementation implies the bound:
+/// - `v ≤ p / k * (s − s')`
+/// - or, solving for `p`: `p >= v * k * (s - s')`
+///
+/// where `p` is the amount of change over `k` blocks.
+///
+/// Hence:
+/// - in a fully congested chain: `p >= v * k * (1 - s')`.
+/// - in an empty chain: `p >= v * k * (-s')`.
+///
+/// For example, when all blocks are full and there are 28800 blocks per day (default in `substrate-node`)
+/// and v == 0.00001, s' == 0.1875, we'd have:
+///
+/// p >= 0.00001 * 28800 * 0.8125
+/// p >= 0.234
+///
+/// Meaning that fees can change by around ~23% per day, given extreme congestion.
+///
+/// More info can be found at:
+/// https://w3f-research.readthedocs.io/en/latest/polkadot/Token%20Economics.html
+pub struct TargetedFeeAdjustment<T, S, V, M>(sp_std::marker::PhantomData<(T, S, V, M)>);
+
+impl<T, S, V, M> Convert<Multiplier, Multiplier> for TargetedFeeAdjustment<T, S, V, M>
+	where T: frame_system::Trait, S: Get<Perquintill>, V: Get<Multiplier>, M: Get<Multiplier>,
+{
+	fn convert(previous: Multiplier) -> Multiplier {
+		// Defensive only. The multiplier in storage should always be at most positive. Nonetheless
+		// we recover here in case of errors, because any value below this would be stale and can
+		// never change.
+		let min_multiplier = M::get();
+		let previous = previous.max(min_multiplier);
+
+		// the computed ratio is only among the normal class.
+		let normal_max_weight =
+			<T as frame_system::Trait>::AvailableBlockRatio::get() *
+			<T as frame_system::Trait>::MaximumBlockWeight::get();
+		let normal_block_weight =
+			<frame_system::Module<T>>::block_weight()
+			.get(frame_support::weights::DispatchClass::Normal)
+			.min(normal_max_weight);
+
+		let s = S::get();
+		let v = V::get();
+
+		let target_weight = (s * normal_max_weight) as u128;
+		let block_weight = normal_block_weight as u128;
+
+		// determines if the first_term is positive
+		let positive = block_weight >= target_weight;
+		let diff_abs = block_weight.max(target_weight) - block_weight.min(target_weight);
+
+		// defensive only, a test case assures that the maximum weight diff can fit in Multiplier
+		// without any saturation.
+		let diff = Multiplier::saturating_from_rational(diff_abs, normal_max_weight.max(1));
+		let diff_squared = diff.saturating_mul(diff);
+
+		let v_squared_2 = v.saturating_mul(v) / Multiplier::saturating_from_integer(2);
+
+		let first_term = v.saturating_mul(diff);
+		let second_term = v_squared_2.saturating_mul(diff_squared);
+
+		if positive {
+			let excess = first_term.saturating_add(second_term).saturating_mul(previous);
+			previous.saturating_add(excess).max(min_multiplier)
+		} else {
+			// Defensive-only: first_term > second_term. Safe subtraction.
+			let negative = first_term.saturating_sub(second_term).saturating_mul(previous);
+			previous.saturating_sub(negative).max(min_multiplier)
+		}
+	}
+}
+
+/// Storage releases of the module.
+#[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, RuntimeDebug)]
+enum Releases {
+	/// Original version of the module.
+	V1Ancient,
+	/// One that bumps the usage to FixedU128 from FixedI128.
+	V2,
+}
+
+impl Default for Releases {
+	fn default() -> Self {
+		Releases::V1Ancient
+	}
+}
+
 pub trait Trait: frame_system::Trait {
 	/// The currency type in which fees will be paid.
 	type Currency: Currency<Self::AccountId> + Send + Sync;
@@ -85,7 +197,9 @@ pub trait Trait: frame_system::Trait {

 decl_storage! {
 	trait Store for Module<T: Trait> as TransactionPayment {
-		pub NextFeeMultiplier get(fn next_fee_multiplier): Multiplier = Multiplier::from_inner(0);
+		pub NextFeeMultiplier get(fn next_fee_multiplier): Multiplier = Multiplier::saturating_from_integer(1);
+
+		StorageVersion build(|_: &GenesisConfig| Releases::V2): Releases;
 	}
 }

@@ -103,6 +217,51 @@ decl_module! {
 				*fm = T::FeeMultiplierUpdate::convert(*fm);
 			});
 		}
+
+		fn integrity_test() {
+			// given weight == u64, we build multipliers from `diff` of two weight values, which can
+			// at most be MaximumBlockWeight. Make sure that this can fit in a multiplier without
+			// loss.
+			use sp_std::convert::TryInto;
+			assert!(
+				<Multiplier as sp_runtime::traits::Bounded>::max_value() >=
+				Multiplier::checked_from_integer(
+					<T as frame_system::Trait>::MaximumBlockWeight::get().try_into().unwrap()
+				).unwrap(),
+			);
+		}
+
+		fn on_runtime_upgrade() -> Weight {
+			use frame_support::migration::take_storage_value;
+			use sp_std::convert::TryInto;
+			use frame_support::debug::native::error;
+
+			type OldMultiplier = sp_runtime::FixedI128;
+			type OldInner = <OldMultiplier as FixedPointNumber>::Inner;
+			type Inner = <Multiplier as FixedPointNumber>::Inner;
+
+			if let Releases::V1Ancient = StorageVersion::get() {
+				StorageVersion::put(Releases::V2);
+
+				if let Some(old) = take_storage_value::<OldMultiplier>(
+					b"TransactionPayment",
+					b"NextFeeMultiplier",
+					&[],
+				) {
+					let inner = old.into_inner();
+					let new_inner = <OldInner as TryInto<Inner>>::try_into(inner)
+						.unwrap_or_default();
+					let new = Multiplier::from_inner(new_inner);
+					NextFeeMultiplier::put(new);
+					T::DbWeight::get().reads_writes(1, 1)
+				} else {
+					error!("transaction-payment migration failed.");
+					T::DbWeight::get().reads(1)
+				}
+			} else {
+				T::DbWeight::get().reads(1)
+			}
+		}
 	}
 }

@@ -157,7 +316,7 @@ impl<T: Trait> Module<T> where
 	/// the minimum fee for a transaction to be included in a block.
 	///
 	/// ```ignore
-	/// inclusion_fee = base_fee + targeted_fee_adjustment * (len_fee + weight_fee);
+	/// inclusion_fee = base_fee + len_fee + [targeted_fee_adjustment * weight_fee];
 	/// final_fee = inclusion_fee + tip;
 	/// ```
 	pub fn compute_fee(
@@ -194,16 +353,21 @@ impl<T: Trait> Module<T> where
 		if pays_fee == Pays::Yes {
 			let len = <BalanceOf<T>>::from(len);
 			let per_byte = T::TransactionByteFee::get();
-			let len_fee = per_byte.saturating_mul(len);
-			let unadjusted_weight_fee = Self::weight_to_fee(weight);

-			// the adjustable part of the fee
-			let adjustable_fee = len_fee.saturating_add(unadjusted_weight_fee);
-			let targeted_fee_adjustment = NextFeeMultiplier::get();
-			let adjusted_fee = targeted_fee_adjustment.saturating_mul_acc_int(adjustable_fee);
+			// length fee. this is not adjusted.
+			let fixed_len_fee = per_byte.saturating_mul(len);
+
+			// the adjustable part of the fee.
+			let unadjusted_weight_fee = Self::weight_to_fee(weight);
+			let multiplier = Self::next_fee_multiplier();
+			// final adjusted weight fee.
+			let adjusted_weight_fee = multiplier.saturating_mul_int(unadjusted_weight_fee);

 			let base_fee = Self::weight_to_fee(T::ExtrinsicBaseWeight::get());
-			base_fee.saturating_add(adjusted_fee).saturating_add(tip)
+			base_fee
+				.saturating_add(fixed_len_fee)
+				.saturating_add(adjusted_weight_fee)
+				.saturating_add(tip)
 		} else {
 			tip
 		}
@@ -213,12 +377,12 @@ impl<T: Trait> Module<T> where
 impl<T: Trait> Module<T> {
 	/// Compute the fee for the specified weight.
 	///
-	/// This fee is already adjusted by the per block fee adjustment factor and is therefore
-	/// the share that the weight contributes to the overall fee of a transaction.
+	/// This fee is already adjusted by the per block fee adjustment factor and is therefore the
+	/// share that the weight contributes to the overall fee of a transaction.
 	///
-	/// This function is generic in order to supply the contracts module with a way
-	/// to calculate the gas price. The contracts module is not able to put the necessary
-	/// `BalanceOf<T>` contraints on its trait. This function is not to be used by this module.
+	/// This function is generic in order to supply the contracts module with a way to calculate the
+	/// gas price. The contracts module is not able to put the necessary `BalanceOf<T>` constraints
+	/// on its trait. This function is not to be used by this module.
 	pub fn weight_to_fee_with_adjustment<Balance>(weight: Weight) -> Balance where
 		Balance: UniqueSaturatedFrom<u128>
 	{
@@ -576,6 +740,37 @@ mod tests {
 		PostDispatchInfo { actual_weight: None, }
 	}

+	#[test]
+	fn migration_to_v2_works() {
+		use sp_runtime::FixedI128;
+		use frame_support::traits::OnRuntimeUpgrade;
+
+		let with_old_multiplier = |mul: FixedI128, expected: FixedU128| {
+			ExtBuilder::default().build().execute_with(|| {
+				frame_support::migration::put_storage_value(
+					b"TransactionPayment",
+					b"NextFeeMultiplier",
+					&[],
+					mul,
+				);
+
+				assert_eq!(StorageVersion::get(), Releases::V1Ancient);
+
+				TransactionPayment::on_runtime_upgrade();
+
+				assert_eq!(StorageVersion::get(), Releases::V2);
+				assert_eq!(NextFeeMultiplier::get(), expected);
+			})
+		};
+
+		with_old_multiplier(FixedI128::saturating_from_integer(-1), FixedU128::zero());
+		with_old_multiplier(FixedI128::saturating_from_rational(-1, 2), FixedU128::zero());
+		with_old_multiplier(
+			FixedI128::saturating_from_rational(1, 2),
+			FixedU128::saturating_from_rational(1, 2),
+		);
+	}
+
 	#[test]
 	fn signed_extension_transaction_payment_work() {
 		ExtBuilder::default()
@@ -620,21 +815,21 @@ mod tests {
 			.execute_with(||
 		{
 			let len = 10;
-			NextFeeMultiplier::put(Multiplier::saturating_from_rational(1, 2));
+			NextFeeMultiplier::put(Multiplier::saturating_from_rational(3, 2));

 			let pre = ChargeTransactionPayment::<Runtime>::from(5 /* tipped */)
 				.pre_dispatch(&2, CALL, &info_from_weight(100), len)
 				.unwrap();
-			// 5 base fee, 3/2 * 10 byte fee, 3/2 * 100 weight fee, 5 tip
-			assert_eq!(Balances::free_balance(2), 200 - 5 - 15 - 150 - 5);
+			// 5 base fee, 10 byte fee, 3/2 * 100 weight fee, 5 tip
+			assert_eq!(Balances::free_balance(2), 200 - 5 - 10 - 150 - 5);

 			assert!(
 				ChargeTransactionPayment::<Runtime>
 					::post_dispatch(pre, &info_from_weight(100), &post_info_from_weight(50), len, &Ok(()))
 					.is_ok()
 			);
-			// 75 (3/2 of the returned 50 units of weight ) is refunded
-			assert_eq!(Balances::free_balance(2), 200 - 5 - 15 - 75 - 5);
+			// 75 (3/2 of the returned 50 units of weight) is refunded
+			assert_eq!(Balances::free_balance(2), 200 - 5 - 10 - 75 - 5);
 		});
 	}

@@ -708,7 +903,7 @@ mod tests {
 			.execute_with(||
 		{
 			// all fees should be x1.5
-			NextFeeMultiplier::put(Multiplier::saturating_from_rational(1, 2));
+			NextFeeMultiplier::put(Multiplier::saturating_from_rational(3, 2));
 			let len = 10;

 			assert!(
@@ -716,7 +911,14 @@ mod tests {
 					.pre_dispatch(&1, CALL, &info_from_weight(3), len)
 					.is_ok()
 			);
-			assert_eq!(Balances::free_balance(1), 100 - 10 - 5 - (10 + 3) * 3 / 2);
+			assert_eq!(
+				Balances::free_balance(1),
+				100 // original
+				- 10 // tip
+				- 5 // base
+				- 10 // len
+				- (3 * 3 / 2) // adjusted weight
+			);
 		})
 	}

@@ -736,7 +938,7 @@ mod tests {
 			.execute_with(||
 		{
 			// all fees should be x1.5
-			NextFeeMultiplier::put(Multiplier::saturating_from_rational(1, 2));
+			NextFeeMultiplier::put(Multiplier::saturating_from_rational(3, 2));

 			assert_eq!(
 				TransactionPayment::query_info(xt, len),
@@ -745,10 +947,8 @@ mod tests {
 					class: info.class,
 					partial_fee:
 						5 * 2 /* base * weight_fee */
-						+ (
-							len as u64 /* len * 1 */
-							+ info.weight.min(MaximumBlockWeight::get()) as u64 * 2 /* weight * weight_to_fee */
-						) * 3 / 2
+						+ len as u64  /* len * 1 */
+						+ info.weight.min(MaximumBlockWeight::get()) as u64 * 2 * 3 / 2 /* weight */
 				},
 			);

@@ -765,7 +965,7 @@ mod tests {
 			.execute_with(||
 		{
 			// Next fee multiplier is zero
-			assert_eq!(NextFeeMultiplier::get(), Multiplier::saturating_from_integer(0));
+			assert_eq!(NextFeeMultiplier::get(), Multiplier::one());

 			// Tip only, no fees works
 			let dispatch_info = DispatchInfo {
@@ -804,8 +1004,8 @@ mod tests {
 			.build()
 			.execute_with(||
 		{
-			// Add a next fee multiplier
-			NextFeeMultiplier::put(Multiplier::saturating_from_rational(1, 2)); // = 1/2 = .5
+			// Add a next fee multiplier. Fees will be x3/2.
+			NextFeeMultiplier::put(Multiplier::saturating_from_rational(3, 2));
 			// Base fee is unaffected by multiplier
 			let dispatch_info = DispatchInfo {
 				weight: 0,
@@ -821,10 +1021,10 @@ mod tests {
 				pays_fee: Pays::Yes,
 			};
 			// 123 weight, 456 length, 100 base
-			// adjustable fee = (123 * 1) + (456 * 10) = 4683
-			// adjusted fee = (4683 * .5) + 4683 = 7024.5 -> 7024
-			// final fee = 100 + 7024 + 789 tip = 7913
-			assert_eq!(Module::<Runtime>::compute_fee(456, &dispatch_info, 789), 7913);
+			assert_eq!(
+				Module::<Runtime>::compute_fee(456, &dispatch_info, 789),
+				100 + (3 * 123 / 2) + 4560 + 789,
+			);
 		});
 	}

@@ -837,9 +1037,10 @@ mod tests {
 			.build()
 			.execute_with(||
 		{
-			// Add a next fee multiplier
-			NextFeeMultiplier::put(Multiplier::saturating_from_rational(-1, 2)); // = -1/2 = -.5
-			// Base fee is unaffected by multiplier
+			// Add a next fee multiplier. All fees will be x1/2.
+			NextFeeMultiplier::put(Multiplier::saturating_from_rational(1, 2));
+
+			// Base fee is unaffected by multiplier.
 			let dispatch_info = DispatchInfo {
 				weight: 0,
 				class: DispatchClass::Operational,
@@ -847,17 +1048,17 @@ mod tests {
 			};
 			assert_eq!(Module::<Runtime>::compute_fee(0, &dispatch_info, 0), 100);

-			// Everything works together :)
+			// Everything works together.
 			let dispatch_info = DispatchInfo {
 				weight: 123,
 				class: DispatchClass::Operational,
 				pays_fee: Pays::Yes,
 			};
 			// 123 weight, 456 length, 100 base
-			// adjustable fee = (123 * 1) + (456 * 10) = 4683
-			// adjusted fee = 4683 - (4683 * -.5)  = 4683 - 2341.5 = 4683 - 2341 = 2342
-			// final fee = 100 + 2342 + 789 tip = 3231
-			assert_eq!(Module::<Runtime>::compute_fee(456, &dispatch_info, 789), 3231);
+			assert_eq!(
+				Module::<Runtime>::compute_fee(456, &dispatch_info, 789),
+				100 + (123 / 2) + 4560 + 789,
+			);
 		});
 	}

@@ -993,7 +1194,7 @@ mod tests {
 			let len = 10;
 			let tip = 5;

-			NextFeeMultiplier::put(Multiplier::saturating_from_rational(1, 4));
+			NextFeeMultiplier::put(Multiplier::saturating_from_rational(5, 4));

 			let pre = ChargeTransactionPayment::<Runtime>::from(tip)
 				.pre_dispatch(&2, CALL, &info, len)
@@ -1007,11 +1208,8 @@ mod tests {
 			let actual_fee = Module::<Runtime>
 				::compute_actual_fee(len as u32, &info, &post_info, tip);

-			// 33 weight, 10 length, 7 base
-			// adjustable fee = (33 * 1) + (10 * 1) = 43
-			// adjusted fee = 43 + (43 * .25)  = 43 + 10.75 = 43 + 10 = 53
-			// final fee = 7 + 53 + 5 tip = 65
-			assert_eq!(actual_fee, 65);
+			// 33 weight, 10 length, 7 base, 5 tip
+			assert_eq!(actual_fee, 7 + 10 + (33 * 5 / 4) + 5);
 			assert_eq!(refund_based_fee, actual_fee);
 		});
 	}