All shares must contain a share version that belongs to a list of supported versions

[rootulp]

Problem

#659 introduced an info byte which contains a share version. Currently a block proposer could construct a share with a version that hasn't yet been defined (ex. share version 5). The block that includes this share would be considered valid even though it should be considered invalid.

Proposal

Introduce a new validity rule that ensures all shares contain a share version that belongs to a list of supported versions. Initially the list of supported versions should contain share version: 0 because

celestia-app/pkg/appconsts/appconsts.go

Line 26 in e69283f

ShareVersion = uint8(0)

[rootulp]

Upon further investigation, this validity rule doesn't seem practical in ProcessProposal because the share version is not included in block data so a validator has no way of knowing what share version to use for the shares that are split from the block data. The current implementation splits block data into shares and parses shares back into block data using the only valid share format (0). If we want to enforce a block validity rule for share versions, then we need a mechanism of persisting share versions into the block data.

Alternatively, if we don't want this to be a ProcessProposal validity rule, we can enforce this type of check when we parse the data square back into block data (e.g. here).

[rootulp]

then we need a mechanism of persisting share versions into the block data.

I think we should consider doing this. If all blocks write shares with one share version, then we can add this as a field to Data and verify it in ProcessProposal. It also seems necessary for when we increase share versions in the future. This needs further investigation if we want to support the scenario where one block can include shares with different share versions but I don't see a use case for that currently. I'd appreciate your input @evan-forbes

[evan-forbes]

what you've discussed here makes sense

as mentioned above already, before other validators vote, they check the data hash by regenerating it from the block data. If a block producer includes an unexpected version, then data hash will be different and the honest validators will reject that block. The only way to include this is with a colluding 2/3s.

This will not yet have an effect on light clients atm, even with a colluding 2/3s, but as you have mentioned it might in the future when supporting multiple versions.

then we need a mechanism of persisting share versions into the block data.

yes exactly, we have to add the ability to specify the version when creating a pfd, and keep track of that version in the celestia-core Message.

I think its worth mentioning that we don't actually need to add an extra check in ProcessProposal, as this should get covered in message inclusion checks. When creating a pfd share commitment, it will include the version that they wish to use. If a validator using an incorrect version, then the commitment will not be the same and we can rely on message inclusion fraud proofs.

[rootulp]

When creating a pfd share commitment, it will include the version that they wish to use. If a validator using an incorrect version, then the commitment will not be the same and we can rely on message inclusion fraud proofs.

This is an interesting benefit of allowing users to specify in their wire PFD which share version they intend to use. It also seems necessary if we'd like to support multiple share formats being included in the same block and would remove the need to store a block-wide share version in the block header.

[rootulp]

#936 adds the share version to MsgWirePayForBlob and as a required parameter when generating a share commitment. This means a user will generate the share commitment prior to publishing their transaction that contains a wire PFB. As @evan-forbes stated above, if a dishonest validator attempts to modify the share version for a blob, the share commitments won't match and a blob inclusion fraud proof may be created.

As a result, it no longer seems necessary to add a block validity rule that all shares must contain a share version that belongs to a list of supported share versions.