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

docs: add vacuum! spec #3990

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

docs: add vacuum! spec #3990

wants to merge 1 commit into from

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Oct 18, 2024

Overview

creating a PR for the vacuum spec.

rendered

@evan-forbes evan-forbes added specs directly relevant to the specs vacuum! labels Oct 18, 2024
@evan-forbes evan-forbes self-assigned this Oct 18, 2024
@evan-forbes evan-forbes requested review from liamsi and a team as code owners October 18, 2024 03:30
@evan-forbes evan-forbes requested review from staheri14 and rach-id and removed request for a team October 18, 2024 03:30
@celestia-bot celestia-bot requested a review from a team October 18, 2024 03:30
Copy link

PR Preview Action v1.4.8
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-3990/
on branch gh-pages at 2024-10-18 03:30 UTC

Copy link
Contributor

coderabbitai bot commented Oct 18, 2024

📝 Walkthrough

Walkthrough

The changes include modifications to the .markdownlint.yaml configuration file, disabling certain Markdown linting rules and adjusting others. A new entry for "Vacuum!" has been added to the specs/src/SUMMARY.md file, linking to a newly created vacuum.md document. This document details the Vacuum! protocol for efficient blob propagation, including its architecture, message types, and operational logic.

Changes

File Change Summary
.markdownlint.yaml - Disabled rules: MD010, MD013, MD033
- Added rule: MD024: false
- Modified rule: MD055 to use style leading_and_trailing
specs/src/SUMMARY.md - Added new entry: [Vacuum!](./vacuum.md) under "Consensus" section
specs/src/vacuum.md - Introduced specification for the Vacuum! protocol, detailing architecture, message types, and logic for blob propagation. Added multiple new message types and methods.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant MarkdownLinter
    participant Summary
    participant VacuumProtocol

    User->>MarkdownLinter: Submit Markdown file
    MarkdownLinter->>MarkdownLinter: Check rules (MD010, MD013, MD033)
    MarkdownLinter->>User: Return linting results

    User->>Summary: View specifications
    Summary->>VacuumProtocol: Access Vacuum! protocol details
    VacuumProtocol->>User: Provide protocol specifications
Loading

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:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • 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 testing code 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (16)
.markdownlint.yaml (5)

Line range hint 2-3: LGTM, but consider standardizing indentation.

Disabling MD010 for code blocks allows the use of hard tabs, which can be beneficial for maintaining consistent indentation in code snippets. However, to ensure consistency across the project, consider establishing a project-wide indentation standard (either spaces or tabs) and communicating it to all contributors.


4-4: LGTM, but consider a soft line length limit.

Disabling MD013 removes line length restrictions, which can improve readability for long URLs or code snippets. However, to maintain overall readability, consider establishing a soft line length limit (e.g., 120 characters) as a guideline for contributors, while allowing exceptions when necessary.


5-5: LGTM, but establish guidelines for inline HTML usage.

Disabling MD033 allows the use of inline HTML, providing more flexibility in formatting and layout. However, to maintain consistency and security:

  1. Establish clear guidelines for when and how to use inline HTML.
  2. Consider implementing a review process for documents containing inline HTML.
  3. Ensure that all contributors are aware of potential security implications.

6-6: LGTM, but consider guidelines for heading uniqueness.

Disabling MD024 allows for repeated heading content, which can be beneficial in certain document structures. However, to maintain clear document organization:

  1. Encourage unique headings where possible.
  2. When repeated headings are necessary, ensure they are used consistently and logically.
  3. Consider using sub-headings or alternative formatting to differentiate between sections with similar titles.

7-8: LGTM. Consider a plan for updating existing documents.

Changing MD055 to use "leading_and_trailing" style enforces a uniform emphasis style across the entire project, which is excellent for consistency. To implement this change effectively:

  1. Create a plan to update existing documents to comply with the new style.
  2. Consider using a script to automatically update emphasis formatting in existing files.
  3. Communicate this change to all contributors and update any relevant style guides.
specs/src/SUMMARY.md (1)

10-10: LGTM! Consider adding a brief description.

The addition of the "Vacuum!" entry under the "Consensus" section is appropriate and follows the existing structure. The link to ./vacuum.md is correct, assuming the file exists in the same directory. The formatting is consistent with other entries.

Consider adding a brief description of the Vacuum! feature, similar to how "CAT Pool" is presented. This would provide users with a quick overview of what to expect in the linked document.

Example:

- [Vacuum!](./vacuum.md) - Efficient blob propagation protocol
specs/src/vacuum.md (10)

5-5: Minor typo in the introduction.

There's a small typo in the word "topoplogy". It should be "topology".

Consider applying this change:

-Vacuum! is a high throughput, extremely efficient, and very robust blob propagation protocol. It uses highly optimized lazy gossiping and clever prioritization to distribute unique blobs over unique connections. This enables close to the most optimal theoretical performance for any given topoplogy or network load.
+Vacuum! is a high throughput, extremely efficient, and very robust blob propagation protocol. It uses highly optimized lazy gossiping and clever prioritization to distribute unique blobs over unique connections. This enables close to the most optimal theoretical performance for any given topology or network load.

7-7: Clarify the abbreviation VAC.

The term "Validator Availability Certificates (VACs)" is introduced without prior context. Consider providing a brief explanation of what VACs are when first mentioning them.

You could add a short explanation like this:

-Validator Availability Ceritificates (VACs) allow for multi-height pipelining of block data before an arbitrary future block is created by providing a mechanism for validators to signal to proposers which block data they have. This is similar, but simpler, to DAG based protocols.
+Validator Availability Certificates (VACs), which are proofs that a validator possesses certain block data, allow for multi-height pipelining of block data before an arbitrary future block is created. They provide a mechanism for validators to signal to proposers which block data they have. This approach is similar to, but simpler than, DAG-based protocols.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~7-~7: This expression is usually spelled with a hyphen.
Context: ... have. This is similar, but simpler, to DAG based protocols. Unlike other high efficienc...

(BASED_HYPHEN)


