Simplify retired IBC transaction rejection - #4010
Conversation
Replace keeper-backed IBC redundancy checks with a lightweight type-URL rejection before transactions enter the mempool. Remove obsolete IBC keeper threading from ante and CheckTx paths while retaining MsgServer tombstones, begin-block behavior, and mounted stores.
PR SummaryMedium Risk Overview The IBC keeper is no longer threaded through Tests cover the ante decorator (IBC vs non-IBC) and assert the app message router still returns Reviewed by Cursor Bugbot for commit ec4d4aa. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4010 +/- ##
==========================================
- Coverage 59.68% 58.64% -1.04%
==========================================
Files 2261 2162 -99
Lines 194352 182680 -11672
==========================================
- Hits 115994 107128 -8866
+ Misses 67632 65766 -1866
+ Partials 10726 9786 -940
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The PR cleanly replaces the keeper-backed IBC redundancy checks with a type-URL prefix rejection and removes the now-unused IBC keeper threading; no leftover references remain and the msg-server tombstones, stores, and module wiring are preserved. One gap: the rejection only inspects top-level messages, so authz.MsgExec-wrapped IBC messages still reach the mempool.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
AnteDecorator.AnteHandleno longer gates onctx.IsCheckTx() || ctx.IsReCheckTx()and ignoressimulate, so an IBC message that reaches DeliverTx now fails in the ante chain (last decorator, after fee deduction and sequence increment) instead of at message execution. That means no fees are charged and no sequence bump occurs for such a tx, and simulation of an IBC tx now errors. This is consistent with retiring IBC and the PR carries theapp-hash-breakinglabel — worth an explicit confirmation that the fee/sequence accounting change is the intended consensus behavior. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
CheckAuthzContainsEvmis duplicated betweenapp/ante/cosmos_checktx.go:569andapp/antedecorators/authz_nested_message.go:40with identical logic and the samemaxNestedMsgscap. Two copies of the nested-authz walk means a future fix to one silently misses the other path.
| return next(ctx, tx, simulate) | ||
| } | ||
| // RejectMessages returns the retirement error when a transaction contains an IBC message. | ||
| func RejectMessages(tx sdk.Tx) error { |
There was a problem hiding this comment.
[suggestion] RejectMessages only walks tx.GetMsgs(), so an IBC message wrapped in authz.MsgExec has type URL /cosmos.authz.v1beta1.MsgExec and passes the prefix check. Such a tx enters the mempool and can be included in a block, only failing later at the retained MsgServer tombstone — which undercuts the stated goal of rejecting retired IBC traffic "before they enter the mempool."
The codebase already has the recursion this needs: CheckAuthzContainsEvm (app/ante/cosmos_checktx.go:569, called from the same CheckTx path at line 216, and app/antedecorators/authz_nested_message.go:40) descends through nested MsgExec with a maxNestedMsgs depth cap. Extending that walk — or mirroring it here — would close the hole at the single choke point every path passes through, and deserves a regression test with an IBC message nested inside MsgExec.
Replace keeper-backed IBC redundancy checks with a lightweight
type-URL rejection before transactions enter the mempool.
Remove obsolete IBC keeper threading from ante and CheckTx paths while
retaining MsgServer tombstones, begin-block behavior, and mounted stores.