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/byzantine): handle additional cases in befp validation #2383

Merged
merged 6 commits into from
Jul 17, 2023

Conversation

vgonkivs
Copy link
Member

Overview

Extend a BEFP validation.
Related to #2377

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

@codecov-commenter
Copy link

Codecov Report

Merging #2383 (b7a7ca0) into main (403eb61) will increase coverage by 0.20%.
The diff coverage is 54.43%.

@@            Coverage Diff             @@
##             main    #2383      +/-   ##
==========================================
+ Coverage   50.44%   50.64%   +0.20%     
==========================================
  Files         155      156       +1     
  Lines        9826     9876      +50     
==========================================
+ Hits         4957     5002      +45     
+ Misses       4433     4432       -1     
- Partials      436      442       +6     
Impacted Files Coverage Δ
nodebuilder/node.go 58.57% <ø> (ø)
share/eds/byzantine/bad_encoding.go 34.40% <35.00%> (+1.37%) ⬆️
libs/pidstore/pidstore.go 60.00% <60.00%> (ø)
cmd/celestia/rpc.go 16.16% <62.06%> (+6.16%) ⬆️

... and 4 files with indirect coverage changes

share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs requested a review from Wondertan June 26, 2023 08:29
@renaynay renaynay changed the title chore(share/byzantine): handle additional cases in befp validation feat(share/byzantine): handle additional cases in befp validation Jun 26, 2023
@renaynay renaynay added kind:feat Attached to feature PRs and removed kind:chore labels Jun 26, 2023
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
share/eds/byzantine/bad_encoding.go Outdated Show resolved Hide resolved
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.

Actually, I just realized that this is only the verification part. We have to triple-check that the generation works for these cases.

@vgonkivs
Copy link
Member Author

These cases are possible with out-of-order shares. But we can't generate them for now

@vgonkivs
Copy link
Member Author

Moving this PR to draft until we can test these conditions

@vgonkivs vgonkivs marked this pull request as draft June 26, 2023 09:39
@vgonkivs vgonkivs marked this pull request as ready for review July 17, 2023 08:56
@vgonkivs vgonkivs mentioned this pull request Jul 17, 2023
3 tasks
Wondertan
Wondertan previously approved these changes Jul 17, 2023
renaynay
renaynay previously approved these changes Jul 17, 2023
Wondertan
Wondertan previously approved these changes Jul 17, 2023
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.

only one non-non-blocking comment

share/eds/byzantine/bad_encoding.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 Show resolved Hide resolved
@vgonkivs vgonkivs requested a review from adlerjohn July 17, 2023 12:59
@vgonkivs vgonkivs dismissed stale reviews from Wondertan and renaynay via 0aafbec July 17, 2023 13:14
@vgonkivs vgonkivs merged commit 6a6c68c into celestiaorg:main Jul 17, 2023
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:feat Attached to feature PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants