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

Add return type to (*RelayMsgs).Send() #667

Merged
merged 2 commits into from
Apr 4, 2022

Conversation

mark-rushakoff
Copy link
Member

This adds a new return type indicating the accumulation of errors
encountered during message sending and the count of successful batches
sent. The return value is not yet used anywhere and existing behavior is
preserved so far.

There are two questionable existing behaviors:

  1. On the first part of the batching, RelayMsgs.Success is &&-ed with
    the newly received success, and when sending the "leftover" messages,
    the Success field is overwritten to the final value. This means we
    could report success if all early batches failed but only the last
    batch to the destination chain succeeded.

    I intend to address this in a following change by adding an
    equivalent Success() method to the SendMsgsResult type which properly
    reports if all sent batches succeeded.

    I am not sure how this will differ from existing behavior in the
    wild. I assume we will see more failures than before.

  2. It is unclear to me, when there are multiple batches to be sent, and
    one batch fails, is it safe to send a following batch, or should the
    entire send operation abort? I don't yet have a thorough
    understanding of what will be sent here to judge for myself which is
    more appropriate.

/cc @jackzampolin and @jtieri for those behavior questions.

This adds a new return type indicating the accumulation of errors
encountered during message sending and the count of successful batches
sent. The return value is not yet used anywhere and existing behavior is
preserved so far.

There are two questionable existing behaviors:

1. On the first part of the batching, RelayMsgs.Success is &&-ed with
   the newly received success, and when sending the "leftover" messages,
   the Success field is overwritten to the final value. This means we
   could report success if all early batches failed but only the last
   batch to the destination chain succeeded.

   I intend to address this in a following change by adding an
   equivalent Success() method to the SendMsgsResult type which properly
   reports if all sent batches succeeded.

   I am not sure how this will differ from existing behavior in the
   wild. I assume we will see more failures than before.

2. It is unclear to me, when there are multiple batches to be sent, and
   one batch fails, is it safe to send a following batch, or should the
   entire send operation abort? I don't yet have a thorough
   understanding of what will be sent here to judge for myself which is
   more appropriate.
@jtieri
Copy link
Member

jtieri commented Apr 4, 2022

Definitely some good questions.

  1. After reading through this it's a bit unclear to me as well. It looks like we treat full batches as successful only if all preceding full batches were successful but partial batches can be considered successful regardless of if preceding full batches were successful. I'm not very sure why this would be desirable behavior.
    To further add to the confusion, it looks like when we consume the Succeeded variable at the call sites of Send() we just assume every pending packet was successful if we get a true value for Succeeded.

if msgs.Send(ctx, log, AsRelayMsgSender(src), AsRelayMsgSender(dst)); msgs.Success() {
if len(msgs.Dst) > 1 {
dst.logPacketsRelayed(src, len(msgs.Dst)-1, srcChannel)
}
if len(msgs.Src) > 1 {
src.logPacketsRelayed(dst, len(msgs.Src)-1, srcChannel)
}
}

  1. I believe if the channel is UNORDERED then we can safely send another batch of msgs even if a previous one failed, since there is no guarantee on what order the packets in the queue will be successfully processed. If the channel is ORDERED the packets must be successfully processed in FIFO order, so essentially if a batch failed we would need to retry or query the packet commitments again and ensure that packet n was processed before packet n+1

I'd like to see @jackzampolin's thoughts on these bits as well.

@mark-rushakoff
Copy link
Member Author

I'm going to merge this PR now since it doesn't change any existing behavior, but still +1 to hear from @jackzampolin on the questions.

@mark-rushakoff mark-rushakoff merged commit 84eefb4 into main Apr 4, 2022
@mark-rushakoff mark-rushakoff deleted the refactor/relay-msgs-send-result branch April 4, 2022 18:26
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.

2 participants