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.

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

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

Merged
merged 33 commits into from
Nov 7, 2023
Merged

Conversation

Deniallugo
Copy link
Contributor

@Deniallugo Deniallugo commented Oct 20, 2023

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.

Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
@codecov
Copy link

codecov bot commented Oct 26, 2023

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.

Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
Signed-off-by: Danil <deniallugo@gmail.com>
montekki
montekki previously approved these changes Oct 31, 2023
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
Signed-off-by: Danil <deniallugo@gmail.com>
popzxc
popzxc previously approved these changes Nov 1, 2023
perekopskiy
perekopskiy previously approved these changes Nov 1, 2023
Signed-off-by: Danil <deniallugo@gmail.com>
@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
Signed-off-by: Danil <deniallugo@gmail.com>
popzxc
popzxc previously approved these changes Nov 7, 2023
Signed-off-by: Danil <deniallugo@gmail.com>
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
github-merge-queue bot pushed a commit that referenced this pull request Nov 15, 2023
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants