refactor(blockifier): inject VC into CallInfo::summarize #2122
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2122 +/- ##
===========================================
+ Coverage 40.10% 69.38% +29.28%
===========================================
Files 26 108 +82
Lines 1895 13984 +12089
Branches 1895 13984 +12089
===========================================
+ Hits 760 9703 +8943
- Misses 1100 3874 +2774
- Partials 35 407 +372 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
dorimedini-starkware
force-pushed
the
11-17-refactor_blockifier_move_logic_to_function
branch
from
7a27e86
to
65e10b3
Compare
dorimedini-starkware
force-pushed
the
11-17-refactor_blockifier_inject_vc_into_callinfo_summarize
branch
from
179fe8c
to
a9e2006
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
I suggest a cleaner approach - don't pass the VC.
Summarize regularly, and if the flag is on, drop the event summary and calc it again on validate & execute call infos.
Also, this should not affect the bouncer.
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @aner-starkware)
There was a problem hiding this comment.
- why is this cleaner? replacing an already-calculated object with another seems jumbled
- this will have no effect on the bouncer because this will have no effect on post-0.13.1 blocks; you are saying, it is worth adding code to ensure the 0.13.1 bouncer still gets "correct" event counting, and only the fee computation should be effected?
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @aner-starkware)
There was a problem hiding this comment.
- No, I say, if we don't pass the VC, you don't need to fix the calc for the bouncer, just for the fee calc.
- It is more local and framed. Doesn't require passing another argument to many places just for this. I prefer not to have this hack in call_info.summarize()
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @aner-starkware)
There was a problem hiding this comment.
the TransactionResources struct doesn't store call infos. I would need to compute the "alternative" event resources when computing the resources, and then choose what to charge for in to_gas_vector.
injecting call infos as a dependency of resource computation seems like a worse option; did you have some specific place in mind where it makes sense to recompute?
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @aner-starkware)
There was a problem hiding this comment.
No, I say, if we don't pass the VC, you don't need to fix the calc for the bouncer, just for the fee calc.
why would we want to fix this for the bouncer at all? bouncer is irrelevant for reexecution, and we should not support "no inner event charge" mode in latest sequencer version, right?
It is more local and framed. Doesn't require passing another argument to many places just for this.
where would we get the callinfos from...? (above question)
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @aner-starkware)
dorimedini-starkware
force-pushed
the
11-17-refactor_blockifier_move_logic_to_function
branch
from
65e10b3
to
975ff5a
Compare
dorimedini-starkware
force-pushed
the
11-17-refactor_blockifier_inject_vc_into_callinfo_summarize
branch
from
a9e2006
to
d525dc2
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 4 of 10 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)
dorimedini-starkware
changed the base branch from
11-17-refactor_blockifier_move_logic_to_function
to
graphite-base/2122
dorimedini-starkware
changed the base branch from
graphite-base/2122
to
main
dorimedini-starkware
force-pushed
the
11-17-refactor_blockifier_inject_vc_into_callinfo_summarize
branch
from
d525dc2
to
70bfbca
Compare
|
Artifacts upload triggered. View details here |
|
Artifacts upload triggered. View details here |
No description provided.