fix: codecv7 challenge digest #50
Conversation
WalkthroughThe changes update multiple codec versions to include a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Codec as DACodec (vX)
participant BlobFunc as constructBlob/constructBlobPayload
participant BatchMgr as newDABatch / Batch Constructor
Client->>Codec: NewDABatch(batch)
Codec->>BlobFunc: Construct blob (includes challengeDigest)
BlobFunc-->>Codec: (blob, blobVersionedHash, ..., challengeDigest)
Codec->>BatchMgr: Create new DABatch(..., challengeDigest)
BatchMgr-->>Codec: DABatch instance with challengeDigest field
Codec-->>Client: Return DABatch object
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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
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: 0
🧹 Nitpick comments (2)
encoding/codecv1.go (1)
460-475: Implementation of ChallengeDigestFromBlobBytes in DACodecV1The implementation correctly:
- Converts blob bytes to canonical form
- Creates a commitment from the blob
- Calculates a versioned hash
- Computes and returns the challenge digest
Unlike V7, this implementation doesn't perform length validation or padding before processing. Consider adding similar validation for consistency across versions.
func (d *DACodecV1) ChallengeDigestFromBlobBytes(blobBytes []byte) (common.Hash, error) { + if len(blobBytes) > maxEffectiveBlobBytes { + return common.Hash{}, fmt.Errorf("blobBytes length exceeds %d bytes, got %d bytes", maxEffectiveBlobBytes, len(blobBytes)) + } + + paddedBlobBytes := make([]byte, maxEffectiveBlobBytes) + copy(paddedBlobBytes, blobBytes) + blob, err := makeBlobCanonical(blobBytes) if err != nil { return common.Hash{}, fmt.Errorf("failed to convert blobBytes to canonical form: %w", err) } c, err := kzg4844.BlobToCommitment(blob) if err != nil { return common.Hash{}, fmt.Errorf("failed to create blob commitment: %w", err) } blobVersionedHash := kzg4844.CalcBlobHashV1(sha256.New(), &c) - challengeDigest := crypto.Keccak256Hash(crypto.Keccak256(blobBytes), blobVersionedHash[:]) + challengeDigest := crypto.Keccak256Hash(crypto.Keccak256(paddedBlobBytes), blobVersionedHash[:]) return challengeDigest, nil }encoding/codecv7.go (1)
368-390: Added ChallengeDigestFromBlobBytes implementation.The new method computes a challenge digest from blob bytes by:
- Validating the blob size
- Converting the bytes to canonical form
- Generating a KZG commitment
- Calculating a versioned hash
- Applying padding to standardize the input size
- Computing the final digest with Keccak256 hashing
This implementation properly handles size validation, canonical conversions, and padding, ensuring consistent digest calculation.
Consider adding a more detailed comment explaining the purpose of this method and how it relates to the overall data availability system. A brief explanation of why padding is necessary and how the challenge digest is used would improve maintainability.
-// ChallengeDigestFromBlobBytes calculates the challenge digest from the given blob bytes. +// ChallengeDigestFromBlobBytes calculates the challenge digest from the given blob bytes. +// This method: +// 1. Validates that the blob doesn't exceed the maximum allowed size +// 2. Converts the blob bytes to canonical form for KZG operations +// 3. Generates a KZG commitment and versioned hash +// 4. Pads the blob to a fixed size to ensure consistent hashing +// 5. Returns the Keccak256 hash of the padded blob and versioned hash +// +// The challenge digest is used for verifying data availability proofs and +// ensuring the integrity of blob data across the system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
encoding/codecv0.go(1 hunks)encoding/codecv1.go(1 hunks)encoding/codecv7.go(2 hunks)encoding/codecv7_test.go(6 hunks)encoding/codecv7_types.go(1 hunks)encoding/interfaces.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (10)
encoding/codecv0.go (1)
433-437: Implementation of ChallengeDigestFromBlobBytes in DACodecV0The function is correctly stubbed out in this legacy codec version, returning an empty hash and nil error. This maintains interface compatibility while acknowledging that V0 doesn't need to implement actual challenge digest calculation.
encoding/interfaces.go (1)
77-78: Interface extension with ChallengeDigestFromBlobBytesGood addition of the
ChallengeDigestFromBlobBytesmethod to the Codec interface. This method enables blob data processing across all codec versions, with a clean signature that matches the pattern of other methods in the interface.encoding/codecv7_types.go (1)
141-149: Improved error handling and padding for blob data processingThe changes enhance robustness by:
- Adding length validation to prevent processing oversized blobs
- Creating a padded byte slice of fixed size (maxEffectiveBlobBytes)
- Ensuring consistent input size for hash computation
This is a good improvement that helps prevent potential issues with variable-sized inputs.
encoding/codecv7_test.go (6)
825-825: Updated test expectations for blob data proof.The expected blob data proof for this test case has been updated to reflect the fix in the challenge digest calculation in the DACodecV7 implementation.
836-836: Updated test expectations for blob data proof.The expected blob data proof for this test case has been updated to match the corrected challenge digest calculation.
847-847: Updated test expectations for blob data proof.The expected blob data proof has been correctly updated to align with the fixed challenge digest calculation logic.
858-858: Updated test expectations for blob data proof.The expected blob data proof for this test case has been properly updated to reflect the challenge digest fix.
869-869: Updated test expectations for blob data proof.The expected blob data proof for the multi-block test case has been updated to match the fixed challenge digest calculation.
880-880: Updated test expectations for blob data proof.The expected blob data proof for this complex multi-block test case has been updated to reflect the corrected challenge digest algorithm.
encoding/codecv7.go (1)
13-13: Added necessary crypto package import.Added the crypto package import, which is required for the Keccak256Hash and Keccak256 functions used in the new ChallengeDigestFromBlobBytes method.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
encoding/codecv0.go (1)
434-437: Add parameter name for documentation clarityThe parameter name is missing, which makes it unclear what the expected input is. Consider naming the parameter for better code documentation and readability.
-func (d *DACodecV0) BlobDataProofFromBlobBytes([]byte) (common.Hash, kzg4844.Commitment, kzg4844.Proof, error) { +func (d *DACodecV0) BlobDataProofFromBlobBytes(blobBytes []byte) (common.Hash, kzg4844.Commitment, kzg4844.Proof, error) { return common.Hash{}, kzg4844.Commitment{}, kzg4844.Proof{}, nil }encoding/codecv1.go (2)
462-494: Add input validation for blob bytesThe function doesn't validate the input blob bytes before processing, which could lead to unexpected behavior if the data doesn't meet size or format requirements.
func (d *DACodecV1) BlobDataProofFromBlobBytes(blobBytes []byte) (common.Hash, kzg4844.Commitment, kzg4844.Proof, error) { + // Validate input + if len(blobBytes) == 0 { + return common.Hash{}, kzg4844.Commitment{}, kzg4844.Proof{}, fmt.Errorf("empty blob bytes") + } + blob, err := makeBlobCanonical(blobBytes) if err != nil { return common.Hash{}, kzg4844.Commitment{}, kzg4844.Proof{}, fmt.Errorf("failed to convert blobBytes to canonical form: %w", err) }
490-490: Simplify error message formattingThe error message includes hex encoding of the
zvalue which adds an import dependency only used in this error message. Unless this is critical for debugging, consider simplifying.- return common.Hash{}, kzg4844.Commitment{}, kzg4844.Proof{}, fmt.Errorf("failed to create KZG proof at point, err: %w, z: %v", err, hex.EncodeToString(z[:])) + return common.Hash{}, kzg4844.Commitment{}, kzg4844.Proof{}, fmt.Errorf("failed to create KZG proof at point: %w", err)If the hex encoding is truly needed for debugging, consider using a logging approach rather than including it in the error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
encoding/codecv0.go(1 hunks)encoding/codecv1.go(2 hunks)encoding/codecv7.go(2 hunks)encoding/interfaces.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- encoding/interfaces.go
- encoding/codecv7.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
good suggestion. @roynalnaruto could you provide a test case here? |
Purpose or design rationale of this PR
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Refactor
Bug Fixes