-
Notifications
You must be signed in to change notification settings - Fork 1k
pallet_revive: Change EVM call opcodes to respect the gas limit passed #10018
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
Open
athei
wants to merge
11
commits into
master
Choose a base branch
from
at/gas-fixes
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+436
−313
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
0d70f68
Remove stale TODOs
athei ef44b11
Fix maxPriorityFee RPC
athei b2e9b0d
Change the EVM call opcodes to use proper gas for subcalls
athei 197c72a
Update tests-evm.yml
pgherveou 1ab0dd1
Update from github-actions[bot] running command 'prdoc --audience run…
github-actions[bot] 686efb3
fix
pgherveou 8a355ae
fix
pgherveou 90bfadd
Merge branch 'master' into at/gas-fixes
athei 7e1a8e3
Update .github/workflows/tests-evm.yml
pgherveou b96ac78
Merge branch 'master' into at/gas-fixes
athei 67814db
update tests-evm
pgherveou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
title: 'pallet_revive: Change EVM call opcodes to respect the gas limit passed' | ||
doc: | ||
- audience: Runtime Dev | ||
description: |- | ||
So far the EVM family of call opcodes did ignore the `gas` argument passed to them. The consequence was that we were not able to limit the resource usage of sub contract calls. This PR changes that. **Gas is now fully functional on the EVM backend.** | ||
|
||
The resources of any sub contract call are now effectively limited. This is both true for `Weight` and storage deposit. The algorithm works in a way that if you pass `x%` of the current `GAS` the the `CALL` opcode the sub call will have `x%` of currently available `Weight` and storage deposit available. This allows the caller to always make sure to execute code after retuning from a sub call. | ||
|
||
### Changes to the gas meter | ||
|
||
I needed to change the gas meter to track `gas_consumed` instead of `gas_left`. Otherwise it is not possible to know the total amount of gas spent for a call stack that is not unwinded, yet. | ||
|
||
### Followup | ||
- Implement a new PVM syscall that takes the new unified gas instead of `Weight` and storage deposit limit | ||
- Change resolc to use this new syscall | ||
- Enable the test added here to run on resolc | ||
crates: | ||
- name: pallet-revive | ||
bump: major | ||
- name: pallet-revive-eth-rpc | ||
bump: major | ||
- name: pallet-revive-fixtures | ||
bump: major |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,4 +32,8 @@ contract Callee { | |
stop() | ||
} | ||
} | ||
|
||
function consumeAllReftime() external { | ||
while (true) {} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any data points on wallets being confused by the fact that this is zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this (eth_maxPriorityFeePerGas) is not really used in practice, wallets and library like alloy will usually use
eth_feeHistory
to propose the priority feeSince we don't support this concept for now anyway we are better off with this change anyway