Fix the broken weight multiplier update function (paritytech#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 <[email protected]>

* Update bin/node/runtime/src/impls.rs

* Fix weight

* Add integrity test

* length is not affected.

Co-authored-by: Alexander Popiak <[email protected]>

bin/node/executor/tests/basic.rs

@@ -19,14 +19,11 @@ use codec::{Encode, Decode, Joiner};
 use frame_support::{
 	StorageValue, StorageMap,
 	traits::Currency,
-	weights::{
-		GetDispatchInfo, DispatchInfo, DispatchClass, constants::ExtrinsicBaseWeight,
-		WeightToFeePolynomial,
-	},
+	weights::{GetDispatchInfo, DispatchInfo, DispatchClass},
 };
 use sp_core::{NeverNativeValue, traits::Externalities, storage::well_known_keys};
 use sp_runtime::{
-	ApplyExtrinsicResult, FixedI128, FixedPointNumber,
+	ApplyExtrinsicResult,
 	traits::Hash as HashT,
 	transaction_validity::InvalidTransaction,
 };
@@ -35,7 +32,7 @@ use frame_system::{self, EventRecord, Phase};
 
 use node_runtime::{
 	Header, Block, UncheckedExtrinsic, CheckedExtrinsic, Call, Runtime, Balances,
-	System, TransactionPayment, Event, TransactionByteFee,
+	System, TransactionPayment, Event,
 	constants::currency::*,
 };
 use node_primitives::{Balance, Hash};
@@ -52,16 +49,17 @@ use self::common::{*, sign};
 /// test code paths that differ between native and wasm versions.
 pub const BLOATY_CODE: &[u8] = node_runtime::WASM_BINARY_BLOATY;
 
-/// Default transfer fee
-fn transfer_fee<E: Encode>(extrinsic: &E, fee_multiplier: FixedI128) -> Balance {
-	let length_fee = TransactionByteFee::get() * (extrinsic.encode().len() as Balance);
-
-	let base_weight = ExtrinsicBaseWeight::get();
-	let base_fee = <Runtime as pallet_transaction_payment::Trait>::WeightToFee::calc(&base_weight);
-	let weight = default_transfer_call().get_dispatch_info().weight;
-	let weight_fee = <Runtime as pallet_transaction_payment::Trait>::WeightToFee::calc(&weight);
-
-	base_fee + fee_multiplier.saturating_mul_acc_int(length_fee + weight_fee)
+/// Default transfer fee. This will use the same logic that is implemented in transaction-payment module.
+///
+/// Note that reads the multiplier from storage directly, hence to get the fee of `extrinsic`
+/// at block `n`, it must be called prior to executing block `n` to do the calculation with the
+/// correct multiplier.
+fn transfer_fee<E: Encode>(extrinsic: &E) -> Balance {
+	TransactionPayment::compute_fee(
+		extrinsic.encode().len() as u32,
+		&default_transfer_call().get_dispatch_info(),
+		0,
+	)
 }
 
 fn xt() -> UncheckedExtrinsic {
@@ -242,7 +240,7 @@ fn successful_execution_with_native_equivalent_code_gives_ok() {
 	).0;
 	assert!(r.is_ok());
 
-	let fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	let fees = t.execute_with(|| transfer_fee(&xt()));
 
 	let r = executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -254,7 +252,6 @@ fn successful_execution_with_native_equivalent_code_gives_ok() {
 	assert!(r.is_ok());
 
 	t.execute_with(|| {
-		let fees = transfer_fee(&xt(), fm);
 		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
 		assert_eq!(Balances::total_balance(&bob()), 69 * DOLLARS);
 	});
@@ -286,7 +283,7 @@ fn successful_execution_with_foreign_code_gives_ok() {
 	).0;
 	assert!(r.is_ok());
 
-	let fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	let fees = t.execute_with(|| transfer_fee(&xt()));
 
 	let r = executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -298,7 +295,6 @@ fn successful_execution_with_foreign_code_gives_ok() {
 	assert!(r.is_ok());
 
 	t.execute_with(|| {
-		let fees = transfer_fee(&xt(), fm);
 		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
 		assert_eq!(Balances::total_balance(&bob()), 69 * DOLLARS);
 	});
@@ -311,7 +307,7 @@ fn full_native_block_import_works() {
 	let (block1, block2) = blocks();
 
 	let mut alice_last_known_balance: Balance = Default::default();
-	let mut fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	let mut fees = t.execute_with(|| transfer_fee(&xt()));
 
 	executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -322,7 +318,6 @@ fn full_native_block_import_works() {
 	).0.unwrap();
 
 	t.execute_with(|| {
-		let fees = transfer_fee(&xt(), fm);
 		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
 		assert_eq!(Balances::total_balance(&bob()), 169 * DOLLARS);
 		alice_last_known_balance = Balances::total_balance(&alice());
@@ -361,7 +356,7 @@ fn full_native_block_import_works() {
 		assert_eq!(System::events(), events);
 	});
 
-	fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	fees = t.execute_with(|| transfer_fee(&xt()));
 
 	executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -372,7 +367,6 @@ fn full_native_block_import_works() {
 	).0.unwrap();
 
 	t.execute_with(|| {
-		let fees = transfer_fee(&xt(), fm);
 		assert_eq!(
 			Balances::total_balance(&alice()),
 			alice_last_known_balance - 10 * DOLLARS - fees,
@@ -450,7 +444,7 @@ fn full_wasm_block_import_works() {
 	let (block1, block2) = blocks();
 
 	let mut alice_last_known_balance: Balance = Default::default();
-	let mut fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	let mut fees = t.execute_with(|| transfer_fee(&xt()));
 
 	executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -461,12 +455,12 @@ fn full_wasm_block_import_works() {
 	).0.unwrap();
 
 	t.execute_with(|| {
-		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - transfer_fee(&xt(), fm));
+		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
 		assert_eq!(Balances::total_balance(&bob()), 169 * DOLLARS);
 		alice_last_known_balance = Balances::total_balance(&alice());
 	});
 
-	fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	fees = t.execute_with(|| transfer_fee(&xt()));
 
 	executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -479,11 +473,11 @@ fn full_wasm_block_import_works() {
 	t.execute_with(|| {
 		assert_eq!(
 			Balances::total_balance(&alice()),
-			alice_last_known_balance - 10 * DOLLARS - transfer_fee(&xt(), fm),
+			alice_last_known_balance - 10 * DOLLARS - fees,
 		);
 		assert_eq!(
 			Balances::total_balance(&bob()),
-			179 * DOLLARS - 1 * transfer_fee(&xt(), fm),
+			179 * DOLLARS - 1 * fees,
 		);
 	});
 }
@@ -755,7 +749,7 @@ fn successful_execution_gives_ok() {
 		assert_eq!(Balances::total_balance(&alice()), 111 * DOLLARS);
 	});
 
-	let fm = t.execute_with(TransactionPayment::next_fee_multiplier);
+	let fees = t.execute_with(|| transfer_fee(&xt()));
 
 	let r = executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
@@ -770,7 +764,6 @@ fn successful_execution_gives_ok() {
 		.expect("Extrinsic failed");
 
 	t.execute_with(|| {
-		let fees = transfer_fee(&xt(), fm);
 		assert_eq!(Balances::total_balance(&alice()), 42 * DOLLARS - fees);
 		assert_eq!(Balances::total_balance(&bob()), 69 * DOLLARS);
 	});

bin/node/executor/tests/fees.rs

@@ -22,9 +22,9 @@ use frame_support::{
 	weights::{GetDispatchInfo, constants::ExtrinsicBaseWeight, IdentityFee, WeightToFeePolynomial},
 };
 use sp_core::NeverNativeValue;
-use sp_runtime::{FixedPointNumber, FixedI128, Perbill};
+use sp_runtime::{Perbill, FixedPointNumber};
 use node_runtime::{
-	CheckedExtrinsic, Call, Runtime, Balances, TransactionPayment,
+	CheckedExtrinsic, Call, Runtime, Balances, TransactionPayment, Multiplier,
 	TransactionByteFee,
 	constants::currency::*,
 };
@@ -38,8 +38,8 @@ use self::common::{*, sign};
 fn fee_multiplier_increases_and_decreases_on_big_weight() {
 	let mut t = new_test_ext(COMPACT_CODE, false);
 
-	// initial fee multiplier must be zero
-	let mut prev_multiplier = FixedI128::from_inner(0);
+	// initial fee multiplier must be one.
+	let mut prev_multiplier = Multiplier::one();
 
 	t.execute_with(|| {
 		assert_eq!(TransactionPayment::next_fee_multiplier(), prev_multiplier);
@@ -59,7 +59,7 @@ fn fee_multiplier_increases_and_decreases_on_big_weight() {
 			},
 			CheckedExtrinsic {
 				signed: Some((charlie(), signed_extra(0, 0))),
-				function: Call::System(frame_system::Call::fill_block(Perbill::from_percent(90))),
+				function: Call::System(frame_system::Call::fill_block(Perbill::from_percent(60))),
 			}
 		]
 	);
@@ -122,7 +122,7 @@ fn fee_multiplier_increases_and_decreases_on_big_weight() {
 }
 
 #[test]
-fn transaction_fee_is_correct_ultimate() {
+fn transaction_fee_is_correct() {
 	// This uses the exact values of substrate-node.
 	//
 	// weight of transfer call as of now: 1_000_000