-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[pallet-revive] add EVM gas call syscalls #10422
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
base: torsten/gas-fixes
Are you sure you want to change the base?
Conversation
Signed-off-by: xermicus <[email protected]>
|
/cmd fmt |
pgherveou
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.
Looks good at first glance
On my phone now will take a second look from laptop
Co-authored-by: PG Herveou <[email protected]>
| flags: u32, | ||
| callee: u32, | ||
| value_ptr: u32, | ||
| gas_ptr: u32, |
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.
shouldn't we pass gas as a u64?
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.
Why u64? It would move the overflow check into contract code.
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 was not thinking of solidity
the uapi would be nicer if it was just
pub fn call_evm(
flags: u32,
callee: u32,
value: u32,
gas: u64,
input_data: u64,
output_data: u64,
) -> ReturnCode;
since gas is just a u64.
but then I guess since in revive you translate call instructions where the gas is a uint256 that would force you to do the overflow check in the contract
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.
Yeah the gas argument is an u256 (since it's the only int type in Yul).
However, actually considering it, an u64 probably wouldn't even be that bad. In many cases, the supplied gas is just what was returned from gas() or some constant value (address.transfer) - both cases don't need overflow checks (this is an optimization).
Such optimizations aren't planned for the initial resolc 1.0 release but easy to implement in the future. So I think it'd actually be helpful when this matches what gas_left returns, which is u64. Done right, I suspect that this will lead to LLVM itself to recognize and eliminate the overflow check - I'll test this to be sure (because then it's definitively favorable).
Thinking it further, we could also use a "no gas" API method for when the gas to use is what gas() returned. This would just use CallResources::NoLimits. Which could just be even be expressed by a sentinel pointer value. Probably fine to just us u64::max too, since this would mean uncapped resources anyways? Need to think about this.
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.
All that make sense.
when I wrote the comment I was mostly thinking of the use case of someone writing a contract in Rust with the low-level API where there I think it make sense to use a u64.
Thinking it further, we could also use a "no gas" API method for when the gas to use is what gas() returned. This would just use CallResources::NoLimits. Which could just be even be expressed by a sentinel pointer value. Probably fine to just us u64::max too, since this would mean uncapped resources anyways? Need to think about this.
Yeah maybe in that case you call the regular call method with W(u64::max, u64::max) as it result in less math ops to compute the gas_left (@TorstenStueber might be better advisor 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.
Yeah the new syscalls are specifically for resolc - with Rust contracts people are not bound to EVM gas and can use the existing syscalls using Weight and Deposit.
Regarding the uncapped calls. I think this depends if we implement the "63/64" rule. Even if we don't implement it exactly like on Ethereum: The contract can't just supply the absolute maximum, instead it needs to be able to express that the call should get whatever is left.
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.
Sounds all good.
Yeah maybe in that case you call the regular call method with W(u64::max, u64::max) as it result in less math ops to compute the gas_left (@TorstenStueber might be better advisor here)
CallResources::NoLimits is always the least overhead. If the transaction as a whole uses Ethereum execution mode (limited by gas), then any call to a subsequent frame won't be able to switch that mode to Substrate execution mode even if using W(u64::max, u64::max).
Regarding the uncapped calls. I think this depends if we implement the "63/64" rule. Even if we don't implement it exactly like on Ethereum: The contract can't just supply the absolute maximum, instead it needs to be able to express that the call should get whatever is left.
There is currently no such implementation itself. We can add a 63/64 later but the current logic is already complicated enough for the first iteration.
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.
Yeah the new syscalls are specifically for resolc - with Rust contracts people are not bound to EVM gas and can use the existing syscalls using Weight and Deposit.
However, actually considering it, an u64 probably wouldn't even be that bad. In many cases, the supplied gas is just what was returned from gas() or some constant value (address.transfer) - both cases don't need overflow checks (this is an optimization).
It's your call but If future compiler optimisation can optimize that away, picking the host call that use u64 might be interesting as well.
If we revisit the whole pallet-transaction-payment, and gas mapping integration to make gas the main metering unit (instead of it being derived from being fees / gas_price) then this could also become a nice api for Rust contract as well
Signed-off-by: xermicus <[email protected]>
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
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.
Nice, I like it.
| flags: u32, | ||
| callee: u32, | ||
| value_ptr: u32, | ||
| gas_ptr: u32, |
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.
Sounds all good.
Yeah maybe in that case you call the regular call method with W(u64::max, u64::max) as it result in less math ops to compute the gas_left (@TorstenStueber might be better advisor here)
CallResources::NoLimits is always the least overhead. If the transaction as a whole uses Ethereum execution mode (limited by gas), then any call to a subsequent frame won't be able to switch that mode to Substrate execution mode even if using W(u64::max, u64::max).
Regarding the uncapped calls. I think this depends if we implement the "63/64" rule. Even if we don't implement it exactly like on Ethereum: The contract can't just supply the absolute maximum, instead it needs to be able to express that the call should get whatever is left.
There is currently no such implementation itself. We can add a 63/64 later but the current logic is already complicated enough for the first iteration.
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).