Merge PR #707: Concurrently create clients in CreateClients

This saves between a quarter and a half second in integration tests that create clients. A small but easy win. Also fix some error handling in a few tx commands that were previously checking the modified flag before checking the error. Co-authored-by: Justin Tieri <[email protected]>
cosmos · Apr 13, 2022 · cd39de3 · cd39de3
1 parent 3890f2d
commit cd39de3
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 34 deletions.
diff --git a/cmd/tx.go b/cmd/tx.go
@@ -142,13 +142,16 @@ func createClientsCmd(a *appState) *cobra.Command {
 			}
 
 			modified, err := c[src].CreateClients(cmd.Context(), c[dst], allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
+			if err != nil {
+				return err
+			}
 			if modified {
 				if err := a.OverwriteConfig(a.Config); err != nil {
 					return err
 				}
 			}
 
-			return err
+			return nil
 		},
 	}
 
@@ -240,6 +243,9 @@ func createClientCmd(a *appState) *cobra.Command {
 			}
 
 			modified, err := relayer.CreateClient(cmd.Context(), c[src], c[dst], srcUpdateHeader, dstUpdateHeader, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
+			if err != nil {
+				return err
+			}
 			if modified {
 				if err = a.OverwriteConfig(a.Config); err != nil {
 					return err
@@ -376,23 +382,26 @@ $ %s tx conn demo-path --timeout 5s`,
 
 			// ensure that the clients exist
 			modified, err := c[src].CreateClients(cmd.Context(), c[dst], allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
+			if err != nil {
+				return err
+			}
 			if modified {
 				if err := a.OverwriteConfig(a.Config); err != nil {
 					return err
 				}
 			}
+
+			modified, err = c[src].CreateOpenConnections(cmd.Context(), c[dst], retries, to)
 			if err != nil {
 				return err
 			}
-
-			modified, err = c[src].CreateOpenConnections(cmd.Context(), c[dst], retries, to)
 			if modified {
 				if err := a.OverwriteConfig(a.Config); err != nil {
 					return err
 				}
 			}
 
-			return err
+			return nil
 		},
 	}
 
@@ -617,36 +626,36 @@ $ %s tx connect demo-path --src-port transfer --dst-port transfer --order unorde
 
 			// create clients if they aren't already created
 			modified, err := c[src].CreateClients(cmd.Context(), c[dst], allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
+			if err != nil {
+				return fmt.Errorf("error creating clients: %w", err)
+			}
 			if modified {
 				if err := a.OverwriteConfig(a.Config); err != nil {
 					return err
 				}
 			}
-			if err != nil {
-				return fmt.Errorf("error creating clients: %w", err)
-			}
 
 			// create connection if it isn't already created
 			modified, err = c[src].CreateOpenConnections(cmd.Context(), c[dst], retries, to)
+			if err != nil {
+				return fmt.Errorf("error creating connections: %w", err)
+			}
 			if modified {
 				if err := a.OverwriteConfig(a.Config); err != nil {
 					return err
 				}
 			}
-			if err != nil {
-				return fmt.Errorf("error creating connections: %w", err)
-			}
 
 			// create channel if it isn't already created
 			modified, err = c[src].CreateOpenChannels(cmd.Context(), c[dst], retries, to, srcPort, dstPort, order, version, override)
+			if err != nil {
+				return fmt.Errorf("error creating channels: %w", err)
+			}
 			if modified {
 				if err := a.OverwriteConfig(a.Config); err != nil {
 					return err
 				}
 			}
-			if err != nil {
-				return fmt.Errorf("error creating channels: %w", err)
-			}
 
 			return nil
 		},

diff --git a/relayer/client.go b/relayer/client.go
@@ -10,30 +10,28 @@ import (
 	ibcexported "github.com/cosmos/ibc-go/v3/modules/core/exported"
 	"github.com/cosmos/relayer/v2/relayer/provider"
 	"go.uber.org/zap"
+	"golang.org/x/sync/errgroup"
 )
 
 // CreateClients creates clients for src on dst and dst on src if the client ids are unspecified.
 func (c *Chain) CreateClients(ctx context.Context, dst *Chain, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override bool) (bool, error) {
-	var (
-		srcUpdateHeader, dstUpdateHeader ibcexported.Header
-		srch, dsth                       int64
-		modified                         bool
-		err                              error
-	)
-
 	// Query the latest heights on src and dst and retry if the query fails
-	if err = retry.Do(func() error {
+	var srch, dsth int64
+	if err := retry.Do(func() error {
+		var err error
 		srch, dsth, err = QueryLatestHeights(ctx, c, dst)
 		if srch == 0 || dsth == 0 || err != nil {
 			return fmt.Errorf("failed to query latest heights: %w", err)
 		}
-		return err
+		return nil
 	}, retry.Context(ctx), RtyAtt, RtyDel, RtyErr); err != nil {
 		return false, err
 	}
 
 	// Query the light signed headers for src & dst at the heights srch & dsth, retry if the query fails
-	if err = retry.Do(func() error {
+	var srcUpdateHeader, dstUpdateHeader ibcexported.Header
+	if err := retry.Do(func() error {
+		var err error
 		srcUpdateHeader, dstUpdateHeader, err = GetLightSignedHeadersAtHeights(ctx, c, dst, srch, dsth)
 		if err != nil {
 			return err
@@ -55,16 +53,31 @@ func (c *Chain) CreateClients(ctx context.Context, dst *Chain, allowUpdateAfterE
 		return false, err
 	}
 
-	// Create client on src for dst if the client id is unspecified
-	modified, err = CreateClient(ctx, c, dst, srcUpdateHeader, dstUpdateHeader, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
-	if err != nil {
-		return modified, fmt.Errorf("failed to create client on src chain{%s}: %w", c.ChainID(), err)
-	}
+	var modifiedSrc, modifiedDst bool
+	eg, egCtx := errgroup.WithContext(ctx)
+	eg.Go(func() error {
+		var err error
+		// Create client on src for dst if the client id is unspecified
+		modifiedSrc, err = CreateClient(egCtx, c, dst, srcUpdateHeader, dstUpdateHeader, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
+		if err != nil {
+			return fmt.Errorf("failed to create client on src chain{%s}: %w", c.ChainID(), err)
+		}
+		return nil
+	})
 
-	// Create client on dst for src if the client id is unspecified
-	modified, err = CreateClient(ctx, dst, c, dstUpdateHeader, srcUpdateHeader, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
-	if err != nil {
-		return modified, fmt.Errorf("failed to create client on dst chain{%s}: %w", dst.ChainID(), err)
+	eg.Go(func() error {
+		var err error
+		// Create client on dst for src if the client id is unspecified
+		modifiedDst, err = CreateClient(egCtx, dst, c, dstUpdateHeader, srcUpdateHeader, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override)
+		if err != nil {
+			return fmt.Errorf("failed to create client on dst chain{%s}: %w", dst.ChainID(), err)
+		}
+		return nil
+	})
+
+	if err := eg.Wait(); err != nil {
+		// If one completed successfully and the other didn't, we can still report modified.
+		return modifiedSrc || modifiedDst, err
 	}
 
 	c.log.Info(
@@ -75,7 +88,7 @@ func (c *Chain) CreateClients(ctx context.Context, dst *Chain, allowUpdateAfterE
 		zap.String("dst_chain_id", dst.ChainID()),
 	)
 
-	return modified, nil
+	return modifiedSrc || modifiedDst, nil
 }
 
 func CreateClient(ctx context.Context, src, dst *Chain, srcUpdateHeader, dstUpdateHeader ibcexported.Header, allowUpdateAfterExpiry, allowUpdateAfterMisbehaviour, override bool) (bool, error) {

diff --git a/relayer/provider/cosmos/log.go b/relayer/provider/cosmos/log.go
@@ -97,10 +97,13 @@ func getFeePayer(tx *typestx.Tx) string {
 		// We don't need the address conversion; just the sender is all that
 		// GetSigners is doing under the hood anyway.
 		return firstMsg.Sender
-	case *clienttypes.MsgUpdateClient:
+	case *clienttypes.MsgCreateClient:
 		// Without this particular special case, there is a panic in ibc-go
 		// due to the sdk config singleton expecting one bech32 prefix but seeing another.
 		return firstMsg.Signer
+	case *clienttypes.MsgUpdateClient:
+		// Same failure mode as MsgCreateClient.
+		return firstMsg.Signer
 	default:
 		return firstMsg.GetSigners()[0].String()
 	}