Skip to content

Remove crisis module wiring and code#3510

Open
codchen wants to merge 5 commits into
mainfrom
codex/remove-crisis-app-module
Open

Remove crisis module wiring and code#3510
codchen wants to merge 5 commits into
mainfrom
codex/remove-crisis-app-module

Conversation

@codchen
Copy link
Copy Markdown
Collaborator

@codchen codchen commented May 27, 2026

Summary

  • remove x/crisis from the Sei app wiring and delete the sei-cosmos/x/crisis module plus its crisis protobuf definitions
  • remove stale crisis wiring from wasmd/simapp test harnesses, local genesis setup, state-dump tooling, and module lists
  • confirmed crisis does not have a dedicated rootmulti KVStore: its former state was params-subspace data under the params store, so this PR does not add a StoreUpgrades deletion for crisis
  • skip stale/unknown module entries during streaming genesis import and validation, matching the non-streaming behavior for existing genesis files that still include crisis app_state

Test Plan

  • go test ./sei-cosmos/types/module
  • go test ./sei-cosmos/x/genutil/client/cli
  • go test ./app
  • go test ./app ./app/legacyabci ./cmd/seid/cmd ./sei-wasmd/app ./sei-wasmd/x/wasm/keeper ./sei-ibc-go/testing/simapp/... ./sei-db/tools/cmd/seidb/operations
  • 4-node Docker upgrade test: started the pre-removal parent binary 5ac709d, let the chain run, passed the software upgrade at height 306, restarted all validators on the crisis-deletion binary, and verified all nodes reached height 366 with catching_up=false and matching app hash 60C93CD1D72AD425D3FD862D115D722B098FEC48A28FD795C466807403F4648A

Drop x/crisis from the Sei app module manager, init-genesis order, end blocker, invariant registration, init flags, and app-owned keeper state. Crisis has no dedicated mounted KV store in app/app.go, so the rootmulti store key list is unchanged.

Also decouple crisis package tests from the full app now that App no longer exposes CrisisKeeper.

Tested with: go test ./app ./app/legacyabci ./cmd/seid/cmd ./sei-cosmos/x/crisis/...

Upgrade tested with a 4-node Docker cluster: started from the pre-removal binary, passed software upgrade v99.0.1-crisis-removal at height 342, restarted all validators on the removal binary, and verified all nodes reached height 514 with catching_up=false and matching app hash A53F9F78B05C8F6C07247186EF5BFB89D5BCFE7535AE52E54895B57E1844485E.
@cursor
Copy link
Copy Markdown

cursor Bot commented May 27, 2026

PR Summary

High Risk
Drops periodic and export-time invariant checks that could halt the chain on accounting bugs, and changes genesis streaming behavior across live upgrades—mitigated by tests and reported 4-node upgrade validation but still core chain safety surface.

Overview
This PR fully removes the Cosmos x/crisis module from Sei and related harnesses: the sei-cosmos/x/crisis package, crisis protos, CLI (invariant-broken), end-block invariant runs, export-time AssertInvariants, and RegisterInvariants wiring are deleted. The main app, seid, legacyabci, sei-wasmd, and sei-ibc-go simapps no longer construct CrisisKeeper, include crisis in the module manager, or expose x-crisis-skip-assert-invariants; invCheckPeriod remains on New signatures but is unused.

Genesis and tooling are aligned with the removal: local genesis no longer sets app_state.crisis, state-dump / seidb module lists drop crisis, and docs drop the crisis entry. Streaming genesis handling in sei-cosmos/types/module now ignores unknown module names (with tests for crisis chunks), so upgraded chains can import streamed genesis that still mentions removed modules without failing.

There is no dedicated crisis KV store teardown in this diff—crisis data lived under the params subspace, consistent with the PR note that no StoreUpgrades deletion was added.

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 27, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMay 29, 2026, 3:44 AM

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.25%. Comparing base (5ac709d) to head (0001bf8).
⚠️ Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
cmd/seid/cmd/root.go 0.00% 1 Missing ⚠️
sei-ibc-go/testing/simapp/app.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3510      +/-   ##
==========================================
- Coverage   59.30%   59.25%   -0.06%     
==========================================
  Files        2127     2129       +2     
  Lines      175876   176767     +891     
==========================================
+ Hits       104305   104745     +440     
- Misses      62473    62888     +415     
- Partials     9098     9134      +36     
Flag Coverage Δ
sei-chain-pr 49.45% <33.33%> (?)
sei-db 70.41% <ø> (ø)

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

