From 63f3a4fbab5349072970447b54a78fe1751f4de5 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 11 Mar 2022 19:51:53 +0100 Subject: [PATCH 1/9] adding VerifyClientMessage and tests --- .../06-solomachine/types/misbehaviour.go | 6 + .../06-solomachine/types/update.go | 93 +++-- .../06-solomachine/types/update_test.go | 351 ++++++++++++++++++ 3 files changed, 409 insertions(+), 41 deletions(-) diff --git a/modules/light-clients/06-solomachine/types/misbehaviour.go b/modules/light-clients/06-solomachine/types/misbehaviour.go index 31b9b1dc97c..217720294f4 100644 --- a/modules/light-clients/06-solomachine/types/misbehaviour.go +++ b/modules/light-clients/06-solomachine/types/misbehaviour.go @@ -22,6 +22,12 @@ func (misbehaviour Misbehaviour) Type() string { return exported.TypeClientMisbehaviour } +// TODO: Remove GetHeight() when new interface type ClientMessage is introduced +// GetHeight implements the exported.Header interface +func (misbehaviour Misbehaviour) GetHeight() exported.Height { + return nil +} + // ValidateBasic implements Misbehaviour interface. func (misbehaviour Misbehaviour) ValidateBasic() error { if err := host.ClientIdentifierValidator(misbehaviour.ClientId); err != nil { diff --git a/modules/light-clients/06-solomachine/types/update.go b/modules/light-clients/06-solomachine/types/update.go index 3896d2dddec..06d9541e9bb 100644 --- a/modules/light-clients/06-solomachine/types/update.go +++ b/modules/light-clients/06-solomachine/types/update.go @@ -17,61 +17,72 @@ import ( // - the currently registered public key did not provide the update signature func (cs ClientState) CheckHeaderAndUpdateState( ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, - header exported.Header, + msg exported.Header, // TODO: Update to exported.ClientMessage ) (exported.ClientState, exported.ConsensusState, error) { - smHeader, ok := header.(*Header) - if !ok { - return nil, nil, sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, "header type %T, expected %T", header, &Header{}, - ) - } - - if err := checkHeader(cdc, &cs, smHeader); err != nil { + if err := cs.VerifyClientMessage(cdc, msg); err != nil { return nil, nil, err } + smHeader := msg.(*Header) clientState, consensusState := update(&cs, smHeader) return clientState, consensusState, nil } -// checkHeader checks if the Solo Machine update signature is valid. -func checkHeader(cdc codec.BinaryCodec, clientState *ClientState, header *Header) error { - // assert update sequence is current sequence - if header.Sequence != clientState.Sequence { - return sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, - "header sequence does not match the client state sequence (%d != %d)", header.Sequence, clientState.Sequence, - ) - } +// VerifyClientMessage checks if the Solo Machine update signature(s) is valid. +func (cs ClientState) VerifyClientMessage(cdc codec.BinaryCodec, clientMsg exported.Header) error { + switch msg := clientMsg.(type) { + case *Header: + // assert update sequence is current sequence + if msg.Sequence != cs.Sequence { + return sdkerrors.Wrapf( + clienttypes.ErrInvalidHeader, + "header sequence does not match the client state sequence (%d != %d)", msg.Sequence, cs.Sequence, + ) + } - // assert update timestamp is not less than current consensus state timestamp - if header.Timestamp < clientState.ConsensusState.Timestamp { - return sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, - "header timestamp is less than to the consensus state timestamp (%d < %d)", header.Timestamp, clientState.ConsensusState.Timestamp, - ) - } + // assert update timestamp is not less than current consensus state timestamp + if msg.Timestamp < cs.ConsensusState.Timestamp { + return sdkerrors.Wrapf( + clienttypes.ErrInvalidHeader, + "header timestamp is less than to the consensus state timestamp (%d < %d)", msg.Timestamp, cs.ConsensusState.Timestamp, + ) + } - // assert currently registered public key signed over the new public key with correct sequence - data, err := HeaderSignBytes(cdc, header) - if err != nil { - return err - } + // assert currently registered public key signed over the new public key with correct sequence + data, err := HeaderSignBytes(cdc, msg) + if err != nil { + return err + } - sigData, err := UnmarshalSignatureData(cdc, header.Signature) - if err != nil { - return err - } + sigData, err := UnmarshalSignatureData(cdc, msg.Signature) + if err != nil { + return err + } - publicKey, err := clientState.ConsensusState.GetPubKey() - if err != nil { - return err - } + publicKey, err := cs.ConsensusState.GetPubKey() + if err != nil { + return err + } - if err := VerifySignature(publicKey, data, sigData); err != nil { - return sdkerrors.Wrap(ErrInvalidHeader, err.Error()) - } + if err := VerifySignature(publicKey, data, sigData); err != nil { + return sdkerrors.Wrap(ErrInvalidHeader, err.Error()) + } + case *Misbehaviour: + // NOTE: a check that the misbehaviour message data are not equal is done by + // misbehaviour.ValidateBasic which is called by the 02-client keeper. + // verify first signature + if err := verifySignatureAndData(cdc, cs, msg, msg.SignatureOne); err != nil { + return sdkerrors.Wrap(err, "failed to verify signature one") + } + // verify second signature + if err := verifySignatureAndData(cdc, cs, msg, msg.SignatureTwo); err != nil { + return sdkerrors.Wrap(err, "failed to verify signature two") + } + + default: + return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected type %T, got type %T", Header{}, msg) + } return nil } diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index f13d5f198d1..11784ad9215 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -180,3 +180,354 @@ func (suite *SoloMachineTestSuite) TestCheckHeaderAndUpdateState() { } } } + +func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { + var ( + clientState *types.ClientState + clientMsg exported.Header // TODO: Update to ClientMessage interface + ) + + // test singlesig and multisig public keys + for _, solomachine := range []*ibctesting.Solomachine{suite.solomachine, suite.solomachineMulti} { + + testCases := []struct { + name string + setup func() + expPass bool + }{ + { + "successful header", + func() { + clientState = solomachine.ClientState() + clientMsg = solomachine.CreateHeader() + }, + true, + }, + { + "successful misbehaviour", + func() { + clientState = solomachine.ClientState() + clientMsg = solomachine.CreateMisbehaviour() + }, + true, + }, + { + "invalid client message type", + func() { + clientState = solomachine.ClientState() + clientMsg = &ibctmtypes.Header{} + }, + false, + }, + { + "wrong sequence in header", + func() { + clientState = solomachine.ClientState() + // store in temp before assigning to interface type + h := solomachine.CreateHeader() + h.Sequence++ + clientMsg = h + }, + false, + }, + { + "invalid header Signature", + func() { + clientState = solomachine.ClientState() + h := solomachine.CreateHeader() + h.Signature = suite.GetInvalidProof() + clientMsg = h + }, false, + }, + { + "invalid timestamp in header", + func() { + clientState = solomachine.ClientState() + h := solomachine.CreateHeader() + h.Timestamp-- + clientMsg = h + }, false, + }, + { + "signature uses wrong sequence", + func() { + clientState = solomachine.ClientState() + solomachine.Sequence++ + clientMsg = solomachine.CreateHeader() + }, + false, + }, + { + "signature uses new pubkey to sign", + func() { + // store in temp before assinging to interface type + cs := solomachine.ClientState() + h := solomachine.CreateHeader() + + publicKey, err := codectypes.NewAnyWithValue(solomachine.PublicKey) + suite.NoError(err) + + data := &types.HeaderData{ + NewPubKey: publicKey, + NewDiversifier: h.NewDiversifier, + } + + dataBz, err := suite.chainA.Codec.Marshal(data) + suite.Require().NoError(err) + + // generate invalid signature + signBytes := &types.SignBytes{ + Sequence: cs.Sequence, + Timestamp: solomachine.Time, + Diversifier: solomachine.Diversifier, + DataType: types.CLIENT, + Data: dataBz, + } + + signBz, err := suite.chainA.Codec.Marshal(signBytes) + suite.Require().NoError(err) + + sig := solomachine.GenerateSignature(signBz) + suite.Require().NoError(err) + h.Signature = sig + + clientState = cs + clientMsg = h + + }, + false, + }, + { + "signature signs over old pubkey", + func() { + // store in temp before assinging to interface type + cs := solomachine.ClientState() + oldPubKey := solomachine.PublicKey + h := solomachine.CreateHeader() + + // generate invalid signature + data := append(sdk.Uint64ToBigEndian(cs.Sequence), oldPubKey.Bytes()...) + sig := solomachine.GenerateSignature(data) + h.Signature = sig + + clientState = cs + clientMsg = h + }, + false, + }, + { + "consensus state public key is nil", + func() { + cs := solomachine.ClientState() + cs.ConsensusState.PublicKey = nil + clientState = cs + clientMsg = solomachine.CreateHeader() + }, + false, + }, + { + "invalid SignatureOne SignatureData", + func() { + clientState = solomachine.ClientState() + m := solomachine.CreateMisbehaviour() + + m.SignatureOne.Signature = suite.GetInvalidProof() + clientMsg = m + }, false, + }, + { + "invalid SignatureTwo SignatureData", + func() { + clientState = solomachine.ClientState() + m := solomachine.CreateMisbehaviour() + + m.SignatureTwo.Signature = suite.GetInvalidProof() + clientMsg = m + }, false, + }, + { + "invalid SignatureOne timestamp", + func() { + clientState = solomachine.ClientState() + m := solomachine.CreateMisbehaviour() + + m.SignatureOne.Timestamp = 1000000000000 + clientMsg = m + }, false, + }, + { + "invalid SignatureTwo timestamp", + func() { + clientState = solomachine.ClientState() + m := solomachine.CreateMisbehaviour() + + m.SignatureTwo.Timestamp = 1000000000000 + clientMsg = m + }, false, + }, + { + "invalid first signature data", + func() { + clientState = solomachine.ClientState() + + // store in temp before assigning to interface type + m := solomachine.CreateMisbehaviour() + + msg := []byte("DATA ONE") + signBytes := &types.SignBytes{ + Sequence: solomachine.Sequence + 1, + Timestamp: solomachine.Time, + Diversifier: solomachine.Diversifier, + DataType: types.CLIENT, + Data: msg, + } + + data, err := suite.chainA.Codec.Marshal(signBytes) + suite.Require().NoError(err) + + sig := solomachine.GenerateSignature(data) + + m.SignatureOne.Signature = sig + m.SignatureOne.Data = msg + clientMsg = m + }, + false, + }, + { + "invalid second signature data", + func() { + clientState = solomachine.ClientState() + + // store in temp before assigning to interface type + m := solomachine.CreateMisbehaviour() + + msg := []byte("DATA TWO") + signBytes := &types.SignBytes{ + Sequence: solomachine.Sequence + 1, + Timestamp: solomachine.Time, + Diversifier: solomachine.Diversifier, + DataType: types.CLIENT, + Data: msg, + } + + data, err := suite.chainA.Codec.Marshal(signBytes) + suite.Require().NoError(err) + + sig := solomachine.GenerateSignature(data) + + m.SignatureTwo.Signature = sig + m.SignatureTwo.Data = msg + clientMsg = m + }, + false, + }, + { + "wrong pubkey generates first signature", + func() { + clientState = solomachine.ClientState() + badMisbehaviour := solomachine.CreateMisbehaviour() + + // update public key to a new one + solomachine.CreateHeader() + m := solomachine.CreateMisbehaviour() + + // set SignatureOne to use the wrong signature + m.SignatureOne = badMisbehaviour.SignatureOne + clientMsg = m + }, false, + }, + { + "wrong pubkey generates second signature", + func() { + clientState = solomachine.ClientState() + badMisbehaviour := solomachine.CreateMisbehaviour() + + // update public key to a new one + solomachine.CreateHeader() + m := solomachine.CreateMisbehaviour() + + // set SignatureTwo to use the wrong signature + m.SignatureTwo = badMisbehaviour.SignatureTwo + clientMsg = m + }, false, + }, + { + "signatures sign over different sequence", + func() { + clientState = solomachine.ClientState() + + // store in temp before assigning to interface type + m := solomachine.CreateMisbehaviour() + + // Signature One + msg := []byte("DATA ONE") + // sequence used is plus 1 + signBytes := &types.SignBytes{ + Sequence: solomachine.Sequence + 1, + Timestamp: solomachine.Time, + Diversifier: solomachine.Diversifier, + DataType: types.CLIENT, + Data: msg, + } + + data, err := suite.chainA.Codec.Marshal(signBytes) + suite.Require().NoError(err) + + sig := solomachine.GenerateSignature(data) + + m.SignatureOne.Signature = sig + m.SignatureOne.Data = msg + + // Signature Two + msg = []byte("DATA TWO") + // sequence used is minus 1 + + signBytes = &types.SignBytes{ + Sequence: solomachine.Sequence - 1, + Timestamp: solomachine.Time, + Diversifier: solomachine.Diversifier, + DataType: types.CLIENT, + Data: msg, + } + data, err = suite.chainA.Codec.Marshal(signBytes) + suite.Require().NoError(err) + + sig = solomachine.GenerateSignature(data) + + m.SignatureTwo.Signature = sig + m.SignatureTwo.Data = msg + + clientMsg = m + }, + false, + }, + { + "consensus state pubkey is nil", + func() { + cs := solomachine.ClientState() + cs.ConsensusState.PublicKey = nil + clientState = cs + clientMsg = solomachine.CreateMisbehaviour() + }, + false, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + // setup test + tc.setup() + + err := clientState.VerifyClientMessage(suite.chainA.Codec, clientMsg) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } + } +} From d6d9920cdd3c43ad3991e975c399c9d92dab09da Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 11 Mar 2022 21:10:49 +0100 Subject: [PATCH 2/9] splitting tests for verify header and misbehaviour --- .../06-solomachine/types/update_test.go | 100 ++++++++++++------ 1 file changed, 66 insertions(+), 34 deletions(-) diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index 11784ad9215..43380d8efed 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -181,10 +181,10 @@ func (suite *SoloMachineTestSuite) TestCheckHeaderAndUpdateState() { } } -func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { +func (suite *SoloMachineTestSuite) TestVerifyClientMessageHeader() { var ( - clientState *types.ClientState clientMsg exported.Header // TODO: Update to ClientMessage interface + clientState *types.ClientState ) // test singlesig and multisig public keys @@ -198,7 +198,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "successful header", func() { - clientState = solomachine.ClientState() clientMsg = solomachine.CreateHeader() }, true, @@ -206,7 +205,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "successful misbehaviour", func() { - clientState = solomachine.ClientState() clientMsg = solomachine.CreateMisbehaviour() }, true, @@ -214,7 +212,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid client message type", func() { - clientState = solomachine.ClientState() clientMsg = &ibctmtypes.Header{} }, false, @@ -222,7 +219,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "wrong sequence in header", func() { - clientState = solomachine.ClientState() // store in temp before assigning to interface type h := solomachine.CreateHeader() h.Sequence++ @@ -233,7 +229,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid header Signature", func() { - clientState = solomachine.ClientState() h := solomachine.CreateHeader() h.Signature = suite.GetInvalidProof() clientMsg = h @@ -242,7 +237,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid timestamp in header", func() { - clientState = solomachine.ClientState() h := solomachine.CreateHeader() h.Timestamp-- clientMsg = h @@ -251,7 +245,7 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "signature uses wrong sequence", func() { - clientState = solomachine.ClientState() + solomachine.Sequence++ clientMsg = solomachine.CreateHeader() }, @@ -316,19 +310,75 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { false, }, { - "consensus state public key is nil", + "consensus state public key is nil - header", func() { - cs := solomachine.ClientState() - cs.ConsensusState.PublicKey = nil - clientState = cs + clientState.ConsensusState.PublicKey = nil clientMsg = solomachine.CreateHeader() }, false, }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + clientState = solomachine.ClientState() + + // setup test + tc.setup() + + err := clientState.VerifyClientMessage(suite.chainA.Codec, clientMsg) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } + } +} + +func (suite *SoloMachineTestSuite) TestVerifyClientMessageMisbehaviour() { + var ( + clientMsg exported.Header // TODO: Update to ClientMessage interface + clientState *types.ClientState + ) + + // test singlesig and multisig public keys + for _, solomachine := range []*ibctesting.Solomachine{suite.solomachine, suite.solomachineMulti} { + + testCases := []struct { + name string + setup func() + expPass bool + }{ + { + "successful misbehaviour", + func() { + clientMsg = solomachine.CreateMisbehaviour() + }, + true, + }, + { + "invalid client message type", + func() { + clientMsg = &ibctmtypes.Header{} + }, + false, + }, + { + "consensus state pubkey is nil", + func() { + clientState.ConsensusState.PublicKey = nil + clientMsg = solomachine.CreateMisbehaviour() + }, + false, + }, { "invalid SignatureOne SignatureData", func() { - clientState = solomachine.ClientState() m := solomachine.CreateMisbehaviour() m.SignatureOne.Signature = suite.GetInvalidProof() @@ -338,7 +388,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid SignatureTwo SignatureData", func() { - clientState = solomachine.ClientState() m := solomachine.CreateMisbehaviour() m.SignatureTwo.Signature = suite.GetInvalidProof() @@ -348,7 +397,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid SignatureOne timestamp", func() { - clientState = solomachine.ClientState() m := solomachine.CreateMisbehaviour() m.SignatureOne.Timestamp = 1000000000000 @@ -358,7 +406,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid SignatureTwo timestamp", func() { - clientState = solomachine.ClientState() m := solomachine.CreateMisbehaviour() m.SignatureTwo.Timestamp = 1000000000000 @@ -368,8 +415,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid first signature data", func() { - clientState = solomachine.ClientState() - // store in temp before assigning to interface type m := solomachine.CreateMisbehaviour() @@ -396,8 +441,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "invalid second signature data", func() { - clientState = solomachine.ClientState() - // store in temp before assigning to interface type m := solomachine.CreateMisbehaviour() @@ -424,7 +467,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "wrong pubkey generates first signature", func() { - clientState = solomachine.ClientState() badMisbehaviour := solomachine.CreateMisbehaviour() // update public key to a new one @@ -439,7 +481,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "wrong pubkey generates second signature", func() { - clientState = solomachine.ClientState() badMisbehaviour := solomachine.CreateMisbehaviour() // update public key to a new one @@ -454,7 +495,6 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { { "signatures sign over different sequence", func() { - clientState = solomachine.ClientState() // store in temp before assigning to interface type m := solomachine.CreateMisbehaviour() @@ -501,22 +541,14 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessage() { }, false, }, - { - "consensus state pubkey is nil", - func() { - cs := solomachine.ClientState() - cs.ConsensusState.PublicKey = nil - clientState = cs - clientMsg = solomachine.CreateMisbehaviour() - }, - false, - }, } for _, tc := range testCases { tc := tc suite.Run(tc.name, func() { + clientState = solomachine.ClientState() + // setup test tc.setup() From 51465ee562bf9b471e7accceff24aafcc46fa78e Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 11 Mar 2022 21:16:53 +0100 Subject: [PATCH 3/9] rename update to UpdateState --- .../06-solomachine/types/update.go | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/modules/light-clients/06-solomachine/types/update.go b/modules/light-clients/06-solomachine/types/update.go index 06d9541e9bb..eba2e10bcb0 100644 --- a/modules/light-clients/06-solomachine/types/update.go +++ b/modules/light-clients/06-solomachine/types/update.go @@ -23,9 +23,7 @@ func (cs ClientState) CheckHeaderAndUpdateState( return nil, nil, err } - smHeader := msg.(*Header) - clientState, consensusState := update(&cs, smHeader) - return clientState, consensusState, nil + return cs.UpdateState(ctx, cdc, clientStore, msg) } // VerifyClientMessage checks if the Solo Machine update signature(s) is valid. @@ -86,16 +84,19 @@ func (cs ClientState) VerifyClientMessage(cdc codec.BinaryCodec, clientMsg expor return nil } -// update the consensus state to the new public key and an incremented sequence -func update(clientState *ClientState, header *Header) (*ClientState, *ConsensusState) { +// UpdateState updates the consensus state to the new public key and an incremented sequence. +func (cs ClientState) UpdateState( + ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, + header exported.Header, +) (exported.ClientState, exported.ConsensusState, error) { + smHeader := header.(*Header) consensusState := &ConsensusState{ - PublicKey: header.NewPublicKey, - Diversifier: header.NewDiversifier, - Timestamp: header.Timestamp, + PublicKey: smHeader.NewPublicKey, + Diversifier: smHeader.NewDiversifier, + Timestamp: smHeader.Timestamp, } - // increment sequence number - clientState.Sequence++ - clientState.ConsensusState = consensusState - return clientState, consensusState + cs.Sequence++ + cs.ConsensusState = consensusState + return &cs, consensusState, nil } From 9be83fbe9646447225a409cb56dcf7f7a1f4c691 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 14 Mar 2022 09:14:41 +0100 Subject: [PATCH 4/9] cleaning up error handling --- modules/light-clients/06-solomachine/types/update.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/modules/light-clients/06-solomachine/types/update.go b/modules/light-clients/06-solomachine/types/update.go index 06d9541e9bb..21c6404c4cc 100644 --- a/modules/light-clients/06-solomachine/types/update.go +++ b/modules/light-clients/06-solomachine/types/update.go @@ -23,7 +23,14 @@ func (cs ClientState) CheckHeaderAndUpdateState( return nil, nil, err } - smHeader := msg.(*Header) + // TODO: Remove this type assertion, replace with misbehaviour checking and update state + smHeader, ok := msg.(*Header) + if !ok { + return nil, nil, sdkerrors.Wrapf( + clienttypes.ErrInvalidHeader, "expected %T, got %T", &Header{}, msg, + ) + } + clientState, consensusState := update(&cs, smHeader) return clientState, consensusState, nil } @@ -81,7 +88,7 @@ func (cs ClientState) VerifyClientMessage(cdc codec.BinaryCodec, clientMsg expor } default: - return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected type %T, got type %T", Header{}, msg) + return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type of %T or %T, got type %T", Header{}, Misbehaviour{}, msg) } return nil } From b5af0f8dff2671b47c817ea3e99f561e9ed5bff0 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 14 Mar 2022 11:36:15 +0100 Subject: [PATCH 5/9] adding additional test for old misbehaviour is sucessful --- modules/light-clients/06-solomachine/types/update_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index 43380d8efed..7fdfdabf4d8 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -361,6 +361,14 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageMisbehaviour() { }, true, }, + { + "old misbehaviour is successful (timestamp is less than current consensus state)", + func() { + clientState = solomachine.ClientState() + solomachine.Time = solomachine.Time - 5 + clientMsg = solomachine.CreateMisbehaviour() + }, true, + }, { "invalid client message type", func() { From 95e38d9ab769bac604885be9a47e68ec16ad8b1a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 14 Mar 2022 16:06:04 +0100 Subject: [PATCH 6/9] split type switch logic into verifyHeader and verifyMisbehaviour priv funcs --- .../06-solomachine/types/update.go | 108 ++++++++++-------- .../06-solomachine/types/update_test.go | 4 +- 2 files changed, 61 insertions(+), 51 deletions(-) diff --git a/modules/light-clients/06-solomachine/types/update.go b/modules/light-clients/06-solomachine/types/update.go index 21c6404c4cc..c43fb76c3d3 100644 --- a/modules/light-clients/06-solomachine/types/update.go +++ b/modules/light-clients/06-solomachine/types/update.go @@ -19,7 +19,7 @@ func (cs ClientState) CheckHeaderAndUpdateState( ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, msg exported.Header, // TODO: Update to exported.ClientMessage ) (exported.ClientState, exported.ConsensusState, error) { - if err := cs.VerifyClientMessage(cdc, msg); err != nil { + if err := cs.VerifyClientMessage(ctx, cdc, clientStore, msg); err != nil { return nil, nil, err } @@ -36,60 +36,70 @@ func (cs ClientState) CheckHeaderAndUpdateState( } // VerifyClientMessage checks if the Solo Machine update signature(s) is valid. -func (cs ClientState) VerifyClientMessage(cdc codec.BinaryCodec, clientMsg exported.Header) error { +func (cs ClientState) VerifyClientMessage(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, clientMsg exported.Header) error { switch msg := clientMsg.(type) { case *Header: - // assert update sequence is current sequence - if msg.Sequence != cs.Sequence { - return sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, - "header sequence does not match the client state sequence (%d != %d)", msg.Sequence, cs.Sequence, - ) - } - - // assert update timestamp is not less than current consensus state timestamp - if msg.Timestamp < cs.ConsensusState.Timestamp { - return sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, - "header timestamp is less than to the consensus state timestamp (%d < %d)", msg.Timestamp, cs.ConsensusState.Timestamp, - ) - } - - // assert currently registered public key signed over the new public key with correct sequence - data, err := HeaderSignBytes(cdc, msg) - if err != nil { - return err - } - - sigData, err := UnmarshalSignatureData(cdc, msg.Signature) - if err != nil { - return err - } - - publicKey, err := cs.ConsensusState.GetPubKey() - if err != nil { - return err - } - - if err := VerifySignature(publicKey, data, sigData); err != nil { - return sdkerrors.Wrap(ErrInvalidHeader, err.Error()) - } + return cs.verifyHeader(ctx, cdc, clientStore, msg) case *Misbehaviour: - // NOTE: a check that the misbehaviour message data are not equal is done by - // misbehaviour.ValidateBasic which is called by the 02-client keeper. - // verify first signature - if err := verifySignatureAndData(cdc, cs, msg, msg.SignatureOne); err != nil { - return sdkerrors.Wrap(err, "failed to verify signature one") - } - - // verify second signature - if err := verifySignatureAndData(cdc, cs, msg, msg.SignatureTwo); err != nil { - return sdkerrors.Wrap(err, "failed to verify signature two") - } - + return cs.verifyMisbehaviour(ctx, cdc, clientStore, msg) default: return sdkerrors.Wrapf(clienttypes.ErrInvalidClientType, "expected type of %T or %T, got type %T", Header{}, Misbehaviour{}, msg) } +} + +func (cs ClientState) verifyHeader(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, header *Header) error { + // assert update sequence is current sequence + if header.Sequence != cs.Sequence { + return sdkerrors.Wrapf( + clienttypes.ErrInvalidHeader, + "header sequence does not match the client state sequence (%d != %d)", header.Sequence, cs.Sequence, + ) + } + + // assert update timestamp is not less than current consensus state timestamp + if header.Timestamp < cs.ConsensusState.Timestamp { + return sdkerrors.Wrapf( + clienttypes.ErrInvalidHeader, + "header timestamp is less than to the consensus state timestamp (%d < %d)", header.Timestamp, cs.ConsensusState.Timestamp, + ) + } + + // assert currently registered public key signed over the new public key with correct sequence + data, err := HeaderSignBytes(cdc, header) + if err != nil { + return err + } + + sigData, err := UnmarshalSignatureData(cdc, header.Signature) + if err != nil { + return err + } + + publicKey, err := cs.ConsensusState.GetPubKey() + if err != nil { + return err + } + + if err := VerifySignature(publicKey, data, sigData); err != nil { + return sdkerrors.Wrap(ErrInvalidHeader, err.Error()) + } + + return nil +} + +func (cs ClientState) verifyMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, misbehaviour *Misbehaviour) error { + // NOTE: a check that the misbehaviour message data are not equal is done by + // misbehaviour.ValidateBasic which is called by the 02-client keeper. + // verify first signature + if err := verifySignatureAndData(cdc, cs, misbehaviour, misbehaviour.SignatureOne); err != nil { + return sdkerrors.Wrap(err, "failed to verify signature one") + } + + // verify second signature + if err := verifySignatureAndData(cdc, cs, misbehaviour, misbehaviour.SignatureTwo); err != nil { + return sdkerrors.Wrap(err, "failed to verify signature two") + } + return nil } diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index 7fdfdabf4d8..2669447679f 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -328,7 +328,7 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageHeader() { // setup test tc.setup() - err := clientState.VerifyClientMessage(suite.chainA.Codec, clientMsg) + err := clientState.VerifyClientMessage(suite.chainA.GetContext(), suite.chainA.Codec, nil, clientMsg) if tc.expPass { suite.Require().NoError(err) @@ -560,7 +560,7 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageMisbehaviour() { // setup test tc.setup() - err := clientState.VerifyClientMessage(suite.chainA.Codec, clientMsg) + err := clientState.VerifyClientMessage(suite.chainA.GetContext(), suite.chainA.Codec, nil, clientMsg) if tc.expPass { suite.Require().NoError(err) From de51d0a9feebee8821eb4dd8a09b3a81c774d1e2 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 14 Mar 2022 17:02:58 +0100 Subject: [PATCH 7/9] adding in place test function for solomachine UpdateState --- .../06-solomachine/types/update_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index 43380d8efed..16bb80c95a9 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -563,3 +563,56 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageMisbehaviour() { } } } + +func (suite *SoloMachineTestSuite) TestUpdateState() { + var ( + clientState exported.ClientState + header exported.Header // TODO: Update to ClientMessage interface + ) + + // test singlesig and multisig public keys + for _, solomachine := range []*ibctesting.Solomachine{suite.solomachine, suite.solomachineMulti} { + + testCases := []struct { + name string + setup func() + expPass bool + }{ + { + "successful update", + func() { + clientState = solomachine.ClientState() + header = solomachine.CreateHeader() + }, + true, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + // setup test + tc.setup() + + clientState, ok := clientState.(*types.ClientState) + if ok { + cs, consensusState, err := clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, header) + + if tc.expPass { + suite.Require().NoError(err) + suite.Require().Equal(header.(*types.Header).NewPublicKey, cs.(*types.ClientState).ConsensusState.PublicKey) + suite.Require().Equal(false, cs.(*types.ClientState).IsFrozen) + suite.Require().Equal(header.(*types.Header).Sequence+1, cs.(*types.ClientState).Sequence) + suite.Require().Equal(consensusState, cs.(*types.ClientState).ConsensusState) + } else { + suite.Require().Error(err) + suite.Require().Nil(clientState) + suite.Require().Nil(consensusState) + } + } + + }) + } + } +} From 7fed804bcb6610a51ed2136148af90297613ea75 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 11 Mar 2022 21:16:53 +0100 Subject: [PATCH 8/9] rename update to UpdateState --- .../06-solomachine/types/update.go | 32 ++++++++----------- .../06-solomachine/types/update_test.go | 4 +-- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/modules/light-clients/06-solomachine/types/update.go b/modules/light-clients/06-solomachine/types/update.go index d59e5edb471..a8b1b4e8039 100644 --- a/modules/light-clients/06-solomachine/types/update.go +++ b/modules/light-clients/06-solomachine/types/update.go @@ -23,16 +23,7 @@ func (cs ClientState) CheckHeaderAndUpdateState( return nil, nil, err } - // TODO: Remove this type assertion, replace with misbehaviour checking and update state - smHeader, ok := msg.(*Header) - if !ok { - return nil, nil, sdkerrors.Wrapf( - clienttypes.ErrInvalidHeader, "expected %T, got %T", &Header{}, msg, - ) - } - - clientState, consensusState := update(&cs, smHeader) - return clientState, consensusState, nil + return cs.UpdateState(ctx, cdc, clientStore, msg) } // VerifyClientMessage checks if the Solo Machine update signature(s) is valid. @@ -103,16 +94,19 @@ func (cs ClientState) verifyMisbehaviour(ctx sdk.Context, cdc codec.BinaryCodec, return nil } -// update the consensus state to the new public key and an incremented sequence -func update(clientState *ClientState, header *Header) (*ClientState, *ConsensusState) { +// UpdateState updates the consensus state to the new public key and an incremented sequence. +func (cs ClientState) UpdateState( + ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, + clientMsg exported.ClientMessage, +) (exported.ClientState, exported.ConsensusState, error) { + smHeader := clientMsg.(*Header) consensusState := &ConsensusState{ - PublicKey: header.NewPublicKey, - Diversifier: header.NewDiversifier, - Timestamp: header.Timestamp, + PublicKey: smHeader.NewPublicKey, + Diversifier: smHeader.NewDiversifier, + Timestamp: smHeader.Timestamp, } - // increment sequence number - clientState.Sequence++ - clientState.ConsensusState = consensusState - return clientState, consensusState + cs.Sequence++ + cs.ConsensusState = consensusState + return &cs, consensusState, nil } diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index 336b9926be0..4d971632348 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -328,7 +328,7 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageHeader() { // setup test tc.setup() - err := clientState.VerifyClientMessage(suite.chainA.GetContext(), suite.chainA.Codec, nil, clientMsg) + err := clientState.VerifyClientMessage(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg) if tc.expPass { suite.Require().NoError(err) @@ -560,7 +560,7 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageMisbehaviour() { // setup test tc.setup() - err := clientState.VerifyClientMessage(suite.chainA.GetContext(), suite.chainA.Codec, nil, clientMsg) + err := clientState.VerifyClientMessage(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, clientMsg) if tc.expPass { suite.Require().NoError(err) From 88280753d9c02bf64ad99f320e7b675637a7d203 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 14 Mar 2022 17:02:58 +0100 Subject: [PATCH 9/9] adding in place test function for solomachine UpdateState --- .../06-solomachine/types/update_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/modules/light-clients/06-solomachine/types/update_test.go b/modules/light-clients/06-solomachine/types/update_test.go index 4d971632348..df04f1775ba 100644 --- a/modules/light-clients/06-solomachine/types/update_test.go +++ b/modules/light-clients/06-solomachine/types/update_test.go @@ -571,3 +571,56 @@ func (suite *SoloMachineTestSuite) TestVerifyClientMessageMisbehaviour() { } } } + +func (suite *SoloMachineTestSuite) TestUpdateState() { + var ( + clientState exported.ClientState + header exported.Header // TODO: Update to ClientMessage interface + ) + + // test singlesig and multisig public keys + for _, solomachine := range []*ibctesting.Solomachine{suite.solomachine, suite.solomachineMulti} { + + testCases := []struct { + name string + setup func() + expPass bool + }{ + { + "successful update", + func() { + clientState = solomachine.ClientState() + header = solomachine.CreateHeader() + }, + true, + }, + } + + for _, tc := range testCases { + tc := tc + + suite.Run(tc.name, func() { + // setup test + tc.setup() + + clientState, ok := clientState.(*types.ClientState) + if ok { + cs, consensusState, err := clientState.UpdateState(suite.chainA.GetContext(), suite.chainA.Codec, suite.store, header) + + if tc.expPass { + suite.Require().NoError(err) + suite.Require().Equal(header.(*types.Header).NewPublicKey, cs.(*types.ClientState).ConsensusState.PublicKey) + suite.Require().Equal(false, cs.(*types.ClientState).IsFrozen) + suite.Require().Equal(header.(*types.Header).Sequence+1, cs.(*types.ClientState).Sequence) + suite.Require().Equal(consensusState, cs.(*types.ClientState).ConsensusState) + } else { + suite.Require().Error(err) + suite.Require().Nil(clientState) + suite.Require().Nil(consensusState) + } + } + + }) + } + } +}