Fix solo machine handshake verification bug

[colin-axner]

Description

closes: #119

---

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

[colin-axner]

I don't know of an efficient way to test this in an end to end manner so I opened #121. We definitely need end to end tests for this, but I don't see a reason to block this fix unless someone is unconvinced this will fix the issue

[AdityaSripal]

utACK, this looks correct to me though it means that the solomachine must sign each message with a new sequence in the correct order during the handshake.

So, solomachine must sign connection with sequence 0, clientstate with sequence 1, and consensus state with sequence 2 in order to successfully pass ConnOpenAck.

This should be documented somewhere

[devashishdxt]

@colin-axner Thanks for a quick fix. But, I'm still a bit unsure of how this fix solves the issue. I understand that solo machine has to increment the sequence after creating a proof. That means that a sequence/height should be passed along with each proof, but in ConnectionOpenAck (or any other call), all the three proofs are checked against proofHeight in produceVerificationArgs.

Maybe I'm missing something, but, shouldn't we send a sequence number associated with each proof.

NOTE: SignBytes does contain that sequence number but implementation verifies sequence against proofHeight and not the sequence number in SignBytes.

[colin-axner]

@devashishdxt You are 100% correct that this didn't fix the issue, the proof height being passed in by the connection keeper should be discarded and we should only use the client state lastest sequence as done in the ICS spec

The proof height presumes that all the proofs are produced at the same height, but in the case of solo machines, this would be misbehaviour

I'll open a fix shortly. Thanks a ton for paying careful attention!! Hopefully end to end tests can mitigate these issues in the future

I'm also very interested in developer feedback for improving interaction with the solo machine since it was largely designed in a black box. Feel free to open any issues with suggestions!

[colin-axner]

Actually upon further inspection, I believe there are several issues at play here. Timeout logic uses the proof height to obtain the consensus state associated with that height to check the timestamp. Solo machines do not set consensus states (as it is stored in the client state), this would result in all timeouts failing (this would be a bug in the spec as well)

It is unclear to me based on the spec, if the proof height needs to be verified or not. Can we assume core IBC doesn't rely on the height of the proof height (outside of the timeout logic mentioned above)? I will discuss this with the spec folks and see if we can come to some resolution