17-21: Enhance readability of constants.

To improve readability and maintain consistency with code style, consider formatting the constant names as inline code.

Apply these changes:

-SAME_VAC_SEND_LIMIT: the number of times a peer can send the same VAC to a peer before being removed. Default = 1
+`SAME_VAC_SEND_LIMIT`: The number of times a peer can send the same VAC to a peer before being removed. Default = 1

-VAC_ROOT_PRUNE_WINDOW: the default number of heights that a VACRoot from a validator is stored for.
+`VAC_ROOT_PRUNE_WINDOW`: The default number of heights that a VACRoot from a validator is stored for.

-MAX_VAC_ROOTS_PER_HEIGHT: specifies the maximum number of VACRoots that are allowed from each valdiator per height. Increasing this constant reduces latency, but increases message passing overhead.
+`MAX_VAC_ROOTS_PER_HEIGHT`: Specifies the maximum number of VACRoots that are allowed from each validator per height. Increasing this constant reduces latency, but increases message passing overhead.

17-21: Provide more detailed explanations for constants.

The explanations for the constants could be more detailed to help implementers understand their significance and impact on the system.

Consider expanding the explanations like this:

-`SAME_VAC_SEND_LIMIT`: The number of times a peer can send the same VAC to a peer before being removed. Default = 1
+`SAME_VAC_SEND_LIMIT`: The maximum number of times a peer can send the same VAC to another peer before being removed from the network. This helps prevent spam and ensures efficient use of network resources. Default = 1

-`VAC_ROOT_PRUNE_WINDOW`: The default number of heights that a VACRoot from a validator is stored for.
+`VAC_ROOT_PRUNE_WINDOW`: The default number of block heights for which a VACRoot from a validator is stored. This constant determines how long historical VACRoots are kept, balancing between maintaining necessary history and managing storage requirements.

-`MAX_VAC_ROOTS_PER_HEIGHT`: Specifies the maximum number of VACRoots that are allowed from each validator per height. Increasing this constant reduces latency, but increases message passing overhead.
+`MAX_VAC_ROOTS_PER_HEIGHT`: Specifies the maximum number of VACRoots that are allowed from each validator per block height. This constant helps control the trade-off between reduced latency (by allowing more VACRoots) and increased message passing overhead. Adjusting this value impacts network performance and resource utilization.

32-40: Enhance documentation for BlobTx structure.

The BlobTx structure is well-defined, but the comment could be more explicit about the relationship between the Tx field and the Blobs field.

Consider updating the comment like this:

 // BlobTx wraps an encoded sdk.Tx with a second field to contain blobs of data.
 // The raw bytes of the blobs are not signed over, instead the commitment over
 // that data is verified for each blob using the relevant MsgPayForBlobs. Those
-// sdk.Msgs are signed over in the encoded sdk.Tx.
+// sdk.Msgs are signed over in the encoded sdk.Tx. The Blobs field contains the
+// actual blob data corresponding to the commitments in the Tx.
 type BlobTx struct {
 	Tx     []byte  
 	Blobs  []*Blob
 }

278-278: Provide more specific guidance on BlobTx selection.

The current description of how to pick BlobTx for block creation is somewhat vague. More specific guidelines or a concrete algorithm would be beneficial for implementers.

Consider expanding this section with a more detailed algorithm or pseudocode. For example:

-When creating a block, the proposer MUST pick the `BlobTx` that have the highest probability of being included. This can be done by first sorting the `BlobTx` that it has received by amount of voting power that has signed a VAC over. All blobs with less than 1/3 of the VAC voting power MAY be excluded. Then it MAY sort those `BlobTx` by priority. If there is still room in the block, it can include the first high priority `BlobTx` from validators `VACRoot`.
+When creating a block, the proposer MUST pick the `BlobTx` that have the highest probability of being included. The selection process SHOULD follow these steps:
+1. Sort all received `BlobTx` by the amount of voting power that has signed a VAC over them, in descending order.
+2. Exclude all blobs with less than 1/3 of the total VAC voting power.
+3. Sort the remaining `BlobTx` by their priority, in descending order.
+4. Include `BlobTx` in the block in this sorted order until the block is full or all `BlobTx` have been included.
+5. If there is still room in the block, include the highest priority `BlobTx` from validators' `VACRoot`s that haven't been included yet.
+
+This process ensures that the most widely-validated and highest-priority transactions are included first, maximizing the likelihood of successful block propagation.

280-280: Address the TODO item.

There's a TODO item in the specification that should be addressed before finalizing the document.

The TODO item states: "Analyze compact block propagation and VACs to figure out the most ideal function for picking transactions that are likely to have been distributed."

This is an important aspect of the protocol that needs to be resolved. Would you like assistance in developing a strategy to address this TODO? I can help draft a proposed approach or outline the key considerations for this analysis.


318-322: Clarify the trade-offs of lazy gossiping.

The explanation of lazy gossiping could be enhanced by more explicitly stating the trade-offs between efficiency and speed.

Consider expanding this explanation:

-The main efficiency of vacuum! comes lazy gossiping. Lazy meaning nodes only ever send data if their peer asks for it first. While lazy gossiping is very efficient, it comes with a meaningful penalty to speed. This is due to additional round trips that are required for peers to ask for unseen data at each hop.
+The main efficiency of Vacuum! comes from lazy gossiping, where nodes only send data if their peer explicitly requests it. This approach offers several benefits:
+1. Reduced network congestion: Only requested data is transmitted, minimizing unnecessary traffic.
+2. Better utilization of network resources: Bandwidth is used more efficiently as nodes only receive data they need.
+3. Improved resilience against certain types of attacks: It's harder for malicious nodes to flood the network with unwanted data.
+
+However, lazy gossiping comes with a trade-off:
+1. Increased latency: Additional round trips are required for peers to request unseen data at each hop, which can slow down overall data propagation.
+2. More complex state management: Nodes need to keep track of what data each peer has or hasn't requested.
+
+The Vacuum! protocol aims to balance these trade-offs through optimizations like chunked blobs and pipelined haves and wants, which are discussed in subsequent sections.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~318-~318: Possible missing comma found.
Context: ...ciency of vacuum! comes lazy gossiping. Lazy meaning nodes only ever send data if th...

