Skip to content

feat: Add limit for pending DA submission blocks - #1609

Merged
MSevey merged 3 commits into
mainfrom
tzdybal/max_pending2
Mar 28, 2024
Merged

feat: Add limit for pending DA submission blocks#1609
MSevey merged 3 commits into
mainfrom
tzdybal/max_pending2

Conversation

@tzdybal

@tzdybal tzdybal commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

Overview

A new function has been added to determine the number of pending blocks for DA submission, alongside a new config parameter to set a limit on this. If this limit is reached, the block production process gets paused. The necessary tests and command flags have also been included.

Resolves #1524

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

Summary by CodeRabbit

  • New Features
    • Introduced a limit on the number of blocks pending submission to enhance system stability and manageability.
    • Added a new configuration option to set the maximum number of pending blocks during system initialization.
  • Tests
    • Implemented new integration tests to verify the block submission limit functionality.
  • Documentation
    • Updated documentation to include guidance on configuring the maximum number of pending blocks.

@coderabbitai

coderabbitai Bot commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

Walkthrough

The recent changes introduce a mechanism to limit the number of pending blocks in a blockchain system, preventing the accumulation of unsubmitted blocks to the Data Availability (DA) layer. By configuring a maximum threshold, the system now can halt new block creation when the pending block count exceeds this limit, ensuring the DA layer has the opportunity to catch up and maintain system integrity.

Changes

File Path Change Summary
block/manager.go, block/pending_blocks.go Added a check in publishBlock to limit pending blocks and a method numPendingBlocks() to count them.
cmd/rollkit/docs/rollkit_start.md Introduced a flag --rollkit.max_pending_blocks to set a pending block limit during rollkit start.
config/config.go Added FlagMaxPendingBlocks to control the pending block limit.
node/full_node_integration_test.go Enhanced testing with imports, a new test function TestMaxPending, and a helper doTestMaxPending.

Assessment against linked issues

Objective Addressed Explanation
Stop pending blocks from piling up (#1524)
Ensure rollup blockchain does not progress when DA submission is broken (#1524)
Sequencer should stop building new blocks if pending blocks cap is reached (#1524)

Poem

In a world where blocks would stack,
A rabbit hopped, no looking back.
With code in hand and wisdom grand,
It set a limit, took a stand.

🐰 "No more," it said, "shall blocks just pile,
A cap in place, with grace and style.
Let DA catch up, let's not rush,
Ensure our chain remains robust and plush."

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

tzdybal added 2 commits March 27, 2024 21:37
A new function has been added to determine the number of pending blocks for DA submission, alongside a new config parameter to set a limit on this. If this limit is reached, the block production process gets paused. The necessary tests and command flags have also been included.
@tzdybal
tzdybal force-pushed the tzdybal/max_pending2 branch from c05df65 to 95784bf Compare March 27, 2024 20:37
This commit adds a function to test managing the maximum number of pending blocks. This includes cases where the limit is set to no limit, 10 pending blocks, and 50 pending blocks. This function confirms the application correctly pauses after reaching the max limit and resumes accepting blobs after changing the mock function to start accepting.
@tzdybal
tzdybal force-pushed the tzdybal/max_pending2 branch from 33d9e2e to a42442b Compare March 27, 2024 21:38
@tzdybal
tzdybal marked this pull request as ready for review March 27, 2024 22:01

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between be8c028 and a42442b.
Files selected for processing (6)
  • block/manager.go (1 hunks)
  • block/pending_blocks.go (1 hunks)
  • block/pending_blocks_test.go (1 hunks)
  • cmd/rollkit/docs/rollkit_start.md (1 hunks)
  • config/config.go (4 hunks)
  • node/full_node_integration_test.go (3 hunks)
Additional comments: 7
block/pending_blocks_test.go (1)
  • 100-100: The addition of the assertion to check the equality of the length of blocks and the result of pb.numPendingBlocks() is a valuable enhancement to the test suite. It ensures that the numPendingBlocks method's behavior is correctly verified, contributing to the overall robustness of the testing framework.
block/pending_blocks.go (1)
  • 82-84: The addition of the numPendingBlocks method is a crucial enhancement for effectively managing the flow of block submission. It ensures that the system can dynamically assess the number of pending blocks, supporting the newly introduced feature of limiting pending blocks. This method is implemented efficiently, using atomic operations for thread safety.
cmd/rollkit/docs/rollkit_start.md (1)
  • 43-43: The documentation update to include the --rollkit.max_pending_blocks uint flag is clear and informative, accurately reflecting the new feature that allows users to set a limit on blocks pending DA submission. This addition enhances the usability of the system by providing more control over the block submission process.
config/config.go (1)
  • 37-38: The introduction of the FlagMaxPendingBlocks configuration flag is a critical update that supports the new feature of limiting pending blocks. This change allows for dynamic configuration of the system, enhancing its flexibility and usability. The implementation is consistent with the existing configuration system's design and follows best practices.
node/full_node_integration_test.go (2)
  • 411-435: The addition of the TestMaxPending function and its associated test cases is a valuable enhancement to the test suite. It ensures comprehensive testing of the new feature under various conditions, contributing significantly to the feature's robustness. The approach taken in these tests is thorough and well-considered, effectively simulating different system states to validate the feature's functionality.
  • 437-492: The doTestMaxPending helper function is a critical component of the testing strategy, facilitating the detailed examination of the system's behavior when the limit of pending blocks is reached. The use of a mock DA client to simulate DA unavailability and subsequent availability is particularly noteworthy, as it allows for a realistic assessment of the feature's behavior in practical scenarios. This function's implementation is exemplary of effective integration testing practices.
block/manager.go (1)
  • 718-721: The implementation of the pending block limit check is correct and aligns with the PR objectives. However, consider the following improvements:
  • Error Handling: The error message is clear, but it might be beneficial to log this event for easier troubleshooting.
  • Magic Number: The check m.conf.MaxPendingBlocks != 0 uses 0 as a magic number to indicate no limit. Consider defining a constant for clarity, e.g., NoLimit.
  • Documentation: Adding a comment explaining the purpose of this check could improve code readability and maintainability.

@Manav-Aggarwal Manav-Aggarwal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall, looks great, left some comments.

Comment thread node/full_node_integration_test.go
Comment thread node/full_node_integration_test.go
@MSevey
MSevey enabled auto-merge March 28, 2024 17:19

@tuxcanfly tuxcanfly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK.

@Manav-Aggarwal Manav-Aggarwal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@MSevey
MSevey added this pull request to the merge queue Mar 28, 2024
Merged via the queue into main with commit b5b58bc Mar 28, 2024
@MSevey
MSevey deleted the tzdybal/max_pending2 branch March 28, 2024 17:55
@tac0turtle tac0turtle removed this from Evolve Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stop pending blocks from piling up

5 participants