refactor(client): change flag for unordered tx timeouts

CHANGELOG.md

@@ -66,6 +66,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
+* (client) [#24561](https://github.com/cosmos/cosmos-sdk/pull/24561) TimeoutTimestamp flag has been changed to TimeoutDuration, which now sets the timeout timestamp of unordered transactions to the current time + duration passed.
 * (telemetry) [#24541](https://github.com/cosmos/cosmos-sdk/pull/24541) Telemetry now includes a pre_blocker metric key. x/upgrade should migrate to this key in v0.54.0.
 * (x/auth) [#24541](https://github.com/cosmos/cosmos-sdk/pull/24541) x/auth's PreBlocker now emits telemetry under the pre_blocker metric key.
 * (x/bank) [#24431](https://github.com/cosmos/cosmos-sdk/pull/24431) Reduce the number of `ValidateDenom` calls in `bank.SendCoins` and `Coin`.

client/flags/flags.go

@@ -74,7 +74,7 @@ const (
 	FlagOffset           = "offset"
 	FlagCountTotal       = "count-total"
 	FlagTimeoutHeight    = "timeout-height"
-	FlagTimeoutTimestamp = "timeout-timestamp"
+	TimeoutDuration      = "timeout-duration"
 	FlagUnordered        = "unordered"
 	FlagKeyAlgorithm     = "algo"
 	FlagKeyType          = "key-type"
@@ -137,9 +137,9 @@ func AddTxFlagsToCmd(cmd *cobra.Command) {
 	f.Bool(FlagOffline, false, "Offline mode (does not allow any online functionality)")
 	f.BoolP(FlagSkipConfirmation, "y", false, "Skip tx broadcasting prompt confirmation")
 	f.String(FlagSignMode, "", "Choose sign mode (direct|amino-json|direct-aux|textual), this is an advanced feature")
-	f.Uint64(FlagTimeoutHeight, 0, "DEPRECATED: Please use --timeout-timestamp instead. Set a block timeout height to prevent the tx from being committed past a certain height")
-	f.Int64(FlagTimeoutTimestamp, 0, "Set a block timeout timestamp to prevent the tx from being committed past a certain time")
-	f.Bool(FlagUnordered, false, "Enable unordered transaction delivery; must be used in conjunction with --timeout-timestamp")
+	f.Uint64(FlagTimeoutHeight, 0, "DEPRECATED: Please use --timeout-duration instead. Set a block timeout height to prevent the tx from being committed past a certain height")
+	f.Duration(TimeoutDuration, 0, "TimeoutDuration is the duration the transaction will be considered valid in the mempool. If the transaction is still in the mempool, and the block time has passed the time of submission + TimeoutTimestamp, the transaction will be rejected.")
+	f.Bool(FlagUnordered, false, "Enable unordered transaction delivery; must be used in conjunction with --timeout-duration")
 	f.String(FlagFeePayer, "", "Fee payer pays fees for the transaction instead of deducting from the signer")
 	f.String(FlagFeeGranter, "", "Fee granter grants fees for the transaction")
 	f.String(FlagTip, "", "Tip is the amount that is going to be transferred to the fee payer on the target chain. This flag is only valid when used with --aux, and is ignored if the target chain didn't enable the TipDecorator")
@@ -149,8 +149,8 @@ func AddTxFlagsToCmd(cmd *cobra.Command) {
 	f.String(FlagGas, "", fmt.Sprintf("gas limit to set per-transaction; set to %q to calculate sufficient gas automatically. Note: %q option doesn't always report accurate results. Set a valid coin value to adjust the result. Can be used instead of %q. (default %d)",
 		GasFlagAuto, GasFlagAuto, FlagFees, DefaultGasLimit))
 
-	cmd.MarkFlagsMutuallyExclusive(FlagTimeoutHeight, FlagTimeoutTimestamp)
-	cmd.MarkFlagsRequiredTogether(FlagUnordered, FlagTimeoutTimestamp)
+	cmd.MarkFlagsMutuallyExclusive(FlagTimeoutHeight, TimeoutDuration)
+	cmd.MarkFlagsRequiredTogether(FlagUnordered, TimeoutDuration)
 
 	AddKeyringFlags(f)
 }

client/tx/factory.go

@@ -88,8 +88,11 @@
 
 	gasAdj := clientCtx.Viper.GetFloat64(flags.FlagGasAdjustment)
 	memo := clientCtx.Viper.GetString(flags.FlagNote)
-	timestampUnix := clientCtx.Viper.GetInt64(flags.FlagTimeoutTimestamp)
-	timeoutTimestamp := time.Unix(timestampUnix, 0)
+	timeout := clientCtx.Viper.GetDuration(flags.TimeoutDuration)
+	var timeoutTimestamp time.Time
+	if timeout > 0 {
+		timeoutTimestamp = time.Now().Add(timeout)
+	}
 	timeoutHeight := clientCtx.Viper.GetUint64(flags.FlagTimeoutHeight)
 	unordered := clientCtx.Viper.GetBool(flags.FlagUnordered)
 

systemtests/cli.go

@@ -195,6 +195,7 @@ func (c CLIWrapper) RunAndWait(args ...string) string {
 // RunCommandWithArgs use for run cli command, not tx
 func (c CLIWrapper) RunCommandWithArgs(args ...string) string {
 	c.t.Helper()
+	args = c.WithKeyringFlags(args...)
 	execOutput, _ := c.run(args)
 	return execOutput
 }

tests/systemtests/unordered_tx_test.go

@@ -7,7 +7,7 @@
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/cosmos/cosmos-sdk/testutil"
 	"github.com/stretchr/testify/require"
 	"github.com/tidwall/gjson"
 
@@ -17,8 +17,8 @@
 func TestUnorderedTXDuplicate(t *testing.T) {
 	// scenario: test unordered tx duplicate
 	// given a running chain with a tx in the unordered tx pool
-	// when a new tx with the same hash is broadcasted
-	// then the new tx should be rejected
+	// when a new tx with the same unordered nonce is broadcasted,
+	// then the new tx should be rejected.
 
 	systest.Sut.ResetChain(t)
 	cli := systest.NewCLIWrapper(t, systest.Sut, systest.Verbose)
@@ -31,22 +31,28 @@
 
 	systest.Sut.StartChain(t)
 
-	timeoutTimestamp := time.Now().Add(time.Minute)
 	// send tokens
-	cmd := []string{"tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from=" + account1Addr, "--fees=1stake", fmt.Sprintf("--timeout-timestamp=%v", timeoutTimestamp.Unix()), "--unordered", "--sequence=1", "--note=1"}
-	rsp1 := cli.Run(cmd...)
+	cmd := []string{"tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from=" + account1Addr, "--fees=1stake", "--timeout-duration=5m", "--unordered", "--note=1", "--chain-id=testing", "--generate-only"}
+	rsp1 := cli.RunCommandWithArgs(cmd...)
+	txFile := testutil.TempFile(t)
+	_, err := txFile.WriteString(rsp1)
+	require.NoError(t, err)
+
+	signCmd := []string{"tx", "sign", txFile.Name(), "--from=" + account1Addr, "--chain-id=testing"}
+	rsp1 = cli.RunCommandWithArgs(signCmd...)
+	signedFile := testutil.TempFile(t)
+	signedFile.WriteString(rsp1)
+
+	cmd = []string{"tx", "broadcast", signedFile.Name(), "--chain-id=testing"}
+	rsp1 = cli.RunCommandWithArgs(cmd...)
 	systest.RequireTxSuccess(t, rsp1)
 
-	assertDuplicateErr := func(xt assert.TestingT, gotErr error, gotOutputs ...interface{}) bool {
-		require.Len(t, gotOutputs, 1)
-		output := gotOutputs[0].(string)
-		code := gjson.Get(output, "code")
-		require.True(t, code.Exists())
-		require.Equal(t, int64(19), code.Int()) // 19 == already in mempool.
-		return false                            // always abort
-	}
-	rsp2 := cli.WithRunErrorMatcher(assertDuplicateErr).Run(cmd...)
+	cmd = []string{"tx", "broadcast", signedFile.Name(), "--chain-id=testing"}
+	rsp2, _ := cli.RunOnly(cmd...)
 	systest.RequireTxFailure(t, rsp2)
+	code := gjson.Get(rsp2, "code")
+	require.True(t, code.Exists())
+	require.Equal(t, int64(19), code.Int())
 
 	require.Eventually(t, func() bool {
 		return cli.QueryBalance(account2Addr, "stake") == 5000
@@ -86,7 +92,7 @@
 	code := gjson.Get(response, "code").Int()
 	require.Equal(t, int64(0), code)
 
-	bankSendCmdArgs = []string{"tx", "bank", "send", senderAddr, valAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--chain-id=" + v50CLI.ChainID(), "--fees=10stake", "--sign-mode=direct", "--unordered", "--timeout-timestamp=10000"}
+	bankSendCmdArgs = []string{"tx", "bank", "send", senderAddr, valAddr, fmt.Sprintf("%d%s", transferAmount, denom), "--chain-id=" + v50CLI.ChainID(), "--fees=10stake", "--sign-mode=direct", "--unordered", "--timeout-duration=8m"}
 	res, ok = v53CLI.RunOnly(bankSendCmdArgs...)
 	require.True(t, ok)