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

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Aug 19, 2022

What does it do?

This PR makes several significant fee modifications:

  • It replaces the fixed Ethereum fee multiplier with a dynamic one. Currently we ignore the max_fee_per_gas and replace it with a fixed 1 Gwei.
  • Unifies the multiplier for both Substrate and Ethereum transactions by deriving both of them from pallet transaction-payment's multiplier. While the multiplier has a slightly different impact on the two fee mechanisms, the trend is very similar.
    • Note that this fee mechanism has a similar response to EIP-1559 in that it goes up during congestion and down during light blocks.
  • Tweaks several parameters of the transaction-payment multiplier algorithm so that it should (1) remain within a useful range and (2) update quickly (tuned roughly to double within one hour when blocks are completely full).
    • In particular, we bound the fee mechanism to min 0.1 and max 100_000.

⚠️ Breaking Changes ⚠️

  • All changes are moonbase only
  • Fixed 1 Gwei gas price (base_fee) removed in favor of pallet-transaction-payment's congestion-based multiplier (acts similarly to EIP-1559)
  • Min and max multiplier enforced (in terms of base-fee, this is 125_000_000 and 125_000_000_000_000 respectively)
  • The same multiplier is used for Substrate-based transactions. While the multiplier scales differently with the two txn types, the trend should be similar.

Notes on compatibility with EIP-1559:

  • This causes us to intentionally deviate from EIP-1559. While our fee mechanism was never compatible and wasn't expected to be strictly compatible, this is a conscious deviation from its entire fee update mechanism.
  • Our transaction-fee calculation is now compatible with EIP-1559. Specifically, given a known base-fee and a txn max_fee_per_gas and max_priority_fee_per_gas, we will deduct the same fee as Ethereum will under EIP-1559.
  • The destination of these fees still differs (and is unchanged in this PR). Under EIP-1559, base_fee is burned and any applicable priority_fee is paid to miners. In Moonbeam we still burn 80% and pay 20% to the treasury for both fees. No part of fees are paid to block authors currently.
  • The adjustment of base_fee is very different than EIP-1559. The specifics of the algorithm can be found in pallet transaction-payment's documentation.

Notes on how this multiplier impacts Substrate and Ethereum fees differently:

  • Ethereum fees are more heavily impacted by the multiplier than Substrate ones. The effect of changes to the multiplier will be amplified for Ethereum transactions. This is because the multiplier applies to the full Ethereum gas_used whereas it applies only to the weight portion of Substrate fees.
  • Ethereum fees scale linearly with no base. The multiplier has a lower bound above 0, but if it were to somehow be set to 0, Ethereum would exhibit 0-priced fees (gas_used * 0, basically).
  • Substrate has several fee mechanisms that are unaffected by the multiplier. So some transactions will seem to be impacted more than others. Specifically:
    • base_fee that converts to a flat currency fee
    • length_fee
    • tip
    • (read more here)

Analysis

image
image

The fees converge as the network becomes more congested (logarithmic scale). If the network remains empty, then the fees are also held at a constant value.

The following table exhibits the effect of the multiplier starting from 0.000003 (currently in moonriver) to increasing magnitudes. We see that the substrate and evm fees converge on higher multiplier values but substrate fees will always be higher than evm fees by a factor of base + length fees.

image

Multiplier                      Fees
------------------------------------
  0.000003        12,746,162,435,424 | substrate
                         147,518,810 | evm
                  12,746,014,916,614 | diff
                              199.9% | diff%
                         147,548,488 | substrate-weight-fee-part
                  12,500,000,000,000 | substrate-base-fee-part
------------------------------------ | 
     0.001        12,783,532,386,936 | substrate
                      37,517,500,000 | evm
                  12,746,014,886,936 | diff
                              198.8% | diff%
                      37,517,500,000 | substrate-weight-fee-part
                  12,500,000,000,000 | substrate-base-fee-part
------------------------------------ | 
       0.1        16,497,764,886,936 | substrate
                   3,751,750,000,000 | evm
                  12,746,014,886,936 | diff
                              125.8% | diff%
                   3,751,750,000,000 | substrate-weight-fee-part
                  12,500,000,000,000 | substrate-base-fee-part
------------------------------------ | 
         1        50,226,016,145,686 | substrate
                  37,480,001,258,750 | evm
                  12,746,014,886,936 | diff
                                 29% | diff%
                  37,480,001,258,750 | substrate-weight-fee-part
                  12,500,000,000,000 | substrate-base-fee-part
