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.

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

fix: allow zero proof height, solo machine discards provided proof height in favor of sequence #2746

Merged
merged 19 commits into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
29c64f0
refactor(solomachine): discard passed in proof height and remove vali…
colin-axner Nov 14, 2022
ed54f58
test: fix solo machine tests
colin-axner Nov 14, 2022
6a896cb
fix(03-connection): remove proof height checks
colin-axner Nov 14, 2022
ed1b491
test(solomachine): add tests for connection handshake
colin-axner Nov 14, 2022
bbee84b
refactor(solomachine): move testing helper functions to testing package
colin-axner Nov 14, 2022
83a4aab
self review: remove comment
colin-axner Nov 14, 2022
6ce6d58
make format
colin-axner Nov 14, 2022
75e4f51
Merge branch 'main' of github.com:cosmos/ibc-go into colin/1874-sm-pr…
colin-axner Nov 14, 2022
abbaeef
chore: add changelog entry
colin-axner Nov 14, 2022
07b9bf4
Merge branch 'main' of github.com:cosmos/ibc-go into colin/1874-sm-pr…
colin-axner Nov 16, 2022
5087d52
chore: add docs for zero proof height logic change
colin-axner Nov 16, 2022
36d83c2
chore: remove empty space
colin-axner Nov 16, 2022
b6dc225
chore: solomachine mock channel handshake, allow zero-height in chann…
damiannolan Nov 16, 2022
2becfbf
chore: update changelog entry
colin-axner Nov 22, 2022
0ed02b7
Merge branch 'colin/1874-sm-proof-height' of github.com:cosmos/ibc-go…
colin-axner Nov 22, 2022
7cb3068
Allow zero proof height packet recv ack timeout (#2781)
chatton Nov 22, 2022
9d58aff
Merge branch 'main' into colin/1874-sm-proof-height
colin-axner Nov 22, 2022
f4f8def
chore: update changelog entry to improve wording
colin-axner Nov 22, 2022
f07f10d
Merge branch 'colin/1874-sm-proof-height' of github.com:cosmos/ibc-go…
colin-axner Nov 22, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (light-clients/06-solomachine) [\#2746](https://github.com/cosmos/ibc-go/pull/2746) Discard proofHeight for solo machines and use sequence. Allow proof height to be zero for connection handshake.
* (modules/light-clients/07-tendermint) [\#1713](https://github.com/cosmos/ibc-go/pull/1713) Allow client upgrade proposals to update `TrustingPeriod`. See ADR-026 for context.
* (modules/core/02-client) [\#1188](https://github.com/cosmos/ibc-go/pull/1188/files) Routing `MsgSubmitMisbehaviour` to `UpdateClient` keeper function. Deprecating `SubmitMisbehaviour` endpoint.
* (modules/core/02-client) [\#1208](https://github.com/cosmos/ibc-go/pull/1208) Replace `CheckHeaderAndUpdateState` usage in 02-client with calls to `VerifyClientMessage`, `CheckForMisbehaviour`, `UpdateStateOnMisbehaviour` and `UpdateState`.
Expand Down
2 changes: 2 additions & 0 deletions docs/migrations/v6-to-v7.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ The `CheckMisbehaviourAndUpdateState` function has been removed from `ClientStat

The function `GetTimestampAtHeight` has been added to the `ClientState` interface. It should return the timestamp for a consensus state associated with the provided height.

A zero proof height is now allowed by core IBC and may be passed into `VerifyMembership` and `VerifyNonMembership`. Light clients are responsible for returning an error if a zero proof height is invalid behaviour.
Copy link
Member

Choose a reason for hiding this comment

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

Should we also mention relayers are must pass zero height for solomachines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relayers can pass non zero heights. Solo machine simply ignores the provided value


### `Header` and `Misbehaviour`

`exported.Header` and `exported.Misbehaviour` interface types have been merged and renamed to `ClientMessage` interface.
Expand Down
9 changes: 0 additions & 9 deletions modules/core/03-connection/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,6 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error {
if len(msg.ProofConsensus) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof of consensus state")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.ConsensusHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "consensus height must be non-zero")
}
Expand Down Expand Up @@ -226,9 +223,6 @@ func (msg MsgConnectionOpenAck) ValidateBasic() error {
if len(msg.ProofConsensus) == 0 {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "cannot submit an empty proof of consensus state")
}
if msg.ProofHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero")
}
if msg.ConsensusHeight.IsZero() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "consensus height must be non-zero")
}
Expand Down Expand Up @@ -271,9 +265,6 @@ func (msg MsgConnectionOpenConfirm) 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
8 changes: 2 additions & 6 deletions modules/core/03-connection/types/msgs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenTry() {
{"empty proofInit", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, emptyProof, suite.proof, suite.proof, clientHeight, clientHeight, signer), false},
{"empty proofClient", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, emptyProof, suite.proof, clientHeight, clientHeight, signer), false},
{"empty proofConsensus", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, emptyProof, clientHeight, clientHeight, signer), false},
{"invalid proofHeight", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clienttypes.ZeroHeight(), clientHeight, signer), false},
{"invalid consensusHeight", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), signer), false},
{"empty singer", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, ""), false},
{"success", types.NewMsgConnectionOpenTry("clienttotesta", "connectiontotest", "clienttotest", clientState, prefix, []*types.Version{ibctesting.ConnectionVersion}, 500, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, signer), true},
Expand Down Expand Up @@ -191,7 +190,6 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenAck() {
{"empty proofTry", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, emptyProof, suite.proof, suite.proof, clientHeight, clientHeight, ibctesting.ConnectionVersion, signer), false},
{"empty proofClient", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, emptyProof, suite.proof, clientHeight, clientHeight, ibctesting.ConnectionVersion, signer), false},
{"empty proofConsensus", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, emptyProof, clientHeight, clientHeight, ibctesting.ConnectionVersion, signer), false},
{"invalid proofHeight", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clienttypes.ZeroHeight(), clientHeight, ibctesting.ConnectionVersion, signer), false},
{"invalid consensusHeight", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clientHeight, clienttypes.ZeroHeight(), ibctesting.ConnectionVersion, signer), false},
{"invalid version", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, &types.Version{}, signer), false},
{"empty signer", types.NewMsgConnectionOpenAck(connectionID, connectionID, clientState, suite.proof, suite.proof, suite.proof, clientHeight, clientHeight, ibctesting.ConnectionVersion, ""), false},
Expand All @@ -212,7 +210,6 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenConfirm() {
testMsgs := []*types.MsgConnectionOpenConfirm{
types.NewMsgConnectionOpenConfirm("test/conn1", suite.proof, clientHeight, signer),
types.NewMsgConnectionOpenConfirm(connectionID, emptyProof, clientHeight, signer),
types.NewMsgConnectionOpenConfirm(connectionID, suite.proof, clienttypes.ZeroHeight(), signer),
types.NewMsgConnectionOpenConfirm(connectionID, suite.proof, clientHeight, ""),
types.NewMsgConnectionOpenConfirm(connectionID, suite.proof, clientHeight, signer),
}
Expand All @@ -224,9 +221,8 @@ func (suite *MsgTestSuite) TestNewMsgConnectionOpenConfirm() {
}{
{testMsgs[0], false, "invalid connection ID"},
{testMsgs[1], false, "empty proofTry"},
{testMsgs[2], false, "invalid proofHeight"},
{testMsgs[3], false, "empty signer"},
{testMsgs[4], true, "success"},
{testMsgs[2], false, "empty signer"},
{testMsgs[3], true, "success"},
}

for i, tc := range testCases {
Expand Down
2 changes: 1 addition & 1 deletion modules/core/04-channel/keeper/handshake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ func (suite *KeeperTestSuite) TestChanCloseInit() {
path.SetChannelOrdered()
err = path.EndpointA.ChanOpenInit()
suite.Require().NoError(err)

// ensure channel capability check passes
suite.chainA.CreateChannelCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
Expand Down
31 changes: 8 additions & 23 deletions modules/light-clients/06-solomachine/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,20 +102,20 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
return sdkerrors.Wrap(clienttypes.ErrInvalidUpgradeClient, "cannot upgrade solomachine client")
}

// VerifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the specified height.
// VerifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the latest sequence.
// The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
func (cs *ClientState) VerifyMembership(
ctx sdk.Context,
clientStore sdk.KVStore,
cdc codec.BinaryCodec,
height exported.Height,
_ exported.Height,
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path exported.Path,
value []byte,
) error {
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, proof)
if err != nil {
return err
}
Expand Down Expand Up @@ -149,19 +149,19 @@ func (cs *ClientState) VerifyMembership(
return nil
}

// VerifyNonMembership is a generic proof verification method which verifies the absence of a given CommitmentPath at a specified height.
// VerifyNonMembership is a generic proof verification method which verifies the absence of a given CommitmentPath at the latest sequence.
// The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
func (cs *ClientState) VerifyNonMembership(
ctx sdk.Context,
clientStore sdk.KVStore,
cdc codec.BinaryCodec,
height exported.Height,
_ exported.Height,
delayTimePeriod uint64,
delayBlockPeriod uint64,
proof []byte,
path exported.Path,
) error {
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, proof)
if err != nil {
return err
}
Expand Down Expand Up @@ -197,18 +197,12 @@ func (cs *ClientState) VerifyNonMembership(

// produceVerificationArgs perfoms the basic checks on the arguments that are
// shared between the verification functions and returns the public key of the
// consensus state, the unmarshalled proof representing the signature and timestamp
// along with the solo-machine sequence encoded in the proofHeight.
// consensus state, the unmarshalled proof representing the signature and timestamp.
func produceVerificationArgs(
cdc codec.BinaryCodec,
cs *ClientState,
height exported.Height,
proof []byte,
) (cryptotypes.PubKey, signing.SignatureData, uint64, uint64, error) {
if revision := height.GetRevisionNumber(); revision != 0 {
return nil, nil, 0, 0, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "revision must be 0 for solomachine, got revision-number: %d", revision)
}

if proof == nil {
return nil, nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "proof cannot be empty")
}
Expand All @@ -228,20 +222,11 @@ func produceVerificationArgs(
return nil, nil, 0, 0, err
}

// sequence is encoded in the revision height of height struct
sequence := height.GetRevisionHeight()
latestSequence := cs.GetLatestHeight().GetRevisionHeight()
if latestSequence != sequence {
return nil, nil, 0, 0, sdkerrors.Wrapf(
sdkerrors.ErrInvalidHeight,
"client state sequence != proof sequence (%d != %d)", latestSequence, sequence,
)
}

if cs.ConsensusState.GetTimestamp() > timestamp {
return nil, nil, 0, 0, sdkerrors.Wrapf(ErrInvalidProof, "the consensus state timestamp is greater than the signature timestamp (%d >= %d)", cs.ConsensusState.GetTimestamp(), timestamp)
}

sequence := cs.GetLatestHeight().GetRevisionHeight()
publicKey, err := cs.ConsensusState.GetPubKey()
if err != nil {
return nil, nil, 0, 0, err
Expand Down
52 changes: 5 additions & 47 deletions modules/light-clients/06-solomachine/client_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,11 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {

var (
clientState *solomachine.ClientState
err error
height clienttypes.Height
path exported.Path
proof []byte
testingPath *ibctesting.Path
signBytes solomachine.SignBytes
err error
)

testCases := []struct {
Expand Down Expand Up @@ -203,7 +202,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
consensusStateBz, err := suite.chainA.Codec.Marshal(consensusState)
suite.Require().NoError(err)

path = sm.GetConsensusStatePath(counterpartyClientIdentifier, height)
path = sm.GetConsensusStatePath(counterpartyClientIdentifier, clienttypes.NewHeight(0, 1))
signBytes = solomachine.SignBytes{
Sequence: sm.Sequence,
Timestamp: sm.Time,
Expand Down Expand Up @@ -424,25 +423,6 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
},
true,
},
{
"client state latest height is less than sequence",
func() {
consensusState := &solomachine.ConsensusState{
Timestamp: sm.Time,
PublicKey: sm.ConsensusState().PublicKey,
}

clientState = solomachine.NewClientState(sm.Sequence-1, consensusState)
},
false,
},
{
"height revision number is not zero",
func() {
height = clienttypes.NewHeight(1, sm.GetHeight().GetRevisionHeight())
},
false,
},
{
"invalid path type",
func() {
Expand Down Expand Up @@ -527,7 +507,6 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
testingPath = ibctesting.NewPath(suite.chainA, suite.chainB)

clientState = sm.ClientState()
height = clienttypes.NewHeight(sm.GetHeight().GetRevisionNumber(), sm.GetHeight().GetRevisionHeight())

path = commitmenttypes.NewMerklePath("ibc", "solomachine")
signBytes = solomachine.SignBytes{
Expand Down Expand Up @@ -560,7 +539,7 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {

err = clientState.VerifyMembership(
suite.chainA.GetContext(), suite.store, suite.chainA.Codec,
height, 0, 0, // solomachine does not check delay periods
clienttypes.ZeroHeight(), 0, 0, // solomachine does not check delay periods
proof, path, signBytes.Data,
)

Expand Down Expand Up @@ -610,11 +589,10 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {

var (
clientState *solomachine.ClientState
err error
height clienttypes.Height
path exported.Path
proof []byte
signBytes solomachine.SignBytes
err error
)

testCases := []struct {
Expand Down Expand Up @@ -654,25 +632,6 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
},
true,
},
{
"client state latest height is less than sequence",
func() {
consensusState := &solomachine.ConsensusState{
Timestamp: sm.Time,
PublicKey: sm.ConsensusState().PublicKey,
}

clientState = solomachine.NewClientState(sm.Sequence-1, consensusState)
},
false,
},
{
"height revision number is not zero",
func() {
height = clienttypes.NewHeight(1, sm.GetHeight().GetRevisionHeight())
},
false,
},
{
"invalid path type",
func() {
Expand Down Expand Up @@ -767,7 +726,6 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {

suite.Run(tc.name, func() {
clientState = sm.ClientState()
height = clienttypes.NewHeight(sm.GetHeight().GetRevisionNumber(), sm.GetHeight().GetRevisionHeight())

path = commitmenttypes.NewMerklePath("ibc", "solomachine")
signBytes = solomachine.SignBytes{
Expand Down Expand Up @@ -800,7 +758,7 @@ func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {

err = clientState.VerifyNonMembership(
suite.chainA.GetContext(), suite.store, suite.chainA.Codec,
height, 0, 0, // solomachine does not check delay periods
clienttypes.ZeroHeight(), 0, 0, // solomachine does not check delay periods
proof, path,
)

Expand Down
12 changes: 12 additions & 0 deletions modules/light-clients/06-solomachine/solomachine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,18 @@ func TestSoloMachineTestSuite(t *testing.T) {
suite.Run(t, new(SoloMachineTestSuite))
}

func (suite *SoloMachineTestSuite) TestConnectionHandshake() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when channel handshake tests are added, this func can be renamed to Setup() and it can be extended to finish a channel handshake

Copy link
Member

Choose a reason for hiding this comment

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

This isn't being used anywhere yet, right?
Where do you think we will use it? And should we mock the counterparty(TRY/CONFIRM) handlers too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it is acting as an integration test for setting up connections. We will use it in integration tests for setting up channels and sending/receiving packets. Currently without the fixes in this pr, main will not pass these tests (solo machine has no integration tests with core IBC and several bugs have historically slipped through because of that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests couldn't be split out because they actually will fail on main (main is currently broken for solo machines)

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

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

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

func (suite *SoloMachineTestSuite) GetSequenceFromStore() uint64 {
bz := suite.store.Get(host.ClientStateKey())
suite.Require().NotNil(bz)
Expand Down
2 changes: 2 additions & 0 deletions modules/light-clients/07-tendermint/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ func (cs ClientState) Initialize(ctx sdk.Context, _ codec.BinaryCodec, clientSto

// VerifyMembership is a generic proof verification method which verifies a proof of the existence of a value at a given CommitmentPath at the specified height.
// The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
// If a zero proof height is passed in, it will fail to retrieve the associated consensus state.
func (cs ClientState) VerifyMembership(
ctx sdk.Context,
clientStore sdk.KVStore,
Expand Down Expand Up @@ -248,6 +249,7 @@ func (cs ClientState) VerifyMembership(

// VerifyNonMembership is a generic proof verification method which verifies the absence of a given CommitmentPath at a specified height.
// The caller is expected to construct the full CommitmentPath from a CommitmentPrefix and a standardized path (as defined in ICS 24).
// If a zero proof height is passed in, it will fail to retrieve the associated consensus state.
func (cs ClientState) VerifyNonMembership(
ctx sdk.Context,
clientStore sdk.KVStore,
Expand Down
Loading