From b046605befb9ced04a4ddc5151ed9fba4312e54f Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Mon, 3 Apr 2023 15:52:06 +0300 Subject: [PATCH 1/6] fix: allow retries for messages signed by relayer. --- e2e/testsuite/testsuite.go | 56 +++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go index 7fe7acf1322..c1b667b1da4 100644 --- a/e2e/testsuite/testsuite.go +++ b/e2e/testsuite/testsuite.go @@ -60,6 +60,7 @@ type E2ETestSuite struct { grpcClients map[string]GRPCClients paths map[string]path + relayers map[ibc.Wallet]bool logger *zap.Logger DockerClient *dockerclient.Client network string @@ -117,6 +118,12 @@ func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...testcon chainARelayerUser := cosmos.NewWallet(ChainARelayerName, chainAAccountBytes, "", chainA.Config()) chainBRelayerUser := cosmos.NewWallet(ChainBRelayerName, chainBAccountBytes, "", chainB.Config()) + if s.relayers == nil { + s.relayers = make(map[ibc.Wallet]bool) + } + s.relayers[chainARelayerUser] = true + s.relayers[chainBRelayerUser] = true + return chainARelayerUser, chainBRelayerUser } @@ -281,7 +288,17 @@ func (s *E2ETestSuite) BroadcastMessages(ctx context.Context, chain *cosmos.Cosm return factory.WithGas(DefaultGasValue) }) - resp, err := cosmos.BroadcastTx(ctx, broadcaster, user, msgs...) + // Retry the operation a few times if the user signing the transaction is a relayer. (See issue #3264) + var resp sdk.TxResponse + var err error + broadcastFunc := func() (sdk.TxResponse, error) { + return cosmos.BroadcastTx(ctx, broadcaster, user, msgs...) + } + if s.IsRelayer(user) { + resp, err = s.retryNtimes(broadcastFunc, 5) + } else { + resp, err = broadcastFunc() + } if err != nil { return sdk.TxResponse{}, err } @@ -611,6 +628,43 @@ func (s *E2ETestSuite) QueryGranterGrants(ctx context.Context, chain *cosmos.Cos return grants.Grants, nil } +// IsRelayer returns true if the provided wallet is used by a relayer. +func (s *E2ETestSuite) IsRelayer(user ibc.Wallet) bool { + if s.relayers == nil { + return false + } + return s.relayers[user] +} + +// 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"} + var err error + var resp sdk.TxResponse + for i := 0; i < attempts; i++ { + // If the response's raw log doesn't contain any of the allowed prefixes we return, else, we retry. + resp, err = f() + if !containsMessage(resp.RawLog, allowedMessages) { + return resp, err + } + s.T().Logf("retrying tx due to ignored raw log: %s", resp.RawLog) + } + return resp, err +} + +// containsPrefix returns true if the string s contains any of the prefixes in the slice. +func containsMessage(s string, messages []string) bool { + var hasPrefix bool + for _, message := range messages { + if strings.Contains(s, message) { + hasPrefix = true + break + } + } + return hasPrefix +} + // GetIBCToken returns the denomination of the full token denom sent to the receiving channel func GetIBCToken(fullTokenDenom string, portID, channelID string) transfertypes.DenomTrace { return transfertypes.ParseDenomTrace(fmt.Sprintf("%s/%s/%s", portID, channelID, fullTokenDenom)) From d922e820e3f0a0b0338b60a796fb4b6a36e09233 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 5 Apr 2023 11:29:03 +0300 Subject: [PATCH 2/6] Fix review nits. --- e2e/testsuite/testsuite.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go index c1b667b1da4..60793cb7f34 100644 --- a/e2e/testsuite/testsuite.go +++ b/e2e/testsuite/testsuite.go @@ -639,13 +639,13 @@ func (s *E2ETestSuite) IsRelayer(user ibc.Wallet) bool { // 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"} + retryMessages := []string{"account sequence mismatch"} var err error var resp sdk.TxResponse for i := 0; i < attempts; i++ { // If the response's raw log doesn't contain any of the allowed prefixes we return, else, we retry. resp, err = f() - if !containsMessage(resp.RawLog, allowedMessages) { + if !containsMessage(resp.RawLog, retryMessages) { return resp, err } s.T().Logf("retrying tx due to ignored raw log: %s", resp.RawLog) @@ -653,16 +653,16 @@ func (s *E2ETestSuite) retryNtimes(f func() (sdk.TxResponse, error), attempts in return resp, err } -// containsPrefix returns true if the string s contains any of the prefixes in the slice. +// containsMessages returns true if the string s contains any of the messages in the slice. func containsMessage(s string, messages []string) bool { - var hasPrefix bool + var containsText bool for _, message := range messages { if strings.Contains(s, message) { - hasPrefix = true + containsText = true break } } - return hasPrefix + return containsText } // GetIBCToken returns the denomination of the full token denom sent to the receiving channel From 3d8031fac3f5d3ee6f5e75ae715a2170d833da49 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Wed, 5 Apr 2023 12:14:28 +0300 Subject: [PATCH 3/6] Simplify containsMessage. --- e2e/testsuite/testsuite.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go index 60793cb7f34..4da24633593 100644 --- a/e2e/testsuite/testsuite.go +++ b/e2e/testsuite/testsuite.go @@ -648,21 +648,19 @@ func (s *E2ETestSuite) retryNtimes(f func() (sdk.TxResponse, error), attempts in if !containsMessage(resp.RawLog, retryMessages) { return resp, err } - s.T().Logf("retrying tx due to ignored raw log: %s", resp.RawLog) + s.T().Logf("ignoring non deterministic tx: %+v", resp) } return resp, err } // containsMessages returns true if the string s contains any of the messages in the slice. func containsMessage(s string, messages []string) bool { - var containsText bool for _, message := range messages { if strings.Contains(s, message) { - containsText = true - break + return true } } - return containsText + return false } // GetIBCToken returns the denomination of the full token denom sent to the receiving channel From 3c663c8529ad4617ddb19b97d0c740c471727c4c Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Sun, 9 Apr 2023 20:10:09 +0300 Subject: [PATCH 4/6] Address feedback. --- e2e/testsuite/testsuite.go | 49 +++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go index 4da24633593..a2425d3817f 100644 --- a/e2e/testsuite/testsuite.go +++ b/e2e/testsuite/testsuite.go @@ -60,7 +60,7 @@ type E2ETestSuite struct { grpcClients map[string]GRPCClients paths map[string]path - relayers map[ibc.Wallet]bool + relayers relayerMap logger *zap.Logger DockerClient *dockerclient.Client network string @@ -105,6 +105,25 @@ func newPath(chainA, chainB *cosmos.CosmosChain) path { } } +// relayerMap is a mapping from test names to a relayer set for that test. +type relayerMap map[string]map[ibc.Wallet]bool + +// addRelayer adds the given relayer to the relayer set for the given test name. +func (r relayerMap) addRelayer(testName string, relayer ibc.Wallet) { + if _, ok := r[testName]; !ok { + r[testName] = make(map[ibc.Wallet]bool) + } + r[testName][relayer] = true +} + +// containsRelayer returns true if the given relayer is in the relayer set for the given test name. +func (r relayerMap) containsRelayer(testName string, wallet ibc.Wallet) bool { + if relayerSet, ok := r[testName]; ok { + return relayerSet[wallet] + } + return false +} + // GetRelayerUsers returns two ibc.Wallet instances which can be used for the relayer users // on the two chains. func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...testconfig.ChainOptionConfiguration) (ibc.Wallet, ibc.Wallet) { @@ -119,10 +138,10 @@ func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...testcon chainBRelayerUser := cosmos.NewWallet(ChainBRelayerName, chainBAccountBytes, "", chainB.Config()) if s.relayers == nil { - s.relayers = make(map[ibc.Wallet]bool) + s.relayers = make(relayerMap) } - s.relayers[chainARelayerUser] = true - s.relayers[chainBRelayerUser] = true + s.relayers.addRelayer(s.T().Name(), chainARelayerUser) + s.relayers.addRelayer(s.T().Name(), chainBRelayerUser) return chainARelayerUser, chainBRelayerUser } @@ -294,7 +313,8 @@ func (s *E2ETestSuite) BroadcastMessages(ctx context.Context, chain *cosmos.Cosm broadcastFunc := func() (sdk.TxResponse, error) { return cosmos.BroadcastTx(ctx, broadcaster, user, msgs...) } - if s.IsRelayer(user) { + if s.relayers.containsRelayer(s.T().Name(), user) { + // Retry five times, the value of 5 chosen is arbitrary. resp, err = s.retryNtimes(broadcastFunc, 5) } else { resp, err = broadcastFunc() @@ -628,27 +648,22 @@ func (s *E2ETestSuite) QueryGranterGrants(ctx context.Context, chain *cosmos.Cos return grants.Grants, nil } -// IsRelayer returns true if the provided wallet is used by a relayer. -func (s *E2ETestSuite) IsRelayer(user ibc.Wallet) bool { - if s.relayers == nil { - return false - } - return s.relayers[user] -} - // 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. retryMessages := []string{"account sequence mismatch"} - var err error var resp sdk.TxResponse + var err error + // If the response's raw log doesn't contain any of the allowed prefixes we return, else, we retry. for i := 0; i < attempts; i++ { - // If the response's raw log doesn't contain any of the allowed prefixes we return, else, we retry. resp, err = f() - if !containsMessage(resp.RawLog, retryMessages) { + if err != nil { return resp, err } - s.T().Logf("ignoring non deterministic tx: %+v", resp) + if !containsMessage(resp.RawLog, retryMessages) { + return sdk.TxResponse{}, err + } + s.T().Logf("retrying tx due to non deterministic failure: %+v", resp) } return resp, err } From fb515f52b92b548defb3fdbb62e6d5d544bad6e7 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 11 Apr 2023 14:41:38 +0300 Subject: [PATCH 5/6] Return the resp if contains fails. Move relayerMap to the relayer folder. --- e2e/relayer/relayer.go | 19 +++++++++++++++++++ e2e/testsuite/testsuite.go | 33 +++++++-------------------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/e2e/relayer/relayer.go b/e2e/relayer/relayer.go index befcd6d127a..3bc864929bd 100644 --- a/e2e/relayer/relayer.go +++ b/e2e/relayer/relayer.go @@ -58,3 +58,22 @@ func newCosmosRelayer(t *testing.T, tag string, logger *zap.Logger, dockerClient func newHermesRelayer() ibc.Relayer { panic("hermes relayer not yet implemented for interchaintest") } + +// relayerMap is a mapping from test names to a relayer set for that test. +type RelayerMap map[string]map[ibc.Wallet]bool + +// addRelayer adds the given relayer to the relayer set for the given test name. +func (r RelayerMap) AddRelayer(testName string, relayer ibc.Wallet) { + if _, ok := r[testName]; !ok { + r[testName] = make(map[ibc.Wallet]bool) + } + r[testName][relayer] = true +} + +// containsRelayer returns true if the given relayer is in the relayer set for the given test name. +func (r RelayerMap) ContainsRelayer(testName string, wallet ibc.Wallet) bool { + if relayerSet, ok := r[testName]; ok { + return relayerSet[wallet] + } + return false +} diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go index a2425d3817f..812610086fa 100644 --- a/e2e/testsuite/testsuite.go +++ b/e2e/testsuite/testsuite.go @@ -60,7 +60,7 @@ type E2ETestSuite struct { grpcClients map[string]GRPCClients paths map[string]path - relayers relayerMap + relayers relayer.RelayerMap logger *zap.Logger DockerClient *dockerclient.Client network string @@ -105,25 +105,6 @@ func newPath(chainA, chainB *cosmos.CosmosChain) path { } } -// relayerMap is a mapping from test names to a relayer set for that test. -type relayerMap map[string]map[ibc.Wallet]bool - -// addRelayer adds the given relayer to the relayer set for the given test name. -func (r relayerMap) addRelayer(testName string, relayer ibc.Wallet) { - if _, ok := r[testName]; !ok { - r[testName] = make(map[ibc.Wallet]bool) - } - r[testName][relayer] = true -} - -// containsRelayer returns true if the given relayer is in the relayer set for the given test name. -func (r relayerMap) containsRelayer(testName string, wallet ibc.Wallet) bool { - if relayerSet, ok := r[testName]; ok { - return relayerSet[wallet] - } - return false -} - // GetRelayerUsers returns two ibc.Wallet instances which can be used for the relayer users // on the two chains. func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...testconfig.ChainOptionConfiguration) (ibc.Wallet, ibc.Wallet) { @@ -138,10 +119,10 @@ func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...testcon chainBRelayerUser := cosmos.NewWallet(ChainBRelayerName, chainBAccountBytes, "", chainB.Config()) if s.relayers == nil { - s.relayers = make(relayerMap) + s.relayers = make(relayer.RelayerMap) } - s.relayers.addRelayer(s.T().Name(), chainARelayerUser) - s.relayers.addRelayer(s.T().Name(), chainBRelayerUser) + s.relayers.AddRelayer(s.T().Name(), chainARelayerUser) + s.relayers.AddRelayer(s.T().Name(), chainBRelayerUser) return chainARelayerUser, chainBRelayerUser } @@ -313,7 +294,7 @@ func (s *E2ETestSuite) BroadcastMessages(ctx context.Context, chain *cosmos.Cosm broadcastFunc := func() (sdk.TxResponse, error) { return cosmos.BroadcastTx(ctx, broadcaster, user, msgs...) } - if s.relayers.containsRelayer(s.T().Name(), user) { + if s.relayers.ContainsRelayer(s.T().Name(), user) { // Retry five times, the value of 5 chosen is arbitrary. resp, err = s.retryNtimes(broadcastFunc, 5) } else { @@ -658,10 +639,10 @@ func (s *E2ETestSuite) retryNtimes(f func() (sdk.TxResponse, error), attempts in for i := 0; i < attempts; i++ { resp, err = f() if err != nil { - return resp, err + return sdk.TxResponse{}, err } if !containsMessage(resp.RawLog, retryMessages) { - return sdk.TxResponse{}, err + return resp, err } s.T().Logf("retrying tx due to non deterministic failure: %+v", resp) } From d3e54a78128d996fb24b8911590a1850697f0f99 Mon Sep 17 00:00:00 2001 From: Jim Fasarakis-Hilliard Date: Tue, 11 Apr 2023 19:33:11 +0300 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Cian Hatton --- e2e/relayer/relayer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/relayer/relayer.go b/e2e/relayer/relayer.go index 3bc864929bd..afa0385fb7c 100644 --- a/e2e/relayer/relayer.go +++ b/e2e/relayer/relayer.go @@ -59,10 +59,10 @@ func newHermesRelayer() ibc.Relayer { panic("hermes relayer not yet implemented for interchaintest") } -// relayerMap is a mapping from test names to a relayer set for that test. +// RelayerMap is a mapping from test names to a relayer set for that test. type RelayerMap map[string]map[ibc.Wallet]bool -// addRelayer adds the given relayer to the relayer set for the given test name. +// AddRelayer adds the given relayer to the relayer set for the given test name. func (r RelayerMap) AddRelayer(testName string, relayer ibc.Wallet) { if _, ok := r[testName]; !ok { r[testName] = make(map[ibc.Wallet]bool)