Solo machine proof height verificaiton & timeout bug

[colin-axner]

The height argument in the Verify functions of the solo machine is never used, but the specification doesn't make this clear. An inline comment should be added that this argument is never used and should be discarded

This likely would have avoided implementation issue outlined here

[colin-axner]

Upon further investigation, I'm wondering whether proof height needs to be verified.

Also, I believe the spec incorrectly supports timeouts. In 04-channel timeouts we obtain the timeout timestamp using the proof height, but the solo machine doesn't store consensus states (as it is stored in the client state). It also doesn't set the timestamp until after the proof is verified

[colin-axner]

@cwgoes @AdityaSripal could I get a response on whether proof height needs to be verified? If it doesn't, then I can fix the solo machine implementation and just note for now that timeouts do not work. I believe this is blocking development/usage of the solo machine

[cwgoes]

I do not believe proof height needs to be verified for solo machine, as the solo machine should always be signing in sequence, and must sign something specific in order for the absence of a packet receipt to be checked.

Also, I believe the spec incorrectly supports timeouts. In 04-channel timeouts we obtain the timeout timestamp using the proof height, but the solo machine doesn't store consensus states (as it is stored in the client state). It also doesn't set the timestamp until after the proof is verified

I see - it looks like timestamp-based timeouts will not work for the solo machine unless we alter this logic. It should be possible in principle, though, for solo machines there can be no past proof heights, so it's just the timestamp on the signature itself.

[colin-axner]

I do not believe proof height needs to be verified for solo machine, as the solo machine should always be signing in sequence, and must sign something specific in order for the absence of a packet receipt to be checked.

My only concern is that core IBC has no way of knowing that the current sequence is equivalent to that noted in the proof height. That is, my solo machine could sign sequence 10, pass in a signature which signs 10 and pass in a proof height of 3.

I think disabling timeouts for solo machines is fine, but if we discard the proof height (in solo machine), it'd be best to know that core IBC will not now, or in the future rely on the value of the proof height (outside of timeouts). If this is the case then I think the specs should be updated to indicate that proof height is only to be optionally used by clients in verification

The alternative is to require proof height to be meaningful and structure the verify client/consensus states such that they increase the proof height received by 1 or 2 (depending on their ordering)

I think this question goes a little outside the immediate necessity of fixing the solo machine. What is the scope of proof height and what contracts exist between core IBC and IBC clients in relation to proof height. It may be useful for core IBC to require clients to check proof height in order to enable some future functionality

[cwgoes]

My only concern is that core IBC has no way of knowing that the current sequence is equivalent to that noted in the proof height. That is, my solo machine could sign sequence 10, pass in a signature which signs 10 and pass in a proof height of 3.

Oh - can we just assert that proof height is equal to the sequence? I mean "not verify" in the sense of "doesn't need to be signed over as a separate field" - I think we should probably assert that the sequence is equal to the proof height - if that's not already done, just an oversight on the part of the specification.

I think this question goes a little outside the immediate necessity of fixing the solo machine. What is the scope of proof height and what contracts exist between core IBC and IBC clients in relation to proof height. It may be useful for core IBC to require clients to check proof height in order to enable some future functionality

Yes, I agree. Proof height is a bit of an implicit assumption about the structure and behaviour of clients beyond the explicit interface, if we can make it clearer that would be helpful.

[colin-axner]

Oh - can we just assert that proof height is equal to the sequence? I mean "not verify" in the sense of "doesn't need to be signed over as a separate field" - I think we should probably assert that the sequence is equal to the proof height - if that's not already done, just an oversight on the part of the specification.

The assertion is currently the problem. During connection handshake lets say 3 verify calls happen:

• verifyconnection
• verifyclient
• verifyconsensus

The proof height is set once entering into this handshake and is not incremented throughout. So solo machine would get sequence X for all 3 verify functions despite the fact that verify client and verify consensus might be expected X + 1 and X + 2. It sounds like this +1 and +2 should just be hardcoded?

In which case there should be a spec update as well

[colin-axner]

I will open a pr to fix the +1, +2 parts as that seems like the most sensible solution.

#569 should fix timeouts which can be unsupported for now

[cwgoes]

@colin-axner Can this be closed now?

[colin-axner]

Yes, there's one remaining issue with the solo machine timeouts, but that can be tackled in #569 (I added a note to my comment on that issue)