docs: Add ADR for Mini Blocks #2236
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughA new Architecture Decision Record (ADR) has been added, proposing a design for streaming transaction batches as "mini blocks" from the sequencer to full nodes in the Rollkit system. This approach introduces a sub-block interval for mini blocks, allowing transactions to be propagated and tentatively applied by full nodes before finalization with the main block. The document outlines the necessary protocol and structural changes, alternative approaches considered, and potential impacts on network and state management. No changes to code or public APIs are included. Changes
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
|
Manav-Aggarwal
force-pushed
the
manav/add-mini-blocks-adr
branch
from
bcd4d55 to
ab123cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
specs/lazy-adr/adr-021-transaction-streaming.md (5)
9-9: Fix awkward phrasing for clarity.The sentence “There is desirable to propagate transactions to full nodes as sooner without waiting for execution or state root computation.” is grammatically incorrect and confusing. Consider rephrasing to:
- There is desirable to propagate transactions to full nodes as sooner without waiting for execution or state root computation. + It is desirable to propagate transactions to full nodes as soon as possible without waiting for execution or state root computation.
11-12: Add blank lines around headings and fenced code blocks.To comply with Markdown style (MD022, MD031) and improve readability, ensure there is an empty line before and after each heading and fenced code block. For example, insert a blank line above “### Current Architecture (Before)” and around the Mermaid code fences in both architecture diagrams.
Also applies to: 26-27, 87-88, 111-112, 135-136
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
12-12: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
77-77: Remove unnecessary comma in compound verb.In the sentence:
Full nodes will receive, and apply the transactions in these mini blocks…
the comma before “and” is not needed because it connects two verbs on the same subject. Change to:
- Full nodes will receive, and apply the transactions in these mini blocks to their state … + Full nodes will receive and apply the transactions in these mini blocks to their state …🧰 Tools
🪛 LanguageTool
[uncategorized] ~77-~77: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ... until the full block header is received and the transaction batches are seen on the...(COMMA_COMPOUND_SENTENCE_2)
118-124: Include security considerations.It would be valuable to add a subsection under “Additional Considerations” addressing security implications of streaming mini‐blocks, such as:
- Authenticity: verifying
sequencer_signature- Replay protection & rate limiting
- Potential attack surface on p2p channels
This will help ensure the design accounts for confidentiality, integrity, and availability concerns.
153-153: Ensure file ends with a single newline.Add a trailing newline at the end of the document to comply with MD047.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
153-153: Files should end with a single newline character
null(MD047, single-trailing-newline)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
specs/lazy-adr/adr-021-transaction-streaming.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/lazy-adr/adr-021-transaction-streaming.md
[grammar] ~53-~53: You’ve repeated a verb. Did you mean to only write one of them?
Context: ... Approaches 1. Immediate Transaction Broadcasting - Broadcast individual transactions immediately upo...
(REPEATED_VERBS)
[grammar] ~73-~73: This sentence seems to be incomplete. Insert a noun before ‘Are’ to make the sentence complete.
Context: ... received within a short time window. - Are not executed by the sequencer before st...
(MISSING_SUBJECT)
[uncategorized] ~77-~77: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ... until the full block header is received and the transaction batches are seen on the...
(COMMA_COMPOUND_SENTENCE_2)
🪛 markdownlint-cli2 (0.17.2)
specs/lazy-adr/adr-021-transaction-streaming.md
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
12-12: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
27-27: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
88-88: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
138-138: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
141-141: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
142-142: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
145-145: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
146-146: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
153-153: Files should end with a single newline character
null
(MD047, single-trailing-newline)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rollkit-docker / docker-build (GHCR; ghcr.io/rollkit/rollkit)
- GitHub Check: test / Run Unit Tests
- GitHub Check: test / Build All Rollkit Binaries
- GitHub Check: Analyze (go)
- GitHub Check: Summary
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2236 +/- ##
=======================================
Coverage 55.01% 55.01%
=======================================
Files 52 52
Lines 4508 4508
=======================================
Hits 2480 2480
Misses 1762 1762
Partials 266 266 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Manav-Aggarwal
force-pushed
the
manav/add-mini-blocks-adr
branch
from
4e14d4f to
7ad132b
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (7)
specs/lazy-adr/adr-021-transaction-streaming.md (7)
7-10: Improve sentence variety and clarity: The Context section has multiple sentences starting with “This” or “Currently.” Consider rephrasing to avoid repetitive openings and improve flow. For example, merge or reword the second and third sentences.🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nly shared after full block production. This ADR proposes a design where the sequenc...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
11-24: Ensure blank lines around mermaid code blocks: Markdown lint tools expect blank lines before and after fenced code. Add an empty line immediately above and below themermaidblock under “Current Architecture.”🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
12-12: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
26-49: Ensure blank lines around mermaid code blocks: Similarly, surround the “Proposed Architecture” mermaid block with blank lines to satisfy markdownlint’s MD031.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
27-27: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
87-93: Clarify Data Structures bullet formatting: The bullet “Are not executed…” lacks a clear subject. Change to “They are not executed by the sequencer before streaming, and no state root is produced at this stage.” Also ensure blank lines surround the list for readability.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
88-88: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
95-103: Parameterize mini-block interval: In the Sequencer modifications, recommend making the mini-block time window configurable (e.g., via a CLI/JSON flag) and documenting the default (200 ms).
114-133: Monitoring metrics for mini-block propagation: In Considerations, you cover network and state impacts. You may also want to track metrics (e.g., p2p delivery latency, mini-block queue size) to observe real-world performance.
150-154: Markdown formatting in References: Ensure there’s a blank line before the References heading and that the file ends with exactly one trailing newline for markdownlint compliance (MD047).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
154-154: Files should end with a single newline character
null(MD047, single-trailing-newline)
🛑 Comments failed to post (2)
specs/lazy-adr/adr-021-transaction-streaming.md (2)
68-78: 🛠️ Refactor suggestion
Add rollback semantics in Decision: The Decision section should specify how full nodes revert tentative mini-block transactions if they aren’t included in the final block. Propose adding a bullet like:
- Define rollback mechanism for mini-block state applications not finalized by the full block header.
🧰 Tools
🪛 LanguageTool
[grammar] ~73-~73: This sentence seems to be incomplete. Insert a noun before ‘Are’ to make the sentence complete.
Context: ... received within a short time window. - Are not executed by the sequencer before st...(MISSING_SUBJECT)
[uncategorized] ~77-~77: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ... until the full block header is received and the transaction batches are seen on the...(COMMA_COMPOUND_SENTENCE_2)
104-109: 🛠️ Refactor suggestion
Specify state reversion in Full Node modifications: Under Full Node changes, include a step to revert tentative state updates when associated mini blocks are omitted from the final block header. This is critical to prevent state drift.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
specs/lazy-adr/adr-021-transaction-streaming.md (8)
9-9: Refine context description to reduce repetition.
The three successive sentences starting with "This" can be merged for readability. For example:- Currently, in single sequencer mode in Rollkit, every configured block time, the sequencer produces blocks. This includes constructing a batch of transactions to put in a block, executing them, and producing a header with an updated state root. This process introduces latency between transaction submission and network-wide propagation... + Currently, in single sequencer mode, the Rollkit sequencer batches incoming transactions at each block interval, executes them, and generates a block header with the updated state root—introducing latency between transaction submission and network-wide propagation...🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nly shared after full block production. This ADR proposes a design where the sequenc...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
11-12: Surround headings and code fences with blank lines.
Markdown lint suggests headings (e.g.,### Current Architecture) and fenced code blocks should be preceded and followed by blank lines. Adding these improves readability and passes MD022/MD031.Also applies to: 26-27
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
12-12: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
72-75: Ensure consistent punctuation in bulleted lists.
In the "Decision" section, lines 72–73 end with periods, while lines 74–75 do not. For uniformity, either add periods to all bullets or remove them across the board.🧰 Tools
🪛 LanguageTool
[grammar] ~73-~73: This sentence seems to be incomplete. Insert a noun before ‘Are’ to make the sentence complete.
Context: ... received within a short time window. - Are not executed by the sequencer before st...(MISSING_SUBJECT)
89-94: Unify casing for "MiniBlock" terminology.
The ADR alternates between “mini blocks”, “Mini-Block”, andMiniBlock. Standardize on one form (e.g.,MiniBlockin code/data-structures and “mini-block” in narrative) to avoid confusion.
99-99: Fix duplicated phrasing in sequencing description.
Line 99 currently reads awkwardly with repeated phrases. Consider simplifying:- Introduce a mini-block time in addition to block time that's much smaller than the block time that controls the rate at which transactions are reaped to produce mini-blocks are produced. + Introduce a mini-block interval (e.g., 200ms) that controls how frequently transactions are reaped into mini-blocks.
107-109: Add signature verification step on full nodes.
The ADR specifiessequencer_signaturein theMiniBlockstructure but omits verification. Recommend adding a bullet under Full Node modifications:- Verify `sequencer_signature` on each mini-block before tentative state application to prevent spoofing.
136-142: Structure Consequences with clear subheadings.
Under "Consequences", the positive and negative impacts are not labeled. Please insert:### Positiveabove line 138 and
### Negativeabove line 142 to improve clarity.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
138-138: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
142-142: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
154-154: Ensure file ends with a single newline.
Markdown lint (MD047) requires a trailing newline at EOF.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
154-154: Files should end with a single newline character
null(MD047, single-trailing-newline)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
specs/lazy-adr/adr-021-transaction-streaming.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/lazy-adr/adr-021-transaction-streaming.md
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nly shared after full block production. This ADR proposes a design where the sequenc...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~53-~53: You’ve repeated a verb. Did you mean to only write one of them?
Context: ... Approaches 1. Immediate Transaction Broadcasting - Broadcast individual transactions immediately upo...
(REPEATED_VERBS)
[grammar] ~73-~73: This sentence seems to be incomplete. Insert a noun before ‘Are’ to make the sentence complete.
Context: ... received within a short time window. - Are not executed by the sequencer before st...
(MISSING_SUBJECT)
🪛 markdownlint-cli2 (0.17.2)
specs/lazy-adr/adr-021-transaction-streaming.md
11-11: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
12-12: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
27-27: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
88-88: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
137-137: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
138-138: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
142-142: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
143-143: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
146-146: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
147-147: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
154-154: Files should end with a single newline character
null
(MD047, single-trailing-newline)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
| - Pros: Maintains traditional block structure | ||
| - Cons: More complex implementation, potential consistency issues on handling transactions with state contention | ||
|
|
||
| 3. **Variable Block Times** |
There was a problem hiding this comment.
we should mention this is already present with lazy mode. Also, would this system conflict with lazy in anyway? when would a user use this vs lazy?
| - No impact on consensus or state safety | ||
| - Removes the need for sequencer to execute transactions before gossiping them to full nodes | ||
|
|
||
| ### Negative |
There was a problem hiding this comment.
this introduces a new layer of trust in the system correct?
| style Seq fill:#f9f,stroke:#333,stroke-width:2px,color:#000 | ||
| style MB1 fill:#ddf,stroke:#333,stroke-width:2px,color:#000 | ||
| style MB2 fill:#ddf,stroke:#333,stroke-width:2px,color:#000 | ||
| style MB3 fill:#ddf,stroke:#333,stroke-width:2px,color:#000 | ||
| style Block fill:#bbf,stroke:#333,stroke-width:2px,color:#000 | ||
| style Nodes fill:#bfb,stroke:#333,stroke-width:2px,color:#000 | ||
| style Txs color:#000 |
There was a problem hiding this comment.
| style Seq fill:#f9f,stroke:#333,stroke-width:2px,color:#000 | |
| style MB1 fill:#ddf,stroke:#333,stroke-width:2px,color:#000 | |
| style MB2 fill:#ddf,stroke:#333,stroke-width:2px,color:#000 | |
| style MB3 fill:#ddf,stroke:#333,stroke-width:2px,color:#000 | |
| style Block fill:#bbf,stroke:#333,stroke-width:2px,color:#000 | |
| style Nodes fill:#bfb,stroke:#333,stroke-width:2px,color:#000 | |
| style Txs color:#000 |
nit
Overview
Proposes an ADR for adding mini-blocks to Rollkit to reduce end-user latency and enhance UX.
Summary by CodeRabbit