fix(share/p2p)!: `share.ErrNamespaceNotFound` and integration into shrex-nd #2156

distractedm1nd · 2023-05-03T06:15:08Z

This PR resolves #2145 . A followup PR may come that changes the response layer between the RPC and the internal GetSharesByNamespace call.

codecov-commenter · 2023-05-03T06:22:41Z

Codecov Report

Merging #2156 (e7b262b) into main (7e65a72) will decrease coverage by 0.10%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##             main    #2156      +/-   ##
==========================================
- Coverage   55.54%   55.44%   -0.10%     
==========================================
  Files         212      212              
  Lines       13583    13601      +18     
==========================================
- Hits         7544     7541       -3     
- Misses       5275     5294      +19     
- Partials      764      766       +2

Impacted Files	Coverage Δ
share/getter.go	`78.78% <ø> (ø)`
share/getters/cascade.go	`76.71% <0.00%> (-3.29%)`	⬇️
share/getters/store.go	`82.27% <0.00%> (ø)`
share/p2p/shrexnd/params.go	`100.00% <ø> (ø)`
share/p2p/shrexnd/pb/share.pb.go	`35.81% <ø> (ø)`
share/getters/utils.go	`75.00% <50.00%> (ø)`
share/getters/shrex.go	`83.63% <62.50%> (-1.95%)`	⬇️
share/p2p/shrexnd/server.go	`67.15% <81.81%> (+0.23%)`	⬆️
share/p2p/shrexnd/client.go	`68.85% <100.00%> (+0.51%)`	⬆️

... and 3 files with indirect coverage changes

walldiss · 2023-05-03T06:22:58Z

Please add support in ipld.Getter too

share/getters/store.go

distractedm1nd · 2023-05-03T06:59:34Z

@walldiss There is support for IPLDGetter - its just through the collectSharesByNamespace util. You can see getter_test to verify it works correctly

walldiss

Need support in shrex-getter to stop retries after client returns ErrNamespaceNotFound

share/getter.go

distractedm1nd · 2023-05-03T07:44:54Z

Thank you @walldiss, somehow I forgot the most important part of the PR 😂

share/getters/utils.go

share/getters/store.go

share/getter.go

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

derrandz · 2023-05-11T13:20:51Z

share/p2p/shrexnd/params.go

@@ -8,7 +8,7 @@ import (
 	"github.com/celestiaorg/celestia-node/share/p2p"
 )

-const protocolString = "/shrex/nd/0.0.1"
+const protocolString = "/shrex/nd/0.0.2"


I had a question about this while I was writing docs for peer manager and exploring getters, why is it that versions in other protocols we have are prefixed with v and shrex/nd is not. (e.g: /shrex/nd/v0.0.2)

Maybe since this is a breaking PR we can fix this and make this protocol uniform with the rest.

good catch. Will fix

you still didnt fix

it is fixed, just not showing in the diff

renaynay · 2023-05-17T08:36:39Z

share/p2p/shrexnd/params.go

@@ -8,7 +8,7 @@ import (
 	"github.com/celestiaorg/celestia-node/share/p2p"
 )

-const protocolString = "/shrex/nd/0.0.1"
+const protocolString = "/shrex/nd/0.0.2"


you still didnt fix

Wondertan · 2023-05-17T16:00:21Z

Conflicts

walldiss · 2023-05-18T14:46:09Z

