Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 11, 2021
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e543542
fix solo machine bug
colin-axner May 5, 2021
7a17b22
add comments
colin-axner May 6, 2021
98e9dab
fix wording
colin-axner May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions spec/client/ics-006-solo-machine-client/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ function verifyClientState(
clientIdentifier: Identifier,
counterpartyClientState: ClientState) {
path = applyPrefix(prefix, "clients/{clientIdentifier}/clientState")
// ICS 003 will not increment the proof height after connection verification
// the solo machine client must increment the proof height to ensure it matches
// the expected sequence used in the signature
abortTransactionUnless(height + 1 == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + counterpartyClientState
Expand All @@ -203,6 +207,10 @@ function verifyClientConsensusState(
consensusStateHeight: uint64,
consensusState: ConsensusState) {
path = applyPrefix(prefix, "clients/{clientIdentifier}/consensusState/{consensusStateHeight}")
// 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)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +210 to +213
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 •
edited
Loading

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

abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + consensusState
Expand All @@ -219,6 +227,7 @@ function verifyConnectionState(
connectionIdentifier: Identifier,
connectionEnd: ConnectionEnd) {
path = applyPrefix(prefix, "connection/{connectionIdentifier}")
abortTransactionUnless(height == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + connectionEnd
Expand All @@ -236,6 +245,7 @@ function verifyChannelState(
channelIdentifier: Identifier,
channelEnd: ChannelEnd) {
path = applyPrefix(prefix, "ports/{portIdentifier}/channels/{channelIdentifier}")
abortTransactionUnless(height == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + channelEnd
Expand All @@ -254,6 +264,7 @@ function verifyPacketData(
sequence: uint64,
data: bytes) {
path = applyPrefix(prefix, "ports/{portIdentifier}/channels/{channelIdentifier}/packets/{sequence}")
abortTransactionUnless(height == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + data
Expand All @@ -272,6 +283,7 @@ function verifyPacketAcknowledgement(
sequence: uint64,
acknowledgement: bytes) {
path = applyPrefix(prefix, "ports/{portIdentifier}/channels/{channelIdentifier}/acknowledgements/{sequence}")
abortTransactionUnless(height == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + acknowledgement
Expand All @@ -289,6 +301,7 @@ function verifyPacketReceiptAbsence(
channelIdentifier: Identifier,
sequence: uint64) {
path = applyPrefix(prefix, "ports/{portIdentifier}/channels/{channelIdentifier}/receipts/{sequence}")
abortTransactionUnless(height == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path
Expand All @@ -306,6 +319,7 @@ function verifyNextSequenceRecv(
channelIdentifier: Identifier,
nextSequenceRecv: uint64) {
path = applyPrefix(prefix, "ports/{portIdentifier}/channels/{channelIdentifier}/nextSequenceRecv")
abortTransactionUnless(height == clientState.consensusState.sequence)
abortTransactionUnless(!clientState.frozen)
abortTransactionUnless(proof.timestamp >= clientState.consensusState.timestamp)
value = clientState.consensusState.sequence + clientState.consensusState.diversifier + proof.timestamp + path + nextSequenceRecv
Expand Down