Skip to content

Conversation

@julienrbrt
Copy link
Member

Extract basic da logic into da client and fi retriever.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2025

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedNov 17, 2025, 2:22 PM

@claude
Copy link
Contributor

claude bot commented Nov 17, 2025

Claude finished @julienrbrt's task —— View job


Code Review Complete

Tasks

  • Review architectural changes and DA logic extraction
  • Examine new DA client implementation
  • Examine new forced inclusion retriever implementation
  • Review sequencer changes (single and based)
  • Assess test coverage and quality
  • Check error handling and concurrency patterns
  • Verify security considerations
  • Provide comprehensive feedback

Summary

This PR successfully extracts DA (Data Availability) logic into well-structured, reusable components. The refactoring creates a cleaner separation of concerns between DA operations, forced inclusion retrieval, and syncing logic. The new block/internal/da package provides a solid foundation for DA interactions across the codebase.

Overall Assessment: Good refactoring with solid architecture, but test coverage needs improvement (64.55% patch coverage with 162 missing lines).


1. Code Quality & Architecture

Strengths ✅

  1. Excellent Separation of Concerns - The extraction of DAClient and ForcedInclusionRetriever into dedicated components is well-designed:

    • block/internal/da/client.go:16-28 - Clean interface definition
    • block/internal/da/forced_inclusion_retriever.go:18-24 - Single responsibility for FI logic
    • Public API exposure through block/public.go:36-74 provides controlled access
  2. Strong Encapsulation - Namespace handling is properly abstracted:

    • block/internal/da/client.go:52-73 - Pre-calculated namespace bytes in constructor
    • Configuration consolidated in da.Config struct
    • No namespace byte manipulation leaked to callers
  3. Idiomatic Go Patterns:

    • Proper use of atomic.Uint64 for concurrent DA height tracking
    • Context propagation throughout
    • Clean interface definitions

Issues & Recommendations 🔧

Critical: Unsafe Pointer Dereferencing

