feat!: differentiate namespaced padded shares

[rootulp]

Implements the proposal in #783 (comment).

Closes #1136
Closes #783
Opens #1344

[codecov-commenter]

Codecov Report

Merging #1341 (1cfab69) into main (0b447ad) will increase coverage by 0.03%.
The diff coverage is 59.09%.

@@            Coverage Diff             @@
##             main    celestiaorg/celestia-app#1341      +/-   ##
==========================================
+ Coverage   48.21%   48.24%   +0.03%     
==========================================
  Files          79       79              
  Lines        4405     4425      +20     
==========================================
+ Hits         2124     2135      +11     
- Misses       2103     2109       +6     
- Partials      178      181       +3     

Impacted Files	Coverage Δ
pkg/shares/parse_sparse_shares.go	64.40% <50.00%> (-1.64%)	⬇️
pkg/shares/namespaced_padding.go	74.19% <62.50%> (-14.05%)	⬇️

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

[cmwaters]

LGTM mostly a few nits.

I wonder why both sparse shares and compact shares are the same version. To me, the versioning indicates a different way of interpreting an array of 512 bytes

[evan-forbes]

thanks for doing this so fast!!

doesn't have to be in this PR, but we should keep a reminder somewhere to update the spec if we don't have one already.

edit: shoot, sorry should have read the intro comment 🤦

[rootulp]

I wonder why both sparse shares and compact shares are the same version. To me, the versioning indicates a different way of interpreting an array of 512 bytes

They don't strictly need to be the same version but at the moment, it's possible to differentiate compact shares and sparse shares via the share's namespace ID so they don't need to be separate versions. I think it's a little clearer to not version them separately because consider the following scenario:

Time t=0

• share version 0 is for compact shares
• share version 1 is for sparse shares

Time t=1

We want to make a breaking change to compact shares so we introduce

• share version 2 is for compact shares
• share version 1 is for sparse shares

Time t=2

We want to introduce a breaking change for sparse shares so

• share version 2 is for compact shares
• share version 3 is for sparse shares

Notice how share versions are flipped back and forth so it's not immediately clear which share version is applicable to which share type. If we version them together:

Time t=0

• share version 0 is for compact shares
• share version 0 is for sparse shares

Time t=1

We want to make a breaking change to compact shares so we introduce

• share version 1 is for compact shares with new format
• share version 1 is for sparse shares

Time t=2

We want to introduce a breaking change for sparse shares so

• share version 2 is for compact shares with new format
• share version 2 is for sparse shares with new format

I'm open to versioning them separately if there are benefits I'm not accounting for.

[cmwaters]

This is true but you're also viewing it from an angle where there are only ever two main types of share encoding/decoding (compact and sparse). These terms themselves are only named relative to themselves. Say we introduce an ExtraCompactShare where some transactions use that and others use CompactShares.

Using the namespace to dictate which protocol we use to interpret the bytes puts us in a bind. If we wanted to change so that certain transactions use SparseShares and perhaps that certain Blobs could use a more compact encoding then we wouldn't be able to do this without making it quite messy.

You could also set constants like CurrentCompactShareVersion = 7 or CurrentSparseShareVersion = 8

Btw, I'm not implying that we have to do all this, I'm just exploring out loud the design space.

[rootulp]

Btw, I'm not implying that we have to do all this, I'm just exploring out loud the design space.

Agreed and thanks for articulating the use case with ExtraCompactShare, I can now see it being valuable. Proposal to continue the discussion in celestiaorg/go-square#60 because the discussion doesn't block this PR.

[rach-id]

LGTM, left a question to understand more. Would defer approval to other reviewers.

[evan-forbes]

LGTM