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

shares|ipld: Rework data retrieval #623

Merged
merged 14 commits into from
Apr 27, 2022
Merged

shares|ipld: Rework data retrieval #623

merged 14 commits into from
Apr 27, 2022

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Apr 14, 2022

What and Why

This PR comes from the requirement to make tests in #598 pass deterministically by reimplementing the original goal of #517. At first, I implemented the goal by simply updating the current code without reworking it. Unfortunately, that didn't work well, and there was a bug that made me stuck for a few days. After extensive multiday debugging, I cleaned up and reworked the existing implementation of RetrieveData in studying the code for the bug. The rework landed into a new ipld.Retriever component, which now not just requests the data via sampling quadrants but also retries for other quadrants.

Eventually, the bug was about rsmt2d not being suitable for recovering over the same data square multiple times, as it sets nil shares to zeroed shares to the given flattened square slice(it actually updates the given slice in place after all!). I will report and link here the issue soon with the suggested fix. The issue celestiaorg/rsmt2d#72. Unless it is fixed, this PR has a workaround with the TODO on it.

TODO

Closes #509
Closes #508
Closes #516 ,
Partially closes #604

Note: @vgonkivs was informed about the new implementation of RetrieveData

@Wondertan Wondertan added area:shares Shares and samples area:ipld IPLD plugin labels Apr 14, 2022
@Wondertan Wondertan requested review from adlerjohn and vgonkivs April 14, 2022 22:23
@Wondertan Wondertan self-assigned this Apr 14, 2022
@Wondertan Wondertan force-pushed the hlib/quadrant-retry branch 2 times, most recently from 0162aab to ffe9940 Compare April 15, 2022 08:15
@Wondertan
Copy link
Member Author

The test failure is #608

@adlerjohn
Copy link
Member

https://github.com/celestiaorg/rsmt2d/releases/tag/v0.4.0 is now released, which should be able to handle multiple repairs on the same square by representing missing shares as nil internally.

ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
@Wondertan Wondertan force-pushed the hlib/quadrant-retry branch from f802f6e to b4536c8 Compare April 19, 2022 13:34
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Left some smaller comments. will try to properly review soon.

ipld/retriever.go Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
service/share/interface.go Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Looks really good. Just a few questions and minor things. Hope we can merge this soon.

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Can we maybe add some debug or trace level logs in retriever.go?

ipld/read.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
service/share/share.go Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2022

Codecov Report

Merging #623 (7845ba9) into main (16dba97) will decrease coverage by 0.15%.
The diff coverage is 80.92%.

@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
- Coverage   61.70%   61.54%   -0.16%     
==========================================
  Files          93       96       +3     
  Lines        4969     5024      +55     
==========================================
+ Hits         3066     3092      +26     
- Misses       1581     1606      +25     
- Partials      322      326       +4     
Impacted Files Coverage Δ
node/config_opts.go 27.27% <0.00%> (-8.73%) ⬇️
node/node.go 60.86% <ø> (ø)
cmd/celestia/full.go 41.93% <33.33%> (-0.93%) ⬇️
cmd/celestia/light.go 41.93% <33.33%> (-0.93%) ⬇️
cmd/celestia/bridge.go 56.89% <50.00%> (-0.80%) ⬇️
cmd/flags_rpc.go 76.00% <76.00%> (ø)
service/share/full_availability.go 93.75% <91.66%> (-6.25%) ⬇️
ipld/retriever.go 91.95% <91.95%> (ø)
ipld/read.go 77.94% <100.00%> (-7.25%) ⬇️
node/components.go 94.11% <100.00%> (-0.69%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28020da...7845ba9. Read the comment docs.

@Wondertan Wondertan force-pushed the hlib/quadrant-retry branch from 7845ba9 to 371efc2 Compare April 24, 2022 21:45
@Wondertan
Copy link
Member Author

Wondertan commented Apr 24, 2022

I updated the branch with randomization and retries over columns. Better be done within this PR.

Tests are still failing due to known issue.

ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever.go Outdated Show resolved Hide resolved
ipld/retriever_quadrant.go Outdated Show resolved Hide resolved
ipld/retriever_quadrant.go Outdated Show resolved Hide resolved
ipld/retriever_quadrant.go Show resolved Hide resolved
ipld/retriever_quadrant.go Show resolved Hide resolved
service/share/share.go Show resolved Hide resolved
@Wondertan Wondertan force-pushed the hlib/quadrant-retry branch from 0401ab6 to 02d2dfa Compare April 26, 2022 13:57
@Wondertan
Copy link
Member Author

Rebased on main

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

looks good - waiting for subsequent test PR

you can resolve my open comments.

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

👏🏼

go.mod Show resolved Hide resolved
@Wondertan Wondertan merged commit cd2488e into main Apr 27, 2022
@Wondertan Wondertan deleted the hlib/quadrant-retry branch April 27, 2022 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ipld IPLD plugin area:shares Shares and samples kind:feat Attached to feature PRs
Projects
No open projects
Archived in project
6 participants