Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use transaction-payment's congestion modifier for Ethereum base-fee #1765

Merged
merged 82 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
82 commits
Select commit Hold shift + click to select a range
a0fc51b
Convert transaction-payments congestion modifier to replace Ethereum'…
notlesh Aug 19, 2022
52bf3f8
Remove outdated comment
notlesh Aug 19, 2022
a182d56
fix precision bug
nbaztec Sep 2, 2022
2bdfb9e
import
nbaztec Sep 2, 2022
b3e84c4
use saturating mul, improve doc
nbaztec Sep 5, 2022
3af5c6c
update fee parameters, add tests
nbaztec Sep 9, 2022
94e6e0e
make tests similar
nbaztec Sep 9, 2022
a83778d
add tests to all runtimes
nbaztec Sep 9, 2022
cac3a2e
fix tests
nbaztec Sep 9, 2022
167d566
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
notlesh Oct 6, 2022
4b58131
Add transaction-payment GenesisConfig to initialize its Multiplier
notlesh Oct 6, 2022
66a1d9b
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
notlesh Oct 10, 2022
86e6d41
Fix some moonbase tests
notlesh Oct 10, 2022
e55c392
Rename FixedGasPrice -> TransactionPaymentAsGasPrice
notlesh Oct 10, 2022
ccce143
fmt
notlesh Oct 10, 2022
751bede
More ts test fixes
notlesh Oct 10, 2022
55058d5
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
Oct 10, 2022
25fdd29
Fixes staking locks
Oct 11, 2022
928a03e
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
Oct 11, 2022
4675940
Fix existential balance tests
notlesh Oct 11, 2022
6382888
Fix length fee tests
notlesh Oct 11, 2022
c29b28e
Merge branch 'notlesh-use-transaction-payment-for-base-fee' of github…
Oct 11, 2022
da46a52
Query base_fee in createTransaction when needed
notlesh Oct 11, 2022
5d403fb
Fixes gasPrice for existential test
Oct 11, 2022
3240bcd
Merge branch 'notlesh-use-transaction-payment-for-base-fee' of github…
Oct 11, 2022
dd71c93
Fix deposit/fee check for multiple deposit
Oct 11, 2022
efa30ac
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
notlesh Oct 27, 2022
e97ed70
Fix merge issue
notlesh Oct 27, 2022
4648b30
Use legacy txns for some tests that expect full gas_price to be charged
notlesh Nov 2, 2022
90a7029
Bump gas price for tests
notlesh Nov 2, 2022
8fdc4b1
Update tests to reflect createTransfer default
notlesh Nov 2, 2022
6a8f12c
Use constant for DEFAULT_TXN_MAX_BASE_FEE
notlesh Nov 3, 2022
840f0f3
Reflect txn values to reflect base_fee change
notlesh Nov 3, 2022
78b3dfb
Prefer constant over literal value
notlesh Nov 3, 2022
3867c06
Overhaul fee calculations in verifyBlockFees
notlesh Nov 4, 2022
9b45a6a
Bound effectiveTipPerGas to 0
notlesh Nov 7, 2022
0b3b938
Fix substrate-based fees
notlesh Nov 7, 2022
6291c7d
Overestimate delegation count weight hint
notlesh Nov 9, 2022
50c275d
Use legacy txns and expect full gas_price to be paid as fee
notlesh Nov 9, 2022
ab6a537
Use constant for gas limit value
notlesh Nov 9, 2022
43d9c97
Start test case for max possible fee conditions
notlesh Nov 9, 2022
029f83e
Clean up
notlesh Nov 9, 2022
a5eb9d3
Add runtime-upgrade test
notlesh Nov 9, 2022
62c355a
prettier
notlesh Nov 9, 2022
a6b186e
First look at max possible fee
notlesh Nov 9, 2022
15271c1
fix auto-compound delegation tx order flakiness
nbaztec Nov 10, 2022
0e6a7f4
prettier
notlesh Nov 10, 2022
05313b3
Remove cargo override (oops)
notlesh Nov 10, 2022
174aad7
Hack to fix race condition
notlesh Nov 10, 2022
4cfd346
prettier
notlesh Nov 10, 2022
240efa8
Use beforeEach for setting max multiplier
notlesh Nov 10, 2022
a80e059
Test multiplier against Fibonacci contract
notlesh Nov 14, 2022
4d4f14d
prettier
notlesh Nov 14, 2022
9456ed9
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
notlesh Nov 28, 2022
bafec82
prettier
notlesh Nov 28, 2022
284ece4
fmt
notlesh Nov 28, 2022
6d3cd9f
Various minor fixes
notlesh Nov 28, 2022
c95da99
Add some fee multiplier scenarios
notlesh Dec 12, 2022
683feff
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
notlesh Dec 13, 2022
ea1c5b3
Don't use EXTRINSIC_BASE_WEIGHT in gas calc
notlesh Dec 13, 2022
a847654
Don't expect genesis value at each beforeAll
notlesh Dec 13, 2022
98d7374
Bump expectation of CREATE cost
notlesh Dec 13, 2022
9da3ce9
Slightly change length fee assumptions
notlesh Dec 13, 2022
3d0ecb6
Use higher gas price
notlesh Dec 13, 2022
0a9f1be
Remove base_fee test
notlesh Dec 13, 2022
370c19f
fmt
notlesh Dec 13, 2022
1c2c5ec
toml sort
notlesh Dec 13, 2022
5bb3b49
Resolve compiler warnings
notlesh Dec 13, 2022
5f2511d
More compiler warning fixes
notlesh Dec 13, 2022
fc0ab3d
Fix receipt/status test
tgmichel Dec 14, 2022
86978cf
Remove irrelevant tests
notlesh Dec 14, 2022
84b9f5d
Use maxDelegationCount for weight hint
notlesh Dec 14, 2022
0c3866e
Remove comment
notlesh Jan 6, 2023
d56ea3f
Remove ignored tests
notlesh Jan 6, 2023
14f9b7a
Move fee tests to integration_tests.rs
notlesh Jan 8, 2023
5988fb2
Merge branch 'master' into notlesh-use-transaction-payment-for-base-fee
notlesh Jan 8, 2023
0890e92
Re-remove ignored test
notlesh Jan 8, 2023
160f82f
Move moonbeam runtime fee tests to integration_tests
notlesh Jan 8, 2023
23c9942
Move moonriver runtime fee tests to integration_tests
notlesh Jan 8, 2023
d84a467
fmt
notlesh Jan 8, 2023
16fa4e8
Use base fee constant for gas price
notlesh Jan 8, 2023
dd5b4cf
Revert test name change...
notlesh Jan 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions node/service/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ moonriver-runtime = { path = "../../runtime/moonriver", optional = true }

