Skip to content

feat!(share/shwap): implement get range request over shwap #4156

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Mar 7, 2025

This PR enables GetRangeRequests over the shwap by introducing the GetRangeNamespaceData container. It allows users to retrieve namespace data using ODS coordinates [from; to]. The user must specify the type of response they want:
Data + Proofs;
ProofsOnly;
The returned proof can be verified against the data root.

@vgonkivs vgonkivs added area:shares Shares and samples kind:feat Attached to feature PRs kind:break! Attached to breaking PRs labels Mar 7, 2025
@vgonkivs vgonkivs self-assigned this Mar 7, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 43.16853% with 1069 lines in your changes missing coverage. Please review.

Project coverage is 44.72%. Comparing base (2469e7a) to head (4093946).
Report is 451 commits behind head on main.

Files with missing lines Patch % Lines
share/shwap/pb/shwap.pb.go 37.88% 617 Missing and 106 partials ⚠️
share/shwap/range_namespace_data.go 51.70% 51 Missing and 20 partials ⚠️
share/shwap/proof.go 60.52% 53 Missing and 7 partials ⚠️
share/shwap/range_namespace_data_id.go 62.72% 29 Missing and 12 partials ⚠️
...re/shwap/p2p/bitswap/range_namespace_data_block.go 51.25% 26 Missing and 13 partials ⚠️
share/shwap/p2p/bitswap/getter.go 0.00% 28 Missing ⚠️
share/eds/validation.go 0.00% 20 Missing ⚠️
store/getter.go 0.00% 15 Missing ⚠️
share/shwap/getters/cascade.go 0.00% 14 Missing ⚠️
share/eds/proofs_cache.go 80.00% 6 Missing and 3 partials ⚠️
... and 9 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4156      +/-   ##
==========================================
- Coverage   44.83%   44.72%   -0.12%     
==========================================
  Files         265      313      +48     
  Lines       14620    24404    +9784     
==========================================
+ Hits         6555    10914    +4359     
- Misses       7313    12252    +4939     
- Partials      752     1238     +486     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@walldiss
Copy link
Member

Discussed with the team. Below are clarifications addressing previously raised concerns and the subsequent next steps for this PR:

  1. We will retain the current Bitswap implementation. There is no need to implement GetRange over ShrEx in this PR, as support for ShrEx will be introduced separately with generalized ShrEx. No changes required here.

  2. Implement the following TODO:
    https://github.com/vgonkivs/celestia-node/blob/4093946715f8d1cd4588a20e651d66c0ca12476a/share/shwap/range_namespace_data.go#L28

TODO: Proof collection can be simplified to store only incomplete rows (the first and the last), as proofs for all complete rows can be recomputed.
  1. Remove the row root hash from the DataRoot proof structure. Currently, verification occurs in two steps:
    • Proof to row root.
    • Proof of row root inclusion in the DataRoot.

It is possible to verify inclusion directly against the DataRoot, bypassing the first step entirely. This adjustment will eliminate the need to store the row root hash within the proof structure. Achieving this will require minimal modifications in the NMT repository to support constructing the row root from Proof + Data. Once NMT support is available, this repository will also require updates to adopt the new verification algorithm.

@walldiss
Copy link
Member

NMT repo feature required for this PR:
celestiaorg/nmt#290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples kind:break! Attached to breaking PRs kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants