Skip to content
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

feat(blob): blobsub #3539

Merged
merged 18 commits into from
Jul 30, 2024
Merged

feat(blob): blobsub #3539

merged 18 commits into from
Jul 30, 2024

Conversation

distractedm1nd
Copy link
Member

@distractedm1nd distractedm1nd commented Jun 28, 2024

Introduces blob.Subscribe. Takes a share.Namespace, returns a <-chan *BlobsubResponse containing a height uint64 and blobs []*blob.Blob.

I made the decision to not support subscribing to multiple namespaces with one call, because the complexity of the return type mitigates the benefits for the client. It is likely more convenient to have one subscription per namespace:

Imagine you are watching two namespaces for different things. On the client side, you would have a select statement reading from the two channels in two cases. This is cleaner than receiving everything in one case, but then having to manually parse and send to the respective handlers yourself.

This closes #2737 , with the caveat that we are not taking the map approach (see rationale above)

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely

blob/service.go Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
@ramin ramin self-requested a review July 2, 2024 09:58
ramin
ramin previously approved these changes Jul 2, 2024
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Show resolved Hide resolved
@Wondertan
Copy link
Member

Do we want to block this PR on absence proofs? The rough consensus from our call is that we should. Thoughts?

blob/service.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Jul 24, 2024
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint.

blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with hlib's comments re buffer + re shutdown

nodebuilder/blob/module.go Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Jul 30, 2024
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
blob/service.go Show resolved Hide resolved
blob/service.go Outdated Show resolved Hide resolved
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
blob/service.go Outdated Show resolved Hide resolved
@distractedm1nd distractedm1nd merged commit 4218db0 into main Jul 30, 2024
29 of 30 checks passed
@distractedm1nd distractedm1nd deleted the blobsub branch July 30, 2024 18:19
sebasti810 pushed a commit to sebasti810/celestia-node that referenced this pull request Aug 14, 2024
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blob kind:feat Attached to feature PRs v0.15.0 Intended for v0.15.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: allow subscribing to new blocks
7 participants