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(basic_witness_producer_input): Add Basic Witness Producer Input component #156

Merged
merged 78 commits into from
Oct 26, 2023

Conversation

EmilLuta
Copy link
Contributor

@EmilLuta EmilLuta commented Oct 4, 2023

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

  • 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.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

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

Comparison is base (ec41692) 35.61% compared to head (090500d) 35.42%.
Report is 11 commits behind head on main.

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     
Files Coverage Δ
core/bin/zksync_server/src/main.rs 100.00% <ø> (ø)
core/lib/types/src/block.rs 85.71% <ø> (+0.52%) ⬆️
core/lib/types/src/proofs.rs 40.00% <ø> (-11.03%) ⬇️
...lib/zksync_core/src/metadata_calculator/updater.rs 93.78% <100.00%> (+0.10%) ⬆️
core/lib/zksync_core/src/state_keeper/io/mod.rs 82.25% <ø> (ø)
core/lib/zksync_core/src/state_keeper/keeper.rs 71.86% <ø> (ø)
core/lib/dal/src/lib.rs 27.53% <0.00%> (-0.41%) ⬇️
core/lib/dal/src/time_utils.rs 50.00% <0.00%> (ø)
core/lib/zksync_core/src/metrics.rs 0.00% <0.00%> (ø)
core/lib/state/src/witness.rs 0.00% <0.00%> (ø)
... and 9 more

... and 38 files with indirect coverage changes

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

@RomanBrodetski RomanBrodetski changed the title Initial scrapyard commit with "maybe" running vm Witness Input Generator: Initial scrapyard commit with "maybe" running vm Oct 10, 2023
@RomanBrodetski RomanBrodetski changed the title Witness Input Generator: Initial scrapyard commit with "maybe" running vm Witness Input Producer: Initial scrapyard commit with "maybe" running vm Oct 10, 2023
@EmilLuta EmilLuta changed the title Witness Input Producer: Initial scrapyard commit with "maybe" running vm feat(basic_witness_producer_input): Add Basic Witness Producer Input component Oct 16, 2023
@EmilLuta EmilLuta force-pushed the evl-draft-vm-run-for-l1-batch branch 3 times, most recently from 20458f4 to 50c51ea Compare October 17, 2023 10:18
Copy link
Collaborator

@RomanBrodetski RomanBrodetski left a 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/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
@EmilLuta EmilLuta force-pushed the evl-draft-vm-run-for-l1-batch branch from da53f73 to 63e6268 Compare October 17, 2023 14:23
@EmilLuta EmilLuta marked this pull request as ready for review October 17, 2023 14:53
@EmilLuta EmilLuta requested a review from a team as a code owner October 17, 2023 14:53
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.

I also have some general questions, will ask them separately.

core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/basic_witness_input_producer_dal.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/lib.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/metadata_calculator/updater.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/lib.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/state_keeper/io/common.rs Outdated Show resolved Hide resolved
core/lib/dal/src/blocks_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/blocks_dal.rs Outdated Show resolved Hide resolved
@EmilLuta EmilLuta force-pushed the evl-draft-vm-run-for-l1-batch branch from 89ca43e to 81c5236 Compare October 19, 2023 13:02
popzxc
popzxc previously approved these changes Oct 24, 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.

LGTM except for unwrap.

Well done. This PR was tough -- normally it gets easier later, once you learn what you'll be nitpicked on 😅

Deniallugo
Deniallugo previously approved these changes Oct 24, 2023
slowli
slowli previously approved these changes Oct 25, 2023
Copy link
Contributor

@slowli slowli left a 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 👍

core/lib/dal/src/transactions_dal.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/metadata_calculator/updater.rs Outdated Show resolved Hide resolved
RomanBrodetski
RomanBrodetski previously approved these changes Oct 25, 2023
Copy link
Collaborator

@RomanBrodetski RomanBrodetski left a comment

Choose a reason for hiding this comment

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

👍

@EmilLuta EmilLuta added this pull request to the merge queue Oct 26, 2023
Merged via the queue into main with commit 3cd24c9 Oct 26, 2023
25 checks passed
@EmilLuta EmilLuta deleted the evl-draft-vm-run-for-l1-batch branch October 26, 2023 11:29
github-merge-queue bot pushed a commit that referenced this pull request Oct 26, 2023
🤖 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).
@spartucus
Copy link
Contributor

Since the current witness_generator use PG instead of blob, as below code:

let storage = PostgresStorage::new(rt_handle, connection, last_miniblock_number, true);

This Basic Witness Producer component does not work any more, am I understand correctly?

@EmilLuta @shahar4

@RomanBrodetski
Copy link
Collaborator

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

@spartucus
Copy link
Contributor

@RomanBrodetski Thanks for your kind reply.

prover subsystem will not have access to any of the core tables. It will use this component output instead

The BasicWitnessInputProducer's output is WitnessBlockState, is it enough for the prover? Or you will add more data in the output?

For the current code implementation, my question is, given that BasicWitnessInputProducer creates a VmInstance, which executes the transaction and finish the batch, why can't its output be used as a witness? WitnessGenerator on the other hand, uses zkevm_test_harness::external_calls::run_with_fixed_params to generate witnesses, any consideration on this?

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

8 participants