fix(pkg/sync): avoid pruning goheader store

[tac0turtle]

Overview

avoid pruning data and headers from store for now, we will add pruning back in the future

[gemini-code-assist]

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request implements a temporary measure to prevent the pruning of stored data and headers within the synchronization service. By configuring a significantly extended pruning window, the system will effectively retain all historical data, ensuring no information is discarded until a more robust pruning strategy can be developed and integrated.

Highlights

• Temporary Pruning Disablement: The pull request introduces a mechanism to temporarily disable the pruning of data and headers from the store.
• Extended Pruning Window: This is achieved by setting the WithPruningWindow option for the goheadersync.Syncer to an extremely long duration (99 years).
• Future Re-enablement: A TODO comment indicates that this change is temporary and pruning will be re-enabled in the future.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[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	Oct 1, 2025, 9:56 AM

[gemini-code-assist]

Code Review

This pull request disables data and header pruning by setting a very large pruning window of 99 years. The changes are correct in achieving this goal. I've provided a couple of suggestions to improve code quality: one is to declare the duration as a constant instead of a variable, and the other is to remove a redundant type conversion.

[claude]

Code Review for PR #2718: fix: avoid pruning store

I've reviewed this pull request that temporarily disables pruning by setting an extremely long pruning window. Here's my comprehensive feedback:

✅ Code Quality and Best Practices

Positive:

• Simple and focused: The change is minimal and targeted, reducing the risk of introducing bugs
• Clear TODO comment: Line 36 explicitly states the intention to re-enable pruning in the future
• Descriptive variable name: ninetyNineYears clearly communicates the intent of the workaround

Suggestions for improvement:

1. Magic number calculation: The calculation 99 * 365 * 24 * time.Hour doesn't account for leap years. While this is acceptable for a temporary fix, consider using a constant or more explicit comment:

   // TODO: when we add pruning we can remove this
   // Set to ~99 years (effectively infinite for current use cases)
   var ninetyNineYears = 99 * 365 * 24 * time.Hour

2. Package-level variable: ninetyNineYears is currently unexported (lowercase), which is appropriate. Consider if const would be more suitable since the value never changes, though var is also acceptable for this use case.

🐛 Potential Bugs or Issues

No critical bugs identified, but some considerations:

1. Alignment with ADR-017: The codebase has ADR-017 (adr-017-node-pruning.md) which describes a comprehensive pruning system. This temporary workaround is sensible, but ensure there's a tracking issue to implement proper pruning configuration as described in the ADR.

2. Consistency across syncers: The change applies to both headerSync and dataSync through the shared newSyncer() function (lines 177-184, 391-397). This is correct, but verify that both sync types should have identical pruning behavior.

3. Store space considerations: According to ADR-017, the sync service stores headers and data "primarily for P2P gossiping purposes" and the contents are cached in headerCache and dataCache. Without pruning, nodes will accumulate all historical sync data indefinitely. Ensure node operators are aware of the storage implications.

⚡ Performance Considerations

1. Storage growth: Without pruning, the go-header store will grow unbounded. Based on ADR-017 line 13, this store is used for P2P gossiping, and data is cached elsewhere. However, the underlying datastore will still grow, potentially impacting:

   • Disk space requirements (especially problematic for long-running chains)
   • Database query performance as the dataset grows
   • Startup/initialization time

2. Memory vs disk trade-off: Since headers/data are cached in memory anyway, the store primarily serves as persistent P2P state. The 99-year window means essentially no cleanup happens, so monitor disk usage in production.

3. Recommendation: Document the expected growth rate in comments or documentation, and ensure monitoring is in place for storage metrics.

🔒 Security Concerns

No security issues identified. The change:

• Doesn't expose any new attack vectors
• Doesn't affect data validation or integrity
• Doesn't change network protocol behavior
• Simply affects local storage retention policy

✅ Test Coverage

Current state:

• The change is a configuration parameter modification, which is typically low-risk
• Existing tests in pkg/sync/sync_service_test.go should continue to pass

Recommendations:

1. Verify existing tests pass: Ensure that tests like TestHeaderSyncServiceRestart still work with the extended pruning window
2. Consider adding a test: While not critical for this temporary fix, consider adding a test that verifies the pruning window is set correctly:

   func TestNoPruningConfiguration(t *testing.T) {
       // Verify that syncer is created with extended pruning window
       // This serves as documentation of the temporary workaround
   }

3. Integration test coverage: When proper pruning is implemented, ensure integration tests cover:
   • Pruning behavior with different configurations
   • Store growth patterns with and without pruning
   • Restart behavior with pruned data

📋 Additional Recommendations

1. Create tracking issue: If not already done, create an issue to track the implementation of proper pruning configuration per ADR-017. Reference it in the TODO comment.

2. Documentation update: Consider updating relevant documentation (deployment guides, operational docs) to mention:

   • Current pruning behavior (disabled)
   • Expected storage growth patterns
   • Timeline for proper pruning implementation

3. Configuration hook for future: When implementing proper pruning, consider making it configurable via the node's config.Config structure, allowing operators to choose retention policies.

4. Monitoring recommendation: Add metrics or logs that track the sync store size so operators can monitor the impact of disabled pruning.

🎯 Summary

Overall assessment: ✅ Approve with suggestions

This is a reasonable temporary workaround to disable pruning while a proper solution is developed. The change is:

• ✅ Simple and low-risk
• ✅ Well-commented with clear intent
• ✅ Consistent with the ADR-017 approach to defer pruning implementation
• ⚠️ Should be tracked with a follow-up issue for proper implementation
• ⚠️ Operators should be made aware of storage implications

The code is ready to merge, but please consider the documentation and tracking issue recommendations above.

---

🤖 Generated with Claude Code

[claude]

PR Review: Avoid Pruning GoHeader Store

Summary

This PR temporarily disables pruning in the go-header store by setting an extremely long pruning window (99 years). This is a simple workaround until proper pruning is implemented per issue #2093.

Code Quality & Best Practices ✅

Strengths:

• Clean, minimal change that achieves the goal
• Clear TODO comment explaining the temporary nature
• Follows existing code patterns in newSyncer function

Suggestions for improvement:

1. Variable naming: The variable name ninetyNineYears is clear but could be more descriptive of its purpose:

   // Better option:
   var disablePruningWindow = 99 * 365 * 24 * time.Hour

2. Magic number: The 99 years is somewhat arbitrary. Consider:

   // More explicit:
   const (
       // disablePruningWindow is effectively infinite for practical purposes.
       // This will be replaced with a configurable pruning window per #2093
       disablePruningWindow = 99 * 365 * 24 * time.Hour
   )

3. TODO format: Consider adding the issue number for better traceability:

   // TODO(#2093): when we add pruning we can remove this

Potential Issues 🔍

1. Time calculation accuracy: The calculation 99 * 365 * 24 * time.Hour doesn't account for leap years. While this is likely intentional (it's meant to be a large number), it could be made more explicit:

   // Approximately 99 years (ignoring leap years)
   var ninetyNineYears = 99 * 365 * 24 * time.Hour