// block/internal/da/forced_inclusion_retriever.go:146-150
func (r *ForcedInclusionRetriever) processForcedInclusionBlobs(
	event *ForcedInclusionEvent,
	lastProcessedHeight *uint64,  // ⚠️ Pointer parameter
  • Issue: The lastProcessedHeight pointer is dereferenced without nil checks (*lastProcessedHeight = height)
  • Risk: Potential panic if called with nil pointer
  • Fix: Add nil check or use value type with return value

Moderate: Magic Numbers

// block/internal/da/client.go:218
batchSize := 100
  • Issue: Hardcoded batch size with no justification
  • Recommendation: Extract to named constant with documentation explaining the choice
  • Consider: Making it configurable if different DA layers have different optimal batch sizes

Minor: Inconsistent Error Handling

// block/internal/da/client.go:144-147
if len(ids) > 0 {
    height, _, err = coreda.SplitID(ids[0])
    if err != nil {
        c.logger.Error().Err(err).Msg("failed to split ID")
        // ⚠️ Error logged but not propagated - height remains 0
    }
}
  • Issue: Error is logged but execution continues with potentially invalid height
  • Recommendation: Either return error or add fallback logic with clear documentation

Code Organization

// sequencers/common/size_validation.go:3
// TODO(@julienrbrt): technically we may need to check for block gas as well
  • Good: TODO properly attributed
  • Recommendation: Consider creating a GitHub issue to track this future enhancement

2. Error Handling & Resilience

Strengths ✅

  1. Comprehensive Error Categorization - DA client properly handles all status codes:

    • block/internal/da/client.go:86-129 - Context cancellation, timeout, mempool errors, size limits
    • Clear separation between retryable and permanent errors
  2. Proper Timeout Management:

    • block/internal/da/client.go:166-167 - Context with timeout for GetIDs
    • block/internal/da/client.go:223-225 - Deferred cancellation in batch processing
  3. Graceful Degradation:

    • sequencers/single/sequencer.go:135-153 - Forced inclusion failures don't crash execution
    • Empty events created on FI retrieval errors

Issues & Recommendations 🔧

Critical: Context Deadline vs Cancellation

// block/internal/da/client.go:166-203
getIDsCtx, cancel := context.WithTimeout(ctx, c.defaultTimeout)
defer cancel()
idsResult, err := c.da.GetIDs(getIDsCtx, height, namespace)
if err != nil {
    // String matching for error detection
    if strings.Contains(err.Error(), coreda.ErrBlobNotFound.Error()) {
  • Issue: Using string matching instead of errors.Is() for sentinel errors
  • Risk: Fragile error detection that breaks if error messages change
  • Fix: Ensure ErrBlobNotFound and ErrHeightFromFuture are properly wrapped with %w in DA implementations

Moderate: Batch Retrieval Error Handling

// block/internal/da/client.go:220-238
for i := 0; i < len(idsResult.IDs); i += batchSize {
    getBlobsCtx, cancel := context.WithTimeout(ctx, c.defaultTimeout)
    batchBlobs, err := c.da.Get(getBlobsCtx, idsResult.IDs[i:end], namespace)
    cancel()
    if err != nil {
        // ⚠️ Early return on first batch failure
        return coreda.ResultRetrieve{...}
    }
  • Issue: Fails fast on first batch error - no partial success handling
  • Consideration: Depending on requirements, might want to collect partial results
  • Current Behavior: Acceptable if atomicity is required, but document this choice

Minor: Error Context Loss

// block/internal/da/forced_inclusion_retriever.go:159
return fmt.Errorf("failed to retrieve forced inclusion blobs at height %d: %s", height, result.Message)
  • Issue: Using %s instead of %w loses error chain
  • Impact: Cannot use errors.Is() or errors.As() on returned errors
  • Fix: Consider wrapping the error if result.Message contains structured error info

3. Security

Strengths ✅

  1. Input Validation:

    • sequencers/common/size_validation.go:14-16 - Blob size validation against absolute maximum
    • Size checks prevent DoS via oversized blobs
    • Cumulative size tracking prevents batch overflow
  2. Namespace Isolation:

    • Clear separation between header, data, and forced inclusion namespaces
    • Namespace bytes pre-calculated to prevent tampering
  3. Signature Validation (in DA retriever):

    • block/internal/syncing/da_retriever.go:295-320 - Proper signature verification
    • Proposer address validation

Issues & Recommendations 🔧

Moderate: Empty Blob Filtering

// block/internal/da/forced_inclusion_retriever.go:163-167
for _, blob := range result.Data {
    if len(blob) > 0 {
        event.Txs = append(event.Txs, blob)
    }
}
  • Issue: Silent filtering of empty blobs
  • Security Consideration: Could hide data submission issues
  • Recommendation: Add debug logging for filtered empty blobs to aid debugging

Minor: Hardcoded Size Limit

// sequencers/common/size_validation.go:8
AbsoluteMaxBlobSize = 1.5 * 1024 * 1024 // 1.5MB
  • Issue: Hardcoded constant might not match all DA layer limits
  • Recommendation: Make this configurable or document the DA layer assumption (appears to be Celestia's limit)
  • Future-proofing: Consider adding DA layer specific validators

4. Performance & Resource Efficiency

Strengths ✅

  1. Efficient Batch Processing:

    • block/internal/da/client.go:218-239 - Batched blob retrieval (100 per batch)
    • Reduces round trips to DA layer
  2. Atomic Operations:

    • sequencers/single/sequencer.go:57 - Proper use of atomic.Uint64 for DA height
    • Lock-free concurrent access
  3. Pre-calculated Namespaces:

    • block/internal/da/client.go:64-71 - Namespace bytes calculated once in constructor
    • Avoids repeated string-to-bytes conversion

Issues & Recommendations 🔧

Moderate: Memory Allocation Pattern

// block/internal/da/client.go:219
blobs := make([][]byte, 0, len(idsResult.IDs))
  • Good: Pre-allocated capacity
  • Concern: For large result sets, this could allocate significant memory upfront
  • Recommendation: Consider streaming approach for very large datasets or add max result size validation

Minor: Repeated Context Creation

// block/internal/da/client.go:223-225
getBlobsCtx, cancel := context.WithTimeout(ctx, c.defaultTimeout)
batchBlobs, err := c.da.Get(getBlobsCtx, idsResult.IDs[i:end], namespace)
cancel()
  • Issue: Creating new context in tight loop
  • Impact: Minimal, but could be optimized
  • Recommendation: If performance critical, reuse timeout duration

Concurrency Consideration

// sequencers/single/sequencer.go:284-372
func (c *Sequencer) processForcedInclusionTxs(event *block.ForcedInclusionEvent, maxBytes uint64) [][]byte {
  • Observation: Pure function with no shared state - good for testability
  • Performance: Sequential processing is fine for current scale
  • Future: If FI txs become large, consider concurrent validation

5. Testing & Reliability

Strengths ✅

  1. Comprehensive Unit Tests:

    • block/internal/da/client_test.go - 525 lines covering all major paths
    • block/internal/da/forced_inclusion_retriever_test.go - 344 lines with good edge case coverage
    • Table-driven tests for validation functions
  2. Good Test Organization:

    • sequencers/common/size_validation_test.go - Clean, focused test cases
    • Proper use of test helpers and mocks
  3. Edge Case Coverage:

    • Empty batch handling
    • Context cancellation
    • Timeout scenarios
    • Epoch boundary conditions

Issues & Recommendations 🔧

Critical: Low Coverage on Key Files

From Codecov report:

  • sequencers/single/sequencer.go - 79 missing lines (12.22% patch coverage)
  • sequencers/based/based.go - 49 missing lines (37.17% patch coverage)

Specific Missing Coverage:

  1. Forced Inclusion Processing - sequencers/single/sequencer.go:284-372

    • Size validation edge cases
    • Pending transaction queue management
    • Overflow scenarios
  2. Based Sequencer Queue Logic - sequencers/based/sequencer.go:138-165

    • Queue trimming logic
    • MaxBytes boundary conditions

Recommendations:

// Add tests for:
// 1. Forced inclusion with maxBytes exactly matching cumulative size
// 2. Forced inclusion with pending queue overflow
// 3. Mixed valid/invalid blob sizes in single epoch
// 4. Multiple epochs with carry-over pending transactions

Missing Integration Tests:

  • No test covering full flow: FI retrieval → sequencer processing → batch creation
  • No test for DA client + FI retriever integration
  • Consider adding integration test in block/internal/da/ package

Test Isolation Improvement:

// sequencers/single/sequencer_test.go:44-46
mockRetriever.On("RetrieveForcedIncludedTxs", mock.Anything, mock.Anything).
    Return(nil, block.ErrForceInclusionNotConfigured).Maybe()
  • Good: Using mocks for isolation
  • Improvement: Add specific test cases that exercise actual FI retrieval logic with various scenarios

6. Documentation & Developer Experience

Strengths ✅

  1. Clear Interface Documentation:

    • block/internal/da/client.go:15-28 - Well-documented interface
    • Public API exposure through block/public.go provides good entry point
  2. Helpful Comments:

    • block/internal/da/client.go:112-113 - Explains why StatusTooBig uses debug logging
    • block/internal/syncing/syncer.go:121-130 - Clear component initialization flow
  3. Error Messages:

    • Descriptive error messages with context (height, counts, etc.)

Issues & Recommendations 🔧

Moderate: Missing Package Documentation

// block/internal/da/client.go:1
package da
// ⚠️ No package-level documentation
  • Add: Package comment explaining purpose, usage patterns, and relationship to core DA
  • Example:
// Package da provides reusable Data Availability layer client components.
// It wraps the core DA interface with common patterns for namespace handling,
// timeout management, and result processing.

Minor: Incomplete Function Documentation

// block/internal/da/client.go:75-76
// Submit submits blobs to the DA layer with the specified options.
func (c *client) Submit(ctx context.Context, data [][]byte, gasPrice float64, namespace []byte, options []byte) coreda.ResultSubmit {
  • Missing: Parameter descriptions (especially options and gasPrice)
  • Missing: Return value details and possible status codes

Configuration Documentation:

// block/internal/da/client.go:42-50
type Config struct {
    DA                       coreda.DA
    Logger                   zerolog.Logger
    DefaultTimeout           time.Duration
    Namespace                string
    DataNamespace            string
    ForcedInclusionNamespace string
}
  • Add: Field-level comments explaining purpose and valid values
  • Add: Example usage in package documentation

7. Architecture-Specific Observations

Excellent Patterns ✅

  1. Zero-Dependency Core Pattern Maintained:

    • DA client is in block/internal/da/ - correctly separated from core
    • Interfaces in core/da remain dependency-free
  2. Clean Dependency Injection:

    • sequencers/single/sequencer.go:62-73 - FI retriever injected as interface
    • Makes testing easier and maintains flexibility
  3. Modular Design:

    • DA client can be used independently
    • FI retriever depends only on DA client interface
    • Clear boundaries between syncing, submitting, and sequencing

Potential Improvements 🔧

Consideration: Interface Granularity

// block/internal/da/client.go:16-28
type Client interface {
    Submit(ctx context.Context, data [][]byte, gasPrice float64, namespace []byte, options []byte) coreda.ResultSubmit
    Retrieve(ctx context.Context, height uint64, namespace []byte) coreda.ResultRetrieve
    RetrieveHeaders(ctx context.Context, height uint64) coreda.ResultRetrieve
    RetrieveData(ctx context.Context, height uint64) coreda.ResultRetrieve
    RetrieveForcedInclusion(ctx context.Context, height uint64) coreda.ResultRetrieve
    // ... namespace getters
}
  • Current: Single large interface
  • Alternative: Consider splitting into Submitter and Retriever interfaces
  • Trade-off: Current approach is simpler; split might be over-engineering
  • Verdict: Current design is acceptable for now

8. Critical Issues Summary

Must Fix Before Merge 🚨

  1. Test Coverage: Add tests for sequencers/single/sequencer.go:284-372 (forced inclusion processing)
  2. Nil Pointer Safety: Add nil check in processForcedInclusionBlobs or change signature

Should Fix Before Merge ⚠️

  1. Error Handling: Replace string matching with errors.Is() in client.go:171-192
  2. Magic Number: Extract batch size constant with documentation
  3. Package Documentation: Add package-level comments to block/internal/da

Nice to Have (Future PRs) 💡

  1. Add integration tests for DA client + FI retriever
  2. Make blob batch size configurable
  3. Add structured error types for better error handling
  4. Document DA layer assumptions (Celestia-specific limits)

9. Positive Highlights 🌟

  1. Clean Refactoring - Successfully extracted complex DA logic without breaking existing functionality
  2. Good Test Foundation - New DA client and FI retriever have solid unit test coverage
  3. Proper Abstractions - Interface design allows for easy mocking and testing
  4. Size Validation - New common package provides reusable blob size validation
  5. Idiomatic Go - Code follows Go best practices and conventions

Conclusion

This is a well-executed refactoring that improves code organization and reusability. The main concern is the low test coverage on sequencer changes (12.22% for single, 37.17% for based). Before merging:

  1. Add tests for forced inclusion processing logic in sequencers
  2. Fix nil pointer risk in processForcedInclusionBlobs
  3. Improve error handling to use errors.Is() instead of string matching

Once these are addressed, this PR will be in excellent shape. The architectural improvements are solid and will make future DA-related changes easier to implement and test.

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

❌ Patch coverage is 64.55142% with 162 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.06%. Comparing base (b39f28c) to head (729e98f).

Files with missing lines Patch % Lines
sequencers/single/sequencer.go 12.22% 79 Missing ⚠️
sequencers/based/based.go 37.17% 44 Missing and 5 partials ⚠️
block/internal/da/forced_inclusion_retriever.go 79.51% 12 Missing and 5 partials ⚠️
block/internal/syncing/syncer.go 23.07% 10 Missing ⚠️
block/internal/da/client.go 97.59% 4 Missing ⚠️
block/public.go 81.81% 2 Missing ⚠️
block/internal/syncing/da_retriever.go 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           julien/fi    #2852      +/-   ##
=============================================
- Coverage      64.48%   64.06%   -0.43%     
=============================================
  Files             83       84       +1     
  Lines           7600     7711     +111     
=============================================
+ Hits            4901     4940      +39     
- Misses          2141     2212      +71     
- Partials         558      559       +1     
Flag Coverage Δ
combined 64.06% <64.55%> (-0.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@julienrbrt julienrbrt marked this pull request as ready for review November 17, 2025 14:22
@julienrbrt julienrbrt merged commit 60ad091 into julien/fi Nov 17, 2025
15 of 18 checks passed
@github-project-automation github-project-automation bot moved this to Done in Evolve Nov 17, 2025
@julienrbrt julienrbrt deleted the julien/fi-split branch November 17, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants