Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(vm): Use the one interface for all vms #277

Merged
merged 33 commits into from
Nov 7, 2023
Merged

feat(vm): Use the one interface for all vms #277

merged 33 commits into from
Nov 7, 2023

Conversation

Deniallugo
Copy link
Contributor

@Deniallugo Deniallugo commented Oct 20, 2023 •
edited
Loading

What ❔
Implement one common interface for all vms. It's the first attempt to do it and it's done only for two latest vms.
In the future all vms will be migrated to the one interface.

Why ❔
We need to unify our work with VM now it's always unpredicable what are we calling and when

p.s. I'm not going to merge it before boojum

The thing, i don't like:
Using vm_latest::HistoryEenable in interface.
It suppose to take another Eternity to refactor HistoryMode

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk fmt and zk lint.

@Deniallugo
Just new interface
cce0d1b 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Remove redundant trait from DefaultTracer
4563b0a 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix interface of tracer
3c579ca 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix interface of tracer
7ac263e 
Signed-off-by: Danil <[email protected]>
@Deniallugo
HistoryMode in the latest vm works correcly
547e8ea 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Use the only tracer
d956403 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Start migrating vm virtual blocks
cb916ed 
Signed-off-by: Danil <[email protected]>
@Deniallugo
auto impl for traits
c191286 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Move call tracer and remove old multi tracer
241e518 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Move storage invocation tracer
6bad2fa 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fully migrate vm virtual blocks to the new interface
40aac00 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Use tracer dispatcher
27625a6 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Use tracer dispatcher for vm intance
0a42fdf 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Use tracer pointer
2025ab0 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Clunky implementation with clone
ba30de8 
Signed-off-by: Danil <[email protected]>
@Deniallugo
some more fixes
7f2b1cc 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Move validation tracer
89fca24 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Migrate the core server to the new interface
de06cc9 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Fix lints
403b4ab 
Signed-off-by: Danil <[email protected]>
@Deniallugo
Restore tests for vm latest
245af44 
Signed-off-by: Danil <[email protected]>
@codecov
Copy link

codecov bot commented Oct 26, 2023 •
edited
Loading

Codecov Report

Attention: 411 lines in your changes are missing coverage. Please review.

Comparison is base (ba88f67) 35.85% compared to head (20a09e7) 35.96%.
Report is 11 commits behind head on main.

❗ Current head 20a09e7 differs from pull request most recent head 466742d. Consider uploading reports for the commit 466742d to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   35.85%   35.96%   +0.10%     
==========================================
  Files         519      525       +6     
  Lines       27828    27811      -17     
==========================================
+ Hits         9978    10002      +24     
+ Misses      17850    17809      -41
Files Coverage Δ
core/bin/system-constants-generator/src/utils.rs 0.00% <ø> (ø)
core/lib/multivm/src/glue/mod.rs 0.00% <ø> (ø)
...b/multivm/src/tracers/call_tracer/vm_latest/mod.rs 55.00% <100.00%> (ø)
...m/src/tracers/call_tracer/vm_virtual_blocks/mod.rs 53.33% <100.00%> (ø)
...vm/src/tracers/storage_invocation/vm_latest/mod.rs 0.00% <ø> (ø)
...ivm/src/versions/vm_1_3_2/oracles/tracer/one_tx.rs 0.00% <ø> (ø)
...ions/vm_1_3_2/oracles/tracer/transaction_result.rs 0.00% <ø> (ø)
...tivm/src/versions/vm_1_3_2/oracles/tracer/utils.rs 0.00% <ø> (ø)
...re/lib/multivm/src/versions/vm_1_3_2/test_utils.rs 0.00% <ø> (ø)
...e/lib/multivm/src/versions/vm_1_3_2/vm_instance.rs 0.00% <ø> (ø)
... and 60 more

... and 73 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Deniallugo
Add inlines
44a41f0 
Signed-off-by: Danil <[email protected]>
popzxc
popzxc reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@popzxc popzxc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, a few nits

