chore!: audit fixes #2764
chore!: audit fixes #2764
Conversation
|
Claude finished @tac0turtle's task —— View job Comprehensive Code Review - Audit Fixes PRI've completed a thorough code review of this PR focusing on code quality, error handling, security, performance, testing, and documentation. Here are my findings: SummaryThis PR introduces important audit fixes addressing error handling, validation, retry logic, and security improvements. The changes are generally well-implemented, but there are several areas that need attention, particularly around idiomatic Go patterns, test coverage, and consistency. Overall Assessment: The PR improves resilience and security, but needs refinements before merge. Key concerns include low test coverage (30.82%), some non-idiomatic error handling, and missing validation in critical paths. 1. Code Quality & ArchitectureCritical Issues❌ Non-Idiomatic Error Joining (block/internal/executing/executor.go:574-575) e.sendCriticalError(fmt.Errorf("failed to execute transactions: %w", err))
return types.State{}, fmt.Errorf("failed to execute transactions: %w", err)Issue: Wrapping the same error twice is redundant. Consider using Recommendation from reviewer @julienrbrt: // More idiomatic approach:
if err := s.exec.SetFinal(s.ctx, nextHeight); err != nil {
return errors.Join(fmt.Errorf("failed to set final height after %d attempts", attempt), err)
}Major Issues// block/internal/common/consts.go
const DefaultMaxBlobSize = 1.5 * 1024 * 1024
// pkg/cmd/run_node.go:29 also defines:
const DefaultMaxBlobSize = 1.5 * 1024 * 1024Issue: Same constant defined in multiple locations. As noted by @julienrbrt, these should be centralized. Recommendation: Use the common constant everywhere: // In executor.go:444
req := coresequencer.GetNextBatchRequest{
Id: []byte(e.genesis.ChainID),
MaxBytes: common.DefaultMaxBlobSize, // Use from common package
}if genesisErr, ok := err.(error); ok && strings.Contains(genesisErr.Error(), "already exists")Issue: Unnecessary type assertion - Recommendation: if err != nil && strings.Contains(err.Error(), "already exists")Minor Issuesℹ️ Redundant Error Logging (block/internal/reaping/reaper.go:149-150) if err != nil {
r.logger.Error().Err(err).Msg("failed to get txs from executor")
return fmt.Errorf("failed to get txs from executor: %w", err)
}Issue: Since errors are returned and logged at the call site with exponential backoff, the internal error logging is redundant. Recommendation: Remove redundant error logs when errors are properly returned and handled with retry logic. 2. Error Handling & ResilienceStrengths ✅
Areas for Improvementif err := e.validateBlock(currentState, header, data); err != nil {
e.sendCriticalError(fmt.Errorf("failed to validate block: %w", err))
e.logger.Error().Err(err).Msg("CRITICAL: Permanent block validation error - halting block production")
return fmt.Errorf("failed to validate block: %w", err)
}Issue: As noted by @julienrbrt, this is technically unnecessary since Recommendation: Consider removing the explicit halt logic here, or add a comment explaining why both mechanisms are needed.
3. SecurityCritical Issues❌ Key Export Security (pkg/cmd/keys.go:67-77) // Print a strong warning to stderr
cmd.PrintErrln("WARNING: EXPORTING PRIVATE KEY. HANDLE WITH EXTREME CARE.")
cmd.PrintErrln("ANYONE WITH ACCESS TO THIS KEY CAN SIGN MESSAGES ON YOUR BEHALF.")
privKeyBytes, err := file.ExportPrivateKey(keyPath, []byte(passphrase))
if err != nil {
return fmt.Errorf("failed to export private key: %w", err)
}
hexKey := hex.EncodeToString(privKeyBytes)
cmd.Println(hexKey) // ❌ Prints to stdoutIssues:
Recommendations:
defer func() {
for i := range privKeyBytes {
privKeyBytes[i] = 0
}
for i := range hexKey {
// Can't modify string, but avoid keeping reference
}
}()Major IssuespassphraseBytes, err := os.ReadFile(passphraseFile)
if err != nil {
return fmt.Errorf("failed to read passphrase from file '%s': %w", passphraseFile, err)
}
passphrase := strings.TrimSpace(string(passphraseBytes))Issues:
Recommendations: passphraseBytes, err := os.ReadFile(passphraseFile)
if err != nil {
return fmt.Errorf("failed to read passphrase file: %w", err) // Don't expose path
}
defer func() {
// Clear passphrase from memory
for i := range passphraseBytes {
passphraseBytes[i] = 0
}
}()
passphrase := strings.TrimSpace(string(passphraseBytes))
// Consider adding minimum length validation
if len(passphrase) < 12 {
return fmt.Errorf("passphrase must be at least 12 characters")
}Good Security Practices ✅
4. Performance & Resource EfficiencyStrengths ✅
Areas for Improvementif s.headerSubmissionMtx.TryLock() {
go func() {
defer s.headerSubmissionMtx.Unlock()
// ... submission logic
}()
}Issue: TryLock is used correctly, but spawning goroutines for every submission could lead to many concurrent goroutines. Recommendation: Consider using a worker pool pattern or limiting concurrent submissions via buffered channel/semaphore. marshaled := make([][]byte, len(items))Issue: Allocates new slice on every call. For large batches, this could be optimized. Recommendation: Consider using a sync.Pool for temporary slice allocations in hot paths. 5. Testing & ReliabilityCritical Issues❌ Low Test Coverage: 30.82% patch coverage (Codecov Report) Files with inadequate coverage:
Missing Test Cases
RecommendationsAdd tests for: // Example test structure needed
func TestReaper_ExponentialBackoff(t *testing.T) {
// Test backoff multiplier calculation
// Test max backoff capping
// Test backoff reset on success
}
func TestSubmitter_CriticalErrorHandling(t *testing.T) {
// Test ErrOversizedItem triggers halt
// Test error channel behavior
// Test retry exhaustion
}
func TestTimestampValidation_ClockDrift(t *testing.T) {
// Test MaxClockDrift boundary
// Test future timestamp rejection
// Test valid timestamp acceptance
}6. Documentation & Developer ExperienceStrengths ✅
Areas for Improvement
const DefaultMaxBlobSize = 1.5 * 1024 * 1024 // 1.5MB fallback blob size limitShould document:
7. Specific Reviewer Comments to AddressFrom @julienrbrt (2025-10-27):
8. Recommendations PriorityP0 - Must Fix Before Merge
P1 - Should Fix Before Merge
P2 - Consider for Follow-up
ConclusionThis PR makes important improvements to error handling, validation, and resilience. However, it needs refinements before merge: Key Actions Needed:
Overall: The architectural direction is sound, but the implementation needs polish to meet production quality standards. Review Completion Status
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2764 +/- ##
==========================================
- Coverage 62.71% 62.09% -0.63%
==========================================
Files 82 82
Lines 7081 7178 +97
==========================================
+ Hits 4441 4457 +16
- Misses 2118 2183 +65
- Partials 522 538 +16
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:
|
| delete(r.pendingData, height) | ||
| } | ||
|
|
||
| // CRITICAL: Validate that data matches the header's DataHash commitment |
There was a problem hiding this comment.
Are we discarding both header and data? Is that the expected behavior for DA sync?
tac0turtle
force-pushed
the
marko/audit
branch
from
fbed87d to
6cf8c4d
Compare
|
the upgrade test will continue to fail because the old binary doesnt have the flag that the new one has. I decided against backwards compat in order to push security onto people |
Overview