# Substrate
frame-system-rpc-runtime-api = { git = "https://github.com/purestake/substrate", branch = "moonbeam-polkadot-v0.9.32" }
pallet-transaction-payment = { git = "https://github.com/purestake/substrate", branch = "moonbeam-polkadot-v0.9.32" }
pallet-transaction-payment-rpc = { git = "https://github.com/purestake/substrate", branch = "moonbeam-polkadot-v0.9.32" }
pallet-transaction-payment-rpc-runtime-api = { git = "https://github.com/purestake/substrate", branch = "moonbeam-polkadot-v0.9.32" }
parity-scale-codec = "3.0.0"
Expand Down
6 changes: 5 additions & 1 deletion node/service/src/chain_spec/moonbase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ use moonbase_runtime::{
EthereumChainIdConfig, EthereumConfig, GenesisAccount, GenesisConfig, InflationInfo,
MaintenanceModeConfig, ParachainInfoConfig, ParachainStakingConfig, PolkadotXcmConfig,
Precompiles, Range, SudoConfig, SystemConfig, TechCommitteeCollectiveConfig,
TreasuryCouncilCollectiveConfig, HOURS, WASM_BINARY,
TransactionPaymentConfig, TreasuryCouncilCollectiveConfig, HOURS, WASM_BINARY,
};
use nimbus_primitives::NimbusId;
use pallet_transaction_payment::Multiplier;
use sc_service::ChainType;
#[cfg(test)]
use sp_core::ecdsa;
Expand Down Expand Up @@ -321,6 +322,9 @@ pub fn testnet_genesis(
},
// This should initialize it to whatever we have set in the pallet
polkadot_xcm: PolkadotXcmConfig::default(),
transaction_payment: TransactionPaymentConfig {
multiplier: Multiplier::from(8u128),
},
}
}

