feat: create square builder class #1659

cmwaters · 2023-04-26T14:09:15Z

This creates a builder struct for assembling squares from a (prioritised) list of transactions.

It also includes two functions Construct and Reconstruct as listed in ADR20

Co-authored-by: Rootul P <rootulp@gmail.com>

cmwaters · 2023-04-26T14:11:26Z

app/estimate_square_size_test.go

+func txsToBytes(txs coretypes.Txs) [][]byte {
+	e := make([][]byte, len(txs))
+	for i, tx := range txs {
+		e[i] = []byte(tx)
+	}
+	return e
+}


note: this was moved out from the shares package because no one was using it. IIRC there's a similar function in core we could just use

cmwaters · 2023-04-26T14:12:24Z

pkg/square/builder.go

+		//
+		// Note that the padding would actually belong to the namespace of the transaction before it, but
+		// this makes no difference to the total share size.
+		maxPadding: shares.SubTreeWidth(shareSize) - 1,


For the reviewer: Please make sure I have this assumption correct

[question][blocking] what is the relationship between subtree width and

Blobs start at an index that is equal to a multiple of the blob length divided by MaxSquareSize rounded up.

Source: https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md

Related: should we extract a function that calculates the max padding for a blob and unit test it in isolation? Then the unit tests can use the table here to verify max padding matches what we expect: https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/assets/worst-case-padding-in-blob-size-range.png

Blobs start at an index that is equal to a multiple of the blob length divided by MaxSquareSize rounded up.

Where do you see this in ADR013? I can't find it

Now we use SubtreeRootThreshold to work out the height in the tree of the highest merkle mountain and thus the width of the leaves

shares.SubTreeWidth(shareSize) - 1 is correct, and good idea @rootulp having a function to make this explicit is a good idea.

created #1660

Where do you see this in ADR013? I can't find it

Sorry I should've linked directly to the line:

celestia-app/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md

Line 27 in f31ee58

Blobs start at an index that is equal to a multiple of the blob length divided by `MaxSquareSize` rounded up.

cmwaters · 2023-04-26T14:13:07Z

pkg/shares/utils.go

-func isPowerOf2(v uint64) bool {
-	return v&(v-1) == 0 && v != 0
-}
-
-func BlobsFromProto(blobs []core.Blob) ([]coretypes.Blob, error) {
-	result := make([]coretypes.Blob, len(blobs))
-	for i, blob := range blobs {
-		if blob.ShareVersion > math.MaxUint8 {
-			return nil, fmt.Errorf("share version %d is too large to be a uint8", blob.ShareVersion)
-		}
-		result[i] = coretypes.Blob{
-			NamespaceID:  blob.NamespaceId,
-			Data:         blob.Data,
-			ShareVersion: uint8(blob.ShareVersion),
-		}
-	}
-	return result, nil
-}


Note: these functions are either not used (and probably shoudn't be) or redundant

cmwaters · 2023-04-26T14:13:37Z

pkg/shares/utils.go

+// AvailableBytesFromSparseShares returns the maximum amount of bytes that could fit in `n` sparse shares
+func AvailableBytesFromSparseShares(n int) int {
+	if n <= 0 {
+		return 0
+	}
+	if n == 1 {
+		return appconsts.FirstSparseShareContentSize
+	}
+	return (n-1)*appconsts.ContinuationSparseShareContentSize + appconsts.FirstSparseShareContentSize
+}


Something that the node team might want to use

pkg/square/builder.go

rootulp · 2023-04-26T14:43:22Z

pkg/square/builder.go

+	// here we keep track of the actual data to go in a square
+	txs   [][]byte
+	pfbs  []*coretypes.IndexWrapper
+	blobs []*element


[informational][no change needed] this godoc comment will only apply to the first field

rootulp · 2023-04-26T15:07:00Z

pkg/square/builder.go

+		// Imagine if, according to the share commitment rules, a transcation took up 11 shares
+		// and had the merkle mountain tree commitment of 4,4,2,1. The first part of the share commitment


I'm having a tough time relating this example to the ascii diagram above b/c it discusses a blob that occupies 11 shares but there are only 8 possible share indexes in the ASCII diagram

The 8 possible shares indexes are a snapshot in the tree. It's used to demonstrate that if the index of the last transaction ended at 1 (inclusive), and the SubtreeWidth() was 4, then the next one would start at index 4 and we would have 2 shares of padding

I also think this example is out dated with the implementation of ADR013

I copied this tree across from somewhere else just so I could explain why we have padding and why that formula gives us the maximum possible padding

rootulp · 2023-04-26T15:12:13Z

pkg/square/builder.go

+		//
+		// Note that the padding would actually belong to the namespace of the transaction before it, but
+		// this makes no difference to the total share size.
+		maxPadding: shares.SubTreeWidth(shareSize) - 1,


[question][blocking] what is the relationship between subtree width and

Blobs start at an index that is equal to a multiple of the blob length divided by MaxSquareSize rounded up.

Source: https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-013-non-interactive-default-rules-for-zero-padding.md

Related: should we extract a function that calculates the max padding for a blob and unit test it in isolation? Then the unit tests can use the table here to verify max padding matches what we expect: https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/assets/worst-case-padding-in-blob-size-range.png

codecov-commenter · 2023-04-26T15:20:35Z

Codecov Report

Merging #1659 (0dd43ef) into main (4c73824) will increase coverage by 1.11%.
The diff coverage is 73.84%.

@@            Coverage Diff             @@
##             main    #1659      +/-   ##
==========================================
+ Coverage   51.56%   52.67%   +1.11%     
==========================================
  Files          95       97       +2     
  Lines        5954     6185     +231     
==========================================
+ Hits         3070     3258     +188     
- Misses       2570     2598      +28     
- Partials      314      329      +15

Impacted Files	Coverage Δ
pkg/namespace/namespace.go	`46.05% <0.00%> (-3.95%)`	⬇️
pkg/shares/share_splitting.go	`64.65% <0.00%> (ø)`
pkg/shares/utils.go	`57.69% <11.11%> (+7.69%)`	⬆️
pkg/square/square.go	`71.42% <71.42%> (ø)`
pkg/square/builder.go	`86.00% <86.00%> (ø)`
pkg/da/data_availability_header.go	`78.89% <100.00%> (-0.20%)`	⬇️

evan-forbes

nice work!! overall this is a really big improvement

mostly non-blocking naming things

evan-forbes · 2023-04-26T15:26:01Z

pkg/square/square.go

+// Reconstruct takes a list of ordered transactions and reconstructs a square, validating that
+// all PFBs are ordered after regular transactions and that the transactions don't collectively
+// exceed the maxSquareSize. Note that this function does not check the underlying validity of
+// the transactions.
+func Reconstruct(txs [][]byte, maxSquareSize int) (Square, error) {


[not blocking]
I can see this naming scheme becoming confusing, since "reconstructing the square" is already a term we use to describe the process of collecting enough erasure encoded shares to rebuild the EDS. (for example, search the word reconstruct in celestia-node)

I understand using the word reconstruct here, but perhaps we could use a synonym to contruct? build and rebuild?

evan-forbes · 2023-04-26T15:44:32Z

pkg/square/builder.go

+		// Imagine if, according to the share commitment rules, a transcation took up 11 shares
+		// and had the merkle mountain tree commitment of 4,4,2,1. The first part of the share commitment


I also think this example is out dated with the implementation of ADR013

evan-forbes · 2023-04-26T15:46:50Z

pkg/square/builder.go

+		//
+		// Note that the padding would actually belong to the namespace of the transaction before it, but
+		// this makes no difference to the total share size.
+		maxPadding: shares.SubTreeWidth(shareSize) - 1,


shares.SubTreeWidth(shareSize) - 1 is correct, and good idea @rootulp having a function to make this explicit is a good idea.

created #1660

evan-forbes · 2023-04-26T16:07:42Z

pkg/square/builder.go

+	blob       core.Blob
+	pfbIndex   int
+	blobIndex  int
+	shareSize  int


[not blocking naming thing]
is it possible to use a different word besides shareSize that describes what this is? This is the number of shares used correct? the shares size makes me think of

celestia-app/pkg/appconsts/appconsts.go

Line 21 in f31ee58

ShareSize = 512

evan-forbes · 2023-04-26T16:16:05Z

pkg/square/builder.go

+	size := iw.Size()
+	pfbShareDiff := c.pfbCounter.Add(size)


nice, this is a significant improvement to

celestia-app/app/estimate_square_size.go

Lines 74 to 105 in f31ee58

// maxWrappedTxOverhead calculates the maximum amount of overhead introduced by

// wrapping a transaction with a shares index

//

// TODO: make more efficient by only generating these numbers once or something

// similar. This function alone can take up to 5ms.

func maxIndexWrapperOverhead(squareSize uint64) int {

maxTxLen := squareSize * squareSize * appconsts.ContinuationCompactShareContentSize

wtx, err := coretypes.MarshalIndexWrapper(

make([]byte, maxTxLen),

uint32(squareSize*squareSize),

)

if err != nil {

panic(err)

}

return len(wtx) - int(maxTxLen)

}

// maxIndexOverhead calculates the maximum amount of overhead in bytes that

// could occur by adding an index to an IndexWrapper.

func maxIndexOverhead(squareSize uint64) int {

maxShareIndex := squareSize * squareSize

maxIndexLen := binary.PutUvarint(make([]byte, binary.MaxVarintLen32), maxShareIndex)

wtx, err := coretypes.MarshalIndexWrapper(make([]byte, 1), uint32(maxShareIndex))

if err != nil {

panic(err)

}

wtx2, err := coretypes.MarshalIndexWrapper(make([]byte, 1), uint32(maxShareIndex), uint32(maxShareIndex-1))

if err != nil {

panic(err)

}

return len(wtx2) - len(wtx) + maxIndexLen

}

evan-forbes · 2023-04-26T16:22:23Z

I'm working on some changes locally for refactoring limiting the blocksize, and I this change pairs very well

Co-authored-by: Rootul P <rootulp@gmail.com>

cmwaters · 2023-04-26T19:24:08Z

Thought I'd post some relatively crude results comparing the square size estimation algorithm before and after (each PFB is 512 bytes)

pfbs 1 | square size before 4 | square size after 2
pfbs 2 | square size before 4 | square size after 4
pfbs 4 | square size before 8 | square size after 4
pfbs 8 | square size before 8 | square size after 8
pfbs 16 | square size before 16 | square size after 8
pfbs 32 | square size before 16 | square size after 16
pfbs 64 | square size before 32 | square size after 16
pfbs 128 | square size before 32 | square size after 32
pfbs 256 | square size before 64 | square size after 32
pfbs 512 | square size before 64 | square size after 64
pfbs 1024 | square size before 128 | square size after 64
pfbs 2048 | square size before 128 | square size after 128

cmwaters and others added 8 commits April 25, 2023 18:42

create compact share counter

9f3c6eb

fix build

7e819b3

Apply suggestions from code review

fb56f25

Co-authored-by: Rootul P <rootulp@gmail.com>

Update pkg/shares/counter.go

1297440

Co-authored-by: Rootul P <rootulp@gmail.com>

remove unnecessary panics

909804e

lint

3951c22

Merge branch 'main' into cal/square-1

0b51d44

feat: create square builder class

c18e0f0

cmwaters requested review from rootulp and evan-forbes as code owners April 26, 2023 14:09

MSevey requested a review from a team April 26, 2023 14:09

cmwaters commented Apr 26, 2023

View reviewed changes

Merge branch 'cal/square-1' into cal/square-2

4c3ae53

rootulp assigned cmwaters Apr 26, 2023

add missing test

00a6ab8

Base automatically changed from cal/square-1 to main April 26, 2023 15:06

Merge branch 'main' into cal/square-2

8bf97f5

rootulp reviewed Apr 26, 2023

View reviewed changes

rootulp added this to the Mainnet milestone Apr 26, 2023

evan-forbes mentioned this pull request Apr 26, 2023

Create a function that calculates the max padding for a blob #1660

Closed

evan-forbes previously approved these changes Apr 26, 2023

View reviewed changes

Apply suggestions from code review

a95a740

Co-authored-by: Rootul P <rootulp@gmail.com>

cmwaters dismissed evan-forbes’s stale review via a95a740 April 26, 2023 17:38

MSevey requested a review from a team April 26, 2023 17:38

cmwaters added 3 commits April 26, 2023 19:42

rename shareSize to numShares

413f13b

fixing up failed attempt

17afd0d

Merge branch 'main' into cal/square-2

0dd43ef

evan-forbes approved these changes Apr 27, 2023

View reviewed changes

evan-forbes requested a review from rootulp April 27, 2023 15:11

rootulp approved these changes Apr 27, 2023

View reviewed changes

cmwaters merged commit a4aec0d into main Apr 28, 2023
27 checks passed

cmwaters deleted the cal/square-2 branch April 28, 2023 08:09

cmwaters mentioned this pull request May 9, 2023

PoC: iterative and deterministic square construction #1301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create square builder class #1659

feat: create square builder class #1659

cmwaters commented Apr 26, 2023

cmwaters Apr 26, 2023

cmwaters Apr 26, 2023

rootulp Apr 26, 2023

cmwaters Apr 26, 2023

cmwaters Apr 26, 2023

evan-forbes Apr 26, 2023 •

edited

rootulp Apr 26, 2023

cmwaters Apr 26, 2023

cmwaters Apr 26, 2023

rootulp Apr 26, 2023

rootulp Apr 26, 2023

cmwaters Apr 26, 2023

evan-forbes Apr 26, 2023

cmwaters Apr 26, 2023

rootulp Apr 26, 2023

codecov-commenter commented Apr 26, 2023 •

edited

evan-forbes left a comment

evan-forbes Apr 26, 2023

evan-forbes Apr 26, 2023

evan-forbes Apr 26, 2023 •

edited

evan-forbes Apr 26, 2023

evan-forbes Apr 26, 2023

evan-forbes commented Apr 26, 2023

cmwaters commented Apr 26, 2023

		// Imagine if, according to the share commitment rules, a transcation took up 11 shares
		// and had the merkle mountain tree commitment of 4,4,2,1. The first part of the share commitment

	// maxWrappedTxOverhead calculates the maximum amount of overhead introduced by
	// wrapping a transaction with a shares index
	//
	// TODO: make more efficient by only generating these numbers once or something
	// similar. This function alone can take up to 5ms.
	func maxIndexWrapperOverhead(squareSize uint64) int {
	maxTxLen := squareSize * squareSize * appconsts.ContinuationCompactShareContentSize
	wtx, err := coretypes.MarshalIndexWrapper(
	make([]byte, maxTxLen),
	uint32(squareSize*squareSize),
	)
	if err != nil {
	panic(err)
	}
	return len(wtx) - int(maxTxLen)
	}

	// maxIndexOverhead calculates the maximum amount of overhead in bytes that
	// could occur by adding an index to an IndexWrapper.
	func maxIndexOverhead(squareSize uint64) int {
	maxShareIndex := squareSize * squareSize
	maxIndexLen := binary.PutUvarint(make([]byte, binary.MaxVarintLen32), maxShareIndex)
	wtx, err := coretypes.MarshalIndexWrapper(make([]byte, 1), uint32(maxShareIndex))
	if err != nil {
	panic(err)
	}
	wtx2, err := coretypes.MarshalIndexWrapper(make([]byte, 1), uint32(maxShareIndex), uint32(maxShareIndex-1))
	if err != nil {
	panic(err)
	}
	return len(wtx2) - len(wtx) + maxIndexLen
	}

feat: create square builder class #1659

feat: create square builder class #1659

Conversation

cmwaters commented Apr 26, 2023

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

evan-forbes Apr 26, 2023 • edited

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

codecov-commenter commented Apr 26, 2023 • edited

Codecov Report

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Apr 26, 2023

cmwaters commented Apr 26, 2023

evan-forbes Apr 26, 2023 •

edited

codecov-commenter commented Apr 26, 2023 •

edited

evan-forbes Apr 26, 2023 •

edited