-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Resumable EVM and heap-allocated callstack #9360
Conversation
@tomusdrw let's review this together by the end of this week :) |
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.
lgtm, just a few minor grumbles. Would be good to get another pair of eyes to look at it
self.stack.push(U256::zero()); | ||
Ok(InstructionResult::Ok) | ||
}, | ||
Err(trap) => { | ||
Ok(InstructionResult::Trap(trap)) |
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 is Err
turned into Ok
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.
If ext.create
returns Err
, it can only be a trap
. (That also means that if trap
parameter is passed false
, Err
case is impossible.) So in here if we want to trap the interpreter, we can directly match this.
We really have three cases for the exec_instruction
return type:
vm::Result::Err
which indicates some errors happened.vm::Result::Ok(Instruction::Result::Trap(..))
which indicates that a trap in the EVM happened.vm::Result::Ok(..)
which indicates some other instruction results.
Putting Trap
to error and wrap vm::Result::Err
can also work, but it means that we need to replicate a lot more From/Into
impls to support ?
syntax in this function, and I think it may be a little bit overkill.
self.stack.push(U256::zero()); | ||
Ok(InstructionResult::Ok) | ||
}, | ||
Err(trap) => { | ||
Ok(InstructionResult::Trap(trap)) |
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 is Err
turned into Ok
here? I just don't understand the flow :)
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 like the same case as #9360 (comment)
Ok(res) | ||
}, | ||
CallCreateExecutiveKind::ResumeCreate(..) => | ||
panic!("Resumable as create, but called resume_call"), |
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.
is there any way we can avoid those panics?
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.
We can create traits similar to what we do for vm:
pub trait Exec: Send {
fn exec(self: Box<Self>, state: &mut State<B>, ...) -> ExecutiveTrapResult<GasLeft>;
}
pub trait ResumeCall: Send {
fn resume_call(self: Box<Self>, state: &mut State<B>, ...) -> Box<Exec>;
}
pub trait ResumeCreate: Send {
fn resume_create(self: Box<Self>, state: &mut State<B>, ...) -> Box<Exec>;
}
However, they would need to use traits objects and dynamic dispatch, which is a small amount of runtime overhead. I think it may be a good idea to only use those when in the future we want to expose Executive
to be some sort of public API.
action: Action::Create(create.expect("self.prepare_trace_create().is_some(): so we must be tracing: qed")), | ||
trace_address: self.index_stack.clone(), | ||
subtraces: self.sublen_stack.last().cloned().unwrap_or(0), | ||
action: Action::Create(Create::from(params.clone())), | ||
result: Res::Create(CreateResult { |
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.
maybe it's worth introducing a third variant? or redesigning the implementation to avoid dummy data creation?
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 think this may be okay. U256::zero()
, Vec::new()
, Address::default()
all would just zero out the fields, and wouldn't allocate anything on the heap.
- From performance's perspective, it wouldn't make any difference if we do dummy data creation or use
Option
, they would both just zeroing out the fields. - From clarity's perspective, this dummy data creation is restraint in
ExecutiveTracer
and doesn't add any complexity outside of it.
…-resume-executive2
Could I get a 2nd review here @fckt @andresilva :) |
…-resume-executive2
return; | ||
} | ||
|
||
let vecindex = self.vecindex_stack.pop().expect("prepare/done_trace are not balanced"); |
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.
would be nice to write why they are always balanced
(and many of those below)
🎉 |
Mostly fixes #6744. All basic things for this PR are fixed now.
Exec
, which starts a VM execution, and can return a resumable trap,ResumeCall
/ResumeCreate
to resume execution from call or create traps. Those are accessed using trait objects.Externalities::call/create
now has an additional parametertrap
. Iftrap
istrue
(the case for EVM), then this function would return a trap. Iftrap
isfalse
(the case for Wasm), then it will directly execute the context.call/create
calls inExecutive
to use a separate structCallCreateExecutive
. This is the main struct that handles all resumes. One can useCallCreateExecutive::exec/resume_call/resume_create
to get the resume behavior, or useCallCreateExecutive::consume
to get the non-resume behavior.Things to be done:
TODO
proof messages.crossbeam
again. In rare cases we can still exceeds current call stack limit (if too many Wasm contexts are stacked).Provide a compile feature flag to enable wasm trap. With pause PR merged to wasmi, that's doable.(This PR is already getting big, let's do this in another PR.)