feat(auth): enhance timeout height validation with clear error messages #25262
Conversation
📝 WalkthroughWalkthroughChanges adjust TxTimeoutHeightDecorator to treat timeout height equal to current block height as invalid, differentiate error messages for equal vs. past timeouts, and retain timestamp checks. A new utility GetRecommendedTimeoutHeight(currentHeight, buffer) was added. Tests updated accordingly and a new test added for the helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Tx as Tx
participant Ante as AnteHandler
participant THD as TxTimeoutHeightDecorator
participant Next as Next Ante
Tx->>Ante: DeliverTx(CheckTx)
Ante->>THD: AnteHandle(ctx, tx, simulate)
THD->>THD: currentHeight = ctx.BlockHeight()
alt timeoutHeight == 0
THD->>Next: AnteHandle(...)
Next-->>Ante: result/err
else timeoutHeight != 0
alt timeoutHeight == currentHeight
THD-->>Ante: error("set to next block height")
else currentHeight > timeoutHeight
THD-->>Ante: error("timeout already passed")
else currentHeight < timeoutHeight
THD->>THD: check timestamp constraints
alt timestamp invalid
THD-->>Ante: error(timestamp)
else timestamp ok
THD->>Next: AnteHandle(...)
Next-->>Ante: result/err
end
end
end
Ante-->>Tx: result/err
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
x/auth/ante/basic.go (5)
201-208: Guard against uint64 overflow and document semantics in helper.While practically unreachable, currentHeight + buffer can overflow uint64. Add a cheap overflow guard and clarify that buffer=0 means “next block.”
Apply this diff within the helper:
func GetRecommendedTimeoutHeight(currentHeight uint64, buffer uint64) uint64 { if buffer == 0 { buffer = 1 // Default buffer of 1 block } - return currentHeight + buffer + // Saturating add to guard against uint64 overflow (extremely unlikely in practice). + sum := currentHeight + buffer + if sum < currentHeight { // overflow detected + return ^uint64(0) + } + return sum }
223-233: Use the new helper and tighten the error message.
- Use GetRecommendedTimeoutHeight for consistency and to inherit any future logic (like overflow/safety).
- Improve grammar and clarity in the error string. “must be strictly greater than the current block height” communicates the rule directly.
if timeoutHeight > 0 { // Check if timeout height is set to current block height (which is invalid) if timeoutHeight == currentHeight { - nextBlockHeight := currentHeight + 1 + nextBlockHeight := GetRecommendedTimeoutHeight(currentHeight, 1) return ctx, errorsmod.Wrapf( sdkerrors.ErrTxTimeoutHeight, - "you must set the timeout height to be the next block height got %d, expected %d", - timeoutHeight, nextBlockHeight, + "timeout height must be strictly greater than the current block height: got %d, expected at least %d", + timeoutHeight, nextBlockHeight, ) }
234-241: Offer an actionable hint when the timeout height is in the past.Adding a recommendation in the error reduces back-and-forth for users and aligns with the PR goal of clearer guidance.
// Check if timeout height has already passed if currentHeight > timeoutHeight { return ctx, errorsmod.Wrapf( sdkerrors.ErrTxTimeoutHeight, - "transaction timeout height %d has already passed; current block height is %d", - timeoutHeight, currentHeight, + "transaction timeout height %d has already passed; current block height is %d (try >= %d)", + timeoutHeight, currentHeight, GetRecommendedTimeoutHeight(currentHeight, 1), ) }
210-214: Minor naming nit: comments refer to TxHeightTimeoutDecorator instead of TxTimeoutHeightDecorator.The type is TxTimeoutHeightDecorator; the doc comments here (and above the interface) say TxHeightTimeoutDecorator. Consider aligning names to avoid confusion and lint warnings.
223-241: PR objective mismatch: duplicate error log rate-limiting isn’t addressed here.The linked issue (#25169) requests rate-limiting identical timeout-height errors (e.g., “only once per minute”). Updating the error message helps UX but won’t reduce duplicate validator logs by itself. That requires changes where logging occurs (CheckTx/DeliverTx error handling in baseapp or logger configuration), not inside this decorator.
Recommendations:
- Implement log sampling/dedup at the app/logger layer keyed by (height, error code, message template).
- Alternatively, cache the last logged error per height with a TTL in the CheckTx path.
Please confirm whether the PR should still “close” #25169 or if a follow-up is planned.
x/auth/ante/basic_test.go (2)
182-237: Add assertions for the new, more actionable error messages.The PR goal is clearer errors, but tests currently only assert on the error type. Add targeted checks for message content for:
- equal-to-current-height case
- past-height case
This prevents regressions and ensures the guidance (including expected next height) is preserved.
You can append a focused subtest:
func TestTxTimeoutHeightDecorator_ErrorMessages(t *testing.T) { suite := SetupTestSuite(t, true) antehandler := sdk.ChainAnteDecorators(ante.NewTxTimeoutHeightDecorator()) priv1, _, addr1 := testdata.KeyTestPubAddr() msg := testdata.NewTestMsg(addr1) feeAmount := testdata.NewTestFeeAmount() gasLimit := testdata.NewTestGasLimit() // Build a base tx suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() require.NoError(t, suite.txBuilder.SetMsgs(msg)) suite.txBuilder.SetFeeAmount(feeAmount) suite.txBuilder.SetGasLimit(gasLimit) // Case 1: same height -> require “strictly greater/next block” guidance { suite.txBuilder.SetTimeoutHeight(10) suite.txBuilder.SetTimeoutTimestamp(time.Time{}) privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0} tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT) require.NoError(t, err) ctx := suite.ctx.WithBlockHeight(10) _, err = antehandler(ctx, tx, true) require.ErrorIs(t, err, sdkerrors.ErrTxTimeoutHeight) require.ErrorContains(t, err, "next block height") } // Case 2: past height -> require hint to try >= next block { suite.txBuilder.SetTimeoutHeight(9) suite.txBuilder.SetTimeoutTimestamp(time.Time{}) privs, accNums, accSeqs := []cryptotypes.PrivKey{priv1}, []uint64{0}, []uint64{0} tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT) require.NoError(t, err) ctx := suite.ctx.WithBlockHeight(10) _, err = antehandler(ctx, tx, true) require.ErrorIs(t, err, sdkerrors.ErrTxTimeoutHeight) require.ErrorContains(t, err, "has already passed") } }
239-258: Nice coverage for the helper; consider one boundary case.Tests look good and cover the default buffer path. Optionally add an overflow boundary check (expected to saturate if you adopt the suggested overflow guard), or at least a documentation note that overflow behavior is unspecified if not guarded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
x/auth/ante/basic.go(2 hunks)x/auth/ante/basic_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (3)
x/auth/ante/basic.go (2)
221-222: Type alignment for height comparisons looks good.Casting ctx.BlockHeight() to uint64 aligns types and removes mixed-signedness comparisons. No concerns here.
223-241: This change is a behavioral break; please call it out and verify downstream impacts.Previously, timeoutHeight == currentHeight was accepted (so long as DeliverTx happened at that height). Now it’s rejected. This tightens correctness but can break existing clients/wallets that set “current height” as the timeout.
- Add a release note entry and migration note.
- Verify common wallets/SDK clients don’t default to “equal height.”
- Confirm no chain tooling relies on the old acceptance semantics.
x/auth/ante/basic_test.go (1)
207-208: Test expectation update looks correct.Switching “same height” to expect sdkerrors.ErrTxTimeoutHeight reflects the new stricter validation.
- Prevent timeout height = current block height - Add clear error messages and helper function - Improve user experience and reduce error logs Signed-off-by: Hwangjae Lee <[email protected]>
- Add error message validation tests - Add overflow boundary tests - Fix timeout height test cases Signed-off-by: Hwangjae Lee <[email protected]>
Signed-off-by: Hwangjae Lee <[email protected]>
meetrick
force-pushed
the
20250826_timeout_handler
branch
from
2fbcfd6 to
ae791bf
Compare
Description
Closes: #25169
Problem
Transactions fail with unclear error messages when users set
timeout_heightto current block height, causing repeated failures and poor UX.Solution
GetRecommendedTimeoutHeight()helper functionChanges
x/auth/ante/basic.go: EnhancedTxTimeoutHeightDecoratorvalidation logicx/auth/ante/basic_test.go: Updated tests and added helper function testsBenefits
Testing
Summary by CodeRabbit
New Features
Bug Fixes