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

Check compressed sequencer commitments #1349

Merged
merged 17 commits into from
Oct 17, 2024

Conversation

rakanalh
Copy link
Contributor

@rakanalh rakanalh commented Oct 16, 2024

Description

This PR uses TaskManager to spawn a new task for running CommitmentService which in turn handles everything related to submitting sequencer commitments.

Detailed changes in this PR

  • Move commitment controller logic & some parts of the state diff threshold into CommitmentStrategy structs.
  • Move logic for running commitment strategies into CommitmentController
  • Use CommitmentService for submitting commitments if CommitmentController advises to.
  • Move sequencer.rs into runner.rs for consistency with prover / full node.
  • Simplify the sequencer runner code base.

How it works

  • Sequencer produces L2 block
  • Sequencer sends height & state diff to CommitmentService
  • CommitmentService consults with CommitmentController whether it should commit or not.
  • CommitmentController runs each CommitmentStrategy and takes whichever returns CommitmentInfo
  • CommitmentService submits commitment if advised to do so.

One more thing to pay attention to, is that the StateDiffThreshold commitment strategy now uses brotli for compressing the state diff before deciding on the threshold. This is a CPU heavy operation which should run in it's own task.

Linked Issues

@rakanalh rakanalh changed the title Rakanalh/compress sequencer commitments Check compressed sequencer commitments Oct 16, 2024
@rakanalh rakanalh marked this pull request as ready for review October 16, 2024 21:07
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 90.03831% with 26 lines in your changes missing coverage. Please review.

Project coverage is 78.2%. Comparing base (8ea8350) to head (567de4a).
Report is 1 commits behind head on nightly.

Files with missing lines Patch % Lines
crates/sequencer/src/commitment/mod.rs 83.2% 25 Missing ⚠️
crates/sequencer/src/commitment/strategy.rs 99.0% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/bitcoin-da/src/helpers/builders/tests.rs 98.0% <ø> (ø)
crates/bitcoin-da/src/helpers/mod.rs 72.0% <ø> (ø)
crates/bitcoin-da/src/service.rs 50.7% <ø> (ø)
crates/primitives/src/compression.rs 100.0% <ø> (ø)
crates/primitives/src/constants.rs 0.0% <ø> (ø)
crates/prover/src/da_block_handler.rs 75.0% <ø> (ø)
crates/sequencer/src/lib.rs 64.2% <ø> (ø)
crates/sequencer/src/runner.rs 94.0% <100.0%> (ø)
crates/sequencer/src/commitment/strategy.rs 99.0% <99.0%> (ø)
crates/sequencer/src/commitment/mod.rs 83.2% <83.2%> (ø)

... and 2 files with indirect coverage changes

Copy link
Member

@eyusufatik eyusufatik left a comment

Choose a reason for hiding this comment

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

very solid PR

crates/common/src/compression.rs Outdated Show resolved Hide resolved
@rakanalh rakanalh requested review from eyusufatik and kpp October 17, 2024 14:18
@eyusufatik eyusufatik merged commit 7c47f06 into nightly Oct 17, 2024
14 checks passed
@eyusufatik eyusufatik deleted the rakanalh/compress-sequencer-commitments branch October 17, 2024 17:04
@kpp kpp mentioned this pull request Dec 4, 2024
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.

Check compressed state diff size before sending seq_commtiments
3 participants