Skip to content

Commit

Permalink
fix: propagate share version (#1054)
Browse files Browse the repository at this point in the history
Follow up to #1028 where
I didn't propagate the new struct field `ShareVersion` to all instances
of `coretypes.Blob`.

Unfortunately Go isn't exhaustive on specifying struct fields so I
wasn't made aware of this miss until working on an unrelated change.
Open to ideas on how to have higher confidence all instances have been
addressed. [The non_exhaustive
attribute](https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute)
makes me think Rust structs are exhaustive by default.
  • Loading branch information
rootulp committed Nov 25, 2022
1 parent 79d681a commit 29d47af
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 10 deletions.
2 changes: 2 additions & 0 deletions pkg/shares/parse_sparse_shares.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ func parseSparseShares(rawShares [][]byte, supportedShareVersions []uint8) ([]co
currentMsg = coretypes.Blob{
NamespaceID: nid,
Data: nextMsgChunk,
// TODO: add the share version to the blob
// https://github.com/celestiaorg/celestia-app/issues/1053
}
// the current share contains the entire msg so we save it and
// progress
Expand Down
5 changes: 3 additions & 2 deletions pkg/shares/shares_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,9 @@ func generateRandomlySizedBlobs(count, maxBlobSize int) []coretypes.Blob {
// generateRandomBlob returns a random blob of the given size (in bytes)
func generateRandomBlob(size int) coretypes.Blob {
blob := coretypes.Blob{
NamespaceID: tmrand.Bytes(appconsts.NamespaceSize),
Data: tmrand.Bytes(size),
NamespaceID: tmrand.Bytes(appconsts.NamespaceSize),
Data: tmrand.Bytes(size),
ShareVersion: appconsts.ShareVersionZero,
}
return blob
}
Expand Down
1 change: 1 addition & 0 deletions x/blob/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ var (
ErrEvidenceNamespace = sdkerrors.Register(ModuleName, 11120, "cannot use evidence namespace ID")
ErrEmptyShareCommitment = sdkerrors.Register(ModuleName, 11121, "empty share commitment")
ErrInvalidShareCommitments = sdkerrors.Register(ModuleName, 11122, "invalid share commitments: all relevant square sizes must be committed to")
ErrUnsupportedShareVersion = sdkerrors.Register(ModuleName, 11123, "unsupported share version")
)
18 changes: 16 additions & 2 deletions x/blob/types/wirepayforblob.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/cosmos/cosmos-sdk/types/tx/signing"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
"golang.org/x/exp/constraints"
"golang.org/x/exp/slices"
)

var _ sdk.Msg = &MsgWirePayForBlob{}
Expand All @@ -31,6 +32,10 @@ func NewWirePayForBlob(namespace []byte, blob []byte, shareVersion uint8) (*MsgW
)
}

if !slices.Contains(appconsts.SupportedShareVersions, shareVersion) {
return nil, ErrUnsupportedShareVersion
}

out := &MsgWirePayForBlob{
NamespaceId: namespace,
BlobSize: uint64(len(blob)),
Expand Down Expand Up @@ -98,6 +103,13 @@ func (msg *MsgWirePayForBlob) ValidateBasic() error {
)
}

if msg.ShareVersion > math.MaxUint8 {
return ErrUnsupportedShareVersion
}
if !slices.Contains(appconsts.SupportedShareVersions, uint8(msg.ShareVersion)) {
return ErrUnsupportedShareVersion
}

return msg.ValidateMessageShareCommitment()
}

