refactor: change byte prefixing scheme for provider #2061
Conversation
github-actions
bot
added
C:x/provider
WalkthroughWalkthroughThe changes involve refactoring key prefix handling in the Cosmos Interchain Security module. The main goal is to shift from using brittle byte prefixes defined with Go's Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/ccv/provider/types/keys.go
Outdated
| for _, prefix := range prefixMap { | ||
| prefixList = append(prefixList, prefix) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
x/ccv/provider/types/keys.go
Outdated
| func MustGetKeyPrefix(index string) byte { | ||
| keyPrefixes := getKeyPrefixes() | ||
| if prefix, found := keyPrefixes[index]; !found { | ||
| panic(fmt.Sprintf("could not find key prefix for index %s", index)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
x/ccv/provider/keeper/validator_set_update.go (1)
56-56: Issue Found: Missing Key PrefixThe key prefix for "ConsumerValidatorKey" is missing in the
getKeyPrefixesfunction, which will cause a runtime error whenMustGetKeyPrefix("ConsumerValidatorKey")is called.
- File:
x/ccv/provider/types/keys.go- Function:
getKeyPrefixesPlease add the key prefix for "ConsumerValidatorKey" to the map returned by
getKeyPrefixes.Analysis chain
Verify the correctness of the new key prefix usage.
Ensure that
types.MustGetKeyPrefix("ConsumerValidatorKey")returns the correct and expected key prefix.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new key prefix usage. # Test: Search for the MustGetKeyPrefix function definition and its usage. # Expect: Correct key prefix for "ConsumerValidatorKey". rg --type go --pattern 'MustGetKeyPrefix("ConsumerValidatorKey")'Length of output: 479
Script:
#!/bin/bash # Description: Verify the correctness of the new key prefix usage. # Test: Search for the MustGetKeyPrefix function definition and its usage. # Expect: Correct key prefix for "ConsumerValidatorKey". rg 'MustGetKeyPrefix("ConsumerValidatorKey")' --type goLength of output: 57
Script:
#!/bin/bash # Description: Search for the MustGetKeyPrefix function definition and inspect its implementation. # Search for the MustGetKeyPrefix function definition. rg 'func MustGetKeyPrefix' --type go -A 10Length of output: 667
Script:
#!/bin/bash # Description: Search for the getKeyPrefixes function definition and inspect its implementation. # Search for the getKeyPrefixes function definition. rg 'func getKeyPrefixes' --type go -A 10Length of output: 837
Script:
#!/bin/bash # Description: Display the full implementation of the getKeyPrefixes function to verify if "ConsumerValidatorKey" is included. # Display the full implementation of the getKeyPrefixes function. rg 'func getKeyPrefixes' --type go -A 50Length of output: 3732
| func MustGetKeyPrefix(key string) byte { | ||
| keyPrefixes := getKeyPrefixes() | ||
| if prefix, found := keyPrefixes[key]; !found { | ||
| panic(fmt.Sprintf("could not find key prefix for index %s", key)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (3)
x/ccv/provider/types/keys.go (3)
Line range hint
325-637:
Update key retrieval functions to handle errors fromMustGetKeyPrefix.The key retrieval functions use
MustGetKeyPrefix, which can panic if the key is not found. Consider updating these functions to handle errors fromMustGetKeyPrefixinstead.- func ParametersKey() []byte { - return []byte{MustGetKeyPrefix(ParametersKeyName)} - } + func ParametersKey() ([]byte, error) { + prefix, err := MustGetKeyPrefix(ParametersKeyName) + if err != nil { + return nil, err + } + return []byte{prefix}, nil + } # Repeat similar changes for other key retrieval functions.Tools
golangci-lint
532-532: File is not
gofumpt-ed with-extra(gofumpt)
Line range hint
477-482:
Avoid using panic in consensus methods.Using panic in
MustParseThrottledPacketDataKeycan cause a chain halt if used in consensus methods. Consider returning an error instead.- func MustParseThrottledPacketDataKey(key []byte) (string, uint64) { - chainId, ibcSeqNum, err := ParseThrottledPacketDataKey(key) - if err != nil { - panic(fmt.Sprintf("failed to parse throttled packet data key: %s", err.Error())) - } - return chainId, ibcSeqNum - } + func MustParseThrottledPacketDataKey(key []byte) (string, uint64, error) { + chainId, ibcSeqNum, err := ParseThrottledPacketDataKey(key) + if err != nil { + return "", 0, fmt.Errorf("failed to parse throttled packet data key: %s", err.Error()) + } + return chainId, ibcSeqNum, nil + }Tools
golangci-lint
362-362: File is not
gofumpt-ed with-extra(gofumpt)
GitHub Check: CodeQL
[warning] 303-305: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 314-316: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 292-292: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
Line range hint
502-517:
Avoid using panic in consensus methods.Using panic in
MustParseGlobalSlashEntryKeycan cause a chain halt if used in consensus methods. Consider returning an error instead.- func MustParseGlobalSlashEntryKey(bz []byte) ( - recvTime time.Time, consumerChainID string, ibcSeqNum uint64, - ) { - // Prefix is in first byte - expectedPrefix := []byte{MustGetKeyPrefix(GlobalSlashEntryKeyName)} - if prefix := bz[:1]; !bytes.Equal(prefix, expectedPrefix) { - panic(fmt.Sprintf("invalid prefix; expected: %X, got: %X", expectedPrefix, prefix)) - } - // 8 bytes for uint64 storing time bytes - timeBz := sdk.BigEndianToUint64(bz[1:9]) - recvTime = time.Unix(0, int64(timeBz)).UTC() - // 8 bytes for uint64 storing ibc seq num - ibcSeqNum = sdk.BigEndianToUint64(bz[9:17]) - // ChainID is stored after 8 byte ibc seq num - chainID := string(bz[17:]) - return recvTime, chainID, ibcSeqNum - } + func MustParseGlobalSlashEntryKey(bz []byte) ( + recvTime time.Time, consumerChainID string, ibcSeqNum uint64, error, + ) { + // Prefix is in first byte + expectedPrefix := []byte{MustGetKeyPrefix(GlobalSlashEntryKeyName)} + if prefix := bz[:1]; !bytes.Equal(prefix, expectedPrefix) { + return time.Time{}, "", 0, fmt.Errorf("invalid prefix; expected: %X, got: %X", expectedPrefix, prefix) + } + // 8 bytes for uint64 storing time bytes + timeBz := sdk.BigEndianToUint64(bz[1:9]) + recvTime = time.Unix(0, int64(timeBz)).UTC() + // 8 bytes for uint64 storing ibc seq num + ibcSeqNum = sdk.BigEndianToUint64(bz[9:17]) + // ChainID is stored after 8 byte ibc seq num + chainID := string(bz[17:]) + return recvTime, chainID, ibcSeqNum, nil + }
github-actions
bot
added
the
C:x/consumer
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
x/ccv/provider/types/keys.go (1)
125-286: Missing Key Prefixes ingetKeyPrefixesFunctionThe following key prefixes are used in the codebase but are missing from the
getKeyPrefixesfunction:
x/ccv/provider/types/keys.go:AllowlistKeyNamex/ccv/provider/types/keys.go:ChainToChannelKeyNamex/ccv/provider/types/keys.go:ChainToClientKeyNamex/ccv/provider/types/keys.go:ChannelToChainKeyNamex/ccv/provider/types/keys.go:ConsumerAddrsToPruneKeyNamex/ccv/provider/types/keys.go:ConsumerCommissionRateKeyNamex/ccv/provider/types/keys.go:ConsumerGenesisKeyNamex/ccv/provider/types/keys.go:ConsumerRewardDenomsKeyNamex/ccv/provider/types/keys.go:ConsumerRewardsAllocationKeyNamex/ccv/provider/types/keys.go:ConsumerValidatorKeyNamex/ccv/provider/types/keys.go:ConsumerValidatorsKeyNamex/ccv/provider/types/keys.go:DenylistKeyNamex/ccv/provider/types/keys.go:EquivocationEvidenceMinHeightKeyNamex/ccv/provider/types/keys.go:GlobalSlashEntryKeyNamex/ccv/provider/types/keys.go:InitChainHeightKeyNamex/ccv/provider/types/keys.go:InitTimeoutTimestampKeyNamex/ccv/provider/types/keys.go:KeyAssignmentReplacementsKeyNamex/ccv/provider/types/keys.go:MaturedUnbondingOpsKeyNamex/ccv/provider/types/keys.go:MinimumPowerInTopNKeyNamex/ccv/provider/types/keys.go:OptedInKeyNamex/ccv/provider/types/keys.go:ParametersKeyNamex/ccv/provider/types/keys.go:PendingCAPKeyNamex/ccv/provider/types/keys.go:PendingCRPKeyNamex/ccv/provider/types/keys.go:PendingVSCsKeyNamex/ccv/provider/types/keys.go:PortKeyNamex/ccv/provider/types/keys.go:ProposedConsumerChainKeyNamex/ccv/provider/types/keys.go:SlashAcksKeyNamex/ccv/provider/types/keys.go:SlashLogKeyNamex/ccv/provider/types/keys.go:SlashMeterKeyNamex/ccv/provider/types/keys.go:SlashMeterReplenishTimeCandidateKeyNamex/ccv/provider/types/keys.go:ThrottledPacketDataKeyNamex/ccv/provider/types/keys.go:ThrottledPacketDataSizeKeyNamex/ccv/provider/types/keys.go:TopNKeyNamex/ccv/provider/types/keys.go:UnbondingOpIndexKeyNamex/ccv/provider/types/keys.go:UnbondingOpKeyNamex/ccv/provider/types/keys.go:VSCMaturedHandledThisBlockKeyNamex/ccv/provider/types/keys.go:ValidatorSetCapKeyNamex/ccv/provider/types/keys.go:ValidatorSetUpdateIdKeyNamex/ccv/provider/types/keys.go:ValidatorsByConsumerAddrKeyNamex/ccv/provider/types/keys.go:ValidatorsPowerCapKeyNamex/ccv/provider/types/keys.go:ValsetUpdateBlockHeightKeyNamex/ccv/provider/types/keys.go:VscSendTimestampKeyNamePlease ensure these keys are included in the
getKeyPrefixesfunction.Analysis chain
LGTM! Verify the completeness of key prefixes.
The function
getKeyPrefixesis well-defined and improves maintainability by centralizing key prefixes.However, ensure that all necessary key prefixes are included.
To ensure the completeness of key prefixes, we need to gather all key names used in the codebase and compare them with the keys defined in the
getKeyPrefixesfunction.
To verify the completeness of key prefixes, we will use the
diffcommand to compare the key names extracted from the codebase with those in thegetKeyPrefixesfunction.
To verify the completeness of key prefixes, we will write the key names to temporary files and then use the
diffcommand to compare them.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the completeness of key prefixes. # Test: Ensure all key prefixes are included in `getKeyPrefixes`. rg --type go -A 5 $'getKeyPrefixes'Length of output: 1907
Script:
#!/bin/bash # Description: Verify the completeness of key prefixes by comparing all key names used in the codebase with those in `getKeyPrefixes`. # Step 1: Extract all key names from the codebase. key_names=$(rg --type go -o '\b[A-Za-z]+KeyName\b' | sort | uniq) # Step 2: Extract all key names from the `getKeyPrefixes` function. prefix_keys=$(rg --type go -o '\b[A-Za-z]+KeyName\b' x/ccv/provider/types/keys.go | sort | uniq) # Step 3: Compare the lists and find any missing keys. echo "Key names in codebase:" echo "$key_names" echo echo "Key names in getKeyPrefixes:" echo "$prefix_keys" echo echo "Missing keys in getKeyPrefixes:" comm -23 <(echo "$key_names") <(echo "$prefix_keys")Length of output: 13561
Script:
#!/bin/bash # Description: Verify the completeness of key prefixes by comparing all key names used in the codebase with those in `getKeyPrefixes`. # Step 1: Extract all key names from the codebase. key_names=$(rg --type go -o '\b[A-Za-z]+KeyName\b' | sort | uniq) # Step 2: Extract all key names from the `getKeyPrefixes` function. prefix_keys=$(rg --type go -o '\b[A-Za-z]+KeyName\b' x/ccv/provider/types/keys.go | sort | uniq) # Step 3: Compare the lists and find any missing keys using `diff`. echo "Key names in codebase:" echo "$key_names" echo echo "Key names in getKeyPrefixes:" echo "$prefix_keys" echo echo "Missing keys in getKeyPrefixes:" diff <(echo "$key_names") <(echo "$prefix_keys") | grep '<'Length of output: 13583
Script:
#!/bin/bash # Description: Verify the completeness of key prefixes by comparing all key names used in the codebase with those in `getKeyPrefixes`. # Step 1: Extract all key names from the codebase. key_names=$(rg --type go -o '\b[A-Za-z]+KeyName\b' | sort | uniq) # Step 2: Extract all key names from the `getKeyPrefixes` function. prefix_keys=$(rg --type go -o '\b[A-Za-z]+KeyName\b' x/ccv/provider/types/keys.go | sort | uniq) # Step 3: Write the lists to temporary files. echo "$key_names" > /tmp/key_names.txt echo "$prefix_keys" > /tmp/prefix_keys.txt # Step 4: Compare the lists and find any missing keys using `diff`. echo "Key names in codebase:" cat /tmp/key_names.txt echo echo "Key names in getKeyPrefixes:" cat /tmp/prefix_keys.txt echo echo "Missing keys in getKeyPrefixes:" diff /tmp/key_names.txt /tmp/prefix_keys.txt | grep '<'Length of output: 12609
|
Just to double-check, this doesn't need a migration, since the actual keys remain the same, correct? |
| keyPrefixes := getKeyPrefixes() | ||
| if prefix, found := keyPrefixes[key]; !found { | ||
| panic(fmt.Sprintf("could not find key prefix for index %s", key)) | ||
| } else { |
There was a problem hiding this comment.
very nit: else statement is unnecessary here.
There was a problem hiding this comment.
without the else I need to define prefix outside the if.
|
LGTM. Thanks for the refactor! |
Description
Closes: #939
TODO:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Bug Fixes
Refactor
Tests