core/lib/multivm/src/interface/traits/vm.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/interface/traits/vm.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/tracers/call_tracer/mod.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/tracers/call_tracer/mod.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/tracers/validator/vm_latest/mod.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_latest/tracers/dispatcher.rs Outdated Show resolved Hide resolved
core/lib/multivm/src/versions/vm_virtual_blocks/tracers/dispatcher.rs Outdated Show resolved Hide resolved
popzxc
popzxc previously approved these changes Nov 1, 2023
View reviewed changes
perekopskiy
perekopskiy previously approved these changes Nov 1, 2023
View reviewed changes
@Deniallugo Deniallugo dismissed stale reviews from perekopskiy and popzxc via 7569bdf November 1, 2023 15:23
popzxc
popzxc previously approved these changes Nov 3, 2023
View reviewed changes
popzxc
popzxc previously approved these changes Nov 7, 2023
View reviewed changes
@Deniallugo
Fix tests
466742d 
Signed-off-by: Danil <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2023

Detected VM performance changes

Benchmark name Difference in runtime
event_spam +5.6%
call_far +2.5%
access_memory +6.3%
decode_shl_sub +6.8%
finish_eventful_frames +3.7%
slot_hash_collision +6.4%
write_and_decode +6.6%

@Deniallugo Deniallugo added this pull request to the merge queue Nov 7, 2023
Merged via the queue into main with commit 91bb99b Nov 7, 2023
22 of 23 checks passed
@Deniallugo Deniallugo deleted the deniallugo-vm-interface branch November 7, 2023 14:27
@zksync-era-bot zksync-era-bot mentioned this pull request Nov 7, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 15, 2023
@zksync-era-bot
chore(main): release core 18.0.0 (#424)
44cbb94 
🤖 I have created a release *beep* *boop*
---


##
[18.0.0](core-v17.1.0...core-v18.0.0)
(2023-11-14)


### ⚠ BREAKING CHANGES

* boojum integration
([#112](#112))

### Features

* **basic_witness_input_producer:** Witness inputs queued after BWIP run
([#345](#345))
([9c2be91](9c2be91))
* boojum integration
([#112](#112))
([e76d346](e76d346))
* **core:** adds a get proof endpoint in zks namespace
([#455](#455))
([f4313a4](f4313a4))
* **core:** Split config definitions and deserialization
([#414](#414))
([c7c6b32](c7c6b32))
* **dal:** Do not load config from env in DAL crate
([#444](#444))
([3fe1bb2](3fe1bb2))
* **house_keeper:** Remove GCS Blob Cleaner
([#321](#321))
([9548914](9548914))
* **job-processor:** report attempts metrics
([#448](#448))
([ab31f03](ab31f03))
* **vm:** Use the one interface for all vms
([#277](#277))
([91bb99b](91bb99b))


### Bug Fixes

* **boojnet:** various boojnet fixes
([#462](#462))
([f13648c](f13648c))
* change vks upgrade logic
([#491](#491))
([cb394f3](cb394f3))
* **eth-sender:** Correct ABI for get_verification_key
([#445](#445))
([8af0d85](8af0d85))
* **metadata-calculator:** Save commitment for pre-boojum
([#481](#481))
([664ce33](664ce33))
* Versioned L1 batch metadata
([#450](#450))
([8a40dc3](8a40dc3))
* **vm:** storage_refunds for `vm_refunds_enhancement`
([#449](#449))
([1e1e59f](1e1e59f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
toni-calvin referenced this pull request in lambdaclass/zksync-era Aug 8, 2024
@dnkolegov
Add typos checker and fix typos (#277)
1e50836
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@popzxc popzxc popzxc approved these changes

@montekki montekki montekki left review comments

@perekopskiy perekopskiy perekopskiy left review comments

@RomanBrodetski RomanBrodetski Awaiting requested review from RomanBrodetski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Deniallugo @montekki @popzxc @perekopskiy