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(state-keeper): miniblock max payload size (BFT-417) #1284

Merged
merged 17 commits into from
Apr 17, 2024
Merged

feat(state-keeper): miniblock max payload size (BFT-417) #1284

merged 17 commits into from
Apr 17, 2024

Conversation

moshababo
Copy link
Contributor

@moshababo moshababo commented Feb 29, 2024 •
edited

What ❔

Adding miniblock sealing according to max payload size.

Why ❔

Compliance with max payload size imposed in consensus modules.

Notes

  • Building the payload incrementally isn’t feasible because the miniblock’s transactions are flushed to DB before being reconstructed to be encoded as the payload. So the payload size accumulator added to MiniblockUpdates is just a heuristic done by encoding every executed transaction only to find out the estimated encoding size.
  • miniblock_max_payload_size was added to StateKeeperConfig, decoupled from max_payload_size in consensus::config::Config. It is possible to only use the latter, or to make the latter define/override the former. The state keeper max size should be less than 1/2 the consensus max size, because it never rejects the latest transaction, and doesn’t account for some metadata fields. Actual value in that respect was not set yet, because I wasn’t sure about values used for different non-test environments, or if we even want to keep this approach.
  • MiniblockMaxPayloadSizeSealer was added, implementing should_seal_miniblock() alone (without should_seal_l1_batch_unconditionally, as in trait IoSealCriteria). This isn’t an ideal solution, so other ideas are welcome.

The applied config are:

  • Statekeeper threshold: 1mb
  • Consensus threshold: 2.5mb

Closing BFT-417

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.

@moshababo moshababo marked this pull request as ready for review March 6, 2024 00:42
@pompon0 pompon0 requested a review from popzxc March 6, 2024 12:45
slowli
slowli reviewed Mar 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.

Future work: It would probably make sense to somehow de-prioritize / space out large transactions so that they don't significantly increase latency of processing other transactions. (Miniblocks must have distinct timestamps, so if a large transaction causes a miniblock to be sealed early, the latency of the transactions following it in the mempool would suffer.)

core/lib/zksync_core/src/state_keeper/seal_criteria/mod.rs Show resolved Hide resolved
core/lib/zksync_core/src/state_keeper/seal_criteria/mod.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/state_keeper/updates/miniblock_updates.rs Outdated Show resolved Hide resolved
core/lib/zksync_core/src/state_keeper/updates/miniblock_updates.rs Show resolved Hide resolved
pompon0
pompon0 reviewed Mar 11, 2024
View reviewed changes
etc/env/base/chain.toml Outdated Show resolved Hide resolved
@moshababo
Copy link
Contributor Author

The newly applied config are:

  • Statekeeper threshold: 1mb
  • Consensus threshold: 2.5mb

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.

Generally LGTM except for public models module.

core/lib/dal/src/lib.rs Outdated Show resolved Hide resolved
@moshababo
Create higher-level, public consensus module
b3f943e
@moshababo
Merge branch 'main' into miniblock_max_payload_size
8f8982e 
# Conflicts:
#	core/lib/config/src/testonly.rs
#	core/lib/dal/src/consensus_dal.rs
#	core/lib/dal/src/lib.rs
#	core/lib/dal/src/models/mod.rs
#	core/lib/protobuf_config/src/proto/chain.proto
#	core/lib/zksync_core/src/state_keeper/io/mempool.rs
#	etc/env/base/chain.toml
@moshababo
Post-merge fixes
fa3876d
@moshababo
Add config field
b28a2a6
@moshababo
Merge branch 'main' into miniblock_max_payload_size
a22ce74 
# Conflicts:
#	core/lib/protobuf_config/src/proto/chain.proto
@moshababo moshababo requested review from popzxc and pompon0 April 9, 2024 13:57
pompon0
pompon0 previously approved these changes Apr 10, 2024
View reviewed changes
@moshababo moshababo added this pull request to the merge queue Apr 17, 2024
Merged via the queue into main with commit a3c8e81 Apr 17, 2024
41 checks passed
@moshababo moshababo deleted the miniblock_max_payload_size branch April 17, 2024 13:45
@zksync-era-bot zksync-era-bot mentioned this pull request Apr 17, 2024
Merged
RomanBrodetski pushed a commit that referenced this pull request Apr 22, 2024
@zksync-era-bot @perekopskiy
chore(main): release core 23.1.0 (#1709)
d95a1fd 
🤖 I have created a release *beep* *boop*
---


##
[23.1.0](core-v23.0.0...core-v23.1.0)
(2024-04-22)


### Features

* **en:** Add boxed L2 client and use it in DI
([#1627](#1627))
([9948187](9948187))
* Extract block_reverter into separate crate
([#1632](#1632))
([8ab2488](8ab2488))
* Extract house keeper into separate crate
([#1685](#1685))
([f6f49b7](f6f49b7))
* remove enum index migration
([#1734](#1734))
([13c0f52](13c0f52))
* **state-keeper:** miniblock max payload size (BFT-417)
([#1284](#1284))
([a3c8e81](a3c8e81))


### Bug Fixes

* **en:** Fix miscellaneous snapshot recovery nits
([#1701](#1701))
([13bfecc](13bfecc))
* ensure two connections for both executor and async catchup
([#1755](#1755))
([3b14a9f](3b14a9f))
* made consensus store certificates asynchronously from statekeeper
([#1711](#1711))
([d1032ab](d1032ab))
* **merkle_tree:** don't panic in `BlockOutputWithProofs::verify_proofs`
([#1717](#1717))
([a44fac9](a44fac9))
* **types:** fix LegacyCall type
([#1739](#1739))
([712919f](712919f))


### Performance Improvements

* **en:** Monitor recovery latency by stage
([#1725](#1725))
([d7efdd5](d7efdd5))


### Reverts

* **env:** Remove `ZKSYNC_HOME` env var from server
([#1713](#1713))
([aed23e1](aed23e1))

---
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 <mikeson.dp@gmail.com>
Co-authored-by: perekopskiy <53865202+perekopskiy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slowli slowli slowli left review comments

@pompon0 pompon0 pompon0 approved these changes

@popzxc popzxc Awaiting requested review from popzxc

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@moshababo @pompon0 @slowli @popzxc