-
Notifications
You must be signed in to change notification settings - Fork 615
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
vmlogic: split up into two #11643
vmlogic: split up into two #11643
Conversation
The split boundary has been chosen to be what's necessary to compute a VMOutcome, which now in turn allows us to load a contract without constructing a VMLogic, or contract memory quite yet. This might very well resolve issues I've been working through by attempting to remove lifetimes and such from `VMLogic`...? As previous changes this makes quite some sense in isolation regardless of the ongoing projects. While I imagine there are more lines, they will mostly be due to the fact that in many places the previous code now needs to go through an additional field projection to get to types it needs to operate.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11643 +/- ##
==========================================
- Coverage 71.64% 71.63% -0.02%
==========================================
Files 787 787
Lines 160969 161191 +222
Branches 160969 161191 +222
==========================================
+ Hits 115333 115475 +142
- Misses 40602 40683 +81
+ Partials 5034 5033 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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! It’s great, this change avoids the VMLogic thread-through issues I had been having, by actually creating the things at a more intuitive place :)
/// This is a subset of [`VMLogic`] that's strictly necessary to produce `VMOutcome`s. | ||
pub struct ExecutionResultState { | ||
/// All gas and economic parameters required during contract execution. | ||
config: Arc<Config>, |
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.
Hmm I’m not sure I get why gas parameters would be required to build a VMOutcome
. AFAIU, this is only required in order to actually execute the contract, and not really for the result.
So WDYT about naming this struct ExecutionState
rather than ExecutionResultState
?
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.
The config (or rather ext_costs
within it) is necessary for the profile data computation. Since ext_costs
always gets constructed as part of an overall Config
, I didn't see a good reason to split it up anyhow. In principle ExecutionResultState::compute_outcome
was the guiding factor on what to include here and what to leave back at VMLogic
.
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.
(The comment also explains why this is specifically a "ResultState" -- it contains things that are strictly only for the eventual computation of a VMOutcome
)
The split boundary has been chosen to be what's necessary to compute a VMOutcome, which now in turn allows us to load a contract without constructing a VMLogic, or contract memory quite yet.
This might very well resolve issues I've been working through by attempting to remove lifetimes and such from
VMLogic
...? As previous changes this makes quite some sense in isolation regardless of the ongoing projects. While I imagine there are more lines, they will mostly be due to the fact that in many places the previous code now needs to go through an additional field projection to get to types it needs to operate.@Ekleog-NEAR I think you'll appreciate these as I recall you've struggled with the VMLogic nonsense as well in the past.
Part of #11319