docs: ADR for High Throughput Recovery #4315
Conversation
evan-forbes
added
ADR
evan-forbes
requested review from
cmwaters and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThis pull request introduces a new ADR document, "ADR 024: High Throughput Recovery," that outlines a separation of block propagation into a "Preparation" and a "Recovery" phase for the Celestia protocol. Additional supporting documents detail the integration of backward-compatible block propagation into the gossip protocols, including a new syncing routine, handshake logic to differentiate between legacy and upgraded nodes, a “Logic and State” section explaining PBBT reactor behavior, and the definition of new PBBT messages with their validation logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Peer
participant Node
participant ConsensusReactor
Peer->>Node: Initiate handshake (node info)
Node->>ConsensusReactor: Check block propagation capabilities
alt Supports new reactor
ConsensusReactor->>Node: Trigger syncData routine
else Legacy support
ConsensusReactor->>Node: Activate legacyPropagation routine
end
Node->>Peer: Communicate block propagation method
sequenceDiagram
participant Proposer
participant Validator
Proposer->>Validator: Send Commitment (CompactBlock details)
Validator->>Proposer: Verify signature and merkle proof (via HaveParts)
Validator->>Proposer: Request missing parts (using WantParts)
Proposer->>Validator: Provide RecoveryPart (Data message)
Validator->>Proposer: Confirm data integrity
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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 (13)
docs/architecture/adr-024-high-throughput-recovery.md (1)
13-16: Refine Descriptive Wording and HyphenationThe context paragraph uses phrases such as “pull based” which should be hyphenated (e.g. “pull‐based”) to clearly indicate the compound adjective. Also, consider simplifying the wording to reduce repetitiveness.
🧰 Tools
🪛 LanguageTool
[style] ~13-~13: Consider a shorter alternative to avoid wordiness.
Context: ... data after the block has been created. In order to utilize the data distributed before the...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~13-~13: The adjective “pull-based” is spelled with a hyphen.
Context: ...reated, the recovery phase must also be pull based. Therefore, the constraints for recover...(BASED_HYPHEN)
[uncategorized] ~16-~16: This expression is usually spelled with a hyphen.
Context: ...e ProposalTimeout is reached - MUST use pull based gossip ## Decision TBD ## Detailed D...(BASED_HYPHEN)
docs/architecture/assets/adr024/connecting_to_consensus.md (7)
1-4: Enhance Header HyphenationThe header “Backwards Compatible Block Propagation” would benefit from hyphenating the compound adjective. For example, use “Backwards‐Compatible Block Propagation.”
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: # Backwards Compatible Block Propagation This document is an ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
7-11: Correct Spelling and Terminology in IntroThere are a couple of typographical issues in this section:
- “seemless” should be corrected to “seamless.”
- “hotswapple” appears to be a misspelling; consider replacing it with “hotswappable” or “hot‐swappable.”
15-17: Fix Typographic and Possessive ErrorsIn the sentence discussing entry points, “the consenus reactors internal message channel” should be revised to “the consensus reactor’s internal message channel” to fix both the spelling of “consensus” and to correctly apply the possessive form.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...s that exist now. That is, the consenus reactors internal message channel to the consens...(AI_HYDRA_LEO_APOSTROPHE_S_XS)
22-75: Review of syncData Function ImplementationThe Go function
syncDatais well structured and clearly handles synchronization. One suggestion is to verify that the channelcs.peerMsgQueueis appropriately buffered to avoid blocking in high-throughput scenarios.
78-86: Clarify Legacy Propagation ExplanationThe explanatory text before the
legacyPropagationcode can be made more direct. For instance, instead of “Something along the lines of the below code should suffice,” consider “The following code illustrates the legacy propagation approach.” This change improves clarity and professionalism.🧰 Tools
🪛 LanguageTool
[style] ~84-~84: ‘along the lines of’ might be wordy. Consider a shorter alternative.
Context: ...tines are simply not spun up. Something along the lines of the below code should suffice. ```go f...(EN_WORDINESS_PREMIUM_ALONG_THE_LINES_OF)
87-104: Improve Variable Naming in legacyPropagationWithin the
legacyPropagationfunction, the variablelegacyblockPropcould be more readable aslegacyBlockProp(using camelCase) to enhance consistency in naming.
106-116: Refine Text in the Parity Data SectionA few improvements are suggested in this section:
- Change “pull based gossip” to “pull‐based gossip.”
- Correct the misspelling “Simulataneously” to “Simultaneously.”
- Consider substituting phrases like “At the moment” with “Currently” to streamline the text.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: This expression is usually spelled with a hyphen.
Context: ...ly advantageous for broadcast trees and pull based gossip. However, the added parity data ...(BASED_HYPHEN)
[style] ~110-~110: For conciseness, consider replacing this expression with an adverb.
Context: ...res being committed to by the proposer. At the moment, the proposer commits over the block da...(AT_THE_MOMENT)
[style] ~111-~111: Consider a shorter alternative to avoid wordiness.
Context: ...the block data via thePartSetHeader. In order to be backwards compatible, we can't break...(IN_ORDER_TO_PREMIUM)
[style] ~113-~113: Consider a shorter alternative to avoid wordiness.
Context: ...a requiring commitments computed twice. In order to solve this dilemma, we can simply reuse...(IN_ORDER_TO_PREMIUM)
docs/architecture/assets/adr024/messages.md (5)
28-32: Correct Misspelling in NoteIn the note following the
CompactBlockdefinition, “siganture” is misspelled. Please change it to “signature.”
33-40: Fix Typographical ErrorThe word “messasges” (referring to the
Havemessages) is misspelled. Please correct it to “messages.”
89-100: Refine Explanatory Wording in Parity Data SectionThe explanation regarding parity data can be tightened for clarity. Consider:
- Revising “chunked in an even size” to “divided into even‐sized chunks.”
- Streamlining the sentence structure to reduce wordiness, for instance by replacing transitional phrases such as “however” when used improperly.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~94-~94: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...ing means that the data must be chunked in an even size. All transactions in that ...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
[style] ~95-~95: Consider a shorter alternative to avoid wordiness.
Context: ...in that chunk must have been downloaded in order to use it alongside parity data to reconst...(IN_ORDER_TO_PREMIUM)
[typographical] ~96-~96: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ock. Most scenarios would likely be fine, however it would be possible for a node to have...(HOWEVER_SENTENCE)
[style] ~98-~98: Consider removing “of” to be more concise
Context: ..., but have no complete parts, rendering all of the parity data useless. The way to fix thi...(ALL_OF_THE)
122-131: Add Boundary Checks in SubParts MethodIn the
SubPartsmethod of theParttype, the slice operation assumes thatp.Bytesis sufficiently long. It would be prudent to add a boundary check before slicing to avoid potential panics if the byte slice isn’t the expected length.Suggested Diff:
- for i := uint32(0); i < SubPartsPerPart; i++ { - sps[i] = SubPart{ - Index: uint32(i), - Bytes: p.Bytes[i*SubPartSize : (i+1)*SubPartSize], - } - } + for i := uint32(0); i < SubPartsPerPart; i++ { + if len(p.Bytes) < int((i+1)*SubPartSize) { + panic(fmt.Sprintf("unexpected byte length: got %d, need at least %d", len(p.Bytes), int((i+1)*SubPartSize))) + } + sps[i] = SubPart{ + Index: i, + Bytes: p.Bytes[i*SubPartSize : (i+1)*SubPartSize], + } + }
133-145: Clarify Omitted Proof in PartFromSubPartsIn the
PartFromSubPartsfunction, note that the returnedPartdoes not set theProoffield. It might be helpful to add a comment explaining why this field is omitted or how it is handled in downstream logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/architecture/adr-024-high-throughput-recovery.md(1 hunks)docs/architecture/assets/adr024/connecting_to_consensus.md(1 hunks)docs/architecture/assets/adr024/handlers_and_state.md(1 hunks)docs/architecture/assets/adr024/messages.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/assets/adr024/handlers_and_state.md
🧰 Additional context used
🪛 LanguageTool
docs/architecture/adr-024-high-throughput-recovery.md
[style] ~13-~13: Consider a shorter alternative to avoid wordiness.
Context: ... data after the block has been created. In order to utilize the data distributed before the...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~13-~13: The adjective “pull-based” is spelled with a hyphen.
Context: ...reated, the recovery phase must also be pull based. Therefore, the constraints for recover...
(BASED_HYPHEN)
[uncategorized] ~16-~16: This expression is usually spelled with a hyphen.
Context: ...e ProposalTimeout is reached - MUST use pull based gossip ## Decision TBD ## Detailed D...
(BASED_HYPHEN)
docs/architecture/assets/adr024/connecting_to_consensus.md
[uncategorized] ~1-~1: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: # Backwards Compatible Block Propagation This document is an ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~16-~16: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...s that exist now. That is, the consenus reactors internal message channel to the consens...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
[style] ~84-~84: ‘along the lines of’ might be wordy. Consider a shorter alternative.
Context: ...tines are simply not spun up. Something along the lines of the below code should suffice. ```go f...
(EN_WORDINESS_PREMIUM_ALONG_THE_LINES_OF)
[uncategorized] ~108-~108: This expression is usually spelled with a hyphen.
Context: ...ly advantageous for broadcast trees and pull based gossip. However, the added parity data ...
(BASED_HYPHEN)
[style] ~110-~110: For conciseness, consider replacing this expression with an adverb.
Context: ...res being committed to by the proposer. At the moment, the proposer commits over the block da...
(AT_THE_MOMENT)
[style] ~111-~111: Consider a shorter alternative to avoid wordiness.
Context: ...the block data via the PartSetHeader. In order to be backwards compatible, we can't break...
(IN_ORDER_TO_PREMIUM)
[style] ~113-~113: Consider a shorter alternative to avoid wordiness.
Context: ...a requiring commitments computed twice. In order to solve this dilemma, we can simply reuse...
(IN_ORDER_TO_PREMIUM)
docs/architecture/assets/adr024/messages.md
[uncategorized] ~94-~94: The preposition “into” seems more likely in this position than the preposition “in”.
Context: ...ing means that the data must be chunked in an even size. All transactions in that ...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_IN_INTO)
[style] ~95-~95: Consider a shorter alternative to avoid wordiness.
Context: ...in that chunk must have been downloaded in order to use it alongside parity data to reconst...
(IN_ORDER_TO_PREMIUM)
[typographical] ~96-~96: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ock. Most scenarios would likely be fine, however it would be possible for a node to have...
(HOWEVER_SENTENCE)
[style] ~98-~98: Consider removing “of” to be more concise
Context: ..., but have no complete parts, rendering all of the parity data useless. The way to fix thi...
(ALL_OF_THE)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: docker-txsim-build / docker-build (GHCR; ghcr.io/celestiaorg/txsim)
- GitHub Check: docker-security-build / docker-build (GHCR; ghcr.io/celestiaorg/celestia-app)
- GitHub Check: test / test-race
- GitHub Check: test / test-short
- GitHub Check: test / test-fuzz
- GitHub Check: test / test-coverage
- GitHub Check: test / test
- GitHub Check: markdown-linter / markdown-link-check
- GitHub Check: Summary
🔇 Additional comments (3)
docs/architecture/adr-024-high-throughput-recovery.md (2)
1-6: Initial Document Structure ApprovedThe ADR title, changelog, status, and introduction sections are clearly laid out and provide a solid starting point for further elaboration.
22-27: Good Linking of Detailed Design ReferencesThe detailed design section cleanly references related files (Messages, Handlers and State, Connecting to Consensus). Please double-check that these links remain valid as the related documents evolve.
docs/architecture/assets/adr024/messages.md (1)
1-5: Document Structure and ClarityThe introductory part of the messages document is clear and provides a concise overview of the PBBT message types.
Overview
still a draft for now as we'll add more details as the implementation comes along