feat: new ParseShare API

[rootulp]

Closes #723
Opens #839

[codecov-commenter]

Codecov Report

Merging #828 (c184932) into main (6b86e91) will increase coverage by 2.10%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##             main     #828      +/-   ##
==========================================
+ Coverage   23.38%   25.49%   +2.10%     
==========================================
  Files          71       75       +4     
  Lines        8846     9261     +415     
==========================================
+ Hits         2069     2361     +292     
- Misses       6601     6679      +78     
- Partials      176      221      +45     

Impacted Files	Coverage Δ
pkg/shares/share_merging.go	74.78% <88.46%> (+3.99%)	⬆️
pkg/shares/shares.go	40.00% <100.00%> (+17.14%)	⬆️
testutil/testnode/node_interaction_api.go	60.22% <0.00%> (ø)
testutil/testnode/rpc_client.go	81.81% <0.00%> (ø)
testutil/testnode/full_node.go	75.00% <0.00%> (ø)
testutil/testnode/node_init.go	65.92% <0.00%> (ø)

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

[evan-forbes]

[question]
should we also be checking that the length delimiter matches the sequence length?

[evan-forbes]

[comment for other reviewers/posterity]
this isn't actually consensus breaking (see below comment) I don't think, even tho we weren't previously checking for this, because is order for this to be hit, the incorrect info byte has to be used which won't happen due to validators inserting it themselves and comparing the data root.

[evan-forbes]

on second thought, once we start using this, it could be consensus breaking with a dishonest majority. Honest full nodes will reject blocks that hit this error where we previously were not.

[rootulp]

once we start using this

I thought ParseShares was for celestia-node consumption. Do we expect the state machine to invoke this API? I didn't expect this API to be consensus breaking because I assumed the state machine would not call it.

[evan-forbes]

even if light clients are the only ones rejecting a block, then that is still consensus breaking (just like a change to fraud proofs)

[evan-forbes]

at least, I think its consensus breaking... 😅

imo, it makes sense to eventually use this code when we merge shares. It keeps things consistent and can possibly even inform other implementations on how to use universal shares.

[rootulp]

should we also be checking that the length delimiter matches the sequence length?

It's possible to add this check to the contents of ParseShares(). In order to do so, it must trim padding from the last share of the sequence to calculate the true total length. If this approach is recommended:

1. How do we differentiate padding bytes (0) from intentionally added zero bytes (i.e. a user included 3 trailing 0 bytes in their message)?
2. Should this API return an error if the true total data length doesn't match the total data length varint?

[evan-forbes]

In order to do so, it must trim padding from the last share of the sequence to calculate the true total length.

do we need to trim if we're only checking sequence length, not the exact length of bytes?

edit: like, we should use something similar to

celestia-app/pkg/shares/utils.go

Lines 18 to 29 in e69283f

	// MsgSharesUsed calculates the minimum number of shares a message will take up.
	// It accounts for the necessary delimiter and potential padding.
	func MsgSharesUsed(msgSize int) int {
	// add the delimiter to the message size
	msgSize = DelimLen(uint64(msgSize)) + msgSize
	shareCount := msgSize / appconsts.SparseShareContentSize
	// increment the share count if the message overflows the last counted share
	if msgSize%appconsts.SparseShareContentSize != 0 {
	shareCount++
	}
	return shareCount
	}

[rootulp]

do we need to trim if we're only checking sequence length, not the exact length of bytes?

We need to trim the last share of padding if we want to compare the total data length varint to the exact sequence length. We don't need to trim if we want this check to compare the total data length varint to an approximate sequence length (i.e. for message shares)

(len(shareSequence.Shares) - 1) * appconsts. SparseShareContentSize <= varint <= len(shareSequence.Shares) * appconsts. SparseShareContentSize

[evan-forbes]

left one remaining change request for the testing we're using.

While I still think we should check for the length of the sequence (len(shareSequence.Shares)), that would require a large enough change to the existing code that doing this in a different PR makes sense

[evan-forbes]

dope! thanks! preapprooovvviinnngggggg