-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[pallet-revive] add EVM gas call syscalls #10554
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
Conversation
Signed-off-by: xermicus <[email protected]>
|
/cmd fmt |
|
/cmd prdoc --audience runtime_dev --bump minor |
…time_dev --bump minor'
Signed-off-by: xermicus <[email protected]>
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.
After the discussion from #10422:
- Going with passing the gas as
u64. - Adds the gas stipend if the call has value.
- If
u64::MAXis supplied,NoLmitswill be used instead. This is nuanced but will make an important difference once we implement some "63/64 rule" logic: When we would not do this, "63/64 rule" is futile, because supplying a fraction ofu64::MAXeffectively circumvents it. But withNoLimits, the "63/64 rule" will take whatever is left in the tank and not the absolute maximum.
Differential Tests Results (PolkaVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
Differential Tests Results (REVM)Specified Tests
Counts
FailuresThe test specifiers seen in this section have the format 'path::case_idx::compilation_mode' and they're compatible with the revive differential tests framework and can be specified to it directly in the same way that they're provided through the The failures are provided in an expandable section to ensure that the PR does not get polluted with information. Please click on the section below for more information Detailed Differential Tests Failure Information
|
| let add_stipend = !value.is_zero(); | ||
| CallResources::from_ethereum_gas(gas.into(), add_stipend) |
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.
Don't we want this full logic?
polkadot-sdk/substrate/frame/revive/src/vm/evm/instructions/contract.rs
Lines 193 to 201 in 68c1250
| // We use ALL_STIPEND to detect the typical gas limit solc defines as a call stipend. This is | |
| // just a heuristic. | |
| let (add_stipend, reentracy) = | |
| match (value.is_zero(), gas_limit.try_into().is_ok_and(|limit: u64| limit == CALL_STIPEND)) | |
| { | |
| (false, _) => (true, ReentrancyProtection::AllowReentry), | |
| (_, true) => (true, ReentrancyProtection::AllowNext), | |
| (_, _) => (false, ReentrancyProtection::AllowReentry), | |
| }; |
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.
I considered it but am not sure if it makes a difference. resolc always allows re-entrancy. Is the assumption that with the Gas stipend we can have multiple re-entrant calls into the contract? That would require at least 4 new frames.
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.
What about adding the stipend when the supplied gas is 2300? I think we need this in order to replace the 2300 with the scaled stipend.
resolcalways allows re-entrancy.
So does solc. We still set it to once when a transfer is detected. This is as additional security in case out scaled stipend allows for too much stuff.
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.
let _1 := 0
if iszero(var_amount) { _1 := 2300 }
if iszero(call(_1,
This is what solc emits for transfers. So if the value is zero, gas is 2300. If the value is non-zero, gas is zero but we add the 2300 gas stipend. So if we were to add 2300 gas if gas is 2300, this would just result in 4600 gas for transfers with 0 value. I don't think this is necessary. The geth EVM also does the same: Just add the stipend for (any) non-zero calls. Maybe I'm missing something but I think we should be good here.
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.
additional security in case out scaled stipend allows for too much stuff
I see but I would be highly surprised if "2300 gas" would allow for the execution 4 additional frames including exploit logic
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.
Got it now. We need to add something scaled. My (wrong) assumption so far was that when 2300 is supplied, that 2300 will eventually be something scaled anyways.
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.
My (wrong) assumption so far was that when 2300 is supplied, that 2300 will eventually be something scaled anyways.
Exactly. We generally don't scale gas values. The gas stipend is the exception since it is a hard coded magic number. So we have no way around this.
|
@0xOmarA did this go down here? actually seems to be the same as #10552 (comment) |
TorstenStueber
left a comment
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.
👍
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10554-to-unstable2507
git worktree add --checkout .worktree/backport-10554-to-unstable2507 backport-10554-to-unstable2507
cd .worktree/backport-10554-to-unstable2507
git reset --hard HEAD^
git cherry-pick -x 5a1128b94bbd6bcab40654c9468542a1f21f5eab
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10554-to-stable2509
git worktree add --checkout .worktree/backport-10554-to-stable2509 backport-10554-to-stable2509
cd .worktree/backport-10554-to-stable2509
git reset --hard HEAD^
git cherry-pick -x 5a1128b94bbd6bcab40654c9468542a1f21f5eab
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-10554-to-stable2512
git worktree add --checkout .worktree/backport-10554-to-stable2512 backport-10554-to-stable2512
cd .worktree/backport-10554-to-stable2512
git reset --hard HEAD^
git cherry-pick -x 5a1128b94bbd6bcab40654c9468542a1f21f5eab
git push --force-with-lease |
This PR adds two new syscalls for calls accepting EVM gas instead of Weight and Deposit. This is an important change for the initial release as it will align PVM contracts closer to EVM (the problem can't be solved in the Solidity compiler). --------- Signed-off-by: xermicus <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Backport all pallet-revive related changes into `unstable2507`. These are all the changes we want to get onto the next Kusama release. Main changes include - EVM backend - Ethereum block storage - Generalized gas mapping The complete list of PRs in this backport is - #9482 - #9455 - #9454 - #9501 - #9177 - #9285 - #9606 - #9414 - #9557 - #9617 - #9385 - #9679 - #9705 - #9561 - #9744 - #9736 - #9701 - #9517 - #9771 - #9683 - #9791 - #9717 - #9759 - #9823 - #9768 - #9853 - #9801 - #9780 - #9796 - #9878 - #9841 - #9670 - #9865 - #9803 - #9928 - #9818 - #9911 - #9942 - #9831 - #9945 - #9603 - #9968 - #9939 - #9991 - #9914 - #9997 - #9985 - #10016 - #10027 - #10026 - #9418 - #9988 - #10041 - #10047 - #10032 - #10065 - #10089 - #10080 - #10090 - #10106 - #10020 - #9512 - #10109 - #9699 - #10100 - #9909 - #10120 - #10146 - #10157 - #10168 - #10169 - #10160 - #10129 - #10175 - #10186 - #10192 - #10148 - #10193 - #10220 - #10233 - #10191 - #10225 - #10246 - #10239 - #10159 - #10252 - #10224 - #10267 - #10271 - #10214 - #10297 - #10290 - #10281 - #10272 - #10303 - #10336 - #10244 - #10366 - #10380 - #10383 - #10387 - #10302 - #10309 - #10427 - #10385 - #10451 - #10471 - #10166 - #10510 - #10393 - #10540 - #9587 - #10071 - #10558 - #10554 - #10325 --------- Signed-off-by: xermicus <[email protected]> Co-authored-by: Pavlo Khrystenko <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Javier Viola <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: pgherveou <[email protected]> Co-authored-by: Omar <[email protected]> Co-authored-by: 0xRVE <[email protected]> Co-authored-by: xermicus <[email protected]> Co-authored-by: Alexander Samusev <[email protected]>
This PR adds two new syscalls for calls accepting EVM gas instead of Weight and Deposit.
This is an important change for the initial release as it will align PVM contracts closer to EVM (the problem can't be solved in the Solidity compiler).