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(share/availability)!: Store success results for light availability #3887

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

walldiss
Copy link
Member

@walldiss walldiss commented Oct 28, 2024

Store coordinates of successful sampling calls

@walldiss walldiss added the kind:feat Attached to feature PRs label Oct 28, 2024
@walldiss walldiss self-assigned this Oct 28, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 74.07407% with 14 lines in your changes missing coverage. Please review.

Project coverage is 45.27%. Comparing base (2469e7a) to head (0c2113b).
Report is 371 commits behind head on main.

Files with missing lines Patch % Lines
share/availability/light/availability.go 75.55% 8 Missing and 3 partials ⚠️
share/availability/light/sample.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3887      +/-   ##
==========================================
+ Coverage   44.83%   45.27%   +0.44%     
==========================================
  Files         265      309      +44     
  Lines       14620    21814    +7194     
==========================================
+ Hits         6555     9877    +3322     
- Misses       7313    10871    +3558     
- Partials      752     1066     +314     

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

@walldiss walldiss marked this pull request as draft October 28, 2024 12:49
@Wondertan
Copy link
Member

Why do we store successful sampling results? I know we wanted to do this, but I think we should wait to do it. My concern is mainly about not doing this together with the reconstruction and the new light node storage that would keep the bitmap-like state. If think you we need to do it earlier, then please explain.

@walldiss
Copy link
Member Author

Why do we store successful sampling results? I know we wanted to do this, but I think we should wait to do it. My concern is mainly about not doing this together with the reconstruction and the new light node storage that would keep the bitmap-like state. If think you we need to do it earlier, then please explain.

We need to store list of sampled coordinates for pruning. We would need to know coordinates to delete samples from blockstore by cid

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.

Appeoving to unblock. The error issue can be resolved async

share/availability/light/availability.go Show resolved Hide resolved
@walldiss walldiss enabled auto-merge (squash) November 1, 2024 12:28
@walldiss walldiss merged commit 46882c4 into celestiaorg:main Nov 1, 2024
30 checks passed
if err != nil {
return err
}
// Verify total samples count.
totalSamples := len(samples.Remaining) + len(samples.Available)
if totalSamples != int(la.params.SampleAmount) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: alternatively could have a validation func on SamplingResult

non blockin

@Wondertan Wondertan changed the title feat(share/availability): Store success results for light availability feat(share/availability)!: Store success results for light availability Nov 4, 2024
@Wondertan Wondertan deleted the store-samples-coords branch November 4, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants