service/share, ipld: adds ability to retrieve shares by namespace ID #170

renaynay · 2021-11-04T16:36:02Z

This PR implements the ability to retrieve shares by namespace ID via two helper methods in the ipld package as well as a broader RetrieveShares function in service/share package.

One thing to note:
PutData in ipld/write.go prepends the namespaceID twice, so in several tests, it's necessary to check the returnedShare[8:] against the expectedShare.

TODO:

full test for RetrieveShares
clean up code / address TODOs
lint / make sure good test coverage
check on mod to make sure no accidents occurred

Wondertan

General comment: Pls base it on top of #167 and implement a method there.

Initial review will need a more in-depth in logic itself, but for now pls fix mentioned things

go.mod

service/share/share.go

Wondertan

The main stopper here is that we download the data we are not interested in to check if we stopped loading shares in a row. We shouldn't merge such behavior.

Also, some things were fixed during the sync review in between.

service/share/sample.go

service/share/share_test.go

ipld/read.go

service/share/testing.go

service/share/share.go

ipld/read.go

ipld/errors.go

Wondertan

Approve!!!

One thing: pls create an issue about parallelization of GetSharesByNamespace. The speed-up must be significant if we parallelized GetSharesByNamespace for each Row independently.

Wondertan · 2021-11-09T12:46:28Z

The tests if failing due to #173. To resolve this here we can either:

Rerun until success
Merge share: Availability interface and implementation #171 first, which fixes the problem inside.

@renaynay, WDYT?

ipld/plugin/nmt.go

Wondertan · 2021-11-10T14:20:20Z

@renaynay, rebase on master and tests will go green

Wondertan · 2021-11-10T14:28:37Z

1.16 tests running for too long, ***, seems like TestSubscriber still hangs

Wondertan · 2021-11-10T14:30:48Z

@renaynay, add pls context.WithTimeout(time.Second * 10) to TestSubscriber. This way we ensure that tests won't run longer than 10*seconds if it hangs.

Wondertan · 2021-11-10T14:37:06Z

Rerun helped and we can merge this, though in master, rarely hanging still might be the case.

liamsi

Extra dope 🚀
Left some minor comments and nits.

go.mod

ipld/plugin/nmt.go

ipld/read.go

ipld/read_test.go

service/share/share_test.go

ipld/read_test.go

adlerjohn

No comments, other than agreeing with @liamsi's review

adlerjohn · 2021-11-11T14:15:35Z

Conflicting with default branch @renaynay

This reverts commit ee8d591.

adlerjohn

liamsi

Awesome @renaynay 🚀

…170)

renaynay requested review from liamsi, mattdf and Wondertan as code owners November 4, 2021 16:36

Wondertan requested changes Nov 5, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

service/share/share.go Outdated Show resolved Hide resolved

service/share/share.go Outdated Show resolved Hide resolved

renaynay mentioned this pull request Nov 5, 2021

shares: Implement the SharesService #23

Closed

5 tasks

renaynay added the area:shares Shares and samples label Nov 5, 2021

renaynay force-pushed the shares-by-namespace branch 2 times, most recently from 884b578 to 9f95c2a Compare November 8, 2021 10:00

renaynay requested a review from Wondertan November 8, 2021 10:00

Wondertan requested changes Nov 8, 2021

View reviewed changes

Wondertan reviewed Nov 8, 2021

View reviewed changes

service/share/testing.go Outdated Show resolved Hide resolved

service/share/share.go Outdated Show resolved Hide resolved

ipld/read.go Outdated Show resolved Hide resolved

ipld/errors.go Outdated Show resolved Hide resolved

renaynay requested a review from Wondertan November 9, 2021 10:41

Wondertan previously approved these changes Nov 9, 2021

View reviewed changes

mattdf reviewed Nov 10, 2021

View reviewed changes

ipld/plugin/nmt.go Outdated Show resolved Hide resolved

renaynay dismissed Wondertan’s stale review via c108645 November 10, 2021 14:22

renaynay force-pushed the shares-by-namespace branch from c108645 to d287d6e Compare November 10, 2021 14:23

renaynay requested review from mattdf and Wondertan November 10, 2021 14:24

liamsi reviewed Nov 10, 2021

View reviewed changes

adlerjohn reviewed Nov 10, 2021

View reviewed changes

renaynay requested a review from liamsi November 11, 2021 13:45

renaynay force-pushed the shares-by-namespace branch from 62271b1 to cc42239 Compare November 11, 2021 15:56

implemented RetrieveShares

293d196

renaynay and others added 17 commits November 11, 2021 16:57

row indices from NID -> row roots from NID

d00bd8d

test prog

9571017

ipld/plugin: make nmtLeafNode to return 0 links

d861a90

ipld: alternative recursive GetLeavesByNamespace

d3cbed6

Revert "ipld/plugin: make nmtLeafNode to return 0 links"

73ddf7e

This reverts commit ee8d591.

ipld: detect leaf node by 1 link and not by zero

3fc4f36

use GetLeavesByNamespace

18cba8a

progress - waiting on debugging random 8 prepended bytes

d8cf427

ipld: fix comment

b3aa3ae

share: fix test

0b57c9e

table test

bd061d0

linted

21e8661

docs for GetLeavesByNamespace

0082283

overflow test

ab73307

changes req by @Wondertan

aa23e4d

use nmt exported min max funcs

f80160f

ismail requested changes

bd47aab

renaynay force-pushed the shares-by-namespace branch from cc42239 to bd47aab Compare November 11, 2021 15:59

fixed test

3961232

adlerjohn approved these changes Nov 11, 2021

View reviewed changes

liamsi approved these changes Nov 11, 2021

View reviewed changes

renaynay merged commit c2d6a35 into celestiaorg:main Nov 12, 2021

renaynay deleted the shares-by-namespace branch November 12, 2021 13:15

Wondertan mentioned this pull request Nov 13, 2021

Implement DAS and query for app specific data celestiaorg/dalc#17

Merged

renaynay mentioned this pull request Nov 16, 2021

ipld: Implement API for application sovereignty: query data per namespace #5

Closed

renaynay referenced this pull request in renaynay/celestia-node Nov 16, 2021

service/share, ipld: adds ability to retrieve shares by namespace ID (#…

873943d

…170)

renaynay referenced this pull request in renaynay/celestia-node Nov 16, 2021

service/share, ipld: adds ability to retrieve shares by namespace ID (#…

cb9b47f

…170)

renaynay referenced this pull request in renaynay/celestia-node Nov 16, 2021

service/share, ipld: adds ability to retrieve shares by namespace ID (#…

2e1f293

…170)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service/share, ipld: adds ability to retrieve shares by namespace ID #170

service/share, ipld: adds ability to retrieve shares by namespace ID #170

renaynay commented Nov 4, 2021 •

edited

Loading

Wondertan left a comment

Wondertan left a comment

Wondertan left a comment

Wondertan commented Nov 9, 2021

Wondertan commented Nov 10, 2021

Wondertan commented Nov 10, 2021

Wondertan commented Nov 10, 2021

Wondertan commented Nov 10, 2021

liamsi left a comment

adlerjohn left a comment

adlerjohn commented Nov 11, 2021

adlerjohn left a comment

liamsi left a comment

service/share, ipld: adds ability to retrieve shares by namespace ID #170

service/share, ipld: adds ability to retrieve shares by namespace ID #170

Conversation

renaynay commented Nov 4, 2021 • edited Loading

Wondertan left a comment

Choose a reason for hiding this comment

Wondertan left a comment

Choose a reason for hiding this comment

Wondertan left a comment

Choose a reason for hiding this comment

Wondertan commented Nov 9, 2021

Wondertan commented Nov 10, 2021

Wondertan commented Nov 10, 2021

Wondertan commented Nov 10, 2021

Wondertan commented Nov 10, 2021

liamsi left a comment

Choose a reason for hiding this comment

adlerjohn left a comment

Choose a reason for hiding this comment

adlerjohn commented Nov 11, 2021

adlerjohn left a comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

renaynay commented Nov 4, 2021 •

edited

Loading