fix: staking optimization #1907
Conversation
randy-cro
requested review from
calvinaco and
songgaoye
and removed request for
a team
This comment has been minimized.
This comment has been minimized.
WalkthroughApp startup now exposes and mounts memory-backed stores (new memKeys) and wires them into keepers; staking CLI and integration tests add staking flows and cache-size scenarios; workflow test matrix gains a Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Init
participant StoreMgr as MemoryStores
participant Keepers as Keepers (Staking/Cronos)
participant KV as PersistentKV
rect rgb(247,250,255)
note over App,StoreMgr: Startup — expose & mount memory stores
App->>StoreMgr: StoreKeys() -> memKeys
App->>App: MountMemoryStores(memKeys)
App->>Keepers: New(..., MemStoreService(memKeys[staking]))
end
rect rgb(240,248,255)
note over Keepers,StoreMgr: Runtime — cache lookup/update flow
Keepers->>StoreMgr: query cache for queue entries
alt cache hit
StoreMgr-->>Keepers: cached entries
else cache miss
Keepers->>KV: iterate persistent KV for entries
KV-->>Keepers: entries
Keepers->>StoreMgr: update cache
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
bab1a75 to
65fb87c
Compare
|
Lets change the target to v1.6.x is scheduled to be released soon and we do not have time to test this feature |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app.go (1)
387-407: Consider validating feeBump range and documenting arithmetic assumptions.The TxReplacement calculation
np >= op*threshold/100wherethreshold = 100 + feeBumpcould overflow if bothop(old priority) andfeeBumpare very large. While priorities are typically bounded in practice, consider:
- Adding validation for feeBump (e.g.,
feeBump < 0 || feeBump > 10000)- Documenting the expected priority range assumptions
- Using safer arithmetic (e.g., rearranging to
np*100 >= op*thresholdwith overflow checks)Example validation:
feeBump := cast.ToInt64(appOpts.Get(FlagMempoolFeeBump)) + if feeBump < 0 || feeBump > 10000 { + logger.Error("Invalid feeBump value, must be between 0 and 10000", "feeBump", feeBump) + feeBump = 0 + } if mempoolMaxTxs >= 0 && feeBump >= 0 {
🧹 Nitpick comments (2)
CHANGELOG.md (1)
5-5: Grammar suggestion: Use hyphenated compound modifiers.As per static analysis hint, compound modifiers before nouns should be hyphenated:
- "in memory KV store" → "in-memory KV store"
- "staking related messages" → "staking-related messages"
Based on learnings
cmd/cronosd/opendb/opendb_rocksdb.go (1)
58-59: Based on the verification, here is the rewritten review comment:Verify AsyncIO support for multi-platform deployments and consider making readahead size configurable.
The AsyncIO and 16MB readahead settings are hardcoded performance optimizations. Based on verification:
AsyncIO platform support: AsyncIO in grocksdb v1.10.2 is experimental and platform/FS-dependent, with no guaranteed support on macOS or Windows. While AsyncIO was intentionally enabled in this codebase, it may not provide actual asynchronous I/O on all target platforms.
Hardcoded readahead size: The 16MB readahead is not configurable, yet different deployment scenarios (resource-constrained nodes vs. high-performance validators) may benefit from different values.
Consider making these tunable via configuration to allow operators to optimize for their specific hardware and platform:
func openRocksdb(dir string, readonly bool) (dbm.DB, error) { + readaheadSize := viper.GetInt64("rocksdb.readahead-size") + if readaheadSize == 0 { + readaheadSize = 16 * 1024 * 1024 // default 16MB + } + enableAsyncIO := viper.GetBool("rocksdb.async-io") + ro := grocksdb.NewDefaultReadOptions() - ro.SetAsyncIO(true) - ro.SetReadaheadSize(16 * 1024 * 1024) + ro.SetAsyncIO(enableAsyncIO) + ro.SetReadaheadSize(readaheadSize)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumversiondb/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(11 hunks)app/block_address.go(2 hunks)cmd/cronosd/cmd/root.go(3 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)cmd/cronosd/config/config.go(1 hunks)cmd/cronosd/config/toml.go(1 hunks)cmd/cronosd/opendb/opendb_rocksdb.go(1 hunks)default.nix(1 hunks)go.mod(3 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/default.jsonnet(2 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/contracts/contracts/TestBlockTxProperties.sol(1 hunks)integration_tests/cosmoscli.py(4 hunks)integration_tests/test_basic.py(1 hunks)integration_tests/test_mempool.py(2 hunks)integration_tests/test_staking_cache.py(1 hunks)integration_tests/utils.py(1 hunks)nix/rocksdb.nix(1 hunks)versiondb/go.mod(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
app/block_address.go (1)
testground/benchmark/benchmark/cosmostx.py (1)
MsgEthereumTx(80-86)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(153-195)integration_tests/utils.py (1)
wait_for_new_blocks(127-135)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
app/app.go (2)
store/setup.go (1)
SetupMemIAVL(29-59)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/test_mempool.py (2)
integration_tests/utils.py (4)
get_account_nonce(455-457)replace_transaction(466-470)send_transaction(460-463)wait_for_new_blocks(127-135)integration_tests/network.py (3)
w3(39-42)cosmos_cli(56-57)cosmos_cli(81-82)
integration_tests/test_basic.py (2)
integration_tests/network.py (1)
w3(39-42)integration_tests/utils.py (2)
deploy_contract(415-435)send_transaction(460-463)
cmd/cronosd/cmd/versiondb.go (1)
app/app.go (1)
StoreKeys(233-263)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(53-54)node_rpc(78-79)
cmd/cronosd/cmd/root.go (2)
cmd/cronosd/config/config.go (2)
CronosConfig(12-17)DefaultCronosConfig(19-24)cmd/cronosd/config/toml.go (1)
DefaultCronosConfigTemplate(4-16)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...fix: Optimize staking endblocker with in memory KV store and fix gas consumption ...
(QB_NEW_EN_HYPHEN)
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...tore and fix gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
17-17: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
30-30: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
38-38: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.4)
integration_tests/test_staking_cache.py
39-39: Unused function argument: node_index
(ARG001)
171-171: Do not catch blind exception: Exception
(BLE001)
217-217: Do not catch blind exception: Exception
(BLE001)
361-361: Do not catch blind exception: Exception
(BLE001)
409-409: Do not catch blind exception: Exception
(BLE001)
705-705: Do not catch blind exception: Exception
(BLE001)
749-749: Do not catch blind exception: Exception
(BLE001)
824-824: Do not catch blind exception: Exception
(BLE001)
844-844: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (22)
app/block_address.go (1)
7-7: LGTM!The import is necessary for EIP-7702 authorization validation.
default.nix (1)
14-14: LGTM!Version metadata updated correctly from v1.5.0 to v1.5.4.
versiondb/go.mod (1)
19-19: LGTM!Grocksdb dependency updated from pseudo-version to stable release v1.10.2, aligning with the root go.mod changes.
go.mod (2)
36-36: LGTM! Grocksdb version updated.The grocksdb dependency is updated to a stable release v1.10.2, which aligns with the RocksDB 10.4.2 upgrade in the Nix configuration.
300-300: Cometbft fork version b0e778b18408 is correct and properly maintained.Verification confirms the commit is a legitimate PR #10 merge (Oct 15, 2025) on the v0.38.x branch. The patch adds essential validation to BitArray structures in consensus messages, preventing state corruption through inconsistency checks between bit count and element array length. Changes include:
- New
ValidateBasic()method for BitArray consistency validation- Validation calls in three consensus message types
- Defensive bounds checking in bit operations
- Comprehensive test coverage for invalid states
The patch addresses a real correctness issue and is properly tested.
gomod2nix.toml (1)
197-199: LGTM! Dependency hashes updated correctly.The cometbft and grocksdb version updates with corresponding hash changes appear correct and align with the go.mod updates.
Also applies to: 517-518
cmd/cronosd/config/toml.go (1)
1-16: LGTM! CronosConfig struct correctly matches template.The TOML template and
CronosConfigstruct are in sync:
DisableTxReplacementandDisableOptimisticExecutionfields defined with matching mapstructure tags- TOML keys (
disable-tx-replacement,disable-optimistic-execution) properly mappednix/rocksdb.nix (1)
24-30: I need to gather more information to fully assess the upgrade concerns.Based on my verification, I can now address the review comment with confidence.
Backward compatibility is guaranteed by RocksDB for database format.
RocksDB's compatibility guarantee states that "newer version of RocksDB should be able to open DBs generated by all previous releases for normal configuration." RocksDB maintains backward compatibility by understanding all formats previously written to disk, which is central to its design for large-scale distributed systems.
The upgrade path from 9.11.2 to 10.4.2 is supported. The breaking changes in v10.0 (C++20 compiler requirement, cache defaults, API signatures) are at the application compilation level, not the database format level—they affect code that compiles against RocksDB headers, not existing persisted databases.
Since this is a Nix package update that provides the RocksDB binary, applications using existing 9.x databases will be able to read them with the 10.4.2 binary without migration. The upgrade is a normal major version bump with no special on-disk format migration required for the stated use case.
The original review's cautionary recommendations (test with actual data, keep backups) remain sensible operational practices, but are not blocking concerns for this version bump.
app/app.go (7)
115-115: LGTM!The new import and flag definitions are properly structured and follow existing conventions.
Also applies to: 177-182
293-293: LGTM!The memKeys field addition is consistent with other store key fields in the App struct.
428-442: LGTM! Clear concurrency-safety logic.The conditional memiavl cache and optimistic execution setup correctly handles concurrency constraints. The comment explaining why the cache is disabled for concurrent executors is helpful.
452-452: LGTM!The StoreKeys call and memKeys assignment are correctly updated to handle the new memory store keys.
Also applies to: 466-466
984-984: LGTM!Memory stores are correctly mounted following the same pattern as other store types.
233-263: All callers of StoreKeys() correctly handle the updated return signature.The search confirms both call sites already unpack all 4 return values:
cmd/cronosd/cmd/versiondb.go:17useskeys, _, _, _app/app.go:452useskeys, tkeys, okeys, memKeysThe breaking change has been properly handled throughout the codebase.
991-1006: Verify that ethermint'scache.NewAnteCache()correctly handles -1 to disable tx replacement caching.The code logic is correct: when
FlagDisableTxReplacementis set,mempoolCacheMaxTxsis set to -1 and passed through tocache.NewAnteCache(). However, the ethermint package (local forkv0.0.0-20251112055637-cce6d2501268) behavior with the -1 parameter could not be verified without access to its source. Confirm that the external package properly interprets -1 as a signal to disable caching, or verify the intended semantics align with the implementation.cmd/cronosd/cmd/versiondb.go (1)
17-17: LGTM!Correctly handles the updated StoreKeys signature by ignoring the memory keys return value, which is not needed for the versiondb command.
integration_tests/configs/default.jsonnet (1)
31-31: LGTM!The feebump configuration value of 10 (10% minimum increase) is reasonable for integration testing the new mempool transaction replacement feature.
.github/workflows/test.yml (1)
22-22: LGTM!Adding staking tests to the CI matrix is appropriate given the staking cache optimization introduced in this PR.
integration_tests/utils.py (1)
66-66: LGTM!The TestBlockTxProperties contract is properly registered following the existing TEST_CONTRACTS pattern.
cmd/cronosd/cmd/root.go (1)
15-15: LGTM!The CronosConfig is properly integrated into the application configuration structure, following the established pattern for other config sections.
Also applies to: 276-276, 285-285, 288-288
cmd/cronosd/config/config.go (1)
12-24: LGTM!The CronosConfig struct is well-defined with clear documentation and sensible defaults (both optimizations enabled by default).
integration_tests/contracts/contracts/TestBlockTxProperties.sol (1)
1-17: LGTM!The TestBlockTxProperties contract is well-structured for testing transaction properties, capturing relevant tx and msg globals in the emitted event.
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
e53a31f to
65fb87c
Compare
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
65fb87c to
475ba9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/app.go (1)
654-658: Pass the Cronos memory store key, not a KV key.CronosKeeper.NewKeeper(cdc, storeKey, memKey, ...) should get a MemoryStoreKey. Passing keys[cronostypes.MemStoreKey] will be nil (not created) and of wrong type. Use memKeys instead (after adding it as suggested).
- app.CronosKeeper = *cronoskeeper.NewKeeper( - appCodec, - keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + app.CronosKeeper = *cronoskeeper.NewKeeper( + appCodec, + keys[cronostypes.StoreKey], + memKeys[cronostypes.MemStoreKey],integration_tests/cosmoscli.py (1)
460-519: Fix gas_prices mismatch in staking methods to prevent test failures.DEFAULT_GAS_PRICE ("100000000000basetcro" = 1e11) is 50x below staking_cache.jsonnet's minimum-gas-prices ("5000000000000basetcro" = 5e12). Staking transactions using delegate_amount, unbond_amount, and redelegate_amount will be rejected during test_staking_cache.py tests. Either:
- Bump DEFAULT_GAS_PRICE to match or exceed the config minimum, or
- Override gas_prices in these methods when used with staking_cache configs
♻️ Duplicate comments (1)
gomod2nix.toml (1)
215-218: Critical: deps point to a personal fork; pin to org-owned or upstream tags.Replacing to github.com/randy-cro/{cosmos-sdk,ethermint} is risky for production. Use an organization-owned fork (e.g., crypto-org-chain/*) or upstream with immutable tags, and align go.mod replaces. Regenerate gomod2nix hashes afterward.
Run to locate and cross-check go.mod replaces and refresh nix data:
#!/bin/bash set -euo pipefail # Show relevant replaces in go.mod rg -nP '^\s*replace\s+.+\s+=>\s+.+(cosmos-sdk|ethermint)' go.mod || true # Verify gomod2nix entries point to same modules/versions rg -n 'github.com/(randy-cro|crypto-org-chain)/(cosmos-sdk|ethermint)' gomod2nix.toml # If you switch repos/tags, regenerate: # nix run .#gomod2nix -- -out gomod2nix.tomlAlso applies to: 315-317
🧹 Nitpick comments (3)
CHANGELOG.md (1)
5-5: Wording nit: hyphenate “in-memory”.Change “in memory KV store” to “in-memory KV store” for clarity.
-* fix: Optimize staking endblocker with in memory KV store and fix gas consumption for staking related messages +* fix: Optimize staking endblocker with in-memory KV store and fix gas consumption for staking-related messagesapp/app.go (1)
1222-1236: Expose memory store keys too (optional).GetStoreKeys() omits memKeys. Including them can help executors/diagnostics that consume the full set of stores.
- keys := make([]storetypes.StoreKey, 0, len(app.keys)+len(app.tkeys)+len(app.okeys)) + keys := make([]storetypes.StoreKey, 0, len(app.keys)+len(app.tkeys)+len(app.okeys)+len(app.memKeys)) ... for _, key := range app.okeys { keys are appended } + for _, key := range app.memKeys { + keys = append(keys, key) + }integration_tests/test_staking_cache.py (1)
39-47: Remove unused parameter.get_validator_address doesn’t use node_index. Drop it or rename to _ to appease linters.
-def get_validator_address(cli, node_index): +def get_validator_address(cli): """Get validator address for a specific node.""" return cli.address("validator", bech="val")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(2 hunks)gomod2nix.toml(2 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(4 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/cronosd/cmd/versiondb.go (1)
app/app.go (1)
StoreKeys(228-258)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
app/app.go (1)
x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
🪛 GitHub Check: Lint python
integration_tests/test_staking_cache.py
[failure] 92-92:
./integration_tests/test_staking_cache.py:92:56: BLK100 Black would make changes.
🪛 Ruff (0.14.4)
integration_tests/test_staking_cache.py
39-39: Unused function argument: node_index
(ARG001)
171-171: Do not catch blind exception: Exception
(BLE001)
217-217: Do not catch blind exception: Exception
(BLE001)
361-361: Do not catch blind exception: Exception
(BLE001)
409-409: Do not catch blind exception: Exception
(BLE001)
705-705: Do not catch blind exception: Exception
(BLE001)
749-749: Do not catch blind exception: Exception
(BLE001)
824-824: Do not catch blind exception: Exception
(BLE001)
844-844: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
cmd/cronosd/cmd/versiondb.go (1)
16-16: Signature drift handled correctly.Ignoring the new 4th return from app.StoreKeys() is fine for this command. No action needed.
integration_tests/cosmoscli.py (2)
22-22: Add constant: good default for staking tx gas.STAKING_DEFAULT_GAS centralizes staking gas; makes tests predictable.
413-459: New staking query helpers look good.get_delegations / get_unbonding_delegations / get_redelegations simplify tests and return normalized lists.
.github/workflows/test.yml (1)
22-22: CI matrix updated to run staking tests.Good addition; ensures coverage of the new suite.
app/app.go (1)
504-515: Verify the custom cosmos-sdk fork definesserver.FlagStakingCacheSize.This codebase uses a custom fork (
github.com/randy-cro/cosmos-sdk v0.0.0-20251112055410-d2e10806c1d7via go.mod replace), andserver.FlagStakingCacheSizeis not a standard Cosmos SDK flag. The flag is used at line 504 but not defined in the current codebase—it must come from the fork.Confirm the fork revision includes this flag definition, or define it locally:
- stakingCacheSize := cast.ToInt(appOpts.Get(server.FlagStakingCacheSize)) + const flagStakingCacheSize = "staking-cache-size" + stakingCacheSize := cast.ToInt(appOpts.Get(flagStakingCacheSize))If the custom fork is known to define it, cite the fork commit/branch.
integration_tests/configs/staking_cache.jsonnet (1)
18-23: Config properly wired to pass cache-size through appOpts to StakingKeeper.The per-node staking cache-size values are correctly wired:
- jsonnet app-config → pystarport generates TOML config files
- App dynamically reads from appOpts via
server.FlagStakingCacheSize(not a hardcoded CLI flag)- Value passed directly to
stakingkeeper.NewKeeper()constructor- Integration tests validate different cache sizes (-1, 0, 1, 2, 3) produce expected results
The configuration flows from jsonnet through TOML into the app, ensuring per-validator cache settings take effect.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app.go (1)
654-663: Critical: Use memKeys for CronosKeeper's memory store parameter.Line 657 incorrectly passes
keys[cronostypes.MemStoreKey]as the memory store key parameter. This passes a KVStoreKey when a MemoryStoreKey is expected, causing the memory store to not be properly wired to the CronosKeeper.After adding
cronostypes.MemStoreKeyto thememKeysinitialization (line 254), you must also update this line to usememKeysinstead ofkeys.Apply this diff:
app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + memKeys[cronostypes.MemStoreKey], app.BankKeeper, app.TransferKeeper, app.EvmKeeper, app.AccountKeeper, authAddr, )
♻️ Duplicate comments (2)
app/app.go (2)
254-254: [Duplicate] Add cronostypes.MemStoreKey to memory store keys.This issue was already flagged in previous reviews. The CronosKeeper requires a memory store key (see line 657), but
cronostypes.MemStoreKeyis not included in thememKeysinitialization.
504-515: [Duplicate] FlagStakingCacheSize is undefined.This critical issue was already flagged in previous reviews. The
server.FlagStakingCacheSizeconstant does not exist and will cause a runtime error during app initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/app.go(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
app/app.go (1)
x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
🪛 Ruff (0.14.4)
integration_tests/test_staking_cache.py
39-39: Unused function argument: node_index
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: unittest
- GitHub Check: Analyze (go)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
app/app.go (3)
285-288: LGTM – Memory store keys field added correctly.The
memKeysfield is properly added to the App struct with the correct type to store memory store keys.
434-448: LGTM – Memory store keys properly unpacked and assigned.The code correctly unpacks the fourth return value from
StoreKeys()and assigns it toapp.memKeys.
966-966: LGTM – Memory stores properly mounted.The call to
MountMemoryStores(memKeys)is correctly placed with other store mounting operations and is necessary for the memory stores to function.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
integration_tests/test_staking_cache.py (3)
170-183: Drop the stale post-loop print.This print runs after the loop and only reports the last node’s numbers, which is misleading. Remove it (or move logging inside the loop) to avoid confusion.
465-569: Fail fast when the validator self-delegation lookup fails.If the CLI call returns nothing or omits the validator entry, the test silently skips the unbonding flow and still passes—exactly the regression we need to catch. Assert on the response and fail when the self-delegation isn’t present so the test surfaces real problems.
- if delegation_info and "delegation_responses" in delegation_info: - for del_resp in delegation_info["delegation_responses"]: - if del_resp["delegation"]["validator_address"] == val_addr: - self_delegation = del_resp["delegation"]["shares"] - print(f"Self-delegation shares: {self_delegation}") - ... - break + assert delegation_info and "delegation_responses" in delegation_info, ( + f"Failed to fetch self-delegations for {val_addr} before unbonding." + ) + for del_resp in delegation_info["delegation_responses"]: + if del_resp["delegation"]["validator_address"] == val_addr: + self_delegation = del_resp["delegation"]["shares"] + print(f"Self-delegation shares: {self_delegation}") + ... + break + else: + pytest.fail( + f"Validator self-delegation entry for {val_addr} missing in response." + )
478-486: Assert the self-unbond transaction succeeds.Continuing after a non-zero
codejust prints a warning and masks a failed unbond. Assert on the result so any failure stops the test with useful context.- if rsp["code"] == 0: - print("Unbonding transaction successful") - else: - msg = f"Warning: Unbonding returned code {rsp['code']}: " - msg += rsp.get("raw_log", "") - print(msg) + assert rsp["code"] == 0, ( + f"Validator self-unbond returned code {rsp['code']}: " + f"{rsp.get('raw_log', rsp)}" + ) + print("Unbonding transaction successful")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/configs/staking_cache.jsonnet
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
🪛 Ruff (0.14.4)
integration_tests/test_staking_cache.py
39-39: Unused function argument: node_index
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
integration_tests/test_staking_cache.py (3)
173-177: Remove the misleading post-loop print.This summary reuses
node_idx,cache_size, andtotal_countfrom the last iteration only, so the numbers don’t describe the cluster. It was already called out earlier—please drop it (or compute a real aggregate).- msg = ( - f"Node {node_idx} (cache-size={cache_size}): " - f"{total_count} total unbonding delegations" - ) - print(msg) + # Summary already asserted above; no redundant final print needed.
472-480: Fail the test when the self-unbond transaction fails.We’re still only printing a warning on non-zero codes, so the validator could fail to unbond and the test would keep going. Make it a hard assertion instead.
rsp = cli.unbond_amount(val_addr, unbond_amount, "validator") print(f"Unbonding response: {rsp}") - - if rsp["code"] == 0: - print("Unbonding transaction successful") - else: - msg = f"Warning: Unbonding returned code {rsp['code']}: " - msg += rsp.get("raw_log", "") - print(msg) + assert rsp.get("code") == 0, ( + f"Validator unbonding failed: {rsp.get('raw_log', rsp)}" + ) + print("Unbonding transaction successful")
491-509: Fail fast when any node can’t fetch the validator.If one node returns nothing, we just print “Validator not found,” skip appending a status, and the test can still pass if the remaining nodes agree. Please turn that into a failure so cache inconsistencies don’t slip through.
- if validator and "validator" in validator: - val_info = validator["validator"] - status = val_info.get("status", "unknown") - tokens = val_info.get("tokens", "0") - jailed = val_info.get("jailed", False) - unbonding_statuses.append(status) - print( - f"Node {node_idx} (cache-size={cache_size}): " - f"Status={status}, Tokens={tokens}, Jailed={jailed}" - ) - else: - print( - f"Node {node_idx} (cache-size={cache_size}): " - f"Validator not found" - ) + if not validator or "validator" not in validator: + pytest.fail( + f"Node {node_idx} (cache-size={cache_size}) " + f"failed to return validator {val_addr}" + ) + val_info = validator["validator"] + status = val_info.get("status", "unknown") + tokens = val_info.get("tokens", "0") + jailed = val_info.get("jailed", False) + unbonding_statuses.append(status) + print( + f"Node {node_idx} (cache-size={cache_size}): " + f"Status={status}, Tokens={tokens}, Jailed={jailed}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_staking_cache.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
🪛 GitHub Check: Lint python
integration_tests/test_staking_cache.py
[failure] 38-38:
./integration_tests/test_staking_cache.py:38:1: E302 expected 2 blank lines, found 1
[failure] 38-38:
./integration_tests/test_staking_cache.py:38:1: BLK100 Black would make changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: integration_tests (staking)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: unittest
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration_tests/test_staking_cache.py (1)
174-178: Fix the post-assertion summary log.After the loop,
node_idx,cache_size, andtotal_countstill reference the last iteration, so the printed summary misreports the result. Emit a single consistent count derived from the list you just asserted instead.- msg = ( - f"Node {node_idx} (cache-size={cache_size}): " - f"{total_count} total unbonding delegations" - ) - print(msg) + consistent_count = total_unbonding_counts[0] + print(f"✓ All nodes consistent: {consistent_count} total unbonding delegations")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration_tests/test_staking_cache.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
5-5: Fix hyphenation in compound adjectives.Two compound adjectives need hyphens: "in-memory" and "staking-related".
Apply this diff:
-* [#1907](https://github.com/crypto-org-chain/cronos/pull/1907) perf: Optimize staking endblocker with an in-memory KV store and standardize gas consumption for staking related messages +* [#1907](https://github.com/crypto-org-chain/cronos/pull/1907) perf: Optimize staking endblocker with an in-memory KV store and standardize gas consumption for staking-related messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
CHANGELOG.md (1)
5-5: Staking optimization is confirmed present in this PR.The concern can be resolved:
- Memory store infrastructure is wired to StakingKeeper (app/app.go:508) with proper routing of
stakingtypes.CacheStoreKey- Cache size parameter is implemented (app/app.go:504) and passed during keeper initialization (app/app.go:514)
- Integration tests are included:
integration_tests/test_staking_cache.pywith corresponding configintegration_tests/configs/staking_cache.jsonnet- Git commit
475ba9dspecifically addresses this with message "fix: staking optimization, add integration tests for staking cache"The optimization has not been reverted and is fully functional in the current PR state.
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
da9e628 to
57f2da3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
go.mod (2)
284-284: Personal fork remains in go.mod — production risk persists.The cosmos-sdk replacement still points to
github.com/randy-cro/cosmos-sdk, a personal fork. This was flagged in previous reviews and remains unresolved. For a production-ready PR targeting the main branch (with v1.6.x release imminent per thomas-nguy's comment), all replacements must reference organization-owned repositories with proper tagged releases.
306-306: Personal fork remains in go.mod — production risk persists.The ethermint replacement still points to
github.com/randy-cro/ethermint, a personal fork. This was flagged in previous reviews and remains unresolved. Both the cosmos-sdk and ethermint dependencies should be migrated to organization-owned forks with proper version tags before merging to main.app/app.go (1)
504-515: Critical: FlagStakingCacheSize undefined — runtime error guaranteed.Line 504 references
server.FlagStakingCacheSize, which does not exist in the Cosmos SDK server package and was not found anywhere in the codebase (per previous review searches). This will cause a runtime panic during app initialization.You must either:
- Define this flag constant and register it in the CLI (e.g., in cmd/cronosd/cmd/root.go), or
- Use an alternative configuration mechanism (e.g., app.toml config value)
The integration test config at
integration_tests/configs/staking_cache.jsonnetshows the intended usage viaapp-config.staking.cache-size, suggesting this should be read from app config, not a server flag.Consider reading from app config instead:
-stakingCacheSize := cast.ToInt(appOpts.Get(server.FlagStakingCacheSize)) +stakingCacheSize := cast.ToInt(appOpts.Get("staking.cache-size"))
🧹 Nitpick comments (1)
CHANGELOG.md (1)
5-5: Minor grammar improvement for changelog entry.The changelog entry could be improved for clarity and grammar:
- "staking related messages" should be "staking-related messages" (hyphenated)
- Consider "fix gas consumption" instead of "standardize gas consumption" to be more explicit
Apply this diff:
-* [#1907](https://github.com/crypto-org-chain/cronos/pull/1907) fix: Optimize staking endblocker with an in-memory KV store and standardize gas consumption for staking related messages +* [#1907](https://github.com/crypto-org-chain/cronos/pull/1907) fix: Optimize staking endblocker with an in-memory KV store and fix gas consumption for staking-related messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(2 hunks)gomod2nix.toml(2 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(4 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- integration_tests/cosmoscli.py
- gomod2nix.toml
- integration_tests/test_staking_cache.py
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/cronosd/cmd/versiondb.go (1)
app/app.go (1)
StoreKeys(228-258)
app/app.go (1)
x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
🪛 GitHub Actions: Run Gosec
.github/workflows/test.yml
[error] CodeQL Action version v2 is deprecated. Please update all occurrences of the CodeQL Action in your workflow files to v3. Reference: github/codeql-action/upload-sarif@v3
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
integration_tests/configs/staking_cache.jsonnet (1)
1-246: Excellent test configuration for staking cache validation.This test configuration systematically covers different cache size scenarios:
- Disabled cache (-1)
- Unlimited cache (0)
- Size-limited caches (1, 2, 3)
The consistent validator setup and clear inline comments make this a well-designed integration test. The comprehensive genesis configuration ensures thorough testing of the staking optimization feature.
.github/workflows/test.yml (1)
22-22: LGTM - enables staking cache integration tests.The addition of "staking" to the test matrix properly wires the new integration tests into CI, ensuring the staking optimization is validated in automated testing.
app/app.go (2)
434-448: Memory store integration properly implemented.The memory store wiring is correctly implemented:
- Line 434: StoreKeys() returns memKeys as 4th value
- Line 448: App stores memKeys reference
- Line 966: Memory stores are mounted before initialization
The integration follows the expected pattern for adding memory-backed stores to the Cosmos SDK app.
Also applies to: 966-966
228-233: All StoreKeys() callers properly updated to handle 4 return values.The ripgrep results confirm both callers have been correctly updated:
- app/app.go:434 unpacks all 4 return values
- cmd/cronosd/cmd/versiondb.go:16 captures all 4 with blank identifiers for unused values
The breaking API change is consistently implemented throughout the codebase.
cmd/cronosd/cmd/versiondb.go (1)
16-16: LGTM! Correctly adapts to the new StoreKeys() signature.The change properly unpacks all four return values from
app.StoreKeys(), discarding the newly added memory store keys (fourth return value) which is appropriate sinceChangeSetCmdoperates on persisted database stores, not ephemeral in-memory caches.
randy-cro
force-pushed
the
fix/staking-optimization
branch
2 times, most recently
from
e677e38 to
73f1631
Compare
randy-cro
force-pushed
the
fix/staking-optimization
branch
2 times, most recently
from
e920e8a to
8a5aad4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
go.mod (2)
284-284: Critical: Personal fork used for cosmos-sdk replacement.This issue was already flagged in previous reviews. The personal fork should be replaced with an organization-owned repository before merging to main.
306-306: Critical: Personal fork used for ethermint replacement.This issue was already flagged in previous reviews. The personal fork should be replaced with an organization-owned repository before merging to main.
gomod2nix.toml (2)
216-218: Critical: Module replacement points to personal fork.This issue was already flagged in previous reviews. The cosmos-sdk replacement should point to an organization-owned repository.
315-317: Critical: Module replacement points to personal fork.This issue was already flagged in previous reviews. The ethermint replacement should point to an organization-owned repository.
app/app.go (1)
504-514: FlagStakingCacheSize is not defined — critical runtime error.This issue was already flagged in previous reviews. The flag
server.FlagStakingCacheSizedoes not exist and will cause a runtime error during app initialization.
🧹 Nitpick comments (2)
integration_tests/cosmoscli.py (2)
413-426: Clarify distinction between get_delegations and get_delegated_amount.The new
get_delegationsmethod (lines 413-426) is nearly identical to the existingget_delegated_amountmethod (lines 399-411). Both query the same data butget_delegationsreturns the raw JSON response whileget_delegated_amountreturns it as well. This duplication may cause confusion.Consider either:
- Deprecating
get_delegated_amountin favor ofget_delegations- Renaming one method to clarify their different purposes
- Documenting the difference more clearly
460-477: Consider allowing gas parameter overrides.The delegate_amount method now uses fixed gas values (DEFAULT_GAS_PRICE and STAKING_DEFAULT_GAS), which ensures consistent gas consumption. However, this removes flexibility for callers who may need to customize gas settings for specific scenarios.
Consider adding optional **kwargs parameter to allow gas overrides when needed:
-def delegate_amount(self, to_addr, amount, from_addr): +def delegate_amount(self, to_addr, amount, from_addr, **kwargs): + kwargs.setdefault("gas_prices", DEFAULT_GAS_PRICE) + kwargs.setdefault("gas", STAKING_DEFAULT_GAS) return json.loads( self.raw( "tx", "staking", "delegate", to_addr, amount, "-y", home=self.data_dir, from_=from_addr, keyring_backend="test", chain_id=self.chain_id, node=self.node_rpc, - gas_prices=DEFAULT_GAS_PRICE, - gas=STAKING_DEFAULT_GAS, + **kwargs, ) )Apply similar changes to unbond_amount and redelegate_amount.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(4 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/test.yml
- cmd/cronosd/cmd/versiondb.go
- CHANGELOG.md
- integration_tests/test_staking_cache.py
🧰 Additional context used
🧬 Code graph analysis (2)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
8a5aad4 to
e0ac2e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app.go (1)
228-257: Cronos memory store key is miswired—use memKeys, not keys, for the mem store.The new
StoreKeys/memKeysplumbing andMountMemoryStores(memKeys)are the right shape for the staking and Cronos in‑memory stores, but CronosKeeper is still given a KV key instead of the memory key:
cronostypes.MemStoreKeyis only registered inmemKeys(MemoryStoreKeys), not inkeys(KVStoreKeys).cronoskeeper.NewKeeper’smemKey storetypes.StoreKeyshould therefore receivememKeys[cronostypes.MemStoreKey].As written,
keys[cronostypes.MemStoreKey]will benil, and any attempt to use this mem store through the Cronos keeper risks panics or silently missing state.Suggested fix:
@@ func StoreKeys() ( - memKeys := storetypes.NewMemoryStoreKeys(stakingtypes.CacheStoreKey, cronostypes.MemStoreKey) + memKeys := storetypes.NewMemoryStoreKeys( + stakingtypes.CacheStoreKey, + cronostypes.MemStoreKey, + ) @@ func New( - keys, tkeys, okeys, memKeys := StoreKeys() + keys, tkeys, okeys, memKeys := StoreKeys() @@ - app.CronosKeeper = *cronoskeeper.NewKeeper( - appCodec, - keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + app.CronosKeeper = *cronoskeeper.NewKeeper( + appCodec, + keys[cronostypes.StoreKey], + memKeys[cronostypes.MemStoreKey], @@ - app.MountMemoryStores(memKeys) + app.MountMemoryStores(memKeys)Also applies to: 285-289, 654-663, 962-967
♻️ Duplicate comments (4)
integration_tests/test_staking_cache.py (3)
170-178: Drop the trailing print that reuses loop-scoped vars.The final
msg/node_idx,cache_size, andtotal_countfrom the last loop iteration only and adds no information beyond the per-node prints above. It’s slightly misleading; consider removing this block.
462-476: Avoid hard-coding validator self-unbond amounts; derive from the actual delegation balance.Both validator-unbonding flows assume the self-delegation is exactly
"1000000000000000000stake". If the staking genesis or params change (different amount/denom, inflation, etc.), these tests may fail even when the cache logic is correct.You already fetch the delegation responses; instead of a literal string, build the unbond amount from the
balanceobject:- # Unbond all self-delegation - unbond_amount = "1000000000000000000stake" + balance = del_resp.get("balance") or {} + assert {"amount", "denom"} <= balance.keys(), ( + f"Missing balance info for validator {val_addr}: {del_resp}" + ) + # Unbond the full self-delegation so the validator actually leaves the set + unbond_amount = f"{balance['amount']}{balance['denom']}"And similarly for the later validator-unbonding in
test_staking_cache_consistency, deriveunbond_val_amountfrom the validator’s self-delegation balance instead of hard-coding the string.Also applies to: 763-768
365-388: Narrow the broadexcept Exceptionaround matured redelegation queries.The try/except blocks interpreting
"not found"as “no redelegations remaining” are logically fine, but catching bareExceptionis still broad and likely to keep Ruff’s BLE001 unhappy. It also risks masking unexpected failure modes.Consider:
- Catching the specific CLI/
subprocess.CalledProcessError(or whichever exceptionnode_cli.rawraises), and- Treating only the well-known “not found” case as zero while re-raising everything else (which you already do), or
- Letting errors propagate and asserting instead on explicit empty responses if your CLI returns
redelegation_responses: []on success.This keeps the test behavior the same while tightening the error handling.
Also applies to: 809-822
gomod2nix.toml (1)
216-218: Avoid personal GitHub forks for core consensus deps (cosmos‑sdk, ethermint).
github.com/cosmos/cosmos-sdkandgithub.com/evmos/ethermintare now replaced withgithub.com/randy-cro/...personal forks in gomod2nix. For a production chain this is a governance and supply‑chain risk; replacements should point to org‑owned repos (e.g.crypto-org-chain/*) or upstream tags that are reproducible and reviewable, and must stay consistent with go.mod.#!/bin/bash # Check all references to personal forks so they can be moved to org-owned repos. rg -n "randy-cro/(cosmos-sdk|ethermint)" go.mod gomod2nix.toml -S || echo "No personal forks found"Also applies to: 315-317
🧹 Nitpick comments (2)
CHANGELOG.md (1)
5-5: UNRELEASED entry now matches the implemented staking optimization and gas changes.The #1907 line accurately reflects the in‑memory staking endblocker and fixed‑gas staking message work in this PR. If you care about copy‑editing, consider “staking‑related messages”, but functionally this entry looks good.
integration_tests/cosmoscli.py (1)
22-22: Fixed‑gas staking helpers verified; consolidate duplicate getter methods.All call sites in
integration_tests/test_staking_cache.pycorrectly use the new signatures—no tests pass extra parameters todelegate_amount,unbond_amount, orredelegate_amount. The API changes are safe.However,
get_delegated_amountandget_delegationsare exact duplicates (both query the same endpoint identically); keep one and deprecate or remove the other. Consider renaming the remaining method for clarity if usage differs by context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(4 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cronosd/cmd/versiondb.go
- go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)get_delegations(413-426)validator(339-349)status(175-176)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: integration_tests (ibc)
- GitHub Check: Run golangci-lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (slow)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
integration_tests/configs/staking_cache.jsonnet (1)
1-245: Staking cache test config matches the test scenarios.The jsonnet config cleanly sets up five validators with
staking.cache-size= -1/0/1/2/3 and a 20‑second unbonding window, which aligns with the new staking cache tests’ expectations. No functional issues spotted here.app/app.go (1)
504-515: To verify the claims aboutserver.FlagStakingCacheSizeandstakingkeeper.NewKeeper, I need additional context:
- Repository URL (or owner/repo on GitHub)
- Commit hash or branch name being reviewed
- Alternatively, the go.mod file from your repository (to identify the cosmos-sdk version/commit you're pinning)
Once you provide these details, I can inspect the cosmos-sdk version in use and verify whether:
server.FlagStakingCacheSizeis definedstakingkeeper.NewKeeperaccepts the memory store service and cache size parameters as shownCould you share the repository URL, commit, and/or go.mod details?
.github/workflows/test.yml (1)
22-22: Verification passed—staking test label is properly wired throughout the CI pipeline.The matrix value flows correctly:
matrix.tests: staking→TESTS_TO_RUN=staking→./scripts/run-integration-tests→pytest -m 'staking'→test_staking_cache.pywithpytestmark = pytest.mark.staking. All integration points are in place.
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
e0ac2e2 to
55eef37
Compare
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
55eef37 to
772c444
Compare
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
772c444 to
c6748a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
app/app.go (1)
654-663: Wire CronosKeeper with the memory store key instead of a non‑existent KV key.Here you pass:
app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, keys[cronostypes.StoreKey], keys[cronostypes.MemStoreKey], // <- nil, no KVStoreKey registered under this name ... )
cronostypes.MemStoreKeyis only created inmemKeys := storetypes.NewMemoryStoreKeys(stakingtypes.CacheStoreKey, cronostypes.MemStoreKey)and mounted viaMountMemoryStores(memKeys), sokeys[cronostypes.MemStoreKey]is nil. Any use of the keeper’smemKeywill then panic at runtime.Use the memory‑store map instead:
app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + memKeys[cronostypes.MemStoreKey], app.BankKeeper, app.TransferKeeper, app.EvmKeeper, app.AccountKeeper, authAddr, )integration_tests/test_staking_cache.py (4)
44-223: Multiple-unbonding test logic is sound; consider trimming the final summary print.The scenario (multi‑validator delegations from several accounts, cross‑node unbonding checks, then maturation checks after
wait_for_new_blocks(cli, 60)) exercises the staking cache well and uses the new CLI helpers correctly.The only minor nit is the final summary block at Lines 174‑178, which reuses
node_idx/cache_size/total_countfrom the last loop iteration. It’s harmless but slightly misleading (“Node 4…” even though you’ve just asserted all nodes are consistent). You can either drop that print or replace it with an aggregate message that doesn’t depend on loop‑scoped variables.
300-407: Narrow redelegation maturity error handling instead of catching bareException.In the redelegation tests you have blocks like:
try: redelegations = node_cli.get_redelegations(...) count = len(redelegations) if redelegations else 0 ... except Exception as e: # treat "not found" as zero, re-raise othersThis keeps tests passing on “not found” CLI errors but still swallows all exception types up front, which is brittle and keeps Ruff’s BLE001 warning alive.
Consider one of:
- Catch the specific subprocess error type raised by
self.raw(e.g.subprocess.CalledProcessError) and inspect its output for"not found", or- Adjust
CosmosCLI.get_redelegationsto normalize the “not found” case into an empty list, so the test code no longer needs any try/except block at all.Either approach keeps the intended behavior while avoiding a broad
except Exception.Also applies to: 811-840
411-560: Validator unbonding tests are thorough; hard-coded unbond amounts are acceptable but slightly brittle.Both validator-unbonding flows:
- Query the validator’s actual
tokens.- Set
min_self_delegationto that value viaedit_validator.- Unbond a fixed
"1000000000000000000stake"to trigger the min-self-delegation check, then assert the validator transitions toBOND_STATUS_UNBONDINGand that validator counts drop by 1 after the unbonding period.Given
actual_tokensis read from chain state, this pattern is robust as long as the validator’s self-delegation is ≥ 1 stake, which is true for the current test config. If you expect future genesis tweaks or denoms to change magnitudes, you might consider deriving the unbond amount fromactual_tokens(e.g., a small fraction or fullbalance['amount']) to make the tests future-proof, but the current approach is logically consistent.Also applies to: 735-777
563-881: Comprehensive consistency test ties all staking flows together cleanly.
test_staking_cache_consistencycombines delegations, redelegations, unbonding delegations, and validator unbonding with cross-node checks before and after maturation. The structure (phased prints + per-node assertions) gives good coverage of the cache behavior across cache-size configurations.The main remaining nit is the same redelegation “maturity” handling at Lines 817‑830, which mirrors the broad
except Exceptionblock already discussed—applying the same refinement there (or centralizing the normalization intoCosmosCLI.get_redelegations) will keep error handling consistent and lint‑friendly.go.mod (1)
24-24: Replace personal forks with org‑owned or upstream modules before merging.
go.modstill routes core dependencies through personal forks:
github.com/cosmos/cosmos-sdk => github.com/randy-cro/cosmos-sdk ...github.com/evmos/ethermint => github.com/randy-cro/ethermint ...For a chain mainline branch this is a significant operational and trust risk. Before release, push these changes into an organization‑owned fork (e.g.
github.com/crypto-org-chain/cosmos-sdk/.../ethermint) or upstream, tag proper versions, and updatereplaceto point at those repos/tags (keeping gomod2nix.toml in sync).Also applies to: 80-81, 272-272, 284-285, 306-306
gomod2nix.toml (1)
139-145: Keep gomod2nix in sync, but avoid locking to personal forks.The updated hashes for
btcd,btcec,gogoproto, andgoogle.golang.org/genproto/googleapis/rpccorrectly track the go.mod changes. However, the lock still pegs:
github.com/cosmos/cosmos-sdk→replaced = "github.com/randy-cro/cosmos-sdk"github.com/evmos/ethermint→replaced = "github.com/randy-cro/ethermint"As with go.mod, these personal forks are not appropriate long‑term for a mainline chain. Once the SDK and Ethermint changes are moved to an org‑owned repo or upstream tag, regenerate this file (e.g., via gomod2nix) so the
versionandhashfields point at the official modules instead.Also applies to: 215-218, 225-227, 315-318, 780-784
🧹 Nitpick comments (2)
CHANGELOG.md (1)
3-8: UNRELEASED staking entry matches the PR; minor wording nit only.The UNRELEASED line now accurately reflects the staking endblocker mem‑KV optimization and fixed‑gas staking messages introduced in this PR. Consider tightening the wording to “staking‑related messages” for style consistency.
integration_tests/cosmoscli.py (1)
399-459: New staking query helpers look good; consider reusing them to avoid duplication.
get_delegations,get_unbonding_delegations, andget_redelegationsare straightforward wrappers over the staking query CLI and are used cleanly by the new staking cache tests. You now have bothget_delegated_amountandget_delegationscalling the same endpoint—if you want to reduce duplication,get_delegated_amountcould simply delegate toget_delegationsinternally, but this is optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cronosd/cmd/versiondb.go
- integration_tests/configs/staking_cache.jsonnet
🧰 Additional context used
🧬 Code graph analysis (3)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Analyze (go)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (4)
app/app.go (1)
228-258: Staking cache/memory‑store wiring looks consistent with the new SDK API.StoreKeys now returns
memKeys(includingstakingtypes.CacheStoreKey), App tracks them,stakingkeeper.NewKeeperis wired withruntime.NewMemStoreService(memKeys[stakingtypes.CacheStoreKey])and a configurablestakingCacheSize, and the memory stores are mounted viaMountMemoryStores(memKeys). This matches the in‑memory staking cache design and should give you node‑configurable cache behavior.Also applies to: 285-289, 434-449, 504-515, 962-967
.github/workflows/test.yml (1)
20-24: Matrix update correctly adds the staking test shard.Adding
stakingto thetestsmatrix and wiring it intoTESTS_TO_RUNwill exercise the new staking cache integration tests as part of CI. Just ensuremake run-integration-testsunderstands this value (e.g., via a pytest marker or test selector), which appears consistent with how the other entries are used.integration_tests/test_staking_cache.py (1)
26-42: Fixture and helper wiring for staking cache cluster look good.
cronos_staking_cachecorrectly provisions a multi‑node Cronos cluster fromstaking_cache.jsonnetviasetup_custom_cronos, andget_delegator_addressis a thin, readable wrapper overcli.address. No issues here.integration_tests/cosmoscli.py (1)
19-23: Fixed-gas staking tx wiring aligns with the PR’s goals.Introducing
STAKING_DEFAULT_GASand using it (withDEFAULT_GAS_PRICE) fordelegate_amount,unbond_amount,redelegate_amount, andedit_validatormakes staking-related messages consume a predictable gas limit from the client side, which matches the “fixed gas consumption for staking messages” objective. The call shapes remain backward-compatible for existing tests that only assert onrsp["code"].Also applies to: 460-519, 724-758
randy-cro
force-pushed
the
fix/staking-optimization
branch
4 times, most recently
from
8fd8052 to
d394f03
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
go.mod (1)
280-307: Personal forks still used for core dependencies (cosmos‑sdk, ethermint)
replacestill pointsgithub.com/cosmos/cosmos-sdkandgithub.com/evmos/ethermintat personal forks undergithub.com/randy-cro/*. For a main-branch node implementation this is a serious production risk (trust, availability, reviewability, and long‑term maintenance).Before merging to
main, these should instead point to:
- Official upstreams, or
- Organization-owned forks under
github.com/crypto-org-chain/*with proper, documented semver tags,and both
go.modandgomod2nix.tomlshould be updated to those repos/tags.You can sanity-check for lingering personal forks with:
#!/bin/bash rg -n "github.com/randy-cro" go.mod gomod2nix.tomlintegration_tests/test_staking_cache.py (1)
143-183: Tidy redundant summary print and align unbonding comment with wait durationIn
test_staking_cache_multiple_unbonding:
- The final
msg/node_idx,cache_size, andtotal_countfrom the last iteration only. It adds no new information beyond what’s already logged per node and after the assertion, and is potentially misleading. Consider dropping it.- The comment says “Wait for unbonding period to complete (20 seconds, 20 blocks)” but you actually wait for 60 blocks. It would be clearer to either update the comment to 60 blocks or adjust the wait if 20 is sufficient for this config.
These are cosmetic, but cleaning them up will make the test output easier to reason about.
app/app.go (1)
654-663: CronosKeeper is given a nil mem store key; usememKeysinstead ofkeysHere:
app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, keys[cronostypes.StoreKey], keys[cronostypes.MemStoreKey], ... )
cronostypes.MemStoreKeyis only registered inmemKeysvia:memKeys := storetypes.NewMemoryStoreKeys(stakingtypes.CacheStoreKey, cronostypes.MemStoreKey)and is not part of the
storeKeysslice used to buildkeys. As a result,keys[cronostypes.MemStoreKey]isnil, and any attempt byCronosKeeperto open its mem store will panic at runtime.This should instead use the memory store key:
- app.CronosKeeper = *cronoskeeper.NewKeeper( - appCodec, - keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + app.CronosKeeper = *cronoskeeper.NewKeeper( + appCodec, + keys[cronostypes.StoreKey], + memKeys[cronostypes.MemStoreKey], app.BankKeeper, app.TransferKeeper, app.EvmKeeper, app.AccountKeeper, authAddr, )so the keeper receives the actual
MemoryStoreKeythat you mount withMountMemoryStores(memKeys).gomod2nix.toml (1)
139-145: gomod2nix locks in personal forks for cosmos‑sdk and ethermintThe gomod2nix entries for:
github.com/cosmos/cosmos-sdk(lines 215‑218), andgithub.com/evmos/ethermint(lines 315‑317)now both reference versions from
github.com/randy-cro/*and setreplacedto those personal forks. This mirrors the critical concern in go.mod and effectively hard-wires those forks into your Nix builds as well.Once you move go.mod back to an org-owned fork or upstream tag, regenerate gomod2nix so that:
versionmatches the chosen tag, andreplaced(if needed) points to an organization-controlled repo, not a personal account.To confirm and later validate the cleanup:
#!/bin/bash rg -n 'github.com/randy-cro' gomod2nix.tomlAlso applies to: 215-218, 315-317, 783-784
🧹 Nitpick comments (3)
CHANGELOG.md (1)
3-31: Changelog entry now matches PR; only minor wording/date nitsThe UNRELEASED entry for [#1907] correctly describes this PR (staking endblocker optimization + standardized gas for staking messages). Two small, non-blocking nits you may want to adjust:
- Line 5: consider “staking‑related messages” (hyphenated) for readability.
- Lines 7/18/31: dates like
*Nov 30, 2025*and*Oct 30, 2025*are in the future relative to today; if these represent planned releases, that’s fine, but if they slip it may be clearer to keep them marked as “Unreleased” until the actual ship date is known.integration_tests/cosmoscli.py (1)
20-23: Staking helpers and fixed‑gas wiring look consistentThe new staking helpers and gas configuration are coherent:
STAKING_DEFAULT_GASis reused acrossdelegate_amount,unbond_amount,redelegate_amount, andedit_validator, keeping staking txs on a fixed gas schedule for tests.get_delegations,get_unbonding_delegations, andget_redelegationsmirror the CLI APIs correctly and always query the right node/chain.edit_validator’s newmin_self_delegationparameter integrates cleanly into the existing options filtering.If staking gas requirements drift over time, consider funneling these staking defaults through a single test‑config helper (or deriving from chain params) so you don’t have to touch multiple call sites when tuning gas, but functionally this looks solid.
Also applies to: 413-476, 479-519, 724-758
integration_tests/test_staking_cache.py (1)
535-556: Avoid hard‑coding initial validator count intest_staking_cache_unbonding_validatorThe test currently assumes
initial_validator_count = 5and asserts that the final count is5 - 1. That’s true for the current staking_cache.jsonnet, but will break as soon as the template changes validator count.Safer pattern:
initial_validator_count = len(cronos.cosmos_cli().validators()) ... expected_count = initial_validator_count - 1 assert validator_counts[0] == expected_count, ...so the test remains valid if you ever change the number of validators in this scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_tests/configs/staking_cache.jsonnet
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/cronosd/cmd/versiondb.go (1)
app/app.go (1)
StoreKeys(228-258)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
.github/workflows/test.yml (1)
22-22: Add ofstakingtest shard looks correctIncluding
stakingin the integration test matrix is consistent with the new staking cache tests and keeps CI coverage aligned. No further changes needed here.cmd/cronosd/cmd/versiondb.go (1)
15-29: Handling of newStoreKeysreturn value is correctUpdating to unpack four values from
app.StoreKeys()and discardingmemKeyshere keeps versiondb behavior unchanged while matching the new API.app/app.go (1)
228-258: Staking mem‑store integration and mounting look correctThe in‑memory store plumbing is consistent:
StoreKeys()now returnsmemKeysand initializes memory stores forstakingtypes.CacheStoreKeyandcronostypes.MemStoreKey.AppstoresmemKeys,New()captures them, andMountMemoryStores(memKeys)ensures they’re part of the multistore.StakingKeeper.NewKeeperis given aMemStoreServicebuilt frommemKeys[stakingtypes.CacheStoreKey]plus a configurablestakingCacheSize(fromserver.FlagStakingCacheSize), matching the cache‑backed staking design.This wiring matches the intended in‑memory staking cache behavior and should avoid the heavy I/O in the staking endblocker that this PR is targeting.
Also applies to: 284-289, 434-449, 504-515, 962-967
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
d394f03 to
dad9f9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
go.mod (1)
284-284: Critical: Personal forks remain in module replacements.Both cosmos-sdk (line 284) and ethermint (line 306) still point to personal github.com/randy-cro repositories. As extensively detailed in previous reviews, production releases targeting the main branch must use organization-owned forks with proper tagged releases.
Also applies to: 306-306
CHANGELOG.md (1)
7-31: Fix chronologically impossible release dates.The timeline is broken:
- v1.5.4: "Nov 30, 2025" (9 days in the future)
- v1.5.3: "Oct 30, 2025" (comes AFTER v1.5.4 but dated earlier??)
- v1.5.2: "Oct 15, 2025" (also chronologically after v1.5.4)
These are almost certainly typos—2025 should be 2024 for the October dates. Please correct all three dates to their actual release dates or mark unreleased versions as "Unreleased" with no date.
gomod2nix.toml (1)
218-218: Critical: Personal fork replacements in gomod2nix.toml.The module replacements at lines 218 (cosmos-sdk) and 317 (ethermint) point to github.com/randy-cro personal repositories. This mirrors the go.mod concern and must be resolved before merging to main.
Also applies to: 317-317
🧹 Nitpick comments (1)
integration_tests/test_staking_cache.py (1)
174-178: Remove redundant print using stale loop variables.This print statement uses
node_idx,cache_size, andtotal_countfrom the last loop iteration (node=4), which is misleading. Line 168 already prints per-node counts inside the loop, and line 171 asserts consistency. This trailing print serves no purpose and confuses readers.Apply this diff:
assert ( len(set(total_unbonding_counts)) == 1 ), f"Nodes have different unbonding delegation counts: {total_unbonding_counts}" -msg = ( - f"Node {node_idx} (cache-size={cache_size}): " - f"{total_count} total unbonding delegations" -) -print(msg) +print(f"✓ All nodes consistent: {total_unbonding_counts[0]} total unbonding delegations")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/test.yml
- cmd/cronosd/cmd/versiondb.go
- integration_tests/cosmoscli.py
- app/app.go
- integration_tests/configs/staking_cache.jsonnet
🧰 Additional context used
🧬 Code graph analysis (1)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Analyze (go)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
dad9f9d to
8063639
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app.go (1)
228-258: Cronos mem-store key is nil – usememKeys[cronostypes.MemStoreKey]instead ofkeys[...]
StoreKeysonly registerscronostypes.MemStoreKeyas a memory store:memKeys := storetypes.NewMemoryStoreKeys(stakingtypes.CacheStoreKey, cronostypes.MemStoreKey) ... return keys, tkeys, okeys, memKeysbut
CronosKeeperis constructed with:app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, keys[cronostypes.StoreKey], keys[cronostypes.MemStoreKey], // this map entry is never created ... )Since
cronostypes.MemStoreKeyis not in thestoreKeysslice used forNewKVStoreKeys,keys[cronostypes.MemStoreKey]is nil. That means the Cronos keeper’smemKeywill be nil even though you mount a proper memory store inMountMemoryStores(memKeys), leading to a panic or effectively disabling the intended in-memory Cronos store.You likely want to pass the memory key here:
app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, - keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + keys[cronostypes.StoreKey], + memKeys[cronostypes.MemStoreKey], app.BankKeeper, app.TransferKeeper, app.EvmKeeper, app.AccountKeeper, authAddr, )This will align the keeper with the mounted memory store and the design of
StoreKeys.Also applies to: 285-289, 434-449, 654-663, 962-967
♻️ Duplicate comments (4)
go.mod (1)
24-47: Critical: core deps still point to personal forks (randy-cro/cosmos-sdk,randy-cro/ethermint)The
replacedirectives forgithub.com/cosmos/cosmos-sdkandgithub.com/evmos/ethermintnow target personal forks undergithub.com/randy-cro/*. For a PR targetingmainon a production chain, this is not acceptable:
- Security & trust: consensus code comes from a personal account outside the org’s review/audit process.
- Availability: personal repos can disappear or be force-pushed without governance.
- Operations: validators and integrators expect dependencies to come from upstream or org-controlled forks with tagged releases.
Before merging to
main, these should be moved to org-owned modules (e.g.github.com/crypto-org-chain/cosmos-sdkandgithub.com/crypto-org-chain/ethermint) or to official upstream tags, and the fork-specific changes merged there.Example (pseudo) diff for illustration:
-replace github.com/cosmos/cosmos-sdk => github.com/randy-cro/cosmos-sdk v0.0.0-20251121082545-b3fa9253c274 +replace github.com/cosmos/cosmos-sdk => github.com/crypto-org-chain/cosmos-sdk vX.Y.Z-cronos -replace github.com/evmos/ethermint => github.com/randy-cro/ethermint v0.0.0-20251121082919-46c057ac4dde +replace github.com/evmos/ethermint => github.com/crypto-org-chain/ethermint vA.B.C-cronosMake sure gomod2nix and any other lockfiles are updated to match once you flip these.
#!/bin/bash # Sanity-check remaining personal forks in module metadata rg -n "randy-cro/(cosmos-sdk|ethermint)" go.mod gomod2nix.toml || echo "No personal forks found"Also applies to: 80-81, 272-273, 280-285, 293-307
CHANGELOG.md (1)
7-32: Check v1.5.4 release date before merging
*Nov 30, 2025*is currently a future date. If this PR lands before that, consider either:
- updating the date when v1.5.4 actually ships, or
- marking it as “Unreleased” until the tag is cut,
to avoid confusion in downstream tooling and ops docs. This was raised previously and still appears unresolved.
gomod2nix.toml (1)
139-145: gomod2nix still pins cosmos-sdk & ethermint to personal forks
gomod2nix.tomlmirrors go.mod by mapping:
github.com/cosmos/cosmos-sdk→github.com/randy-cro/cosmos-sdkgithub.com/evmos/ethermint→github.com/randy-cro/ethermintwith corresponding pseudo-versions and hashes. For the same reasons as in go.mod (security, governance, availability), these should point to org-owned repos or upstream tags before merging to
main, and the hashes must be regenerated accordingly.Once you update go.mod to use the final org-owned modules, please rerun your gomod2nix generation step and refresh the
versionandhashfields here to keep Nix builds reproducible.#!/bin/bash # Verify no personal forks remain in Nix module metadata rg -n 'randy-cro/(cosmos-sdk|ethermint)' gomod2nix.toml || echo "No personal forks referenced in gomod2nix.toml"Also applies to: 215-218, 225-227, 315-317, 783-784
integration_tests/test_staking_cache.py (1)
176-180: Remove redundant print statement with stale loop variables.This print statement uses
node_idx,cache_size, andtotal_countfrom the last loop iteration only, making it misleading. The assertion above already confirms consistency, so this print is redundant.Apply this diff:
- msg = ( - f"Node {node_idx} (cache-size={cache_size}): " - f"{total_count} total unbonding delegations" - ) - print(msg) + print( + f"✓ All nodes consistent: {total_unbonding_counts[0]} " + f"total unbonding delegations" + )
🧹 Nitpick comments (3)
CHANGELOG.md (1)
5-6: Minor wording nit in UNRELEASED entryThe description matches the PR scope well. If you touch this again, consider hyphenating “staking related messages” → “staking-related messages” for readability.
app/app.go (1)
285-289: Consider including memory store keys inGetStoreKeysfor consistency
Appnow tracksmemKeys, and you mount them viaMountMemoryStores(memKeys), butGetStoreKeys()still only returns KV, transient, and object keys. For callers like the block‑STM executor that take a list ofstoretypes.StoreKeys, it may be more accurate to include the memory stores as well.A minimal refactor:
func (app *App) GetStoreKeys() []storetypes.StoreKey { - keys := make([]storetypes.StoreKey, 0, len(app.keys)+len(app.tkeys)+len(app.okeys)) + keys := make([]storetypes.StoreKey, 0, len(app.keys)+len(app.tkeys)+len(app.okeys)+len(app.memKeys)) for _, key := range app.keys { keys = append(keys, key) } for _, key := range app.tkeys { keys = append(keys, key) } for _, key := range app.okeys { keys = append(keys, key) } + for _, key := range app.memKeys { + keys = append(keys, key) + } sort.SliceStable(keys, func(i, j int) bool { return keys[i].Name() < keys[j].Name() }) return keys }Not strictly required for correctness, but it keeps
GetStoreKeysbehavior aligned with the newmemKeysfield and may help tools that expect a complete store list.Also applies to: 1222-1235, 1038-1043
integration_tests/cosmoscli.py (1)
399-427: Alignget_delegationsreturn shape with other staking query helpers
get_unbonding_delegationsandget_redelegationsunwrap their JSON to the*_responsesarrays, butget_delegationscurrently returns the full response dict. For consistency and simpler test code, you might want:- def get_delegations(self, which_addr): - """Query all delegations made from one delegator.""" - return json.loads( - self.raw( - "query", - "staking", - "delegations", - which_addr, - home=self.data_dir, - chain_id=self.chain_id, - node=self.node_rpc, - output="json", - ) - ) + def get_delegations(self, which_addr): + """Query all delegations made from one delegator.""" + return json.loads( + self.raw( + "query", + "staking", + "delegations", + which_addr, + home=self.data_dir, + chain_id=self.chain_id, + node=self.node_rpc, + output="json", + ) + ).get("delegation_responses", [])Not mandatory, but it would make the API more uniform.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/cronosd/cmd/versiondb.go (1)
app/app.go (1)
StoreKeys(228-258)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
🪛 Flake8 (7.3.0)
integration_tests/test_staking_cache.py
[error] 574-574: f-string is missing placeholders
(F541)
[error] 807-807: f-string is missing placeholders
(F541)
[error] 892-892: f-string is missing placeholders
(F541)
🪛 GitHub Check: Lint python
integration_tests/test_staking_cache.py
[failure] 892-892:
./integration_tests/test_staking_cache.py:892:15: F541 f-string is missing placeholders
[failure] 841-841:
./integration_tests/test_staking_cache.py:841:89: E501 line too long (103 > 88 characters)
[failure] 816-816:
./integration_tests/test_staking_cache.py:816:89: E501 line too long (91 > 88 characters)
[failure] 807-807:
./integration_tests/test_staking_cache.py:807:11: F541 f-string is missing placeholders
[failure] 803-803:
./integration_tests/test_staking_cache.py:803:89: E501 line too long (100 > 88 characters)
[failure] 574-574:
./integration_tests/test_staking_cache.py:574:15: F541 f-string is missing placeholders
[failure] 520-520:
./integration_tests/test_staking_cache.py:520:89: E501 line too long (100 > 88 characters)
[failure] 486-486:
./integration_tests/test_staking_cache.py:486:89: E501 line too long (91 > 88 characters)
[failure] 471-471:
./integration_tests/test_staking_cache.py:471:89: E501 line too long (90 > 88 characters)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.5)
integration_tests/test_staking_cache.py
574-574: f-string without any placeholders
Remove extraneous f prefix
(F541)
807-807: f-string without any placeholders
Remove extraneous f prefix
(F541)
892-892: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
cmd/cronosd/cmd/versiondb.go (1)
15-29: Adaptation to newStoreKeyssignature looks correctConsuming the additional return value and continuing to derive
storeNamesfrom the KV keys keeps the existing behavior intact; ignoring transient/object/memory keys here is appropriate for versiondb usage..github/workflows/test.yml (1)
20-55: Newstakingmatrix entry – ensure the integration harness supports itAdding
stakingto thetestsmatrix looks fine, but please confirmmake run-integration-testsrecognizesTESTS_TO_RUN=stakingand actually runs the new staking cache tests rather than silently no-oping or failing.integration_tests/configs/staking_cache.jsonnet (1)
1-270: Staking cache test config looks well‑structuredThe
cronos_777-1Jsonnet config sensibly spans disabled/unlimited and several boundedstaking.cache-sizevalues across validators, with short timeouts and generous balances suitable for stress tests. This should give good coverage for the new staking cache behavior.integration_tests/cosmoscli.py (1)
19-23: Fixed-gas staking tx helpers align with standardized gas consumptionUsing a dedicated
STAKING_DEFAULT_GASand always passinggas_prices=DEFAULT_GAS_PRICEfordelegate_amount,unbond_amount,redelegate_amount, andedit_validatorgives deterministic gas settings for staking-related messages, which matches the PR’s intent and keeps the integration tests independent of auto gas estimation.Also applies to: 460-477, 480-519, 724-758
integration_tests/test_staking_cache.py (4)
1-26: LGTM!The module documentation clearly describes the test scenarios and cache configurations. The imports and pytest marker are appropriate.
28-38: LGTM!The fixture is properly scoped and configured. The use of
tmp_path_factoryandsetup_custom_cronosfollows established patterns from the codebase.
41-43: LGTM!Simple, focused helper function with clear purpose.
229-410: LGTM!The test properly covers multiple redelegations with consistency checks across all cache configurations. The exception handling for matured redelegations (lines 367-390) is appropriate since "not found" errors are expected when redelegations complete.
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
8063639 to
c71cb89
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/app.go (1)
228-258: Fix CronosKeeper mem store key wiring; staking memstore wiring LGTM.The new store-key plumbing is mostly solid:
StoreKeys()now returnsmemKeysbuilt viastoretypes.NewMemoryStoreKeys(stakingtypes.CacheStoreKey, cronostypes.MemStoreKey).AppholdsmemKeysandNewcallsapp.MountMemoryStores(memKeys).StakingKeeperis correctly constructed withruntime.NewMemStoreService(memKeys[stakingtypes.CacheStoreKey])and a configurablestakingCacheSize.However, the
CronosKeepermem store is wired incorrectly:app.CronosKeeper = *cronoskeeper.NewKeeper( appCodec, keys[cronostypes.StoreKey], keys[cronostypes.MemStoreKey], // <- problem ... )
keysis amap[string]*storetypes.KVStoreKeybuilt fromstoreKeys, which only includescronostypes.StoreKey—notcronostypes.MemStoreKey. That meanskeys[cronostypes.MemStoreKey]isnil, and any use of this memKey inside the keeper will panic when used with the multistore.You already have the correct
MemoryStoreKeyinmemKeys, so this should pass that instead:- app.CronosKeeper = *cronoskeeper.NewKeeper( - appCodec, - keys[cronostypes.StoreKey], - keys[cronostypes.MemStoreKey], + app.CronosKeeper = *cronoskeeper.NewKeeper( + appCodec, + keys[cronostypes.StoreKey], + memKeys[cronostypes.MemStoreKey], app.BankKeeper, app.TransferKeeper, app.EvmKeeper, app.AccountKeeper, authAddr, )With this change, the Cronos in-memory store will be properly created, mounted, and used without nil-key panics.
Also applies to: 284-289, 434-449, 504-515, 654-663, 963-967
♻️ Duplicate comments (3)
integration_tests/test_staking_cache.py (1)
145-181: Remove misleading summary print after the unbonding-count loop (and optionally assert a non-zero count).After you build
total_unbonding_countsper node, the finalmsg/msg = ( f"Node {node_idx} (cache-size={cache_size}): " f"{total_count} total unbonding delegations" ) print(msg)logs only the last loop iteration’s
node_idx/cache_size/total_count, which is redundant and slightly misleading given you already logged each node inside the loop and asserted consistency across nodes.Consider dropping this trailing print, and (if you want a stronger signal that the operations actually happened) add an assertion that the agreed count is > 0, e.g.:
assert total_unbonding_counts[0] > 0, "Expected some unbonding entries but found none"This keeps the focus on cross-node consistency while also ensuring the unbonding operations weren’t silently no-ops.
go.mod (1)
24-24: Replace personal forks with org-owned modules before merging; validate new dependency bumps.The version bumps for
github.com/cosmos/gogoproto,github.com/btcsuite/btcd,github.com/btcsuite/btcd/btcec/v2, andgoogle.golang.org/genproto/googleapis/rpclook reasonable but need to be exercised via full build/test to catch any behavioral changes.The critical issue is the
replaceentries:
github.com/cosmos/cosmos-sdk => github.com/randy-cro/cosmos-sdk v0.0.0-20251121082545-b3fa9253c274github.com/evmos/ethermint => github.com/randy-cro/ethermint v0.0.0-20251121082919-46c057ac4ddeUsing a personal fork on the
mainbranch for core consensus/state libraries is not acceptable for production (trust, maintenance, and governance risks). These changes should be moved into:
- An organization-owned fork under
github.com/crypto-org-chain/*(or upstream if appropriate),- With a proper semver tag,
- Then updated here to point to that org repo/tag (and synced with
gomod2nix.toml).Until that’s done, this PR should not be merged into
main.Also applies to: 80-81, 272-272, 284-284, 306-306
gomod2nix.toml (1)
140-144: Keep gomod2nix in sync with go.mod, but avoid personal forks for core modules.The version/hash updates for
btcd,btcec/v2,gogoproto, andgoogle.golang.org/genproto/googleapis/rpcare consistent with the go.mod changes and should be fine as long as CI passes with the new transitive graph.The problematic part is:
github.com/cosmos/cosmos-sdkwithreplaced = "github.com/randy-cro/cosmos-sdk"github.com/evmos/ethermintwithreplaced = "github.com/randy-cro/ethermint"These mirror the personal-fork replacements in go.mod and carry the same production risks. Once you move those changes into org-owned forks/tags and update go.mod accordingly, this file must be regenerated/updated to point at the new org repositories and hashes (e.g., via your standard gomod2nix regeneration pipeline) so Nix builds resolve against trusted, organization-controlled modules.
Also applies to: 216-218, 226-227, 315-317, 783-784
🧹 Nitpick comments (4)
CHANGELOG.md (1)
5-24: Changelog entry matches the PR scope; consider minor wording/date cleanups.The UNRELEASED line now correctly reflects this PR (staking endblocker optimization via in‑memory KV and standardized staking gas). Two small polish nits:
- Hyphenation: “staking-related messages” reads better and fixes the grammar hint.
- Dates:
*Nov 30, 2025*and the following dated sections are in the future relative to current history; if these are planned releases, consider marking them explicitly as “Unreleased” until the tag is cut to avoid confusion and markdownlint noise about emphasis-as-heading.These are non-blocking but easy to tidy.
integration_tests/cosmoscli.py (1)
21-23: Staking CLI helpers and fixed gas wiring look correct; minor duplication only.The introduction of
STAKING_DEFAULT_GASplus explicitgas/gas_pricesondelegate_amount,unbond_amount,redelegate_amount, andedit_validatoris consistent with the fixed-gas goal and matches the existing CLI argument style. The new query helpers (get_delegations,get_unbonding_delegations,get_redelegations) are also wired correctly.If you want to reduce duplication,
get_delegated_amountcould internally callget_delegations(or vice versa) since they hit the samestaking delegationsquery, but that’s purely cosmetic.Also applies to: 399-459, 479-519, 724-758
integration_tests/test_staking_cache.py (2)
302-408: Redelegation completion checks are sound; you may narrow the broadexceptif desired.The matured-redelegation phase correctly:
- Queries each node via
get_redelegations,- Treats “not found” errors as an expected sign that redelegations have fully matured (recording 0),
- Re-raises any other exception, and
- Asserts both cross-node consistency and that the final count is 0.
If you want to appease stricter linters and make the intent clearer, you could narrow
except Exception as e:to the specific CLI error type you expect fromself.raw(e.g.,subprocess.CalledProcessError) while keeping the “not found” string check and re-raise for other cases. Behavior-wise, though, this block already does the right thing.
645-783: End-to-end consistency test is thorough; consider asserting non-zero counts in pre-maturation phases.
test_staking_cache_consistencydoes a nice job of exercising delegations, redelegations, unbonding, validator unbonding, and post-maturation checks across all nodes, with strong consistency assertions at the end.In the pre-maturation phases (initial delegations, redelegations, and unbonding):
- You already assert that counts are equal across nodes (
len(set(counts)) == 1),- But you don’t assert that those counts are actually > 0.
If you want the test to also catch the case where some upstream change silently disables these flows while still returning consistent “0” everywhere, you could add simple checks like:
assert delegation_counts[0] > 0, "Expected delegations but got zero across all nodes"and similar for redelegations/unbonding. This is optional but would make the test more semantically robust without impacting the cache-consistency focus.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(7 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- integration_tests/configs/staking_cache.jsonnet
- .github/workflows/test.yml
- cmd/cronosd/cmd/versiondb.go
🧰 Additional context used
🧬 Code graph analysis (3)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
c71cb89 to
b99bf6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
gomod2nix.toml (2)
216-218: Verify: Cosmos SDK still uses personal fork replacement.Line 218 shows
replaced = "github.com/randy-cro/cosmos-sdk". Past reviews flagged this as critical and noted it was addressed in commit e920e8a, but it remains in the current state. Confirm whether this is intentional for this PR or if the fix was reverted. For production use, prefer organization-owned repositories.
315-317: Verify: Ethermint still uses personal fork replacement.Line 317 shows
replaced = "github.com/randy-cro/ethermint". Similar to the cosmos-sdk concern above, past reviews flagged this as critical and noted it was addressed in commit 8fd8052, yet it persists here. Clarify whether this is intentional or a regression, and ensure organization-owned repositories are used for production.
🧹 Nitpick comments (3)
integration_tests/test_staking_cache.py (1)
176-180: Remove redundant and misleading print statement.This print statement after the assertion uses loop-scoped variables (
node_idx,cache_size,total_count) that reflect only the last iteration of the loop, making the output misleading. The preceding assertion already validates consistency.Apply this diff:
assert ( len(set(total_unbonding_counts)) == 1 ), f"Nodes have different unbonding delegation counts: {total_unbonding_counts}" - msg = ( - f"Node {node_idx} (cache-size={cache_size}): " - f"{total_count} total unbonding delegations" - ) - print(msg) + print(f"✓ All nodes consistent: {total_unbonding_counts[0]} total unbonding delegations")CHANGELOG.md (2)
5-5: Fix hyphenation in changelog entry.Use a hyphen to join compound adjectives: change "staking related messages" to "staking-related messages".
-* [#1907](https://github.com/crypto-org-chain/cronos/pull/1907) fix: Optimize staking endblocker with an in-memory KV store and standardize gas consumption for staking related messages +* [#1907](https://github.com/crypto-org-chain/cronos/pull/1907) fix: Optimize staking endblocker with an in-memory KV store and standardize gas consumption for staking-related messages
7-7: Use proper markdown heading syntax for release date markers.The date markers (lines 7, 18, 31) use emphasis syntax (
*Nov 30, 2025*) rather than proper markdown headings, which violates MD036 (no-emphasis-as-heading). Consider using a standard heading format or removing the emphasis.-*Nov 30, 2025* +**Nov 30, 2025**Or, if dates are meant to be visual separators only, consider using plain text or a different marker style consistent with the file's convention.
Also applies to: 18-18, 31-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(8 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(4 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/cronosd/cmd/versiondb.go
- .github/workflows/test.yml
- go.mod
- integration_tests/configs/staking_cache.jsonnet
🧰 Additional context used
🧬 Code graph analysis (3)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
app/app.go (2)
x/cronos/types/keys.go (2)
MemStoreKey(18-18)StoreKey(12-12)x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages Nov 30, 2025 ## v1....
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
18-18: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: unittest
- GitHub Check: Analyze (go)
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (19)
integration_tests/cosmoscli.py (7)
22-22: LGTM! Fixed gas constant for staking operations.The
STAKING_DEFAULT_GASconstant aligns with the PR objective to implement fixed gas consumption for staking-related messages, reducing variability in gas usage.
428-441: LGTM! New query method for unbonding delegations.The method correctly queries unbonding delegations and safely handles the response structure with
.get("unbonding_responses", []).
443-458: LGTM! New query method for redelegations.The method correctly queries redelegations between validators and safely handles the response structure with
.get("redelegation_responses", []).
460-477: LGTM! Fixed gas configuration for delegations.The addition of explicit
gas_pricesandgasparameters implements the PR's fixed-gas objective for staking delegation transactions.
480-520: LGTM! Fixed gas configuration for unbonding and redelegation.The addition of explicit
gasandgas_pricesparameters to bothunbond_amountandredelegate_amountensures consistent gas usage across all staking operations.
724-760: LGTM! Enhancededit_validatorwith fixed gas andmin_self_delegation.The addition of the
min_self_delegationparameter enables test scenarios where validators can set minimum self-delegation thresholds, and the fixed gas configuration aligns with the PR's staking optimization objectives.
413-426: Remove duplicate methodget_delegations.This method is functionally identical to the existing
get_delegated_amount(lines 399-411). Both query the same CLI command with identical parameters and return the same result. Please consolidate these methods to avoid duplication.Apply this diff to remove the duplicate:
- def get_delegations(self, which_addr): - """Query all delegations made from one delegator.""" - return json.loads( - self.raw( - "query", - "staking", - "delegations", - which_addr, - home=self.data_dir, - chain_id=self.chain_id, - node=self.node_rpc, - output="json", - ) - ) -Then update the docstring of
get_delegated_amountto be more descriptive:def get_delegated_amount(self, which_addr): + """Query all delegations made from one delegator.""" return json.loads(Likely an incorrect or invalid review comment.
integration_tests/test_staking_cache.py (4)
1-26: LGTM! Clear module documentation and appropriate test marker.The module docstring provides excellent context about the test scenarios and cache configurations being tested. The
pytest.mark.stakingmarker enables selective test execution.
28-43: LGTM! Fixture and helper are correctly implemented.The fixture properly provisions a multi-node cluster with the staking cache configuration, and the helper function provides a clean interface for address resolution.
229-410: LGTM! Well-structured redelegation consistency test.The test correctly handles expected "not found" errors when querying matured redelegations (lines 367-390) while re-raising unexpected errors, ensuring proper error visibility.
413-977: LGTM! Comprehensive validator unbonding and consistency tests.Both tests thoroughly verify staking cache behavior across nodes with different cache configurations. The phase-based structure and cross-node consistency checks ensure robust validation of the staking optimization.
app/app.go (7)
228-233: LGTM: Memory store keys added to return signature.The StoreKeys() signature correctly adds memKeys as the fourth return value. This is consistent with the rest of the changes that wire memory stores throughout the application.
254-254: LGTM: Both memory store keys properly initialized.Both
stakingtypes.CacheStoreKeyandcronostypes.MemStoreKeyare correctly included in the memory store keys initialization, addressing previous review concerns.
285-288: LGTM: App struct updated with memory store keys.The App struct now includes the
memKeysfield, consistent with the updated StoreKeys() signature and initialization flow.
434-434: LGTM: Memory store keys properly captured and assigned.The memKeys are correctly captured from StoreKeys() and assigned to the App struct during initialization.
Also applies to: 448-448
654-663: LGTM: CronosKeeper correctly wired with memory store.The CronosKeeper initialization properly passes
memKeys[cronostypes.MemStoreKey], matching the expected keeper signature from x/cronos/keeper/keeper.go.
966-966: LGTM: Memory stores properly mounted.The memory stores are correctly mounted with
app.MountMemoryStores(memKeys), completing the integration of memory-backed stores into the application initialization flow.
504-515: Verify the forked cosmos-sdk supports the required parameters.Both
server.FlagStakingCacheSizeandstakingkeeper.NewKeeperare imported from the forked cosmos-sdk (github.com/randy-cro/cosmos-sdk). While the code already references the fork, you should:
- Confirm
randy-cro/cosmos-sdkdefinesserver.FlagStakingCacheSize- Confirm the
stakingkeeper.NewKeepersignature acceptsruntime.MemStoreServiceandstakingCacheSizeparameters in the positions shownSince the forked cosmos-sdk is external and cannot be inspected in the sandbox environment, please verify these changes against the fork's staking keeper implementation.
CHANGELOG.md (1)
3-7: Changelog entry verified as accurate; no issues found.The PR #1907 successfully integrates staking cache support. The cosmos-sdk fork (
randy-cro/cosmos-sdk v0.0.0-20251121082545-b3fa9253c274) contains the staking EndBlocker optimization with in-memory KV store and gas standardization. Cronos' changes properly wire the MemStoreService to the staking keeper and add comprehensive integration tests confirming staking operations work correctly with different cache configurations. The changelog entry accurately reflects the work included.
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
b99bf6c to
6a9e900
Compare
randy-cro
force-pushed
the
fix/staking-optimization
branch
from
6a9e900 to
5a5b11a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
integration_tests/test_staking_cache.py (1)
172-180: Drop or rewrite the final unbonding summary that reuses loop-scoped vars.After asserting consistency of
total_unbonding_counts, the trailingmsg/node_idx,cache_size, andtotal_countfrom the last loop iteration, so it just re-prints the last node’s values and can be misleading:msg = ( f"Node {node_idx} (cache-size={cache_size}): " f"{total_count} total unbonding delegations" ) print(msg)Either remove this block (the per-node prints inside the loop already give full detail) or replace it with a true summary based on
total_unbonding_counts(e.g., a single “all nodes consistent” line).
🧹 Nitpick comments (3)
integration_tests/cosmoscli.py (1)
399-427: Avoid duplicating the staking delegations query.
get_delegated_amountandget_delegationsboth hitquery staking delegationswith identical arguments. To keep behavior in one place and simplify future changes to the query/response shape, consider delegating one to the other:- def get_delegated_amount(self, which_addr): - return json.loads( - self.raw( - "query", - "staking", - "delegations", - which_addr, - home=self.data_dir, - chain_id=self.chain_id, - node=self.node_rpc, - output="json", - ) - ) + def get_delegated_amount(self, which_addr): + # Backwards-compatible alias to the richer delegations helper. + return self.get_delegations(which_addr)CHANGELOG.md (1)
5-5: Minor wording/consistency tweaks in changelog.Very small nits you may want to tidy up while you’re here:
- Line 5: use a hyphen in “staking-related messages” for standard English.
- Line 33: you now have both “authorization” and “authorisation” in nearby entries; consider standardizing on one spelling to avoid mixing variants.
These are purely cosmetic and don’t affect semantics.
Also applies to: 33-33
integration_tests/test_staking_cache.py (1)
93-99: Centralize the cache-size mapping used across tests.The literal
[-1, 0, 1, 2, 3, 100, 1000][node_idx]appears in many places to derive a node’s cache-size. To avoid duplication and keep tests aligned with the jsonnet config, consider introducing a module-level constant or helper:CACHE_SIZES = [-1, 0, 1, 2, 3, 100, 1000] def cache_size_for(node_idx: int) -> int: return CACHE_SIZES[node_idx]and then use
cache_size_for(node_idx)wherever you currently index the list inline. This makes future cache-size changes less error-prone.Also applies to: 154-157, 195-197, 321-323, 445-447, 485-488, 519-523, 551-553, 589-595, 804-808, 817-822, 853-857, 879-891, 961-965
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.github/workflows/test.yml(1 hunks)CHANGELOG.md(1 hunks)app/app.go(8 hunks)cmd/cronosd/cmd/versiondb.go(1 hunks)go.mod(5 hunks)gomod2nix.toml(5 hunks)integration_tests/configs/staking_cache.jsonnet(1 hunks)integration_tests/cosmoscli.py(7 hunks)integration_tests/test_staking_cache.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/cronosd/cmd/versiondb.go (1)
app/app.go (1)
StoreKeys(228-258)
app/app.go (1)
x/cronos/keeper/keeper.go (1)
NewKeeper(52-78)
integration_tests/cosmoscli.py (2)
testground/benchmark/benchmark/cli.py (1)
raw(9-21)integration_tests/network.py (2)
node_rpc(55-56)node_rpc(80-81)
integration_tests/test_staking_cache.py (3)
integration_tests/network.py (1)
setup_custom_cronos(155-197)integration_tests/utils.py (1)
wait_for_new_blocks(133-141)integration_tests/cosmoscli.py (11)
address(303-313)validators(351-356)delegate_amount(460-477)get_delegated_amount(399-411)unbond_amount(480-497)get_unbonding_delegations(428-441)redelegate_amount(500-520)get_redelegations(443-458)validator(339-349)edit_validator(724-760)status(175-176)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ... standardize gas consumption for staking related messages ### Improvements * [#...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~33-~33: Do not mix variants of the same word (‘authorisation’ and ‘authorization’) within a single text.
Context: ...rypto-org-chain/cronos/pull/1898) Check authorisation list for blocklisted address. ## v1.5....
(EN_WORD_COHERENCY)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
17-17: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
28-28: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
41-41: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (staking)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (13)
gomod2nix.toml (1)
140-141: Verify dependency version bumps are intentional and compatible.Lines 140–141, 143–144, 226–227, and 783 bump versions for btcsuite/btcd, btcsuite/btcd/btcec/v2, gogoproto, and genproto/googleapis/rpc respectively. Confirm these updates are intentional and compatible with the staking optimization work, especially for core dependencies like cosmos-sdk ecosystem modules.
Also applies to: 143-144, 226-227, 783-783
app/app.go (7)
228-233: LGTM: Memory store keys return type added.The StoreKeys() function signature correctly updated to return memory store keys as the fourth value. This breaking change is necessary for the staking optimization feature.
254-254: LGTM: Both required memory store keys initialized.The memKeys initialization correctly includes both
stakingtypes.CacheStoreKeyandcronostypes.MemStoreKey, addressing the critical issues flagged in previous review comments.
257-257: LGTM: Return statement matches updated signature.
285-288: LGTM: App struct updated with memory store keys field.The memKeys field follows the same pattern as the existing store key fields (keys, tkeys, okeys).
434-434: LGTM: Memory store keys correctly captured and assigned.The StoreKeys() call properly unpacks all four return values, and memKeys is correctly assigned to the App struct.
Also applies to: 448-448
654-663: LGTM: CronosKeeper correctly initialized with memory store key.The CronosKeeper constructor properly receives the memory store key from
memKeys[cronostypes.MemStoreKey], matching the NewKeeper signature shown in the relevant code snippets.
966-966: LGTM: Memory stores properly mounted.The memory stores are correctly mounted after other store types and before the app initialization, ensuring they are available when the keepers need them.
cmd/cronosd/cmd/versiondb.go (1)
16-16: LGTM: Correctly updated for StoreKeys() signature change.The code properly unpacks all four return values from StoreKeys() and discards memKeys since this command only needs the KV store keys.
integration_tests/cosmoscli.py (2)
399-459: New staking query helpers are consistent with CLI usage.
get_delegations,get_unbonding_delegations, andget_redelegationsmirror the underlyingstakingqueries and return shapes your tests expect (delegation_responses,unbonding_responses,redelegation_responses) while letting CLI errors surface, which is exactly what you want for fail-fast integration tests.
22-22: All existing call sites are compatible—no custom gas overrides detected.Verification of
delegate_amount,unbond_amount,redelegate_amount, andedit_validatoracross the integration tests reveals no call sites passing customgasorgas_priceskwargs. All calls use only positional arguments and a single keyword argument (min_self_delegationinedit_validator). The hard-coded gas values do not break any existing functionality.integration_tests/configs/staking_cache.jsonnet (1)
1-268: Staking cache test config looks coherent and well-targeted.The 7-validator setup with varied
staking.cache-sizevalues, short unbonding time, ample test balances, and tuned feemarket/mint params is well-suited for the new staking cache tests; structure and field names match existing pystarport configs and Cronos modules.integration_tests/test_staking_cache.py (1)
229-411: Staking cache integration flows are comprehensive and fail fast on errors.The three scenarios (multiple unbondings, multiple redelegations, validator unbonding, plus the combined consistency test) are well structured: every tx checks
rsp["code"], blocks are advanced explicitly viawait_for_new_blocks, and cross-node comparisons use simple set-equality on counts/statuses to detect divergence. The limited try/except blocks only special-case expected “not found” redelegation queries after maturation and otherwise re-raise, which keeps regressions visible.Also applies to: 413-611, 613-977
| stakingCacheSize := cast.ToInt(appOpts.Get(server.FlagStakingCacheSize)) | ||
| app.StakingKeeper = stakingkeeper.NewKeeper( | ||
| appCodec, | ||
| runtime.NewKVStoreService(keys[stakingtypes.StoreKey]), | ||
| runtime.NewMemStoreService(memKeys[stakingtypes.CacheStoreKey]), | ||
| app.AccountKeeper, | ||
| app.BankKeeper, | ||
| authAddr, | ||
| address.NewBech32Codec(sdk.GetConfig().GetBech32ValidatorAddrPrefix()), | ||
| address.NewBech32Codec(sdk.GetConfig().GetBech32ConsensusAddrPrefix()), | ||
| stakingCacheSize, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the FlagStakingCacheSize flag exists.
The staking keeper initialization retrieves a cache size using server.FlagStakingCacheSize and passes it to the keeper constructor along with a memory store service. Ensure that this flag is defined in the cosmos-sdk server package or in this codebase.
Run the following script to verify the flag exists:
🏁 Script executed:
#!/bin/bash
# Search for FlagStakingCacheSize definition in the codebase and dependencies
echo "=== Searching for FlagStakingCacheSize in codebase ==="
rg -n 'FlagStakingCacheSize' --type go -C 3
echo -e "\n=== Checking if it's defined in server package ==="
rg -n 'FlagStakingCacheSize.*=' --type goLength of output: 442
🏁 Script executed:
#!/bin/bash
# Search for other server.Flag* usages to understand the pattern
echo "=== Searching for other server.Flag usages ==="
rg -n 'server\.Flag[A-Za-z]*' --type go -C 1 | head -40
echo -e "\n=== Checking go.mod for cosmos-sdk dependency ==="
cat go.mod | grep -i "cosmos-sdk"Length of output: 2580
🌐 Web query:
cosmos-sdk server FlagStakingCacheSize flag
💡 Result:
Short answer: There is no built‑in server flag named FlagStakingCacheSize in the upstream cosmos-sdk server package. The server flags list in cosmos-sdk does not include a "staking cache size" flag. [1]
What exists in practice: some application chains add a staking cache-size configuration under their staking section (app.toml) or as an app-level config (e.g., Cronos added a [staking] cache-size option in its releases to control the module’s cache). That is an app/chain-level config, not a built-in cosmos-sdk server flag. [2][3]
Sources
- cosmos-sdk server flags (no FlagStakingCacheSize present). [1]
- Cronos release notes showing a [staking] cache-size config option. [2]
- community/app config summary mentioning staking/app toml cache options. [3]
🏁 Script executed:
#!/bin/bash
# Search for where flags are registered in the root command
echo "=== Searching for flag registration patterns ==="
rg -n 'Flags\(\)\.Int.*Flag' --type go -C 2
echo -e "\n=== Checking root.go for flag registration ==="
cat cmd/cronosd/cmd/root.go | grep -A 5 -B 5 'Flags\(\)' | head -60
echo -e "\n=== Searching for server flag definitions in this codebase ==="
rg -n 'const.*Flag.*=' --type go -C 1 | grep -i stakingLength of output: 2327
🏁 Script executed:
#!/bin/bash
# Search for FlagStakingCacheSize constant definition and flag registration
echo "=== Searching for FlagStakingCacheSize as const or var ==="
rg -n 'FlagStakingCacheSize\s*=' --type go
echo -e "\n=== Searching for Staking and Cache in constant definitions ==="
rg -n 'const.*Staking|const.*Cache' --type go -A 1 -B 1
echo -e "\n=== Checking cronos module for flag registration ==="
fd 'module.go|init.go' --type f | xargs grep -l 'AddModuleInitFlags' 2>/dev/null
echo -e "\n=== Looking for server flag definitions ==="
rg -n 'FlagIAVLCacheSize|FlagMempoolMaxTxs' --type go -B 3 | grep -E 'const|var' -A 1Length of output: 689
🏁 Script executed:
#!/bin/bash
# Verify the import path for server and check if flag constant exists
echo "=== Checking app/app.go imports ==="
head -50 app/app.go | grep -E 'import|server'
echo -e "\n=== Checking if server is cosmos-sdk or custom ==="
rg -n 'import.*server' app/app.go -A 30 | head -40
echo -e "\n=== Searching for any definition or mention of StakingCacheSize ==="
rg -rn 'StakingCacheSize' --type goLength of output: 235
🏁 Script executed:
#!/bin/bash
# Direct read of the exact line in question
echo "=== Reading lines 500-515 from app/app.go ==="
sed -n '500,515p' app/app.go
echo -e "\n=== Verifying no FlagStakingCacheSize definition exists anywhere ==="
rg 'FlagStakingCacheSize' --type go
echo -e "\n=== Check the import block of app/app.go for server package ==="
sed -n '1,100p' app/app.go | grep -A 50 '^import'Length of output: 3321
FlagStakingCacheSize is undefined and will cause a runtime error.
The code at line 504 attempts to retrieve server.FlagStakingCacheSize, but this flag constant is not defined in the codebase and does not exist in the cosmos-sdk server package. This will cause either a compilation error or a nil value at runtime, resulting in undefined behavior when the staking keeper is initialized.
Required fixes:
- Define the
FlagStakingCacheSizeconstant in the appropriate location (likely in a srvflags package or cronos module flags) - Register the flag with cobra in the command initialization (likely in
addModuleInitFlags()or a similar flag registration function)
Verify that similar flags like FlagMempoolMaxTxs and FlagIAVLCacheSize follow the same pattern for reference.
🤖 Prompt for AI Agents
In app/app.go around lines 504 to 515, the code references
server.FlagStakingCacheSize which does not exist; define a new
FlagStakingCacheSize constant in the same flags package you use for other flags
(e.g., srvflags or your module's flags file) and mirror the naming/value pattern
used by FlagMempoolMaxTxs and FlagIAVLCacheSize, then register this flag with
cobra in your command initialization (e.g., addModuleInitFlags or equivalent) so
appOpts.Get(server.FlagStakingCacheSize) returns a valid value; ensure the flag
is added to the same command/flag set used by the app options and update any
imports to reference the correct flags package.
| version = "v0.0.0-20251121082919-46c057ac4dde" | ||
| hash = "sha256-8eJk4xUTm1aRKJiuHdTv0ZGg4kwTUrTiO8E/NyHlU8A=" | ||
| replaced = "github.com/randy-cro/ethermint" |
There was a problem hiding this comment.
Critical: ethermint module replacement still points to personal fork.
The github.com/evmos/ethermint module replacement at line 317 is still using github.com/randy-cro/ethermint (personal fork) instead of an organization-owned repository. This contradicts the past review comment indicating this issue was addressed in commit 8fd8052.
For production releases, use the organization-owned fork or upstream module. Update the replacement target to point to github.com/crypto-org-chain/ethermint (consistent with how cosmos-sdk was corrected at line 218), and regenerate the hash accordingly using your gomod2nix tooling.
🤖 Prompt for AI Agents
In gomod2nix.toml around lines 315-317, the replace entry still points to the
personal fork "github.com/randy-cro/ethermint"; change the replaced value to the
organization-owned module "github.com/crypto-org-chain/ethermint" (matching the
fix applied for cosmos-sdk at line 218) and then regenerate the corresponding
version/hash pair using your gomod2nix tooling so the version and sha256 reflect
the new upstream source.
Description
To address the issue of slow nodes due to heavy i/o in staking endblocker, two key changes were introduced:
Use of an in-memory KV store to manage staking caches.
Reference
Implementation of fixed gas consumption for staking-related messages to ensure consistent gas usage.
Reference
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Tests
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.