-
Notifications
You must be signed in to change notification settings - Fork 246
feat: Ensure Header integrity on DA #2948
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
base: main
Are you sure you want to change the base?
Conversation
|
Claude finished @alpe's task in 2m 53s —— View job Code Review CompleteI've completed a comprehensive review of the DAHeaderEnvelope implementation. This is a well-designed security enhancement that adds an envelope signature layer to DA headers. The implementation demonstrates solid architectural thinking with backward compatibility and a strict mode transition mechanism. Todo List
SummaryThis PR introduces a ✅ Strengths:
Detailed Review1. Code Quality & Architecture✅ Protobuf Schema Design (proto/evnode/v1/evnode.proto)Good:
message SignedHeader {
Header header = 1;
bytes signature = 2;
Signer signer = 3;
reserved 4; // ✅ Good: Prevents field number reuse
}
message DAHeaderEnvelope {
Header header = 1;
bytes signature = 2;
Signer signer = 3;
bytes envelope_signature = 4; // ✅ Good: Extends SignedHeader compatibly
}Concerns:
Recommendation: Add proto documentation explaining the binary compatibility guarantee and migration path: // DAHeaderEnvelope is a wrapper around SignedHeader for DA submission.
// Wire-format compatible with SignedHeader (fields 1-3).
// Migration: Nodes accept legacy SignedHeader until first valid envelope,
// then switch to strict envelope-only mode.
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and integrity of headers submitted to the Data Availability (DA) layer. It introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a DAHeaderEnvelope to wrap signed headers, enhancing security by adding an extra layer of signature verification for content submitted to the Data Availability layer. The implementation is well-designed, providing backward compatibility with legacy SignedHeader formats and transitioning to a 'strict mode' once the first valid envelope is detected, thereafter rejecting legacy formats. The changes are logical, and the addition of TestDARetriever_StrictEnvelopeMode_Switch provides good test coverage for the new behavior. My review includes one suggestion to simplify the code by removing a redundant check.
| if r.strictMode && !isValidEnvelope { | ||
| r.logger.Warn().Msg("strict mode: rejecting block that is not a fully valid envelope") | ||
| return nil | ||
| } |
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.
This check appears to be redundant. Based on the logic flow, if r.strictMode is true when this function is called, isValidEnvelope must also be true for execution to reach this line. Any case where r.strictMode is true and isValidEnvelope would be false (e.g., failed unmarshal, invalid signature, no signature) results in an early return from the function. Therefore, this conditional block seems to be unreachable and can be removed to simplify the code.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2948 +/- ##
==========================================
- Coverage 59.18% 58.86% -0.33%
==========================================
Files 90 90
Lines 8627 8715 +88
==========================================
+ Hits 5106 5130 +24
- Misses 2940 2998 +58
- Partials 581 587 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Introduce envelope for headers on DA to fail fast on unauthorized content.
Similar approach as in #2891 with a binary compatible sibling type that carries the additional information.
SignedHeaderproto typeSignedHeaderfor legacy support until first signed envelope read