2. No validation: The code doesn't validate that the pruning window is actually being respected. Consider adding a log statement during initialization:

   logger.Info().Dur("pruning_window", ninetyNineYears).Msg("pruning disabled with long window")

Performance Considerations ⚡

Positive: This change prevents premature data loss and avoids potential issues with aggressive pruning.

Concern: Without pruning, node storage will grow unbounded over time. This is acceptable as a temporary measure, but teams should be aware:

• Monitor disk usage on long-running nodes
• Plan for proper pruning implementation (issue [EPIC]: Node Pruning #2093)
• Consider documenting this in deployment/operations guides

Recommendation: Add a metric or log warning when storage exceeds certain thresholds until proper pruning is implemented.

Security Considerations 🔒

No security concerns identified. The change:

• Doesn't expose new attack surfaces
• Doesn't handle sensitive data differently
• Is a conservative approach (keeping more data is safer than losing it)

Test Coverage 🧪

Current state:

• Existing tests in pkg/sync/sync_service_test.go cover the sync service restart flow
• Tests don't explicitly validate pruning behavior (which is acceptable given pruning is disabled)

Recommendations:

1. Add integration test to verify storage growth doesn't cause issues:

   func TestSyncServiceNoPruning(t *testing.T) {
       // Verify that old headers/data remain accessible
       // after a long sync period
   }

2. Future work: When implementing proper pruning ([EPIC]: Node Pruning #2093), add tests to verify:

   • Pruning window is respected
   • Headers/data outside window are removed
   • Headers/data inside window are retained

Alignment with CLAUDE.md ✅

The PR follows repository conventions:

• ✅ Minimal, focused change
• ✅ Clear commit message with semantic commits
• ✅ Modular design maintained
• ✅ No external dependencies added
• ⚠️ Missing: Test coverage for the new behavior (acceptable for this temporary fix)

Overall Assessment

Verdict: ✅ Approve with minor suggestions

This is a pragmatic, low-risk change that addresses an immediate need. The implementation is clean and straightforward. The suggestions above are minor improvements that could enhance code clarity and observability, but they don't block merging.

Action Items:

1. Consider adding issue reference to TODO comment
2. Consider adding a log statement about pruning being disabled
3. Plan for proper pruning implementation per [EPIC]: Node Pruning #2093
4. Monitor storage usage on production nodes

Great work on the clean, minimal fix! 🎉

[claude]

Pull Request Review: fix(pkg/sync): avoid pruning goheader store

Summary

This PR temporarily disables pruning in the goheader store by setting an extremely long pruning window (99 years), with plans to add proper pruning back in the future.

---

✅ Code Quality & Best Practices

Strengths:

• Clear TODO comment explaining the temporary nature of this change
• Minimal, focused change that addresses the immediate concern
• Uses a well-named constant (ninetyNineYears) for clarity
• Follows Go conventions for constant naming

Suggestions:

1. Add issue reference in TODO: Link the TODO comment to issue [EPIC]: Node Pruning #2093 (Node Pruning epic) for better tracking:

   // TODO(#2093): when we add pruning we can remove this

2. Consider adding a comment explaining the calculation:

   // TODO(#2093): when we add pruning we can remove this
   // Setting to 99 years effectively disables pruning until proper implementation
   const ninetyNineYears = 99 * 365 * 24 * time.Hour

---

🐛 Potential Bugs or Issues

No critical bugs identified, but considerations:

1. Leap years: The calculation doesn't account for leap years (minor - ~24.75 extra days over 99 years). For a temporary fix, this is acceptable, but if this becomes longer-term, consider using a more precise duration or simply a very large constant like math.MaxInt64.

2. No validation of external library behavior: Verify that goheadersync.WithPruningWindow() accepts such a large duration without overflow or unexpected behavior. Consider checking the go-header library's documentation or source.

---

⚡ Performance Considerations

Storage Growth:

• Impact: Without pruning, the goheader store will grow indefinitely, which directly contradicts the goals outlined in ADR-017
• Mitigation: This is acknowledged as temporary, but consider:
  • Adding metrics to monitor store size growth
  • Setting up alerts if the store grows beyond expected thresholds
  • Documenting expected storage requirements for operators

Memory/CPU:

• Disabling pruning may increase memory pressure if the go-header library loads indexes or metadata for all stored headers
• Consider monitoring memory usage in production environments

Recommendation: Add telemetry or logging to track store growth rate while pruning is disabled.

---

🔒 Security Concerns

No security issues identified. This change:

• Does not expose new attack surfaces
• Does not handle sensitive data differently
• Does not modify authentication or authorization logic

---

🧪 Test Coverage

Gaps Identified:

1. No test for the pruning window configuration: Consider adding a test to verify the syncer is created with the correct pruning window:

   func TestSyncerPruningWindowConfiguration(t *testing.T) {
       // Verify syncer is created with 99-year window
       // This ensures the fix is actually applied
   }

2. Existing test doesn't cover long-term storage: The TestHeaderSyncServiceRestart test only creates 2 blocks. Consider adding a test that:

   • Creates a larger number of blocks (e.g., 1000+)
   • Verifies all headers remain accessible (not pruned)
   • Validates store size growth characteristics

3. No integration test for store growth monitoring: As this disables pruning, tests should verify the store grows as expected without errors.

---

📋 Additional Recommendations

1. Documentation: Update any operator documentation to note that pruning is currently disabled and storage will grow unbounded until proper pruning is implemented.

2. Monitoring: Consider adding:

   // Log a warning periodically about disabled pruning
   logger.Warn().Msg("Header/data pruning is currently disabled - store will grow indefinitely")

3. Migration Path: When proper pruning is added per ADR-017, ensure there's a migration strategy for nodes that have accumulated large stores during this period.

4. Consistency: Verify this change is applied to both headerSync and dataSync - the PR shows it's in the shared newSyncer function, which is correct.

5. Timeline: Consider adding a comment or tracking issue about when proper pruning should be implemented, to prevent this temporary fix from becoming permanent.

---

🎯 Alignment with Project Standards

Per CLAUDE.md:

• ✅ Follows Go conventions and formatting
• ✅ Error handling patterns maintained (N/A for this change)
• ✅ Logging patterns not affected
• ✅ Minimal, focused change
• ✅ Uses appropriate constant for magic number

Per ADR-017:

• ⚠️ This temporarily reverts the direction of ADR-017, but is acknowledged as temporary
• 📝 Consider updating ADR-017 status to reflect this interim state

---

Verdict: Approve with Suggestions ✅

This is a reasonable temporary fix that addresses an immediate issue while proper pruning is being designed/implemented per ADR-017. The code quality is good, and the change is minimal and focused.

Key Action Items:

1. Add issue reference to TODO comment ([EPIC]: Node Pruning #2093)
2. Consider adding store growth monitoring/alerting
3. Add test coverage for pruning window configuration
4. Update operator documentation about unbounded storage growth
5. Document expected timeline for proper pruning implementation

The PR is safe to merge as-is, but the suggestions above would strengthen it for production use.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.27%. Comparing base (94e545a) to head (991afb2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2718   +/-   ##
=======================================
  Coverage   66.26%   66.27%           
=======================================
  Files          77       77           
  Lines        7868     7869    +1     
=======================================
+ Hits         5214     5215    +1     
  Misses       2169     2169           
  Partials      485      485           

Flag	Coverage Δ
combined	66.27% <100.00%> (+<0.01%)	⬆️

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.