Files with missing lines Coverage Δ
app/app.go 64.69% <ø> (-4.71%) ⬇️
app/export.go 0.00% <ø> (ø)
app/legacyabci/end_block.go 100.00% <ø> (ø)
sei-ibc-go/testing/simapp/export.go 0.00% <ø> (ø)
sei-ibc-go/testing/simapp/simd/cmd/root.go 66.87% <100.00%> (-0.21%) ⬇️
sei-wasmd/app/app.go 84.07% <ø> (-0.51%) ⬇️
sei-wasmd/app/export.go 12.24% <ø> (+0.12%) ⬆️
cmd/seid/cmd/root.go 25.09% <0.00%> (+0.09%) ⬆️
sei-ibc-go/testing/simapp/app.go 76.31% <0.00%> (-1.08%) ⬇️

... and 12 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.

@codchen codchen changed the title Remove crisis module from app wiring Remove crisis module code and store May 28, 2026
Comment thread app/app.go Outdated
@@ -170,8 +169,7 @@ func initRootCmd(rootCmd *cobra.Command, encodingConfig params.EncodingConfig) {
rootCmd.AddCommand(server.RosettaCommand(encodingConfig.InterfaceRegistry, encodingConfig.Marshaler))
}

func addModuleInitFlags(startCmd *cobra.Command) {
crisis.AddModuleInitFlags(startCmd)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This would break the startup for anyone setting the flag.

Perhaps a more graceful approach is to print a WARNING saying this is noop, and flag will be removed in the next release.

Alternatively a more meaningful fatal error to point them to some TBD comms (e.g. release notes) that would explain the module removal).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This simapp is only used in unit tests so I think it should be okay, and once ibc is removed we will remove this simapp altogether too

Comment thread app/app.go Outdated
app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
}

if (upgradeInfo.Name == "v6.6.0") && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Going forward since v6.5, upgrade names will follow v<major>.<minor> format. No more repetitive .0 suffix.

Suggested change
if (upgradeInfo.Name == "v6.6.0") && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
if (upgradeInfo.Name == "v6.6") && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {

Comment thread app/app.go
Comment thread app/app.go Outdated

if (upgradeInfo.Name == "v6.6.0") && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
storeUpgrades := storetypes.StoreUpgrades{
Deleted: []string{crisisStoreKeyName},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But there is no such key name and this would result in an error i believe.

Comment thread app/app.go
Copy link
Copy Markdown
Contributor

@philipsu522 philipsu522 left a comment

Choose a reason for hiding this comment

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

In the past, when updating some fields for modules I believe we saw some incompatibilities. For this particular PR, in the genesis file we see:
https://github.com/sei-protocol/chain-configs/blob/main/pacific-1/genesis.json#L715

We should probably test workflows like state sync'ing with the new binary and the genesis file

@codchen codchen changed the title Remove crisis module code and store Remove crisis module wiring and code Jun 1, 2026
Copy link
Copy Markdown
Collaborator Author

codchen commented Jun 1, 2026

Checked the genesis/state-sync concern.

The concern is valid for paths that stream or validate app_state from existing genesis files, because pacific-1/arctic-1/atlantic-2 genesis still include a crisis entry. Normal JSON genesis import already iterates registered modules and skips unknown module keys, so the removed crisis app_state is not used there. Standard state sync should rely on the genesis doc plus snapshots rather than running removed module InitGenesis from app_state, so I do not see a separate failure in that path from the crisis entry itself.

I did find and fix the risky streaming paths in 667afb9:

  • streaming InitGenesis skips unknown modules before calling InitGenesis
  • streaming ValidateGenesis drains/skips unknown module chunks instead of indexing a missing module
  • added regression coverage using a streamed crisis genesis payload

Validated with:

  • go test ./sei-cosmos/types/module
  • go test ./sei-cosmos/x/genutil/client/cli
  • go test ./app

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 667afb9. Configure here.

Comment thread cmd/seid/cmd/root.go
func addModuleInitFlags(startCmd *cobra.Command) {
crisis.AddModuleInitFlags(startCmd)
}
func addModuleInitFlags(_ *cobra.Command) {}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Removed CLI flag breaks operators using it

Medium Severity

The --x-crisis-skip-assert-invariants flag is silently removed by making addModuleInitFlags a no-op. Any operator who has this flag in their seid start command (e.g. in a systemd unit file or launch script) will get an "unknown flag" error from cobra, preventing the node from starting after the upgrade. As the PR reviewer noted, a graceful deprecation (accepting the flag as a no-op with a warning) would avoid unexpected startup failures.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 667afb9. Configure here.

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