Skip to content

Simplify retired IBC transaction rejection - #4010

Merged
masih merged 1 commit into
mainfrom
masih/rm-tx-plumbing
Aug 25, 2026
Merged

Simplify retired IBC transaction rejection#4010
masih merged 1 commit into
mainfrom
masih/rm-tx-plumbing

Conversation

@masih

@masih masih commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

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.

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.
@masih
masih requested a review from codchen August 25, 2026 18:15
@masih
masih marked this pull request as ready for review August 25, 2026 18:15
@cursor

cursor Bot commented Aug 25, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes mempool/CheckTx admission for any tx containing IBC messages and drops relayer redundancy filtering; behavior must stay aligned with deliver-path tombstones to avoid inconsistent rejection.

Overview
Retired IBC transactions are blocked earlier and more simply. The keeper-driven CheckTx/ante logic that simulated packet handling (RecvPacket, Ack, Timeout, UpdateClient) and rejected “all redundant packet” multisigs is removed in favor of rejecting any message whose type URL starts with /ibc. with ErrIBCDeprecated.

The IBC keeper is no longer threaded through HandlerOptions, CheckTxKeepers, CosmosCheckTxAnte, or the wasm app ante wiring; ibcante.NewAnteDecorator() is now stateless. CheckTx’s final step calls the shared ibcante.RejectMessages helper instead of the deleted CheckMessage.

Tests cover the ante decorator (IBC vs non-IBC) and assert the app message router still returns ErrIBCDeprecated for IBC msgs (e.g. MsgChannelCloseInit).

Reviewed by Cursor Bugbot for commit ec4d4aa. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 25, 2026, 6:21 PM

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.64%. Comparing base (d768283) to head (ec4d4aa).

Files with missing lines Patch % Lines
app/ante/cosmos_checktx.go 0.00% 1 Missing ⚠️
app/legacyabci/check_tx.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
sei-chain-pr 48.93% <85.71%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
app/ante.go 65.51% <100.00%> (ø)
app/app.go 71.73% <ø> (-0.04%) ⬇️
sei-ibc-go/modules/core/ante/ante.go 100.00% <100.00%> (+91.11%) ⬆️
sei-wasmd/app/ante.go 61.29% <100.00%> (ø)
sei-wasmd/app/app.go 77.84% <ø> (-0.07%) ⬇️
app/ante/cosmos_checktx.go 50.30% <0.00%> (+5.40%) ⬆️
app/legacyabci/check_tx.go 0.00% <0.00%> (ø)

... and 99 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.AnteHandle no longer gates on ctx.IsCheckTx() || ctx.IsReCheckTx() and ignores simulate, 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 the app-hash-breaking label — 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] CheckAuthzContainsEvm is duplicated between app/ante/cosmos_checktx.go:569 and app/antedecorators/authz_nested_message.go:40 with identical logic and the same maxNestedMsgs cap. 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@masih
masih added this pull request to the merge queue Aug 25, 2026
Merged via the queue into main with commit 4835271 Aug 25, 2026
85 of 87 checks passed
@masih
masih deleted the masih/rm-tx-plumbing branch August 25, 2026 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants