Skip to content

Commit

Permalink
ibc: packet fixes (#7810)
Browse files Browse the repository at this point in the history 
* ibc: msg validation fields

* msg tests

* rename proof

* rename proofs

* check seq ≠ 0

Co-authored-by: Aditya <adityasripal@gmail.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
  • Loading branch information
@fedekunze @AdityaSripal @colin-axner
3 people authored Nov 13, 2020
1 parent 6c9a02b commit e77c950
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 144 deletions.
8 changes: 4 additions & 4 deletions proto/ibc/core/channel/v1/tx.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ message MsgRecvPacket {
option (gogoproto.goproto_getters) = false;

Packet packet = 1 [(gogoproto.nullable) = false];
bytes proof = 2;
bytes proof_commitment = 2 [(gogoproto.moretags) = "yaml:\"proof_commitment\""];
ibc.core.client.v1.Height proof_height = 3
[(gogoproto.moretags) = "yaml:\"proof_height\"", (gogoproto.nullable) = false];
string signer = 4;
Expand All @@ -163,7 +163,7 @@ message MsgTimeout {
option (gogoproto.goproto_getters) = false;

Packet packet = 1 [(gogoproto.nullable) = false];
bytes proof = 2;
bytes proof_unreceived = 2 [(gogoproto.moretags) = "yaml:\"proof_unreceived\""];
ibc.core.client.v1.Height proof_height = 3
[(gogoproto.moretags) = "yaml:\"proof_height\"", (gogoproto.nullable) = false];
uint64 next_sequence_recv = 4 [(gogoproto.moretags) = "yaml:\"next_sequence_recv\""];
Expand All @@ -179,7 +179,7 @@ message MsgTimeoutOnClose {
option (gogoproto.goproto_getters) = false;

Packet packet = 1 [(gogoproto.nullable) = false];
bytes proof = 2;
bytes proof_unreceived = 2 [(gogoproto.moretags) = "yaml:\"proof_unreceived\""];
bytes proof_close = 3 [(gogoproto.moretags) = "yaml:\"proof_close\""];
ibc.core.client.v1.Height proof_height = 4
[(gogoproto.moretags) = "yaml:\"proof_height\"", (gogoproto.nullable) = false];
Expand All @@ -197,7 +197,7 @@ message MsgAcknowledgement {

Packet packet = 1 [(gogoproto.nullable) = false];
bytes acknowledgement = 2;
bytes proof = 3;
bytes proof_acked = 3 [(gogoproto.moretags) = "yaml:\"proof_acked\""];
ibc.core.client.v1.Height proof_height = 4
[(gogoproto.moretags) = "yaml:\"proof_height\"", (gogoproto.nullable) = false];
string signer = 5;
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/core/03-connection/keeper/handshake_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func (suite *KeeperTestSuite) TestConnOpenTry() {
suite.Require().True(found)

connection.State = types.INIT
connection.Versions = []*types.Version{&types.Version{}}
connection.Versions = []*types.Version{{}}

suite.chainB.App.IBCKeeper.ConnectionKeeper.SetConnection(suite.chainB.GetContext(), connB.ID, connection)

Expand Down
2 changes: 1 addition & 1 deletion x/ibc/core/03-connection/types/connection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestConnectionValidateBasic(t *testing.T) {
},
{
"invalid version",
types.ConnectionEnd{clientID, []*types.Version{&types.Version{}}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}},
types.ConnectionEnd{clientID, []*types.Version{{}}, types.INIT, types.Counterparty{clientID2, connectionID2, commitmenttypes.NewMerklePrefix([]byte("prefix"))}},
false,
},
{
Expand Down
2 changes: 1 addition & 1 deletion x/ibc/core/03-connection/types/msgs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenTry() {
{"invalid consensusHeight", types.NewMsgConnectionOpenTry("ibcconntest", provedID, "clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), signer), false},
{"empty singer", types.NewMsgConnectionOpenTry("ibcconntest", provedID, "clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, nil), false},
{"success", types.NewMsgConnectionOpenTry("ibcconntest", provedID, "clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true},
{"invalid version", types.NewMsgConnectionOpenTry("ibcconntest", provedID, "clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{&types.Version{}}, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
{"invalid version", types.NewMsgConnectionOpenTry("ibcconntest", provedID, "clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{{}}, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
}

for _, tc := range testCases {
Expand Down
45 changes: 29 additions & 16 deletions x/ibc/core/04-channel/types/msgs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,14 +405,14 @@ var _ sdk.Msg = &MsgRecvPacket{}
// NewMsgRecvPacket constructs new MsgRecvPacket
// nolint:interfacer
func NewMsgRecvPacket(
packet Packet, proof []byte, proofHeight clienttypes.Height,
packet Packet, proofCommitment []byte, proofHeight clienttypes.Height,
signer sdk.AccAddress,
) *MsgRecvPacket {
return &MsgRecvPacket{
Packet: packet,
Proof: proof,
ProofHeight: proofHeight,
Signer: signer.String(),
Packet: packet,
ProofCommitment: proofCommitment,
ProofHeight: proofHeight,
Signer: signer.String(),
}
}

Expand All @@ -423,7 +423,7 @@ func (msg MsgRecvPacket) Route() string {

// ValidateBasic implements sdk.Msg
func (msg MsgRecvPacket) ValidateBasic() error {
if len(msg.Proof) == 0 {
if len(msg.ProofCommitment) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof")
}
if msg.ProofHeight.IsZero() {
Expand Down Expand Up @@ -468,13 +468,13 @@ var _ sdk.Msg = &MsgTimeout{}
// NewMsgTimeout constructs new MsgTimeout
// nolint:interfacer
func NewMsgTimeout(
packet Packet, nextSequenceRecv uint64, proof []byte,
packet Packet, nextSequenceRecv uint64, proofUnreceived []byte,
proofHeight clienttypes.Height, signer sdk.AccAddress,
) *MsgTimeout {
return &MsgTimeout{
Packet: packet,
NextSequenceRecv: nextSequenceRecv,
Proof: proof,
ProofUnreceived: proofUnreceived,
ProofHeight: proofHeight,
Signer: signer.String(),
}
Expand All @@ -487,12 +487,15 @@ func (msg MsgTimeout) Route() string {

// ValidateBasic implements sdk.Msg
func (msg MsgTimeout) ValidateBasic() error {
if len(msg.Proof) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof")
if len(msg.ProofUnreceived) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty unreceived proof")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.NextSequenceRecv == 0 {
return sdkerrors.Wrap(sdkerrors.ErrInvalidSequence, "next sequence receive cannot be 0")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down Expand Up @@ -524,13 +527,13 @@ func (msg MsgTimeout) Type() string {
// nolint:interfacer
func NewMsgTimeoutOnClose(
packet Packet, nextSequenceRecv uint64,
proof, proofClose []byte,
proofUnreceived, proofClose []byte,
proofHeight clienttypes.Height, signer sdk.AccAddress,
) *MsgTimeoutOnClose {
return &MsgTimeoutOnClose{
Packet: packet,
NextSequenceRecv: nextSequenceRecv,
Proof: proof,
ProofUnreceived: proofUnreceived,
ProofClose: proofClose,
ProofHeight: proofHeight,
Signer: signer.String(),
Expand All @@ -544,7 +547,10 @@ func (msg MsgTimeoutOnClose) Route() string {

// ValidateBasic implements sdk.Msg
func (msg MsgTimeoutOnClose) ValidateBasic() error {
if len(msg.Proof) == 0 {
if msg.NextSequenceRecv == 0 {
return sdkerrors.Wrap(sdkerrors.ErrInvalidSequence, "next sequence receive cannot be 0")
}
if len(msg.ProofUnreceived) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof")
}
if len(msg.ProofClose) == 0 {
Expand Down Expand Up @@ -585,11 +591,15 @@ var _ sdk.Msg = &MsgAcknowledgement{}
// NewMsgAcknowledgement constructs a new MsgAcknowledgement
// nolint:interfacer
func NewMsgAcknowledgement(
packet Packet, ack []byte, proof []byte, proofHeight clienttypes.Height, signer sdk.AccAddress) *MsgAcknowledgement {
packet Packet,
ack, proofAcked []byte,
proofHeight clienttypes.Height,
signer sdk.AccAddress,
) *MsgAcknowledgement {
return &MsgAcknowledgement{
Packet: packet,
Acknowledgement: ack,
Proof: proof,
ProofAcked: proofAcked,
ProofHeight: proofHeight,
Signer: signer.String(),
}
Expand All @@ -602,12 +612,15 @@ func (msg MsgAcknowledgement) Route() string {

// ValidateBasic implements sdk.Msg
func (msg MsgAcknowledgement) ValidateBasic() error {
if len(msg.Proof) == 0 {
if len(msg.ProofAcked) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if len(msg.Acknowledgement) == 0 {
return sdkerrors.Wrap(ErrInvalidAcknowledgement, "ack bytes cannot be empty")
}
_, err := sdk.AccAddressFromBech32(msg.Signer)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
Expand Down
9 changes: 6 additions & 3 deletions x/ibc/core/04-channel/types/msgs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func (suite *TypesTestSuite) TestMsgRecvPacketValidateBasic() {
msg *types.MsgRecvPacket
expPass bool
}{
{"", types.NewMsgRecvPacket(packet, suite.proof, height, addr), true},
{"success", types.NewMsgRecvPacket(packet, suite.proof, height, addr), true},
{"proof height is zero", types.NewMsgRecvPacket(packet, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"proof contain empty proof", types.NewMsgRecvPacket(packet, emptyProof, height, addr), false},
{"missing signer address", types.NewMsgRecvPacket(packet, suite.proof, height, emptyAddr), false},
Expand Down Expand Up @@ -369,8 +369,9 @@ func (suite *TypesTestSuite) TestMsgTimeoutValidateBasic() {
msg *types.MsgTimeout
expPass bool
}{
{"", types.NewMsgTimeout(packet, 1, suite.proof, height, addr), true},
{"success", types.NewMsgTimeout(packet, 1, suite.proof, height, addr), true},
{"proof height must be > 0", types.NewMsgTimeout(packet, 1, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"seq 0", types.NewMsgTimeout(packet, 0, suite.proof, height, addr), false},
{"missing signer address", types.NewMsgTimeout(packet, 1, suite.proof, height, emptyAddr), false},
{"cannot submit an empty proof", types.NewMsgTimeout(packet, 1, emptyProof, height, addr), false},
{"invalid packet", types.NewMsgTimeout(invalidPacket, 1, suite.proof, height, addr), false},
Expand Down Expand Up @@ -398,6 +399,7 @@ func (suite *TypesTestSuite) TestMsgTimeoutOnCloseValidateBasic() {
expPass bool
}{
{"success", types.NewMsgTimeoutOnClose(packet, 1, suite.proof, suite.proof, height, addr), true},
{"seq 0", types.NewMsgTimeoutOnClose(packet, 0, suite.proof, suite.proof, height, addr), false},
{"empty proof", types.NewMsgTimeoutOnClose(packet, 1, emptyProof, suite.proof, height, addr), false},
{"empty proof close", types.NewMsgTimeoutOnClose(packet, 1, suite.proof, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgTimeoutOnClose(packet, 1, suite.proof, suite.proof, clienttypes.ZeroHeight(), addr), false},
Expand Down Expand Up @@ -426,8 +428,9 @@ func (suite *TypesTestSuite) TestMsgAcknowledgementValidateBasic() {
msg *types.MsgAcknowledgement
expPass bool
}{
{"", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, height, addr), true},
{"success", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, height, addr), true},
{"proof height must be > 0", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, clienttypes.ZeroHeight(), addr), false},
{"empty ack", types.NewMsgAcknowledgement(packet, nil, suite.proof, height, addr), false},
{"missing signer address", types.NewMsgAcknowledgement(packet, packet.GetData(), suite.proof, height, emptyAddr), false},
{"cannot submit an empty proof", types.NewMsgAcknowledgement(packet, packet.GetData(), emptyProof, height, addr), false},
{"invalid packet", types.NewMsgAcknowledgement(invalidPacket, packet.GetData(), suite.proof, height, addr), false},
Expand Down
Loading

0 comments on commit e77c950

Please sign in to comment.