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

modshare: GetRow method #3981

Open
Wondertan opened this issue Dec 5, 2024 · 5 comments · May be fixed by #4002
Open

modshare: GetRow method #3981

Wondertan opened this issue Dec 5, 2024 · 5 comments · May be fixed by #4002
Labels
area:shares Shares and samples

Comments

@Wondertan
Copy link
Member

Overview

We need the ability to get rows out of EDS for a height. Integration users(cc @Ferret-san) need access to the entire row with parity data. Currently, users resort to fetching full EDS via GetEDS to get a row, which doesn't scale and adds additional complexity.

API

To achieve this, we add the following method to ShareModule.

  GetRow(context.Context,  height uint64, rowIdx int) (shwap.Row, error)

Implementation

The necessary protocol changes have already been implemented, and it's already possible to request Row with Shwap. The remaining work is to plumb support up to the API.

We add and use the new shwap.Getter method:

  GetRow(context.Context, *header.ExtendedHeader, rowIdx int) (shwap.Row, error)

And add implementations to BitswapGetter and CascadeGetter

The BitswapGetter already uses Row fetching to get the EDS, which can be used as an example of how to fetch Rows/

Testing

The feature must be delivered with a Swamp test, ensuring integration test coverage and API breakage protection. The test case should verify that every Q1Q2 row can be fetched and matches the DAH. Besides, we should check that Q3Q4 rows are restricted from fetching.

@Wondertan Wondertan added the area:shares Shares and samples label Dec 5, 2024
@walldiss
Copy link
Member

walldiss commented Dec 5, 2024

Besides, we should check that Q3Q4 rows are restricted from fetching.

What is the motivation for API to be restricted?

@Wondertan
Copy link
Member Author

What is the motivation for API to be restricted?

We don't allow parity namespaces and tail padding for namespace requests. Similalrly here, we don't allow data that doesn't have meaningful data.

@vgonkivs
Copy link
Member

vgonkivs commented Dec 6, 2024

GetRow(context.Context, height uint64, rowIdx int) (shwap.Row, error)

The shwap.Row contains only half of the shares in the row. As a result, the high-level API will not function as expected, since users require the full row of shares. This issue can be fixed by caching the extended shares on the client side before returning the result to the user.

@Wondertan
Copy link
Member Author

Wondertan commented Dec 6, 2024

Two more things:

  • Row should define Marshal/UnmarshalJSON
  • Row should cache parity data.
    • Currently it recomputes it meaning that serialization will only return the left half instead of full row
    • This is also pretty important optimization, as currently fetching EDS over Bitswap does the compute twice

@Wondertan
Copy link
Member Author

Wondertan commented Dec 6, 2024

The caching was part of the shwap hardening issue. As there is now another reason to do it, I made an issue for it #3984

@renaynay renaynay removed their assignment Dec 10, 2024
@vgonkivs vgonkivs linked a pull request Dec 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:shares Shares and samples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants