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

Introduce Weight v2 support for EVM (frontier) #2316

Merged
merged 96 commits into from Jun 6, 2023
Merged

Conversation

tgmichel
Copy link
Contributor

@tgmichel tgmichel commented May 29, 2023

What does it do?

This PR introduces support for external cost recording in evm execution.

The goal is to add additional metric(s) capacity that will result in an OutOfGas when exhausted. When the evm is compiled with a with-substrate feature, adds two additional PrecompileHandle methods - record_external_cost and refund_external_cost - which are called on pre_validate, that is, before an OpCode is executed. This way we make sure each OpCode can be executed not only taking native remaining gas into consideration, but also any other metric that the execution is bounded with. In the case of Moonbeam there is only one metric for now - pov_size - but it will also support ref_time soon™. This keeps behavioural changes in the evm to bare minimum while adding the virtual gasometer dimension in the Substrate backend.

Runtime Changes

  • Switch to shanghai configuration.
  • External cost recording. Introduces a new capacity based on GasLimitPovRatio, which is set to 1/4 - 4 gas unit per byte.
  • Adapt all precompiles to record pov cost associated with storage read operations in addition to gas cost.
  • Ethereum call runtime api now supports external cost recording in non-transactional context as well.

Client Changes

  • New ethereum-block based notification. Pubsub previsouly reacted to substrate import notification stream for NewHeads subscriptions. This caused race-conditions, because that is the channel also used by the secondary db mapper. Now pubsub will emit to subscribers only when the secondary db has effectively mapped the ethereum block.

⚠️ Breaking Changes ⚠️

The concept of OutOfGas and refunds is expanded, and might break some Dapp asumptions.

OutOfGas can now be thrown transaction-wide if any of the configured metrics' capacity is reached - in other words, the evm can OOG with remaining legacy gas in the gasometer.

Refunds are based on the more consumed resource after the execution - in other words, if more pov_size has been consumed proportionally than legacy gas, the refund will be calculated using pov_size.

@github-actions
Copy link
Contributor

github-actions bot commented May 29, 2023

Coverage generated "Tue Jun 6 10:52:23 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2316/html/index.html

Master coverage: 71.19%
Pull coverage: 71.17%

@librelois librelois mentioned this pull request May 30, 2023
24 tasks
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

I lelft some comments, there is also some TODO that should be fixed before merge

pallets/erc20-xcm-bridge/src/lib.rs Outdated Show resolved Hide resolved
pallets/erc20-xcm-bridge/src/lib.rs Outdated Show resolved Hide resolved
pallets/ethereum-xcm/src/lib.rs Outdated Show resolved Hide resolved
@tgmichel
Copy link
Contributor Author

I lelft some comments, there is also some TODO that should be fixed before merge

thank you @librelois , done

@librelois librelois 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 May 30, 2023
pallets/erc20-xcm-bridge/src/lib.rs Outdated Show resolved Hide resolved
runtime/common/src/apis.rs Show resolved Hide resolved
runtime/common/src/apis.rs Show resolved Hide resolved
let pubsub_notification_sinks: fc_mapping_sync::EthereumBlockNotificationSinks<
fc_mapping_sync::EthereumBlockNotification<Block>,
> = Default::default();
let pubsub_notification_sinks = Arc::new(pubsub_notification_sinks);
Copy link
Contributor

Choose a reason for hiding this comment

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

A question about the channel, if this is created per-subscription is it possible to OOM kill the client via flooding it with subscriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the connection is kept open in the client end, the limit of channels is the number of websocket connections the server supports. Each time a new ethereum block is produced, channels associated to closed connections are removed from the pool.

@tgmichel tgmichel merged commit 16b1898 into master Jun 6, 2023
26 checks passed
@tgmichel tgmichel deleted the tgm-evm-weight-v2 branch June 6, 2023 11:22
@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 Jun 14, 2023
@crystalin crystalin changed the title Update frontier (evm + weight v2) Introduce Weight v2 support for EVM (frontier) Jun 15, 2023
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