feat!: pack squares densely by implementing ADR013 #1604

evan-forbes · 2023-04-06T23:04:45Z

Overview

implements ADR013

closes #1572
part of #1538

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

evan-forbes

note that there's only one real logic change required here, the rest is just fixing tests

app/write_square_test.go

pkg/inclusion/paths.go

pkg/inclusion/paths_test.go

pkg/shares/non_interactive_defaults.go

codecov-commenter · 2023-04-06T23:20:15Z

Codecov Report

Merging #1604 (4dd34c9) into main (952edae) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1604      +/-   ##
==========================================
+ Coverage   49.05%   49.13%   +0.08%     
==========================================
  Files          85       85              
  Lines        4964     4976      +12     
==========================================
+ Hits         2435     2445      +10     
- Misses       2290     2292       +2     
  Partials      239      239

Impacted Files	Coverage Δ
app/write_square.go	`92.98% <100.00%> (ø)`
pkg/inclusion/paths.go	`100.00% <100.00%> (ø)`
pkg/shares/non_interactive_defaults.go	`100.00% <100.00%> (ø)`
pkg/shares/share_splitting.go	`61.73% <100.00%> (ø)`
x/blob/types/payforblob.go	`71.42% <100.00%> (-0.42%)`	⬇️

... and 1 file with indirect coverage changes

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

pkg/shares/non_interactive_defaults.go

rootulp · 2023-04-07T14:49:21Z

I'm still reviewing but @evan-forbes what are you thoughts on updating the specs (i.e. here) at the same time as merging the implementation?

evan-forbes · 2023-04-07T15:08:55Z

I'm still reviewing but @evan-forbes what are you thoughts on updating the specs (i.e. here) at the same time as merging the implementation?

that's a good point! I don't want to block the merging of this by codeowners of ADRs and docs, so will do in separate PRs, I added #1606 and #1607 as requirements to the EPIC though

rootulp

Overall mostly LGTM. One blocking comment. Defer to you if we want to update specs in this PR or after this PR.

pkg/appconsts/appconsts.go

pkg/inclusion/paths.go

pkg/inclusion/paths_test.go

rootulp · 2023-04-07T18:02:03Z

pkg/inclusion/paths_test.go

-			{instructions: []WalkInstruction{WalkRight, WalkRight, WalkRight, WalkRight, WalkRight, WalkLeft}, row: 63},
-		}},
+		{
+			"all paths for a basic 2x2", 2, 2, 2,


[for reviewers] the first few test cases in this file have square layouts like:

// | | | // |B|B|

// | | |B|B| // | | | | | // | | | | | // | | | | |

// | | | |B| // |B| | | | // | | | | | // | | | | |

pkg/inclusion/paths_test.go

pkg/shares/non_interactive_defaults_test.go

x/blob/types/payforblob.go

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

…-app into evan/dense-square

Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>

…-app into evan/dense-square

pkg/inclusion/paths.go

Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>

…lestia-app into evan/dense-square

cmwaters

This LGTM. Where do we update the ShareCommitment generation rules (do they need to be updated). Do we also need to modify the logic for proving blobs?

pkg/appconsts/appconsts.go

cmwaters · 2023-04-19T13:10:05Z

pkg/shares/non_interactive_defaults.go

+// SubTreeWidth determines the maximum number of leaves per subtree in the share
+// commitment over a given blob. The input should be the total number of shares


👍 Like this description

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

staheri14

Thanks for being so responsive and attentive to the comments 👍 LGTM!

pkg/shares/non_interactive_defaults_test.go

pkg/shares/non_interactive_defaults.go

rootulp · 2023-04-20T16:41:19Z

pkg/appconsts/appconsts.go

+	// share commitment. If a blob contains more shares than this number, than the height
+	// of the subtree roots will gradually increases to so that the amount remains within that limit.


Suggested change

// share commitment. If a blob contains more shares than this number, than the height

// of the subtree roots will gradually increases to so that the amount remains within that limit.

// share commitment. If a blob contains more shares than this number, then the height

// of the subtree roots will increase so that the number of subtree roots remains below the threshold.

feat!: pack squares densly by implementing ADR013

973f8a1

evan-forbes added the consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules label Apr 6, 2023

evan-forbes added this to the Mainnet milestone Apr 6, 2023

evan-forbes requested a review from rootulp as a code owner April 6, 2023 23:04

evan-forbes self-assigned this Apr 6, 2023

MSevey requested review from a team and staheri14 and removed request for a team April 6, 2023 23:04

evan-forbes requested review from mojtaba-esk, cmwaters and nashqueue April 6, 2023 23:05

fix: linter

9a93941

MSevey requested a review from a team April 6, 2023 23:07

evan-forbes commented Apr 6, 2023

View reviewed changes

app/write_square_test.go Show resolved Hide resolved

app/write_square_test.go Show resolved Hide resolved

pkg/inclusion/paths.go Show resolved Hide resolved

pkg/inclusion/paths_test.go Show resolved Hide resolved

pkg/shares/non_interactive_defaults.go Outdated Show resolved Hide resolved

refactor: rename to subtreewidth

0185f6b

evan-forbes changed the title ~~feat!: pack squares densly by implementing ADR013~~ feat!: pack squares densely by implementing ADR013 Apr 6, 2023

evan-forbes added 2 commits April 7, 2023 05:13

chore: better docs

3cc81ba

chore: docs

052f570

evan-forbes commented Apr 7, 2023

View reviewed changes

pkg/shares/non_interactive_defaults.go Outdated Show resolved Hide resolved

evan-forbes mentioned this pull request Apr 7, 2023

Update the non-interative default rules to reflect the changes from ADR013 #1606

Closed

rootulp reviewed Apr 7, 2023

View reviewed changes

chore: docs

3726499

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

MSevey requested a review from a team April 7, 2023 20:34

evan-forbes added 4 commits April 7, 2023 22:46

chore: incorporate feedback

6b0353f

Merge branch 'evan/dense-square' of github.com:celestiaorg/lazyledger…

06a8da1

…-app into evan/dense-square

chore: docs

ad22606

chore: linter

77ae1e3

evan-forbes and others added 4 commits April 18, 2023 17:10

fix: height -> depth

3615971

chore: add max in docs

c84b42a

Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>

chore: docs

81509f8

Merge branch 'evan/dense-square' of github.com:celestiaorg/lazyledger…

d4ead8d

…-app into evan/dense-square

staheri14 reviewed Apr 19, 2023

View reviewed changes

pkg/inclusion/paths.go Outdated Show resolved Hide resolved

chore: height -> depth

2ccb521

Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>

MSevey requested a review from a team April 19, 2023 11:59

evan-forbes mentioned this pull request Apr 19, 2023

Remove or unexport the DefaultMinSquareSize #1636

Closed

evan-forbes and others added 3 commits April 19, 2023 07:08

chore: linter

442ff7d

Merge branch 'evan/dense-square' of https://github.com/celestiaorg/ce…

c51feee

…lestia-app into evan/dense-square

Merge branch 'main' into evan/dense-square

68149a1

cmwaters previously approved these changes Apr 19, 2023

View reviewed changes

fix: use clearer description

4dd34c9

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

evan-forbes dismissed cmwaters’s stale review via 4dd34c9 April 19, 2023 14:03

MSevey requested a review from a team April 19, 2023 14:04

staheri14 previously approved these changes Apr 19, 2023

View reviewed changes

cmwaters reviewed Apr 20, 2023

View reviewed changes

pkg/shares/non_interactive_defaults_test.go Outdated Show resolved Hide resolved

pkg/shares/non_interactive_defaults.go Show resolved Hide resolved

evan-forbes added 2 commits April 20, 2023 11:14

chore: rename test

0b2802e

chore: testing changes

74c4b24

evan-forbes dismissed staheri14’s stale review via 74c4b24 April 20, 2023 16:16

MSevey requested a review from a team April 20, 2023 16:16

evan-forbes requested review from rootulp, cmwaters and staheri14 April 20, 2023 16:20

evan-forbes added 2 commits April 20, 2023 11:20

fix: test

b0dde1f

chore: actually remove invalid check

7ac3247

cmwaters approved these changes Apr 20, 2023

View reviewed changes

rootulp approved these changes Apr 20, 2023

View reviewed changes

evan-forbes merged commit 31ab512 into main Apr 20, 2023
27 checks passed

evan-forbes deleted the evan/dense-square branch April 20, 2023 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: pack squares densely by implementing ADR013 #1604

feat!: pack squares densely by implementing ADR013 #1604

evan-forbes commented Apr 6, 2023 •

edited

evan-forbes left a comment

codecov-commenter commented Apr 6, 2023 •

edited

rootulp commented Apr 7, 2023

evan-forbes commented Apr 7, 2023

rootulp left a comment

rootulp Apr 7, 2023

cmwaters left a comment

cmwaters Apr 19, 2023

staheri14 left a comment

rootulp Apr 20, 2023

		// SubTreeWidth determines the maximum number of leaves per subtree in the share
		// commitment over a given blob. The input should be the total number of shares

		// share commitment. If a blob contains more shares than this number, than the height
		// of the subtree roots will gradually increases to so that the amount remains within that limit.

feat!: pack squares densely by implementing ADR013 #1604

feat!: pack squares densely by implementing ADR013 #1604

Conversation

evan-forbes commented Apr 6, 2023 • edited

Overview

Checklist

evan-forbes left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 6, 2023 • edited

Codecov Report

rootulp commented Apr 7, 2023

evan-forbes commented Apr 7, 2023

rootulp left a comment

Choose a reason for hiding this comment

rootulp Apr 7, 2023

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

cmwaters Apr 19, 2023

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

rootulp Apr 20, 2023

Choose a reason for hiding this comment

evan-forbes commented Apr 6, 2023 •

edited

codecov-commenter commented Apr 6, 2023 •

edited