docs: add share version 1 to specs #3955
Conversation
rootulp
requested review from
rach-id and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe changes include the addition of a new constant Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (2)
specs/src/shares.md (2)
53-63: LGTM with suggestion: Clarify the purpose of thesignerfieldThe new "Share Version 1" section clearly explains the structural difference between share versions 0 and 1, including a helpful visual representation. However, it would be beneficial to provide more context about the
signerfield:
- What does the
signerfield represent?- How is it used in the system?
- Why was it necessary to introduce this field in version 1?
Adding this information would help readers understand the significance of this change and its impact on the system.
Line range hint
65-95: LGTM with suggestion: Update visual representationsThe updates to the "Transaction Shares" section provide clear and detailed information about the
SHARE_RESERVED_BYTESfield and its placement in different types of shares. This additional clarity is valuable for understanding the structure of transaction shares.However, I noticed that the visual representations (Figure 3 and Figure 4) have not been updated to reflect these changes. To maintain consistency and avoid confusion, consider updating these figures to include the
SHARE_RESERVED_BYTESfield in the appropriate positions for both the first share and continuation shares in a sequence.🧰 Tools
🪛 LanguageTool
[style] ~51-~51: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...itional metadata in their raw blob data prior to inclusion in a Celestia block. For exam...(EN_WORDINESS_PREMIUM_PRIOR_TO)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
specs/src/figures/first_share_with_signer.dotis excluded by!**/*.dotspecs/src/figures/first_share_with_signer.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
- specs/src/consensus.md (1 hunks)
- specs/src/shares.md (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
specs/src/shares.md (3)
26-29: LGTM: Clear introduction of share versioningThe new "Share Version" section provides a concise and clear explanation of share versioning. It correctly outlines the purpose of versioning and when a new version should be introduced.
Line range hint
97-131: LGTM: Helpful clarification on padding sharesThe updates to the "Padding" section provide useful clarification that clients may ignore the contents of padding shares. This information is valuable for implementers and helps to distinguish the significance of different types of shares in the system.
Line range hint
1-185: Overall assessment: Comprehensive update with minor suggestions for improvementThe changes to this specification document significantly enhance its completeness and clarity. The introduction of share versioning and the detailed explanation of Share Version 1 are particularly valuable additions. The updates to the Transaction Shares and Padding sections also provide important clarifications.
To further improve the document:
- Consider adding more context about the purpose and usage of the
signerfield in Share Version 1.- Update the visual representations in the Transaction Shares section to reflect the recent changes.
These changes align well with the PR objectives of updating the documentation to include specifications for share version 1 and addressing the gap in existing documentation.
To ensure consistency across the codebase, let's check for any references to share versions:
✅ Verification successful
Verification Successful: All references to "share version" are confined to
specs/src/shares.md.The review comment has been fully verified. There are no additional references to "share version" outside of the updated specification document, ensuring that the changes are well-contained and do not impact other areas of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to share versions in the codebase rg --type-add 'spec:*.md' --type spec 'share version' specs/Length of output: 1587
🧰 Tools
🪛 LanguageTool
[style] ~51-~51: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...itional metadata in their raw blob data prior to inclusion in a Celestia block. For exam...(EN_WORDINESS_PREMIUM_PRIOR_TO)
specs/src/consensus.md (1)
35-35: Verify usage ofSignerSizein other filesWhile the addition of
SignerSizeis correct and necessary for share version 1, it's important to ensure that this constant is properly utilized in other relevant parts of the codebase, such as share encoding/decoding logic or data structure definitions.Let's check for potential uses or updates needed in other files:
| @@ -32,6 +32,7 @@ | |||
| | `SHARE_INFO_BYTES` | `uint64` | `1` | `byte` | The number of bytes used for [share](data_structures.md#share) information | | |||
| | `SHARE_RESERVED_BYTES` | `uint64` | `4` | `byte` | The number of bytes used to store the index of the first transaction in a transaction share. Must be able to represent any integer up to and including `SHARE_SIZE - 1`. | | |||
| | `SHARE_SIZE` | `uint64` | `512` | `byte` | Size of transaction and blob [shares](data_structures.md#share), in bytes. | | |||
| | `SignerSize` | `int` | `20` | `byte` | The number of bytes used to store the signer in a [share](data_structures.md#share). | | |||
There was a problem hiding this comment.
💡 Codebase verification
SignerSize is defined but not used in the codebase
The SignerSize constant in specs/src/consensus.md is currently not referenced or utilized in any other .md or .rust files. Please verify if this constant is intended for future use or if it should be removed to prevent unnecessary clutter.
🔗 Analysis chain
New constant SignerSize added
The addition of the SignerSize constant is consistent with the PR objectives to include specifications for share version 1. This constant defines the number of bytes used to store the signer in a share, which is a key feature of share version 1.
To ensure this constant is used correctly throughout the codebase, let's run a search:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for uses of SignerSize in the codebase
rg --type md --type rust "SignerSize"
Length of output: 311
| @@ -32,6 +32,7 @@ | |||
| | `SHARE_INFO_BYTES` | `uint64` | `1` | `byte` | The number of bytes used for [share](data_structures.md#share) information | | |||
| | `SHARE_RESERVED_BYTES` | `uint64` | `4` | `byte` | The number of bytes used to store the index of the first transaction in a transaction share. Must be able to represent any integer up to and including `SHARE_SIZE - 1`. | | |||
| | `SHARE_SIZE` | `uint64` | `512` | `byte` | Size of transaction and blob [shares](data_structures.md#share), in bytes. | | |||
| | `SignerSize` | `int` | `20` | `byte` | The number of bytes used to store the signer in a [share](data_structures.md#share). | | |||
There was a problem hiding this comment.
Is it problematic that there's a different formatting i.e. SignerSize and not SIGNER_SIZE?
There was a problem hiding this comment.
Agreed it's inconsistent but the specs already have inconsistent casing (e.g. v1.Version). This PR uses the same casing as the constant defined in Go code.
Added #3660 (comment)
Closes #3867