chore: integration tests

[tac0turtle]

Overview

[github-actions]

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

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Sep 12, 2025, 4:05 PM

[claude]

Code Review: Integration Tests PR #2658

Thank you for working on improving the integration tests! I've reviewed the changes and have the following feedback:

✅ Positive Changes

1. Correct Logic Fix in isEmptyDataExpected() (block/internal/syncing/da_retriever.go:297)

   • The previous logic had a negation error (!bytes.Equal) that would incorrectly identify empty data
   • The fix properly returns true when DataHash is either nil/empty OR equals to DataHashForEmptyTxs
   • This aligns with the behavior throughout the codebase where empty blocks are handled

2. Test Assertion Fix (block/internal/syncing/da_retriever_test.go:189)

   • Updated test to correctly expect true when DataHash equals DataHashForEmptyTxs
   • Test now accurately reflects the intended behavior

3. Error Message Improvement (node/helpers.go:124)

   • Changed error message from "syncer not initialized" to "submitter not initialized"
   • This accurately reflects the actual component being checked (fn.blockComponents.Submitter)

🔍 Code Quality & Best Practices

The changes follow Go best practices and maintain consistency with the existing codebase patterns. The logic is now clearer and more maintainable.

🐛 Potential Issues

None identified. The logic fix addresses what appears to be a critical bug in determining when empty data is expected for a header.

⚡ Performance Considerations

No performance impact - these are simple boolean logic fixes that don't introduce any additional computational overhead.

🔒 Security Concerns

No security issues identified. The changes properly handle empty data validation which is important for block integrity.

✅ Test Coverage

The test has been updated to match the corrected logic. The test suite should now properly validate the empty data detection behavior.

📝 Recommendations

1. PR Description: Please update the PR description to explain what integration tests are being fixed and why these changes were necessary. This helps reviewers understand the context.

2. Consider adding more test cases for edge cases in isEmptyDataExpected():

   • Header with non-empty DataHash that doesn't match DataHashForEmptyTxs
   • Ensure the function behaves correctly for all scenarios

3. Documentation: The empty line at line 296 in isEmptyDataExpected() could be removed for cleaner code.

Conclusion

These are good fixes that correct important logic errors. Once the PR description is updated with more context about the integration tests being addressed, this should be ready to merge.

Great work on catching and fixing these issues! 👍

[claude]

Updated Code Review: Integration Tests PR #2658

I've reviewed all the changes in this PR, including the recent syncer optimizations. Here's my comprehensive feedback:

✅ Positive Changes

1. Critical Bug Fix in isEmptyDataExpected() (block/internal/syncing/da_retriever.go:296)

• Fixed logic error where !bytes.Equal incorrectly identified empty data
• Now correctly returns true when DataHash is nil/empty OR equals DataHashForEmptyTxs
• Tests updated to match corrected behavior

2. Performance Optimization in Syncer (block/internal/syncing/syncer.go)

• Removed ticker-based polling: Replaced time.NewTicker with a more efficient continuous retrieval approach
• Smart backoff strategy: Implements intelligent backoff only for "height-from-future" errors, avoiding unnecessary delays
• CPU-friendly idle behavior: Uses runtime.Gosched() to yield CPU when no events are available
• Non-blocking P2P processing: Changed from blocking selects to opportunistic processing with defaults

3. Accurate Error Messages (node/helpers.go:114,124)

• Fixed component reference from Syncer to Submitter in error messages

🔍 Performance Analysis

The syncer changes represent a significant performance improvement:

• Lower latency: DA retrieval now happens continuously instead of on fixed intervals
• Better resource utilization: CPU yielding prevents tight spinning
• Adaptive behavior: Smart backoff only when needed (HFF errors)
• Improved throughput: Can process DA events as fast as they arrive

⚠️ Potential Issues

1. CI Failures: Integration tests are failing. This could be due to:

   • Tests expecting the old ticker-based timing behavior
   • Race conditions exposed by faster processing
   • Need to update test expectations for the new continuous retrieval pattern

2. Backoff Edge Case: When config.DA.BlockTime.Duration is 0 or negative, the code defaults to 2 seconds. Consider logging a warning when this fallback is used.

📝 Recommendations

1. Fix Integration Tests: The failing tests need attention before merging
2. Add metrics/logging: Consider adding debug logs for backoff events to help diagnose issues in production
3. Document the change: The shift from ticker-based to continuous polling is a significant architectural change that should be documented
4. Clean up: Remove the empty line at line 296 in isEmptyDataExpected()

🔒 Security Review

No security concerns identified. The changes maintain proper error handling and don't expose sensitive information.

Conclusion

These are excellent optimizations that should improve sync performance significantly. However, the integration test failures must be resolved before merging. The shift from ticker-based to continuous DA retrieval is a smart optimization that better utilizes available resources.

Please address the failing tests and consider adding documentation about the new sync behavior. Once tests pass, this PR will be ready to merge! 🚀

[claude]

Code Review for PR #2658: Integration Tests

Summary

This PR improves the syncing mechanism and fixes a logic bug in the DA retriever. The changes enhance performance by removing the fixed ticker-based approach in favor of continuous processing with intelligent backoff.

✅ Strengths

1. Critical Bug Fix: The fix in isEmptyDataExpected() (da_retriever.go:296) correctly changes the logic from !bytes.Equal to bytes.Equal, which now properly identifies empty data scenarios.

2. Performance Improvement: The new sync loop eliminates the fixed ticker delay, allowing for faster DA retrieval when data is available while implementing smart backoff for "height from future" errors.

3. Improved Error Handling: The enhanced isHeightFromFutureError() function now properly checks for both coreda.ErrHeightFromFuture and uses string matching as a fallback.

🔍 Areas for Improvement

1. CPU Usage Concern: The tight loop with only runtime.Gosched() (syncer.go:334) may still consume significant CPU when idle. Consider adding a small sleep:

   if !processedP2P && nextDARequestAt.After(now) {
       time.Sleep(10 * time.Millisecond)
   }

2. TODO Comment: There's a TODO at line 251 that should be addressed or tracked in an issue.

3. Channel Blocking: The non-blocking sends to heightInCh (lines 284, 304, 322) drop events when the channel is full. Consider implementing a buffer overflow strategy or increasing channel capacity.

4. Test Assertion: The helper function change from checking Syncer to Submitter (node/helpers.go:113) appears correct but ensure this aligns with the actual component responsible for DA inclusion tracking.

🔒 Security Considerations

No security issues identified. The changes maintain proper context cancellation and error handling.

📊 Test Coverage

The test updates properly reflect the logic change in isEmptyDataExpected(). However, consider adding integration tests for:

• The new continuous sync behavior
• Backoff mechanism under "height from future" scenarios
• Performance under high load

🎯 Recommendations

1. Add metrics/logging to track sync performance and backoff occurrences
2. Consider making the backoff multiplier configurable
3. Document the sync strategy change in code comments for future maintainers
4. Address the CPU usage concern to prevent resource waste during idle periods

Verdict

The changes look good overall, with the critical bug fix and performance improvements being valuable additions. The minor suggestions above would further enhance the robustness of the implementation.

Approved with suggestions ✅

[julienrbrt]

ACK. Good catch! Merging this right now, as the base PR is being reviewed as we speak.