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

fix(blob/service): fix handling of the padding shares #3404

Merged
merged 6 commits into from
May 20, 2024

Conversation

vgonkivs
Copy link
Member

Fixes the issue with the missing blob. Initially, there was an incorrect assumption that only one padding share is possible between two blobs. So, the Blob service was skipping only one share instead of multiple.

  • Fixed parsing logic allowing to skip multiple padding shares;
  • Added a test case that retrieves the EDS and finds the correct number of blobs;

@vgonkivs vgonkivs added kind:fix Attached to bug-fixing PRs area:blob labels May 15, 2024
@vgonkivs vgonkivs self-assigned this May 15, 2024
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.

The fix LGTM. Only have two blocking on the test file and need to make an issue.

blob/service_test.go Outdated Show resolved Hide resolved
blob/service_test.go Outdated Show resolved Hide resolved
blob/service_test.go Outdated 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.

Lets make test, that generates edses with multiple conditions of padding shares instead of using eds.json. 1 padding share, multiple padding shares between blobs, multiple padding shares between namespaces. It will help to add more cases in future too.

blob/parser.go Show resolved Hide resolved
@vgonkivs vgonkivs force-pushed the fix_missing_blob branch from e6ae531 to 8b3a48d Compare May 16, 2024 11:05
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 44.53%. Comparing base (2469e7a) to head (81b42de).
Report is 72 commits behind head on main.

Files Patch % Lines
blob/parser.go 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3404      +/-   ##
==========================================
- Coverage   44.83%   44.53%   -0.30%     
==========================================
  Files         265      272       +7     
  Lines       14620    15282     +662     
==========================================
+ Hits         6555     6806     +251     
- Misses       7313     7692     +379     
- Partials      752      784      +32     

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

@vgonkivs vgonkivs force-pushed the fix_missing_blob branch from 8b3a48d to 309c91b Compare May 16, 2024 11:51
@vgonkivs vgonkivs requested review from Wondertan and walldiss May 16, 2024 11:52
walldiss
walldiss previously approved these changes May 16, 2024
blob/parser.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

@renaynay, I propose to make a new patch for this fix, instead of 0.14.0 as its blocking Astria and in general a critical bug.

Wondertan
Wondertan previously approved these changes May 17, 2024
@vgonkivs vgonkivs dismissed stale reviews from Wondertan and walldiss via 81b42de May 17, 2024 09:29
@renaynay renaynay removed the v0.14.0 label May 17, 2024
@Wondertan Wondertan enabled auto-merge (squash) May 20, 2024 15:01
@Wondertan Wondertan merged commit e66c311 into celestiaorg:main May 20, 2024
25 of 29 checks passed
Wondertan pushed a commit that referenced this pull request May 20, 2024
Fixes the issue with the missing blob. Initially, there was an incorrect assumption that only one padding share is possible between two blobs. So, the Blob service was skipping only one share instead of multiple. 

* Fixed parsing logic allowing to skip multiple padding shares;
* Added a test case that retrieves the EDS and finds the correct number of blobs;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:blob kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants