-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix!: Share splitting bug #1111
Conversation
… using the old estimation algorithm
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we able to specifically add a unit test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yeah defo needed, thanks. fails on main
c679765
also #1110 (comment)
Co-authored-by: Rootul P <rootulp@gmail.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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