-
Notifications
You must be signed in to change notification settings - Fork 323
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
feat: write data length to first compact share #781
Merged
Merged
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
f93266b
wip: write data length to first compact share
rootulp e9848f2
refactor: extract NumberOfBytesVarint
rootulp 6f879fe
fix: TestCompactShareWriter
rootulp a15cff4
rename: txLen to unitLen. improve docs
rootulp 1ce5bad
fix: TestCount
rootulp b287b3f
feat: introduce CompactStartShareContentSize
rootulp 55dd412
fix: all tests cases of TestTxSharePosition but one
rootulp 11027e9
fix: joinByteSlice impl
rootulp 4a97567
Merge branch 'rp/fix-proof-test' into rp/data-length-prefix
rootulp d88a0c1
bug: some tests fail b/c last byte of tx is not included in tx range
rootulp d9c507f
rename: ContinuationCompactShareContentSize
rootulp 14d8ccd
Merge branch 'main' into rp/data-length-prefix
rootulp bbcfd16
chore: more test cases for TestTxShareIndex
rootulp 1229433
discovered bug in compact share splitting
rootulp 753779d
reorder consts to use order in which they are encoded
rootulp 676cee1
remove duplicate unnecessary test
rootulp 1393238
implement failing test for SplitTxs
rootulp b6a9966
Merge branch 'main' into rp/data-length-prefix
rootulp ac1ac10
pass TestSplitTxs with one transaction
rootulp 92202a5
add test case for two transactions
rootulp e35f613
add test names
rootulp 3bc2389
replace hard-coded padding with bytes.Repeat
rootulp 181c118
replace hard-coded padding with bytes.Repeat for test case 2
rootulp 3be79dc
use bytes.Repeat for third test case
rootulp ea1f7ce
reserve byte should be non-zero
rootulp 28e4606
Merge branch 'main' into rp/data-length-prefix
rootulp 7e86c15
improve test name
rootulp 72713a3
implement failing fourth test case
rootulp d6c0c9e
fix: test case four
rootulp 9f44876
uncomment proof tests
rootulp 1452acf
remove unused helper, improve comments
rootulp 4f38d3e
use css.isEmpty()
rootulp 4fd8493
remove debug logging
rootulp 5b5d535
spin out txLen rename
rootulp 8361113
revert namespace rename
rootulp fca6cd5
chore: move `zeroPadIfNecessary` to utils (#811)
rootulp 537d091
Rename `InfoReservedByte` to `InfoByte` (#805)
rootulp c650400
test: transaction splitting (#813)
rootulp c02ef12
chore: rename txLen to unitLen (#814)
rootulp c952795
Merge branch 'main' into rp/data-length-prefix
rootulp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,11 +26,12 @@ func TestSplitTxs(t *testing.T) { | |
want: [][]uint8{ | ||
append([]uint8{ | ||
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, // namespace id | ||
0x1, // info byte | ||
0x0, // reserved byte | ||
0x1, // info byte | ||
0x2, 0x0, 0x0, 0x0, // 1 byte (unit) + 1 byte (unit length) = 2 bytes message length | ||
0x0, // BUG: reserved byte should be non-zero | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opens #817 |
||
0x1, // unit length of first transaction | ||
0xa, // data of first transaction | ||
}, bytes.Repeat([]byte{0}, 244)...), // padding | ||
}, bytes.Repeat([]byte{0}, 240)...), // padding | ||
}, | ||
}, | ||
{ | ||
|
@@ -39,25 +40,27 @@ func TestSplitTxs(t *testing.T) { | |
want: [][]uint8{ | ||
append([]uint8{ | ||
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, // namespace id | ||
0x1, // info byte | ||
0x0, // reserved byte | ||
0x1, // info byte | ||
0x4, 0x0, 0x0, 0x0, // 2 bytes (first transaction) + 2 bytes (second transaction) = 4 bytes message length | ||
0x0, // BUG: reserved byte should be non-zero | ||
0x1, // unit length of first transaction | ||
0xa, // data of first transaction | ||
0x1, // unit length of second transaction | ||
0xb, // data of second transaction | ||
}, bytes.Repeat([]byte{0}, 242)...), // padding | ||
}, bytes.Repeat([]byte{0}, 238)...), // padding | ||
}, | ||
}, | ||
{ | ||
name: "one large tx that spans two shares", | ||
txs: coretypes.Txs{bytes.Repeat([]byte{0xC}, 245)}, | ||
txs: coretypes.Txs{bytes.Repeat([]byte{0xC}, 241)}, | ||
want: [][]uint8{ | ||
append([]uint8{ | ||
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, // namespace id | ||
0x1, // info byte | ||
0x0, // BUG reserved byte should be non-zero see https://github.com/celestiaorg/celestia-app/issues/802 | ||
245, 1, // unit length of first transaction is 245 | ||
}, bytes.Repeat([]byte{0xc}, 244)...), // data of first transaction | ||
0x1, // info byte | ||
243, 1, 0, 0, // 241 (unit) + 2 (unit length) = 243 message length | ||
0x0, // BUG: reserved byte should be non-zero | ||
241, 1, // unit length of first transaction is 241 | ||
}, bytes.Repeat([]byte{0xc}, 240)...), // data of first transaction | ||
append([]uint8{ | ||
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, // namespace id | ||
0x0, // info byte | ||
|
@@ -68,22 +71,23 @@ func TestSplitTxs(t *testing.T) { | |
}, | ||
{ | ||
name: "one small tx then one large tx that spans two shares", | ||
txs: coretypes.Txs{coretypes.Tx{0xd}, bytes.Repeat([]byte{0xe}, 243)}, | ||
txs: coretypes.Txs{coretypes.Tx{0xd}, bytes.Repeat([]byte{0xe}, 241)}, | ||
want: [][]uint8{ | ||
append([]uint8{ | ||
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, // namespace id | ||
0x1, // info byte | ||
0x0, // BUG reserved byte should be non-zero see https://github.com/celestiaorg/celestia-app/issues/802 | ||
0x1, // info byte | ||
245, 1, 0, 0, // 2 bytes (first transaction) + 243 bytes (second transaction) = 245 bytes message length | ||
0x0, // BUG: reserved byte should be non-zero | ||
1, // unit length of first transaction | ||
0xd, // data of first transaction | ||
243, 1, // unit length of second transaction is 243 | ||
}, bytes.Repeat([]byte{0xe}, 242)...), // data of first transaction | ||
241, 1, // unit length of second transaction is 241 | ||
}, bytes.Repeat([]byte{0xe}, 238)...), // data of first transaction | ||
append([]uint8{ | ||
0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, // namespace id | ||
0x0, // info byte | ||
0x0, // reserved byte | ||
0xe, // continuation data of second transaction | ||
}, bytes.Repeat([]byte{0x0}, 245)...), // padding | ||
0x0, // info byte | ||
0x0, // reserved byte | ||
0xe, 0xe, 0xe, // continuation data of second transaction | ||
}, bytes.Repeat([]byte{0x0}, 243)...), // padding | ||
}, | ||
}, | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
[question]
does it no longer account for the namespace as well?
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.
this comment rename could've been extracted to a separate PR but the reason I removed namespace id from the comment is because
ContinuationCompactShareContentSize
already accounts for subtracting the namespace bytes from the share size.celestia-app/pkg/appconsts/appconsts.go
Lines 35 to 39 in 395e268
The
- 1
is an attempt at accounting for the tx length delimiter prepended to each tx.