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

test(share): add Full Node reconstruction tests #702

Merged
merged 12 commits into from
Jun 24, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented May 11, 2022

Mainly:

  • Improves testnet utility in share pkg
  • Introduces tests that assert that our reconstruction works in semi-ideal scenarios with values taken from the original paper
    • 1x One Full and multiple Lights
    • 2x Two Fulls and two subnets with multiple Lights that cannot reconstruct data on their own

For an in-depth description of each test consult a complementary comment to each test.

Substitutes #598
Based on #682
Closes #600

My favourite part about these tests is that they won't require any changes if we e.g., move to a completely new protocol for Share transportation

@adlerjohn
Copy link
Member

Did you link to the right issue? #598 is a closed PR.

@adlerjohn
Copy link
Member

Is this going to be merged into #682, or does it require #682 to be merged into main first? If the latter, please convert this into a draft to avoid merging too soon.

@Wondertan Wondertan marked this pull request as draft May 11, 2022 14:31
@Wondertan
Copy link
Member Author

Draft until #682 is merged

@adlerjohn adlerjohn linked an issue May 11, 2022 that may be closed by this pull request
@Wondertan Wondertan force-pushed the hlib/concurrent-get-leaves branch 4 times, most recently from 88119a0 to 2d9bd5c Compare May 13, 2022 11:49
Base automatically changed from hlib/concurrent-get-leaves to main May 13, 2022 14:32
@Wondertan Wondertan force-pushed the hlib/reconstruction-tests branch 2 times, most recently from 79635d2 to d5c0dd4 Compare May 20, 2022 12:23
@Wondertan Wondertan force-pushed the hlib/reconstruction-tests branch from d5c0dd4 to a07a81f Compare June 3, 2022 14:38
@Wondertan Wondertan force-pushed the hlib/reconstruction-tests branch from a07a81f to 20c0936 Compare June 9, 2022 18:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Attention: Patch coverage is 75.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 54.19%. Comparing base (0f8b81c) to head (16143c5).
Report is 1488 commits behind head on main.

Files with missing lines Patch % Lines
service/share/testing.go 75.00% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   53.93%   54.19%   +0.25%     
==========================================
  Files         121      121              
  Lines        6968     7016      +48     
==========================================
+ Hits         3758     3802      +44     
- Misses       2823     2830       +7     
+ Partials      387      384       -3     

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

@Wondertan
Copy link
Member Author

So the main blocker here is to make tests run while being limited with 8k goroutines, which is not trivial.

@Wondertan Wondertan marked this pull request as ready for review June 9, 2022 19:36
@adlerjohn adlerjohn changed the title test(share): add Full Node reconstrustrution tests test(share): add Full Node reconstruction tests Jun 10, 2022
service/share/full_availability_test.go Outdated Show resolved Hide resolved
service/share/testing.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member Author

No plan yet. I would like to have them merged so hard, so I will probably disable the race flag for them somehow

@Wondertan
Copy link
Member Author

Also, it would be good to extend these tests with discovery

@Wondertan Wondertan force-pushed the hlib/reconstruction-tests branch 2 times, most recently from d3a150d to 5182a17 Compare June 14, 2022 17:30
@Wondertan
Copy link
Member Author

I extracted the tests and disabled the race detector for them. Fully ready for review

@Wondertan
Copy link
Member Author

@musalbas, added you here as a reviewer. Do we need more harness tests or is this enough? Do we need tests where there are more than two Fulls with their own subnetworks?

@musalbas
Copy link
Member

In general we need to test multiple full nodes, but I'm not sure if that's something we need as a harness test, but rather a network test using e.g. testground.

@Wondertan
Copy link
Member Author

Wondertan commented Jun 24, 2022

Ok, then I think there is nothing else to do in this PR. Testground reconstruction test is planned but orthogonal to this PR.

@Wondertan Wondertan force-pushed the hlib/reconstruction-tests branch from 88e5015 to 982b22d Compare June 24, 2022 15:34
service/share/full_reconstruction_test.go Outdated Show resolved Hide resolved
Co-authored-by: John Adler <adlerjohn@users.noreply.github.com>
@Wondertan Wondertan merged commit d051a94 into main Jun 24, 2022
@Wondertan Wondertan deleted the hlib/reconstruction-tests branch June 24, 2022 18:25
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:testing Related to unit tests
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

share: test complex recovery scenarious
6 participants