Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix solo machine proof height sequence mismatch in connection handshake verification #570

Merged
merged 3 commits into from
May 11, 2021

Conversation

colin-axner
Copy link
Contributor

ref: #562

Addresses one problem outlined in the issue above. Enforces that the proof height is equivalent to the current client state sequence.
NOTE: VerifyClientState is missing from ICS 03-connection as noted in #525. It is assumed that it will be inserted after VerifyConnection and before VerifyClientConsensusState

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

We should explain the height + 2 bit

Comment on lines +210 to +213
// ICS 003 will not increment the proof height after connection or client state verification
// the solo machine client must increment the proof height by 2 to ensure it matches
// the expected sequence used in the signature
abortTransactionUnless(height + 2 == clientState.consensusState.sequence)
Copy link
Member

Choose a reason for hiding this comment

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

Is verify client consensus state always going to be called after 2 previous signature checks? It seems weird to hard code this delta in the verification function itself.

Copy link
Contributor Author

@colin-axner colin-axner May 7, 2021

Choose a reason for hiding this comment

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

Did you see the discussion in #562? I agree it is strange to hard code the delta in the verify function and that we cannot guarantee client consensus state verification will always occur in this order , but I don't see a more elegant solution. The proof height incrementing for each verification call is unique to the solo machine and thus cannot be brought into core IBC. Non-unique sequences is misbehaviour for the solo machine. Proof height may be meaningful to core IBC now or later and thus needs to be verified

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

ACK

@cwgoes cwgoes merged commit d4e7af6 into master May 11, 2021
@cwgoes cwgoes deleted the colin/562-sm-sequence-fixes branch May 11, 2021 21:01
@cwgoes
Copy link
Contributor

cwgoes commented May 11, 2021

(merging with requisite 2/3 quorum from IBC spec committee)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants