Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow zero proof height packet recv ack timeout #2781

Merged
merged 13 commits into from
Nov 22, 2022
Merged

Allow zero proof height packet recv ack timeout #2781

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c8b18a3
adding mock channel handshake to solomachine, removing enforcement of…
damiannolan Nov 15, 2022
4dea53f
adding additional godocs
damiannolan Nov 15, 2022
5a437ad
adding chan close confirm step to solomachine test
damiannolan Nov 16, 2022
29c008a
adding solomachine mocked packet recv, allow zero height on MsgRecvPa…
damiannolan Nov 16, 2022
b1f2595
test: adding test for solomachine packet acknowledgement
chatton Nov 16, 2022
ab52d23
wip: started work on timeout test
chatton Nov 16, 2022
e9e371b
test: adding test for solomachine timeout
chatton Nov 17, 2022
33eb551
chore: merging colin/1874-sm-proof-height
chatton Nov 18, 2022
527f86f
chore: fixing bad merge
chatton Nov 18, 2022
460d237
wip: adding test for TimeoutPacketOnClose
chatton Nov 18, 2022
e525018
adding timeout on close solomachine test
damiannolan Nov 18, 2022
2f18bf4
fixing unit tests
damiannolan Nov 18, 2022
def9454
updating godocs and aligning apis
damiannolan Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions modules/core/04-channel/types/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,6 @@ func (msg MsgChannelOpenTry) ValidateBasic() error {
if len(msg.ProofInit) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof init")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.Channel.State != TRYOPEN {
return sdkerrors.Wrapf(ErrInvalidChannelState,
"channel state must be TRYOPEN in MsgChannelOpenTry. expected: %s, got: %s",
Expand Down Expand Up @@ -159,9 +156,6 @@ func (msg MsgChannelOpenAck) ValidateBasic() error {
if len(msg.ProofTry) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof try")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, 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 @@ -207,9 +201,6 @@ func (msg MsgChannelOpenConfirm) ValidateBasic() error {
if len(msg.ProofAck) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof ack")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, 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 @@ -294,9 +285,6 @@ func (msg MsgChannelCloseConfirm) ValidateBasic() error {
if len(msg.ProofInit) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof init")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
_, 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 @@ -335,9 +323,6 @@ func (msg MsgRecvPacket) ValidateBasic() error {
if len(msg.ProofCommitment) == 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")
}
_, 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 @@ -384,9 +369,6 @@ func (msg MsgTimeout) ValidateBasic() error {
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")
}
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
if msg.NextSequenceRecv == 0 {
return sdkerrors.Wrap(sdkerrors.ErrInvalidSequence, "next sequence receive cannot be 0")
}
Expand Down Expand Up @@ -479,9 +461,6 @@ func (msg MsgAcknowledgement) ValidateBasic() error {
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")
}
Expand Down
9 changes: 1 addition & 8 deletions modules/core/04-channel/types/msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
"github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/v6/modules/core/23-commitment/types"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
"github.com/cosmos/ibc-go/v6/testing/simapp"
)

Expand Down Expand Up @@ -56,9 +55,7 @@ var (
packet = types.NewPacket(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp)
invalidPacket = types.NewPacket(unknownPacketData, 0, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp)

emptyProof = []byte{}
invalidProofs1 = exported.Proof(nil)
invalidProofs2 = emptyProof
emptyProof = []byte{}

addr = sdk.AccAddress("testaddr111111111111").String()
emptyAddr string
Expand Down Expand Up @@ -158,7 +155,6 @@ func (suite *TypesTestSuite) TestMsgChannelOpenTryValidateBasic() {
{"too long port id", types.NewMsgChannelOpenTry(invalidLongPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"port id contains non-alpha", types.NewMsgChannelOpenTry(invalidPort, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, "", suite.proof, height, addr), true},
{"proof height is zero", types.NewMsgChannelOpenTry(portid, version, types.ORDERED, connHops, cpportid, cpchanid, version, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"invalid channel order", types.NewMsgChannelOpenTry(portid, version, types.Order(4), connHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"connection hops more than 1 ", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
{"too short connection id", types.NewMsgChannelOpenTry(portid, version, types.UNORDERED, invalidShortConnHops, cpportid, cpchanid, version, suite.proof, height, addr), false},
Expand Down Expand Up @@ -202,7 +198,6 @@ func (suite *TypesTestSuite) TestMsgChannelOpenAckValidateBasic() {
{"channel id contains non-alpha", types.NewMsgChannelOpenAck(portid, invalidChannel, chanid, version, suite.proof, height, addr), false},
{"", types.NewMsgChannelOpenAck(portid, chanid, chanid, "", suite.proof, height, addr), true},
{"empty proof", types.NewMsgChannelOpenAck(portid, chanid, chanid, version, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgChannelOpenAck(portid, chanid, chanid, version, suite.proof, clienttypes.ZeroHeight(), addr), false},
{"invalid counterparty channel id", types.NewMsgChannelOpenAck(portid, chanid, invalidShortChannel, version, suite.proof, height, addr), false},
}

Expand Down Expand Up @@ -235,7 +230,6 @@ func (suite *TypesTestSuite) TestMsgChannelOpenConfirmValidateBasic() {
{"too long channel id", types.NewMsgChannelOpenConfirm(portid, invalidLongChannel, suite.proof, height, addr), false},
{"channel id contains non-alpha", types.NewMsgChannelOpenConfirm(portid, invalidChannel, suite.proof, height, addr), false},
{"empty proof", types.NewMsgChannelOpenConfirm(portid, chanid, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgChannelOpenConfirm(portid, chanid, suite.proof, clienttypes.ZeroHeight(), addr), false},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -297,7 +291,6 @@ func (suite *TypesTestSuite) TestMsgChannelCloseConfirmValidateBasic() {
{"too long channel id", types.NewMsgChannelCloseConfirm(portid, invalidLongChannel, suite.proof, height, addr), false},
{"channel id contains non-alpha", types.NewMsgChannelCloseConfirm(portid, invalidChannel, suite.proof, height, addr), false},
{"empty proof", types.NewMsgChannelCloseConfirm(portid, chanid, emptyProof, height, addr), false},
{"proof height is zero", types.NewMsgChannelCloseConfirm(portid, chanid, suite.proof, clienttypes.ZeroHeight(), addr), false},
}

for _, tc := range testCases {
Expand Down
3 changes: 0 additions & 3 deletions modules/light-clients/06-solomachine/client_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ func (cs ClientState) GetTimestampAtHeight(
cdc codec.BinaryCodec,
height exported.Height,
) (uint64, error) {
if !cs.GetLatestHeight().EQ(height) {
return 0, sdkerrors.Wrapf(ErrInvalidSequence, "not latest height (%s)", height)
}
Comment on lines -49 to -51
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is correct 👍

return cs.ConsensusState.Timestamp, nil
}

Expand Down
13 changes: 4 additions & 9 deletions modules/light-clients/06-solomachine/client_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
)

commitmentBz := channeltypes.CommitPacket(suite.chainA.Codec, packet)
merklePath := sm.GetPacketCommitmentPath(ibctesting.MockPort, ibctesting.FirstChannelID)
merklePath := sm.GetPacketCommitmentPath(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
signBytes = solomachine.SignBytes{
Sequence: sm.Sequence,
Timestamp: sm.Time,
Expand Down Expand Up @@ -387,7 +387,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
{
"success: packet acknowledgement verification",
func() {
merklePath := sm.GetPacketAcknowledgementPath(ibctesting.MockPort, ibctesting.FirstChannelID)
merklePath := sm.GetPacketAcknowledgementPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
signBytes = solomachine.SignBytes{
Sequence: sm.Sequence,
Timestamp: sm.Time,
Expand Down Expand Up @@ -417,7 +417,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
{
"success: packet receipt verification",
func() {
merklePath := sm.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID)
merklePath := sm.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
signBytes = solomachine.SignBytes{
Sequence: sm.Sequence,
Timestamp: sm.Time,
Expand Down Expand Up @@ -607,7 +607,7 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
{
"success: packet receipt absence verification",
func() {
merklePath := suite.solomachine.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID)
merklePath := suite.solomachine.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID, 1)
signBytes = solomachine.SignBytes{
Sequence: sm.GetHeight().GetRevisionHeight(),
Timestamp: sm.Time,
Expand Down Expand Up @@ -802,11 +802,6 @@ func (suite *SoloMachineTestSuite) TestGetTimestampAtHeight() {
expValue: suite.solomachine.ClientState().ConsensusState.Timestamp,
expPass: true,
},
{
name: "get timestamp at height not exists",
clientState: suite.solomachine.ClientState(),
height: suite.solomachine.ClientState().GetLatestHeight().Increment(),
},
Comment on lines -798 to -801
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test case was testing the behaviour of GetTimestampAtHeight which would error out when !cs.GetLatestHeight().EQ(height) , which means a zero proof would never be valid.

I'm not sure exactly how to go ahead with this test case or if it's still valid. cc @damiannolan @colin-axner

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change the behaviour of GetTimestampAtHeight() to ignore the passed in height, the timestamp returned should simply be the latest timestamp, thus this test case can be removed

}

for i, tc := range testCases {
Expand Down
86 changes: 83 additions & 3 deletions modules/light-clients/06-solomachine/solomachine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package solomachine_test

import (
"testing"
"time"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
Expand All @@ -11,10 +12,17 @@ import (
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"

clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
solomachine "github.com/cosmos/ibc-go/v6/modules/light-clients/06-solomachine"
ibctesting "github.com/cosmos/ibc-go/v6/testing"
"github.com/cosmos/ibc-go/v6/testing/mock"
)

var (
channelIDSolomachine = "channel-on-solomachine" // channelID generated on solo machine side
)

type SoloMachineTestSuite struct {
Expand Down Expand Up @@ -46,18 +54,90 @@ func TestSoloMachineTestSuite(t *testing.T) {
suite.Run(t, new(SoloMachineTestSuite))
}

func (suite *SoloMachineTestSuite) TestConnectionHandshake() {
func (suite *SoloMachineTestSuite) TestSolomachineRecvPacket() {
channelID := suite.SetupSolomachine()

// send packet is not necessary as the solo machine implementation is mocked

suite.solomachine.RecvPacket(suite.chainA, channelID)

// close init is not necessary as the solomachine implementation is mocked

suite.solomachine.ChanCloseConfirm(suite.chainA, channelID)
}

func (suite *SoloMachineTestSuite) SetupSolomachine() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should maybe return a SolomachineConfig struct from this function which encapsulates the client/connection/channel/port IDs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could make use of ibctesting.Endpoint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking into this now, is looks like the endpoints in all the other tests are constructed using a suite.chainA and suite.chainB, for this will we need to fully create instances of ibctesting.Endpoint that are referencing solomachine and chainA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colin-axner do you think we should do this in this PR or are you happy with a follow up issue? This PR is already pretty big!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think any change to use endpoint can be made in a followup pr. I would not recommend modifying the testing package to handle solo machine endpoints, I was primarily thinking that you could manually fill in the endpoint with the correct values without supporting the ability to do suite.coordinator.Setup(path). I see the difficulty though since when you call NewEndpoint you provide quite a bit of information. What do you think about having a NewSolomachineEndpoint, also very open to just creating a new solomachine endpoint struct which contains less information (if that makes sense). I figure we can always consolidate the two types in the future if there is enough overlap

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a new type may work well here, I will create a follow up issue for this and look into after this is merged into your branch! 👍

clientID := suite.solomachine.CreateClient(suite.chainA)

connectionID := suite.solomachine.ConnOpenInit(suite.chainA, clientID)

// open try is not necessary as the solo machine implementation is mock'd
// open try is not necessary as the solo machine implementation is mocked

suite.solomachine.ConnOpenAck(suite.chainA, clientID, connectionID)

// open confirm is not necessary as the solo machine implementation is mock'd
// open confirm is not necessary as the solo machine implementation is mocked

channelID := suite.solomachine.ChanOpenInit(suite.chainA, connectionID)

// open try is not necessary as the solo machine implementation is mocked

suite.solomachine.ChanOpenAck(suite.chainA, channelID)

// open confirm is not necessary as the solo machine implementation is mocked

return channelID
}

func (suite *SoloMachineTestSuite) TestSolomachineAck() {
channelID := suite.SetupSolomachine()

packet := channeltypes.NewPacket(
mock.MockPacketData,
1,
mock.PortID,
channelID,
mock.PortID,
channelIDSolomachine,
clienttypes.ZeroHeight(),
uint64(suite.chainA.GetContext().BlockTime().Add(time.Hour).UnixNano()),
)

suite.solomachine.SendPacket(suite.chainA, packet)

// recv packet is not necessary as the solo machine implementation is mocked

suite.solomachine.AcknowledgePacket(suite.chainA, packet)

// close init is not necessary as the solomachine implementation is mocked

suite.solomachine.ChanCloseConfirm(suite.chainA, channelID)
}

func (suite *SoloMachineTestSuite) TestSolomachineTimeout() {
channelID := suite.SetupSolomachine()

packet := channeltypes.NewPacket(
mock.MockPacketData,
1,
mock.PortID,
channelID,
mock.PortID,
channelIDSolomachine,
clienttypes.ZeroHeight(),
1,
)

// immediately timeout the packet
time.Sleep(time.Nanosecond * 2)

suite.solomachine.SendPacket(suite.chainA, packet)

suite.solomachine.TimeoutPacket(suite.chainA, packet)

suite.solomachine.ChanCloseConfirm(suite.chainA, channelID)
}


func (suite *SoloMachineTestSuite) GetSequenceFromStore() uint64 {
bz := suite.store.Get(host.ClientStateKey())
suite.Require().NotNil(bz)
Expand Down
Loading