feat: upgrade to cosmos-sdk v0.54.3 + ibc-go v11 + cometbft v0.39#2080
feat: upgrade to cosmos-sdk v0.54.3 + ibc-go v11 + cometbft v0.39#2080JayT106 wants to merge 49 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCoordinated upgrade: Go toolchain to 1.25.9, Cosmos SDK imports migrated to v0.54/v2 packages, IBC-Go v10→v11, App.New/keeper refactors, ICA/ICS4 middleware wiring changes, proposal fast-path and governance updates, upgrade handler/store-loader adjustments, and dependency pin refreshes. ChangesCosmos SDK v0.54 and IBC-Go v11 migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/architecture/cosmos-sdk-v0.54-upgrade-plan.md (1)
22-45: ⚡ Quick winAvoid forcing persistent global git rewrites in the default setup path.
Using
git config --global ... insteadOfcan leak into unrelated repositories and surprise developers. Please either (a) document a repo-scoped config alternative first, or (b) include explicit cleanup commands in this section.Suggested doc tweak
-## Pre-merge dev setup (insteadOf redirect) +## Pre-merge dev setup (scoped redirect preferred) -Configure git to redirect: +Prefer repository-scoped redirect (run from this repo): + +```bash +git config --local url."https://github.com/JayT106/ethermint".insteadOf \ + "https://github.com/crypto-org-chain/ethermint" +git config --local url."https://github.com/JayT106/cronos-store".insteadOf \ + "https://github.com/crypto-org-chain/cronos-store" +``` + +If you use `--global`, add cleanup after PRs merge: + +```bash +git config --global --unset-all url."https://github.com/JayT106/ethermint".insteadOf +git config --global --unset-all url."https://github.com/JayT106/ethermint.git".insteadOf +git config --global --unset-all url."https://github.com/JayT106/cronos-store".insteadOf +git config --global --unset-all url."https://github.com/JayT106/cronos-store.git".insteadOf +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/cosmos-sdk-v0.54-upgrade-plan.md` around lines 22 - 45, The docs currently recommend using git config --global url."...".insteadOf which can leak into other repos; change the instructions to prefer a repo-scoped config using git config --local url."...".insteadOf for the two redirect entries (the existing git config --global url."https://github.com/JayT106/ethermint".insteadOf / git config --global url."https://github.com/JayT106/ethermint.git".insteadOf and the cronos-store equivalents) and keep the go env -w GOPRIVATE guidance, and if you still document the --global option include explicit cleanup commands using git config --global --unset-all for each url."...".insteadOf entry so developers can revert the global rewrites after the upstream PRs merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/cosmos-sdk-v0.54-upgrade-plan.md`:
- Line 202: The link at line 202 uses the floating `/sdk/next/upgrade/upgrade`
URL which can drift; update the markdown reference text "[Cosmos SDK v0.54
upgrade docs](https://cosmos-docs.mintlify.app/sdk/next/upgrade/upgrade)" to a
version-pinned v0.54 docs URL (replace `/sdk/next/` with the v0.54 path) so the
runbook always points to the stable v0.54 upgrade documentation.
- Line 11: Replace the vague version token "v11.x" for the ibc-go dependency
with the exact release "v11.0.0" in the upgrade plan; locate the table row
containing the "ibc-go" entry and update its version cell from "v11.x" to
"v11.0.0" so the document pins the baseline release for Cosmos SDK v0.54
rehearsals.
In `@go.mod`:
- Line 3: The module's Go version directive currently reads "go 1.26.3" which
conflicts with CI's pinned setup-go version 1.25.9; update the go directive in
go.mod from "go 1.26.3" to "go 1.25.9" so the module and CI toolchain match (or
alternatively update the CI setup-go pins to 1.26.3), and ensure any references
to the Go version in CI workflows (.github/workflows/*) are consistent with the
updated go.mod.
---
Nitpick comments:
In `@docs/architecture/cosmos-sdk-v0.54-upgrade-plan.md`:
- Around line 22-45: The docs currently recommend using git config --global
url."...".insteadOf which can leak into other repos; change the instructions to
prefer a repo-scoped config using git config --local url."...".insteadOf for the
two redirect entries (the existing git config --global
url."https://github.com/JayT106/ethermint".insteadOf / git config --global
url."https://github.com/JayT106/ethermint.git".insteadOf and the cronos-store
equivalents) and keep the go env -w GOPRIVATE guidance, and if you still
document the --global option include explicit cleanup commands using git config
--global --unset-all for each url."...".insteadOf entry so developers can revert
the global rewrites after the upstream PRs merge.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 436950d0-eee8-4267-8674-49af553a9d90
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
.github/workflows/codeql-analysis.yml.github/workflows/lint.yml.github/workflows/sims.ymlapp/app.goapp/bench_test.goapp/export.goapp/proposal.goapp/proposal_test.goapp/sim_test.goapp/storeloader.goapp/test_helpers.goapp/upgrades.goapp/versiondb.goapp/versiondb_placeholder.gocmd/cronosd/cmd/root.gocmd/cronosd/dbmigrate/migrate.gocmd/cronosd/dbmigrate/migrate_basic_test.gocmd/cronosd/dbmigrate/migrate_dbname_test.gocmd/cronosd/dbmigrate/migrate_enhanced_test.gocmd/cronosd/dbmigrate/migrate_rocksdb_test.gocmd/cronosd/dbmigrate/migrate_test.gocmd/cronosd/dbmigrate/patch.gocmd/cronosd/dbmigrate/patch_advanced_test.gocmd/cronosd/dbmigrate/patch_integration_test.godocs/architecture/cosmos-sdk-v0.54-upgrade-plan.mdgo.modgomod2nix.tomltestutil/simapp/simapp.gox/cronos/client/cli/tx.gox/cronos/events/decoders.gox/cronos/events/events.gox/cronos/keeper/ibc.gox/cronos/keeper/keeper.gox/cronos/keeper/keeper_test.gox/cronos/keeper/mock/ibckeeper_mock.gox/cronos/keeper/params.gox/cronos/keeper/precompiles/bank.gox/cronos/keeper/precompiles/ica.gox/cronos/keeper/precompiles/relayer.gox/cronos/middleware/conversion_middleware.gox/cronos/middleware/conversion_middleware_test.gox/cronos/migrations/v2/migrate.gox/cronos/migrations/v2/migrate_test.gox/cronos/rpc/api.gox/cronos/types/interfaces.gox/cronos/types/proposal.gox/e2ee/keeper/keeper.gox/e2ee/keyring/keyring.go
💤 Files with no reviewable changes (1)
- app/proposal_test.go
efa739a to
9374171
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/upgrades.go`:
- Around line 35-38: The current code calls
UpgradeKeeper.ReadUpgradeInfoFromDisk() and returns false on error, silently
skipping critical upgrade/store-loader setup; change the implementation so the
error is not swallowed: propagate or return the error (e.g., change the caller
signature to return (bool, error) or return the error up the stack) instead of
returning false, and ensure any callers of this function handle the error
appropriately; locate the call to UpgradeKeeper.ReadUpgradeInfoFromDisk and the
surrounding logic that uses upgradeInfo to wire the error propagation through or
fail fast (log/return) so disk read failures cannot be ignored.
In `@gomod2nix.toml`:
- Around line 296-306: The three cronos-store module entries in gomod2nix.toml
([mod."github.com/crypto-org-chain/cronos-store/memiavl"],
[mod."github.com/crypto-org-chain/cronos-store/store"],
[mod."github.com/crypto-org-chain/cronos-store/versiondb"]) are pinned to commit
704a4bc6440f while go.mod references commit 596a1419d7a1, causing inconsistent
dependency graphs; fix by reconciling versions so both files match—either update
the three version fields in gomod2nix.toml to the same pseudo-version that
includes 596a1419d7a1 (and update the corresponding hash values to the correct
sha256 entries from go.sum or from a regenerated gomod2nix output) or regenerate
gomod2nix.toml from the current go.mod so the module entries and hashes for the
cronos-store modules align with the commit referenced in go.mod.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1783d3a3-a5be-4c22-9a47-b29f0e352209
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
.github/workflows/codeql-analysis.yml.github/workflows/lint.yml.github/workflows/sims.ymlapp/app.goapp/bench_test.goapp/export.goapp/proposal.goapp/proposal_test.goapp/sim_test.goapp/storeloader.goapp/test_helpers.goapp/upgrades.goapp/versiondb.goapp/versiondb_placeholder.gocmd/cronosd/cmd/root.gocmd/cronosd/dbmigrate/migrate.gocmd/cronosd/dbmigrate/migrate_basic_test.gocmd/cronosd/dbmigrate/migrate_dbname_test.gocmd/cronosd/dbmigrate/migrate_enhanced_test.gocmd/cronosd/dbmigrate/migrate_rocksdb_test.gocmd/cronosd/dbmigrate/migrate_test.gocmd/cronosd/dbmigrate/patch.gocmd/cronosd/dbmigrate/patch_advanced_test.gocmd/cronosd/dbmigrate/patch_integration_test.godocs/architecture/cosmos-sdk-v0.54-upgrade-plan.mdgo.modgomod2nix.tomltestutil/simapp/simapp.gox/cronos/client/cli/tx.gox/cronos/events/decoders.gox/cronos/events/events.gox/cronos/keeper/ibc.gox/cronos/keeper/keeper.gox/cronos/keeper/keeper_test.gox/cronos/keeper/mock/ibckeeper_mock.gox/cronos/keeper/params.gox/cronos/keeper/precompiles/bank.gox/cronos/keeper/precompiles/ica.gox/cronos/keeper/precompiles/relayer.gox/cronos/middleware/conversion_middleware.gox/cronos/middleware/conversion_middleware_test.gox/cronos/migrations/v2/migrate.gox/cronos/migrations/v2/migrate_test.gox/cronos/rpc/api.gox/cronos/types/interfaces.gox/cronos/types/proposal.gox/e2ee/keeper/keeper.gox/e2ee/keyring/keyring.go
💤 Files with no reviewable changes (1)
- app/proposal_test.go
✅ Files skipped from review due to trivial changes (10)
- x/cronos/migrations/v2/migrate_test.go
- cmd/cronosd/dbmigrate/migrate_basic_test.go
- app/storeloader.go
- x/cronos/keeper/mock/ibckeeper_mock.go
- x/cronos/types/proposal.go
- x/cronos/events/decoders.go
- cmd/cronosd/dbmigrate/migrate.go
- .github/workflows/sims.yml
- .github/workflows/codeql-analysis.yml
- cmd/cronosd/dbmigrate/patch_advanced_test.go
d7524ed to
79983fa
Compare
6a2b173 to
e8437d0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2080 +/- ##
==========================================
- Coverage 16.87% 15.58% -1.30%
==========================================
Files 72 77 +5
Lines 6163 5249 -914
==========================================
- Hits 1040 818 -222
+ Misses 5000 4351 -649
+ Partials 123 80 -43
🚀 New features to boost your workflow:
|
0063035 to
adf8c57
Compare
- app/proposal: cap NoOp PrepareProposal selected gas at consensus MaxGas (decode-on-demand) so proposals do not exceed the block gas budget. Document that non-FeeTx is admitted without gas accounting — unreachable in practice since all cronos txs implement sdk.FeeTx. - middleware/conversion: only commit cacheCtx when the transfer ack succeeds, preventing persisted cacheCtx writes when the underlying transfer fails. - precompiles/ica: early-return baseCost on unknown method ID before checking SubmitMsgsMethodName, making the control flow explicit. - app/upgrades: clarify v1.8 PopulateQueuePendingSlots is idempotent (keeper Set overwrites the per-time slot). - app/mempool/reap_test: add post-quiesce double-reap count-stability check to the concurrent insert race test; reference cosmos/cosmos-sdk#1751 for the upstream per-tx uniqueness issue deferred from this scope. - Makefile: add test-race-mempool target; CI: add race-detector mempool step. - integration_tests/test_app_mempool.py: collapse isort import to one line.
8d6bdec to
0c2fa8a
Compare
Ethermint @ eb38211d (jt/feat-sdk-54) builds against crypto-org-chain/go-ethereum release/v1.16 which adds SkipTransactionChecks to core.Message. The branch was accidentally pinned to release/v1.15 (a4fbafcae0dd) after the rebase-main merge resolved the replace directive conflict on the wrong side. Update go.mod replace directive to the release/v1.16 pseudoversion and refresh gomod2nix.toml hashes accordingly.
PrepareProposal decodes each tx to read gas (when MaxBlockGas > 0); BaseApp.runTx decodes the same bytes again during FinalizeBlock. Install a shared caching decoder via bApp.SetTxDecoder so both paths share work. Implementation: - 16 shards × 625-entry LRU (container/list + per-shard sync.Mutex) - LRU eviction keeps re-proposed txs warm across block rounds - Per-entry 64KB size cap bounds worst-case memory to ~640MB - xxhash routing with byte-level collision guard - cachingDecoder also installed on app.txDecoder (CheckTx path) Verified: go test -race ./app/... passes; 7 focused cache tests cover hit/miss, error-not-cached, LRU order, eviction bound, concurrency, and size-capped payloads.
|
as follow up we can override |
0c2fa8a to
3aca3d1
Compare
I think so. I added some cache layer for decoder in this PR. |
… DoS Without a seed, xxhash.Sum64 is deterministic and public — an attacker can precompute which tx bytes land in which shard and flood a single shard with 625 crafted (valid, signed) txs, evicting all real txs and spiking cache miss rate for 1/16 of traffic. XOR with a per-process random seed at startup makes shard assignment unpredictable. Preserves xxhash distribution (XOR with constant is a bijection). Correctness is unaffected: bytes.Equal guards against both collisions and cross-shard lookups.
Add cronos.disable-tx-decode-cache app.toml flag. When true, the raw TxDecoder is used directly instead of the sharded LRU caching decoder. Useful for debugging or profiling to isolate cache effects. Default (unset / false) keeps the cache active.
|
Overall looks fine but can you change the PR description to split
This PR seems to do too many things, for reference |
Pin cosmos-sdk fork to pre-PR-1815 commit (7d79ddc38152), reverting the SDK-default InsertTx AnteHandler path. Implement equivalent validation in cronos itself via NewInsertTxHandler in app/mempool/insert.go. - Runs RunTx(ExecModeCheck) on each peer-relayed tx before admission, mirroring CheckTx semantics: bad-sig/replay/wrong-chainID rejected at insertion, not at FinalizeBlock. - FIFO seen-cache (default 16384, tunable via mempool.insert-tx-cache-size) deduplicates AnteHandler cost on gossip fan-out (~82ns cache-hit vs ~500µs RunTx). - ErrMempoolTxMaxCapacity maps to CodeTypeRetry so peers backoff gracefully. - txRunner interface keeps BaseApp dependency injectable for unit tests.
- upgrades.go: extract planName const instead of inline "v1.8" literal
- proposal.go: fix uint64 overflow in gas accumulation (gasWanted > maxBlockGas-totalGas)
- app.go: panic on unrecognized mempool.type instead of silent warn
- decode_cache.go: fix misspelling randomises→randomizes
- mempool/insert{,_test}.go: fix gci import ordering
Previous test asserted parent.lastMemTx==nil (old buggy behavior). Replace with: - assertion that real memTx reaches parent unmodified - gas-cap integration test: gasCapSelector rejects second tx when first already consumed 60k of 100k budget, proving the nil-was-bug fix
Defensive ordering: build pending-slot indexes on fully-migrated queue keys so a future staking migration that rewrites ValidatorQueueKey or UnbondingQueueKey doesn't leave the indexes stale at upgrade height.
|
Splitting this PR per @thomas-nguy's request in #issuecomment-4572434133.
Closing this PR. |
- fix(exchange): add Name() to ExchangeContract to satisfy vm.PrecompiledContract - fix(upgrades): move PopulateQueuePendingSlots after RunMigrations so indexes are built on fully-migrated queue keys - test(proposal): add test proving ExtTxSelector validates with original memTx but forwards nil to parent to bypass block-gas enforcement - docs(middleware): clarify IBCConversionModule only implements IBCModule, not full porttypes.Middleware (missing SetUnderlyingApplication)
- upgrades.go: extract planName const instead of inline "v1.8" literal
- proposal.go: fix uint64 overflow in gas accumulation (gasWanted > maxBlockGas-totalGas)
- app.go: panic on unrecognized mempool.type instead of silent warn
- decode_cache.go: fix misspelling randomises→randomizes
- mempool/insert{,_test}.go: fix gci import ordering
- upgrades.go: extract planName const instead of inline "v1.8" literal
- proposal.go: fix uint64 overflow in gas accumulation (gasWanted > maxBlockGas-totalGas)
- app.go: panic on unrecognized mempool.type instead of silent warn
- decode_cache.go: fix misspelling randomises→randomizes
- mempool/insert{,_test}.go: fix gci import ordering
- upgrades.go: extract planName const instead of inline "v1.8" literal
- proposal.go: fix uint64 overflow in gas accumulation (gasWanted > maxBlockGas-totalGas)
- app.go: panic on unrecognized mempool.type instead of silent warn
- decode_cache.go: fix misspelling randomises→randomizes
- mempool/insert{,_test}.go: fix gci import ordering
- upgrades.go: extract planName const instead of inline "v1.8" literal
- proposal.go: fix uint64 overflow in gas accumulation (gasWanted > maxBlockGas-totalGas)
- app.go: panic on unrecognized mempool.type instead of silent warn
- decode_cache.go: fix misspelling randomises→randomizes
- mempool/insert{,_test}.go: fix gci import ordering
Summary
cosmossdk.io/{store,log,x/evidence,x/feegrant,x/upgrade,x/tx}standalone modules to in-treegithub.com/cosmos/cosmos-sdkpaths (store/v2, log/v2, contrib/x/crisis)PopulateQueuePendingSlotsfeat(staking): expose queue migration as utility, keep consensus version at 5 cosmos-sdk#1814fastNoOpPrepareProposal: skip per-txTxDecodefor NoOp/nil mempool; delegate to upstream default for priority mempoolsmempool.type=app: cronos overridesReapTxs(app/mempool/reap.go) forMaxBytes/MaxGascap;InsertTxuses cosmos-sdk fork default (runs AnteHandler chain viaRunTx(execModeCheck), mapsErrMempoolIsFull→CodeTypeRetry)ExtTxSelectorgas cap: passmemTxthrough to parent selector sotxGasLimitaccumulates andmaxBlockGastriggers (prevented 1B-gas blocks under app-mempool)PriorityNonceMempoolmutex during reap encode: snapshot viaSelectByunder lock, encode + cap loop outside;Iteratoris not thread-safe so concurrentInsertwas racing the priority skiplistcompat_mode: '0.38'+index-events: []for v0.39 ABCIPrepareProposal(gas accounting) andBaseApp.runTx(FinalizeBlock); installed viabApp.SetTxDecoderso all decode paths (PrepareProposal, runTx, CheckTx) share work; disable able viacronos.disable-tx-decode-cache = trueinapp.tomlProtocol Upgrade Notice
v1.8upgrade plan height.v1.8. Operators must use the matching binary; mismatched binaries will halt or fork.PopulateQueuePendingSlotsruns once at upgrade height to populate staking queue pending-slot indexes (opt-in optimization; idempotent re-runs not supported).StoreUpgrades. Both regular and upgrade-height boots use cronos'sMaxVersionStoreLoader(the existing custom loader).upgrade-info.jsonflow; no manual store ops.Relayer / Hermes Setup
nix/hermes.nix.relayer.tomlper-chain config:compat_mode = '0.38'— required so hermes treats v0.39 ABCI as Tendermint 0.38-compatible.event_source = { mode = 'push', url = 'ws://...:26657/websocket', batch_delay = '500ms' }(existing config retained).config.toml/app.toml):index-events = [](index all) — required so hermes can pull packet data; non-empty filter dropssend_packetevents and breaks relaying.compat_modechange; verify withhermes health-check.App-mempool (
mempool.type=app) operator notesmempool.type=flood) unchanged; the cronosReapTxsABCI hook is not registered.app, CometBFT drives the priority mempool via ABCI:InsertTxuses BaseApp's default (cosmos-sdk fork:feat(baseapp): run AnteHandler in default InsertTx handler+map ErrMempoolIsFull to CodeTypeRetry). Peer-relayed txs run the full AnteHandler chain before mempool admission. Equivalent peer-trust posture toflood.ReapTxsis overridden by cronos: snapshots the priority mempool under lock, then encodes + appliesRequestReapTxs.MaxBytes/MaxGasoutside the lock.flood). Per-tx admission cost ≈ RPCCheckTxcost; net throughput unchanged.mempool.typelogsWarnand falls back to flood; no panic.Tx-decode cache (
app.toml)cronos.disable-tx-decode-cachefalsetrueto bypass the sharded LRU cache and use the raw decoder. Useful for debugging or profiling to isolate cache effects.Follow-ups
SetRouterV2, transfer v2, attestation v2)