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

share/befp: rework collecting and verifying the BEFP #2408

Merged
merged 10 commits into from
Jul 12, 2023

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Jun 28, 2023

Overview

  • Use an orthogonal axis to collect and verify BEFP instead of collecting and verifying it against a single Merkle Root(where ErrByzantine occurred)
  • collect only 1/2 of the shares instead of all available shares from the row/col

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs added area:shares Shares and samples kind:misc Attached to miscellaneous PRs labels Jun 28, 2023
@vgonkivs vgonkivs self-assigned this Jun 28, 2023
@renaynay renaynay requested a review from adlerjohn June 29, 2023 13:25
@vgonkivs vgonkivs marked this pull request as ready for review July 4, 2023 09:51
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2023

Codecov Report

Merging #2408 (d8ce446) into main (387c160) will decrease coverage by 0.20%.
The diff coverage is 61.42%.

@@            Coverage Diff             @@
##             main    #2408      +/-   ##
==========================================
- Coverage   53.25%   53.05%   -0.20%     
==========================================
  Files         156      156              
  Lines        9915     9946      +31     
==========================================
- Hits         5280     5277       -3     
- Misses       4180     4211      +31     
- Partials      455      458       +3     
Impacted Files Coverage Δ
share/eds/byzantine/bad_encoding.go 35.65% <30.76%> (-1.05%) ⬇️
share/eds/byzantine/share_proof.go 50.00% <60.00%> (ø)
share/eds/byzantine/byzantine.go 73.33% <89.65%> (+11.79%) ⬆️

... and 5 files with indirect coverage changes

@vgonkivs vgonkivs force-pushed the rework_befp_validation branch 2 times, most recently from 3a66578 to 6e5b6bb Compare July 4, 2023 11:12
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/byzantine.go Outdated Show resolved Hide resolved
@adlerjohn adlerjohn changed the title share/befp: rework collecting and veryfying the BEFP share/befp: rework collecting and verifying the BEFP Jul 4, 2023
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/byzantine.go Show resolved Hide resolved
share/eds/byzantine/byzantine.go Show resolved Hide resolved
share/eds/byzantine/share_proof.go Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/byzantine.go Outdated Show resolved Hide resolved
share/eds/byzantine/byzantine.go Show resolved Hide resolved
share/eds/byzantine/byzantine.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Jul 10, 2023
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.

LGTM

renaynay
renaynay previously approved these changes Jul 11, 2023
@vgonkivs vgonkivs dismissed stale reviews from renaynay and Wondertan via 945870e July 11, 2023 13:18
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

looks ok.

@vgonkivs vgonkivs enabled auto-merge July 11, 2023 14:57
@vgonkivs vgonkivs added this pull request to the merge queue Jul 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 11, 2023
@mergify mergify bot mentioned this pull request Jul 12, 2023
5 tasks
@Wondertan Wondertan enabled auto-merge (squash) July 12, 2023 15:49
@Wondertan Wondertan merged commit c8fa853 into celestiaorg:main Jul 12, 2023
@renaynay renaynay mentioned this pull request Jul 14, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fraud area:shares Shares and samples kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants