-
Notifications
You must be signed in to change notification settings - Fork 69
Fix bug for invalid sequencer BLS key #808
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
WalkthroughIn Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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: 0
🧹 Nitpick comments (1)
node/core/sequencers.go (1)
98-103: Existing safeguard partially mitigates the risk; review comment's suggestions address additional concerns.The code does have a safeguard at lines 41-43 in
verifySignature()that checksif len(e.valsByTmKey) == 0and returns an error. This will catch the catastrophic case of an empty validator set after the update at line 115 assigns potentially emptyvalsByTmKeyto global state.However, this safeguard is:
- Reactive, not proactive (catches at verification time, not at update time)
- Partial (only catches empty sets, not reduced/insufficient validator sets)
- Limited in observability (error detection without metrics or alerting for skipped keys)
The review comment's suggested improvements—validating the resulting set before state assignment (lines 115-116), logging invalid counts, and optionally enforcing minimum thresholds—address these gaps and would improve operational safety and observability. These are valid recommendations beyond what the existing safeguard provides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
node/core/sequencers.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). (9)
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: check
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
Summary by CodeRabbit