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

Re-Prioritize pool transactions by effective gas price #581

Merged
merged 11 commits into from
Sep 14, 2021

Conversation

JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Jul 7, 2021

This PR is a stop-gap measure that allows ethereum transactions to be properly prioritized by gas_price and also properly prioritized alongside non-ethereum transactions that specify a flat tip amount rather than a gas price.

It makes two concrete changes to our transaction prioritization:

  1. It stops adding-weight-based fees to all transactions and relies only on those set by pallet-transaction-payment. This is based on Allow adjusting priority coming from Signed Extensions paritytech/substrate#9596 which is not yet merged in Substrate, but is on our pinned Substrate branch. (notice the changes to Cargo.lock.)
  2. It overwrites the priorities assigned by pallet_transaction_payment with "effective gas prices" calculated from our GasWeightMapping.

The latter is a bit of a stop-gap measure. A more complete ethereum-first approach would be to replace pallet-transaction-payment with a new pallet that allows users to specify gas price for all transactions including "normal" frame transactions.

This stopgap approach was also explored briefly in the Frontier repo in polkadot-evm/frontier#378.

Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

Interesting idea. Keep in mind that EIP-1559 will make Ethereum's gas structure look more like Substrate's pallet-transaction-payment, although if we adopt it we will probably want backwards-compatibility (as Ethereum will do).

None => 0,
Some((_, _, signed_extra)) => {
// Yuck, this depends on the index of charge transaction in Signed Extra
let charge_transaction = signed_extra.6;
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is ugly. I think the more thorough approach would be to have a pallet-ethereum-transaction-payment that properly handles fees in a Frontier-equipped chain. This is a stop-gap measure.

@JoshOrndorff

This comment has been minimized.

@JoshOrndorff JoshOrndorff added B0-silent Changes should not be mentioned in any release notes C1-low Does not elevate a release containing this beyond "low priority". labels Aug 18, 2021
@JoshOrndorff
Copy link
Contributor Author

JoshOrndorff commented Sep 7, 2021

This will make sense again in conjunction with paritytech/substrate#9596

EDIT: That is not merge in Substrate yet, so I've put it on our pinned Susbtrate fork. See moonbeam-foundation/substrate@2af6e2c

@JoshOrndorff JoshOrndorff added the A0-pleasereview Pull request needs code review. label Sep 13, 2021
runtime/moonbase/src/lib.rs Outdated Show resolved Hide resolved
runtime/moonbase/src/lib.rs Show resolved Hide resolved
@JoshOrndorff
Copy link
Contributor Author

So the test failure here is legit. It happens with the call to sudo_unchecked_weight when the weight is set too low. I see where a weight set to zero would cause a division by zero, but I didn't expect it to happen for low-but-non-zero weights. The call that exists in our test has a weight of 1. By experimenting manually, I've found that a weight of 20_000 is low enough that the test fails, but a weight of 30_000 is high enough that it passes.

@crystalin crystalin merged commit 3a88031 into master Sep 14, 2021
@crystalin crystalin deleted the joshy-reprioritize-effective-gas-price-hack branch September 14, 2021 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low Does not elevate a release containing this beyond "low priority".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants