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

fix: allow retries for messages signed by relayer. #3402

Conversation

DimitrisJim
Copy link
Contributor

Description

closes: #3264

Commit Message / Changelog Entry

fix: allow retries for messages signed by relayer.

see the guidelines for commit messages. (view raw markdown for examples)


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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @DimitrisJim! It's looking good; I just left a bunch for nits.

// retryNtimes retries the provided function up to the provided number of attempts.
func (s *E2ETestSuite) retryNtimes(f func() (sdk.TxResponse, error), attempts int) (sdk.TxResponse, error) {
// Ignore account sequence mismatch errors.
allowedMessages := []string{"account sequence mismatch"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a configuration parameter, maybe we can move it to the top together with the const? We can make it an unexpected var.

e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

Looking great! Left a few comments/suggestions

@@ -60,6 +60,7 @@ type E2ETestSuite struct {

grpcClients map[string]GRPCClients
paths map[string]path
relayers map[ibc.Wallet]bool
Copy link
Contributor

Choose a reason for hiding this comment

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

while we do currently just run a single test in each run, we should try and make sure that if we change that we need to make as few changes as possible. With relayers being at the top level, this means that these relayer wallets would be shared across all tests. What might make sense to do is to create a mapping of testName -> relayer pair. Similar to how we do here.

This could even be a map[string]map[ibc.Wallet]bool which isn't super pretty, but maybe we could work with an alias of some sort here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did go with an alias here and basically just added a couple of functions on it. Lmk what you think.

e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Excellent work @DimitrisJim! Left a few suggestions, but overall looks great to me! Have we seen this solution work in the logs?

e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
e2e/testsuite/testsuite.go Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks @DimitrisJim 🙂

e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved
@DimitrisJim
Copy link
Contributor Author

@colin-axner re: have we seen it work in the logs. Not so far as far as I can tell, I've searched the logs for the added log "retrying tx due to ..." i.e searching the logs for the log stating the retry) and haven't come up with anything. Unfortunate but can't really do much better when dealing with non-determinism.

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @DimitrisJim

e2e/relayer/relayer.go Outdated Show resolved Hide resolved
e2e/relayer/relayer.go Outdated Show resolved Hide resolved
@DimitrisJim DimitrisJim merged commit f726023 into main Apr 12, 2023
@DimitrisJim DimitrisJim deleted the jim/3264-avoid-sequence-mismatch-when-using-relayer-wallet-to-sign-ibc-tx-msgs branch April 12, 2023 09:00
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.

Avoid sequence mismatch when using relayer wallet to sign ibc tx msgs
4 participants