fix!: Share splitting bug #1111
Conversation
… using the old estimation algorithm
evan-forbes
added
bug
pkg/shares/share_splitting.go
Outdated
| // force msgIndexes to be the first share index | ||
| if len(blobIndexes) != 0 && useShareIndexes { | ||
| blobShareStart = int(blobIndexes[0]) | ||
| } |
There was a problem hiding this comment.
Are we able to specifically add a unit test for this?
There was a problem hiding this comment.
👍 yeah defo needed, thanks. fails on main
c679765
also #1110 (comment)
Co-authored-by: Rootul P <rootulp@gmail.com>
evan-forbes
dismissed stale reviews from rahulghangas and rootulp
via
7e70bdb
Codecov Report
@@ Coverage Diff @@
## main #1111 +/- ##
==========================================
+ Coverage 47.43% 48.30% +0.87%
==========================================
Files 71 71
Lines 3970 3993 +23
==========================================
+ Hits 1883 1929 +46
+ Misses 1915 1898 -17
+ Partials 172 166 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -62,6 +64,32 @@ func TestMerge(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // TestPadFirstIndexedBlob ensures that we are adding padding to the first share | |||
| // instead of calculating the value. | |||
There was a problem hiding this comment.
[non-blocking] sorry I don't understand what "calculating the value" means in this context. Do you mind elaborating? It doesn't strictly have to go into the comment b/c I don't want to block this PR's merge.
There was a problem hiding this comment.
calculate using the non-interactive defaults, previously this is what it was doing. The estimation process was accurate enough that this was the case the vast majority of the time, but not for all cases.
## Overview fixes the bug described in celestiaorg#1108 the modified tests fails on main. Also see celestiaorg#1110, which we should really get to as soon as we don't have more important things as this code is ancient closes celestiaorg#1108 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords Co-authored-by: Rootul P <rootulp@gmail.com>
Overview
fixes the bug described in #1108
the modified tests fails on main. Also see #1110, which we should really get to as soon as we don't have more important things as this code is ancient
closes #1108
Checklist