feat!: support authored blobs #3765
Conversation
|
cmwaters
requested review from
rootulp and
staheri14
and removed request for
a team
WalkthroughWalkthroughThe changes across the codebase primarily involve a transition from using constants and methods from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Application
participant BlobFactory
participant Share
User->>Application: Create Blob Request
Application->>BlobFactory: Generate Blob with Signer
BlobFactory->>Share: Create Blob (Version 0 or 1)
Share-->>BlobFactory: Return Blob
BlobFactory-->>Application: Return Created Blob
Application-->>User: Respond with Blob Information
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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, codebase verification and nitpick comments (3)
test/txsim/blob.go (1)
24-27: Add comments for the new fieldshareVersions.Consider adding comments to explain the purpose and usage of the
shareVersionsfield for better maintainability and readability.+ // shareVersions holds the possible share versions for blobs.x/blob/client/cli/payforblob.go (2)
Line range hint
127-133: Handle errors consistently when broadcasting PFB.Ensure that the error returned by
broadcastPFBis handled consistently. Consider logging the error or providing additional context if needed.if err != nil { fmt.Printf("Error broadcasting PFB: %v\n", err) return err }
Line range hint
142-146: Ensure consistent error handling for parsed blobs.The loop processing parsed blobs should handle errors consistently. Consider logging errors or adding context to error messages for better debugging.
if err != nil { fmt.Printf("Error processing parsed blob: %v\n", err) return err }
| // WithShareVersion provides the option of fixing a predefined share version for | ||
| // all blobs else it will randomly select a share version for each blob. | ||
| func (s *BlobSequence) WithShareVersion(version uint8) *BlobSequence { | ||
| if version != share.ShareVersionZero && version != share.ShareVersionOne { | ||
| panic(fmt.Sprintf("invalid share version %d", version)) | ||
| } | ||
| s.shareVersions = []uint8{version} | ||
| return s | ||
| } |
There was a problem hiding this comment.
Handle invalid share version more gracefully.
Instead of panicking, consider returning an error when an invalid share version is provided. This approach is more robust and user-friendly.
- panic(fmt.Sprintf("invalid share version %d", version))
+ return fmt.Errorf("invalid share version %d", version)Committable suggestion was skipped due to low confidence.
| d.Txs[0] = rawTx | ||
| d.Hash = calculateNewDataHash(t, d.Txs) | ||
| }, | ||
| appVersion: appconsts.LatestVersion, |
There was a problem hiding this comment.
[blocking] I think this test case should fail b/c app version is 2. Should this feature be gated behind an app version bump?
Ref: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-21.md#backwards-compatibility
There was a problem hiding this comment.
I was waiting on this until we have more information regarding your v3 prototype. At the moment it's not gated but it should be
| // ContinuationSparseShareContentSize is the number of bytes usable for data | ||
| // in a continuation sparse share of a sequence. | ||
| ContinuationSparseShareContentSize = ShareSize - NamespaceSize - ShareInfoBytes | ||
| DefaultShareVersion = share.ShareVersionZero |
There was a problem hiding this comment.
[question] why default to the previous share version when a new one exists?
There was a problem hiding this comment.
I actually don't know what is meant by the default here. I have left it as is because v1 is a new feature and I a) don't know if many people will use it and b) should be opt in anyway.
Maybe if this is just for tests we can change it but it would require a large refactor since the new version requires new arguments
| @@ -22,16 +22,16 @@ import ( | |||
| tmrand "github.com/tendermint/tendermint/libs/rand" | |||
| ) | |||
|
|
|||
| func TestNewBlob(t *testing.T) { | |||
| func TestNewV0Blob(t *testing.T) { | |||
There was a problem hiding this comment.
[question] do we want a new test in this file for TestNewV1Blob?
evan-forbes
added
WS: V3 3️⃣
|
started to address the versioning thing in #3769 |
There was a problem hiding this comment.
LGTM.
[follow-up] maybe we should have a follow-up issue to ensure the authored blobs feature is only supported for app version 3. Even if we adopt the multiplexed app prototype, that shouldn't prevent us from adding defensive app version checks for new features (like authored blobs) in low level tests like x/blob/types/blob_tx_test.go.
celestia-bot
requested review from
a team and
rach-id
and removed request for
a team
This PR uses the go-square to support authored blobs (using the v1 share version).
In this PR is the new block validation logic which ensures that all "authored" blobs have a signer that matches the signer of the PFB
TxSim now will randomly use blobs of share version 0 and 1