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

refactor(share): GetShare -> GetSamples #3891

Merged
merged 28 commits into from
Nov 27, 2024
Merged

refactor(share): GetShare -> GetSamples #3891

merged 28 commits into from
Nov 27, 2024

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Oct 28, 2024

Samples were requested individually rather than grouped. This was causing issues for Bitswap in the mode where each request is a distinct session, so the solution is to start grouping those as we originally planned.

Mainly removes GetShare and introduces:

GetSamples(ctx context.Context, header *header.ExtendedHeader, indices []SampleIndex) ([]Sample, error)

And introduces SampleIndex with helper funcs.

The current state of the PR unblocks me and allows me to iterate further on Bitswap issues, while the remaining TODOs still need to be finished.

TODOs:

  • eds.Accessor should take the new shwap.SampleIndex as param in Sample method
  • !Add GetSamples to the ShareModule.
    • Need to decide which release we want to put this breaking change in
  • Godoc new things
  • Fix remaining tests
  • Rebase
  • Decide on whether SampleIndex should be an index or a tuple.

🎱 mb blonks certified

@cristaloleg cristaloleg marked this pull request as ready for review November 19, 2024 15:02
share/availability/light/availability.go Outdated Show resolved Hide resolved
share/availability/light/availability.go Outdated Show resolved Hide resolved
nodebuilder/share/share.go Outdated Show resolved Hide resolved
@walldiss
Copy link
Member

Yes, it is a 1D index. I think it's worth having it for convenience, as there are still cases where 1D is used. It is not necessary internally but externally. Users, like Diego and Rollkit operated with index. E.g Rollkit fraud proofs point to 1D index, rather than a coord.

Well, we don't have any 1D indexes in our code base internal API, not in external json-rpc API we have. Introduction of 1D index brings lots of conversion from 1D to 2D and back here and there as we can see in this PR. Which suggests that we don't need it anywhere. We don't really need to introduce the second way to express same thing.

Diego and Rollkit operated with index

Do you know why they use it instead of 2D? Might be aswell be historical or random decision. Perhaps we should not introduce additional 1D index unless there is reason to have it

@Wondertan
Copy link
Member Author

We should keep 1D convenience methods, while we default coordinates tuple.

nodebuilder/share/share.go Show resolved Hide resolved
share/shwap/getter.go Outdated Show resolved Hide resolved
Copy link
Member Author

@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.

Approving

Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

There is a problem in cascade getter. If any of requested samples fail, it will retry with full sampleCoordinates set with next fallback getter, throwing away all found samples. There is no proper support added in this PR right now. It would technically work with current setup because bitswap getter is the last, but it will misbehaive in any other case.
If left as is it may become a huge footgun and nightmare to debug later.

share/availability/light/availability.go Show resolved Hide resolved
share/availability/light/availability.go Outdated Show resolved Hide resolved
share/availability/light/availability.go Outdated Show resolved Hide resolved
share/availability/light/availability_test.go Show resolved Hide resolved
share/availability/light/availability_test.go Show resolved Hide resolved
share/availability/light/availability_test.go Show resolved Hide resolved
share/availability/light/availability_test.go Show resolved Hide resolved
share/availability/light/availability_test.go Outdated Show resolved Hide resolved
share/shwap/p2p/bitswap/getter.go Outdated Show resolved Hide resolved
share/shwap/getters/cascade.go Show resolved Hide resolved
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

There is a problem in cascade getter. If any of requested samples fail, it will retry with full sampleCoordinates set with next fallback getter, throwing away all found samples. There is no proper support added in this PR right now. It would technically work with current setup because bitswap getter is the last, but it will misbehaive in any other case.
If left as is it may become a huge footgun and nightmare to debug later.

@cristaloleg cristaloleg enabled auto-merge (squash) November 27, 2024 13:18
@cristaloleg cristaloleg merged commit 5d9192f into main Nov 27, 2024
22 checks passed
@cristaloleg cristaloleg deleted the get-samples branch November 27, 2024 13:24
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:refactor Attached to refactoring PRs shwap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants