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

Zero state bug #760

Merged
merged 3 commits into from
Jun 1, 2022
Merged

Zero state bug #760

merged 3 commits into from
Jun 1, 2022

Conversation

agouin
Copy link
Member

@agouin agouin commented May 31, 2022

Relay packet flows in one direction even if there are errors with the other direction.

Replaces error groups with wait groups so that relaying can proceed for packet flows from src to dst if there are errors for queries from dst to src, for example.

Reason for this change:

  • We are using the ibc-go imported module: chantypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
  • chantypes.QueryClient has a method PacketAcknowledgements which we are using to query the packet commitments on the destination chain in order to assemble MsgAcknowledgement messages to send back to the source chain.
  • The PacketAcknowledgements query will return nil error, and also nil response, if no MsgTransfer messages have been committed to the chain.
  • This condition was being transformed into a custom error in the relayer, errQueryUnrelayedPacketAcks.
  • The relayer method making these queries, UnrelayedAcknowledgements, was using an error group to make this query on both chains. If this error occurred on one chain involved in the path, it would return the error and refuse to continue with a success for the other chain.
  • This brought up the idea of replacing the error groups in both UnrelayedAcknowledgements and UnrelayedSequences with wait groups, and only proceeding for the non-error condition packet flows. This allows packet flows in one direction to continue to be processed even if the other chain has an error with one of the queries throughout the process.
  • The errQueryUnrelayedPacketAcks is still a valid error, since we cannot query packet commitments on a chain that does not have any MsgTransfer messages sent yet. But, it does not need to halt the processing of the packet flows in the opposite direction.

@agouin agouin requested review from DavidNix and joeabbey May 31, 2022 19:59
Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

A couple small style questions but overall looks ready to ship.

relayer/naive-strategy.go Outdated Show resolved Hide resolved
relayer/naive-strategy.go Outdated Show resolved Hide resolved
relayer/naive-strategy.go Show resolved Hide resolved
relayer/naive-strategy.go Outdated Show resolved Hide resolved
relayer/naive-strategy.go Outdated Show resolved Hide resolved
relayer/naive-strategy.go Show resolved Hide resolved
relayer/naive-strategy.go Show resolved Hide resolved
@jackzampolin jackzampolin merged commit 76c890d into main Jun 1, 2022
@jackzampolin jackzampolin deleted the andrew/relay_one_if_other_has_error branch June 1, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants