Enforce solo machine signature type uniqueness #7394

colin-axner · 2020-09-25T12:36:10Z

Description

closes: #7277

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…-solo-uniqueness

codecov · 2020-09-25T12:41:14Z

Codecov Report

Merging #7394 into master will increase coverage by 0.07%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #7394      +/-   ##
==========================================
+ Coverage   54.99%   55.07%   +0.07%     
==========================================
  Files         588      588              
  Lines       36664    36760      +96     
==========================================
+ Hits        20163    20245      +82     
- Misses      14409    14417       +8     
- Partials     2092     2098       +6

…-solo-uniqueness

Adjusts SignatureAndData proto definition to take in a DataType. Updates misbehaviour basic validation to do checks on the data type. Adds unmarshaling tests.

…n strange error on testing codec.go

x/ibc/light-clients/solomachine/types/codec.go

Rename CanUnmarshalDataByType -> UnmarshalDataByType. Return a new interface and error. Refactor tests to work. Refactor misbehaviour_handle.go to check unmarshaling of the data and DRY code by separating signature and data checks into its own function. Update godoc.

…-solo-uniqueness

colin-axner

I think this is the cleanest way of handling DataType atm. This is one step closer to converting SignBytes to an interface/Any.

Changing SignBytes to an Any would reduce one marshaling layer and isn't too far of a step now that I have added the Data interface, but it also would make each proto data definition have sequence, timestamp, diversifier, and data type fields which is pretty redundant since we have like 9 types of data definitions. I'm not really sure what is better.

colin-axner · 2020-09-30T10:38:07Z

x/ibc/light-clients/solomachine/types/client_state_test.go

-	clientPrefixedPath := "clients/" + counterpartyClientIdentifier + "/" + host.ClientStatePath()
-	path, err := commitmenttypes.ApplyPrefix(prefix, clientPrefixedPath)
-	suite.Require().NoError(err)
+	path := suite.solomachine.GetClientStatePath(counterpartyClientIdentifier)


I think ideally we abstract this in host for all path type's so the users doesn't need to call ApplyPrefix. I only abstracted this in solo machine because I started to write dup code and then figured I might as well do it for all path types

colin-axner · 2020-09-30T10:41:10Z

x/ibc/light-clients/solomachine/types/misbehaviour_handle.go

+// unmarshaled into the specified data type.
+func verifySignatureAndData(cdc codec.BinaryMarshaler, clientState ClientState, misbehaviour *Misbehaviour, sigAndData *SignatureAndData) error {
+	// ensure data can be unmarshaled to the specified data type
+	if _, err := UnmarshalDataByType(cdc, sigAndData.DataType, sigAndData.Data); err != nil {


this cannot be done in misbehaviour.validate basic because we need a codec that has registered the client state/consensus state interface

colin-axner · 2020-09-30T10:42:14Z

x/ibc/light-clients/solomachine/types/proof.go

@@ -96,6 +99,29 @@ func ClientStateSignBytes(
 	diversifier string,
 	path commitmenttypes.MerklePath, // nolint: interfacer
 	clientState exported.ClientState,
+) ([]byte, error) {
+	dataBz, err := ClientStateDataBytes(cdc, path, clientState)


splitting this is useful testing and external callers

colin-axner · 2020-09-30T10:43:31Z

x/ibc/light-clients/solomachine/types/solomachine.go

 )

 // Interface implementation checks.
 var _, _, _, _ codectypes.UnpackInterfacesMessage = &ClientState{}, &ConsensusState{}, &Header{}, &HeaderData{}

+// Data is an interface used for all the signature data bytes proto definitions.
+type Data interface{}


this is added just so I can return the unmarshaled data in UnmarshalDataByType

colin-axner · 2020-09-30T10:44:12Z

x/ibc/light-clients/solomachine/types/solomachine.go

@@ -28,3 +32,13 @@ func (h Header) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error {
 func (hd HeaderData) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error {
 	return unpacker.UnpackAny(hd.NewPubKey, new(crypto.PubKey))
 }
+
+// UnpackInterfaces implements the UnpackInterfaceMessages.UnpackInterfaces method
+func (csd ClientStateData) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error {


we should do a sweep of all proto structs with Any to ensure these funcs are there. They are hard to debug we you run into the error

x/ibc/light-clients/solomachine/spec/01_concepts.md

x/ibc/light-clients/solomachine/types/codec_test.go

x/ibc/light-clients/solomachine/types/misbehaviour.go

x/ibc/light-clients/solomachine/types/proof.go

Co-authored-by: Federico Kunze <[email protected]>

@fedekunze

apply most of @fedekunze comments.

…-sdk into colin/7277-solo-uniqueness

…-solo-uniqueness

fedekunze

ACK

cwgoes

ACK 🎉

fedekunze · 2020-10-01T11:15:39Z

@colin-axner the async acks PR created some conflicts, can you fix them so it auto merges the PR 🙏

colin-axner · 2020-10-01T11:24:17Z

@colin-axner the async acks PR created some conflicts, can you fix them so it auto merges the PR 🙏

currently working on it. It'd be good to double check my diffs, the conflicts are non-trivial

colin-axner added 2 commits September 25, 2020 14:20

update solo machine proto types to use enum for uniqueness

d4f0c05

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/7277…

9c1f9cf

…-solo-uniqueness

colin-axner mentioned this pull request Sep 25, 2020

Alter solo machine signature types to ensure uniqueness #7277

Closed

colin-axner added 2 commits September 28, 2020 13:49

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/7277…

a68eef7

…-solo-uniqueness

move data type to SignatureAndData

cf62ad4

Adjusts SignatureAndData proto definition to take in a DataType. Updates misbehaviour basic validation to do checks on the data type. Adds unmarshaling tests.

colin-axner mentioned this pull request Sep 28, 2020

ibc: async acknowledgements #7361

Merged

9 tasks

colin-axner added 3 commits September 29, 2020 12:55

split signature bytes creation to allow for function reusing. Stuck o…

d513674

…n strange error on testing codec.go

fix test bug

a6c1d12

merge master

72762c7

colin-axner commented Sep 29, 2020

View reviewed changes

x/ibc/light-clients/solomachine/types/codec.go Outdated Show resolved Hide resolved

colin-axner added 3 commits September 30, 2020 12:21

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/7277…

e555694

…-solo-uniqueness

add tests to codec_test.go

69734d4

colin-axner commented Sep 30, 2020

View reviewed changes

colin-axner added 2 commits September 30, 2020 12:52

self review + lint

6e40696

update spec

0d2aec6

colin-axner added x/ibc C:Encoding labels Sep 30, 2020

fix lint

a53b041

colin-axner marked this pull request as ready for review September 30, 2020 11:37

colin-axner requested review from aaronc, alessio and alexanderbez as code owners September 30, 2020 11:37

colin-axner requested review from cwgoes, fedekunze and AdityaSripal September 30, 2020 12:08

Merge branch 'master' into colin/7277-solo-uniqueness

72cf4ee

fedekunze reviewed Sep 30, 2020

View reviewed changes

Update x/ibc/light-clients/solomachine/spec/01_concepts.md

e67e578

Co-authored-by: Federico Kunze <[email protected]>

colin-axner and others added 4 commits October 1, 2020 11:02

increase code cov, update spec

3ef0d5c

apply most of @fedekunze comments.

Merge branch 'colin/7277-solo-uniqueness' of github.com:cosmos/cosmos…

8136909

…-sdk into colin/7277-solo-uniqueness

Merge branch 'master' of github.com:cosmos/cosmos-sdk into colin/7277…

65f9bbc

…-solo-uniqueness

format spec

3e8e6c8

colin-axner requested a review from fedekunze October 1, 2020 09:05

format spec

e980068

fedekunze approved these changes Oct 1, 2020

View reviewed changes

fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 1, 2020

cwgoes approved these changes Oct 1, 2020

View reviewed changes

colin-axner added 3 commits October 1, 2020 12:22

merge master

58e891c

make proto

e918ae6

fix merge conflicts

774247b

colin-axner added 2 commits October 1, 2020 13:16

merge master

6657213

make proto

24c6473

fix conflicts

f501df7

mergify bot merged commit a32e2a0 into master Oct 1, 2020

mergify bot deleted the colin/7277-solo-uniqueness branch October 1, 2020 11:40

colin-axner mentioned this pull request Oct 30, 2020

Cleanup keys.go in 24-host #7710

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce solo machine signature type uniqueness #7394

Enforce solo machine signature type uniqueness #7394

colin-axner commented Sep 25, 2020 •

edited

Loading

codecov bot commented Sep 25, 2020 •

edited

Loading

colin-axner left a comment

colin-axner Sep 30, 2020

colin-axner Sep 30, 2020

colin-axner Sep 30, 2020

colin-axner Sep 30, 2020

colin-axner Sep 30, 2020

fedekunze left a comment

cwgoes left a comment

fedekunze commented Oct 1, 2020

colin-axner commented Oct 1, 2020 •

edited

Loading

Enforce solo machine signature type uniqueness #7394

Enforce solo machine signature type uniqueness #7394

Conversation

colin-axner commented Sep 25, 2020 • edited Loading

Description

codecov bot commented Sep 25, 2020 • edited Loading

Codecov Report

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner Sep 30, 2020

Choose a reason for hiding this comment

colin-axner Sep 30, 2020

Choose a reason for hiding this comment

colin-axner Sep 30, 2020

Choose a reason for hiding this comment

colin-axner Sep 30, 2020

Choose a reason for hiding this comment

colin-axner Sep 30, 2020

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze commented Oct 1, 2020

colin-axner commented Oct 1, 2020 • edited Loading

colin-axner commented Sep 25, 2020 •

edited

Loading

codecov bot commented Sep 25, 2020 •

edited

Loading

colin-axner commented Oct 1, 2020 •

edited

Loading