feat: support parsing out of context compact shares #1770

Manav-Aggarwal · 2023-05-15T19:13:15Z

Overview

Adds logic to handle out of context compact shares and uses it in parseCompactShares. Also, adds a corresponding test with different random slices of list of shares to check they are interpretable back into Txs.

Closes: #802

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

codecov-commenter · 2023-05-15T19:43:43Z

Codecov Report

Merging #1770 (1139a4e) into main (5443e5f) will increase coverage by 0.20%.
The diff coverage is 53.65%.

@@            Coverage Diff             @@
##             main    #1770      +/-   ##
==========================================
+ Coverage   51.60%   51.81%   +0.20%     
==========================================
  Files         107      107              
  Lines        6647     6680      +33     
==========================================
+ Hits         3430     3461      +31     
+ Misses       2862     2860       -2     
- Partials      355      359       +4

Impacted Files	Coverage Δ
pkg/shares/shares.go	`69.00% <40.62%> (-6.54%)`	⬇️
pkg/shares/parse_compact_shares.go	`76.47% <100.00%> (+12.47%)`	⬆️

... and 2 files with indirect coverage changes

rootulp

Overall LGTM! Great work

test/util/testfactory/txs.go

rootulp · 2023-05-15T20:08:35Z

pkg/shares/compact_shares_test.go

@@ -112,6 +112,23 @@ func Test_processCompactShares(t *testing.T) {
 	}
 }

