-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(basic_witness_producer_input): Add Basic Witness Producer Input component #156
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
==========================================
- Coverage 35.61% 35.42% -0.20%
==========================================
Files 520 524 +4
Lines 28352 28500 +148
==========================================
- Hits 10098 10096 -2
- Misses 18254 18404 +150
☔ View full report in Codecov by Sentry. |
20458f4
to
50c51ea
Compare
core/lib/dal/migrations/20231016052911_add_basic_witness_input_producer_jobs_table.up.sql
Outdated
Show resolved
Hide resolved
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.
good progress - just left some nits
core/lib/zksync_core/src/basic_witness_input_producer/vm_interactions.rs
Outdated
Show resolved
Hide resolved
da53f73
to
63e6268
Compare
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 also have some general questions, will ask them separately.
core/lib/dal/migrations/20231016052911_add_basic_witness_input_producer_jobs_table.up.sql
Outdated
Show resolved
Hide resolved
89ca43e
to
81c5236
Compare
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 except for unwrap.
Well done. This PR was tough -- normally it gets easier later, once you learn what you'll be nitpicked on 😅
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.
Other than small nits, looks fine 👍
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.
👍
1c9ff8f
🤖 I have created a release *beep* *boop* --- ## [16.2.0](core-v16.1.0...core-v16.2.0) (2023-10-26) ### Features * **basic_witness_producer_input:** Add Basic Witness Producer Input component ([#156](#156)) ([3cd24c9](3cd24c9)) * **core:** adding pubdata to statekeeper and merkle tree ([#259](#259)) ([1659c84](1659c84)) ### Bug Fixes * **db:** Fix root cause of RocksDB misbehavior ([#301](#301)) ([d6c30ab](d6c30ab)) * **en:** gracefully shutdown en waiting for reorg detector ([#270](#270)) ([f048485](f048485)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Since the current
This Basic Witness Producer component does not work any more, am I understand correctly? |
@spartucus the component works, but its output is indeed not used. This migration is planned for the coming months - prover subsystem will not have access to any of the core tables. It will use this component output instead |
@RomanBrodetski Thanks for your kind reply.
The For the current code implementation, my question is, given that |
What ❔
This PR introduces a new component that will be ran alongside all components in the service. The component extracts all information needed for running Basic Witness Generator in a binary serialized Input and uploads it to Object Store (GCS for now).
Why ❔
Part of work decoupling Prover Subsystems from Server Core. Today, Witness Generators need Server Core Database's access to generate witnesses. More information can be found here for the Witness Generator split and here for the Prover Subsystems and Server core split.
Checklist
zk fmt
andzk lint
.