Skip to content

Conversation

@notlesh
Copy link
Contributor

@notlesh notlesh commented May 12, 2022

What does it do?

Introduces a non-linear LengthToFee transaction length fee (only for Substrate-based transactions -- does not impact Ethereum-based ones). This replaces #1353, and like it will need to be reconciled with #1218.

This currently has two components to the length fee:

  • a proportional one (1byte = 1X)
  • a cubic one (1byte = 1X**3)

The result is that the proportional fee dominates from [1byte .. 1000bytes] after which the cubic portion grows very rapidly. The goal is to cause fees to become untenable quickly into the 1MB+ range.

See the rust test assertions for a table-ish of results.

I haven't done much research into what range of txn sizes we typically see in the real world. Ideally, these would be nearly unaffected by the exponential byte fee portion. Furthermore, we need to make some assumptions about runtime upgrades, as these are the only (?) legitimate large transactions. It is acceptable that these be expensive, but (1) they can't be "too" expensive and (2) their exception should not prevent a serious determent to much larger txns.

Another note: Ethereum transactions have a maximum size (implied from the max block size divided by their own per-byte fee). The Ethereum per-byte fee is adjusted by gas price while the LengthToFee from pallet-transaction-payment is unadjusted.

Related: paritytech/substrate#10785

@notlesh notlesh added B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 12, 2022
Comment on lines +2336 to +2348
// left: cost of length fee, right: size in bytes
// /------------- proportional component: O(N * 1B)
// | /- exponential component: O(N ** 3)
// | |
assert_eq!( 1_000_000_001, calc_fee(1));
assert_eq!( 10_000_001_000, calc_fee(10));
assert_eq!( 100_001_000_000, calc_fee(100));
assert_eq!( 1_001_000_000_000, calc_fee(1_000));
assert_eq!( 11_000_000_000_000, calc_fee(10_000)); // inflection point
assert_eq!( 1_100_000_000_000_000, calc_fee(100_000));
assert_eq!( 1_001_000_000_000_000_000, calc_fee(1_000_000)); // one UNIT, ~ 1MB
assert_eq!( 1_000_010_000_000_000_000_000, calc_fee(10_000_000));
assert_eq!(1_000_000_100_000_000_000_000_000, calc_fee(100_000_000));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the initial results

@notlesh notlesh requested a review from crystalin May 16, 2022 14:57
expect(
((await context.polkadotApi.query.system.account(BALTATHAR)) as any).data.free.toBigInt()
).to.eq(1208925818354628766561176n);
).to.eq(1208925819614502764560800n);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should have dissapeared if you bring master

@girazoki
Copy link
Collaborator

I dont fully understand the concern with runtime upgrades. By looking at https://github.com/paritytech/cumulus/blob/f9bc77c3c801973b4b6943cb6f236fb3934eb993/pallets/parachain-system/src/lib.rs#L406 it seems to me as if this extrinsic does not lead to fee payment. Therefore the LengthToFee implementation should not affect, am I right?

@notlesh notlesh merged commit ce40228 into master May 23, 2022
@notlesh notlesh deleted the notlesh-length-fee-polynomial-2 branch May 23, 2022 21:21
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants