feat!: increase share size to 512 bytes #850

rootulp · 2022-10-07T20:22:13Z

Closes #825
Opens #883

codecov-commenter · 2022-10-09T05:52:40Z

Codecov Report

Merging #850 (faca891) into main (ded7346) will increase coverage by 0.14%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #850      +/-   ##
==========================================
+ Coverage   26.15%   26.30%   +0.14%     
==========================================
  Files          75       76       +1     
  Lines        9344     9368      +24     
==========================================
+ Hits         2444     2464      +20     
- Misses       6674     6676       +2     
- Partials      226      228       +2

Impacted Files	Coverage Δ
pkg/shares/split_compact_shares.go	`82.16% <80.95%> (+1.11%)`	⬆️
pkg/shares/reserved_bytes.go	`85.00% <85.00%> (ø)`
testutil/testnode/full_node.go	`75.29% <100.00%> (ø)`
pkg/shares/parse_compact_shares.go	`61.66% <0.00%> (-5.00%)`	⬇️
pkg/shares/share_merging.go	`77.10% <0.00%> (+0.60%)`	⬆️

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

rootulp · 2022-10-09T05:49:16Z

app/estimate_square_size_test.go

-		{"random small block square size 4", 0, 1, appconsts.SparseShareContentSize, 4},
+		{"random small block square size 2", 0, 1, appconsts.SparseShareContentSize, 2},
 		{"random small block square size 4", 0, 1, appconsts.SparseShareContentSize * 10, 4},
-		{"random small block w/ 10 normal txs square size 4", 10, 1, appconsts.SparseShareContentSize, 8},
+		{"random small block w/ 10 normal txs square size 4", 10, 1, appconsts.SparseShareContentSize, 4},


Since the share size increased, these test cases can fit in slightly smaller square sizes

rootulp · 2022-10-09T05:51:01Z

pkg/appconsts/appconsts.go