share/getters/store.go

 	shares, err = collectSharesByNamespace(ctx, blockGetter, root, nID)
 	if errors.Is(err, ipld.ErrNodeNotFound) {
 		// convert error to satisfy getter interface contract
-		err = share.ErrNotFound
+		err = share.ErrNamespaceNotFound


As part of this PR collectSharesByNamespace has been changed to return share.ErrNamespaceNotFound. Could you please explain the case, when you get ipld.ErrNodeNotFound and need to convert it?

This is actually incorrect AFAIU. ErrNodeNotFound should in fact be converted to an ErrNotFound. So I will look into it one more time while writing tests and update

walldiss · 2023-05-18T14:49:26Z

share/getters/shrex.go

+	roots := filterRootsByNamespace(root, id)
+	if len(roots) == 0 {
+		return nil, share.ErrNamespaceNotFound
+	}


Optional: Would be nice if you find time to add quick test for this case.

done but not showing in diff, will reopen PR

…x-nd (#2230) This PR replaces #2156 , as gh was not showing the diff properly. It closes #2145 --------- Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

…x-nd (celestiaorg#2230) This PR replaces celestiaorg#2156 , as gh was not showing the diff properly. It closes celestiaorg#2145 --------- Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

distractedm1nd added area:shares Shares and samples kind:fix Attached to bug-fixing PRs labels May 3, 2023

distractedm1nd self-assigned this May 3, 2023

distractedm1nd requested review from renaynay, Wondertan, vgonkivs and walldiss as code owners May 3, 2023 06:15

Wondertan reviewed May 3, 2023

View reviewed changes

share/getters/store.go Show resolved Hide resolved

walldiss reviewed May 3, 2023

View reviewed changes

share/getter.go Outdated Show resolved Hide resolved

Wondertan reviewed May 3, 2023

View reviewed changes

share/getters/utils.go Outdated Show resolved Hide resolved

share/getters/store.go Show resolved Hide resolved

share/getter.go Outdated Show resolved Hide resolved

distractedm1nd and others added 5 commits May 10, 2023 08:34

feat(share)!: share.ErrNamespaceNotFound

2c155d7

feat!: bumping shrexnd protocol id for new status code

c7866c2

chore: fixing tests and comment

0bbc2a3

review fixes

e6e05f7

Apply suggestions from code review

a909e51

Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

distractedm1nd force-pushed the errnamespacenotfound branch from b6d8e89 to a909e51 Compare May 10, 2023 06:35

derrandz reviewed May 11, 2023

View reviewed changes

renaynay approved these changes May 17, 2023

View reviewed changes

walldiss reviewed May 18, 2023

View reviewed changes

Wondertan enabled auto-merge (squash) May 19, 2023 05:32

distractedm1nd closed this May 19, 2023

auto-merge was automatically disabled May 19, 2023 08:09
Pull request was closed

distractedm1nd mentioned this pull request May 19, 2023

fix(share/p2p)!: share.ErrNamespaceNotFound and integration into shrex-nd #2230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(share/p2p)!: `share.ErrNamespaceNotFound` and integration into shrex-nd #2156

fix(share/p2p)!: `share.ErrNamespaceNotFound` and integration into shrex-nd #2156

distractedm1nd commented May 3, 2023

codecov-commenter commented May 3, 2023 •

edited

Loading

walldiss commented May 3, 2023

distractedm1nd commented May 3, 2023

walldiss left a comment

distractedm1nd commented May 3, 2023

derrandz May 11, 2023 •

edited

Loading

distractedm1nd May 15, 2023

renaynay May 17, 2023

distractedm1nd May 19, 2023

renaynay May 17, 2023

Wondertan commented May 17, 2023

walldiss May 18, 2023

distractedm1nd May 19, 2023 •

edited

Loading

walldiss May 18, 2023

distractedm1nd May 19, 2023

fix(share/p2p)!: share.ErrNamespaceNotFound and integration into shrex-nd #2156

fix(share/p2p)!: share.ErrNamespaceNotFound and integration into shrex-nd #2156

Conversation

distractedm1nd commented May 3, 2023

codecov-commenter commented May 3, 2023 • edited Loading

Codecov Report

walldiss commented May 3, 2023

distractedm1nd commented May 3, 2023

walldiss left a comment

Choose a reason for hiding this comment

distractedm1nd commented May 3, 2023

derrandz May 11, 2023 • edited Loading

Choose a reason for hiding this comment

distractedm1nd May 15, 2023

Choose a reason for hiding this comment

renaynay May 17, 2023

Choose a reason for hiding this comment

distractedm1nd May 19, 2023

Choose a reason for hiding this comment

renaynay May 17, 2023

Choose a reason for hiding this comment

Wondertan commented May 17, 2023

walldiss May 18, 2023

Choose a reason for hiding this comment

distractedm1nd May 19, 2023 • edited Loading

Choose a reason for hiding this comment

walldiss May 18, 2023

Choose a reason for hiding this comment

distractedm1nd May 19, 2023

Choose a reason for hiding this comment

fix(share/p2p)!: `share.ErrNamespaceNotFound` and integration into shrex-nd #2156

fix(share/p2p)!: `share.ErrNamespaceNotFound` and integration into shrex-nd #2156

codecov-commenter commented May 3, 2023 •

edited

Loading

derrandz May 11, 2023 •

edited

Loading

distractedm1nd May 19, 2023 •

edited

Loading