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(eds/store): store.GetDAH #1511

Merged
merged 12 commits into from
Dec 22, 2022

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Dec 19, 2022

Overview

Closes #1509

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Linked issues closed with keywords

@distractedm1nd distractedm1nd added area:shares Shares and samples kind:feat Attached to feature PRs labels Dec 19, 2022
@distractedm1nd distractedm1nd self-assigned this Dec 19, 2022
@Wondertan
Copy link
Member

@distractedm1nd, It seems like we would have to fully access the shard-avoiding cache in GetCar to solve the issue. The Seeker is fine, but it does not work when multiple requests for the same car happen at the same time. Readers must be exclusive. Pretty embarrassing that I didn't catch it earlier.

The optimization using the same mmap file works only for the multiple blockstore accessors because it has internal mutex protection. We can try to make similar synchronization and make the same mmap file work over simultaneous GetCar, but in fact, this would be sequential instead of parallel, though it has only one CAR copy in mem, rather than the amount of requests amount.

The simplest solution right now is just to avoid cache in GetCar and further, we can optimize memory usage by making multiple Readers over the same mmap, rather than multiple Reders with personal mmap

share/eds/store.go Outdated Show resolved Hide resolved
walldiss
walldiss previously approved these changes Dec 19, 2022
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.

Nice!

share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
@vgonkivs
Copy link
Member

vgonkivs commented Dec 19, 2022

Unit test failed :

=== RUN   TestBlockstore_Operations
(https://github.com/celestiaorg/celestia-node/actions/runs/3731466266/jobs/6329873704#step:4:1267)
    blockstore_test.go:36: 
(https://github.com/celestiaorg/celestia-node/actions/runs/3731466266/jobs/6329873704#step:4:1268)
        	Error Trace:	/home/runner/work/celestia-node/celestia-node/share/eds/blockstore_test.go:36
(https://github.com/celestiaorg/celestia-node/actions/runs/3731466266/jobs/6329873704#step:4:1269)
        	Error:      	Received unexpected error:
(https://github.com/celestiaorg/celestia-node/actions/runs/3731466266/jobs/6329873704#step:4:1270)
        	            	failed to read DAH from car reader: malformed car; header is bigger than util.MaxAllowedSectionSize
(https://github.com/celestiaorg/celestia-node/actions/runs/3731466266/jobs/6329873704#step:4:1271)
        	Test:       	TestBlockstore_Operations

share/eds/store.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Note about updating the ADR

Co-authored-by: Viacheslav <viacheslavgonkivskyi@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Merging #1511 (4478305) into main (e3062d4) will decrease coverage by 0.25%.
The diff coverage is 65.95%.

@@            Coverage Diff             @@
##             main    #1511      +/-   ##
==========================================
- Coverage   55.49%   55.23%   -0.26%     
==========================================
  Files         203      210       +7     
  Lines       12126    12866     +740     
==========================================
+ Hits         6729     7107     +378     
- Misses       4730     5038     +308     
- Partials      667      721      +54     
Impacted Files Coverage Δ
share/eds/store.go 58.29% <65.21%> (+6.19%) ⬆️
share/eds/blockstore.go 38.77% <100.00%> (ø)
nodebuilder/p2p/genesis.go 0.00% <0.00%> (-33.34%) ⬇️
header/store/init.go 72.72% <0.00%> (-27.28%) ⬇️
nodebuilder/header/constructors.go 54.54% <0.00%> (-20.46%) ⬇️
nodebuilder/share/module.go 58.73% <0.00%> (-18.36%) ⬇️
share/empty.go 86.66% <0.00%> (-13.34%) ⬇️
header/core/exchange.go 31.81% <0.00%> (-9.10%) ⬇️
header/p2p/exchange.go 66.25% <0.00%> (-8.44%) ⬇️
nodebuilder/share/constructors.go 92.10% <0.00%> (-7.90%) ⬇️
... and 41 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

vgonkivs
vgonkivs previously approved these changes Dec 20, 2022
Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@distractedm1nd distractedm1nd changed the title feat(eds): adding DAH to CARBlockstore feat(eds/store): store.GetDAH Dec 21, 2022
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Dec 21, 2022
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, a q and a nit

share/eds/store.go Outdated Show resolved Hide resolved
share/eds/store.go Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
Co-authored-by: Vlad <13818348+walldiss@users.noreply.github.com>
vgonkivs
vgonkivs previously approved these changes Dec 22, 2022
Wondertan
Wondertan previously approved these changes Dec 22, 2022
walldiss
walldiss previously approved these changes Dec 22, 2022
share/eds/store.go Outdated Show resolved Hide resolved
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.

utACK

@distractedm1nd distractedm1nd merged commit 6179f3e into celestiaorg:main Dec 22, 2022
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:feat Attached to feature PRs
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

share/eds: Store.GetRoot
7 participants