Expand Down
6 changes: 5 additions & 1 deletion node/service/src/chain_spec/moonbeam.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ use moonbeam_runtime::{
Balance, BalancesConfig, CouncilCollectiveConfig, CrowdloanRewardsConfig, DemocracyConfig,
EVMConfig, EthereumChainIdConfig, EthereumConfig, GenesisAccount, GenesisConfig, InflationInfo,
MaintenanceModeConfig, ParachainInfoConfig, ParachainStakingConfig, PolkadotXcmConfig,
Precompiles, Range, SystemConfig, TechCommitteeCollectiveConfig,
Precompiles, Range, SystemConfig, TechCommitteeCollectiveConfig, TransactionPaymentConfig,
TreasuryCouncilCollectiveConfig, HOURS, WASM_BINARY,
};
use nimbus_primitives::NimbusId;
use pallet_transaction_payment::Multiplier;
use sc_service::ChainType;
#[cfg(test)]
use sp_core::ecdsa;
Expand Down Expand Up @@ -313,6 +314,9 @@ pub fn testnet_genesis(
},
// This should initialize it to whatever we have set in the pallet
polkadot_xcm: PolkadotXcmConfig::default(),
transaction_payment: TransactionPaymentConfig {
multiplier: Multiplier::from(8u128),
},
}
}

Expand Down
6 changes: 5 additions & 1 deletion node/service/src/chain_spec/moonriver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ use moonriver_runtime::{
CouncilCollectiveConfig, CrowdloanRewardsConfig, DemocracyConfig, EVMConfig,
EthereumChainIdConfig, EthereumConfig, GenesisAccount, GenesisConfig, InflationInfo,
MaintenanceModeConfig, ParachainInfoConfig, ParachainStakingConfig, PolkadotXcmConfig,
Precompiles, Range, SystemConfig, TechCommitteeCollectiveConfig,
Precompiles, Range, SystemConfig, TechCommitteeCollectiveConfig, TransactionPaymentConfig,
TreasuryCouncilCollectiveConfig, HOURS, WASM_BINARY,
};
use nimbus_primitives::NimbusId;
use pallet_transaction_payment::Multiplier;
use sc_service::ChainType;
#[cfg(test)]
use sp_core::ecdsa;
Expand Down Expand Up @@ -313,6 +314,9 @@ pub fn testnet_genesis(
},
// This should initialize it to whatever we have set in the pallet
polkadot_xcm: PolkadotXcmConfig::default(),
transaction_payment: TransactionPaymentConfig {
multiplier: Multiplier::from(8u128),
},
}
}

Expand Down
37 changes: 23 additions & 14 deletions runtime/moonbase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ pub fn native_version() -> NativeVersion {
}

const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75);
const NORMAL_WEIGHT: Weight = MAXIMUM_BLOCK_WEIGHT.saturating_mul(3).saturating_div(4);
pub const NORMAL_WEIGHT: Weight = MAXIMUM_BLOCK_WEIGHT.saturating_mul(3).saturating_div(4);
// Here we assume Ethereum's base fee of 21000 gas and convert to weight, but we
// subtract roughly the cost of a balance transfer from it (about 1/3 the cost)
// and some cost to account for per-byte-fee.
Expand Down Expand Up @@ -388,27 +388,37 @@ parameter_types! {
pub const TargetBlockFullness: Perquintill = Perquintill::from_percent(25);
/// The adjustment variable of the runtime. Higher values will cause `TargetBlockFullness` to
/// change the fees more rapidly. This low value causes changes to occur slowly over time.
pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(3, 100_000);
pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(4, 1_000);
/// Minimum amount of the multiplier. This value cannot be too low. A test case should ensure
/// that combined with `AdjustmentVariable`, we can recover from the minimum.
/// See `multiplier_can_grow_from_zero` in integration_tests.rs.
/// This value is currently only used by pallet-transaction-payment as an assertion that the
/// next multiplier is always > min value.
pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000u128);
pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000);
/// Maximum multiplier. We pick a value that is expensive but not impossibly so; it should act
/// as a safety net.
pub MaximumMultiplier: Multiplier = Multiplier::from(100_000u128);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add a migration to initialize the FeeMultiplier amount? Today it seems to be worth 1000000000000 on moonbase, moonriver in moonbeam, which is not in the expected range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I've explored this pretty thoroughly, here's my summary:

  • For new blockchains, the genesis value will kick in (this PR made that possible)
  • For existing blockchains, the minimum and maximum multipliers in transaction-payment will kick in immediately at the end of the runtime upgrade block

A migration could force the multiplier bounds to be enforced earlier (before extrinsics are charged fees for the runtime upgrade block), but this doesn't seem like a big deal to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate:

In transaction-payment's on_finalize(), the TargetedFeeAdjustment::convert() will check the new min and max bounds and enforce them. This value will then be updated in pallet storage and will cause the next block to have fees within bounds.

pub PrecompilesValue: MoonbasePrecompiles<Runtime> = MoonbasePrecompiles::<_>::new();
pub WeightPerGas: Weight = Weight::from_ref_time(WEIGHT_PER_GAS);
}

pub struct FixedGasPrice;
impl FeeCalculator for FixedGasPrice {
pub struct TransactionPaymentAsGasPrice;
impl FeeCalculator for TransactionPaymentAsGasPrice {
fn min_gas_price() -> (U256, Weight) {
(
(1 * currency::GIGAWEI * currency::SUPPLY_FACTOR).into(),
Weight::zero(),
)
// TODO: transaction-payment differs from EIP-1559 in that its tip and length fees are not
// scaled by the multiplier, which means its multiplier will be overstated when
// applied to an ethereum transaction
// note: transaction-payment uses both a congestion modifier (next_fee_multiplier, which is
// updated once per block in on_finalize) and a 'WeightToFee' implementation. Our
// runtime implements this as a 'ConstantModifier', so we can get away with a simple
// multiplication here.
// It is imperative that `saturating_mul_int` be performed as late as possible in the
// expression since it involves fixed point multiplication with a division by a fixed
// divisor. This leads to truncation and subsequent precision loss if performed too early.
// This can lead to min_gas_price being same across blocks even if the multiplier changes.
// There's still some precision loss when the final `gas_price` (used_gas * min_gas_price)
// is computed in frontier, but that's currently unavoidable.
let min_gas_price = TransactionPayment::next_fee_multiplier()
.saturating_mul_int(currency::WEIGHT_FEE.saturating_mul(WEIGHT_PER_GAS as u128));
(min_gas_price.into(), Weight::zero())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that we read TransactionPayment::next_fee_multiplier() but we still account zero weight for the min_gas_price call. Is next_fee_multiplier expected to be cached and the weight in the min_gas_price call context to be negligible? Or what is the reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should be more than 0, will PR a fix soon :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notlesh Think I vaguely recall us discussing this and you mentioned that since this is read way too often, we could offer this for free (since it's cached more or less). Not sure if that condition still applied.

}
}

Expand Down Expand Up @@ -451,7 +461,7 @@ where
moonbeam_runtime_common::impl_on_charge_evm_transaction!();

impl pallet_evm::Config for Runtime {
type FeeCalculator = FixedGasPrice;
type FeeCalculator = TransactionPaymentAsGasPrice;
type GasWeightMapping = pallet_evm::FixedGasWeightMapping<Self>;
type WeightPerGas = WeightPerGas;
type BlockHashMapping = pallet_ethereum::EthereumBlockHashMapping<Self>;
Expand Down Expand Up @@ -1131,7 +1141,7 @@ construct_runtime! {
Sudo: pallet_sudo::{Pallet, Call, Config<T>, Storage, Event<T>} = 4,
RandomnessCollectiveFlip: pallet_randomness_collective_flip::{Pallet, Storage} = 5,
ParachainSystem: cumulus_pallet_parachain_system::{Pallet, Call, Storage, Inherent, Event<T>} = 6,
TransactionPayment: pallet_transaction_payment::{Pallet, Storage, Event<T>} = 7,
TransactionPayment: pallet_transaction_payment::{Pallet, Storage, Config, Event<T>} = 7,
ParachainInfo: parachain_info::{Pallet, Storage, Config} = 8,
EthereumChainId: pallet_ethereum_chain_id::{Pallet, Storage, Config} = 9,
EVM: pallet_evm::{Pallet, Config, Call, Storage, Event<T>} = 10,
Expand Down Expand Up @@ -1389,7 +1399,6 @@ mod tests {
5_u8
);
assert_eq!(STORAGE_BYTE_FEE, Balance::from(100 * MICROUNIT));
assert_eq!(FixedGasPrice::min_gas_price().0, (1 * GIGAWEI).into());

// democracy minimums
assert_eq!(
Expand Down
21 changes: 15 additions & 6 deletions runtime/moonbase/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ use moonbase_runtime::{asset_config::AssetRegistrarMetadata, xcm_config::AssetTy
pub use moonbase_runtime::{
currency::{GIGAWEI, SUPPLY_FACTOR, UNIT, WEI},
AccountId, AssetId, AssetManager, Assets, AuthorInherent, Balance, Balances, CrowdloanRewards,
Ethereum, Executive, FixedGasPrice, InflationInfo, LocalAssets, ParachainStaking, Range,
Runtime, RuntimeCall, RuntimeEvent, System, TransactionConverter, UncheckedExtrinsic, HOURS,
WEEKS,
Ethereum, Executive, InflationInfo, LocalAssets, ParachainStaking, Range, Runtime, RuntimeCall,
RuntimeEvent, System, TransactionConverter, TransactionPaymentAsGasPrice, UncheckedExtrinsic,
HOURS, WEEKS,
};
use nimbus_primitives::{NimbusId, NIMBUS_ENGINE_ID};
use sp_core::{Encode, H160};
Expand All @@ -38,12 +38,13 @@ use sp_runtime::{Digest, DigestItem, Perbill, Percent};
use std::collections::BTreeMap;

use fp_rpc::ConvertTransaction;
use pallet_transaction_payment::Multiplier;

// A valid signed Alice transfer.
pub const VALID_ETH_TX: &str =
"f86880843b9aca0083b71b0094111111111111111111111111111111111111111182020080820a26a\
08c69faf613b9f72dbb029bb5d5acf42742d214c79743507e75fdc8adecdee928a001be4f58ff278ac\
61125a81a582a717d9c5d6554326c01b878297c6522b12282";
"02f86d8205018085174876e80085e8d4a5100082520894f24ff3a9cf04c71dbc94d0b566f7a27b9456\
6cac8080c001a0e1094e1a52520a75c0255db96132076dd0f1263089f838bea548cbdbfc64a4d19f031c\
92a8cb04e2d68d20a6158d542a07ac440cc8d07b6e36af02db046d92df";

// An invalid signed Alice transfer with a gas limit artifically set to 0.
pub const INVALID_ETH_TX: &str =
Expand Down Expand Up @@ -291,6 +292,14 @@ impl ExtBuilder {
)
.unwrap();

<pallet_transaction_payment::GenesisConfig as GenesisBuild<Runtime>>::assimilate_storage(
&pallet_transaction_payment::GenesisConfig {
multiplier: Multiplier::from(8u128),
},
&mut t,
)
.unwrap();

let mut ext = sp_io::TestExternalities::new(t);

let local_assets = self.local_assets.clone();
Expand Down
Loading