(AI_HYDRA_LEO_MISSING_COMMA)


456-473: Enhance explanation of disconnection rules.

While the disconnection rules are clear, providing more context for each rule would help implementers understand the rationale behind them and implement them more effectively.

Consider expanding the explanations for each rule. For example:

 ## Disconnection Rules

-These are the following scenarios which should never occur for honest peers. If they do, not only is it safe to disconnect from a peer, but they must have some consequence in order to prevent malicious nodes from spamming useless data.
+The following scenarios should never occur for honest peers. If they do, it is not only safe to disconnect from the peer, but necessary to prevent malicious nodes from spamming the network with useless data or manipulating the protocol flow. Each rule is designed to maintain the integrity and efficiency of the Vacuum! protocol:

 ### Out of Order Messages

-Since the `VACRoot` is required to verify the `VAC`, the corresponding `VACRoot` must be sent to a peer before a given `VAC`. If a node receives a `VAC` before it has received a `VACRoot`, then it must disconnect the peer for this error. Without this, malicious instances could spam `VAC` messages without repercussion.
+Rationale: The `VACRoot` is required to verify the `VAC`, establishing a critical dependency in the protocol flow.
+Rule: If a node receives a `VAC` before it has received the corresponding `VACRoot`, it MUST disconnect the peer.
+Consequence of violation: Without this rule, malicious nodes could spam `VAC` messages without the ability to verify them, potentially overwhelming honest nodes with unverifiable data.

 ### Redundant Messages

-If a node sends a peer the same `VACRoot` or `VAC` twice, then it must be disconnected for the same reasons as above.
+Rationale: Honest nodes should not need to send the same message multiple times under normal circumstances.
+Rule: If a node receives the same `VACRoot` or `VAC` twice from a peer, it MUST disconnect that peer.
+Consequence of violation: Allowing redundant messages could enable denial-of-service attacks through message flooding.

 ### Unsolicited Data

-If a node receives any part of a blobTx that it didn't ask for, then it must disconnect from that peer.
+Rationale: The lazy gossiping mechanism relies on nodes only sending data that has been explicitly requested.
+Rule: If a node receives any part of a `BlobTx` that it didn't request via a `WantBlob` message, it MUST disconnect from that peer.
+Consequence of violation: Allowing unsolicited data transmission undermines the efficiency gains of lazy gossiping and could be used to overwhelm nodes with unwanted data.

 ### Invalid Messages

-If a peer sends a message that does not pass that method `ValidateBasic` method, then that peer must be disconnected.
+Rationale: All messages should conform to basic validity rules to ensure protocol integrity.
+Rule: If a peer sends a message that does not pass the `ValidateBasic` method, the receiving node MUST disconnect that peer.
+Consequence of violation: Invalid messages could indicate a bug in the peer's implementation or a deliberate attempt to disrupt the protocol, both of which justify disconnection to maintain network health.
🧰 Tools
🪛 LanguageTool

[style] ~457-~457: Consider a shorter alternative to avoid wordiness.
Context: ...er, but they must have some consequence in order to prevent malicious nodes from spamming u...

(IN_ORDER_TO_PREMIUM)


456-473: Consider edge cases in disconnection scenarios.

While the current disconnection rules cover major scenarios, there might be edge cases that could be exploited by malicious actors or cause issues for honest nodes under certain network conditions.

Consider addressing the following edge cases:

  1. Temporary network issues: In cases of temporary network partitions or high latency, honest nodes might accidentally violate these rules. Consider implementing a grace period or threshold before disconnection.

  2. Partial message reception: If a message is partially received due to network issues, it might fail the ValidateBasic check. Consider how to handle incomplete messages.

  3. Clock synchronization: If nodes' clocks are not perfectly synchronized, it might lead to apparent out-of-order messages. Consider how time differences between nodes might affect the protocol.

  4. Reconnection attempts: Specify if and how quickly a disconnected peer can attempt to reconnect, to prevent continuous reconnection attempts from malicious nodes.

  5. Differential treatment for trusted peers: Consider if there should be different disconnection rules or thresholds for peers that are known to be trusted (e.g., other validator nodes).

Adding considerations for these edge cases would make the protocol more robust and resistant to both unintentional issues and potential attacks.

🧰 Tools
🪛 LanguageTool

[style] ~457-~457: Consider a shorter alternative to avoid wordiness.
Context: ...er, but they must have some consequence in order to prevent malicious nodes from spamming u...

(IN_ORDER_TO_PREMIUM)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2507aaf and e666c7d.

📒 Files selected for processing (3)
  • .markdownlint.yaml (1 hunks)
  • specs/src/SUMMARY.md (1 hunks)
  • specs/src/vacuum.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/src/vacuum.md

[uncategorized] ~7-~7: This expression is usually spelled with a hyphen.
Context: ... have. This is similar, but simpler, to DAG based protocols. Unlike other high efficienc...

(BASED_HYPHEN)