@@ -30,7 +30,7 @@ const (

 	// CompactShareReservedBytes is the number of bytes reserved for the location of
 	// the first unit (transaction, ISR, evidence) in a compact share.
-	CompactShareReservedBytes = 1
+	CompactShareReservedBytes = 2


needed because #711 (comment)

pkg/da/data_availability_header_test.go

rootulp · 2022-10-09T06:05:20Z

x/payment/types/wirepayfordata_test.go

-		{
-			name:    "wrong but valid square size",
-			msg:     badSquareSizeMsg,
-			wantErr: ErrInvalidShareCommit,
-		},


I'm not sure why this test fails now. Attempts to fix it result in a different error: ErrInvalidShareCommitments and not ErrInvalidShareCommit.

I don't think this test is still necessary because we test for ErrInvalidShareCommit here but curious for reviewers thoughts.

I think this is just because the after we increase the share size, it makes the commitment to the message the same for different sizes since it can now fit on one row. If we change the test data to create valid pfd with 4000 byte msg instead of 2000, this test passes

rach-id

Awesome stuff 🚀
Left some questions.

pkg/da/data_availability_header_test.go

pkg/shares/reserved_bytes.go

pkg/shares/share_splitting_test.go

fix: remove unnecssary conditional chore: introduce fillShare to differentiate from padShare

rach-id

Approved. Thanks a lot for discussion 🔥
Will defer to Evan to approve it as I'm still exploring this part of the codebase.

evan-forbes · 2022-10-10T07:40:26Z

will try to review this soon, and can help debug why TestFuzzPrepareProcessProposal is failing. Likely just a silly oversight in the non-interactive defaults code.

rootulp · 2022-10-10T15:34:46Z

It looks like tests on CI have failed for separate issues. Attempt number:

https://github.com/celestiaorg/celestia-app/actions/runs/3215621970/jobs/5256852009#step:6:153
FAIL: TestIntegrationTestSuite/TestMaxBlockSize (14.41s)
https://github.com/celestiaorg/celestia-app/actions/runs/3215621970/jobs/5258908752#step:6:147
FAIL: TestFuzzPrepareProcessProposal/randomized_inputs_to_Prepare_and_Process_Proposal#05
https://github.com/celestiaorg/celestia-app/actions/runs/3215621970/jobs/5267387987#step:6:153
FAIL: TestIntegrationTestSuite/TestMaxBlockSize (16.24s)

Passed on fourth attempt so this PR isn't blocked on #574 but we should do it soon anyway

evan-forbes · 2022-10-10T21:58:41Z

this is dooooope, can we keep this as a draft until we plan to merge? I will review as if its not a draft tho

evan-forbes

would prefer to try to fix the test as described in the comment, but tbh, I think its fine as well.

this LGTM, I think we're ready to merge when we need to. Have we tried this out with celestia-node yet? perhaps we could just upgrade arabica to v0.8.0 since its basically ready

edit: the tests for celestia-node worked locally after creating an updated branch w/ this change

evan-forbes

8MB blocks, nbd

Closes celestiaorg#825 This is ready for review but we may not merge it b/c we haven't made a final decision on what the share size should be Co-authored-by: evan-forbes <evan.samuel.forbes@gmail.com>

Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for #850

Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for celestiaorg/celestia-app#850

rootulp added the C: Celestia app label Oct 7, 2022

rootulp self-assigned this Oct 7, 2022

rootulp changed the title ~~feat: define reserved_bytes.go~~ feat!: increase share size to 512 bytes Oct 7, 2022

rootulp force-pushed the rp/increase-share-size branch from a09f03c to 14109e3 Compare October 9, 2022 05:21

feat!: increase share size to 512 bytes

8ea9c82

rootulp force-pushed the rp/increase-share-size branch from 14109e3 to 8ea9c82 Compare October 9, 2022 05:47

rootulp commented Oct 9, 2022

View reviewed changes

rootulp marked this pull request as ready for review October 9, 2022 06:06

rootulp requested a review from evan-forbes as a code owner October 9, 2022 06:06

rach-id reviewed Oct 9, 2022

View reviewed changes

rootulp added 2 commits October 9, 2022 11:52

address @sweexordious feedback

b2ce542

fix: remove unnecssary conditional chore: introduce fillShare to differentiate from padShare

chore: remove named return values

8a6a66b

rach-id reviewed Oct 9, 2022

View reviewed changes

rootulp marked this pull request as draft October 10, 2022 22:04

evan-forbes reviewed Oct 17, 2022

View reviewed changes

Merge branch 'main' into rp/increase-share-size

31efe9c

rootulp marked this pull request as ready for review October 17, 2022 23:57

evan-forbes added 2 commits October 17, 2022 19:29

fix: increase test times

277062b

fix: try test query again if result is nil

faca891

evan-forbes approved these changes Oct 18, 2022

View reviewed changes

evan-forbes merged commit ecc9423 into celestiaorg:main Oct 18, 2022

rootulp mentioned this pull request Oct 18, 2022

Increase share size to 512 bytes #711

Closed

rootulp deleted the rp/increase-share-size branch October 18, 2022 01:26

rootulp mentioned this pull request Oct 18, 2022

specs: increase number of reserved bytes to two #883

Closed

rootulp mentioned this pull request Oct 31, 2022

Potentially increase share size #732

Closed

rootulp added a commit that referenced this pull request Dec 12, 2022

docs: update FirstCompactShareSequenceLengthBytes

4572b86

Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for #850

rootulp mentioned this pull request Dec 12, 2022

docs: update FirstCompactShareSequenceLengthBytes #1107

Merged

rootulp added a commit that referenced this pull request Dec 13, 2022

docs: update FirstCompactShareSequenceLengthBytes (#1107)

00e31f8

Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for #850

evan-forbes added this to the Incentivized Testnet milestone Jan 3, 2023

0xchainlover pushed a commit to celestia-org/celestia-app that referenced this pull request Aug 1, 2024

docs: update FirstCompactShareSequenceLengthBytes (#1107)

3f6f237

Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for celestiaorg/celestia-app#850

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: increase share size to 512 bytes #850

feat!: increase share size to 512 bytes #850

rootulp commented Oct 7, 2022 •

edited

Loading

codecov-commenter commented Oct 9, 2022 •

edited

Loading

rootulp Oct 9, 2022

rootulp Oct 9, 2022

rootulp Oct 9, 2022

evan-forbes Oct 17, 2022

rach-id left a comment

rach-id left a comment

evan-forbes commented Oct 10, 2022

rootulp commented Oct 10, 2022 •

edited

Loading

evan-forbes commented Oct 10, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes left a comment

feat!: increase share size to 512 bytes #850

feat!: increase share size to 512 bytes #850

Conversation

rootulp commented Oct 7, 2022 • edited Loading

codecov-commenter commented Oct 9, 2022 • edited Loading

Codecov Report

rootulp Oct 9, 2022

Choose a reason for hiding this comment

rootulp Oct 9, 2022

Choose a reason for hiding this comment

rootulp Oct 9, 2022

Choose a reason for hiding this comment

evan-forbes Oct 17, 2022

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

evan-forbes commented Oct 10, 2022

rootulp commented Oct 10, 2022 • edited Loading

evan-forbes commented Oct 10, 2022 • edited Loading

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

rootulp commented Oct 7, 2022 •

edited

Loading

codecov-commenter commented Oct 9, 2022 •

edited

Loading

rootulp commented Oct 10, 2022 •

edited

Loading

evan-forbes commented Oct 10, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading