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: Separate Prover and Server DAL #1334

Merged
merged 52 commits into from
Mar 20, 2024
Merged

feat: Separate Prover and Server DAL #1334

merged 52 commits into from
Mar 20, 2024

Conversation

Artemka374
Copy link
Contributor

@Artemka374 Artemka374 commented Mar 2, 2024 •
edited
Loading

What ❔

  • New db_connection crate, which contains all the main logic for managing ConnectionPool and StorageProcessor
  • Prover parts of DAL are moved to prover_dal crate
  • Now we have typed ConnectionPools and StorageProcessors, to use appropriate ConnectionPool there are 2 options:
    ConnectionPool<Server>, ConnectionPool<Prover> - which will return appropriate StorageProcessor with corresponding methods accessible only by this connection pool.
  • vm_version, prover_dal and part of protocol_version types were moved to basic_types crate

Further steps:

  • Add migrations for dropping tables that are not needed in databases, done here.
  • Rename Server to Core, StorageProcessor to Connection

Why ❔

To separate logic of prover and server. To prevent accessing wrong database with one ConnectionPool.

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.

@Artemka374 Artemka374 changed the title feat: Separate Prover and Server Dal's feat: Separate Prover and Server DAL Mar 2, 2024
@Artemka374 Artemka374 changed the title feat: Separate Prover and Server DAL [WIP] feat: Separate Prover and Server DAL Mar 2, 2024
@Artemka374 Artemka374 mentioned this pull request Mar 8, 2024
Closed
6 tasks
Artemka374 added 20 commits March 8, 2024 17:34
@Artemka374
few more updates
ff4e9c7
@Artemka374
Merge branch 'main' into afo/dal-refactoring
76de3c5 
# Conflicts:
#	core/lib/dal/.sqlx/query-204cfd593c62a5a1582215a5f0f4d3648b75bf01ff336bbd77d15f9aa5fd6443.json
#	core/lib/dal/.sqlx/query-23be43bf705d679ca751c89353716065fcad42c6b621efb3a135a16b477dcfd9.json
#	core/lib/dal/.sqlx/query-2a2680234c38904e5c19df45193a8c13d04079683e09c65f7f4e76a9987e2ab4.json
#	core/lib/dal/.sqlx/query-5659480e5d79dab3399e35539b240e7eb9f598999c28015a504605f88bf84b33.json
#	core/lib/dal/.sqlx/query-6692ff6c0fbb2fc94f5cd2837a43ce80f9b2b27758651ccfc09df61a4ae8a363.json
#	core/lib/dal/.sqlx/query-75f6eaa518e7840374c4e44b0788bf92c7f2c55386c8208e3a82b30456abd5b4.json
#	core/lib/dal/.sqlx/query-c02f404ce9b0f92b8052ef6f3eaabda70cb9c56ae3e30dc0a8257e43d6714155.json
#	core/lib/dal/.sqlx/query-cb0a9f6137fb6bee5d17d644714b3b22ea2cd184932fcd59f5931239c7a78003.json
#	core/lib/dal/.sqlx/query-d6b70256793417a949081899eccf75260c7afaf110870656061a04079c35c2d8.json
#	core/lib/dal/.sqlx/query-e3479d12d9dc97001cf03dc42d9b957e92cd375ec33fe16f855f319ffc0b208e.json
#	core/lib/dal/.sqlx/query-fe06e06c04466429bb85709e6fe8dd6c2ad2793c06071f4a067dcc31306adebc.json
#	core/lib/dal/Cargo.toml
#	core/lib/dal/src/consensus_dal.rs
#	core/lib/dal/src/system_dal.rs
#	core/lib/db_connection/src/lib.rs
#	prover/prover_dal/src/fri_scheduler_dependency_tracker_dal.rs
@Artemka374
move some files around
b8fa7e9
@Artemka374
structurize separated files
2996367
@Artemka374
rename traits, types, fix some imports
e291ebc
@Artemka374
make db_connection build
1bb55c5
@Artemka374
make prover_dal compile
2060921
@Artemka374
fmt/build sqlx
ccd0bc9
@Artemka374
make dal build
24ebf2b
@Artemka374
first wave of renaming/type assigning
66b51e2
@Artemka374
Merge branch 'main' into afo/dal-refactoring
b82ee58
@Artemka374
second wave of type assigning, fix test in db_connection
7c432a1
@Artemka374
Merge remote-tracking branch 'origin/afo/dal-refactoring' into afo/da…
59edfcd 
…l-refactoring
@Artemka374
use appropriate processors
2cfac71
@Artemka374
some more type assigning
cda5ba2
@Artemka374
make core build
2579f47
@Artemka374
Merge branch 'main' into afo/dal-refactoring
ec6e38b
@Artemka374
fix dal test
9d8b17d
@Artemka374
fix prover tests
0742352
@Artemka374
fix build in prover parts, uncomment global config
481e35d
@Artemka374
Copy link
Contributor Author

