Skip to content

Commit

Permalink
Fix the broken weight multiplier update function (paritytech#6334)
Browse files Browse the repository at this point in the history 
* 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>
  • Loading branch information
@kianenigma @apopiak
kianenigma and apopiak authored Jun 17, 2020
1 parent 17be6fd commit 0c42ced
Show file tree
Hide file tree
Showing 10 changed files with 456 additions and 251 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

55 changes: 24 additions & 31 deletions bin/node/executor/tests/basic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -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};
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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);
});
Expand Down Expand Up @@ -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,
Expand All @@ -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);
});
Expand All @@ -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,
Expand All @@ -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());
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
);
});
}
Expand Down Expand Up @@ -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,
Expand All @@ -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);
});
Expand Down
12 changes: 6 additions & 6 deletions bin/node/executor/tests/fees.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::*,
};
Expand All @@ -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);
Expand All @@ -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))),
}
]
);
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 0c42ced

Please sign in to comment.