[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ..., to DAG based protocols. Unlike other high efficiency block propagation protocols such as Tur...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[misspelling] ~9-~9: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...obs results in a more optimal path than a AOT approach. This is derived from inco...

(EN_A_VS_AN)


[grammar] ~57-~57: Consider using either the past participle “sent” or the present participle “sending” here.
Context: ... The order in which they are send MUST be go WantBlob{VAC_A_Pri...

(BEEN_PART_AGREEMENT)


[uncategorized] ~65-~65: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...dator could hide the last chunk of very high priority data, which could waste a large amount ...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[grammar] ~116-~116: The plural noun “nodes” cannot be used with the article “a”. Did you mean “a given node” or “given nodes”?
Context: ...obafter receiving the firstVACfor a givenRootVAC- nodes MAY request aBlobTxby sending aWa...

(A_NNS)


[style] ~121-~121: Consider removing “of” to be more concise
Context: ...one of the "worst case" scenarios where all of the validator's mempools naturally differ. ...

(ALL_OF_THE)


[uncategorized] ~121-~121: Possible wrong verb form detected. Did you mean “maintained” or “maintaining”?
Context: ...When prioritizing these, the network is maintains its ability to get a high throughput an...

(BE_WITH_WRONG_VERB_FORM)


[misspelling] ~129-~129: Did you mean “holding on to”?
Context: ...ators to both communicate that they are holding onto some data for a period of time and to d...

(HOLD_ONTO)


[uncategorized] ~188-~188: Possible missing comma found.
Context: ...rs that send a VAC before sending its corresponding VACRoot - Nodes MUST remove peers tha...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~196-~196: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t send the same VAC multiple times. - Nodes MUST only send a VAC for a BlobTx t...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[grammar] ~197-~197: Use the singular noun “node” with the word ‘a’.
Context: ...e requested the blob via a WantBlob - Nodes MUST send WantBlobs for the highest p...

(DT_JJ_NNS_MD)


[uncategorized] ~230-~230: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...s3A.png) It is generated using a fixed amount of bytes for all uints. This example me...

(AMOUNTOF_TO_NUMBEROF)


[style] ~268-~268: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re the VACRoot from each validator. - Nodes MAY delete the VACRoot after a subjec...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~311-~311: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...locks for the same height and round. - Nodes MUST broadcast the CompactBlock` to al...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~312-~312: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cast the CompactBlock to all peers. - Nodes MUST send WantBlob messages to the pe...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~318-~318: Possible missing comma found.
Context: ...ciency of vacuum! comes lazy gossiping. Lazy meaning nodes only ever send data if th...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~323-~323: Consider a shorter alternative to avoid wordiness.
Context: ...zAh0.png) ### Speeding up Lazy Gossip In order to maintain high throughput block propagat...

(IN_ORDER_TO_PREMIUM)


[uncategorized] ~333-~333: “its” (belonging to it) seems less likely than “it’s” (it is)
Context: ...ps://hackmd.io/_uploads/BJFoLfCh0.png) Its worth noting that the benefit of chunki...

(AI_HYDRA_LEO_CPT_ITS_ITIS)


[uncategorized] ~334-~334: Possible missing comma found.
Context: ...t of chunking blobs compounds each hop. Meaning for networks that don't have a lot of h...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~334-~334: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...p. Meaning for networks that don't have a lot of hops, chunking will not have as signifi...

(A_LOT_OF)


[duplication] ~338-~338: Possible typo: you repeated a word
Context: ...eceives the VAC either has the BlobTx that that VAC commits over or it ends up reques...

(ENGLISH_WORD_REPEAT_RULE)


[uncategorized] ~338-~338: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...he BlobTx that that VAC commits over or it ends up requesting the VAC. This m...

(COMMA_COMPOUND_SENTENCE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s to see if it has room for that VAC. If it does, then it requests that VAC. I...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...f it does, then it requests that VAC. If it doesn't have room, then it checks to...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e transactions that it currently holds. If the priority is higher, then it request...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y is higher, then it requests that VAC. If it is not, then it can safely ignore th...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~338-~338: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... not, then it can safely ignore that VAC and it does not gossip the VAC. ![Screensh...

(COMMA_COMPOUND_SENTENCE)


[uncategorized] ~449-~449: “its” (belonging to it) seems less likely than “it’s” (it is)
Context: ...tized Gossiping While gossiping blobs, its crucial that nodes are sending the high...

(AI_HYDRA_LEO_CPT_ITS_ITIS)


[style] ~457-~457: Consider a shorter alternative to avoid wordiness.
Context: ...er, but they must have some consequence in order to prevent malicious nodes from spamming u...

(IN_ORDER_TO_PREMIUM)

🪛 Markdownlint
specs/src/vacuum.md

42-42: Expected: 0 or 2; Actual: 1
Trailing spaces

(MD009, no-trailing-spaces)

🔇 Additional comments (1)
specs/src/vacuum.md (1)

1-473: Overall assessment of the Vacuum! protocol specification.

The Vacuum! protocol specification provides a comprehensive and well-structured description of a novel approach to efficient blob propagation in distributed networks. The specification covers key aspects of the protocol, including message types, block creation, gossiping logic, and network management rules.

Strengths of the specification:

  1. Detailed explanation of the protocol's core concepts, such as lazy gossiping and prioritized data propagation.
  2. Clear definitions of message types and their structures.
  3. Thoughtful consideration of network efficiency and security aspects.

Areas for improvement:

  1. Some sections could benefit from more detailed explanations or concrete examples.
  2. A few potential issues in the implementation logic have been identified and should be addressed.
  3. Consideration of edge cases, particularly in the disconnection rules, could be expanded.
  4. There are some TODOs and areas marked for further analysis that should be resolved before finalizing the specification.

Overall, this specification provides a solid foundation for implementing the Vacuum! protocol. Addressing the suggested improvements will enhance its clarity, robustness, and implementability.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~7-~7: This expression is usually spelled with a hyphen.
Context: ... have. This is similar, but simpler, to DAG based protocols. Unlike other high efficienc...

(BASED_HYPHEN)


[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ..., to DAG based protocols. Unlike other high efficiency block propagation protocols such as Tur...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[misspelling] ~9-~9: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...obs results in a more optimal path than a AOT approach. This is derived from inco...

(EN_A_VS_AN)


[grammar] ~57-~57: Consider using either the past participle “sent” or the present participle “sending” here.
Context: ... The order in which they are send MUST be go WantBlob{VAC_A_Pri...

(BEEN_PART_AGREEMENT)


[uncategorized] ~65-~65: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...dator could hide the last chunk of very high priority data, which could waste a large amount ...

(EN_COMPOUND_ADJECTIVE_INTERNAL)


[grammar] ~116-~116: The plural noun “nodes” cannot be used with the article “a”. Did you mean “a given node” or “given nodes”?
Context: ...obafter receiving the firstVACfor a givenRootVAC- nodes MAY request aBlobTxby sending aWa...

(A_NNS)


[style] ~121-~121: Consider removing “of” to be more concise
Context: ...one of the "worst case" scenarios where all of the validator's mempools naturally differ. ...

(ALL_OF_THE)


[uncategorized] ~121-~121: Possible wrong verb form detected. Did you mean “maintained” or “maintaining”?
Context: ...When prioritizing these, the network is maintains its ability to get a high throughput an...

(BE_WITH_WRONG_VERB_FORM)


[misspelling] ~129-~129: Did you mean “holding on to”?
Context: ...ators to both communicate that they are holding onto some data for a period of time and to d...

(HOLD_ONTO)


[uncategorized] ~188-~188: Possible missing comma found.
Context: ...rs that send a VAC before sending its corresponding VACRoot - Nodes MUST remove peers tha...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~196-~196: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t send the same VAC multiple times. - Nodes MUST only send a VAC for a BlobTx t...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[grammar] ~197-~197: Use the singular noun “node” with the word ‘a’.
Context: ...e requested the blob via a WantBlob - Nodes MUST send WantBlobs for the highest p...

(DT_JJ_NNS_MD)


[uncategorized] ~230-~230: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...s3A.png) It is generated using a fixed amount of bytes for all uints. This example me...

(AMOUNTOF_TO_NUMBEROF)


[style] ~268-~268: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...re the VACRoot from each validator. - Nodes MAY delete the VACRoot after a subjec...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~311-~311: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...locks for the same height and round. - Nodes MUST broadcast the CompactBlock` to al...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~312-~312: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...cast the CompactBlock to all peers. - Nodes MUST send WantBlob messages to the pe...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~318-~318: Possible missing comma found.
Context: ...ciency of vacuum! comes lazy gossiping. Lazy meaning nodes only ever send data if th...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~323-~323: Consider a shorter alternative to avoid wordiness.
Context: ...zAh0.png) ### Speeding up Lazy Gossip In order to maintain high throughput block propagat...

