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: add support for Blobstream API #3470

Merged
merged 59 commits into from
Jul 23, 2024
Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Jun 5, 2024

Closes #3361

@rach-id rach-id requested a review from evan-forbes June 5, 2024 11:17
@github-actions github-actions bot added the external Issues created by non node team members label Jun 5, 2024
@rach-id rach-id marked this pull request as draft June 5, 2024 11:17
go.mod Outdated Show resolved Hide resolved
nodebuilder/blobstream/service.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/service.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/service.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/service.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/service_test.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/types.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/service.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/service.go Outdated Show resolved Hide resolved
@rach-id rach-id self-assigned this Jun 5, 2024
@rach-id rach-id marked this pull request as ready for review June 5, 2024 11:27
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 40.98712% with 275 lines in your changes missing coverage. Please review.

Project coverage is 45.55%. Comparing base (2469e7a) to head (1183e46).
Report is 144 commits behind head on main.

Files Patch % Lines
blob/service.go 51.53% 53 Missing and 10 partials ⚠️
nodebuilder/blobstream/data_root_tuple_root.go 37.07% 52 Missing and 4 partials ⚠️
nodebuilder/blobstream/service.go 0.00% 41 Missing ⚠️
blob/commitment_proof.go 32.75% 32 Missing and 7 partials ⚠️
nodebuilder/blobstream/mocks/api.go 16.66% 20 Missing ⚠️
nodebuilder/share/share.go 0.00% 16 Missing ⚠️
nodebuilder/share/mocks/api.go 0.00% 10 Missing ⚠️
nodebuilder/blob/mocks/api.go 0.00% 9 Missing ⚠️
nodebuilder/blobstream/module.go 0.00% 6 Missing ⚠️
nodebuilder/blobstream/blobstream.go 0.00% 4 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3470      +/-   ##
==========================================
+ Coverage   44.83%   45.55%   +0.71%     
==========================================
  Files         265      280      +15     
  Lines       14620    15848    +1228     
==========================================
+ Hits         6555     7219     +664     
- Misses       7313     7798     +485     
- Partials      752      831      +79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rach-id
Copy link
Member Author

rach-id commented Jul 19, 2024

@renaynay @Wondertan blobstream module re-introduced and endpoints removed from header module. Please review when you have time :D

nodebuilder/header/data_root_tuple_root.go Outdated Show resolved Hide resolved
nodebuilder/header/data_commitment.go Outdated Show resolved Hide resolved
nodebuilder/blobstream/data_root_tuple_root.go Outdated Show resolved Hide resolved
renaynay
renaynay previously approved these changes Jul 22, 2024
nodebuilder/blobstream/data_root_tuple_root.go Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd previously approved these changes Jul 22, 2024
@rach-id rach-id dismissed stale reviews from distractedm1nd and renaynay via 1183e46 July 22, 2024 12:31
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

This comment is probably a little late and I acknowledge that but I don't think this is generic and widespread enough to warrant being a part of the node API. This will probably only be run by a single machine. We don't need to bloat the repo with this added module. I feel like it can be implemented completely outside (in the blobstreamX relayer code) using the header module and constructing the merkle tree from the results.

We can still keep the blob inclusion proofs. Although I think we should not use the word Commitment publicly because it's not clear what it means. I would advocate GetInclusionProof

@rach-id
Copy link
Member Author

rach-id commented Jul 22, 2024

This will probably only be run by a single machine

This will be run by zk-Blobstream provers, we now have:

  • BlobstreamX with some operators
  • Blobstream0: close to production

And also by users/validators in case of a fraud proof. For the commitment proof, it can also be used by sovereign rollups in their fraud proofs (still to be defined/discussed). So, there are a few parties that will use these endpoints.

We don't need to bloat the repo with this added module. I feel like it can be implemented completely outside (in the blobstreamX relayer code) using the header module and constructing the merkle tree from the results

I don't think it's bloating the repo. The share proof and commitment proof belong to this repo. For the GetDataRootTupleRoot and GetDataRootTupleInclusionProof, they can be implemented somewhere else (I did the first implementation here: celestiaorg/blobstream-node-api#1), but Blobstream is part of the Celestia protocol in the short-mid term, so it's fine to have its API as part of the official implementation of node.

Although I think we should not use the word Commitment publicly because it's not clear what it means. I would advocate GetInclusionProof

For this, it will replace the blob.Proof in here in a subsequent PR. We just didn't want to add more work to this PR as it's been open for such a long time. Check here for corresponding issue: #3582

@renaynay renaynay enabled auto-merge (squash) July 23, 2024 11:06
@renaynay renaynay merged commit b933118 into celestiaorg:main Jul 23, 2024
23 of 25 checks passed
@cmwaters
Copy link
Contributor

For the commitment proof, it can also be used by sovereign rollups in their fraud proofs (still to be defined/discussed).

I think the commitment proof (and share proof) makes sense. It's a generic use case that you want to prove inclusion of the blob (and shares). My original push back was to adding the blobstream module. Can't that logic be run by the "relayer", "prover" which makes the calls to the node's API rather than being inside the node itself.

but Blobstream is part of the Celestia protocol in the short-mid term

I don't view Blobstream as core to the DA protocol. It's a service built "on top off" Celestia.

@rach-id
Copy link
Member Author

rach-id commented Jul 23, 2024

Can't that logic be run by the "relayer", "prover" which makes the calls to the node's API rather than being inside the node itself.

you're right, it can. I just checked our current implementations of Blobstream, BlobstreamX and Blobstream0, and they both rewrite that logic, so they're not using these endpoints. So the main usage of those two endpoints would be when generating an inclusion proof to verify the inclusion of a blob.

So we have two options:

  • Provide this API with the proof and users can just use it + it's self-contained a can be removed from node anytime in the future
  • Or, we delete it and write some specs for the endpoints. Then, whenever someone is building a rollup, we tell them to re-implement that using the specs.

I'll take this discussion to slack and we can get other's opinion.

Thanks for flagging this 👍

@Wondertan
Copy link
Member

Another future option is for when the node actually becomes pluggable. You will be able to make a module that lives outside the node but can be integrated into the node like its part of it. Basically, there will be enhanced nodes with some additional application-specific functionality depending on module configuration. So, we get seamless integration into the node without needing the node team to maintain all the nonstandard modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blobstream Related to the Blobstream feature kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blobstream API should be moved to node