feat(sequencers/based): add based batch time #2911
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task —— View job Code Review CompleteI've reviewed the PR changes that add based block time based on epoch DA time. Here's my comprehensive feedback: SummaryThis PR implements timestamp tracking for based sequencer batches by propagating DA block timestamps through the forced inclusion retrieval system. The changes are focused and well-tested. ✅ Positive Observations
🔍 Issues & Recommendations1. Zero-Value Timestamp Semantics (Minor)Location: When no new DA epoch is fetched,
Recommendation: Consider using // t will be time.Time{} if we're serving from cached transactions
// and the DA timestamp was already retrieved in a previous call
t := time.Time{}2. Test Timestamp Assertion Inconsistency (Minor)Location: assert.Equal(t, event.Timestamp, time.Time{})This test asserts that the timestamp remains zero when Recommendation: Either explicitly set a timestamp in the test data or add a comment explaining why zero is expected here. 3. Missing Error Handling Test (Medium)Location: The
However, there's no test in Recommendation: Add integration tests for 4. Timestamp Comparison Logic (Minor Security/Correctness)Location: if result.Timestamp.After(event.Timestamp) {
event.Timestamp = result.Timestamp
}This logic takes the latest timestamp across all blobs in an epoch. However:
Recommendation: Add validation to ensure 5. UTC Conversion Timing (Minor)Location: return forcedTxsEvent.Timestamp.UTC(), forcedTxsEvent.EndDaHeight, nilThe UTC conversion happens at return time, but the timestamp comparison in Recommendation: Normalize to UTC earlier in the pipeline, ideally when receiving the timestamp from the DA layer. 6. Missing Documentation (Minor)Location: The
Recommendation: Add field documentation: type ForcedInclusionEvent struct {
// Timestamp is the latest DA block timestamp from the epoch range.
// Will be time.Time{} if no blobs were found in the epoch.
Timestamp time.Time
// ...
}🔒 Security & Resilience✅ No security issues identified
✅ Resilience looks good
⚡ Performance✅ No performance concerns
📝 TestingStrong: 223 new lines of test coverage including:
Gap: Missing sequencer-level tests for 🎯 Final AssessmentThis is a well-implemented PR that solves the timestamp tracking requirement cleanly. The issues raised are minor and mostly about defensive programming and documentation. The code is production-ready but would benefit from the improvements suggested above. Recommendation: ✅ Approve with minor suggestions - The PR can be merged as-is, but consider addressing the documentation and test coverage gaps in a follow-up. |
Summary of ChangesHello @julienrbrt, 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 enhances the Highlights
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
The pull request introduces changes to incorporate the Data Availability (DA) block timestamp into the batching process for the BasedSequencer. This addresses a previous TODO to use the DA block timestamp for determinism. The ForcedInclusionEvent struct now includes a Timestamp field, which is populated during the retrieval of forced inclusion blobs. The BasedSequencer's fetchNextDAEpoch method has been updated to return this timestamp, which is then used in the GetNextBatchResponse. New test cases have been added to client_test.go to cover batch retrieval scenarios, and existing tests in forced_inclusion_retriever_test.go have been updated to assert on the new timestamp field. The changes are well-implemented and improve the determinism of the sequencer.
| assert.Equal(t, coreda.StatusError, result.Code) | ||
| assert.Assert(t, result.Message != "") | ||
| // Error message should mention the batch range | ||
| assert.Assert(t, len(result.Message) > 0) |
There was a problem hiding this comment.
This assertion is a bit weak. It only checks that the error message is not empty. It would be more robust to assert on the specific content of the error message, or at least a substring, to ensure the expected error is being returned. For example, checking if it contains "network error in batch 2" or "failed to get blobs".
| assert.Assert(t, len(result.Message) > 0) | |
| assert.ErrorContains(t, errors.New(result.Message), "network error in batch 2") |
| return &coresequencer.GetNextBatchResponse{ | ||
| Batch: batch, | ||
| Timestamp: time.Time{}, // TODO(@julienrbrt): we need to use DA block timestamp for determinism | ||
| Timestamp: t, |
There was a problem hiding this comment.
We currently want strictly increasing time and not the same time. So maybe we can have a jitter based on the tx index (so if it doesn't fit all in the same block the time is the same for everyone)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## julien/persist-base #2911 +/- ##
=======================================================
+ Coverage 65.75% 65.82% +0.06%
=======================================================
Files 87 87
Lines 7995 7999 +4
=======================================================
+ Hits 5257 5265 +8
+ Misses 2164 2160 -4
Partials 574 574
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:
|
Overview
ref: #2906
Add based block time based on epoch da time.
DO NOT MERGE YET, based on #2908 to avoid conflicts later.