Although the PR is pretty big, here are some main points for reviewing(because PR is big mostly because of renaming/import changes):

  • db_connection crate - most of the logic moved there and generalized, although some of the metrics are exposed to be properly used in DAL crates. Also, Postgres metrics were left in the core dal crate, because they are accessing database.
  • Changes in zksync_types - prover types are moved from fri_prover_dal there, because they are being accessed from different crates
  • lib.rs files in core dal and prover_dal crates - that's where main logic for typed storage processors are implemented.

@Artemka374
Copy link
Contributor Author

Also a few points worth discussion:

  • Right now dal crates are reexporting some types/traits from db_connection crate - it was done mostly because most of the crates are not using both prover and core DALs - except witness generator and house keeper - not sure, but probably it is still better not to reexport types to prevent confusion.
  • global_config method in ConnectionPool seems to be static and not depending on type of ConnectionPool - it generally makes sense to move it out of the ConnectionPool struct, but not sure that it worth doing in this exact PR - probably we can do it in the other one.
  • Server/ServerProcessor and Prover/ProverProcessor struct can be generally united - but I don't think we want to use ConnectionPool<ServerProcessor> or ConnectionPool -> Server syntax
  • Any renaming suggestions are also appreciated

@EmilLuta
Copy link
Contributor

I gave it a diagonal look. The split overall makes sense, but something still rubs me the wrong way. It feels more complicated than it has to be. That said, I see no alternative. Will leave it up to @popzxc to figure out how to make this interface nicer. Also, let's fix conflicts and what not so we can be merge ready.

@Artemka374
Copy link
Contributor Author

The problem with merges is that I need to fix it every few hours, because PR itself touches a lot of places and almost every merge to main causes conflicts. I will merge it for now, but I think will do it once a day if it will take much time for this one to merge

@EmilLuta
Copy link
Contributor

Oh, I see. No need. Let's give Igor a bit of time and only do a merge when we're ready to fully merge. Keeping it up to date might be more pain than necessary.

popzxc
popzxc reviewed Mar 19, 2024
View reviewed changes
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 looks good, don't want to nitpick too much given the huge scope.
This certainly feels like a good starting point, well done 👍

core/lib/db_connection/src/healthcheck.rs Show resolved Hide resolved
core/lib/db_connection/src/processor.rs Outdated Show resolved Hide resolved
core/lib/dal/src/metrics.rs Show resolved Hide resolved
@Artemka374
move some types to basic_types
21c58e8
@Artemka374
Merge branch 'main' into afo/dal-refactoring
2457639 
# Conflicts:
#	core/lib/dal/Cargo.toml
#	core/lib/zksync_core/src/consensus/storage/mod.rs
#	core/lib/zksync_core/src/consensus/testonly.rs
#	core/lib/zksync_core/src/utils/testonly.rs
@Artemka374
post merge fixes
4467c0a
popzxc
popzxc previously approved these changes Mar 19, 2024
View reviewed changes
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 think it's in a good enough state to merge it. Minor things will get polished over time (hehe).

EmilLuta
EmilLuta approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@EmilLuta EmilLuta left a comment

Choose a reason for hiding this comment

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

This looks really good. Let's get it merged!

core/bin/contract-verifier/src/main.rs Show resolved Hide resolved
core/lib/zksync_core/src/api_server/contract_verification/mod.rs Show resolved Hide resolved
core/lib/zksync_core/Cargo.toml Show resolved Hide resolved
prover/prover_fri/src/prover_job_processor.rs Show resolved Hide resolved
@Artemka374 Artemka374 enabled auto-merge March 20, 2024 09:24
@Artemka374 Artemka374 added this pull request to the merge queue Mar 20, 2024
Merged via the queue into main with commit 103a56b Mar 20, 2024
38 checks passed
@Artemka374 Artemka374 deleted the afo/dal-refactoring branch March 20, 2024 09:45
This was referenced Mar 20, 2024
Merged
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 22, 2024
@zksync-era-bot
chore(main): release core 22.0.0 (#1424)
14a8b78 
🤖 I have created a release *beep* *boop*
---


##
[22.0.0](core-v21.1.0...core-v22.0.0)
(2024-03-21)


### ⚠ BREAKING CHANGES

* Use protocol version v22 as latest
([#1432](#1432))

### Features

* add docs for Dal
([#1273](#1273))
([66ceb0b](66ceb0b))
* adds debug_traceBlockByNumber.callFlatTracer
([#1413](#1413))
([d2a5e36](d2a5e36))
* **api:** introduce mempool cache
([#1460](#1460))
([c5d6c4b](c5d6c4b))
* **commitment-generator:** `events_queue` shadow mode
([#1138](#1138))
([9bb47fa](9bb47fa))
* **contract-verifier:** Allow sc code reverification
([#1455](#1455))
([5a37b42](5a37b42))
* **database:** add an optional master replica max connections settings
([#1472](#1472))
([e5c8127](e5c8127))
* **db:** Configurable maximum number of open RocksDB files
([#1401](#1401))
([b00c052](b00c052))
* **en:** Check recipient contract and function selector in consistency
checker ([#1367](#1367))
([ea5c684](ea5c684))
* Follow-up for DAL split
([#1464](#1464))
([c072288](c072288))
* **node_framework:** Ergonomic improvements
([#1453](#1453))
([09b6887](09b6887))
* **node_framework:** Only store each wiring layer once
([#1468](#1468))
([4a393dc](4a393dc))
* **node_framework:** Support for preconditions and oneshot tasks
([#1398](#1398))
([65ea881](65ea881))
* **node-framework:** Add eth sender layer
([#1390](#1390))
([0affdf8](0affdf8))
* **node-framework:** Add housekeeper layer
([#1409](#1409))
([702e739](702e739))
* Remove batch dry_run
([#1076](#1076))
([b82d093](b82d093))
* Separate Prover and Server DAL
([#1334](#1334))
([103a56b](103a56b))
* **storage:** Make the storage caches task cancellation aware
([#1430](#1430))
([ab532bb](ab532bb))
* support running consensus from snapshot (BFT-418)
([#1429](#1429))
([f9f4d38](f9f4d38))
* **tx-sender:** Limit concurrent tx submissions
([#1473](#1473))
([4bdf3ca](4bdf3ca))
* Use protocol version v22 as latest
([#1432](#1432))
([1757412](1757412))
* **vm:** Prestate tracer implementation
([#1306](#1306))
([c36be65](c36be65))


### Bug Fixes

* **core:** drop correct index of proof_generation_details table during
database migration
([#1199](#1199))
([76a6821](76a6821))
* **dal:** correct `tx_index_in_l1_batch` in
`get_vm_events_for_l1_batch`
([#1463](#1463))
([8bf37ac](8bf37ac))
* **node_framework:** Fix main node example
([#1470](#1470))
([ac4a744](ac4a744))
* **protocol:** Remove verifier address from protocol upgrade
([#1443](#1443))
([90dee73](90dee73))
* **reth:** use reth instead of geth
([#1410](#1410))
([bd98dc7](bd98dc7))
* **verified sources fetcher:** Use correct `contact_name` for
`SolSingleFile`
([#1433](#1433))
([0764227](0764227))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
montekki pushed a commit that referenced this pull request Mar 22, 2024
@zksync-era-bot @montekki
chore(main): release core 22.0.0 (#1424)
9b35900 
🤖 I have created a release *beep* *boop*
---


##
[22.0.0](core-v21.1.0...core-v22.0.0)
(2024-03-21)


### ⚠ BREAKING CHANGES

* Use protocol version v22 as latest
([#1432](#1432))

### Features

* add docs for Dal
([#1273](#1273))
([66ceb0b](66ceb0b))
* adds debug_traceBlockByNumber.callFlatTracer
([#1413](#1413))
([d2a5e36](d2a5e36))
* **api:** introduce mempool cache
([#1460](#1460))
([c5d6c4b](c5d6c4b))
* **commitment-generator:** `events_queue` shadow mode
([#1138](#1138))
([9bb47fa](9bb47fa))
* **contract-verifier:** Allow sc code reverification
([#1455](#1455))
([5a37b42](5a37b42))
* **database:** add an optional master replica max connections settings
([#1472](#1472))
([e5c8127](e5c8127))
* **db:** Configurable maximum number of open RocksDB files
([#1401](#1401))
([b00c052](b00c052))
* **en:** Check recipient contract and function selector in consistency
checker ([#1367](#1367))
([ea5c684](ea5c684))
* Follow-up for DAL split
([#1464](#1464))
([c072288](c072288))
* **node_framework:** Ergonomic improvements
([#1453](#1453))
([09b6887](09b6887))
* **node_framework:** Only store each wiring layer once
([#1468](#1468))
([4a393dc](4a393dc))
* **node_framework:** Support for preconditions and oneshot tasks
([#1398](#1398))
([65ea881](65ea881))
* **node-framework:** Add eth sender layer
([#1390](#1390))
([0affdf8](0affdf8))
* **node-framework:** Add housekeeper layer
([#1409](#1409))
([702e739](702e739))
* Remove batch dry_run
([#1076](#1076))
([b82d093](b82d093))
* Separate Prover and Server DAL
([#1334](#1334))
([103a56b](103a56b))
* **storage:** Make the storage caches task cancellation aware
([#1430](#1430))
([ab532bb](ab532bb))
* support running consensus from snapshot (BFT-418)
([#1429](#1429))
([f9f4d38](f9f4d38))
* **tx-sender:** Limit concurrent tx submissions
([#1473](#1473))
([4bdf3ca](4bdf3ca))
* Use protocol version v22 as latest
([#1432](#1432))
([1757412](1757412))
* **vm:** Prestate tracer implementation
([#1306](#1306))
([c36be65](c36be65))


### Bug Fixes

* **core:** drop correct index of proof_generation_details table during
database migration
([#1199](#1199))
([76a6821](76a6821))
* **dal:** correct `tx_index_in_l1_batch` in
`get_vm_events_for_l1_batch`
([#1463](#1463))
([8bf37ac](8bf37ac))
* **node_framework:** Fix main node example
([#1470](#1470))
([ac4a744](ac4a744))
* **protocol:** Remove verifier address from protocol upgrade
([#1443](#1443))
([90dee73](90dee73))
* **reth:** use reth instead of geth
([#1410](#1410))
([bd98dc7](bd98dc7))
* **verified sources fetcher:** Use correct `contact_name` for
`SolSingleFile`
([#1433](#1433))
([0764227](0764227))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
@zksync-era-bot @RomanBrodetski
chore(main): release prover 12.2.0 (#1439)
0999dea 
🤖 I have created a release *beep* *boop*
---


##
[12.2.0](prover-v12.1.0...prover-v12.2.0)
(2024-03-28)


### Features

* **api:** introduce mempool cache
([#1460](#1460))
([c5d6c4b](c5d6c4b))
* **commitment-generator:** `events_queue` shadow mode
([#1138](#1138))
([9bb47fa](9bb47fa))
* Drop prover tables in core database
([#1436](#1436))
([0d78122](0d78122))
* Follow-up for DAL split
([#1464](#1464))
([c072288](c072288))
* **prover:** export prover traces through OTLP
([#1427](#1427))
([16dce75](16dce75))
* **prover:** File-info tool to help prover debugging
([#1216](#1216))
([9759907](9759907))
* Separate Prover and Server DAL
([#1334](#1334))
([103a56b](103a56b))
* support running consensus from snapshot (BFT-418)
([#1429](#1429))
([f9f4d38](f9f4d38))


### Bug Fixes

* **api:** Fix API server shutdown flow
([#1425](#1425))
([780f6b0](780f6b0))
* **prover:** Remove FriProtocolVersionId
([#1510](#1510))
([6aa51b0](6aa51b0))

---
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: Roman Brodetski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perekopskiy perekopskiy perekopskiy approved these changes

@EmilLuta EmilLuta EmilLuta approved these changes

@popzxc popzxc popzxc left review comments

@slowli slowli Awaiting requested review from slowli

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
@Artemka374 @EmilLuta @popzxc @perekopskiy