feat(testda): add header support with GetHeaderByHeight method #2946
Conversation
Add header storage and retrieval to DummyDA to support timestamp determinism in tests. This enables tests to use the same header retrieval pattern as the real DA client. Changes: - Add Header struct with Height, Timestamp, and Time() method - Store headers with timestamps when blobs are submitted - Store headers when height ticker advances - Add GetHeaderByHeight method mirroring HeaderAPI.GetByHeight - Update Retrieve to use stored timestamps for consistency Closes #2944 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 2m 11s —— View job Code Review CompleteI've thoroughly reviewed this PR that adds header support to DummyDA for timestamp determinism. Overall, the implementation is solid and addresses the previous review concerns about non-determinism. Here's my detailed feedback: Positive Aspects ✅1. Determinism Achieved
2. Clean Architecture
3. Thread Safety
Areas for Improvement1. Missing Test Coverage 🔴 High PriorityThe // Key scenarios to test:
- GetHeaderByHeight returns header with correct timestamp after Submit
- GetHeaderByHeight returns header with correct timestamp after ticker advance
- GetHeaderByHeight returns ErrHeightFromFuture for future heights
- GetHeaderByHeight returns ErrBlobNotFound for past heights with no header
- Retrieve uses stored header timestamp (deterministic)
- Retrieve uses zero timestamp when header doesn't exist (deterministic)
- Concurrent access to Submit/GetHeaderByHeight (race detection)
- Reset clears headers mapThis is especially important since the primary goal is determinism - tests should verify timestamps are consistent across multiple calls. 2. Inconsistent Error Handling in GetHeaderByHeight 🟡 Medium PriorityLocation: The error handling logic has a subtle issue: if header == nil {
currentHeight := d.height.Load()
if height > currentHeight {
return nil, datypes.ErrHeightFromFuture
}
return nil, datypes.ErrBlobNotFound // Line 289
}Problem: Scenarios to consider:
Recommendation: Consider one of these approaches: Option A: Return a more specific error var ErrHeaderNotFound = errors.New("header: not found")
// In GetHeaderByHeight:
if height > currentHeight {
return nil, datypes.ErrHeightFromFuture
}
return nil, ErrHeaderNotFound // More accurate than ErrBlobNotFoundOption B: Document that missing headers for valid heights indicate an inconsistent state // GetHeaderByHeight retrieves the header for the given DA height.
// Returns ErrHeightFromFuture if height > current height.
// Returns ErrBlobNotFound if the height is valid but no header was stored.
// Note: All heights should have headers if DummyDA is used correctly
// (either from Submit or StartHeightTicker).I'd recommend Option A for semantic clarity. 3. Race Condition in GetHeaderByHeight 🟡 Medium PriorityLocation: func (d *DummyDA) GetHeaderByHeight(_ context.Context, height uint64) (*Header, error) {
d.mu.Lock()
header := d.headers[height]
d.mu.Unlock()
if header == nil {
currentHeight := d.height.Load() // ⚠️ Lock released before reading height
if height > currentHeight {
return nil, datypes.ErrHeightFromFuture
}
return nil, datypes.ErrBlobNotFound
}
return header, nil
}Problem: There's a window between releasing
Impact: Low in practice (test code, short time window), but violates the determinism goal. Fix: func (d *DummyDA) GetHeaderByHeight(_ context.Context, height uint64) (*Header, error) {
d.mu.Lock()
header := d.headers[height]
currentHeight := d.height.Load() // Read while holding lock
d.mu.Unlock()
if header == nil {
if height > currentHeight {
return nil, datypes.ErrHeightFromFuture
}
return nil, datypes.ErrBlobNotFound
}
return header, nil
}4. Documentation Could Be More Explicit 🟢 Low PriorityLocation: Current: // DummyDA is a test implementation of the DA client interface.
// It supports blob storage, height simulation, failure injection, and header retrieval.Suggestion: Emphasize the determinism guarantee: // DummyDA is a test implementation of the DA client interface.
// It provides deterministic timestamp behavior by storing headers with timestamps
// when blobs are submitted or heights advance, enabling tests to use the same
// header retrieval pattern as real DA clients.5. Potential Memory Leak in Long-Running Tests 🟢 Low PriorityThe // Option: Add a cleanup mechanism
func (d *DummyDA) PruneHeadersBefore(height uint64) {
d.mu.Lock()
defer d.mu.Unlock()
for h := range d.headers {
if h < height {
delete(d.headers, h)
}
}
}However, since this is test code and Security & PerformanceSecurity: ✅ No concerns. This is test-only code with no external inputs or sensitive data. Performance: ✅ Appropriate for test code:
Concurrency: ✅ Generally good, but see race condition note above. SummaryThis PR successfully achieves its primary goal of adding timestamp determinism to DummyDA. The previous non-determinism issues flagged by Gemini have been properly resolved. The implementation is clean and mirrors the real DA client API well. Required before merge:
Recommended improvements:
Nice to have:
The code quality is good and follows Go idioms. With tests added and the race condition fixed, this will be a solid addition to the test infrastructure. |
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! 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 significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to add header storage and retrieval to DummyDA to improve timestamp determinism in tests. The changes are well-structured, introducing a Header type and integrating it into blob submission, height ticking, and data retrieval. However, I've identified a couple of areas in the new and modified functions where time.Now() is used as a fallback. This undermines the goal of determinism. My review provides specific suggestions to address these points and ensure the test utility is fully deterministic, aligning with the pull request's objective.
test/testda/dummy.go
Outdated
| if height <= currentHeight && height > 0 { | ||
| return &Header{ | ||
| Height: height, | ||
| Timestamp: time.Now(), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The goal of this PR is to introduce timestamp determinism in tests, but this logic does the opposite when a header is not found for a valid height. By creating a new header with time.Now(), any test that relies on this function will become non-deterministic, as each call can return a different timestamp for the same height.
Furthermore, the function's documentation comment says "Returns nil if no header exists for the given height", but this implementation contradicts it by generating a header on the fly.
To ensure determinism and align with the documentation, you should not generate a header here. If a header for an empty block is expected, it should have been created by the StartHeightTicker mechanism. If it's not present, it means the block doesn't exist in the test's context, so returning nil is the correct and deterministic behavior.
if height <= currentHeight && height > 0 {
// To ensure determinism, return nil. A header for an empty block
// should be created by the height ticker if needed.
return nil, nil
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2946 +/- ##
==========================================
- Coverage 59.18% 59.15% -0.04%
==========================================
Files 90 90
Lines 8627 8632 +5
==========================================
Hits 5106 5106
- Misses 2940 2944 +4
- Partials 581 582 +1
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:
|
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
Good start. IMHO the GetHeaderByHeight method is a bit inconsistent but I don't want to block you on this.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
alpe
left a comment
alpe
left a comment
There was a problem hiding this comment.
Perfect, thanks for the update
Add header storage and retrieval to DummyDA to support timestamp determinism in tests. This enables tests to use the same header retrieval pattern as the real DA client.
Changes:
Closes #2944
🤖 Generated with Claude Code