Problem: iavl prune bug not reproduced in integration test #1705
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1705 +/- ##
===========================================
+ Coverage 16.87% 33.68% +16.81%
===========================================
Files 72 124 +52
Lines 6163 11898 +5735
===========================================
+ Hits 1040 4008 +2968
- Misses 5000 7521 +2521
- Partials 123 369 +246
|
Solution: manage to reproduce it fix test temp temp
yihuang
requested review from
mmsqe and
leejw51crypto
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
integration_tests/test_permissions.py (3)
7-10: LGTM, but consider enhancing the docstring.The docstring provides good context, but could be more detailed about the expected behavior and the specific conditions needed to reproduce the IAVL pruning issue.
Consider expanding the docstring to include:
- Expected permission states before and after updates
- Specific conditions required to trigger the IAVL pruning issue
- Why node1 is specifically required for this test
24-26: LGTM, but enhance error validation.The test correctly verifies both unauthorized and authorized permission updates, but could be more specific about expected error messages.
Consider adding specific error message validation:
rsp = cli.update_permissions(acc, 3, from_="community") - assert rsp["code"] != 0, "should not have the permission" + assert rsp["code"] != 0 and "unauthorized" in rsp["raw_log"].lower(), \ + f"Expected unauthorized error, got: {rsp['raw_log']}"
38-41: LGTM, but add explicit state verification.The test verifies the permission update succeeds after pruning, but doesn't explicitly verify the final permission state.
Consider adding final state verification:
rsp = cli.update_permissions(acc, 4, from_="admin") assert rsp["code"] == 0, rsp["raw_log"] wait_for_new_blocks(cli, 5) + final_perms = cli.query_permissions(acc) + assert final_perms["can_change_token_mapping"] is True, "Permissions not preserved after pruning"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(2 hunks)integration_tests/cosmoscli.py(1 hunks)integration_tests/test_permissions.py(2 hunks)scripts/.env(1 hunks)store/rootmulti/store.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration_tests/cosmoscli.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
store/rootmulti/store.go
[warning] 449-449: store/rootmulti/store.go#L449
Added line #L449 was not covered by tests
🔇 Additional comments (6)
integration_tests/test_permissions.py (1)
27-32: Verify the necessity of increased block wait time.
The wait time has been increased from 1 to 5 blocks. While this might be necessary for the IAVL pruning reproduction, it increases test execution time.
Could you confirm if this increased wait time is specifically required for the IAVL pruning issue reproduction? If not, we could optimize the test execution time.
scripts/.env (2)
11-11: LGTM! Export keyword standardization.
The standardization of the export keyword for IBC_CRO_DENOM maintains consistency while preserving the correct IBC denomination value.
9-10: LGTM! Verify the admin address derivation.
The addition of CRONOS_ADMIN using the same account as VALIDATOR1_MNEMONIC is well-documented.
Let's verify the address derivation from the mnemonic:
✅ Verification successful
The admin address is correctly used and verified in tests
The address crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp is consistently used across the codebase:
- In test cases validating Cronos address formats
- In integration tests for governance parameter updates
- In test configurations for auto-deployment settings
- In message validation tests for token mapping updates
The address format is validated in params_test.go and is being used as a known-good test address, which aligns with its reuse as the admin address. The consistent usage across test files indicates this is a well-established test account.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that CRONOS_ADMIN address matches VALIDATOR1_MNEMONIC
# Note: This assumes the presence of the Cronos CLI tool in the test environment
# Extract the values
ADMIN_ADDRESS=$(grep CRONOS_ADMIN scripts/.env | cut -d'"' -f2)
VALIDATOR1_MNEMONIC=$(grep VALIDATOR1_MNEMONIC scripts/.env | cut -d'"' -f2)
echo "Admin address: $ADMIN_ADDRESS"
echo "Expected to match address derived from Validator1 mnemonic: $VALIDATOR1_MNEMONIC"
Length of output: 1048
Script:
#!/bin/bash
# Let's search for any tests or scripts that use this admin address
# to verify its usage and correctness
# Search for the admin address in the codebase
rg "crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp" -B 2 -A 2
# Also search for any validator address derivation logic
rg -l "VALIDATOR1_MNEMONIC|CRONOS_ADMIN" --type ts --type js --type go
Length of output: 3274
go.mod (2)
92-92: Verify IAVL version selection
The PR description mentions reproducing the bug with v1.2.1, but this change updates to v1.2.2. Please verify if v1.2.2 contains the fix for the pruning bug.
254-256: Verify compatibility with updated dependencies
All cosmos-sdk related replacements are updated to the same commit hash. Please ensure these versions are compatible with the new IAVL v1.2.2.
✅ Verification successful
Dependencies are compatible with IAVL v1.2.2
The codebase is already using IAVL v1.2.2 as specified in go.mod, and there are multiple direct imports of IAVL across the codebase, particularly in the versiondb and memiavl packages. Since the cosmos-sdk fork replacements are all pointing to the same commit hash (6031e793ee95), and the codebase is actively using IAVL v1.2.2, this indicates the dependencies are aligned and compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for IAVL version compatibility in the updated cosmos-sdk fork
# Check IAVL version in cosmos-sdk go.mod
gh api repos/crypto-org-chain/cosmos-sdk/contents/go.mod -q '.content' | base64 -d | grep "github.com/cosmos/iavl"
# Check for any breaking changes in the cosmos-sdk fork
gh api repos/crypto-org-chain/cosmos-sdk/commits/6031e793ee95 --jq '.commit.message' | grep -i "breaking"
Length of output: 313
Script:
#!/bin/bash
# Let's try a different approach to check IAVL compatibility
# First, let's check our own go.mod for IAVL version
rg "github.com/cosmos/iavl" go.mod -A 2 -B 2
# Check if there are any IAVL-related imports in the codebase
rg "github.com/cosmos/iavl" --type go
# Let's also check for any store or IAVL-related changes in recent commits
git log --oneline -n 10 | grep -i "store\|iavl"
Length of output: 1117
store/rootmulti/store.go (1)
448-451: Add documentation for empty implementation.
The empty implementation appears intentional since memiavl manages its own pruning strategy. However, it would be helpful to add a comment explaining this, similar to other pruning-related methods in this file.
// Implements interface CommitMultiStore
func (rs *Store) SetIAVLSyncPruning(syncPruning bool) {
+ // not needed, memiavl manages its own pruning strategy
}Additionally, please add test coverage for this method to maintain consistency with the rest of the codebase.
✅ Verification successful
Let me gather more context about the pruning-related methods and tests in the codebase.
Let me check one more thing to understand the relationship between memiavl and pruning functionality.
Add explanatory comment for empty implementation
The empty implementation is correct as it follows the same pattern as other pruning-related methods in the file. Please add a comment to maintain consistency with similar methods:
// Implements interface CommitMultiStore
func (rs *Store) SetIAVLSyncPruning(syncPruning bool) {
+ // not needed, memiavl manage its own snapshot/pruning strategy
}No test coverage is needed since this is an intentionally empty method, consistent with other pruning-related methods in the same file that are also empty and documented as not needed due to memiavl managing its own pruning strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the SetIAVLSyncPruning method
# Search for test files that might contain tests for this method
fd -e go -e _test.go | xargs rg "SetIAVLSyncPruning"
Length of output: 133
Script:
#!/bin/bash
# Search for other pruning-related methods and their tests
rg -A 3 "func.*Pruning|func.*prune" --type go
# Search for the CommitMultiStore interface definition
ast-grep --pattern 'type CommitMultiStore interface {
$$$
}'
Length of output: 1675
Script:
#!/bin/bash
# Search for comments and documentation about memiavl pruning
rg -B 2 -A 2 "memiavl.*pruning|pruning.*memiavl" --type go
# Check if there are other empty pruning-related methods with explanatory comments
rg -B 1 -A 1 "not needed, memiavl|memiavl manage" --type go
Length of output: 1165
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 449-449: store/rootmulti/store.go#L449
Added line #L449 was not covered by tests
| @@ -89,7 +89,7 @@ require ( | |||
| github.com/cosmos/cosmos-proto v1.0.0-beta.5 // indirect | |||
| github.com/cosmos/go-bip39 v1.0.0 // indirect | |||
| github.com/cosmos/gogogateway v1.2.0 // indirect | |||
| github.com/cosmos/iavl v1.2.1-0.20240731145221-594b181f427e // indirect | |||
| github.com/cosmos/iavl v1.2.2 // indirect | |||
There was a problem hiding this comment.
|
right, not sure why get app mismatch in versiondb after permission test |
Solution: manage to reproduce it, change the iavl dependency to
v1.2.1will fail thetest_permissionstest case.👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
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
Release Notes
New Features
prunemethod in the CosmosCLI for data management.Bug Fixes
Chores