feat: implement sync mode #2961
Conversation
|
Claude finished @tac0turtle's task in 2m 12s —— View job Comprehensive Code ReviewI've completed a thorough review of the sync mode implementation. This is a significant architectural improvement that introduces dynamic synchronization modes for the DA worker. Below are my findings across multiple dimensions: 1. Architecture & Design✅ StrengthsWell-designed state machine: The dual-mode approach (catchup vs follow) is architecturally sound and addresses a real performance need. The transition logic in Clean separation of concerns: The introduction of Appropriate use of interfaces: The
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
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 data availability (DA) worker's synchronization capabilities by introducing dynamic sync modes. It allows the system to efficiently transition between an aggressive polling strategy when catching up to the DA chain and a more resource-optimized, subscription-based approach once it reaches the chain head. This change aims to improve the overall performance and responsiveness of DA data processing. 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 introduces a dual-mode synchronization mechanism, allowing the node to switch between an aggressive polling 'catchup' mode and a more efficient subscription-based 'follow' mode. This is a significant enhancement for nodes that are at the head of the chain. The changes are well-structured, introducing new DA client methods, metrics, and the core state machine logic in the daWorkerLoop. My review identified two critical bugs related to incorrect loop variable capturing that could lead to data corruption, and a couple of medium-severity design and style issues. Once these points are addressed, the implementation will be much more robust.
…in daRetriever; update tests and syncer for subscription handling
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2961 +/- ##
==========================================
- Coverage 59.26% 58.47% -0.80%
==========================================
Files 108 109 +1
Lines 10103 10319 +216
==========================================
+ Hits 5988 6034 +46
- Misses 3484 3656 +172
+ Partials 631 629 -2
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:
|
tac0turtle
force-pushed
the
marko/sync_subscribe
branch
from
95aeea4 to
ecfcf83
Compare
|
CI is not so glad. |
fixed |
block/internal/syncing/syncer.go
Outdated
| } | ||
|
|
||
| // Subscribe to forced inclusion namespace if configured | ||
| var forcedInclusionCh <-chan *blobrpc.SubscriptionResponse |
There was a problem hiding this comment.
We don't need to follow the force included namespace. The retriever itself does the caching itself. Maybe we should align this logic in the force inclusion retriever as well instead of using the async block fetching (in da)
block/internal/syncing/syncer.go
Outdated
| s.logger.Error().Err(err).Uint64("height", resp.Height).Msg("failed to process data subscription") | ||
| } | ||
|
|
||
| case resp, ok := <-forcedInclusionCh: |
| daStartHeight uint64 | ||
| asyncFetcher AsyncBlockRetriever | ||
|
|
||
| mu sync.Mutex |
There was a problem hiding this comment.
why do we need to track all of this the tracking is done in the retrieve function
There was a problem hiding this comment.
working on some cleanup yea
| } | ||
|
|
||
| // LocalHead returns the height of the locally synced DA head. | ||
| func (c *client) LocalHead(ctx context.Context) (uint64, error) { |
There was a problem hiding this comment.
for me while reading, appears a little confusing localHead in this context.
Is it querying the local node? is the last header that the DA has of my chain? is the last header that the DA layer has synced?
maybe this thought changes once all the review is done
There was a problem hiding this comment.
ill amend to make more clear
| Subscribe(ctx context.Context, namespace []byte) (<-chan *blobrpc.SubscriptionResponse, error) | ||
|
|
||
| // LocalHead returns the height of the locally synced DA head. | ||
| // Used to determine if the node is caught up with the DA layer. |
There was a problem hiding this comment.
I see, not sure if LocalHead is clear enough.
| } | ||
|
|
||
| // HandleSubscriptionResponse caches forced inclusion blobs from subscription updates. | ||
| func (r *ForcedInclusionRetriever) HandleSubscriptionResponse(resp *blobrpc.SubscriptionResponse) { |
There was a problem hiding this comment.
I'd prefer if subscribing was internal to the async block retriever, and sequencer can make use of this too.
Overview
this pr add sync modes allowing us to move to subscriptions when at the head of the chain