------------------------------------ | 
        10       387,546,027,474,436 | substrate
                 374,800,012,587,500 | evm
                  12,746,014,886,936 | diff
                                3.3% | diff%
                 374,800,012,587,500 | substrate-weight-fee-part
                  12,500,000,000,000 | substrate-base-fee-part
------------------------------------ | 
       100     3,760,746,140,761,936 | substrate
               3,748,000,125,875,000 | evm
                  12,746,014,886,936 | diff
                                0.3% | diff%
               3,748,000,125,875,000 | substrate-weight-fee-part
                  12,500,000,000,000 | substrate-base-fee-part

From the above table the range of 1.0 - 200.0 seems good so far.

TODO

  • Find solution for no GenesisConfig in transaction-payment
  • Fix existing test cases
  • Tune values to work for various transaction types
  • New tests
  • Document breaking changes

@nbaztec nbaztec added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Sep 2, 2022
runtime/moonbase/src/lib.rs Outdated Show resolved Hide resolved
runtime/moonbase/src/lib.rs Outdated Show resolved Hide resolved
@boundless-forest
Copy link
Contributor

The NextFeeMultiplier is much similar to the base-fee pallet but on the substrate side. Why not use base-fee pallet here?

@nbaztec
Copy link
Contributor

nbaztec commented Sep 9, 2022

The NextFeeMultiplier is much similar to the base-fee pallet but on the substrate side. Why not use base-fee pallet here?

@AsceticBear The initial reason being having to maintain/re-implement less code/functionality which is quite easy to get wrong (e.g. fees will never recover or continuously drift in a single direction).
The original motivation we have here is to adapt the EVM fees to the changing network traffic and substrate already provides us with a well-tested mechanism in place - this also means the network fee would be more in tandem between Substrate and EVM.

@notlesh
Copy link
Contributor Author

notlesh commented Sep 13, 2022

The NextFeeMultiplier is much similar to the base-fee pallet but on the substrate side. Why not use base-fee pallet here?

A bit more detail:

I explored using this and came to the conclusion that EIP1559's design isn't well suited for a network that doesn't experience consistent congestion.

Basically, I think the original design creates a feedback mechanism that keeps the congestion modifier within a reasonable range, something like this:

congestion up -> fees up -> fewer txns eligible -> blocks less full -> congestion down

and

congestion down -> fees down -> more txns eligible -> blocks more full -> congestion up

Without this, it becomes critical to tune pallet base-fee's Threshold parameters because incorrect tuning will create a permanent upward or downward trend. The pallet has no bounds on the fee itself (nor does EIP1559) which can eventually result in fees being either near-zero or impossibly expensive.

While pallet transaction-payment has a similar mechanism, it has far more knobs to tune (including a minimum fee) to alleviate this.

@notlesh notlesh mentioned this pull request Sep 13, 2022
3 tasks
@librelois librelois mentioned this pull request Oct 10, 2022
24 tasks
@notlesh notlesh added the breaking Needs to be mentioned in breaking changes label Jan 8, 2023
/// 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.

@notlesh notlesh requested a review from nbaztec January 10, 2023 16:07
Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

Small questions but overall looks good