Expand Down Expand Up @@ -191,6 +203,7 @@ func (msg *MsgWirePayForBlob) unsignedPayForBlob() (*MsgPayForBlob, error) {
BlobSize: msg.BlobSize,
ShareCommitment: commitment,
Signer: msg.Signer,
ShareVersion: msg.ShareVersion,
}
return &mpfb, nil
}
Expand All @@ -201,8 +214,9 @@ func (msg *MsgWirePayForBlob) unsignedPayForBlob() (*MsgPayForBlob, error) {
func ProcessWirePayForBlob(msg *MsgWirePayForBlob) (*tmproto.Blob, *MsgPayForBlob, []byte, error) {
// add the blob to the list of core blobs to be returned to celestia-core
coreMsg := tmproto.Blob{
NamespaceId: msg.GetNamespaceId(),
Data: msg.GetBlob(),
NamespaceId: msg.GetNamespaceId(),
Data: msg.GetBlob(),
ShareVersion: msg.GetShareVersion(),
}

// wrap the signed transaction data
Expand Down
43 changes: 37 additions & 6 deletions x/blob/types/wirepayfordata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ func TestWirePayForBlob_ValidateBasic(t *testing.T) {
badCommitMsg := validWirePayForBlob(t)
badCommitMsg.ShareCommitment.ShareCommitment = []byte{1, 2, 3, 4}

// wire PFB with unsupported share version
unsupportedShareVersionWirePFB := validWirePayForBlob(t)
unsupportedShareVersionWirePFB.ShareVersion = 5 // unsupported

tests := []test{
{
name: "valid msg",
Expand Down Expand Up @@ -80,6 +84,11 @@ func TestWirePayForBlob_ValidateBasic(t *testing.T) {
msg: tailPaddingMsg,
wantErr: ErrTailPaddingNamespace,
},
{
name: "unsupported share version",
msg: unsupportedShareVersionWirePFB,
wantErr: ErrUnsupportedShareVersion,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -139,17 +148,23 @@ func TestMsgMinSquareSize(t *testing.T) {

func TestProcessWirePayForBlob(t *testing.T) {
type test struct {
name string
namespace []byte
msg []byte
expectErr bool
modify func(*MsgWirePayForBlob) *MsgWirePayForBlob
name string
namespace []byte
msg []byte
expectErr bool
modify func(*MsgWirePayForBlob) *MsgWirePayForBlob
shareVersion uint8
}

dontModify := func(in *MsgWirePayForBlob) *MsgWirePayForBlob {
return in
}

overrideShareVersion := func(in *MsgWirePayForBlob) *MsgWirePayForBlob {
in.ShareVersion = 5 // unsupported share version
return in
}

kb := generateKeyring(t, "test")

signer := NewKeyringSigner(kb, "test", "chain-id")
Expand All @@ -173,10 +188,25 @@ func TestProcessWirePayForBlob(t *testing.T) {
msg: []byte{},
modify: dontModify,
},
{
name: "wire pay for blob with share version 0",
namespace: []byte{1, 1, 1, 1, 1, 1, 1, 2},
msg: []byte{},
shareVersion: 0,
modify: dontModify,
},
{
name: "wire pay for blob with unsupported share version",
namespace: []byte{1, 1, 1, 1, 1, 1, 1, 2},
msg: []byte{},
shareVersion: 0,
expectErr: true,
modify: overrideShareVersion,
},
}

for _, tt := range tests {
wpfb, err := NewWirePayForBlob(tt.namespace, tt.msg, appconsts.ShareVersionZero)
wpfb, err := NewWirePayForBlob(tt.namespace, tt.msg, tt.shareVersion)
require.NoError(t, err, tt.name)
err = wpfb.SignShareCommitment(signer)
assert.NoError(t, err)
Expand All @@ -196,5 +226,6 @@ func TestProcessWirePayForBlob(t *testing.T) {
assert.Equal(t, wpfb.NamespaceId, spfb.NamespaceId, tt.name)
assert.Equal(t, wpfb.ShareCommitment.ShareCommitment, spfb.ShareCommitment, tt.name)
assert.Equal(t, wpfb.ShareCommitment.Signature, sig, tt.name)
assert.Equal(t, wpfb.ShareVersion, spfb.ShareVersion, tt.name)
}
}

0 comments on commit 29d47af

Please sign in to comment.