-
Notifications
You must be signed in to change notification settings - Fork 243
refactor: migrate to blob api and remove da interface. #2893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Introduced a new `fetchBlobs` function to streamline the process of retrieving blobs from the DA client. - Replaced direct calls to `client.DA.GetIDs` and `client.DA.Get` with the new `fetchBlobs` function to handle errors more gracefully. - Added custom error types for better clarity when blobs are not found or when a height is in the future. - Updated blob processing logic to work with the new batch structure, improving efficiency and readability. - Modified tests to reflect changes in ID creation and splitting, ensuring compatibility with the new blob handling logic.
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @randygrok's task —— View job Comprehensive Code Review CompleteI've performed a thorough review of the blob API migration PR. This is a significant architectural refactoring that removes the DA interface layer and migrates to a more direct Blob API approach. Below is my detailed analysis: 🏗️ 1. Code Quality & Architecture✅ Strengths
|
|
Full nodes syncing from DA were not persisting the DAIncludedHeightKey metadata, causing the E2E test to fail when querying this value. The submitter (sequencer) already persists this key when verifying DA inclusion. This change adds the same logic to the syncer so full nodes also track which blocks have been DA included.
The LocalDA's GetAll method was missing the height-from-future check that existed in the old GetIDs method. This caused the syncer to iterate indefinitely instead of backing off when caught up with DA. Also simplified IsHeightDAIncluded by removing unused variable.
This reverts commit 3c0a15c.
|
|
||
| // API defines the jsonrpc service module API | ||
| // API exposes the blob RPC methods used by the node. | ||
| type API struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we able to use subscribe here?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2893 +/- ##
==========================================
- Coverage 64.53% 57.70% -6.83%
==========================================
Files 81 81
Lines 7370 7339 -31
==========================================
- Hits 4756 4235 -521
- Misses 2072 2583 +511
+ Partials 542 521 -21
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:
|
Fix broken markdown links by correcting the relative path depth from ../../../ to ../../ for linking to execution/grpc and sequencers/single READMEs.
alpe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
I added some comments to the changes in the Docker file and about the wait group. Otherwise LGTM 👍
|
|
||
| COPY go.mod go.sum ./ | ||
| COPY core core | ||
| COPY da da |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you copy core and da here? they are copied in line 10.
Go mods are normally copied as a separate step before the code to optimize build time as they usually do not change as often as the code. This improves build time significantly.
To copy core and da before running go mod works against this
| defer wg.Done() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good fix to exit early and release resources. This may was blocking before. 🚀
| bz, err := marshalFn(itm) | ||
| if err != nil { | ||
| resultCh <- fmt.Errorf("failed to marshal %s item at index %d: %w", itemType, idx, err) | ||
| cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an optimization which should not be needed without the wg.Wait.
| } | ||
|
|
||
| // Wait for all goroutines to complete and check for errors | ||
| defer wg.Wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add the wait group? The code below was waiting for all Go routines to land on the happy path and exited early on failures.
| if strings.Contains(err.Error(), context.Canceled.Error()) { | ||
| api.Logger.Debug().Str("method", "GetIDs").Msg("RPC call canceled due to context cancellation") | ||
| return res, context.Canceled | ||
| // BlobAPI exposes the methods needed by block/internal/da. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Godoc should start with Submit
| } | ||
| *cfg = SubmitOptions(j) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you implement Un/MarshalJSON ? This is a lot of code when the field names/types are equal.
| type BlobAPI interface { | ||
| Submit(ctx context.Context, blobs []*blob.Blob, opts *blob.SubmitOptions) (uint64, error) | ||
| GetAll(ctx context.Context, height uint64, namespaces []share.Namespace) ([]*blob.Blob, error) | ||
| GetProof(ctx context.Context, height uint64, namespace share.Namespace, commitment blob.Commitment) (*blob.Proof, error) | ||
| Included(ctx context.Context, height uint64, namespace share.Namespace, proof *blob.Proof, commitment blob.Commitment) (bool, error) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add subscribe here? ideally we can use subscribe to stay at the head of the chain instead of doing gets on heights. The integration into syncing can come later, but we should have the implementation here
| @@ -0,0 +1,154 @@ | |||
| package blob | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add docs on this is a copy of the code in node and why. If its a copy it might be better to copy outright so we can track changes in the future through some ci job
| defer cancel() | ||
| idsResult, err := c.da.GetIDs(getIDsCtx, height, namespace) | ||
|
|
||
| blobs, err := c.blobClient.GetAll(getIDsCtx, height, []share.Namespace{ns}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be rate limited? if we have 10k blobs could it crash something?
| @@ -0,0 +1,80 @@ | |||
| package da | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can this file be marked as _tests? or a comment at the top saying its for tests
| server.SetDAVisualizationServer( | ||
| server.NewDAVisualizationServer( | ||
| func(ctx context.Context, id []byte, ns []byte) ([]datypes.Blob, error) { | ||
| // minimal fetch: derive height from ID and use namespace provided | ||
| height, _ := blob.SplitID(id) | ||
| res := client.Retrieve(ctx, height, ns) | ||
| if res.Code != datypes.StatusSuccess || len(res.Data) == 0 { | ||
| return nil, fmt.Errorf("blob not found") | ||
| } | ||
| return res.Data, nil | ||
| }, | ||
| visualizerLogger, | ||
| config.Node.Aggregator, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we modify the server to not need the function wrapper?
| @@ -1,32 +1,33 @@ | |||
| //go:build !ignore | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
| "time" | ||
|
|
||
| testutils "github.com/celestiaorg/utils/test" | ||
| "github.com/evstack/ev-node/block" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we revert this
| <div class="blob-ids"> | ||
| {{$namespace := .Namespace}} | ||
| {{range .BlobIDs}} | ||
| <a href="/da/blob?id={{.}}&namespace={{$namespace}}" class="blob-link blob-id" title="Click to view blob details">{{slice . 0 8}}...</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a mistaken deletion, can you revert
| ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) | ||
| defer cancel() | ||
|
|
||
| var namespace []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be reverted as well.
| @@ -1,3 +1,5 @@ | |||
| //go:build ignore | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| @@ -1,3 +1,5 @@ | |||
| //go:build ignore | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| type blobProofVerifier interface { | ||
| GetProof(ctx context.Context, height uint64, namespace share.Namespace, commitment blob.Commitment) (*blob.Proof, error) | ||
| Included(ctx context.Context, height uint64, namespace share.Namespace, proof *blob.Proof, commitment blob.Commitment) (bool, error) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this pr has a lot of interfaces spread through the code, why this approach instead of defining one interface in core?
| @@ -1,4 +1,5 @@ | |||
| //go:build evm | |||
| // +build evm | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still some cleanup needed from merge conflict issues.
can we split this pr into some smaller ones? like adding blob pkg in one pr moving interfaces around in another then the impl in a third, it will make the review and merge faster
This will be CLOSED and proposed as small PRs.
Please check
Part 1: #2905
Overview