@@ -36,7 +36,7 @@ describeDevMoonbeam(
(context) => {
it("should have low balance transfer fees", async () => {
const fee = await testBalanceTransfer(context);
expect(fee).to.equal(20958001520875n);
expect(fee).to.equal(79_359_001_520_875n);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to fetch this through a runtime API? I feel hardcoding this value is prone to having to change it every time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to avoid hard coding fee values everywhere, they break nearly all the time.

We have verifyBlockFees, which could be suitable here. Note that it calculates the fees by hand, as opposed to asking the node/runtime to verify it, which I think is important for some of these tests.

Maybe we need an equivalent for a single extrinsic...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought there was a payment info runtime API that could return the expected fee, is that not suitable for this?

tests/tests/test-precompile/test-precompile-xtokens.ts Outdated Show resolved Hide resolved
@@ -158,7 +159,7 @@ describeDevMoonbeamAllEthTxTypes("Precompiles - xtokens", (context) => {
});
});

describeDevMoonbeamAllEthTxTypes("Precompiles - xtokens", (context) => {
describeDevMoonbeam("Precompiles - xtokens", (context) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentional because it takes EIP-1559 by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually takes legacy by default: https://github.com/PureStake/moonbeam/blob/24fddc3701e3e40c2f8febcc4ba1f0af7a49e144/tests/util/setup-dev-tests.ts#L81

I found this a bit surprising, but maybe it make a lot of sense to do it that way when we first introduced EIP1559.

Anyway, IIRC, the reason for this change is because the test uses the TRANSACTION_TEMPLATE which uses gasPrice instead of maxFeePerGas/maxPriorityFeePerGas. I'll rerun the test and see if I can figure out why exactly this was a problem.

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, so the problem is the way createTransaction handles maxPriorityFeePerGas, which I think could be improved (I'll open an internal issue to track that).

Basically, in the case of (1) EIP-1559 txn with (2) gasPrice provided but no maxFeePerGas, it will specify maxPriorityFeePerGas: 0, which leads to a discrepancy in the exact fees charged between txn types:

export const createTransaction = async (
  context: DevTestContext,
  options: TransactionOptions
): Promise<string> => {
  const isLegacy = context.ethTransactionType === "Legacy";
  const isEip2930 = context.ethTransactionType === "EIP2930";
  const isEip1559 = context.ethTransactionType === "EIP1559";

  const gasPrice = options.gasPrice !== undefined ? options.gasPrice : DEFAULT_TXN_MAX_BASE_FEE;
  const maxPriorityFeePerGas =
    options.maxPriorityFeePerGas !== undefined ? options.maxPriorityFeePerGas : 0;

@librelois librelois mentioned this pull request Jan 17, 2023
19 tasks
@notlesh notlesh merged commit f90be04 into master Jan 17, 2023
@notlesh notlesh deleted the notlesh-use-transaction-payment-for-base-fee branch January 17, 2023 16:58
@notlesh notlesh mentioned this pull request Feb 1, 2023
1 task
// 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.

@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 26, 2023
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
…moonbeam-foundation#1765)

* Convert transaction-payments congestion modifier to replace Ethereum's base-fee

* Remove outdated comment

* fix precision bug

* import

* use saturating mul, improve doc

* update fee parameters, add tests

* make tests similar

* add tests to all runtimes

* fix tests

* Add transaction-payment GenesisConfig to initialize its Multiplier

* Fix some moonbase tests

* Rename FixedGasPrice -> TransactionPaymentAsGasPrice

* fmt

* More ts test fixes

* Fixes staking locks

* Fix existential balance tests

* Fix length fee tests

* Query base_fee in createTransaction when needed

* Fixes gasPrice for existential test

* Fix deposit/fee check for multiple deposit

* Fix merge issue

* Use legacy txns for some tests that expect full gas_price to be charged

* Bump gas price for tests

* Update tests to reflect createTransfer default

* Use constant for DEFAULT_TXN_MAX_BASE_FEE

* Reflect txn values to reflect base_fee change

* Prefer constant over literal value

* Overhaul fee calculations in verifyBlockFees

* Bound effectiveTipPerGas to 0

* Fix substrate-based fees

* Overestimate delegation count weight hint

* Use legacy txns and expect full gas_price to be paid as fee

* Use constant for gas limit value

* Start test case for max possible fee conditions

* Clean up

* Add runtime-upgrade test

* prettier

* First look at max possible fee

* fix auto-compound delegation tx order flakiness

* prettier

* Remove cargo override (oops)

* Hack to fix race condition

* prettier

* Use beforeEach for setting max multiplier

* Test multiplier against Fibonacci contract

* prettier

* prettier

* fmt

* Various minor fixes

* Add some fee multiplier scenarios

* Don't use EXTRINSIC_BASE_WEIGHT in gas calc

* Don't expect genesis value at each beforeAll

* Bump expectation of CREATE cost

* Slightly change length fee assumptions

* Use higher gas price

* Remove base_fee test

* fmt

* toml sort

* Resolve compiler warnings

* More compiler warning fixes

* Fix receipt/status test

* Remove irrelevant tests

* Use maxDelegationCount for weight hint

* Remove comment

* Remove ignored tests

* Move fee tests to integration_tests.rs

* Re-remove ignored test

* Move moonbeam runtime fee tests to integration_tests

* Move moonriver runtime fee tests to integration_tests

* fmt

* Use base fee constant for gas price

* Revert test name change...

Co-authored-by: Nisheeth Barthwal <nbaztec@gmail.com>
Co-authored-by: Crystalin <alan@purestake.com>
Co-authored-by: tgmichel <telmo@purestake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes 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.

None yet

6 participants