Repository: cosmos/relayer
Pull request #1007 (2022-09-28 22:59:07)

Fix account sequence mismatch errors
While broadcasting txs on heavily used chains, we frequently see account mismatch errors, often repetitive with the same sequence mismatch:

```
2022-09-28T19:11:43.957745Z     info    Error building or broadcasting transaction      {"provider_type": "cosmos", "chain_id": "axelar-dojo-1", "attempt": 2, "max_attempts": 5, "error": "rpc error: code = InvalidArgument desc = account sequence mismatch, expected 10, got 9: incorrect account sequence: invalid request"}
2022-09-28T19:11:51.252966Z     info    Error building or broadcasting transaction      {"provider_type": "cosmos", "chain_id": "axelar-dojo-1", "attempt": 3, "max_attempts": 5, "error": "rpc error: code = InvalidArgument desc = account sequence mismatch, expected 10, got 9: incorrect account sequence: invalid request"}
2022-09-28T19:11:59.269768Z     info    Error building or broadcasting transaction      {"provider_type": "cosmos", "chain_id": "axelar-dojo-1", "attempt": 4, "max_attempts": 5, "error": "rpc error: code = InvalidArgument desc = account sequence mismatch, expected 10, got 9: incorrect account sequence: invalid request"}
2022-09-28T19:12:06.017468Z     info    Error building or broadcasting transaction      {"provider_type": "cosmos", "chain_id": "axelar-dojo-1", "attempt": 1, "max_attempts": 5, "error": "rpc error: code = InvalidArgument desc = account sequence mismatch, expected 10, got 9: incorrect account sequence: invalid request"}
```

After one of these account sequence errors, it should not try to send another transaction with sequence 9, since it has already been given an error from the node that 10 is expected.

Additionally, the chain provider can guard its own use of the wallet by using a mutex surrounding the one-at-a-time-per-wallet process of:

- Query for account sequence
- Build tx
- Simulate tx to calculate gas
- Sign tx
- Broadcast tx

agouin: > is there any way to query the expected sequence number associated with a particular address?

This query is happening with `GetAddress` for each transaction as part of the simulated tx, but it can return the wrong number, sometimes over and over again, so the expected sequence in the error helps us get back on track. The increment of the account sequence after a successful transaction also helps to make sure we don't reuse a sequence number from a successful tx.


jtieri: > > is there any way to query the expected sequence number associated with a particular address?
> 
> This query is happening with `GetAddress` for each transaction as part of the simulated tx, but it can return the wrong number, sometimes over and over again, so the expected sequence in the error helps us get back on track. The increment of the account sequence after a successful transaction also helps to make sure we don't reuse a sequence number from a successful tx.

Awesome, thanks for explaining that! Gonna go ahead and approve now 🚢 

