implement solo machine client #6267

colin-axner · 2020-05-21T21:54:17Z

Description

closes: #5382

old-pr: #6115

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

…-solo-machine-client

…ommitment type

…-solo-machine-client

colin-axner · 2020-08-21T14:55:15Z

I think a spec directory should still be added, but I don't think they need to block merge of this pr. I probably won't get to it until after finishing proto migration in 07-tendermint + removing some more amino

colin-axner · 2020-08-21T14:57:51Z

proto/ibc/lightclients/solomachine/v1/solomachine.proto

+  // current sequence of the consensus state
+  uint64 sequence = 1;
+  // public key of the solo machine
+  cosmos.base.crypto.v1beta1.PublicKey public_key = 2


this will likely become Any in the near future, but I didn't want to block this pr on the sdk supporting that functionality

colin-axner · 2020-08-21T14:59:05Z

proto/ibc/lightclients/solomachine/v1/solomachine.proto

+}
+
+// Signature contains the signature and the timestamp of the signature.
+message Signature {


would SignatureAndTimestamp be more clear? Other suggestions?

TimestampedSignature?

colin-axner · 2020-08-21T15:01:57Z

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

+	if len(sd.Signature) == 0 {
+		return sdkerrors.Wrap(ErrInvalidSignatureAndData, "signature cannot be empty")
+	}
+	if len(sd.Data) == 0 {


what happens if the solo machine signs over just the sequence @cwgoes ? should I remove this check?

At the moment that would be invalid, so this check is fine

colin-axner · 2020-08-21T15:08:50Z

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

+
+// VerifyClientState verifies a proof of the client state of the running chain
+// stored on the solo machine.
+func (cs ClientState) VerifyClientState(


@cwgoes @AdityaSripal please verify this is correct, it hasn't been updated in the ICS spec yet

At first glance this seems fine. I'll update the spec.

colin-axner · 2020-08-21T15:18:51Z

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

+	consensusState := &ConsensusState{
+		// increment sequence number
+		Sequence:  clientState.ConsensusState.Sequence + 1,
+		PublicKey: header.NewPublicKey,


Timestamp is missing here. Do I need to add timestamp to the header? This is missing on the ICS spec as well

I think it'd make sense to address this in a followup. Would prefer to have this pr merged so all future changes are easier to review and so merge conflicts don't stack up

It won't really be up to date since a header update is only used to change the public key

AdityaSripal

Nice work!! One main issue regarding VerifyClientConsensusState

AdityaSripal · 2020-08-21T18:18:17Z

proto/ibc/lightclients/solomachine/v1/solomachine.proto

+}
+
+// Signature contains the signature and the timestamp of the signature.
+message Signature {


TimestampedSignature?

AdityaSripal · 2020-08-21T18:29:26Z

proto/ibc/lightclients/solomachine/v1/solomachine.proto

+message MsgCreateClient {
+  option (gogoproto.goproto_getters) = false;
+  string         client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""];
+  string         chain_id  = 2 [(gogoproto.moretags) = "yaml:\"chain_id\""];


Why would a solo-machine have a chain-id?

#6267 (comment)

I'm not too convinced of its usefulness which is why I have left off any validation of it (it can be blank)

Oh, my response was just because it seemed we needed to have that function, I don't think it needs to be in the message - where is that function used anyways? There shouldn't need to be client-external reference to a chain ID

AdityaSripal · 2020-08-21T18:31:35Z

x/ibc/02-client/exported/exported.go

@@ -169,14 +176,16 @@ type ClientType byte

 // available client types
 const (
-	Tendermint ClientType = iota + 1 // 1
-	Localhost
+	SoloMachine ClientType = 6


I like this change 👍

AdityaSripal · 2020-08-21T18:35:10Z

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

+func NewClientState(chainID string, consensusState *ConsensusState) *ClientState {
+	return &ClientState{
+		ChainId:        chainID,
+		FrozenHeight:   0,


Easier to understand if this is FrozenSequence

AdityaSripal · 2020-08-21T18:37:04Z

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

+	}
+}
+
+// GetChainID returns an empty string.


doc says it should return empty string and I agree. ClientState shouldn't have ChainID and this should return ""

Agreed (also see above). Sorry for my confusing comment before. Why do we have this interface function in the first place?

It seems there is partial consensus on removing ChainID. cc/ @fedekunze

good question. I don't think it is being used outside of tendermint (but it can remain a tm client state specific func). I think we can just remove it in another pr

opened #7143

AdityaSripal · 2020-08-21T18:44:47Z

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

+		return err
+	}
+
+	data, err := ConsensusStateSignBytes(cdc, sequence, signature.Timestamp, path, cs.ConsensusState)


Suggested change

data, err := ConsensusStateSignBytes(cdc, sequence, signature.Timestamp, path, cs.ConsensusState)

data, err := ConsensusStateSignBytes(cdc, sequence, signature.Timestamp, path, consState)

AdityaSripal · 2020-08-21T18:49:57Z

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

+		)
+	}
+
+	if cs.ConsensusState.GetTimestamp() > signature.Timestamp {


Suggested change

if cs.ConsensusState.GetTimestamp() > signature.Timestamp {

if cs.ConsensusState.GetTimestamp() >= signature.Timestamp {

this is currently correct based on ICS spec

abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)

cc/ @cwgoes

The timestamp needs to be non-decreasing; it does not need to be increasing technically (though usually it will be)

AdityaSripal · 2020-08-21T18:50:39Z

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

+
+// sets the client state to the store
+func setClientState(store sdk.KVStore, cdc codec.BinaryMarshaler, clientState clientexported.ClientState) {
+	bz, err := codec.MarshalAny(cdc, clientState)


nit: You can use clienttypes.MustMarshalClientState

AdityaSripal · 2020-08-21T18:57:18Z

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

+	pubKey := clientState.ConsensusState.GetPubKey()
+
+	// assert that provided signature data are different
+	if bytes.Equal(evidence.SignatureOne.Data, evidence.SignatureTwo.Data) {


Hasn't this check happened in ValidateBasic?

AdityaSripal · 2020-08-21T19:49:14Z

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

+// EvidenceSignBytes returns the sign bytes for verification of misbehaviour.
+//
+// Format: {sequence}{data}
+func EvidenceSignBytes(sequence uint64, data []byte) []byte {


nit: This function can be renamed to be more general

agreed. But I'm not sure what a good replacement would be. I guess I could do SignBytes? but that feels a little too vague. Going to leave for now since it is only used by evidence

cwgoes

Mostly LGTM; see comments. Agreed that most can be addressed in a follow-up.

cwgoes · 2020-08-24T08:24:28Z

proto/ibc/lightclients/solomachine/v1/solomachine.proto

+message MsgCreateClient {
+  option (gogoproto.goproto_getters) = false;
+  string         client_id = 1 [(gogoproto.moretags) = "yaml:\"client_id\""];
+  string         chain_id  = 2 [(gogoproto.moretags) = "yaml:\"chain_id\""];


Oh, my response was just because it seemed we needed to have that function, I don't think it needs to be in the message - where is that function used anyways? There shouldn't need to be client-external reference to a chain ID

cwgoes · 2020-08-24T08:25:20Z

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

+	}
+}
+
+// GetChainID returns an empty string.


Agreed (also see above). Sorry for my confusing comment before. Why do we have this interface function in the first place?

cwgoes · 2020-08-24T08:25:30Z

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

+}
+
+// Validate performs basic validation of the client state fields.
+func (cs ClientState) Validate() error {


Agreed, see above comments.

cwgoes · 2020-08-24T08:27:34Z

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

+
+// VerifyClientState verifies a proof of the client state of the running chain
+// stored on the solo machine.
+func (cs ClientState) VerifyClientState(


At first glance this seems fine. I'll update the spec.

cwgoes · 2020-08-24T08:29:34Z

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

+	return nil
+}
+
+// VerifyClientConsensusState verifies a proof of the consensus state of the


Aye. This is true of all of the verifyXYZ functions; they all verify some state stored by the solo machine.

cwgoes · 2020-08-24T08:30:34Z

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

+	if len(sd.Signature) == 0 {
+		return sdkerrors.Wrap(ErrInvalidSignatureAndData, "signature cannot be empty")
+	}
+	if len(sd.Data) == 0 {


At the moment that would be invalid, so this check is fine

cwgoes · 2020-08-24T08:32:46Z

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

@@ -0,0 +1,217 @@
+package types


I should have thought about this earlier, but I wonder if we should make these all proto structs instead of this kinda-arbitrary bytestring concatenation encoding? cc @warner any thoughts?

This can be deferred to a follow-up, though, since this file is nicely abstracted (thanks)

cwgoes · 2020-08-24T08:33:44Z

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

+	consensusState := &ConsensusState{
+		// increment sequence number
+		Sequence:  clientState.ConsensusState.Sequence + 1,
+		PublicKey: header.NewPublicKey,


It won't really be up to date since a header update is only used to change the public key

…-solo-machine-client

cwgoes

ACK modulo follow-ups

addressed

fedekunze · 2020-08-24T10:37:15Z

Finally! this is awesome 🙌 So excited

colin-axner added 23 commits April 27, 2020 11:54

pause work until client refactor resolved

cbc16d7

continued scaffolding

e045498

add msgs and evidence

c993f95

add update and misbehaviour functionality

6de06a4

implement cli

c7e2b38

various types compile issues

fedb7c8

add sig proof and various bug fixes

066d6b0

added rest routes

57c7ad5

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

f83c1d6

…-solo-machine-client

verification funcs now update sequence number

aa8703b

add sm suite and header test

c70d6ec

msgcreateclient and msgupdateclient tests

5c31eda

add basic evidence test

dcb688b

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

d6de635

…-solo-machine-client

evidence validate basic test

3463dc1

consensus state tests

fa52816

rm accidental file

c163056

add verify con state and channel state, pause work for counterparty c…

9e30000

…ommitment type

client state tests added

480aa6c

update clienttype numbers

afbc501

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

11c5707

…-solo-machine-client

update test added

83805ca

add misbehaviour tests

d459d57

colin-axner added x/ibc S:blocked Status: Blocked labels May 21, 2020

colin-axner added 4 commits May 21, 2020 14:58

update alias

bc4b50a

update cli tx

8b2e924

update import alias

43fb4a2

cleanup code

8032570

colin-axner marked this pull request as ready for review August 21, 2020 14:53

colin-axner requested review from aaronc, alessio, clevinson and jgimeno as code owners August 21, 2020 14:53

colin-axner requested review from AdityaSripal and cwgoes August 21, 2020 14:53

fedekunze approved these changes Aug 21, 2020

View reviewed changes

colin-axner commented Aug 21, 2020

View reviewed changes

AdityaSripal previously requested changes Aug 21, 2020

View reviewed changes

cwgoes suggested changes Aug 24, 2020

View reviewed changes

colin-axner added 6 commits August 24, 2020 10:43

rename Signature to TimestampedSignature

8395379

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

7dfc425

…-solo-machine-client

remove chainID

60a1034

rename FrozenHeight to FrozenSequence

4f7d604

apply verify consensus state changes requested by @AdityaSripal

e939091

remove dup check

30f45c0

colin-axner requested a review from sunnya97 as a code owner August 24, 2020 09:21

colin-axner and others added 2 commits August 24, 2020 11:23

fix typo in proto file comment

ffd3086

Merge branch 'master' into colin/5382-solo-machine-client

871a6a7

colin-axner requested review from cwgoes and AdityaSripal August 24, 2020 09:59

cwgoes approved these changes Aug 24, 2020

View reviewed changes

colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 24, 2020

mergify bot merged commit 46927c3 into master Aug 24, 2020

mergify bot deleted the colin/5382-solo-machine-client branch August 24, 2020 10:06

colin-axner mentioned this pull request Aug 26, 2020

Standardize solo machine signature format in proto definitions #7171

Closed

4 tasks

	data, err := ConsensusStateSignBytes(cdc, sequence, signature.Timestamp, path, cs.ConsensusState)
	data, err := ConsensusStateSignBytes(cdc, sequence, signature.Timestamp, path, consState)

	if cs.ConsensusState.GetTimestamp() > signature.Timestamp {
	if cs.ConsensusState.GetTimestamp() >= signature.Timestamp {

implement solo machine client #6267

implement solo machine client #6267

Conversation

colin-axner commented May 21, 2020 • edited Loading

Description

colin-axner commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze commented Aug 24, 2020

colin-axner commented May 21, 2020 •

edited

Loading

colin-axner Aug 24, 2020 •

edited

Loading