chore: refactor share splitting and merging #637
Conversation
There was a problem hiding this comment.
[optional][proposal] with respect to naming consistency
Currently
parseMsgSharesinmerge_message_shares.goprocessContiguousSharesinmerge_contiguous_shares.go
It may help if similar behavior was named consistently. I think parse may be a better fit so proposal
parseMsgSharesinparse_message_shares.goparseContiguousSharesinparse_contiguous_shares.go
^ this doesn't necessarily need to be addressed in this PR
| type shareStack struct { | ||
| shares [][]byte | ||
| txLen uint64 | ||
| txs [][]byte |
There was a problem hiding this comment.
[optional][proposal] since this variable may also contain intermediate state roots or evidence
// shareStack hold variables for peel
type shareStack struct {
shares [][]byte
dataLen uint64
// data may be transactions, intermediate state roots, or evidence depending
// on the namespace ID for this share
data [][]byte
cursor int
}a side benefit is that the comment below is now more precise
// intermediate state root, or evidence) and adds it to the underlying slice of data.
pkg/shares/utils.go
Outdated
| func powerOf2(v uint64) bool { | ||
| if v&(v-1) == 0 && v != 0 { | ||
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
[optional][proposal]
func isPowerOf2(v uint64) bool {
return v&(v-1) == 0 && v != 0
}
|
this refactor is a herculean effort 💪 great work! @evan-forbes |
Co-authored-by: Rootul Patel <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #637 +/- ##
==========================================
+ Coverage 29.89% 35.87% +5.98%
==========================================
Files 16 24 +8
Lines 2054 2620 +566
==========================================
+ Hits 614 940 +326
- Misses 1380 1595 +215
- Partials 60 85 +25
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Great work! The typos are non-blocking but indicative that something is wrong with our misspell config, I'll investigate
UPDATE: it looks like misspell isn't as accurate as I would've hoped. It only flagged 3 issues on this branch and none are the ones I flagged so it wouldn't help reduce PR churn here.
$ gco evan/refactor-shares-pkg
$ misspell .
third_party/proto/tendermint/crypto/proof.proto:30:49: "nessecary" is a misspelling of "necessary"
third_party/proto/confio/proofs.proto:48:57: "collission" is a misspelling of "collisions"
x/payment/spec/docs.md:147:57: "contians" is a misspelling of "contains"
it's not clear to me why these 3 violations weren't flagged when misspell was run via golangci-lint run. It's possible the version of my misspell binary (latest) flags more issues than the version of misspell golangci-lint run uses
| // exactMsgShareSize is the length of message that will fit exactly into a single | ||
| // share, accounting for namespace id and the length delimiter prepended to | ||
| // each message | ||
| const exactMsgShareSize = consts.MsgShareSize - 2 |
There was a problem hiding this comment.
[no change needed] because this was present in the test prior to the refactor but since consts.MsgShareSize already accounts for the 8 byte namespace id, this assumes that the length delimiter is 2 bytes. Since length delimiter is a varint, isn't it possible that the delimiter is > 2 bytes?
There was a problem hiding this comment.
yeah! it could be more, but not for this test? tbh I'd need to go check if there are large enough messages
review feedback Co-authored-by: Rootul Patel <[email protected]>
| @@ -275,227 +274,6 @@ func TestFuzz_Merge(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func Test_processContiguousShares(t *testing.T) { | |||
There was a problem hiding this comment.
[for reviewers] this was moved to pkg/shares/contiguous_shares_test.go
|
merging now, even tho we're planning on moving this to its own repo as per this comment |
Description
This PR refactors splitting and merging. It incorporates the changes found in celestiaorg/celestia-core#820, please see the second commit that addresses the remaining feedback from that PR. The rest of the changes involves splitting up the existing code into different files and adding backwards compatibility with blocks that use and don't use wrapped txs for non-interactive defaults (pointed out below).
part of: #382
closes: #632