Patch:
diff --git a/relayer/chains/cosmos/provider.go b/relayer/chains/cosmos/provider.go
index c1b04f9..4f455d7 100644
--- a/relayer/chains/cosmos/provider.go
+++ b/relayer/chains/cosmos/provider.go
@@ -96,9 +96,12 @@ func ChainClientConfig(pcfg *CosmosProviderConfig) *lens.ChainClientConfig {
 type CosmosProvider struct {
 	log *zap.Logger
 
-	lens.ChainClient
 	PCfg CosmosProviderConfig
 
+	lens.ChainClient
+	nextAccountSeq uint64
+	txMu           sync.Mutex
+
 	// metrics to monitor the provider
 	TotalFees   sdk.Coins
 	totalFeesMu sync.Mutex
@@ -250,3 +253,9 @@ func (cc *CosmosProvider) BlockTime(ctx context.Context, height int64) (time.Tim
 func (cc *CosmosProvider) SetMetrics(m *processor.PrometheusMetrics) {
 	cc.metrics = m
 }
+
+func (cc *CosmosProvider) updateNextAccountSequence(seq uint64) {
+	if seq > cc.nextAccountSeq {
+		cc.nextAccountSeq = seq
+	}
+}
diff --git a/relayer/chains/cosmos/tx.go b/relayer/chains/cosmos/tx.go
index 736ee13..fdd770f 100644
--- a/relayer/chains/cosmos/tx.go
+++ b/relayer/chains/cosmos/tx.go
@@ -5,6 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"math/big"
+	"regexp"
+	"strconv"
 	"strings"
 	"time"
 
@@ -34,6 +36,7 @@ var (
 	rtyAtt    = retry.Attempts(rtyAttNum)
 	rtyDel    = retry.Delay(time.Millisecond * 400)
 	rtyErr    = retry.LastErrorOnly(true)
+	numRegex  = regexp.MustCompile("[0-9]+")
 )
 
 // Default IBC settings
@@ -74,12 +77,23 @@ func (cc *CosmosProvider) SendMessages(ctx context.Context, msgs []provider.Rela
 	var resp *sdk.TxResponse
 	var fees sdk.Coins
 
+	// Guard against account sequence number mismatch errors by locking for the specific wallet for
+	// the account sequence query all the way through the transaction broadcast success/fail.
+	cc.txMu.Lock()
+	defer cc.txMu.Unlock()
+
 	if err := retry.Do(func() error {
-		txBytes, f, err := cc.buildMessages(ctx, msgs, memo)
+		txBytes, sequence, f, err := cc.buildMessages(ctx, msgs, memo)
 		fees = f
 		if err != nil {
 			errMsg := err.Error()
 
+			// Account sequence mismatch errors can happen on the simulated transaction also.
+			if strings.Contains(errMsg, sdkerrors.ErrWrongSequence.Error()) {
+				cc.handleAccountSequenceMismatchError(err)
+				return err
+			}
+
 			// Occasionally the client will be out of date,
 			// and we will receive an RPC error like:
 			//     rpc error: code = InvalidArgument desc = failed to execute message; message index: 1: channel handshake open try failed: failed channel state verification for client (07-tendermint-0): client state height < proof height ({0 58} < {0 59}), please ensure the client has been updated: invalid height: invalid request
@@ -146,16 +160,18 @@ func (cc *CosmosProvider) SendMessages(ctx context.Context, msgs []provider.Rela
 
 		resp, err = cc.BroadcastTx(ctx, txBytes)
 		if err != nil {
-			if err == sdkerrors.ErrWrongSequence {
-				// Allow retrying if we got an invalid sequence error when attempting to broadcast this tx.
+			if strings.Contains(err.Error(), sdkerrors.ErrWrongSequence.Error()) {
+				cc.handleAccountSequenceMismatchError(err)
 				return err
 			}
 
 			// Don't retry if BroadcastTx resulted in any other error.
-			// (This was the previous behavior. Unclear if that is still desired.)
 			return retry.Unrecoverable(err)
 		}
 
+		// we had a successful tx with this sequence, so update it to the next
+		cc.updateNextAccountSequence(sequence + 1)
+
 		return nil
 	}, retry.Context(ctx), rtyAtt, rtyDel, rtyErr, retry.OnRetry(func(n uint, err error) {
 		cc.log.Info(
@@ -214,11 +230,11 @@ func parseEventsFromTxResponse(resp *sdk.TxResponse) []provider.RelayerEvent {
 	return events
 }
 
-func (cc *CosmosProvider) buildMessages(ctx context.Context, msgs []provider.RelayerMessage, memo string) ([]byte, sdk.Coins, error) {
+func (cc *CosmosProvider) buildMessages(ctx context.Context, msgs []provider.RelayerMessage, memo string) ([]byte, uint64, sdk.Coins, error) {
 	// Query account details
 	txf, err := cc.PrepareFactory(cc.TxFactory())
 	if err != nil {
-		return nil, sdk.Coins{}, err
+		return nil, 0, sdk.Coins{}, err
 	}
 
 	if memo != "" {
@@ -231,7 +247,7 @@ func (cc *CosmosProvider) buildMessages(ctx context.Context, msgs []provider.Rel
 	// If users pass gas adjustment, then calculate gas
 	_, adjusted, err := cc.CalculateGas(ctx, txf, CosmosMsgs(msgs...)...)
 	if err != nil {
-		return nil, sdk.Coins{}, err
+		return nil, 0, sdk.Coins{}, err
 	}
 
 	// Set the gas amount on the transaction factory
@@ -246,13 +262,7 @@ func (cc *CosmosProvider) buildMessages(ctx context.Context, msgs []provider.Rel
 		}
 		return nil
 	}, retry.Context(ctx), rtyAtt, rtyDel, rtyErr); err != nil {
-		return nil, sdk.Coins{}, err
-	}
-
-	// Attach the signature to the transaction
-	// Force encoding in the chain specific address
-	for _, msg := range msgs {
-		cc.Codec.Marshaler.MustMarshalJSON(CosmosMsg(msg))
+		return nil, 0, sdk.Coins{}, err
 	}
 
 	done := cc.SetSDKContext()
@@ -263,27 +273,43 @@ func (cc *CosmosProvider) buildMessages(ctx context.Context, msgs []provider.Rel
 		}
 		return nil
 	}, retry.Context(ctx), rtyAtt, rtyDel, rtyErr); err != nil {
-		return nil, sdk.Coins{}, err
+		return nil, 0, sdk.Coins{}, err
 	}
 
 	done()
 
-	fees := txb.GetTx().GetFee()
+	tx := txb.GetTx()
+	fees := tx.GetFee()
 
 	var txBytes []byte
 	// Generate the transaction bytes
 	if err := retry.Do(func() error {
 		var err error
-		txBytes, err = cc.Codec.TxConfig.TxEncoder()(txb.GetTx())
+		txBytes, err = cc.Codec.TxConfig.TxEncoder()(tx)
 		if err != nil {
 			return err
 		}
 		return nil
 	}, retry.Context(ctx), rtyAtt, rtyDel, rtyErr); err != nil {
-		return nil, sdk.Coins{}, err
+		return nil, 0, sdk.Coins{}, err
 	}
 
-	return txBytes, fees, nil
+	return txBytes, txf.Sequence(), fees, nil
+}
+
+// handleAccountSequenceMismatchError will parse the error string, e.g.:
+// "account sequence mismatch, expected 10, got 9: incorrect account sequence"
+// and update the next account sequence with the expected value.
+func (cc *CosmosProvider) handleAccountSequenceMismatchError(err error) {
+	sequences := numRegex.FindAllString(err.Error(), -1)
+	if len(sequences) != 2 {
+		return
+	}
+	nextSeq, err := strconv.ParseUint(sequences[0], 10, 64)
+	if err != nil {
+		return
+	}
+	cc.nextAccountSeq = nextSeq
 }
 
 // MsgCreateClient creates an sdk.Msg to update the client on src with consensus state from dst
@@ -1051,7 +1077,7 @@ func castClientStateToTMType(cs *codectypes.Any) (*tmclient.ClientState, error)
 	return clientState, nil
 }
 
-//DefaultUpgradePath is the default IBC upgrade path set for an on-chain light client
+// DefaultUpgradePath is the default IBC upgrade path set for an on-chain light client
 var defaultUpgradePath = []string{"upgrade", "upgradedIBCState"}
 
 // NewClientState creates a new tendermint client state tracking the dst chain.


Test patch:
diff --git a/relayer/chains/cosmos/tx_test.go b/relayer/chains/cosmos/tx_test.go
new file mode 100644
index 0000000..d02974f
--- /dev/null
+++ b/relayer/chains/cosmos/tx_test.go
@@ -0,0 +1,23 @@
+package cosmos
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+type mockAccountSequenceMismatchError struct {
+	Expected uint64
+	Actual   uint64
+}
+
+func (err mockAccountSequenceMismatchError) Error() string {
+	return fmt.Sprintf("account sequence mismatch, expected %d, got %d: incorrect account sequence", err.Expected, err.Actual)
+}
+
+func TestHandleAccountSequenceMismatchError(t *testing.T) {
+	p := &CosmosProvider{}
+	p.handleAccountSequenceMismatchError(mockAccountSequenceMismatchError{Actual: 9, Expected: 10})
+	require.Equal(t, p.nextAccountSeq, uint64(10))
+}