(IN_ORDER_TO_PREMIUM)


[uncategorized] ~333-~333: “its” (belonging to it) seems less likely than “it’s” (it is)
Context: ...ps://hackmd.io/_uploads/BJFoLfCh0.png) Its worth noting that the benefit of chunki...

(AI_HYDRA_LEO_CPT_ITS_ITIS)


[uncategorized] ~334-~334: Possible missing comma found.
Context: ...t of chunking blobs compounds each hop. Meaning for networks that don't have a lot of h...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~334-~334: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...p. Meaning for networks that don't have a lot of hops, chunking will not have as signifi...

(A_LOT_OF)


[duplication] ~338-~338: Possible typo: you repeated a word
Context: ...eceives the VAC either has the BlobTx that that VAC commits over or it ends up reques...

(ENGLISH_WORD_REPEAT_RULE)


[uncategorized] ~338-~338: Use a comma before ‘or’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...he BlobTx that that VAC commits over or it ends up requesting the VAC. This m...

(COMMA_COMPOUND_SENTENCE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s to see if it has room for that VAC. If it does, then it requests that VAC. I...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...f it does, then it requests that VAC. If it doesn't have room, then it checks to...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...e transactions that it currently holds. If the priority is higher, then it request...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[style] ~338-~338: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...y is higher, then it requests that VAC. If it is not, then it can safely ignore th...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[uncategorized] ~338-~338: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... not, then it can safely ignore that VAC and it does not gossip the VAC. ![Screensh...

(COMMA_COMPOUND_SENTENCE)


[uncategorized] ~449-~449: “its” (belonging to it) seems less likely than “it’s” (it is)
Context: ...tized Gossiping While gossiping blobs, its crucial that nodes are sending the high...

(AI_HYDRA_LEO_CPT_ITS_ITIS)


[style] ~457-~457: Consider a shorter alternative to avoid wordiness.
Context: ...er, but they must have some consequence in order to prevent malicious nodes from spamming u...

(IN_ORDER_TO_PREMIUM)

🪛 Markdownlint

42-42: Expected: 0 or 2; Actual: 1
Trailing spaces

(MD009, no-trailing-spaces)

Comment on lines +136 to +150
// Commitment is commitment to the underlying BlobTx.
Commitment []byte
// VACRoot is the commitment over the batch of VACs that this VAC was included
// in.
VACRoot []byte
// Priority is a deterministic value that nodes can use to compare the
// priority of any two VACs.
Priority uint
// Size is the size of the BlobTx in bytes.
Size uint
// ID indicates which VAC this is.
ID uint
// Proof of inclusion from this VAC to the VACRoot
Proof merkle.Proof
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential issue with VAC Commitment field type.

The Commitment field in the VAC struct is defined as []byte, but in the VACRoot struct, it's defined as [32]byte. This inconsistency could lead to issues when comparing or processing these fields.

Consider standardizing the Commitment field type across both structs. If a fixed-size byte array is appropriate, update the VAC struct like this:

 type VAC struct {
 	// Commitment is commitment to the underlying BlobTx.
-	Commitment []byte
+	Commitment [32]byte
 	// VACRoot is the commitment over the batch of VACs that this VAC was included
 	// in.
 	VACRoot []byte
 	// Priority is a deterministic value that nodes can use to compare the
 	// priority of any two VACs.
 	Priority uint
 	// Size is the size of the BlobTx in bytes.
 	Size uint
 	// ID indicates which VAC this is.
 	ID uint
 	// Proof of inclusion from this VAC to the VACRoot
 	Proof merkle.Proof
 }

If a variable-length byte slice is more appropriate, update the VACRoot struct instead. Make sure to update any related functions or methods that use these fields.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Commitment is commitment to the underlying BlobTx.
Commitment []byte
// VACRoot is the commitment over the batch of VACs that this VAC was included
// in.
VACRoot []byte
// Priority is a deterministic value that nodes can use to compare the
// priority of any two VACs.
Priority uint
// Size is the size of the BlobTx in bytes.
Size uint
// ID indicates which VAC this is.
ID uint
// Proof of inclusion from this VAC to the VACRoot
Proof merkle.Proof
}
// Commitment is commitment to the underlying BlobTx.
Commitment [32]byte
// VACRoot is the commitment over the batch of VACs that this VAC was included
// in.
VACRoot []byte
// Priority is a deterministic value that nodes can use to compare the
// priority of any two VACs.
Priority uint
// Size is the size of the BlobTx in bytes.
Size uint
// ID indicates which VAC this is.
ID uint
// Proof of inclusion from this VAC to the VACRoot
Proof merkle.Proof
}

