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): eds.Blockstore #1395

Merged
merged 10 commits into from
Dec 7, 2022
Merged

Conversation

distractedm1nd
Copy link
Member

@distractedm1nd distractedm1nd commented Nov 24, 2022

Overview

This PR adds a blockstore to eds.Store. It was extracted from the eds.Store scaffolding PR. Tests are still needed, so still in draft for now.

Closes #1115, #1427

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 NA
  • Linked issues closed with keywords

@distractedm1nd distractedm1nd added area:shares Shares and samples kind:feat Attached to feature PRs labels Nov 24, 2022
@distractedm1nd distractedm1nd self-assigned this Nov 24, 2022
@distractedm1nd distractedm1nd marked this pull request as ready for review November 24, 2022 11:16
@codecov-commenter
Copy link

Codecov Report

Merging #1395 (31e2b28) into main (b4a8145) will decrease coverage by 0.05%.
The diff coverage is 65.00%.

@@            Coverage Diff             @@
##             main    #1395      +/-   ##
==========================================
- Coverage   55.28%   55.22%   -0.06%     
==========================================
  Files         180      182       +2     
  Lines       10904    11074     +170     
==========================================
+ Hits         6028     6116      +88     
- Misses       4276     4344      +68     
- Partials      600      614      +14     
Impacted Files Coverage Δ
share/eds/blockstore.go 50.00% <50.00%> (ø)
share/eds/store.go 48.97% <70.00%> (+0.77%) ⬆️
nodebuilder/p2p/p2p.go 85.18% <85.18%> (ø)
api/rpc/client/client.go 78.57% <100.00%> (+0.79%) ⬆️
nodebuilder/p2p/flags.go 67.46% <100.00%> (ø)
nodebuilder/p2p/host.go 90.90% <100.00%> (+9.65%) ⬆️
nodebuilder/p2p/module.go 93.75% <100.00%> (+0.20%) ⬆️
nodebuilder/rpc/rpc.go 100.00% <100.00%> (ø)
share/eds/byzantine/bad_encoding.go 66.00% <0.00%> (-3.00%) ⬇️
share/eds/byzantine/pb/share.pb.go 32.88% <0.00%> (-1.97%) ⬇️
... and 5 more

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

share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.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.

Mostly nits and questions. Nothing criminal was found. The locking design is legit

share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/store.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Show resolved Hide resolved
share/eds/blockstore.go Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/accessor_cache.go Outdated Show resolved Hide resolved
share/eds/accessor_cache.go Outdated Show resolved Hide resolved
share/eds/accessor_cache.go Outdated Show resolved Hide resolved
share/eds/blockstore.go Show resolved Hide resolved
share/eds/accessor_cache.go Show resolved Hide resolved
share/eds/blockstore.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.

One last comment and LFG. Also, let's make an issue for configurability

share/eds/blockstore.go Outdated Show resolved Hide resolved
share/eds/cache_test.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Dec 6, 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.

🚢 it

walldiss
walldiss previously approved these changes Dec 6, 2022
share/eds/cache_test.go Show resolved Hide resolved
@distractedm1nd distractedm1nd merged commit b27e710 into celestiaorg:main Dec 7, 2022
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Dec 9, 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: EDSStore.Blockstore
5 participants