Add `InfoReservedByte` to compact and sparse shares #691

rootulp · 2022-09-08T03:23:30Z

Implementation for #659

Note this may be easier for reviewers to review after #718 is completed. In the interim #660 may be helpful.

@sweexordious

…rg#710) * feat: move share consts from celestia-core * feat: new consts for universal share encoding * remove universal share constants * address @sweexordious feedback

…elestiaorg#710)" This reverts commit 75e2b9d.

Makes `TestParsePaddedMsg` pass

codecov-commenter · 2022-09-16T00:47:34Z

Codecov Report

Merging #691 (b067e48) into main (566b3d4) will increase coverage by 0.16%.
The diff coverage is 63.79%.

@@            Coverage Diff             @@
##             main     #691      +/-   ##
==========================================
+ Coverage   46.22%   46.38%   +0.16%     
==========================================
  Files          33       33              
  Lines        3217     3251      +34     
==========================================
+ Hits         1487     1508      +21     
- Misses       1619     1625       +6     
- Partials      111      118       +7

Impacted Files	Coverage Δ
pkg/shares/parse_sparse_shares.go	`83.52% <54.54%> (-5.08%)`	⬇️
pkg/shares/split_compact_shares.go	`83.21% <60.00%> (-0.79%)`	⬇️
pkg/shares/split_sparse_shares.go	`66.66% <62.50%> (-2.85%)`	⬇️
pkg/shares/parse_compact_shares.go	`82.35% <75.00%> (+4.57%)`	⬆️
pkg/shares/share_splitting.go	`59.75% <100.00%> (-0.49%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

evan-forbes

nice work! I need to take a second deeper look, but at first glance this looks like it's as minimal addition as possible, which is impressive given you had to grok all the encoding code lol

evan-forbes

[blocking question]
I didn't see anywhere that we were throwing an error if the first share was not the start of the message. If that's missing, then I think we should add that.

[comment on future work]
This does an excellent job of adding the info byte, but we have yet to have a universal share API. celestia-node should be able to call a function on any set of shares and get a message out.

As discussed sync and in other issues (I think?), this might entail a larger refactor of share encoding and decoding in general. Things such as changing the name of Messages to data, maybe refactoring the encoding logic to better fit the new API, and then refactoring the Merge/Split functions to use that new API. I think we still have to write a few issues for this, and should definitely be done in different PRs. I don't think we can close existing Universal share encoding issues until that is completed, and we should update the ADR to reflect the changes to the API there. Perhaps this was just confusion from the name of this PR?

evan-forbes · 2022-09-16T17:14:33Z

pkg/shares/compact_shares_test.go

+func TestCompactShareContainsInfoByte(t *testing.T) {
+	css := NewCompactShareSplitter(appconsts.TxNamespaceID, appconsts.ShareVersion)
+	txs := generateRandomCompactShares(1, 100)
+
+	for _, tx := range txs {
+		css.WriteTx(tx)
+	}
+
+	shares := css.Export().RawShares()
+	assert.Condition(t, func() bool { return len(shares) == 1 })
+
+	infoByte := shares[0][appconsts.NamespaceSize : appconsts.NamespaceSize+appconsts.ShareInfoBytes][0]
+
+	isMessageStart := true
+	want, err := NewInfoReservedByte(appconsts.ShareVersion, isMessageStart)
+
+	require.NoError(t, err)
+	assert.Equal(t, byte(want), infoByte)
+}
+
+func TestContiguousCompactShareContainsInfoByte(t *testing.T) {
+	css := NewCompactShareSplitter(appconsts.TxNamespaceID, appconsts.ShareVersion)
+	txs := generateRandomCompactShares(1, 1000)
+
+	for _, tx := range txs {
+		css.WriteTx(tx)
+	}
+
+	shares := css.Export().RawShares()
+	assert.Condition(t, func() bool { return len(shares) > 1 })
+
+	infoByte := shares[1][appconsts.NamespaceSize : appconsts.NamespaceSize+appconsts.ShareInfoBytes][0]
+
+	isMessageStart := false
+	want, err := NewInfoReservedByte(appconsts.ShareVersion, isMessageStart)
+
+	require.NoError(t, err)
+	assert.Equal(t, byte(want), infoByte)
+}


[optional, prefer different PR due to timing]
I think this should be done in a different PR (if at all) since we want to optimize to merge into #692 asap

but we might refactor these tests into a table drive test that tests evidence and the rest of the shares that are exported. another super minor [nit], would be to use a formula to calculate the size of the random compact shares so that we can incorporate appconsts. (we don't do this other places, but should so that can probably be a separate PR too)

can you please elaborate on

would be to use a formula to calculate the size of the random compact shares so that we can incorporate appconsts

yeah sure,

instead of just using the number 100 or 1000, we could say appconsts.TxShareSize / 2 or appconsts.TxShareSize * 4

evan-forbes · 2022-09-16T17:27:32Z

this does break #692, and I consider it to be blocking, since that PR should handle adding the info byte. We might also want to consider changing the estimation API and fixing tx inclusion before we merge that too

rootulp · 2022-09-16T18:15:14Z

I didn't see anywhere that we were throwing an error if the first share was not the start of the message. If that's missing, then I think we should add that.

Is this question specifically for parse_sparse_shares.go? If so, good call! Done in 5c4fef7

There is some ambiguity on when the start indicator should be 1 for compact shares, see #660 (comment)

Perhaps this was just confusion from the name of this PR?

Agreed. Renamed PR to clarify.

celestia-node should be able to call a function on any set of shares and get a message out.

Clarifying question: celestia-node should be able to call a function on any set of shares and get data out (i.e. transactions if the shares provided are transaction shares), right?

I think celestia-node is relying on celestia-core's share parsing logic (see here). So I think upgrading them to a release of celestia-app with the existing (refactored) share logic is a prerequisite to converting them to using a new API exposed by celestia-app. So we likely need to cut a celestia-app release ASAP and tackle that separately.

Created #723 for new API.

this does break #692, and I consider it to be blocking, since that PR should handle adding the info byte

Sorry I don't quite understand. For clarity: this PR is blocking #692 so you want to expedite merging this PR - correct?

evan-forbes · 2022-09-16T19:19:29Z

Is this question specifically for parse_sparse_shares.go? If so, good call! Done in 5c4fef7

also for compact shares, as I think that makes sense, wdyt?

Clarifying question: celestia-node should be able to call a function on any set of shares and get data out (i.e. transactions if the shares provided are transaction shares), right?

yeah

So I think upgrading them to a release of celestia-app with the existing (refactored) share logic is a prerequisite to converting them to using a new API exposed by celestia-app.

not sure I follow entirely, as what we do in #723 will change all of this? The existing logic being this PR and the naming changes? Is this just to split up the changes necessary?

edit: agree that we should get something out asap, but also want to limit the number of changes they need to make

rootulp · 2022-09-16T19:34:09Z

also for compact shares, as I think that makes sense, wdyt?

Makes sense, thanks for clarifying that only the first share of a reserved namespace should have the start indicator = 1. Addressed in b067e48

Is this just to split up the changes necessary?

Yep exactly. I'm proposing we split this process into multiple steps to reduce the size of PRs which may make it easier to identify regressions:

Cut a release of celestia-app that includes the refactored share logic
Upgrade celestia-node to ^ this version. Replace core on this line with an import from celestia-app (defined here)
Add ParseShare API in celestia-app
Cut a new release of celestia-app
Upgrade celestia-node to ^ this version

but also want to limit the number of changes they need to make

Agreed. Maybe we make the PRs to celestia-node because we're issuing the breaking changes and should know how to use the old / new APIs.

…geStart=true

evan-forbes · 2022-09-16T19:41:02Z

agree that we need to put more thought into which releases we cut, as we also need to account for non-interactive defaults (which will could effect the plugin and definitely their future planning), qgb state machine (this will change how to initalize a network and therefore their tests) and the other breaking changes we have. We can isolate the encoding changes to their own release, tho yeah. lets discuss this sync outside of this PR

evan-forbes

we still have a lot things going on with universal shares, but this is ready and isolated. Great work! happy someone else had to grok the encoding logic lololol

rootulp added 8 commits August 31, 2022 15:50

Add ParseInfoReservedByte

0ea94fe

Update split contiguous shares to account for info byte

29ab27e

Merge branch 'main' into rp/universal-share-encoding

243aa90

Fix TestContigShareWriter test

fd62420

Improve names in parseDataLength

522d6c2

Remove unused function parseDataLength

1592824

Merge branch 'main' into rp/universal-share-encoding

6705967

revert local celestia-core replace

88fbe5f

rootulp self-assigned this Sep 8, 2022

This was referenced Sep 8, 2022

EPIC: Universal share prefix #659

Closed

Get rid of unused tests #693

Closed

rootulp added 8 commits September 13, 2022 18:38

docs: improve docs

89ce0f6

docs: fix doc comments

c77951a

feat: move share consts from celestia-core

23ce09c

feat: new consts for universal share encoding

f616dee

Merge branch 'main' into rp/universal-share-constants

2936923

remove universal share constants

e2d3858

Merge branch 'rp/rename-constants' into rp/universal-share-encoding

70dbf7c

fix: pass Test_parseSparseShares

73dc2f5

rootulp changed the title ~~Implement universal share encoding for compact shares~~ Implement universal share encoding Sep 15, 2022

rootulp added 9 commits September 15, 2022 14:17

Rename appconsts to use Compact and Sparse terminology (celestiao…

75e2b9d

…rg#710) * feat: move share consts from celestia-core * feat: new consts for universal share encoding * remove universal share constants * address @sweexordious feedback

Revert "Rename appconsts to use Compact and Sparse terminology (c…

0facaf6

…elestiaorg#710)" This reverts commit 75e2b9d.

Merge branch 'main' into rp/universal-share-encoding

9e2e504

revert change to exactMsgShareSize

027878c

fix: add info byte to namespace padded shares

efd1b8f

Makes `TestParsePaddedMsg` pass

pass first two test cases of TestMerge

feee270

pass: TestMerge

8f2d0f0

pass: TestTxInclusion by updating proof to use share pkg

f87d4ec

pass: TestTxSharePosition

ae55527

rootulp force-pushed the rp/universal-share-encoding branch from 2312263 to ae55527 Compare September 15, 2022 20:15

pass: TestCreateCommitment by updating expected bytes

ae2fc21

rootulp added 2 commits September 15, 2022 21:05

extract doc comments to separate PR

e11055e

extract separate PR for specifying square size in shares_test.go

175469e

This was referenced Sep 16, 2022

docs: improve doc comments #716

Merged

chore: define OriginalSquareSize inside generateRandomBlockData #717

Merged

rootulp added 4 commits September 15, 2022 22:04

test: add TestMsgShareContainsInfoByte

db0351f

test: add TestContiguousMsgShareContainsInfoByte

159ecc7

test: add compact share tests

2c6a761

Merge branch 'main' into rp/universal-share-encoding

0650502

rootulp requested a review from evan-forbes September 16, 2022 02:55

rootulp marked this pull request as ready for review September 16, 2022 02:56

evan-forbes reviewed Sep 16, 2022

View reviewed changes

rootulp mentioned this pull request Sep 16, 2022

ADR-007: Universal Share Prefix #660

Merged

rootulp changed the title ~~Implement universal share encoding~~ Add InfoReservedByte to compact and sparse shares Sep 16, 2022

rootulp mentioned this pull request Sep 16, 2022

feat!: Refactor PrepareProposal to produce blocks using the non-interactive defaults #692

Merged

rootulp added 2 commits September 16, 2022 14:21

throw error if isMessageStart != isNewMessage

5c4fef7

Merge branch 'main' into rp/universal-share-encoding

297dd00

throw error if compact share does not start with a share with isMessa…

b067e48

…geStart=true

evan-forbes approved these changes Sep 16, 2022

View reviewed changes

rootulp merged commit b98f9ab into celestiaorg:main Sep 16, 2022

rootulp deleted the rp/universal-share-encoding branch September 16, 2022 20:40

rootulp mentioned this pull request Sep 21, 2022

Use appconsts in tests instead of 100 or 1000 #755

Closed

evan-forbes mentioned this pull request Sep 21, 2022

Incentivized Testnet Tracking Issue #514

Closed

41 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `InfoReservedByte` to compact and sparse shares #691

Add `InfoReservedByte` to compact and sparse shares #691

rootulp commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 16, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes Sep 16, 2022

rootulp Sep 16, 2022

evan-forbes Sep 16, 2022

evan-forbes commented Sep 16, 2022

rootulp commented Sep 16, 2022 •

edited

Loading

evan-forbes commented Sep 16, 2022 •

edited

Loading

rootulp commented Sep 16, 2022 •

edited

Loading

evan-forbes commented Sep 16, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading

Add InfoReservedByte to compact and sparse shares #691

Add InfoReservedByte to compact and sparse shares #691

Conversation

rootulp commented Sep 8, 2022 • edited Loading

codecov-commenter commented Sep 16, 2022 • edited Loading

Codecov Report

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

evan-forbes Sep 16, 2022

Choose a reason for hiding this comment

rootulp Sep 16, 2022

Choose a reason for hiding this comment

evan-forbes Sep 16, 2022

Choose a reason for hiding this comment

evan-forbes commented Sep 16, 2022

rootulp commented Sep 16, 2022 • edited Loading

evan-forbes commented Sep 16, 2022 • edited Loading

rootulp commented Sep 16, 2022 • edited Loading

evan-forbes commented Sep 16, 2022 • edited Loading

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

Add `InfoReservedByte` to compact and sparse shares #691

Add `InfoReservedByte` to compact and sparse shares #691

rootulp commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 16, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading

evan-forbes left a comment •

edited

Loading

rootulp commented Sep 16, 2022 •

edited

Loading

evan-forbes commented Sep 16, 2022 •

edited

Loading

rootulp commented Sep 16, 2022 •

edited

Loading

evan-forbes commented Sep 16, 2022 •

edited

Loading

evan-forbes left a comment •

edited

Loading