Comment on lines +349 to +399
func requestVAC(peer p2p.Peer, vp VACPool, vac VAC) {
vp.allocate(vac)
peer.Send(WantBlob{[]byte(vac.Commitment)})
}


// ReceiveVAC describes the receive logic for a VAC. Notably, it only broadcasts
// the VAC if the underlying BlobTx is either already held or it is requested.
func ReceiveVAC(vp VACPool, peer p2p.Peer, vac VAC) {
// if the peer has sent this VAC to the node before, then they are safe to kick.
if vp.SeenFrom(peer, vac) {
kick(peer)
return
}

// if the VAC has already been seen, then all all necessary logic has been
// performed and the node can safely ignore it.
if vp.HasVAC(vac) {
return
}

// if this node already has the transaction but has not seen this VAC
// before, then it must proceed to gossip the VAC, but it does not need to
// request the blob.
if vp.Has(vac.Commitment) {
// ensure that the underlying BlobTx is not pruned until the hold height
// or a higher priority VAC is received.
vp.allocate(vac)
broadcastToAllPeers(vac)
return
}

// if this VAC is the highest priority from a given validator and this node
// has not seen it, then it must request the BlobTx and gossip the VAC
if vac.ID == 0 {
requestVAC(peer, vac)
broadcastToAllPeers(vac)
return
}

// At this point, this node does not have the BlobTx, but it needs to see if
// it has room in its mempool or if the underlying BlobTx is higher priority
// than any of the transactions in its mempool. If either is true, it must
// request and gossip the VAC.
if vp.LowestPriority() < vac.Priority || vp.HasRoom(vac) {
requestVAC(peer, vac)
broadcastToAllPeers(vac)
}

return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential issue in VAC handling logic.

The ReceiveVAC function has a logical issue that could lead to inefficient network usage.

In the current implementation, if a node already has the transaction but hasn't seen the VAC before, it allocates space for the VAC and broadcasts it to all peers. However, it doesn't check if the VAC is of higher priority than the ones it already has. This could lead to unnecessary propagation of lower-priority VACs.

Consider modifying the logic like this:

 func ReceiveVAC(vp VACPool, peer p2p.Peer, vac VAC) {
     // ... (existing checks)

     // if this node already has the transaction but has not seen this VAC
     // before, then it must proceed to gossip the VAC, but it does not need to
     // request the blob.
     if vp.Has(vac.Commitment) {
+        // Check if this VAC is higher priority than existing ones
+        if vp.LowestPriority() < vac.Priority || vp.HasRoom(vac) {
             // ensure that the underlying BlobTx is not pruned until the hold height
             // or a higher priority VAC is received.
             vp.allocate(vac)
             broadcastToAllPeers(vac)
+        }
         return
     }

     // ... (rest of the function)
 }

This change ensures that only higher priority VACs or VACs for which there's room are propagated, even when the node already has the underlying transaction.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func requestVAC(peer p2p.Peer, vp VACPool, vac VAC) {
vp.allocate(vac)
peer.Send(WantBlob{[]byte(vac.Commitment)})
}
// ReceiveVAC describes the receive logic for a VAC. Notably, it only broadcasts
// the VAC if the underlying BlobTx is either already held or it is requested.
func ReceiveVAC(vp VACPool, peer p2p.Peer, vac VAC) {
// if the peer has sent this VAC to the node before, then they are safe to kick.
if vp.SeenFrom(peer, vac) {
kick(peer)
return
}
// if the VAC has already been seen, then all all necessary logic has been
// performed and the node can safely ignore it.
if vp.HasVAC(vac) {
return
}
// if this node already has the transaction but has not seen this VAC
// before, then it must proceed to gossip the VAC, but it does not need to
// request the blob.
if vp.Has(vac.Commitment) {
// ensure that the underlying BlobTx is not pruned until the hold height
// or a higher priority VAC is received.
vp.allocate(vac)
broadcastToAllPeers(vac)
return
}
// if this VAC is the highest priority from a given validator and this node
// has not seen it, then it must request the BlobTx and gossip the VAC
if vac.ID == 0 {
requestVAC(peer, vac)
broadcastToAllPeers(vac)
return
}
// At this point, this node does not have the BlobTx, but it needs to see if
// it has room in its mempool or if the underlying BlobTx is higher priority
// than any of the transactions in its mempool. If either is true, it must
// request and gossip the VAC.
if vp.LowestPriority() < vac.Priority || vp.HasRoom(vac) {
requestVAC(peer, vac)
broadcastToAllPeers(vac)
}
return
}
func requestVAC(peer p2p.Peer, vp VACPool, vac VAC) {
vp.allocate(vac)
peer.Send(WantBlob{[]byte(vac.Commitment)})
}
// ReceiveVAC describes the receive logic for a VAC. Notably, it only broadcasts
// the VAC if the underlying BlobTx is either already held or it is requested.
func ReceiveVAC(vp VACPool, peer p2p.Peer, vac VAC) {
// if the peer has sent this VAC to the node before, then they are safe to kick.
if vp.SeenFrom(peer, vac) {
kick(peer)
return
}
// if the VAC has already been seen, then all all necessary logic has been
// performed and the node can safely ignore it.
if vp.HasVAC(vac) {
return
}
// if this node already has the transaction but has not seen this VAC
// before, then it must proceed to gossip the VAC, but it does not need to
// request the blob.
if vp.Has(vac.Commitment) {
// Check if this VAC is higher priority than existing ones
if vp.LowestPriority() < vac.Priority || vp.HasRoom(vac) {
// ensure that the underlying BlobTx is not pruned until the hold height
// or a higher priority VAC is received.
vp.allocate(vac)
broadcastToAllPeers(vac)
}
return
}
// if this VAC is the highest priority from a given validator and this node
// has not seen it, then it must request the BlobTx and gossip the VAC
if vac.ID == 0 {
requestVAC(peer, vac)
broadcastToAllPeers(vac)
return
}
// At this point, this node does not have the BlobTx, but it needs to see if
// it has room in its mempool or if the underlying BlobTx is higher priority
// than any of the transactions in its mempool. If either is true, it must
// request and gossip the VAC.
if vp.LowestPriority() < vac.Priority || vp.HasRoom(vac) {
requestVAC(peer, vac)
broadcastToAllPeers(vac)
}
return
}

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Only partially reviewed. I think a diagram or high level description of the protocol would help prior to diving into inbound / outbound logic.

