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(commitment): Commitment component #1024

Merged
merged 12 commits into from
Feb 13, 2024
Merged

feat(commitment): Commitment component #1024

merged 12 commits into from
Feb 13, 2024

Conversation

perekopskiy
Copy link
Contributor

@perekopskiy perekopskiy commented Feb 7, 2024 •
edited
Loading

What ❔

  • Adding new component that calculate commitment for L1 batches.
  • Metadata calculator now only calculates things that require tree, i.e. root hash and merkle paths in case full tree mode is on.
  • Soft removal of some l1_batches columns: parent_hash, l2_l1_compressed_messages, compressed_write_logs, compressed_contracts.
  • Fixed field factory_deps of L1BatchWithMetadata, it's renamed to raw_published_factory_deps, now it represents all published bytecodes for pre-boojum batches and published uncompressed bytecodes for post-boojum batches (compressed bytecodes are published as a part of long L2 to L1 messages).

Why ❔

  • To follow the single responsibility principle
  • To improve the performance of full tree which directly affects EN performance

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.
  • Spellcheck has been run via zk spellcheck.
  • Linkcheck has been run via zk linkcheck.

@perekopskiy perekopskiy marked this pull request as ready for review February 7, 2024 12:03
@perekopskiy perekopskiy requested a review from a team as a code owner February 7, 2024 12:03
slowli
slowli reviewed Feb 7, 2024
View reviewed changes
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.

Will try to perform more in-depth review tomorrow.

core/lib/dal/src/blocks_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/blocks_dal.rs Show resolved Hide resolved
core/lib/dal/src/blocks_dal.rs Outdated Show resolved Hide resolved
core/lib/dal/src/blocks_dal.rs Show resolved Hide resolved
core/lib/dal/src/blocks_dal.rs Outdated Show resolved Hide resolved
core/lib/types/src/commitment.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/metrics.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Show resolved Hide resolved
RomanBrodetski
RomanBrodetski previously approved these changes Feb 8, 2024
View reviewed changes
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.

Looks good!

core/lib/dal/migrations/20240126114931_add-l1_batches-commitment-index.up.sql Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Show resolved Hide resolved
slowli
slowli reviewed Feb 9, 2024
View reviewed changes
core/lib/types/src/commitment/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/eth_sender/eth_tx_aggregator.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/genesis.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/metadata_calculator/mod.rs Show resolved Hide resolved
core/lib/zksync_core/src/metadata_calculator/updater.rs Show resolved Hide resolved
@perekopskiy perekopskiy requested a review from slowli February 12, 2024 13:17
slowli
slowli previously approved these changes Feb 12, 2024
View reviewed changes
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.

Looks really good! Optimally, there should be a couple of unit tests for the main component, but integration tests give some level of assurance that it works.

core/lib/zksync_core/src/commitment_generator/mod.rs Show resolved Hide resolved
core/lib/zksync_core/src/commitment_generator/mod.rs Show resolved Hide resolved
RomanBrodetski
RomanBrodetski previously approved these changes Feb 12, 2024
View reviewed changes
@perekopskiy
Copy link
Contributor Author

perekopskiy commented Feb 13, 2024 •
edited
Loading

Optimally, there should be a couple of unit tests for the main component, but integration tests give some level of assurance that it works.

Created a task, will address soon

@perekopskiy perekopskiy dismissed stale reviews from RomanBrodetski and slowli via 6ac51c5 February 13, 2024 09:23
@perekopskiy perekopskiy added this pull request to the merge queue Feb 13, 2024
Deniallugo
Deniallugo reviewed Feb 13, 2024
View reviewed changes
core/lib/types/src/commitment/mod.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move it out? To some new library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally yes, but I didn't take into in a scope of this PR

core/lib/zksync_core/src/commitment_generator/mod.rs
Comment on lines +80 to +86
let events_queue_commitment = events_commitment_task
.await
.context("`events_commitment_task` failed")??;
let bootloader_initial_content_commitment = bootloader_memory_commitment_task
.await
.context("`bootloader_memory_commitment_task` failed")??;

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to spawn them in parallel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, events commitment take around 500ms on mainnet at the moment and bootloader memory around 2s, so we 500ms by running in parallel

Merged via the queue into main with commit 60305ba Feb 13, 2024
29 of 30 checks passed
@perekopskiy perekopskiy deleted the perekopskiy-commitment-component branch February 13, 2024 10:30
@zksync-era-bot zksync-era-bot mentioned this pull request Feb 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 16, 2024
@zksync-era-bot
chore(main): release core 20.7.0 (#1049)
98f87cd 
🤖 I have created a release *beep* *boop*
---


##
[20.7.0](core-v20.6.0...core-v20.7.0)
(2024-02-16)


### Features

* Add input field to CallRequest
([#1069](#1069))
([5087121](5087121))
* **api:** Remove unused and obsolete token info
([#1071](#1071))
([e920897](e920897))
* Better errors for JsonRPC calls
([#1002](#1002))
([079f999](079f999))
* **commitment:** Commitment component
([#1024](#1024))
([60305ba](60305ba))
* **en:** Make snapshots applier resilient and process storage log
chunks in parallel
([#1036](#1036))
([805218c](805218c))
* **node_framework:** Resources and layers for ETH clients
([#1074](#1074))
([776337a](776337a))
* **node_framework:** Support StateKeeper in the framework
([#1043](#1043))
([a80fff2](a80fff2))


### Bug Fixes

* **api:** Return on duplicate earlier
([#1059](#1059))
([cfa5701](cfa5701))
* **contract-verifier:** Use optimizer mode in solidity-single-file
verification
([#1079](#1079))
([fdab638](fdab638))
* Token distribution
([#1051](#1051))
([bd63b3a](bd63b3a))

---
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
Reviewers

@Deniallugo Deniallugo Deniallugo left review comments

@StanislavBreadless StanislavBreadless StanislavBreadless approved these changes

@artmakh artmakh artmakh approved these changes

@slowli slowli slowli left review comments

@RomanBrodetski RomanBrodetski RomanBrodetski left review comments

@yorik yorik Awaiting requested review from yorik yorik is a code owner automatically assigned from matter-labs/devops

@alexandrst88 alexandrst88 Awaiting requested review from alexandrst88 alexandrst88 is a code owner automatically assigned from matter-labs/devops

@hatemosphere hatemosphere Awaiting requested review from hatemosphere hatemosphere is a code owner automatically assigned from matter-labs/devops

@onyxet onyxet Awaiting requested review from onyxet onyxet is a code owner automatically assigned from matter-labs/devops

@otani88 otani88 Awaiting requested review from otani88 otani88 is a code owner automatically assigned from matter-labs/devops

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

Successfully merging this pull request may close these issues.
6 participants
@perekopskiy @RomanBrodetski @Deniallugo @slowli @artmakh @StanislavBreadless