fix: make signature verification backwards compatible #2782
Conversation
|
Claude finished @tac0turtle's task —— View job Comprehensive Code ReviewI've performed a thorough review of the backwards compatible signature verification implementation. This is a well-architected solution for handling legacy header formats during network upgrades. Below are my findings across multiple dimensions: 1. Code Quality & Architecture ⭐⭐⭐⭐Strengths:
Areas for Improvement: 1.1 Memory Efficiency in Signature VerificationLocation: types/signed_header.go:124-145 and 203-224 The Recommendation: Extract the common payload verification logic: func (sh *SignedHeader) verifyPayload(payload []byte, tried [][]byte) (bool, [][]byte, error) {
if len(payload) == 0 {
return false, tried, nil
}
for _, p := range tried {
if bytes.Equal(p, payload) {
return false, tried, nil
}
}
tried = append(tried, payload)
verified, err := sh.Signer.PubKey.Verify(payload, sh.Signature)
if err != nil {
return false, tried, err
}
return verified, tried, nil
}1.2 Clone Method Should Be a Pointer MethodLocation: types/header.go:235-245 The Recommendation: Change to pointer receiver for clarity: func (h *Header) Clone() *Header {
if h == nil {
return nil
}
clone := *h
clone.LastHeaderHash = cloneBytes(h.LastHeaderHash)
// ... rest of cloning
return &clone
}2. Error Handling & Resilience ⭐⭐⭐⭐⭐Strengths:
Critical Security Issue: 2.1 Protowire Parsing Error HandlingLocation: types/serialization.go:450-496 The protowire parsing code correctly checks for negative return values indicating errors, but the error messages could be more descriptive for debugging. Recommendation: Add more context to protowire errors: if n < 0 {
return nil, fmt.Errorf("failed to parse protowire tag at offset %d: %w",
len(pHeader.ProtoReflect().GetUnknown())-len(unknown), protowire.ParseError(n))
}3. Security 🔒 ⭐⭐⭐⭐Strengths:
Security Concerns: 3.1 Potential Timing Attack in Payload VerificationLocation: types/signed_header.go:134 The sequential comparison using Severity: Low (requires local access and precise timing measurements) Recommendation: This is likely acceptable for this use case, but document the decision if timing attacks were considered. 3.2 Legacy Field Size ValidationLocation: types/serialization.go:474-478 Good validation of field sizes, but consider if 1KB is appropriate for all hash algorithms. Modern hash functions like SHA-256 use 32 bytes, but some systems might use longer identifiers. Recommendation: Document why 1KB was chosen and which hash algorithms are supported. 4. Performance & Resource Efficiency ⭐⭐⭐⭐Strengths:
Performance Issues: 4.1 Triple Marshal Operation in VerificationLocation: types/signed_header.go:161-177 The signature verification performs up to 3 marshaling operations:
Impact: For headers that require legacy verification, this means two failed marshal attempts before success. Recommendation:
4.2 Inefficient Legacy EncodingLocation: types/serialization.go:513-591 The Recommendation: Pre-calculate buffer size: func marshalLegacyHeader(h *Header) ([]byte, error) {
// ... existing validation ...
// Pre-calculate size to avoid reallocations
size := estimateLegacyHeaderSize(h)
payload := make([]byte, 0, size)
// ... rest of encoding ...
}
func estimateLegacyHeaderSize(h *Header) int {
// Rough estimate: fixed fields + variable length fields
size := 100 // overhead for tags and varints
size += len(h.LastHeaderHash)
size += len(h.DataHash)
// ... etc
return size
}5. Testing & Reliability ⭐⭐⭐Strengths:
Critical Coverage Gaps: 5.1 Missing Edge Case TestsRecommendation: Add tests for: // Test 1: Malformed protowire data
func TestDecodeLegacyHeaderFields_MalformedData(t *testing.T) {
// Test with truncated protowire data
// Test with negative field numbers
// Test with invalid wire types
}
// Test 2: Maximum size edge cases
func TestDecodeLegacyHeaderFields_MaxSize(t *testing.T) {
// Test with exactly maxUnknownFieldSize bytes
// Test with exactly maxLegacyHashSize bytes
// Test with maxSize + 1 (should fail)
}
// Test 3: Multiple legacy fields in unknown fields
func TestDecodeLegacyHeaderFields_AllLegacyFields(t *testing.T) {
// Test with all three legacy fields populated
// Test with duplicate field numbers
// Test with fields in different orders
}
// Test 4: Concurrent signature verification
func TestSignedHeader_ConcurrentValidation(t *testing.T) {
// Verify thread-safety of ValidateBasic
// Ensure no race conditions in signature verification
}
// Test 5: Legacy encoding produces identical bytes
func TestMarshalLegacyHeader_Deterministic(t *testing.T) {
// Verify that legacy encoding is deterministic
// Encode same header multiple times, verify bytes match
}5.2 Insufficient Benchmark CoverageLocation: No benchmark tests found Recommendation: Add benchmarks to track performance: func BenchmarkSignedHeader_ValidateBasic_SlimFormat(b *testing.B)
func BenchmarkSignedHeader_ValidateBasic_LegacyFormat(b *testing.B)
func BenchmarkMarshalBinaryLegacy(b *testing.B)
func BenchmarkDecodeLegacyHeaderFields(b *testing.B)5.3 Test Coverage NumbersThe codecov report shows:
Critical: The signature verification fallback logic (lines 124-185 in signed_header.go) appears to have very low coverage. This is the core security mechanism and MUST be thoroughly tested. 6. Documentation & Developer Experience ⭐⭐⭐⭐⭐Strengths:
Minor Improvements: 6.1 Add Examples to Migration GuideLocation: types/header.go:142-189 Recommendation: Add code examples: // ## Example: Creating a Legacy Header for Testing
//
// header := &Header{
// BaseHeader: BaseHeader{Height: 1, Time: now, ChainID: "test"},
// // ... other fields ...
// Legacy: &LegacyHeaderFields{
// LastCommitHash: make(Hash, 32),
// ConsensusHash: make(Hash, 32),
// LastResultsHash: make(Hash, 32),
// },
// }
// header.ApplyLegacyDefaults() // Ensures ConsensusHash is initialized6.2 Document Performance CharacteristicsLocation: types/signed_header.go:109 Recommendation: Add complexity analysis: // ValidateBasic performs signature verification using a multi-format fallback:
// 1. Custom provider (if set)
// 2. Slim format (new format)
// 3. Legacy format (old format with deprecated fields)
//
// Performance: O(3) marshal operations in worst case (legacy headers).
// Memory: O(1) additional allocation for duplicate tracking slice.
// Network: For historical sync, ~99% of headers should verify on first or second attempt.7. Additional Findings7.1 Inconsistent Error VariablesLocation: types/signed_header.go:93-107 Some errors are package-level variables (good), but the list is incomplete. Recommendation: Consider adding: var (
ErrCustomProviderFailed = errors.New("custom signature provider failed")
ErrSlimFormatVerificationFailed = errors.New("slim format signature verification failed")
ErrLegacyFormatVerificationFailed = errors.New("legacy format signature verification failed")
)This would help with debugging by indicating which format failed. 7.2 Missing Validation in ApplyLegacyDefaultsLocation: types/header.go:226-231
Recommendation: Add validation or document assumptions: // ApplyLegacyDefaults ensures the Header has a Legacy block initialised with
// the expected defaults. This should only be called on headers that require
// legacy encoding (e.g., when MarshalBinaryLegacy is used).
//
// Note: This mutates the header in place. For immutability, use Clone() first.7.3 Potential Nil Pointer DereferenceLocation: types/serialization.go:559, 569
Current code is safe because Recommendation: Add defensive check or comment: clone.ApplyLegacyDefaults()
// Legacy is guaranteed non-nil after ApplyLegacyDefaults
if len(clone.Legacy.ConsensusHash) > 0 {
payload = appendBytesField(payload, legacyConsensusHashField, clone.Legacy.ConsensusHash)
}Summary & Priority RecommendationsMust Fix (High Priority):
Should Fix (Medium Priority):
Nice to Have (Low Priority):
Overall AssessmentThis is a solid, production-ready implementation of backwards compatible signature verification. The architecture is well thought out, with good separation of concerns and comprehensive documentation. The main concern is the low test coverage (especially for the signature verification fallback logic), which should be addressed before merging to ensure reliability during network upgrades. Recommendation: Address the test coverage issues, then this PR is ready to merge. |
|
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 #2782 +/- ##
==========================================
- Coverage 62.86% 62.71% -0.15%
==========================================
Files 82 82
Lines 6912 7081 +169
==========================================
+ Hits 4345 4441 +96
- Misses 2063 2118 +55
- Partials 504 522 +18
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:
|
* main: fix: make signature verification backwards compatible (#2782) chore: adding upgrade test for evm-single (#2780) refactor: replace interface{} with any for clarity and modernization (#2781) feat: replay mechanism to sync node with execution layer (#2771) docs: update readme for sync pkg (#2776) build(deps): Bump the all-go group across 6 directories with 4 updates (#2772) refactor: remove obsolete // +build tag (#2774) build(deps): Bump vite from 5.4.20 to 5.4.21 in /docs in the npm_and_yarn group across 1 directory (#2775) build(deps): Bump actions/setup-node from 5 to 6 (#2773)
Overview
This pr creates a custom encoding and decoding function to decode and encode legacy header to allow current networks to sync from genesis even with the new slimmed down header type