Historically the specs were filled with lots of content that we thought we would implement but didn't end up implementing. As a result, the specs still contain information that doesn't match the current implementation.

TBH I don't think the vacuum spec should be added to the specs yet because vaccum isn't implemented or used. IMO it makes more sense to add this as an ADR in celestia-app or potentially even celestia-core. After it gets implemented and incorporated in the protocol, we can add the ADR to the specs.

@@ -3,5 +3,6 @@
"code_blocks": false # Disable rule for hard tabs in code blocks
"MD013": false # Disable rule for line length
"MD033": false # Disable rule banning inline HTML
"MD024": false
Copy link
Collaborator

Choose a reason for hiding this comment

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

[no change needed] we may be able to get the behavior we want by setting siblings_only: true instead of disabling the rule

Ref: https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md024---multiple-headings-with-the-same-content


## Intro

Vacuum! is a high throughput, extremely efficient, and very robust blob propagation protocol. It uses highly optimized lazy gossiping and clever prioritization to distribute unique blobs over unique connections. This enables close to the most optimal theoretical performance for any given topoplogy or network load.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] The adjectives hype up the protocol but they don't seem necessary.

Suggested change
Vacuum! is a high throughput, extremely efficient, and very robust blob propagation protocol. It uses highly optimized lazy gossiping and clever prioritization to distribute unique blobs over unique connections. This enables close to the most optimal theoretical performance for any given topoplogy or network load.
Vacuum! is a high throughput, efficient, and robust blob propagation protocol. It uses optimized lazy gossiping and prioritization to distribute unique blobs over unique connections. This enables close to the most optimal theoretical performance for any given topoplogy or network load.


Validator Availability Ceritificates (VACs) allow for multi-height pipelining of block data before an arbitrary future block is created by providing a mechanism for validators to signal to proposers which block data they have. This is similar, but simpler, to DAG based protocols.

Unlike other high efficiency block propagation protocols such as Turbine, the topology of the network and the route of the block data is not known ahead of time. Instead, to acheive high throughput, the topology of the network and path of each blob is discovered on the fly as data is propagated via the pipelined lazy gossip of `VAC`s and `WantBlob`s. Beyond the simplicity benefits, using a JIT approach to routing blobs results in a more optimal path than a AOT approach. This is derived from incorporating variables that are impossible to know a head of time, such as the realtime congestion and latency between each peer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] define acronyms at their first usage

Suggested change
Unlike other high efficiency block propagation protocols such as Turbine, the topology of the network and the route of the block data is not known ahead of time. Instead, to acheive high throughput, the topology of the network and path of each blob is discovered on the fly as data is propagated via the pipelined lazy gossip of `VAC`s and `WantBlob`s. Beyond the simplicity benefits, using a JIT approach to routing blobs results in a more optimal path than a AOT approach. This is derived from incorporating variables that are impossible to know a head of time, such as the realtime congestion and latency between each peer.
Unlike other high efficiency block propagation protocols such as Turbine, the topology of the network and the route of the block data is not known ahead of time. Instead, to achieve high throughput, the topology of the network and path of each blob is discovered on the fly as data is propagated via the pipelined lazy gossip of `VAC`s and `WantBlob`s. Beyond the simplicity benefits, using a Just-In-Time (JIT) approach to routing blobs results in a more optimal path than a Ahead-Of-Time (AOT) approach. This is derived from incorporating variables that are impossible to know a head of time, such as the realtime congestion and latency between each peer.


Vacuum! must be built ontop of a protocol such as QUIC or TCP, meaning it assumes there are guarantees in the order in which messages are read.

## Constants
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] before defining constants, it would be nice to give readers a high level understanding of the protocol. For example, a diagram could be really helpful.

When I read this section I have a number of questions like:

  • What is a VACRoot? How does it differ from a VAC?
  • Are peers and validators interchangeable in this context? If yes, can this consolidate on one term? If not, can the distinction be clarified?


- Before sending a BlobTx, a node MUST first receive a `WantBlob` for an existing `VAC` that commits to that `BlobTx`
- Nodes MUST send the highest priority `BlobTx` before sending other `BlobTx` to peers that have requested more than one `BlobTx`.
- ONLY IF CHUNKING IS ENABLED: The highest priority MUST be determined by first sorting requested blobs by priority, then prioritized by unique validator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[quesiton] is chunking == prioritization? Can this feature be defined or described before this line?


> nodes MUST send a `WantBlob` after receiving the first `VAC` for a given `RootVAC`

