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.

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

MsgTransfer fails for solo machine client #209

Closed
3 tasks
devashishdxt opened this issue Jun 3, 2021 · 2 comments · Fixed by #214
Closed
3 tasks

MsgTransfer fails for solo machine client #209

devashishdxt opened this issue Jun 3, 2021 · 2 comments · Fixed by #214
Assignees
Labels
type: bug Something isn't working as expected
Milestone

Comments

@devashishdxt
Copy link

devashishdxt commented Jun 3, 2021

Summary of Bug

In the handling of MsgTransfer, the code calls GetTimestampAtHeight() (here) when sending IBC packet to solo machine. GetTimestampAtHeight() is called with height returned by clientState.GetLatestHeight() (this returns the sequence stored in clientState for solo machine).

As the code increments the sequence at many places and does not store consensusState for new height (or sequence), GetClientConsensusState() fails (which is called inside GetTimestampAtHeight()). I've tried sending MsgUpdateClient before sending MsgTransfer but, even in the handling of MsgUpdateClient, the code increments the sequence of clientState (making it header.sequence + 1) and stores consensusState for header.sequence.

So, at all times, there seems to be is no way of getting consensusState for clientState.GetLatestHeight().

Version

master

Steps to Reproduce

  1. Create a solo machine client on IBC enabled chain (using https://github.com/devashishdxt/ibc-solo-machine).
  2. Send MsgUpdateClient and then MsgTransfer.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner
Copy link
Contributor

Thanks @devashishdx for testing the solo machine! GetTimestampAtHeight() is quite problematic for solo machines and is also blocking timeouts from working

We have a long term solution in mind for how to fix this effectively, but this requires a bit of work and due to the current resource constraints, I don't expect a fix until earliest sometime in the late fall

The only short term solution I can think of is hard coding a bypass check for solo machines. That is, if the client is a solo machine, skip the timeout check

The timeout is also a defensive check, but I think it is nice to have. Pinging @cwgoes

cc @AdityaSripal

@cwgoes
Copy link
Contributor

cwgoes commented Jun 3, 2021

This check is defensive and can safely be skipped for solo machines. I suppose we could store past timestamps along with sequence numbers, but that adds extra storage cost for a defensive-only check - not desirable.

If we add a bypass for solo machines now, in the future we should change the abstraction to make it less awkward.

@colin-axner colin-axner self-assigned this Jun 3, 2021
@colin-axner colin-axner added the type: bug Something isn't working as expected label Jun 3, 2021
@colin-axner colin-axner added this to the 1.0.0 milestone Jun 3, 2021
faddat referenced this issue in notional-labs/ibc-go Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants