-
Notifications
You must be signed in to change notification settings - Fork 69
Ignore verify blob proof #806
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
Conversation
WalkthroughA blob proof verification call in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node/derivation/derivation.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (rust)
| //if err := VerifyBlobProof(&blob, commitment, kzg4844.Proof(sidecar.KZGProof)); err != nil { | ||
| // d.logger.Error("Blob verification failed", "error", err) | ||
| // continue | ||
| //} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Blob proof verification has been disabled without justification.
Commenting out VerifyBlobProof bypasses the cryptographic validation that ensures blob data matches its KZG commitment. This verification is fundamental to EIP-4844 blob security and data availability guarantees in rollup systems.
Without verification, the system will accept:
- Corrupted blob data
- Malicious or tampered blobs
- Data that doesn't match the commitment hash
This could lead to state corruption, consensus failures, or security vulnerabilities in the rollup derivation process.
Required actions:
- Provide clear justification for why verification is being disabled
- If this is temporary (debugging/testing), add a TODO comment with a tracking issue
- Consider using a feature flag if verification needs to be optionally disabled
- Otherwise, restore the verification call immediately
If there's a legitimate reason for this change (e.g., verification is redundant due to upstream checks), please document it clearly in the code and PR description.
🤖 Prompt for AI Agents
node/derivation/derivation.go lines 362-365: the call to VerifyBlobProof was
commented out, disabling required KZG blob verification; restore the
VerifyBlobProof call as originally implemented so blobs are cryptographically
validated and log+continue on error, or if verification must be disabled
temporarily add a clear TODO with a tracking issue number and a short
justification comment; if you need opt-out behavior make it behind a feature
flag/config (default enabled) and gate the VerifyBlobProof call accordingly,
ensuring any removal or bypass is documented in the PR description and code
comments.
Summary by CodeRabbit