feat!: increase share size to 512 bytes #850
Conversation
rootulp
changed the title
rootulp
force-pushed
the
rp/increase-share-size
branch
from
a09f03c to
14109e3
Compare
rootulp
force-pushed
the
rp/increase-share-size
branch
from
14109e3 to
8ea9c82
Compare
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| {"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}, |
There was a problem hiding this comment.
Since the share size increased, these test cases can fit in slightly smaller square sizes
| @@ -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 | |||
There was a problem hiding this comment.
needed because #711 (comment)
| { | ||
| name: "wrong but valid square size", | ||
| msg: badSquareSizeMsg, | ||
| wantErr: ErrInvalidShareCommit, | ||
| }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
fix: remove unnecssary conditional chore: introduce fillShare to differentiate from padShare
|
will try to review this soon, and can help debug why |
|
It looks like tests on CI have failed for separate issues. Attempt number:
Passed on fourth attempt so this PR isn't blocked on #574 but we should do it soon anyway |
|
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 |
There was a problem hiding this comment.
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
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 <[email protected]>
Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for #850
Update godoc comment for `FirstCompactShareSequenceLengthBytes` to account for #850
Closes #825
Opens #883