share!(shwap): implement GetRange over Shwap #3773

vgonkivs · 2024-09-25T09:29:21Z

Implement GetRange over Shwap

codecov-commenter · 2024-12-06T11:20:22Z

Codecov Report

Attention: Patch coverage is 43.77990% with 470 lines in your changes missing coverage. Please review.

Project coverage is 44.93%. Comparing base (2469e7a) to head (daf1162).
Report is 433 commits behind head on main.

Files with missing lines	Patch %	Lines
share/shwap/pb/shwap.pb.go	36.76%	155 Missing and 29 partials ⚠️
share/shwap/range_namespace_data.go	52.22%	56 Missing and 19 partials ⚠️
share/shwap/range_namespace_data_id.go	52.68%	31 Missing and 13 partials ⚠️
...re/shwap/p2p/bitswap/range_namespace_data_block.go	52.63%	24 Missing and 12 partials ⚠️
share/shwap/p2p/bitswap/getter.go	0.00%	28 Missing ⚠️
share/shwap/namespace_data.go	0.00%	18 Missing ⚠️
share/shwap/getters/cascade.go	0.00%	14 Missing ⚠️
nodebuilder/share/share.go	0.00%	13 Missing ⚠️
share/eds/proofs_cache.go	75.00%	8 Missing and 4 partials ⚠️
store/getter.go	0.00%	12 Missing ⚠️
... and 8 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3773      +/-   ##
==========================================
+ Coverage   44.83%   44.93%   +0.10%     
==========================================
  Files         265      311      +46     
  Lines       14620    23302    +8682     
==========================================
+ Hits         6555    10471    +3916     
- Misses       7313    11669    +4356     
- Partials      752     1162     +410

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cristaloleg · 2024-12-06T13:30:16Z

share/eds/rsmt2d.go

+	ns libshare.Namespace,
+	from, to shwap.SampleCoords,
+) (shwap.RangeNamespaceData, error) {
+	rawShares := make([][]libshare.Share, 0, to.Row-from.Row+1)


Should we verify to.Row > from.Row ?

yes, it makes sense. I have checked this but on a deeper level. I guess the best place to check it in the cascade getter(sanity check before passing the request further)
cc @Wondertan

cristaloleg · 2024-12-06T13:31:12Z

share/shwap/getters/cascade.go

+	namespace libshare.Namespace,
+	from, to shwap.SampleCoords,
+) (shwap.RangeNamespaceData, error) {
+	ctx, span := tracer.Start(ctx, "cascade/get-shares-by-namespace", trace.WithAttributes(


Wrong tag? Just cascade/get-shares ?

Wondertan

First pass. Looks good

Wondertan · 2024-12-10T00:06:24Z

share/shwap/row_namespace_data.go

-	namespacedShares := make([]libshare.Share, count)
-	copy(namespacedShares, shares[from:from+count])
-


There was a subtle reason for doing that, and I don't remember exactly which one. @walldiss had to debug it for quite some time because initially, we avoided the copy as well. The comment explaining this is definitely missing.
Let's recover what is going on here

why do we need an extra copy here? Maybe it was left as an artifact? cc @walldiss

Wondertan · 2024-12-10T00:32:48Z

store/getter.go

+	acc, err := g.store.GetByHeight(ctx, h.Height())
+	if err != nil {
+		return shwap.RangeNamespaceData{}, err
+	}
+	return acc.RangeNamespaceData(
+		ctx,
+		ns,
+		from, to,
+	)
+}


Error wrappings are helpful here. Let's keep them as with other methods.

Wondertan · 2024-12-10T01:17:28Z

share/shwap/namespace_data.go

-// Verify checks the integrity of the NamespaceData against a provided root and namespace.
-func (nd NamespaceData) Verify(root *share.AxisRoots, namespace libshare.Namespace) error {


The rule of thumb we have: Validate - stateless, Verify - stateful.

In this case, it is stateful as it expects additional states to do verification, so it should be kept Verify.

Wondertan · 2024-12-10T01:19:10Z

.github/workflows/integration-tests.yml

@@ -143,3 +143,18 @@ jobs:

      - name: run sync tests
        run: make test-integration SHORT=true TAGS=pruning
+
+  share_tests:


Extract to a separate PR?

Wondertan · 2024-12-10T01:20:50Z

nodebuilder/share/share.go

+	GetRange(
+		ctx context.Context, ns libshare.Namespace, height uint64, from, to uint32, proofsOnly bool,
+	) (*Range, error)


We decided to use shwap,SampleCoords in the node API. As we use them already in other methods and internally it is still SampleCoords, I propose to move use them here as well.

Akchually, the SampleCoords are for the whole EDS, while this index is ODS only. For that reason, we should consider having another type, particularly for ODS coordinates or indices. We could also try to have them both in a single type, but we should be careful with documentation and input validation. (This is for all API, Getters, Accessors)

To my understanding SampleCoords can serve for both cases as it contains RowIndex and ColIndex which will be same. We should only check that range request asks for ODS only shares(RowIndex < len(dah.RowRoots)/2 && ColIndex < len(dah.ColRoots)/2).

Decided to be consistent with all other methods. Reworked to coordinates

Wondertan · 2024-12-11T14:34:55Z

share/shwap/range_namespace_data.go

+		for offset := range shrs[from.Col:end] {
+			if !namespace.Equals(shrs[from.Col+offset].Namespace()) {
+				return RangeNamespaceData{},
+					fmt.Errorf("targeted namespace was not found in share at {Row: %d, Col:%d}",


Suggested change

fmt.Errorf("targeted namespace was not found in share at {Row: %d, Col:%d}",

fmt.Errorf("targeted namespace was not found in share at {Row: %d, Col: %d}",

Wondertan · 2024-12-11T14:35:22Z

share/shwap/range_namespace_data.go

+
+// Validate performs a validation of the incoming data. It ensures that the response contains proofs and performs
+// data validation in case the user has requested it.
+func (rngdata *RangeNamespaceData) Validate(root *share.AxisRoots, req *RangeNamespaceDataID) error {


Please use Verify

Wondertan · 2024-12-11T14:50:28Z

share/shwap/p2p/bitswap/getter.go

+	for _, blk := range blks {
+		rnd := blk.(*RangeNamespaceDataBlock).Container
+		return rnd, nil
+	}
+	return shwap.RangeNamespaceData{}, errors.New("no data inside block")


nit: Fetch guarantees having all blocks populated if error is nil, so we can simply do

Suggested change

for _, blk := range blks {

rnd := blk.(*RangeNamespaceDataBlock).Container

return rnd, nil

}

return shwap.RangeNamespaceData{}, errors.New("no data inside block")

return blks[0].(*RangeNamespaceDataBlock).Container, nil

Wondertan · 2024-12-11T14:54:20Z

share/shwap/pb/shwap.proto

+message NamespaceData {
+    repeated RowNamespaceData namespaceData = 1;
+}
+


What about also adding RangeNamespaceData in proto? It would embed NamespaceData. The main motivation would be keeping all the containers part of proto "spec" for consistency and clarity.

Yes, I agree

Wondertan · 2024-12-11T14:54:39Z

share/shwap/pb/shwap.proto

 message Share {
    bytes data = 1;
 }

 enum AxisType {
    ROW = 0;
    COL = 1;
-}
+}


Get back the \n

rach-id

Left some questions to understand more

rach-id · 2025-01-13T20:01:54Z

nodebuilder/share/share.go

@@ -56,7 +55,9 @@ type Module interface {
 		ctx context.Context, height uint64, namespace libshare.Namespace,
 	) (shwap.NamespaceData, error)
 	// GetRange gets a list of shares and their corresponding proof.
-	GetRange(ctx context.Context, height uint64, start, end int) (*GetRangeResult, error)
+	GetRange(
+		ctx context.Context, ns libshare.Namespace, height uint64, from, to uint32, proofsOnly bool,


[docs]
would be good to know if the from and to are inclusive or exclusive. FYI, for core we use [from:to[

changed to coordinates which represents the first and the last share of the range

rach-id · 2025-01-15T12:23:31Z

share/shwap/range_namespace_data.go

+}
+
+// RangedNamespaceDataFromShares builds a range of namespaced data for the given coordinates:
+// shares is a list of shares(grouped by the rows) relative to the data square and


Suggested change

// shares is a list of shares(grouped by the rows) relative to the data square and

// shares is a list of shares (grouped by the rows) relative to the data square and

rach-id · 2025-01-15T13:11:41Z

share/shwap/range_namespace_data.go

+			fmt.Errorf("start row must be less or equal to end row: %d-%d", from.Row, to.Row)
+	}
+
+	odsSize := len(shares[0]) / 2


what if the set of shares is smaller than a row?

shares[0] is a full row

rach-id · 2025-01-15T13:12:11Z

share/shwap/range_namespace_data.go

+// from is the coordinates of the first share of the range within the EDS.
+// to is the coordinates of the last inclusive share of the range within the EDS.
+func RangedNamespaceDataFromShares(
+	shares [][]libshare.Share,


do shares include all the row shares or just the shares we want to prove?

RangedNamespaceDataFromShares accepts all shares from the row but the data structure contains only shares that we want to prove

rach-id · 2025-01-15T13:15:05Z

share/shwap/range_namespace_data.go

+		nmtTree := nmt.New(
+			appconsts.NewBaseHashFunc(),
+			nmt.NamespaceIDSize(libshare.NamespaceSize),
+			nmt.IgnoreMaxNamespace(true),
+		)
+		tree.SetTree(nmtTree)


Suggested change

nmtTree := nmt.New(

appconsts.NewBaseHashFunc(),

nmt.NamespaceIDSize(libshare.NamespaceSize),

nmt.IgnoreMaxNamespace(true),

)

tree.SetTree(nmtTree)

I don't think creating the tree manually or setting the tree here is necessary.

wdym? I want to build nmt proof

this line:

tree := wrapper.NewErasuredNamespacedMerkleTree(uint64(odsSize), uint(row))

does that for you. you don't need those other lines I deleted

rach-id · 2025-01-15T13:15:32Z

share/shwap/range_namespace_data.go

+		)
+		tree.SetTree(nmtTree)
+
+		shrs := shares[i]


[nit]

Suggested change

shrs := shares[i]

rowShares := shares[i]

rach-id · 2025-01-15T13:20:45Z

share/shwap/range_namespace_data.go

+
+		outside, err := share.IsOutsideRange(namespace, root, root)
+		if err != nil {
+			return RangeNamespaceData{}, err
+		}
+		if outside {
+			return RangeNamespaceData{}, ErrNamespaceOutsideRange
+		}


this check could be moved to the top of the function to something like:

Suggested change

outside, err := share.IsOutsideRange(namespace, root, root)

if err != nil {

return RangeNamespaceData{}, err

}

if outside {

return RangeNamespaceData{}, ErrNamespaceOutsideRange

}

outside, err := share.IsOutsideRange(namespace, shares[0][0], shares[len(shares)-1][len(shares[len(shares)-1])-1])

if err != nil {

return RangeNamespaceData{}, err

}

if outside {

return RangeNamespaceData{}, ErrNamespaceOutsideRange

}

This if we assume that the provided list of shares is ordered by namespace. If this is the case, we should document it as a precondition in the function. Otherwise, we should add more checks on the list of shares provided

if the shares are out of order, then tree.Push() fails with

if nID.Less(n.leaves[curSize-1][:nidSize]) { return nil, fmt.Errorf( "%w: last namespace: %x, pushed: %x", ErrInvalidPushOrder, n.leaves[curSize-1][:nidSize], nID, ) }

Also, this function is called from the accessors(within 1 machine), so there is no reason not to trust them.

rach-id · 2025-01-15T14:52:22Z

share/shwap/range_namespace_data.go

+		end := odsSize
+		if i == len(shares)-1 {
+			// `to.Col` is an inclusive index
+			end = to.Col + 1


[nit]
would be nice to reflect this in the name and rename it to exclusiveEnd

rach-id · 2025-01-15T14:53:13Z

share/shwap/range_namespace_data.go

+		for offset := range shrs[from.Col:end] {
+			if !namespace.Equals(shrs[from.Col+offset].Namespace()) {
+				return RangeNamespaceData{},
+					fmt.Errorf("targeted namespace was not found in share at {Row: %d, Col:%d}",
+						row, from.Col+offset,
+					)
+			}
+		}


also this would be nice to do at the beginning of the function to avoid running all the logic to find out the provided range of shares is malformed

Moving it to the beginning of the function could allow us to avoid extra checks of the share.IsOutsideRange. wdyt?

rach-id · 2025-01-15T15:56:12Z

share/shwap/range_namespace_data.go

+}
+
+// ProveRange proves that a range of shares exist in the set of rows that are part of the merkle tree of the data root.
+func (rngdata *RangeNamespaceData) ProveRange(roots *share.AxisRoots, startRow int) *types.ShareProof {


what is this proving? how do you specify the set of shares to be proved?

RangeNamespaceData is a slice of the RowNamespaceData, that contains shares and nmt proofs for these shares:

type RowNamespaceData struct { Shares []libshare.Share `json:"shares"` // Shares within the namespace. Proof *nmt.Proof `json:"proof"` // Proof of the shares' inclusion in the namespace. }

ProveRange builds a proof of the shares to the data root

share/shwap/range_namespace_data_id.go

vgonkivs force-pushed the implement_range_container branch 3 times, most recently from a89eaf5 to 7df200c Compare December 3, 2024 14:34

vgonkivs self-assigned this Dec 3, 2024

vgonkivs marked this pull request as ready for review December 6, 2024 11:40

vgonkivs requested review from renaynay, Wondertan, walldiss and cristaloleg as code owners December 6, 2024 11:40

vgonkivs added kind:misc Attached to miscellaneous PRs kind:break! Attached to breaking PRs labels Dec 6, 2024

vgonkivs changed the title ~~Implement range container~~ share!(shwap): implement GetRange over Shwap Dec 6, 2024

vgonkivs force-pushed the implement_range_container branch from 6f8e471 to 1327371 Compare December 6, 2024 11:45

cristaloleg reviewed Dec 6, 2024

View reviewed changes

Wondertan reviewed Dec 11, 2024

View reviewed changes

rach-id reviewed Jan 15, 2025

View reviewed changes

vgonkivs added 12 commits January 27, 2025 17:48

define and implement range namespacedata request/response

a4a2f45

enable range request in the accessors

d2bcbfc

add range request to getter interface

87844ca

fix lint issues

e38db42

fixes

00c0941

reduce range namespace data id

24ab5dd

prove range to the dataroot

bd8dfb7

update getters

ed908b6

update api

6c4e1a7

fix issues + add more testing

d69f9d3

small fix

4233938

fix review comments

36e61c9

vgonkivs added 4 commits January 29, 2025 17:43

use cache for complete rows

45ebd35

rework api to apply coordinates

9313f45

add helper function to calculate the coordinates

4d549ce

add swamp tests

bf00550

vgonkivs force-pushed the implement_range_container branch from 6229518 to bf00550 Compare January 29, 2025 15:47

vgonkivs added 3 commits January 29, 2025 17:57

fix nits

8b6fcb6

increase timeout

b93611b

fix lint

fcc89e3

vgonkivs force-pushed the implement_range_container branch from a8f5dde to fcc89e3 Compare January 29, 2025 16:43

remove corpus

daf1162

cristaloleg reviewed Jan 30, 2025

View reviewed changes

share/shwap/range_namespace_data_id.go Show resolved Hide resolved

cristaloleg mentioned this pull request Jan 30, 2025

refactor(all): use encoding.AppendBinary rather then custom things #4079

Open

		namespacedShares := make([]libshare.Share, count)
		copy(namespacedShares, shares[from:from+count])

		// Verify checks the integrity of the NamespaceData against a provided root and namespace.
		func (nd NamespaceData) Verify(root *share.AxisRoots, namespace libshare.Namespace) error {

	fmt.Errorf("targeted namespace was not found in share at {Row: %d, Col:%d}",
	fmt.Errorf("targeted namespace was not found in share at {Row: %d, Col: %d}",

	// shares is a list of shares(grouped by the rows) relative to the data square and
	// shares is a list of shares (grouped by the rows) relative to the data square and

share!(shwap): implement GetRange over Shwap #3773

Are you sure you want to change the base?

share!(shwap): implement GetRange over Shwap #3773

Conversation

vgonkivs commented Sep 25, 2024 • edited Loading

codecov-commenter commented Dec 6, 2024 • edited Loading

Codecov Report

cristaloleg Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

vgonkivs Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgonkivs Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgonkivs Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

vgonkivs commented Sep 25, 2024 •

edited

Loading

codecov-commenter commented Dec 6, 2024 •

edited

Loading

cristaloleg Dec 6, 2024 •

edited

Loading

vgonkivs Jan 27, 2025 •

edited

Loading

vgonkivs Jan 9, 2025 •

edited

Loading

vgonkivs Jan 27, 2025 •

edited

Loading