Retire all versions of IBC precompiles and remove legacy implementations - #3884
Retire all versions of IBC precompiles and remove legacy implementations#3884masih wants to merge 2 commits into
Conversation
Retire the IBC precompile at every version while preserving each historical ABI and the registered address. All valid calls now revert with a clear retirement reason. Remove obsolete legacy IBC implementations and unused keeper dependencies. Teach the version generator to skip retired modules, preventing IBC from being archived or reactivated during future upgrades. Add coverage confirming every registered IBC version reverts and remains non-payable.
|
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 #3884 +/- ##
==========================================
- Coverage 61.61% 60.75% -0.86%
==========================================
Files 2348 2255 -93
Lines 200852 190137 -10715
==========================================
- Hits 123755 115518 -8237
+ Misses 66044 64416 -1628
+ Partials 11053 10203 -850
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview The change adds Dependency cleanup: Reviewed by Cursor Bugbot for commit 6d19c70. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit add06b2. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: add06b2823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| versioned := make(utils.VersionedPrecompiles, len(legacyABIByVersion)+1) | ||
| versioned[latestUpgrade] = newRetiredPrecompile(pcommon.MustGetABI(currentABI, "abi.json"), keepers) | ||
| for version, filename := range legacyABIByVersion { | ||
| versioned[version] = newRetiredPrecompile(pcommon.MustGetABI(legacyABIs, filename), keepers) |
There was a problem hiding this comment.
When debug_traceTransaction replays a pre-v6.6 transaction that successfully called the IBC precompile, CustomPrecompiles selects one of these height-specific entries, but every entry now uses the retired executor and reverts rather than reproducing the original call and state transitions. Keeping only each historical ABI is insufficient for replay correctness; retain the legacy executors for historical versions and retire only the active version.
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Retiring the IBC precompile while preserving every historical ABI and the registered address is a clean approach, and the dead-keeper cleanup is thorough with no dangling references. One blocking issue: opting the module out of scripts/bump_version leaves legacyABIByVersion hand-maintained, and it is already missing v6.6, which will resolve to a nil precompile for the IBC address as soon as the next tag is cut.
Findings: 1 blocking | 11 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. Codex also reported it could not run tests (Go 1.25.6 toolchain download blocked); I could not build either, so all findings here are from static reading. precompiles/ibc/versionsis now orphaned:discoverModules()skips retired modules, soarchiveModulewill never append future tags to it, yetx/evm/keeper/keeper_test.go:141(TestGetCustomPrecompiles) still reads it. Either delete the file or make it the source of truth forlegacyABIByVersion— the latter also closes the v6.6 gap permanently.- Test coverage exercises only the
transferWithDefaultTimeoutselector. The other ABI methods, an unknown selector, and sub-4-byte calldata all take different paths throughRunAndCalculateGas(revert with no reason data rather than the retirement reason); worth one case each. - Every retired call now reaches
HandlePrecompileErrorplus the unconditionalfmt.Printf("precompile %s encountered error: ...")atprecompiles/common/precompiles.go:161and increments error metrics. A precompile that reverts by design turns that pre-existing log line into a guaranteed per-call stdout write and error-metric bump that any caller can drive cheaply. Consider whether retirement should bypass the error-reporting path. - Confirmation request, not a correctness claim: retirement is applied to every historical version, so tracing/replaying blocks that originally executed a successful IBC transfer will now revert instead of reproducing the committed result. Per REVIEW_GUIDELINES §2 I am not treating this as a bug, but the retroactive scope (as opposed to gating retirement at the next upgrade) is worth an explicit sign-off in the PR description given the
app-hash-breakinglabel. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| moduleName := entry.Name() | ||
| moduleDir := filepath.Join(precompilesDir, moduleName) | ||
| if fileExists(filepath.Join(moduleDir, retiredMarker)) { |
There was a problem hiding this comment.
[suggestion] The retirement guard now exists in two places — here and in discoverModules() (line 534). regenerateAllSetup re-walks precompilesDir itself rather than iterating discoverModules(), so the next person adding a module-wide filter has to remember both.
AGENTS.md "Structural corrections" calls for exactly this: guard at the single choke point every path passes through. Having regenerateAllSetup iterate discoverModules() (and keep only its versions/{name}.go checks) makes the exclusion an invariant instead of a convention.
| evm := &vm.EVM{StateDB: state.NewDBImpl(ctx, &testApp.EvmKeeper, true)} | ||
|
|
||
| versioned := ibc.GetVersioned("v6.6", testApp.GetPrecompileKeepers()) | ||
| require.Len(t, versioned, 15) |
There was a problem hiding this comment.
[suggestion] require.Len(t, versioned, 15) is the assertion that should have caught the missing v6.6 mapping, and it can't: the count is a literal, and "v6.6" is passed in as latestUpgrade on line 24 so it's present regardless.
Read precompiles/ibc/versions and assert that the key set of GetVersioned(<last line>, …) equals the set of lines in that file. That fails loudly when a tag is added without a corresponding ABI entry — which is now a purely manual step for this module.
| const ( | ||
| IBCAddress = "0x0000000000000000000000000000000000001009" | ||
| ) | ||
| const RetiredReason = "ibc precompile is retired; ibc transfers are disabled" |
There was a problem hiding this comment.
[nit] "ibc transfers are disabled" overstates the change. The IBC transfer module is still mounted and routed (app/app.go:624, :636, :867), so native Cosmos IBC transfers keep working — only the EVM precompile is retired. This string is surfaced to contract callers as the revert reason, so it's worth being precise, e.g. "ibc precompile is retired; use native ibc transfers".
| if err := ValidateNonPayable(value); err != nil { | ||
| return nil, 0, err | ||
| } | ||
| return e.revertData, GetRemainingGas(ctx, e.evmKeeper), e.err |
There was a problem hiding this comment.
[nit] e.revertData is a single slice built once at construction and handed to every caller. Nothing in the current EVM path mutates returned ret, so this isn't a live bug, but the aliasing is invisible from the call site and one in-place write would corrupt the revert reason for every subsequent call on that node. return common.CopyBytes(e.revertData), … removes the question.
There was a problem hiding this comment.
@masih I could imagine some possible issue with OCC here where some weird caching somewhere could lead to a consensus divergence. worth a look maybe
|
|
||
| func (e *retiredExecutor) Execute(ctx sdk.Context, _ *abi.Method, _ common.Address, _ common.Address, _ []interface{}, value *big.Int, _ bool, _ *vm.EVM, _ uint64, _ *tracing.Hooks) ([]byte, uint64, error) { | ||
| if err := ValidateNonPayable(value); err != nil { | ||
| return nil, 0, err |
There was a problem hiding this comment.
[nit] The non-payable rejection returns nil revert data, so a value-bearing call reverts with no reason at all — TestRetiredIBCPrecompileRemainsNonPayable pins that with require.Empty(t, ret). Returning all-gas-consumed matches the convention of the other executors, but the empty reason is at odds with the stated goal that calls revert with a clear retirement reason; the caller sending value gets less information than the caller who doesn't. Consider returning e.revertData here too.
There was a problem hiding this comment.
Something worth to improve, some precompile functions are payable and are also used as such
| minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, ibchost.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| evidencetypes.StoreKey, ibctransfertypes.StoreKey, capabilitytypes.StoreKey, oracletypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, |
There was a problem hiding this comment.
[nit] Dropping ibchost.StoreKey / ibctransfertypes.StoreKey here (and "ibc" / "transfer" from Modules below) goes further than retiring the precompile: both modules are still mounted in app/app.go, so these lists no longer describe the app's actual stores. tools/utils has no importer in-tree today, so the impact is nil, but if it's ever used for a state dump or size report it will silently omit live IBC state. Retiring the precompile doesn't require this hunk.
There was a problem hiding this comment.
Retiring the IBC precompile while keeping 0x1009 registered and preserving every historical ABI is the right shape, and the dangling keeper wiring is removed cleanly with no leftover references. No blockers; the notes below concern the hand-written GetVersioned diverging from the generator's manifest contract, unasserted gas/revert semantics, a duplicated retired-marker guard, and an out-of-scope change to tools/utils.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Neither new test asserts the returned
remainingGas, yet retirement changes it: previously every IBC failure returned0(all supplied gas burned), now the normal revert path returns the real remaining gas. Gas is consensus-relevant on an app-hash-breaking change — pin the expected gas outcome for both the reverting and the value-bearing path. TestEveryIBCVersionIsRetiredonly exercisestransferWithDefaultTimeout. The other registered method (transfer) is never called for any version, so a selector-decoding regression ontransferwould go unnoticed. Iterating overcontractABI.Methodsinstead of hardcoding one name would cover both and stay correct as ABIs differ across versions (v5.5.2 lacksmemo).precompiles/ibc/IBC.solis unchanged and still presentstransfer/transferWithDefaultTimeoutas functional. Keeping the file is correct (the ABI must be preserved), but a retirement note in the interface doc would stop integrators writing against a permanently-reverting contract.RunAndCalculateGasdoesfmt.Printf("precompile %s encountered error: ...")on every error. Retirement makes that the guaranteed outcome for a publicly callable address, so every call now writes a line to validator stdout. Pre-existing code path, but the cost/benefit changes when it is the only possible outcome — worth considering demoting it for retired precompiles.- Cursor's review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues, matching my read on correctness. - I could not compile or run the test suite in this environment (Go toolchain fetch is network-blocked, same limitation Codex hit). Compile-correctness of the
NewKeepersignature changes and the removedTransferK()/ClientK()/ConnectionK()/ChannelK()interface methods was verified by grepping for dangling references (none found inprecompiles/,x/evm/,giga/,tools/,app/), not by a build. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| "v6.5": check(ibcv65.NewPrecompile(keepers)), | ||
| historicalVersions := getHistoricalVersions() | ||
| versioned := make(utils.VersionedPrecompiles, len(historicalVersions)+1) | ||
| for _, version := range historicalVersions { |
There was a problem hiding this comment.
[suggestion] This loop treats every line of versions as a legacy version, but the generator's contract is that the last manifest entry is the active version, not an archived one — regenerateAllSetup uses legacyCount := len(versions) - 1 and keys that final entry via latestUpgrade (scripts/bump_version/main.go:412). The generated setup.go this replaces stopped at v6.5 for exactly that reason, even though versions ends with v6.6.
Consequences today are benign — app/tags ends at v6.6, so line 26 overwrites the loop's v6.6 entry and both resolve to byte-identical ABIs (legacy/v66/abi.json == abi.json). But two things follow:
- In production the map has
len(versions)entries, notlen(versions)+1, sorequire.Len(t, versioned, len(historicalVersions)+1)inibc_test.go:32only passes because the test injects a synthetic"future-upgrade". It asserts a shape the running node never produces, and will silently start being the real shape once the next tag is cut. historicalVersionsis a misnomer: it includes the current version.
Suggest iterating historicalVersions[:len(historicalVersions)-1] (mirroring the generator) and naming it accordingly, or documenting deliberately why the whole manifest is archived here.
| if err := ValidateNonPayable(value); err != nil { | ||
| return nil, 0, err | ||
| } | ||
| return e.revertData, GetRemainingGas(ctx, e.evmKeeper), e.err |
There was a problem hiding this comment.
[suggestion] The two exit paths of this executor have inconsistent semantics. A value-bearing call takes the branch above and returns nil, 0, err — all supplied gas burned, and no revert data, so a Solidity caller sees a bare revert() with no reason. Every other call returns the encoded reason plus the real remaining gas, so it reverts cheaply and explains itself. TestRetiredIBCPrecompileRemainsNonPayable pins the empty-ret half of that asymmetry without noting it is intentional.
The PR description says "All valid calls now revert with a clear retirement reason"; a call carrying value is exactly the case where a caller most needs the reason (their funds are the thing being rejected). Consider returning e.revertData for the non-payable rejection too, or add a comment stating why the payable path deliberately keeps the older burn-all-gas behavior.
There was a problem hiding this comment.
same as above with the payable
| func validCallData(t *testing.T, contractABI abi.ABI) []byte { | ||
| t.Helper() | ||
|
|
||
| method := contractABI.Methods["transferWithDefaultTimeout"] |
There was a problem hiding this comment.
[nit] Single-value map index: if a version's ABI ever lacked transferWithDefaultTimeout, this yields the zero abi.Method — nil ID, empty Inputs — so input becomes empty, ExtractMethodID fails with "input too short", and the test fails at abi.UnpackRevert with a message that points nowhere near the actual cause. Use the two-value form with require.True(t, ok, ...) so the failure names the missing method. (All 15 archived ABIs do currently define it, so this is about the next one, not today.)
|
|
||
| moduleName := entry.Name() | ||
| moduleDir := filepath.Join(precompilesDir, moduleName) | ||
| if fileExists(filepath.Join(moduleDir, retiredMarker)) { |
There was a problem hiding this comment.
[suggestion] The retired-marker check now lives in two places — here and in discoverModules at line 534 — because regenerateAllSetup re-walks precompilesDir itself instead of going through discoverModules(). That is the pattern AGENTS.md calls out under "Guard at the choke point, never at each caller": a third module walker added later has to remember the marker, where routing both through one discovery function makes it an invariant. regenerateAllSetup also duplicates the excludeDirs filter for the same reason.
Suggest having regenerateAllSetup iterate discoverModules() so the marker (and excludeDirs) are honoured in exactly one place.
| minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, ibchost.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, | ||
| evidencetypes.StoreKey, ibctransfertypes.StoreKey, capabilitytypes.StoreKey, oracletypes.StoreKey, | ||
| govtypes.StoreKey, paramstypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey, |
There was a problem hiding this comment.
[suggestion] Dropping ibchost.StoreKey / ibctransfertypes.StoreKey here (and "ibc" / "transfer" from Modules below) makes this helper disagree with the running app: app/app.go:276-277 still mounts both stores, and the IBC and transfer modules are still registered in the module manager (app/app.go:897, 967-970). Those stores still hold state, so a state-size or dump tool built on ModuleKeys/Modules will now silently omit it and under-report rather than error.
Nothing in-tree imports tools/utils today, so this is not an active break — but it is also unrelated to retiring the precompile, and the precompile retirement does not remove the IBC module. Worth either reverting this hunk from the PR or stating why the tool should stop seeing state the chain still keeps.
Superseded: latest AI review found no blocking issues.
3 participants
Retire the IBC precompile at every version while preserving each historical ABI and the registered address. All valid calls now revert with a clear retirement reason.
Remove obsolete legacy IBC implementations and unused keeper dependencies. Teach the version generator to skip retired modules, preventing IBC from being archived or reactivated during future upgrades.
Add coverage confirming every registered IBC version reverts and remains non-payable.