Repository navigation
Remove IBC leftovers and delete retired IBC stores at v6.8 - #4337
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
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 #4337 +/- ##
==========================================
- Coverage 55.56% 55.55% -0.02%
==========================================
Files 2173 2170 -3
Lines 168616 168610 -6
==========================================
- Hits 93699 93676 -23
- Misses 74912 74929 +17
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2ae54fc to
8db2abe
Compare
The transfer module account is no longer created at genesis, so accounts created by the evmrpc test initializers shift down by one.
8db2abe to
6167fce
Compare
PR SummaryHigh Risk Overview The post-v6.7 compatibility shim is removed: README documents v6.8 behavior (including the four mainnet Reviewed by Cursor Bugbot for commit 53e21bb. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The v6.8 migration looks correct, and the tests cover it well. It deletes the capability, ibc and transfer stores alongside oracle, rewrites stored /ibc.* proposals as TextProposals by reading their raw protobuf fields, and prunes the upgradedIBCState keys. The remaining notes are about behaviour visible to users: dropping the 0x1009 precompile, and removing transfer from maccPerms.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Removing the transfer entry from
maccPerms(app/app.go and sei-wasmd/app/app.go) also drops the transfer module address fromModuleAccountAddrs(), so bank no longer blocks sends to it. After v6.8, a user can send coins to the old transfer module account and they are stuck there for good. If that is intended, say so in the PR description; if not, keep the address on the blocked list without Minter/Burner permissions. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // proposal whose type lives under an IBC protobuf package with a TextProposal | ||
| // carrying the original title and description. The proposal record, its | ||
| // deposits, votes and tally indexes are left untouched. | ||
| func (app *App) rewriteRetiredIBCProposals(ctx sdk.Context) { |
There was a problem hiding this comment.
is this so we can remove the ibc proto type from the chain? do we want to leave retired protobufs in a deprecated folder instead?
i'm thinking through some compatibility scenarios in which this could break (i don't have a concrete scenario in mind)
i'm not particularly opinionated, i'm just thinking aloud there
There was a problem hiding this comment.
Yes, that's exactly why. Mainnet has four passed proposals (52, 81, 100, 119) stored as Any{/ibc.core.client.v1.ClientUpdateProposal}. Once the IBC proto types are no longer registered in the interface registry, every code path that unpacks all proposals (gov gRPC/REST Proposals, ExportGenesis, CLI queries, IterateProposals in the gov keeper) panics or errors on those four records. Rewriting them to TextProposal with the original title/description keeps the records readable with no type left behind.
The alternative you describe is what the tree had until this PR: app/retiredibc kept decode-only registrations of the retired proposal types (and a raw-query interceptor) so the stored Anys still resolved. The direction I got for this PR was to remove that layer entirely since IBC will never be re-enabled, hence the one-shot rewrite in the v6.8 handler instead.
Compatibility-wise, the observable differences after v6.8 are: (1) gov queries for those four proposals return @type: /cosmos.gov.v1beta1.TextProposal instead of the IBC type, so an indexer diffing proposal content will see the change once; (2) clients that still send an /ibc.* proposal in MsgSubmitProposal get a codec "unable to resolve type URL" error at decode rather than a gov handler error, which is already the case today for submitting them since no handler is routed. Replay of pre-v6.8 heights is unaffected because that runs the old binary. Nothing else references the types (no txs, no other stores; the upgradedIBCState keys are pruned in the same handler). If you'd rather keep the deprecated-decode-only approach, it's a straightforward revert of the retiredibc deletion plus dropping rewriteRetiredIBCProposals; I'd just want the call made explicitly since it keeps IBC protos compiled into the binary indefinitely.
There was a problem hiding this comment.
@philipsu522 taking the approach of doing a one-off conversion was on purpose on my side. It lets us be explicit and never need to carry over the original types again while avoiding panics during decode.
In terms of someone needing this, we are covered by freeze mode on that which makes moving types around a much easier job to reason about in terms of usability.
But I have no strongly held opinions on this one. Happy to revert back to keeping the protos if you think it's worth it.
|
Holding off merging this until #4328 lands for easier merge mechanics. |
|
@seidroid review |
There was a problem hiding this comment.
The v6.8 migration looks correct. It deletes the capability, ibc and transfer stores together with oracle, rewrites /ibc.* proposals as TextProposals by reading their raw protobuf fields (title and description) before RunMigrations, and prunes the upgradedIBCState keys. The transfer module address stays blocked through retiredModuleAccounts, and the 0x1009 tombstone is kept, so both earlier review points are addressed. No new blocking or non-blocking issues found.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
There was a problem hiding this comment.
This PR removes the retired IBC compatibility layer (the query intercept, the decode-only gov codecs, the transfer module permissions and the mounted capability/ibc/transfer stores). At v6.8 it deletes those stores, rewrites the stored /ibc.* proposals as TextProposals before RunMigrations, and prunes the upgradedIBCState/ keys from the upgrade store. Nothing blocks: the handler ordering, the store-loader deletion list, the version-map cleanup and the protowire title/description extraction are all correct, and the tests cover them; a Go toolchain was not available in the sandbox, so the tests were not run. Of the independent readings I dropped codex's single finding, that deleteByPrefix stores iterator keys without copying them: the memiavl iterator clones keys unless zero-copy is set, zero-copy keys point at immutable tree nodes, and the repo's own dbadapter.Store.DeleteAll uses the same pattern.
Non-blocking
- New chains no longer create the
transfermodule account at genesis, so genesis account numbers shift by one (hence the evmrpc tests moving from account 8 to 7). Existing chains are unaffected, but any external fixture or tooling that hardcodes post-genesis account numbers will need the same update.
1 nit, not posted on the code
app/upgrades.go:208— A malformedContent.Valueon any/ibc.*proposal panics inside the v6.8 handler and halts the chain at the upgrade height. That is acceptable for the four known mainnet records, but logging the error and keeping an empty title or description would be a softer failure mode for a one-shot migration.
seidroid review · decision approve · session 08c0c611c08e47e0bea6995341c4c3dd · turn resp_claude_e686e214b4d92e52a603fd078aca94fc · item d4d703e7e98753d7a958146c93858330
Findings: 0 blocking | 1 non-blocking | 0 posted inline
…ei-protocol#4340) With the IBC modules gone and sei-protocol#4337 deleting their stores at v6.8, `x/upgrade` still carried the machinery that existed only to serve IBC light-client upgrades: the `upgradedIBCState/<height>/upgradedClient|upgradedConsState` keys and their `SetUpgradedClient`/`GetUpgradedConsensusState`/`ClearIBCState` keeper helpers, the deprecated `Plan.upgraded_client_state` field and its `ValidateBasic` rejection, the `Query/UpgradedConsensusState` gRPC and REST route, and the `upgradedConsensusState` method on the upgrade precompile at 0x1015. None of it has a consumer on Sei, and keeping it means keeping a path that writes IBC state back into the upgrade store. This removes all of it. `ScheduleUpgrade`, `ClearUpgradePlan` and `ApplyUpgrade` no longer touch IBC keys; `Plan` reserves field 5 rather than carrying a dead `Any`; the upgrade query service loses the deprecated rpc and its request/response messages, with `query.pb.go`, `query.pb.gw.go`, `upgrade.pb.go` and the swagger regenerated for just these two proto files. On the EVM side the method is dropped from the current precompile, its ABI and `Upgrade.sol`, and from the not-yet-wired `legacy/v68` archive so it matches the v6.8 code it will freeze. The `legacy/v67` snapshot keeps the selector and returns empty bytes, which is what it always returned: neither pacific-1 (`/store/upgrade/subspace` at height 234051819) nor atlantic-2 (same query at height 274315652) has any `upgradedIBCState` key, and none of the four passed mainnet IBC proposals was an `UpgradeProposal`, so the state was never written. To keep v6.7-era replays gas-identical, the legacy method still charges the gas of the store read it used to perform (`ReadCostFlat` plus `ReadCostPerByte` over the `upgradedIBCState/<h>/upgradedConsState` key), and `TestPrecompile_UpgradedConsensusStateRemoved` pins that charge. The replay is gas-identical rather than byte-identical in every observable: the read no longer reaches the store, so a `debug_trace*` prestate dump of a historical tx that hit this selector would lack one absent-key `upgrade` store read. That is not consensus-relevant and no such tx is known. A tx carrying `Plan.upgraded_client_state` is still rejected, now by the unknown-field check at decode time instead of `ValidateBasic`; neither path touches state. Amino JSON sign bytes are unchanged because the pointer field was `omitempty`. State impact is limited to the v6.8 upgrade already in sei-protocol#4337, whose `pruneUpgradedIBCState` step stays as a defensive no-op on mainnet. The v6.8 `rewriteRetiredIBCProposals` scan from sei-protocol#4337 is also bounded here: it stops at proposal ID 1000 before decoding anything above it, so zero-deposit proposals submitted in the deposit window before the upgrade cannot stretch the upgrade block. IBC proposal types stopped being submittable at v6.7, when the highest proposal ID on any Sei network was 286, and every IBC proposal on pacific-1 and atlantic-2 has an ID at or below 163. Reviewers should look at the v67 legacy path and at `ClearUpgradePlan`, which is now a plain plan delete. Validated with the `x/upgrade` and `precompiles/upgrade` suites, the tagged v6.8 upgrade tests, the offline v6.7→v6.8 migration and `make lint`.
…tocol#4420) After sei-protocol#4337 and sei-protocol#4340 removed the IBC modules, stores and upgrade state, a few places in the tree still describe behaviour in terms of IBC that no longer exists on Sei: the tokenfactory `validateCreateDenom` check calls itself a temporary workaround for an IBC bug, the `sc-keep-recent` comment in the docker `app.toml` files and the v2 config migration guide justifies the default with IBC relayers, the vendored `sei-cosmos/x/README.md` points at ibc-go, the `x/upgrade` client spec lists `ibc`, `transfer` and `vesting` module versions, and two store-hash tests mount a store named `ibc`. This PR only rewords and relabels. The tokenfactory check is kept as-is because refusing a subdenom that collides with a denom that already has supply is still correct; only the comment and error string change, and nothing asserts on the old text. The two hash tests mount `evm` instead of `ibc`; they derive their expectations from the stores they create rather than from golden hashes, so the rename does not alter what they verify. No runtime, state or consensus behaviour changes.
3 participants
After the IBC modules were unwired, the chain still carried their leftovers: the
capability,ibcandtransferstores stayed mounted,app/retiredibcintercepted/store/ibc/*raw queries and kept decode-only codecs for the historical/ibc.core.client.v1.*governance proposals, the transfer module account kept minter/burner permissions, and seidb tooling and docs still listed the IBC stores. IBC is never coming back, so none of this compatibility layer should survive.This PR removes that layer and cleans the persisted state at the v6.8 boundary. The
capability,ibcandtransferstores are unmounted from bothapp.Appand the wasmd test app and are deleted alongsideoraclethrough the v6.8UpgradeStoreLoader, with their module-version entries dropped by the handler. Mainnet holds four passed IBC governance proposals whoseAnycontent can no longer be decoded once the codecs go, which would breakIterateProposalsand genesis export, sorewriteRetiredIBCProposalsrewrites any proposal with a/ibc.type URL in place as aTextProposal, reading title and description from the raw protobuf bytes withprotowireand leaving IDs, status, deposits, votes and queue entries untouched.pruneUpgradedIBCStateremoves theupgradedIBCState/*keys from the upgrade store. Bank balances and supply are deliberately not touched: the 96ibc/*voucher denominations on mainnet remain ordinary bank coins. Two things are kept on purpose. The IBC precompile at0x1009stays registered, in both the regular and Giga fail-fast engines, as a tombstone that reverts every call, because deleting it would turn the address into an empty account where low-level calls succeed and value gets stranded. The deterministictransfermodule address stays in the bank blocked-address set throughretiredModuleAccounts, without any module or permissions behind it, so nothing can be sent to an account no key or module can spend from.This is a consensus-affecting state migration and reviewers should focus on the v6.8 handler and the store loader. Coverage is in
app/upgrade_v68_test.go(module versions, proposal rewrite, upgrade-state pruning, second application is a no-op) and the offline v6.8 source/target fixtures, which seed the four deleted stores, an IBC proposal, an upgraded-IBC-state key and anibc/*voucher, then assert physical tree deletion after reopen, crash/replay hash equality, the rewritten proposal, the pruned keys, a successful post-upgrade bank send of the voucher, and that every other key of the retainedbank,govandupgradestores is unchanged apart from the entries the upgrade block is specified to touch.make upgrade-test,make upgrade-test-vet,make upgrade-test-offline FROM_REF=release/v6.7andmake lintpass.The proposal rewrite runs before
RunMigrationsso no module migration can walk the gov store while the four/ibc.core.client.v1.ClientUpdateProposalrecords are still undecodable. It keeps title and description and drops the client fields, recorded here so the loss is explicit rather than silent: proposal 52subject_client_id=07-tendermint-44substitute_client_id=07-tendermint-112; 8107-tendermint-120/07-tendermint-131; 10007-tendermint-135/07-tendermint-146; 11907-tendermint-12/07-tendermint-168. After this upgrade v6.8 nodes cannot answer gov state queries at pre-v6.8 heights for those four proposals, nor any other pre-retirement IBC history; that is served by v6.6 freeze nodes, and the README states so.