client identifier reuse #358
Conversation
relayer/client-tx.go
Outdated
| consensusStateResp, err := clientutils.QueryConsensusStateABCI(c.CLIContext(0), identifiedClientState.ClientId, consensusStateHeight) | ||
| if err != nil { | ||
| // consensus state does not exist, try next client | ||
| continue | ||
| } | ||
|
|
||
| if proto.Equal(consensusStateResp.ConsensusState, createClientMsg.ConsensusState) { | ||
| // matching client found | ||
| return identifiedClientState.ClientId, true | ||
| } |
There was a problem hiding this comment.
Doing this makes it unlikely that the consensus state actually matches. We don't want to rely on the ConsensusState in the CreateClientMsg message.
Why can't we just take the latest consensus state for that client state, we can then just check if that consensus state matches the block at the specified height on chain c. If it does, then we can be sure it's a client of the same chain.
There was a problem hiding this comment.
perfect, exactly the feedback I was hoping for. That makes a lot of sense and should actually make client reuse viable and testing it should become trivial
There was a problem hiding this comment.
Ahh it looks like c is the chain that you are sending the CreateClientMsg to. You want to query the header from the other chain, since that is the chain you are creating the client for. Will require a refactor, but still possible.
|
This pull request introduces 1 alert when merging aa55abc into e24e01b - view on LGTM.com new alerts:
|
|
requires upstream fix |
|
tested locally with the upstream fix and it works for clients. I can do connections/channels in a followup if 0.40.1 is cut before then |
There was a problem hiding this comment.
Great work!! Just have some nits.
Also some manual tests I eventually would like done (could help you on it next week):
- if there's an existing client for the chain that has different parameters than desired, then a new client gets created
- if there's a matching client, but consensus state doesn't match. (2 chains with same chain-id), then new client gets created.
Not blocking the merge on these tests though.
Connections and Channel reuse should be easier to implement.
Co-authored-by: Aditya <[email protected]>
Co-authored-by: Aditya <[email protected]>
|
@AdityaSripal good idea, added #383 so I don't forget. Going to merge for now so others can test on these changes and report any issues if they arise |
ref: #349
only handles clients