+func TestParseOutOfContextShares(t *testing.T) {


[optional] in addition to this test, should we consider adding a test case for https://github.com/rootulp/celestia-app/blob/01282c42a8b1d7ea2cfd325c2ed10aed4c317966/pkg/shares/parse.go#L12 that tests out of context shares?

Motivation: parseCompactShares is private and ParseTxs is public.

SinceParseTxs uses parseCompactShares under the hood so instead of adding another test, modified this test using ParseTxsto make it simpler.

Co-authored-by: Rootul P <[email protected]>

rootulp

🚢

evan-forbes

thanks for adding this @Manav-Aggarwal!!! 🙂

mostly just had a few questions. In future PRs, I think we might be able to test this a bit more thoroughly by incorporating it into some integration tests

pkg/shares/shares.go

test/util/testfactory/txs.go

pkg/shares/compact_shares_test.go

evan-forbes · 2023-05-18T21:27:33Z

ahh nvm, the fact that it was picked up in a square test threw me off, fortunately its not that bad, do we need to check that its a compact share
in https://github.com/Manav-Aggarwal/celestia-app/blob/8d6e812c504dc4fdfa2bb8cc0759592a42ea839f/pkg/shares/shares.go#L218-L224 ?

should we perform that check earlier somehow?

Manav-Aggarwal · 2023-05-18T21:47:13Z

ahh nvm, the fact that it was picked up in a square test threw me off, fortunately its that bad, do we need to check that its a compact share in https://github.com/Manav-Aggarwal/celestia-app/blob/8d6e812c504dc4fdfa2bb8cc0759592a42ea839f/pkg/shares/shares.go#L218-L224 ?

should we perform that check earlier somehow?

Removed the commit that introduced that check for now. I don't think we necessarily need to have that check since the assumption that ParseTxs is only being used with compactShares as input is incorrect. So we can keep the rawDataStartIndex method be more versatile and just process non-compact shares without using reserved bytes.

rootulp

Apologies for comments after already approving this. On re-review I have a few questions

pkg/shares/parse_compact_shares.go

pkg/shares/shares.go

rootulp

Discussed in-person. Thanks for adding the comments!

rootulp

Discussed in-person. Thanks for adding the comments!

## Overview Adds logic to handle out of context compact shares and uses it in `parseCompactShares`. Also, adds a corresponding test with different random slices of list of shares to check they are interpretable back into Txs. Closes: celestiaorg#802  ## 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 <[email protected]> Co-authored-by: Evan Forbes <[email protected]>

Adds ability to: - Convert `TxWithISRs` to Shares - Convert Shares to byte array (that will be posted on celestia) - Convert above byte array back to Shares - Convert Shares back to `TxWithISRs` - Parse Out of Context Shares Adds relevant roundtrip tests Resolves #881 Resolves #934 Resolves #886 Resolves #925 Note: All shares are written and interpreted as compact shares so they contain reserved bytes (see https://celestiaorg.github.io/celestia-app/specs/data_structures.html#compact-share) Related PR in `celestia-app`: celestiaorg/celestia-app#1770

Adds ability to: - Convert `TxWithISRs` to Shares - Convert Shares to byte array (that will be posted on celestia) - Convert above byte array back to Shares - Convert Shares back to `TxWithISRs` - Parse Out of Context Shares Adds relevant roundtrip tests Resolves rollkit#881 Resolves rollkit#934 Resolves rollkit#886 Resolves rollkit#925 Note: All shares are written and interpreted as compact shares so they contain reserved bytes (see https://celestiaorg.github.io/celestia-app/specs/data_structures.html#compact-share) Related PR in `celestia-app`: celestiaorg/celestia-app#1770

Adds ability to: - Convert `TxWithISRs` to Shares - Convert Shares to byte array (that will be posted on celestia) - Convert above byte array back to Shares - Convert Shares back to `TxWithISRs` - Parse Out of Context Shares Adds relevant roundtrip tests Resolves #881 Resolves #934 Resolves #886 Resolves #925 Note: All shares are written and interpreted as compact shares so they contain reserved bytes (see https://celestiaorg.github.io/celestia-app/specs/data_structures.html#compact-share) Related PR in `celestia-app`: celestiaorg/celestia-app#1770

Adds ability to: - Convert `TxWithISRs` to Shares - Convert Shares to byte array (that will be posted on celestia) - Convert above byte array back to Shares - Convert Shares back to `TxWithISRs` - Parse Out of Context Shares Adds relevant roundtrip tests Resolves rollkit#881 Resolves rollkit#934 Resolves rollkit#886 Resolves rollkit#925 Note: All shares are written and interpreted as compact shares so they contain reserved bytes (see https://celestiaorg.github.io/celestia-app/specs/data_structures.html#compact-share) Related PR in `celestia-app`: celestiaorg/celestia-app#1770

Manav-Aggarwal added 2 commits May 15, 2023 14:17

support out of context shares

b51976a

Add test for out of context shares

ab7152a

github-actions bot added the external label May 15, 2023

rootulp assigned Manav-Aggarwal May 15, 2023

rootulp added the share encoding label May 15, 2023

rootulp added this to the Post-mainnet milestone May 15, 2023

Manav-Aggarwal changed the title ~~Manav/support out of context shares~~ Support parsing out of context compact shares May 15, 2023

Merge branch 'main' into manav/support_out_of_context_shares

01282c4

Manav-Aggarwal marked this pull request as ready for review May 15, 2023 19:45

Manav-Aggarwal requested review from evan-forbes, rootulp and cmwaters as code owners May 15, 2023 19:45

rootulp previously approved these changes May 15, 2023

View reviewed changes

Manav-Aggarwal mentioned this pull request May 17, 2023

feat: Txs To Shares Roundtrip rollkit/rollkit#889

Merged

Add explicit return to getRandomSlice

ba2d14f

Co-authored-by: Rootul P <[email protected]>

Manav-Aggarwal dismissed rootulp’s stale review via ba2d14f May 17, 2023 04:57

MSevey requested a review from a team May 17, 2023 04:58

Manav-Aggarwal and others added 3 commits May 17, 2023 00:58

Update comment for checkSubArray

05042aa

Co-authored-by: Rootul P <[email protected]>

use ParseTxs instead of parseCompactShares

7073126

Merge branch 'main' into manav/support_out_of_context_shares

792841d

Manav-Aggarwal requested a review from rootulp May 17, 2023 05:05

rootulp previously approved these changes May 17, 2023

View reviewed changes

rootulp changed the title ~~Support parsing out of context compact shares~~ feat: support parsing out of context compact shares May 17, 2023

evan-forbes reviewed May 17, 2023

View reviewed changes

address comments

b802fa0

Manav-Aggarwal dismissed rootulp’s stale review via b802fa0 May 18, 2023 18:25

MSevey requested a review from a team May 18, 2023 18:27

Add test for out of context shares using share ranges

6f8697b

Manav-Aggarwal requested a review from rootulp May 18, 2023 20:13

run go linter

1a27120

Manav-Aggarwal dismissed evan-forbes’s stale review via 1a27120 May 18, 2023 21:43

Manav-Aggarwal force-pushed the manav/support_out_of_context_shares branch from 8d6e812 to 1a27120 Compare May 18, 2023 21:43

Merge branch 'main' into manav/support_out_of_context_shares

d91cee2

Manav-Aggarwal requested a review from evan-forbes May 18, 2023 21:49

refactor rawDataStartIndexUsingReserved

54048d0

evan-forbes removed the external label May 19, 2023

Merge branch 'main' into manav/support_out_of_context_shares

a97ecc5

evan-forbes previously approved these changes May 19, 2023

View reviewed changes

rootulp reviewed May 19, 2023

View reviewed changes

pkg/shares/parse_compact_shares.go Outdated Show resolved Hide resolved

pkg/shares/parse_compact_shares.go Show resolved Hide resolved

pkg/shares/shares.go Show resolved Hide resolved

add better comments

1139a4e

Manav-Aggarwal dismissed evan-forbes’s stale review via 1139a4e May 19, 2023 18:58

MSevey requested a review from a team May 19, 2023 18:58

rootulp approved these changes May 19, 2023

View reviewed changes

Manav-Aggarwal requested a review from evan-forbes May 19, 2023 19:00

Merge branch 'main' into manav/support_out_of_context_shares

b2d14ad

MSevey requested a review from a team May 22, 2023 19:35

evan-forbes enabled auto-merge (squash) May 22, 2023 19:36

evan-forbes approved these changes May 22, 2023

View reviewed changes

evan-forbes merged commit 243fb72 into celestiaorg:main May 22, 2023

Manav-Aggarwal deleted the manav/support_out_of_context_shares branch May 22, 2023 23:45

rootulp mentioned this pull request May 31, 2023

chore: ban naked returns #1865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support parsing out of context compact shares #1770

feat: support parsing out of context compact shares #1770

Manav-Aggarwal commented May 15, 2023 •

edited

Loading

codecov-commenter commented May 15, 2023 •

edited

Loading

rootulp left a comment

rootulp May 15, 2023

Manav-Aggarwal May 17, 2023

rootulp left a comment

evan-forbes left a comment

evan-forbes commented May 18, 2023 •

edited

Loading

Manav-Aggarwal commented May 18, 2023

rootulp left a comment

rootulp left a comment

rootulp left a comment

feat: support parsing out of context compact shares #1770

feat: support parsing out of context compact shares #1770

Conversation

Manav-Aggarwal commented May 15, 2023 • edited Loading

Overview

Checklist

codecov-commenter commented May 15, 2023 • edited Loading

Codecov Report

rootulp left a comment

Choose a reason for hiding this comment

rootulp May 15, 2023

Choose a reason for hiding this comment

Manav-Aggarwal May 17, 2023

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

evan-forbes commented May 18, 2023 • edited Loading

Manav-Aggarwal commented May 18, 2023

rootulp left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

Manav-Aggarwal commented May 15, 2023 •

edited

Loading

codecov-commenter commented May 15, 2023 •

edited

Loading

evan-forbes commented May 18, 2023 •

edited

Loading