To expand on the reasoning behind this, the first `VAC` from each validator is prioritized throughput the network to handle one of the "worst case" scenarios where all of the validator's mempools naturally differ. When prioritizing these, the network is maintains its ability to get a high throughput and prioritize the highest value txs. This is also why validators send the highest priority `VAC` to all of their peers (more on this in the Outbound Logic for VACs).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
To expand on the reasoning behind this, the first `VAC` from each validator is prioritized throughput the network to handle one of the "worst case" scenarios where all of the validator's mempools naturally differ. When prioritizing these, the network is maintains its ability to get a high throughput and prioritize the highest value txs. This is also why validators send the highest priority `VAC` to all of their peers (more on this in the Outbound Logic for VACs).
To expand on the reasoning behind this, the first `VAC` from each validator is prioritized throughout the network to handle one of the "worst case" scenarios where all of the validator's mempools naturally differ. When prioritizing these, the network maintains its ability to get a high throughput and prioritize the highest value txs. This is also why validators send the highest priority `VAC` to all of their peers (more on this in the Outbound Logic for VACs).

@evan-forbes evan-forbes marked this pull request as draft October 18, 2024 18:52
@evan-forbes
Copy link
Member Author

TBH I don't think the vacuum spec should be added to the specs yet because vaccum isn't implemented or used.

fair! this is for the upcoming blog post. folks suggested it was better to have this up on github instead of hackmd

@evan-forbes
Copy link
Member Author

evan-forbes commented Oct 18, 2024

converted to a draft for now, and we can leave it here until we are confident the implementation and spec match

I think github is better for reviewing this than hackmd to perhaps keep more context and link to ther issues

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Congrats on writing the spec! This gives us a more solid basis to chip away at and improve.

We will probably need to do the same with Compact Blocks. I'd like to weave them in a way where it's clear how Vacuum! extends the protocol. I was thinking about explaining the model of how there are three things that can improve Compact Blocks efficacy:

  • Tx Dissemination Speed - how quickly you can send a transaction to all nodes in a network
  • Preparation - how can you increase the likelihood that 2/3+ validators have all the transactions in the block you propose
  • Recovery - how quickly can nodes retrieve the missing transactions.

Does this seem like an okay model?

I see Vacuum! as mostly providing benefits to the preparation aspect (but perhaps also to tx dissemination with chunking). I think chunking could be written as a separate spec as it's not fundamental to vacuum!

#### Outbound Logic

- Before sending a BlobTx, a node MUST first receive a `WantBlob` for an existing `VAC` that commits to that `BlobTx`
- Nodes MUST send the highest priority `BlobTx` before sending other `BlobTx` to peers that have requested more than one `BlobTx`.
Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, we have a prioritised outbound queue per validator?


If the highest priority VAC was soley determined by the prioritiy from the validator, then a malicous validator could hide the last chunk of very high priority data, which could waste a large amount of bandwidth. This is why, if chunking is enabled, different validator's must be prioritized.

- OPTIONAL: the validator's voting power can also be incorporated into the prioritization logic. This increases the cost of meaningfully spamming the network when chunking is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting optimization


#### Inbound Logic

- Nodes MUST check the validity of the BlobTx via the `CheckTx`. `BlobTx` that fail **stateless** checks are invalid in every scenario. Therefore, blobs that fail stateless checks MUST result in removing the sending peer. `BlobTx`s that fail **stateful** checks MUST not be gossiped, but the peer should not be kicked, as this simply means the `BlobTx` that was originally asked for is no longer relevant.
Copy link
Contributor

Choose a reason for hiding this comment

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

For failure of a stateful check, should the transaction be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this any different to how it currently works

#### Outbound Logic

- Before sending any `BlobChunk`s, a node MUST first receive a `WantBlob` for an existing `VAC` that commits to that `BlobTx`
- Nodes MUST first send the Transaction portion of the `BlobTx`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so they can partly verify the BlobTx right? But they won't be able to fully verify the BlobTx until all blob chunks have been received


#### Outbound Logic

- Before sending a BlobTx, a node MUST first receive a `WantBlob` for an existing `VAC` that commits to that `BlobTx`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I send both BlobTxs and the chunks? When do I send one and when do I send the other


Breaking a blob up into chunks allows for nodes in the network to begin transferring portions of the blob instead of waiting for the entirety of it to be received before sending it to peers. We can see how in basic simulations this speeds up the distribution of data.

![Screenshot from 2024-09-10 20-57-02](https://hackmd.io/_uploads/BJFoLfCh0.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to transfer all these diagrams to the specs directory so we don't get 404s in the future


#### Chunked Blobs

Breaking a blob up into chunks allows for nodes in the network to begin transferring portions of the blob instead of waiting for the entirety of it to be received before sending it to peers. We can see how in basic simulations this speeds up the distribution of data.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of a BlobTx even if chunked, won't you wait until you have received all chunks and verified the complete BlobTx before gossiping it on to other peers. Else could you end up with a case where you pass on an invalid blobTx and are disconnected by that peer?


#### Pipelined Haves and Wants

`VAC`s must be gossiped eagerly if the node that receives the VAC either has the `BlobTx` that that `VAC` commits over or it ends up requesting the `VAC`. This means that upon receiving a `VAC` for the first time, a node broadcasts that same `VAC` to all of its peers. Simultaneously, if that node does not have `BlobTx` that the VAC commits over, then it checks if that is the first `VAC` for that validator. If it is, then it always requests that `VAC`. If not, then it checks to see if it has room for that `VAC`. If it does, then it requests that `VAC`. If it doesn't have room, then it checks to see if the VAC commits over a transaction that is higher in priority than any of the transactions that it currently holds. If the priority is higher, then it requests that VAC. If it is not, then it can safely ignore that VAC and it does not gossip the VAC.
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work with VAC roots? Don't you need to send the VAC roots ahead of time else how can a receiver verify that the process has signed over that VAC?


In vacuum!, each validator starts the cycle by reaping a set of blob transactions from their mempool. `VAC`s are created for each transaction, and then a `VACRoot` is created and signed over using the validator's consensus key.

After creating the `VACRoot`, it can be sent to all peers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate more on when a VACRoot is created and sent out?


### Redundant Messages

If a node sends a peer the same `VACRoot` or `VAC` twice, then it must be disconnected for the same reasons as above.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the node crashes and restarts. Isn't there a chance that they send the same VACRoot and VAC. Do we store to disk all the messages we have sent and received from peers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specs directly relevant to the specs vacuum!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants