Fix: explicit EVM call stack #136
Conversation
| Ok(()) => { | ||
| let mut value = H256::default(); | ||
| U256::one().to_big_endian(&mut value[..]); | ||
| runtime.machine.stack_mut().push(value)?; |
There was a problem hiding this comment.
Why no keep using push_u256! and push! macros?
There was a problem hiding this comment.
The push* macros return the Control structure, whereas in this function I want to return the ExitReason directly.
| } | ||
| } | ||
|
|
||
| impl<'a, 'config> Drop for ResolveCreate<'a, 'config> { |
There was a problem hiding this comment.
If Drop is no longer useful, we can probably get rid of ResolveCreate and ResolveCall entirely?
There was a problem hiding this comment.
Probably they can be removed entirely, yes. I think it should be done as a follow-up PR to keep the diff on this PR minimal.
| @@ -1269,7 +1392,18 @@ impl<'inner, 'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> Pr | |||
| context.clone(), | |||
| ) { | |||
| Capture::Exit((s, v)) => (s, v), | |||
| Capture::Trap(_) => unreachable!("Trap is infaillible since StackExecutor is sync"), | |||
| Capture::Trap(rt) => { | |||
| // Ideally this would pass the interrupt back to the executor so it could be | |||
There was a problem hiding this comment.
That would require that precompiles also trap and be resumed later, which would make them hard to write without using (currently unstable) generators.
There was a problem hiding this comment.
I don't think generators are necessary, but I do agree it makes the precompiles interface much more complex is if it returns a Capture type. If we end up changing it then I think it should be done as a separate PR, and in that PR ergonomics of precompiles can be carefully considered.
|
@sorpaas what do you think about it ? |
birchmd
force-pushed
the
upstream-fix-stackoverflow
branch
from
01c502b
to
2994ee1
Compare
|
Thanks for the review @sorpaas I've fixed the typo you pointed out. I also pushed a fix where some tracing events were missing for EVM calls that executed in the new explicit call stack. And I rebased on master since the PR was pretty old. Please take another look and let me know if you have any other comments. |
* update PrecompileHandle ref: rust-ethereum/evm#122 * update fee calculation ref: rust-ethereum/evm#132 * add code_size/code_hash fn in StackState trait ref: rust-ethereum/evm#140 * update evm call stack ref: rust-ethereum/evm#136 * update evm call stack ref: rust-ethereum/evm#155 * add shanghai eips 3651, 3855, 3860 ref: rust-ethereum/evm#152 * update is_precompile ref: rust-ethereum/evm#157 * fix eip-3860 ref: rust-ethereum/evm#160 * update runtime config ref: rust-ethereum/evm#161 * add eip-4399 ref: rust-ethereum/evm#162 * fix eip-2618 ref: rust-ethereum/evm#163 * fix nonce back to U256 ref: rust-ethereum/evm#166 * remove exit_substate in create functions ref: rust-ethereum/evm#168 * record external cost ref: rust-ethereum/evm#170 * add record_external_operation ref: rust-ethereum/evm#171 * add storage_growth ref: rust-ethereum/evm#173 * update evm * switch to shanghai hardfork * update ecrecover ref: polkadot-evm/frontier#964 (#2696)
In the current design, the EVM call stack is emulated via the Rust call stack (using recursive calls to
call_innerandcreate_inner). However, this creates an incompatibility with the EVM spec when the Rust call stack can overflow before the EVM call stack limit. Indeed, we have observed this in Aurora.In this PR, I use the interrupt mechanism already part of the runtime's design to enable an explicit EVM call stack data structure, and resolve all subcalls in an EVM transaction within one loop instead of recursive Rust function calls. This resolves the stack overflow issue.