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: Port boojum eth-sender changes #293

Merged
merged 10 commits into from
Oct 31, 2023
Merged

feat: Port boojum eth-sender changes #293

merged 10 commits into from
Oct 31, 2023

Conversation

perekopskiy
Copy link
Contributor

@perekopskiy perekopskiy commented Oct 23, 2023 •
edited
Loading

What ❔

  • MetadataCalculatorModeConfig::Full changed so it optionally saves artifacts for witness to GCS, EN runs metadata calculator in full mode, so it has all commitments
  • Circuit breaker for facet selectors removed -- it won't be possible to perform an upgrade with it running.
  • Eth-sender, block-reverter, consistency-checker use L1 contract function that corresponds to the protocol version of L1 batch it's used for.

Why ❔

Preparation for boojum upgrade

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 24, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (c32b88f) 35.86% compared to head (42d817a) 35.84%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
- Coverage   35.86%   35.84%   -0.02%     
==========================================
  Files         521      519       -2     
  Lines       27844    27816      -28     
==========================================
- Hits         9986     9971      -15     
+ Misses      17858    17845      -13
Files Coverage Δ
core/bin/external_node/src/main.rs 0.00% <ø> (ø)
core/lib/circuit_breaker/src/lib.rs 0.00% <ø> (ø)
core/lib/commitment_utils/src/lib.rs 44.44% <100.00%> (ø)
core/lib/config/src/configs/database.rs 61.53% <ø> (ø)
core/lib/types/src/protocol_version.rs 86.36% <ø> (ø)
core/lib/zksync_core/src/block_reverter/mod.rs 0.00% <ø> (ø)
...ore/lib/zksync_core/src/consistency_checker/mod.rs 0.00% <ø> (ø)
core/lib/zksync_core/src/lib.rs 8.12% <ø> (ø)
...ore/lib/zksync_core/src/metadata_calculator/mod.rs 87.75% <100.00%> (+0.25%) ⬆️
core/lib/types/src/commitment.rs 71.00% <93.33%> (+3.67%) ⬆️
... and 4 more

... and 25 files with indirect coverage changes

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

StanislavBreadless
StanislavBreadless previously approved these changes Oct 25, 2023
View reviewed changes
core/lib/contracts/src/lib.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/consistency_checker/mod.rs Outdated Show resolved Hide resolved
Deniallugo
Deniallugo reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@Deniallugo Deniallugo left a comment

Choose a reason for hiding this comment

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

Let's discuss the idea with generics over the encoding data for sending it to L1.

Seems pretty useful for future changes

core/lib/contracts/src/lib.rs Outdated Show resolved Hide resolved
core/lib/types/src/commitment.rs Show resolved Hide resolved
core/lib/zksync_core/src/consistency_checker/mod.rs Outdated Show resolved Hide resolved
@akash-chandrakar
Copy link
Contributor

@perekopskiy I think I didn't follow this comment:

MetadataCalculatorModeConfig::Full changed so it optionally saves artifacts for witness to GCS

If I am reading it right it means the witness-inputs to GCS are not always present, if that's the case do you think it will prevent us from generating proof for batches where the GCS witness input data is not present.

@perekopskiy
Copy link
Contributor Author

@akash-chandrakar I meant, that made saving to GCS configurable. It must be enabled for main server but disabled for EN

@perekopskiy perekopskiy marked this pull request as ready for review October 26, 2023 13:47
@perekopskiy perekopskiy requested a review from a team as a code owner October 26, 2023 13:47
Deniallugo
Deniallugo approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@Deniallugo Deniallugo left a comment

Choose a reason for hiding this comment

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

I'm not fully satisfied with the current state.
I still want to improve upgrade on ETH-Senders side. Now it's a bit painfull.

And we have to think more about how to develop features such as boojum in a nicer way.

@perekopskiy perekopskiy added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main with commit 8027326 Oct 31, 2023
24 checks passed
@perekopskiy perekopskiy deleted the boojum-eth-sender-changes branch October 31, 2023 08:15
@zksync-era-bot zksync-era-bot mentioned this pull request Oct 31, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 3, 2023
@zksync-era-bot @perekopskiy
chore(main): release core 17.1.0 (#337)
ba88f67 
🤖 I have created a release *beep* *boop*
---


##
[17.1.0](core-v16.2.0...core-v17.1.0)
(2023-11-03)


### ⚠ BREAKING CHANGES

* Update to protocol version 17
([#384](#384))

### Features

* **en:** Cache blocks in `fetch_l2_block`
([#403](#403))
([b94c845](b94c845))
* Port boojum eth-sender changes
([#293](#293))
([8027326](8027326))
* **state-keeper:** Disable some seal criteria for boojum
([#390](#390))
([2343532](2343532))
* Update to protocol version 17
([#384](#384))
([ba271a5](ba271a5))
* **vm:** Make calculation for pubdata a bit more percise
([#392](#392))
([6d0e61c](6d0e61c))


### Bug Fixes

* bump zksolc from yanked version to 1.3.16
([#348](#348))
([c32b88f](c32b88f))
* **db-index:** Add missing index from FRI prover jobs
([#334](#334))
([730447f](730447f))
* **db-query:** use join instead of nested query for FRI prover
extracting
([#364](#364))
([f9cc831](f9cc831))
* **db-query:** use nested query for requeuing FRI prover jobs
([#399](#399))
([3890542](3890542))
* incorrect directory of intrinsic.rs generated.
([#332](#332))
([#336](#336))
([eefaad0](eefaad0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: perekopskiy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Deniallugo Deniallugo Deniallugo approved these changes

@mm-zk mm-zk Awaiting requested review from mm-zk

@ly0va ly0va Awaiting requested review from ly0va

@StanislavBreadless StanislavBreadless Awaiting requested review from StanislavBreadless

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
@perekopskiy @akash-chandrakar @Deniallugo @StanislavBreadless