introduce slashing for oracle service #224
Conversation
WalkthroughThe changes in this pull request primarily enhance the integration and functionality of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (18)
proto/exocore/oracle/v1/slashing.proto (1)
8-11: Suggestion: Use snake_case for all field names.The field names and types are generally appropriate, but there's an inconsistency in naming convention:
- The
startHeightfield uses camelCase, while the recommended style for Protocol Buffers is snake_case.- Other fields correctly use snake_case.
Consider changing
startHeighttostart_heightfor consistency and to follow Protocol Buffers best practices:- int64 startHeight = 2; + int64 start_height = 2;🧰 Tools
🪛 buf
9-9: Field name "startHeight" should be lower_snake_case, such as "start_height".
(FIELD_LOWER_SNAKE_CASE)
🪛 GitHub Check: lint
[failure] 9-9:
Field name "startHeight" should be lower_snake_case, such as "start_height".x/oracle/types/key_slashing.go (1)
1-6: Remove commented-out code.The commented-out constant declaration is unnecessary and should be removed to maintain clean code.
Apply this diff to remove the commented-out code:
-// const ( -// SlashingPrefix = "Slashing/" -// )x/oracle/types/types.go (1)
39-44: Add comments to explain theAggFinalPricestruct and its fields.While the struct seems well-structured, adding comments would improve code readability and maintainability. Consider adding a comment explaining the purpose of the
AggFinalPricestruct and brief descriptions for each field.Here's a suggested improvement:
+// AggFinalPrice represents an aggregated final price for an oracle. type AggFinalPrice struct { + // FeederID is the unique identifier for the feeder FeederID uint64 + // SourceID is the unique identifier for the price source SourceID uint64 + // DetID is the deterministic identifier for the price DetID string + // Price is the final aggregated price value Price string }x/oracle/types/expected_keepers.go (2)
4-5: LGTM. Consider removing the redundant import alias.The import of the
timepackage is correct and necessary for the newSlashingKeeperinterface. However, the alias "time" is redundant as it matches the package name.You can simplify the import statement:
-time "time" +import "time"
32-34: LGTM. Consider adding documentation for the interface and method.The
SlashingKeeperinterface is well-defined and aligns with the PR objectives to introduce slashing for the oracle service. TheJailUntilmethod signature is correct and uses appropriate types.To improve code clarity, consider adding documentation comments:
// SlashingKeeper defines the expected slashing keeper used for oracle operations type SlashingKeeper interface { // JailUntil jails a validator until a specified time JailUntil(ctx sdk.Context, consAddr sdk.ConsAddress, jailEnd time.Time) }x/oracle/types/events.go (2)
5-6: LGTM: New event types for oracle liveness and slashing.The addition of
EventTypeOracleLivenessandEventTypeOracleSlashaligns well with the PR objectives. These will be useful for emitting events related to oracle liveness checks and slashing actions.Consider adding inline comments to briefly explain the purpose of these new event types for better code documentation.
19-25: LGTM: New attribute keys for slashing and validator performance.The addition of these attribute key constants (
AttributeKeyValidatorKey,AttributeKeyMissedRounds,AttributeKeyHeight,AttributeKeyPower,AttributeKeyReason,AttributeKeyJailed,AttributeKeyBurnedCoins) provides a comprehensive set of keys for events related to oracle liveness and slashing.Consider grouping these new constants together with a comment to indicate their relation to slashing and validator performance. This would improve code organization and readability.
x/oracle/keeper/keeper.go (1)
28-28: Approve the addition of SlashingKeeper with a minor suggestion.The addition of
SlashingKeeperto theKeeperstruct is appropriate for integrating slashing capabilities into the oracle service. This aligns well with the PR objectives.Consider adding a comment to explain the purpose of this field, e.g.:
// SlashingKeeper provides access to slashing functionality for oracle validators SlashingKeeper types.SlashingKeepertestutil/keeper/oracle.go (1)
Line range hint
1-70: Summary: Slashing integration looks good, but verify actual implementation.The changes to integrate the slashing keeper with the oracle keeper are minimal and align with the PR objectives. However, there are a few points to consider:
- Ensure that the slashing keeper is properly initialized and used in the actual oracle keeper implementation, not just in this test utility.
- Verify that all necessary changes have been made in other parts of the system to support this integration (e.g., in the main application setup).
- Consider adding tests that specifically cover the new slashing functionality in the oracle module.
Given the integration of slashing with the oracle service, it would be beneficial to:
- Document the slashing conditions and mechanisms specific to the oracle service.
- Ensure that the oracle module's documentation is updated to reflect this new capability.
- Consider adding integration tests that cover the interaction between the oracle and slashing modules.
x/operator/keeper/slash.go (2)
26-26: LGTM. Consider adding more context to the comment.The updated comment clarifies the function's behavior, which is great. To further improve it, consider adding a brief explanation of why this reduction is necessary and its implications in the broader context of the slashing mechanism.
139-139: Good catch. Consider creating an issue to track this enhancement.The TODO comment identifies an important edge case in the slashing mechanism. To ensure this doesn't get overlooked:
- Create a GitHub issue to track this enhancement.
- In the issue, detail the steps needed to implement this check and the potential impact on the system.
- Consider adding a link to the created issue in this TODO comment for easier tracking.
This will help in maintaining system integrity by properly handling operators with no remaining assets after slashing.
Would you like me to draft the GitHub issue for this enhancement?
x/oracle/keeper/aggregator/worker.go (1)
23-23: Address the TODO comment by movingrecordsinto contextThe TODO comment indicates that the
recordsfield should be moved into the context via.ctx. Refactoring to moverecordsinto the context can enhance modularity and maintainability.Would you like assistance in refactoring the code to move
recordsinto the context?x/oracle/keeper/aggregator/aggregator.go (1)
196-198: Correct typos and ensure consistency in function comments.
- At line 196, the word "suites" should be "suits" in the TODO comment.
- The function comment at line 197 mentions
GetFinaPriceListForFeederIDs, which does not match the actual function namegetFinalPriceList. Additionally, there's a typo in "GetFina"—it should be "GetFinal".Please correct these typos and update the function comment to accurately reflect the function's name and purpose.
x/oracle/keeper/aggregator/context.go (1)
163-167: Consider removing commented-out code for clarity.The code block at lines 163-167 is commented out. If this code is no longer needed, it's better to remove it to improve code readability and maintainability.
x/oracle/module.go (4)
164-164: File is not formatted according togofumptThe static analysis tool indicates that the file is not
gofumpt-ed. Please format the code usinggofumptto ensure consistency and adherence to Go formatting standards.🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 164-164:
File is notgofumpt-ed (gofumpt)
212-212: Correct typos in commentThe comment contains typos: "feedeerID" should be "feederID", and "srouceID" should be "sourceID".
216-216: Remove outdated TODO commentThe TODO comment states that malicious prices should cause immediate slashing and jailing, which is implemented in the code below. Remove the comment to prevent confusion.
339-339: Fix typo in log messageThe log message contains a typo: "missed repoerting price" should be "missed reporting price".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/params.pb.gois excluded by!**/*.pb.gox/oracle/types/slashing.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
- app/app.go (1 hunks)
- proto/exocore/oracle/v1/params.proto (3 hunks)
- proto/exocore/oracle/v1/slashing.proto (1 hunks)
- testutil/keeper/oracle.go (2 hunks)
- x/delegation/keeper/abci.go (1 hunks)
- x/delegation/keeper/delegation_state.go (1 hunks)
- x/operator/keeper/slash.go (2 hunks)
- x/operator/keeper/usd_value.go (0 hunks)
- x/oracle/keeper/aggregator/aggregator.go (4 hunks)
- x/oracle/keeper/aggregator/calculator.go (0 hunks)
- x/oracle/keeper/aggregator/context.go (5 hunks)
- x/oracle/keeper/aggregator/worker.go (3 hunks)
- x/oracle/keeper/common/expected_keepers.go (2 hunks)
- x/oracle/keeper/keeper.go (3 hunks)
- x/oracle/keeper/slashing.go (1 hunks)
- x/oracle/module.go (4 hunks)
- x/oracle/types/events.go (2 hunks)
- x/oracle/types/expected_keepers.go (2 hunks)
- x/oracle/types/key_slashing.go (1 hunks)
- x/oracle/types/types.go (1 hunks)
💤 Files with no reviewable changes (2)
- x/operator/keeper/usd_value.go
- x/oracle/keeper/aggregator/calculator.go
✅ Files skipped from review due to trivial changes (1)
- x/delegation/keeper/delegation_state.go
🧰 Additional context used
🪛 GitHub Check: lint
proto/exocore/oracle/v1/params.proto
[failure] 8-8:
Import "google/protobuf/timestamp.proto" is unused.
[failure] 9-9:
Import "cosmos_proto/cosmos.proto" is unused.proto/exocore/oracle/v1/slashing.proto
[failure] 9-9:
Field name "startHeight" should be lower_snake_case, such as "start_height".
🪛 buf
proto/exocore/oracle/v1/slashing.proto
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
9-9: Field name "startHeight" should be lower_snake_case, such as "start_height".
(FIELD_LOWER_SNAKE_CASE)
🪛 GitHub Check: Run golangci-lint
x/oracle/keeper/aggregator/context.go
[failure] 214-214:
File is notgofumpt-ed (gofumpt)x/oracle/keeper/aggregator/worker.go
[failure] 41-41:
funcrecordMsg.hasis unused (unused)
[failure] 47-47:
S1005: unnecessary assignment to the blank identifier (gosimple)x/oracle/keeper/common/expected_keepers.go
[failure] 4-4:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 5-5:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)x/oracle/module.go
[failure] 164-164:
File is notgofumpt-ed (gofumpt)
🔇 Additional comments (29)
proto/exocore/oracle/v1/slashing.proto (3)
1-5: LGTM: File structure and package declaration are correct.The syntax, package declaration, and go_package option are all properly set. The file structure follows common conventions for Protocol Buffers files in Go projects.
🧰 Tools
🪛 buf
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
7-7: LGTM: Message definition is appropriate.The
ValidatorReportInfomessage is well-named and follows the correct naming convention. It aligns with the PR objective of introducing slashing capabilities for the oracle service.
1-12: Summary: The new Protocol Buffers file is well-structured and aligns with PR objectives.The
slashing.protofile introduces a newValidatorReportInfomessage type, which is crucial for implementing the slashing mechanism in the oracle service. The file structure, syntax, and package declarations are correct. A minor naming convention issue was identified and a suggestion for improvement was provided.Overall, this new file contributes positively to the PR's goal of introducing slashing capabilities to the oracle service.
🧰 Tools
🪛 buf
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
9-9: Field name "startHeight" should be lower_snake_case, such as "start_height".
(FIELD_LOWER_SNAKE_CASE)
🪛 GitHub Check: lint
[failure] 9-9:
Field name "startHeight" should be lower_snake_case, such as "start_height".x/oracle/types/key_slashing.go (2)
7-11: LGTM: Well-structured prefix declarations.The prefix declarations are clear, efficient, and follow good naming conventions. The use of
appendfor creating composite prefixes is appropriate.
1-24: Overall assessment: Well-implemented slashing key generation.This new file introduces a solid foundation for managing slashing-related data in the oracle service. The key generation functions are well-designed and efficient. However, there are a few minor points to address:
- Remove the commented-out constant declaration at the beginning of the file.
- Ensure that
DelimiterForCombinedKeyandUint64Bytesare properly defined and imported.Once these minor issues are addressed, the implementation will be robust and ready for integration into the larger system.
To ensure the completeness of the slashing implementation, let's verify the integration of this new functionality with the oracle keeper:
#!/bin/bash # Description: Verify the integration of slashing functionality with the oracle keeper # Test 1: Search for SlashingKeeper in the OracleKeeper struct echo "Searching for SlashingKeeper in the OracleKeeper struct:" rg -A 5 "type OracleKeeper struct" | rg "SlashingKeeper" # Test 2: Search for usage of slashing functions in the oracle module echo "Searching for usage of slashing functions in the oracle module:" rg -A 5 "SlashingValidatorReportInfoKey|SlashingMissedBitArrayPrefix|SlashingMissedBitArrayKey" x/oraclePlease ensure that the slashing functionality is properly integrated with the oracle keeper and used appropriately in the oracle module.
x/oracle/types/types.go (2)
Line range hint
1-58: Overall assessment of changes intypes.goThe additions to this file, namely the
AggFinalPricestruct andDelimiterForCombinedKeyvariable, appear to enhance the oracle system's functionality, particularly in aggregating final prices. These changes are consistent with the existing code style and practices.Key points:
- The
AggFinalPricestruct provides a clear structure for representing aggregated final prices.- The
DelimiterForCombinedKeyvariable introduces a standard delimiter for combined keys.To further improve the code:
- Add comments to explain the purpose and usage of new elements.
- Verify that the chosen delimiter doesn't conflict with file paths or URLs in the broader context of the application.
These changes align well with the PR objective of introducing slashing for the oracle service, as they provide necessary data structures for price aggregation, which could be used in the slashing mechanism.
51-51: Add a comment forDelimiterForCombinedKeyand consider potential conflicts.The purpose and usage of this delimiter should be clarified with a comment. Additionally, consider if using '/' might cause any conflicts, especially if the combined keys are used in file paths or URLs.
Here's a suggested improvement:
+// DelimiterForCombinedKey is used to separate parts in combined keys. +// Note: Ensure this doesn't conflict with file path or URL usage. var DelimiterForCombinedKey = byte('/')To verify potential conflicts, let's search for uses of this delimiter:
✅ Verification successful
Add a descriptive comment for
DelimiterForCombinedKey. No potential conflicts with file paths or URLs detected.
- Add a comment in
x/oracle/types/types.goat line 51 to clarify the purpose and usage ofDelimiterForCombinedKey.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for uses of DelimiterForCombinedKey and potential conflicts with file paths or URLs # Test 1: Find direct uses of DelimiterForCombinedKey echo "Direct uses of DelimiterForCombinedKey:" rg "DelimiterForCombinedKey" -A 3 # Test 2: Look for potential conflicts with file paths echo "\nPotential conflicts with file paths:" rg "filepath\.Join|os\.Open|ioutil\.ReadFile" -A 3 # Test 3: Look for potential conflicts with URLs echo "\nPotential conflicts with URLs:" rg "url\.Parse|http\.Get" -A 3Length of output: 7339
x/oracle/types/expected_keepers.go (1)
Line range hint
1-34: Overall, the changes look good and align with the PR objectives.The introduction of the
SlashingKeeperinterface in this file is a crucial part of implementing the slashing mechanism for the oracle service. These changes, combined with the modifications mentioned in the AI-generated summary (such as updates to theOracleKeeperandKeeperstructs in other files), create a solid foundation for integrating slashing capabilities into the oracle system.To ensure the completeness of this implementation:
Verify that the
SlashingKeeperis properly initialized and passed to theOracleKeeper:Confirm that the
JailUntilmethod is called in appropriate places within the oracle module:These verifications will help ensure that the slashing mechanism is properly integrated throughout the oracle module.
x/oracle/types/events.go (3)
4-4: LGTM: Improved code formatting.The reformatting of
EventTypeCreatePriceenhances code consistency.
32-32: LGTM: New attribute value for missing price reports.The addition of
AttributeValueMissingReportPriceprovides a clear identifier for events related to missing price reports in the oracle service.
Line range hint
1-33: Overall assessment: Good additions with one minor issue.The new constants for event types and attribute keys/values align well with the PR objectives of introducing slashing for the oracle service. They provide a comprehensive set of identifiers for events related to oracle liveness and slashing actions.
However, please address the issue with the
AttributeValueMaliciousReportPriceconstant value before merging this PR.Additionally, consider adding brief inline comments for the new constants to improve code documentation, and grouping related constants for better organization.
x/oracle/keeper/keeper.go (3)
43-43: LGTM: SlashingKeeper parameter added correctly.The addition of the
slashingKeeperparameter to theNewKeeperfunction is consistent with the new field in theKeeperstruct and follows good dependency injection practices.
63-63: LGTM: SlashingKeeper assigned correctly.The assignment of the
slashingKeeperparameter to theSlashingKeeperfield in theKeeperstruct initialization is correct and completes the integration of slashing capabilities.
Line range hint
28-63: Summary: Slashing integration looks good, verify dependencies.The changes to integrate slashing capabilities into the oracle keeper are well-implemented and consistent. The new
SlashingKeeperfield, parameter, and assignment are correctly added.To ensure all dependencies are correctly set up, please run the following command:
testutil/keeper/oracle.go (1)
24-24: LGTM: Slashing keeper import added correctly.The new import for the slashing keeper is appropriately placed and follows the existing import structure. This addition aligns with the PR objective of introducing slashing for the oracle service.
x/oracle/keeper/common/expected_keepers.go (3)
57-57: LGTM: Consistent type name update.The change from
stakingTypes.ValidatorItostakingtypes.ValidatorIis consistent with the import rename and improves code consistency.
59-59: LGTM: Consistent type name update.The change from
stakingTypes.Validatortostakingtypes.Validatoris consistent with the import rename and improves code consistency.
62-64: New methods align with PR objectives, but clarification needed.The new methods
ValidatorByConsAddr,SlashWithInfractionReason, andJailare well-aligned with the PR objectives of introducing slashing mechanisms. They enhance the interface's capabilities for validator management and penalization.Could you please clarify the difference between the new
ValidatorByConsAddrmethod and the existingGetValidatorByConsAddrmethod? They seem to serve similar purposes but return different types (stakingtypes.ValidatorIvsstakingtypes.Validator, bool).To help verify the usage and necessity of both methods, please run the following script:
This will help us understand if both methods are necessary and how they are used in the codebase.
✅ Verification successful
Validator methods usage confirmed and appropriate.
The
ValidatorByConsAddrandGetValidatorByConsAddrmethods serve distinct purposes, with the former providing an interface type for flexibility and the latter offering concrete retrieval with existence checking. Their usage across the codebase justifies their coexistence and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of both ValidatorByConsAddr and GetValidatorByConsAddr methods echo "Usages of ValidatorByConsAddr:" rg "ValidatorByConsAddr\(" -A 3 echo "\nUsages of GetValidatorByConsAddr:" rg "GetValidatorByConsAddr\(" -A 3Length of output: 6235
x/operator/keeper/slash.go (1)
Line range hint
1-269: Overall, the changes improve documentation without altering core logic.The modifications in this file are focused on enhancing documentation and planning for future improvements:
- The updated comment for
SlashFromUndelegationprovides better clarity on the function's behavior.- The new TODO comment in
SlashAssetsidentifies an important edge case for future optimization.These changes demonstrate attention to detail and forward-thinking about the slashing mechanism. They don't introduce immediate risks or alterations to the existing logic but pave the way for future enhancements.
To maintain this momentum:
- Consider reviewing other functions in this file for similar documentation improvements.
- Ensure that the TODO item is properly tracked and addressed in future development cycles.
Great job on continuously improving the codebase!
proto/exocore/oracle/v1/params.proto (2)
39-40: Integration of slashing parameters intoParamsmessage.Adding
SlashingParams slashing = 12;appropriately extends theParamsmessage to include slashing-related configurations.
53-86: Definition ofSlashingParamsmessage is well-structured.The
SlashingParamsmessage is correctly defined with appropriate field types and options to support the slashing functionality.x/oracle/keeper/aggregator/context.go (6)
7-7: Importing 'sort' package is appropriate.The addition of
"sort"to the imports is necessary for the sorting operations introduced in the code.
168-173: Logic for handling sealedfeederWorkeris correctly implemented.The updated code properly records messages for performance evaluation when the
feederWorkeris sealed before returning the appropriate error.
213-220: Feeder IDs are correctly sorted before processing.Sorting
feederIDsensures that the IDs are accessed in a consistent order, which is important for slashing calculations.🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 214-214:
File is notgofumpt-ed (gofumpt)
231-236: Properly record when rounds reach the end of their window.Appending
feederIDtowindowClosedwhen a round reaches theMaxNonceoffset aids in tracking and handling slashing operations effectively.
259-259: Return statement inSealRoundupdated appropriately.Including
windowClosedin the return statement aligns with the updated function signature and ensures all relevant data is returned.
369-380: New methodGetFinalPriceListForFeederIDsis implemented correctly.The function efficiently retrieves final price lists for the specified feeder IDs, ensuring the results are ordered as required.
x/oracle/module.go (2)
27-27: Import statement for staking typesThe import of
stakingtypesis necessary for slashing functionality and is appropriately added.
183-188: Proper initialization of report info for validator updatesThe code correctly initializes
ValidatorReportInfofor validators with positive power, ensuring proper tracking for slashing purposes.
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (23)
proto/exocore/oracle/v1/slashing.proto (1)
7-12: LGTM: Well-structured ValidatorReportInfo message.The ValidatorReportInfo message is well-defined with appropriate field types and unique tag numbers.
Consider adding comments to describe the purpose of each field, especially for fields like
index_offsetandmissed_rounds_counter, to improve code readability and maintainability.🧰 Tools
🪛 GitHub Check: lint
[failure] 9-9:
Field name "startHeight" should be lower_snake_case, such as "start_height".🪛 buf
9-9: Field name "startHeight" should be lower_snake_case, such as "start_height".
(FIELD_LOWER_SNAKE_CASE)
x/oracle/types/key_slashing.go (2)
1-6: Remove commented-out code.The commented-out constant definition for
SlashingPrefixis no longer needed as it has been replaced by a variable declaration. Remove this commented-out code to improve code cleanliness.-// const ( -// SlashingPrefix = "Slashing/" -// )
17-20: Clarify the purpose ofDelimiterForCombinedKey.The function is well-implemented, but the use of
DelimiterForCombinedKeyis not explained. Consider adding a comment to clarify its purpose in the key construction.x/oracle/types/types.go (3)
39-44: Consider adding comments and reviewing the Price field type.The new
AggFinalPricestruct looks good overall, but consider the following suggestions:
Add comments for each field to explain their purpose and any constraints. This will improve code maintainability and make it easier for other developers to understand and use the struct correctly.
The
Pricefield is currently a string. While this allows for high precision, it may require parsing for numerical operations. Consider using a numeric type (e.g.,sdkmath.Intordecimal.Decimal) if precise calculations are needed, or add a comment explaining why a string is preferred.Example with comments:
type AggFinalPrice struct { FeederID uint64 // Unique identifier for the price feeder SourceID uint64 // Identifier for the price source DetID string // Deterministic ID for the price (explain its purpose) Price string // Price value as a string (explain precision/format) }
51-51: Add a comment explaining the usage of DelimiterForCombinedKey.The addition of
DelimiterForCombinedKeyis a good practice for maintaining consistency in key generation. However, its purpose and usage are not immediately clear from the context.Consider adding a comment to explain where and how this delimiter is used. For example:
// DelimiterForCombinedKey is used to separate components when creating composite keys // in the oracle module's key-value store. var DelimiterForCombinedKey = byte('/')This will help other developers understand the purpose of this variable and use it correctly.
Line range hint
1-38: Remove commented-out code and update documentation.There are several commented-out fields in the
OracleInfostruct, specifically within theTokenandFeedersub-structs. While these comments might be temporary during development, it's generally better to remove unused code rather than commenting it out. This helps maintain a clean and clear codebase.Consider the following actions:
- If these fields are no longer needed, remove them entirely instead of keeping them commented out.
- If there's a reason to keep track of these removed fields, consider adding a comment explaining why they were removed and when they might be needed again.
- Update any related documentation or comments to reflect the current structure of the
OracleInfostruct.Example:
type OracleInfo struct { // ... (other fields) Token struct { Name string `json:"name"` Desc string Decimal string `json:"decimal"` Contract string `json:"contract"` AssetID string `json:"asset_id"` // Removed: Chain struct - no longer needed as of <date/PR> } `json:"token"` Feeder struct { Interval string `json:"interval"` // Removed: Start, End, RuleID fields - replaced by <new mechanism> as of <date/PR> } `json:"feeder"` // ... (other fields) }x/oracle/types/expected_keepers.go (3)
4-4: Remove the alias for thetimepackage import.The
timepackage is a standard Go package and doesn't require an alias. Using an alias for standard packages is not a common practice and may lead to confusion.Consider changing the import statement to:
-time "time" +import "time"
32-34: LGTM! Consider adding documentation for the SlashingKeeper interface.The
SlashingKeeperinterface is well-defined and consistent with the codebase conventions. TheJailUntilmethod signature is clear and uses appropriate types.To improve code maintainability, consider adding a brief comment explaining the purpose of the
SlashingKeeperinterface and theJailUntilmethod. For example:// SlashingKeeper defines the expected slashing keeper used for oracle operations type SlashingKeeper interface { // JailUntil jails a validator until a specified time JailUntil(sdk.Context, sdk.ConsAddress, time.Time) }
Action Required: Integrate
SlashingKeeperinto OracleKeeperThe
SlashingKeeperinterface is defined but not integrated into theOracleKeeperstruct or initialized in theNewKeeperfunction. Please ensure thatSlashingKeeperis properly incorporated to enable its intended functionality within the Oracle module.
- Update
OracleKeeperto includeSlashingKeeper.- Initialize
SlashingKeeperin theNewKeeperfunction.🔗 Analysis chain
Line range hint
1-34: Summary: SlashingKeeper integration enhances oracle service capabilities.The introduction of the
SlashingKeeperinterface aligns well with the PR objectives of implementing slashing mechanisms for the oracle service. This addition allows for better integration between the oracle and slashing modules, enabling more sophisticated validator management.Key points:
- The new
SlashingKeeperinterface provides a method for jailing validators until a specified time.- The changes are minimal and non-disruptive to existing code.
- The integration enhances the system's ability to penalize misbehaving validators in the oracle service.
To ensure proper integration, please run the following script to check for consistent usage of the new
SlashingKeeperinterface across the codebase:
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of SlashingKeeper in relevant files # Test 1: Check for SlashingKeeper in OracleKeeper echo "Checking OracleKeeper for SlashingKeeper:" rg -p "type OracleKeeper struct" -A 10 | rg "SlashingKeeper" # Test 2: Check for SlashingKeeper initialization in NewKeeper echo "Checking NewKeeper for SlashingKeeper initialization:" rg "func NewKeeper\(" -A 15 | rg "SlashingKeeper:" # Test 3: Check for JailUntil method calls echo "Checking for JailUntil method calls:" rg "JailUntil\("Length of output: 650
Script:
#!/bin/bash # Description: Search for any implementations or references of SlashingKeeper # Test 4: Search for implementations of SlashingKeeper interface echo "Searching for implementations of SlashingKeeper:" rg "type .*SlashingKeeper" --type go # Test 5: Search for references to SlashingKeeper in function parameters echo "Searching for function parameters using SlashingKeeper:" rg "SlashingKeeper" --type go | rg "\("Length of output: 721
x/oracle/types/events.go (2)
19-25: LGTM: New attribute keys for slashing-related data.The new attribute key constants (
AttributeKeyValidatorKey,AttributeKeyMissedRounds,AttributeKeyHeight,AttributeKeyPower,AttributeKeyReason,AttributeKeyJailed,AttributeKeyBurnedCoins) are well-defined and consistent with the existing naming convention. They provide necessary keys for slashing-related information.Consider renaming
AttributeKeyValidatorKeytoAttributeKeyValidatorfor consistency with other attribute keys that don't include the word "Key" in their names.
Issue Found: Duplicate Attribute Value Assignment
The constant
AttributeValueMaliciousReportPriceis incorrectly assigned the value"missing_report_price". It should likely be"malicious_report_price".
- File: x/oracle/types/events.go
- Line:
AttributeValueMaliciousReportPrice = "missing_report_price"🔗 Analysis chain
Line range hint
1-33: Summary: Comprehensive addition of constants for oracle slashing mechanism.The changes in this file provide a solid foundation for implementing the oracle slashing mechanism. The new constants cover event types, attribute keys, and attribute values necessary for tracking oracle liveness and applying slashes. These additions will enable detailed event logging and data management for the slashing feature.
To ensure these constants are being utilized correctly, please run the following script to check their usage across the codebase:
This will help ensure that the new constants are being properly integrated into the slashing logic throughout the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new constants in the codebase. echo "Checking usage of new event types:" rg "EventTypeOracleLiveness|EventTypeOracleSlash" --type go echo "\nChecking usage of new attribute keys:" rg "AttributeKeyValidatorKey|AttributeKeyMissedRounds|AttributeKeyHeight|AttributeKeyPower|AttributeKeyReason|AttributeKeyJailed|AttributeKeyBurnedCoins" --type go echo "\nChecking usage of new attribute values:" rg "AttributeValueMissingReportPrice|AttributeValueMaliciousReportPrice" --type goLength of output: 3116
x/oracle/keeper/keeper.go (1)
28-28: LGTM. Consider adding a comment for the new field.The addition of
SlashingKeeperaligns well with the PR objective of introducing slashing for the oracle service. This change enables theKeeperto interact with slashing functionality.Consider adding a brief comment explaining the purpose of this field:
// SlashingKeeper handles slashing operations for oracle validators SlashingKeeper types.SlashingKeepertestutil/keeper/oracle.go (2)
56-56: Consider initializing SlashingKeeper with necessary dependencies.The addition of
slashingkeeper.Keeper{}as an argument toNewKeeperis correct and aligns with the PR objectives. However, for more robust testing, consider initializing theSlashingKeeperwith its necessary dependencies instead of passing an empty struct.You might want to create a mock or properly initialized
SlashingKeeperfor testing purposes. For example:slashingKeeper := slashingkeeper.NewKeeper( cdc, storeKey, &stakingKeeper, authtypes.NewModuleAddress(govtypes.ModuleName).String(), )Replace
storeKeyandstakingKeeperwith appropriate values for your test environment.
Line range hint
1-70: Summary: Integration of SlashingKeeper into OracleKeeper test utilityThe changes in this file successfully integrate the
SlashingKeeperinto theOracleKeepertest utility. This update aligns with the PR's objective of introducing slashing mechanisms for the oracle service. The modifications are minimal yet impactful, setting the groundwork for testing the new slashing functionality within the oracle module.To further enhance the test utility:
- Consider initializing the
SlashingKeeperwith proper dependencies for more comprehensive testing.- Update any existing tests that use this utility to ensure they account for the new slashing functionality.
- Add new tests specifically targeting the interaction between the
OracleKeeperandSlashingKeeper.These enhancements will help ensure the robustness and reliability of the new slashing mechanisms in the oracle service.
x/oracle/keeper/aggregator/aggregator.go (1)
128-128: LGTM: Efficiency improvement.The addition of the
breakstatement is a good optimization. It prevents unnecessary iterations once a matching price is found.Consider moving the entire price-finding logic into a separate helper function for improved readability and maintainability.
x/operator/keeper/slash.go (2)
139-139: Consider creating an issue for the proposed enhancement.The TODO comment suggests an important consideration for handling edge cases when all assets fall to zero. To ensure this doesn't get overlooked:
- Create a GitHub issue to track this enhancement.
- In the issue, detail the proposed checks and actions:
- Check if
pendingUndelegationis also zero- Delete the item if both are zero
- Opt out the operator if all assets fall to zero
This will help in prioritizing and implementing this improvement in the future.
Would you like me to create a draft for this GitHub issue?
Add Comprehensive Tests for Slashing Edge Cases
The current test suite does not cover critical slashing scenarios such as varying slashing proportions, slashing when assets are near zero, multiple consecutive slashes, and interactions between slashing and undelegation. It's essential to implement tests for these cases to ensure the reliability and robustness of the slashing mechanism.
- Implement tests for slashing with various proportions.
- Create scenarios where assets are close to zero and validate slashing behavior.
- Test multiple consecutive slashes to ensure system stability.
- Verify the interaction between slashing and undelegation processes.
🔗 Analysis chain
Line range hint
1-285: Overall assessment of slashing mechanism.The changes in this file are minor and improve documentation without altering core functionality. However, given the critical nature of slashing operations, it's crucial to ensure comprehensive testing of the entire slashing mechanism, including edge cases such as:
- Slashing with various proportions
- Slashing when assets are close to zero
- Multiple consecutive slashes
- Interaction between slashing and undelegation processes
Consider adding or updating integration tests to cover these scenarios thoroughly.
To assist in verifying the slashing mechanism, you can run the following script to identify existing tests related to slashing:
This script will help identify existing test coverage and areas where additional tests might be needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find existing test files related to slashing echo "Searching for test files related to slashing..." fd -e go -e sh test | rg -i 'slash|undelegat|jail' echo "Searching for test functions related to slashing within Go files..." rg -t go '(?i)func.*test.*(?:slash|undelegat|jail)' -g '*_test.go'Length of output: 1992
Script:
#!/bin/bash # Description: Check if tests cover slashing with various proportions echo "Searching for tests covering slashing with various proportions..." rg -i 'test.*slash.*proportion' $(fd -e go -e sh test) echo "Searching for tests covering slashing when assets are close to zero..." rg -i 'test.*slash.*zero|close to zero|minimum' $(fd -e go -e sh test) echo "Searching for tests covering multiple consecutive slashes..." rg -i 'test.*slash.*multiple|consecutive|repeated' $(fd -e go -e sh test) echo "Searching for tests covering interaction between slashing and undelegation..." rg -i 'test.*slash.*undelegation.*interaction|integration|combined' $(fd -e go -e sh test)Length of output: 23366
proto/exocore/oracle/v1/params.proto (1)
39-39: Correct the grammatical error in the comment for clarity.Update the comment to improve readability and grammar. It should read:
Slashing defines the slashing related params.Apply this diff to correct the comment:
-// slashing defined the slashing related params +// Slashing defines the slashing related paramsx/oracle/keeper/aggregator/worker.go (1)
47-49: Simplify assignment ingetmethodThe assignment to the blank identifier
_is unnecessary. You can simplify the code by directly retrieving the value.Apply this diff to streamline the code:
-func (r recordMsg) get(validator string, feederID, sourceID uint64) []*types.PriceTimeDetID { - v, _ := r[recordKey{validator, feederID, sourceID}] +func (r recordMsg) get(validator string, feederID, sourceID uint64) []*types.PriceTimeDetID { + v := r[recordKey{validator, feederID, sourceID}] return v }🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 47-47:
S1005: unnecessary assignment to the blank identifier (gosimple)x/oracle/keeper/aggregator/context.go (3)
384-390: Log a warning whenfeederWorkerisnilinPerformanceReviewIn the
PerformanceReviewfunction, iffeederWorkerisnil, the code treats the validator as effective and honest by default. Since this scenario "should not happen," it would be helpful to log a warning or error message to aid in debugging and to make potential issues more visible.Apply this diff to add a log message:
if feederWorker == nil { + ctx.Logger().Warn("feederWorker is nil in PerformanceReview", "feederID", finalPrice.FeederID) // something wrong happened in the agc, so we just skip this round by treat the validator as effective&honest exist = true matched = true
218-220: Handle potential largefeederIDsslices efficientlyWhen sorting large slices of
feederIDs, consider the efficiency of the sorting algorithm. The current use ofsort.Sliceis appropriate, but iffeederIDscan become very large, investigate using more efficient sorting methods or data structures to optimize performance.
Ensure all
SealRoundcalls handlewindowClosedreturn valueThe following usages of
SealRounddo not handle the newwindowClosedreturn value:
x/oracle/keeper/single.gox/oracle/keeper/aggregator/context_test.goPlease update these call sites to handle
windowClosedto maintain consistency and prevent potential issues.🔗 Analysis chain
Line range hint
212-259: Ensure all calls toSealRoundhandle the new return valuewindowClosedThe
SealRoundmethod's signature has been updated to include an additional return valuewindowClosed []uint64. Verify that all calls to this method throughout the codebase are updated to handle this new return value to prevent potential issues.Run the following script to find all usages of
SealRound:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to SealRound to ensure they're updated. # Search for SealRound method calls rg 'SealRound\('Length of output: 573
🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 214-214:
File is notgofumpt-ed (gofumpt)x/oracle/module.go (1)
340-340: Fix typographical error in log messageThere's a typo in the log message: "repoerting" should be "reporting".
Suggested change:
"validator would have been slashed for too many missed repoerting price, but was either not found in store or already jailed", + "validator would have been slashed for too many missed reporting price, but was either not found in store or already jailed",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/params.pb.gois excluded by!**/*.pb.gox/oracle/types/slashing.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
- app/app.go (1 hunks)
- proto/exocore/oracle/v1/params.proto (3 hunks)
- proto/exocore/oracle/v1/slashing.proto (1 hunks)
- testutil/keeper/oracle.go (2 hunks)
- x/delegation/keeper/abci.go (1 hunks)
- x/delegation/keeper/delegation_state.go (1 hunks)
- x/operator/keeper/slash.go (2 hunks)
- x/operator/keeper/usd_value.go (0 hunks)
- x/oracle/keeper/aggregator/aggregator.go (4 hunks)
- x/oracle/keeper/aggregator/calculator.go (0 hunks)
- x/oracle/keeper/aggregator/context.go (5 hunks)
- x/oracle/keeper/aggregator/worker.go (3 hunks)
- x/oracle/keeper/common/expected_keepers.go (2 hunks)
- x/oracle/keeper/keeper.go (3 hunks)
- x/oracle/keeper/slashing.go (1 hunks)
- x/oracle/module.go (4 hunks)
- x/oracle/types/events.go (2 hunks)
- x/oracle/types/expected_keepers.go (2 hunks)
- x/oracle/types/key_slashing.go (1 hunks)
- x/oracle/types/types.go (1 hunks)
💤 Files with no reviewable changes (2)
- x/operator/keeper/usd_value.go
- x/oracle/keeper/aggregator/calculator.go
✅ Files skipped from review due to trivial changes (2)
- x/delegation/keeper/abci.go
- x/delegation/keeper/delegation_state.go
🧰 Additional context used
🪛 GitHub Check: lint
proto/exocore/oracle/v1/params.proto
[failure] 8-8:
Import "google/protobuf/timestamp.proto" is unused.
[failure] 9-9:
Import "cosmos_proto/cosmos.proto" is unused.proto/exocore/oracle/v1/slashing.proto
[failure] 9-9:
Field name "startHeight" should be lower_snake_case, such as "start_height".
🪛 buf
proto/exocore/oracle/v1/slashing.proto
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
9-9: Field name "startHeight" should be lower_snake_case, such as "start_height".
(FIELD_LOWER_SNAKE_CASE)
🪛 GitHub Check: Run golangci-lint
x/oracle/keeper/aggregator/context.go
[failure] 214-214:
File is notgofumpt-ed (gofumpt)x/oracle/keeper/aggregator/worker.go
[failure] 41-41:
funcrecordMsg.hasis unused (unused)
[failure] 47-47:
S1005: unnecessary assignment to the blank identifier (gosimple)x/oracle/keeper/common/expected_keepers.go
[failure] 4-4:
ST1019: package "cosmossdk.io/math" is being imported more than once (stylecheck)
[failure] 5-5:
ST1019(related information): other import of "cosmossdk.io/math" (stylecheck)x/oracle/module.go
[failure] 164-164:
File is notgofumpt-ed (gofumpt)
🪛 GitHub Check: CodeQL
x/oracle/module.go
[warning] 164-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 205-347: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 229-229: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 302-302: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🔇 Additional comments (25)
proto/exocore/oracle/v1/slashing.proto (3)
1-3: LGTM: Proper syntax and package declaration.The file correctly uses proto3 syntax and declares an appropriate package name that aligns with the file path.
🧰 Tools
🪛 buf
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
5-5: LGTM: Correct Go package option.The go_package option is properly set to point to the correct GitHub repository and package within the project, following best practices for Go module organization.
1-12: Verify directory structure convention for proto files.The static analysis tool flagged that files with package "exocore.oracle.v1" should be within a directory "exocore/oracle/v1" relative to root, but this file is in "proto/exocore/oracle/v1".
Please confirm if the current directory structure ("proto/exocore/oracle/v1") is intentional and aligns with the project's conventions. If not, consider moving the file to match the package name exactly.
To verify the current structure, run:
✅ Verification successful
Directory structure for proto files adheres to project conventions.
All
.protofiles are correctly located within theproto/directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the directory structure for proto files # Test: List all .proto files in the project fd -e proto # Test: Check if there's a consistent 'proto' directory for all .proto files fd -e proto | grep -v '^proto/' && echo "Some .proto files are not in the 'proto' directory"Length of output: 4201
🧰 Tools
🪛 GitHub Check: lint
[failure] 9-9:
Field name "startHeight" should be lower_snake_case, such as "start_height".🪛 buf
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
9-9: Field name "startHeight" should be lower_snake_case, such as "start_height".
(FIELD_LOWER_SNAKE_CASE)
x/oracle/types/key_slashing.go (4)
7-11: LGTM: Well-structured prefix declarations.The prefix declarations are well-organized and efficiently implemented using byte slices. The hierarchy of prefixes is clear, with
ValidatorReportInfoPrefixandMissedBitArrayPrefixbuilding upon the baseSlashingPrefix.
13-15: LGTM: Clear and concise key generation.The
SlashingValidatorReportInfoKeyfunction is well-implemented, efficiently combining the prefix with the validator identifier.
1-24: Overall: Well-structured and efficient key slashing implementation.The file introduces a clear and efficient implementation of key slashing functionality. The prefix hierarchy and key generation functions are well-designed and follow consistent naming conventions. Minor improvements can be made to enhance code cleanliness and clarity, but the core functionality is solid.
22-24: Verify the existence and import ofUint64Bytesfunction.The
SlashingMissedBitArrayKeyfunction uses an undefinedUint64Bytesfunction. Ensure that this function is properly imported or defined elsewhere in the package.x/oracle/types/types.go (1)
Line range hint
1-58: Summary of changes and potential impactThe changes in this file appear to be part of a larger refactoring or feature addition related to oracle price aggregation. Here's a summary of the main changes and their potential impact:
The new
AggFinalPricestruct introduces a way to represent aggregated final prices, which is likely used in other parts of the codebase for managing price data from multiple sources.The
DelimiterForCombinedKeyvariable suggests a standardized approach to creating composite keys, potentially improving consistency in data storage and retrieval.The simplification of the
OracleInfostruct (by removing some fields) indicates a possible streamlining of the oracle data model.These changes may affect other parts of the oracle module, such as price aggregation logic, data storage, and retrieval mechanisms. Ensure that these changes are consistent with the overall design of the oracle module and that any dependent code is updated accordingly.
Consider the following to ensure smooth integration of these changes:
- Update any code that uses the
OracleInfostruct to reflect the removed fields.- Implement proper error handling and validation for the new
AggFinalPricestruct, especially for thePricefield which is stored as a string.- Document the usage of
DelimiterForCombinedKeyin key generation functions to maintain consistency across the module.- Update unit tests to cover the new
AggFinalPricestruct and any modified functionality related toOracleInfo.To ensure these changes are properly integrated, you may want to run the following checks:
These commands will help identify any areas that might need additional updates or attention due to the changes in this file.
x/oracle/types/events.go (1)
4-6: LGTM: New event types for oracle liveness and slashing.The addition of
EventTypeOracleLivenessandEventTypeOracleSlashaligns well with the PR's objective of introducing slashing for the oracle service. The naming convention is consistent with the existingEventTypeCreatePrice.x/oracle/keeper/keeper.go (1)
Line range hint
1-76: Overall, 6D47 the changes look good and align with the PR objectives.The integration of
SlashingKeeperinto theKeeperstruct and its constructor is well-implemented. These changes enable the oracle service to interact with slashing functionality, which is crucial for the PR's goal of introducing slashing mechanisms.A few minor suggestions:
- Consider adding a brief comment for the new
SlashingKeeperfield in theKeeperstruct.- Verify the proper initialization of
OracleKeeperinapp/app.goto ensure theSlashingKeeperis correctly passed.These changes lay a solid foundation for implementing the slashing logic in the oracle service. Great job!
testutil/keeper/oracle.go (1)
24-24: LGTM: New import for slashingkeeper.The addition of the
slashingkeeperimport is correct and necessary for the changes in theNewKeeperfunction call.x/oracle/keeper/common/expected_keepers.go (2)
57-59: LGTM: Type reference updates.The changes to the method signatures in the
KeeperDogfoodinterface correctly reflect the rename of thestakingtypespackage. This ensures consistency with the updated import statement.
62-64: Approve new methods and request clarification.The new methods
ValidatorByConsAddr,SlashWithInfractionReason, andJailenhance theKeeperDogfoodinterface, aligning with the PR objectives of introducing slashing for the oracle service. These additions look good and provide necessary functionality for validator management and slashing.Could you please clarify the difference between the new
ValidatorByConsAddrmethod and the existingGetValidatorByConsAddrmethod? They seem to serve similar purposes but return different types (stakingtypes.ValidatorIvsstakingtypes.Validator). Is this intentional, and if so, what's the rationale behind having both?To help verify the usage and implementation of these new methods, please run the following script:
x/oracle/keeper/aggregator/aggregator.go (4)
5-5: LGTM: Import addition is appropriate.The addition of the
sortpackage import is necessary for the newgetFinalPriceListmethod implementation.
156-156: LGTM: Consistent price update.The addition of
price.price = priceSourceRound.priceensures that the price is updated consistently with other fields when processing a new confirmed price. This change maintains data integrity and aligns with the method's purpose.
Line range hint
1-224: Overall assessment: Good improvements with a minor issue to address.The changes in this file enhance the functionality of the
aggregatorpackage, aligning well with the PR objectives for the oracle service. The newgetFinalPriceListmethod is a valuable addition, but requires a small modification to prevent potential duplicate entries. Other changes, such as the efficiency improvement infillPriceand the consistent price update inconfirmDSPrice, are well-implemented.Please address the issue in the
getFinalPriceListmethod to ensure the reliability of the aggregated price data. Once this is resolved, the changes will significantly improve the oracle service's capabilities.
196-223:⚠️ Potential issueImplementation looks good, but there's a potential issue with duplicates.
The new
getFinalPriceListmethod is a valuable addition that aligns with the PR objectives. It correctly sorts the sourceIDs and constructs a list ofAggFinalPriceobjects. However, there's a potential issue in the loop structure:The current implementation might lead to duplicate entries in the result list. For each sourceID, it iterates through all reports, potentially adding multiple entries for the same sourceID if multiple reports have matching prices.
To fix this, consider the following modification:
func (agg *aggregator) getFinalPriceList(feederID uint64) []*types.AggFinalPrice { sourceIDs := make([]uint64, 0, len(agg.dsPrices)) for sID := range agg.dsPrices { sourceIDs = append(sourceIDs, sID) } sort.Slice(sourceIDs, func(i, j int) bool { return sourceIDs[i] < sourceIDs[j] }) ret := make([]*types.AggFinalPrice, 0, len(sourceIDs)) for _, sID := range sourceIDs { var finalPrice *types.AggFinalPrice for _, report := range agg.reports { price := report.prices[sID] if price == nil || price.detRoundID != agg.dsPrices[sID] { continue } finalPrice = &types.AggFinalPrice{ FeederID: feederID, SourceID: sID, DetID: price.detRoundID, Price: price.price.String(), } break } if finalPrice != nil { ret = append(ret, finalPrice) } } return ret }This modification ensures that only one price entry is added per sourceID, avoiding potential duplicates.
To verify the current behavior and the need for this change, you can run the following script:
x/operator/keeper/slash.go (1)
26-26: Improved function documentation.The updated comment for
SlashFromUndelegationprovides clearer information about the function's behavior, specifically mentioning that it reduces theActualCompletedAmountfromundelegationRecords. This improvement in documentation enhances code readability and maintainability.proto/exocore/oracle/v1/params.proto (1)
62-62:⚠️ Potential issueAdd missing space in field declaration for correct syntax.
There is a missing space between
=and4in the field declaration fororacle_malicious_jail_duration. It should be= 4instead of=4to adhere to proto syntax standards.Apply this diff to fix the syntax error:
-google.protobuf.Duration oracle_malicious_jail_duration =4 +google.protobuf.Duration oracle_malicious_jail_duration = 4Likely invalid or redundant comment.
x/oracle/keeper/slashing.go (3)
86-87: Clarify the note aboutRoundInt64The comment mentions that
RoundInt64will never panic asMinReportedPerWindowis less than 1. However, ifMinReportedPerWindowis less than 1, multiplying it byreportedRoundsWindowmay result in a value less than 1, andRoundInt64could return zero, which might not be the intended behavior.Please verify that the calculation accurately reflects the desired minimum reported rounds, and consider clarifying the comment.
17-17:⚠️ Potential issueFix grammatical error in the comment
There's a grammatical error in the comment. Consider rephrasing.
Apply this diff:
- // set the record for validator to tracking performance of oracle service + // Set the record for the validator to track performance of the oracle serviceLikely invalid or redundant comment.
34-34:⚠️ Potential issueFix typo in function comment
"returns" is misspelled as "retruns".
Apply this diff:
-// GetValidatorReportInfo retruns the ValidatorReportInfo for a specific validator +// GetValidatorReportInfo returns the ValidatorReportInfo for a specific validatorLikely invalid or redundant comment.
x/oracle/keeper/aggregator/context.go (3)
7-7: Approved addition of thesortpackageThe import of the
"sort"package is necessary for sorting operations introduced in this file.
170-173: Confirm correct handling of sealedfeederWorkermessagesWhen
feederWorkeris sealed, messages are recorded for performance evaluation. Ensure that this logic aligns with the intended slashing mechanism and that it does not introduce unintended side effects.
178-179: Ensure accurate performance recording afterfeederWorkerprocessingAfter processing messages with
feederWorker.do(msg), the code records messages for performance evaluation. Verify thatlistFilledcontains the correct data and thatrecordMessageaccurately reflects the validators' contributions for slashing purposes.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (9)
proto/exocore/oracle/v1/params.proto (2)
38-40: LGTM. Consider adding a comment for the new field.The addition of the
slashingfield to theParamsmessage is appropriate for implementing the slashing mechanism. To improve clarity, consider adding a brief comment explaining the purpose of this field.You could add a comment like this:
// slashing defines the parameters for the slashing mechanism in the oracle service SlashingParams slashing = 12;
53-76: LGTM. Consider adding comments for each field.The
SlashingParamsmessage is well-structured and aligns with the PR objectives. The use of custom types and protobuf options is appropriate. To improve clarity and maintainability, consider adding brief comments for each field explaining their purpose and units (if applicable).You could add comments like this:
message SlashingParams { // Number of rounds to consider for slashing calculations int64 reported_rounds_window = 1; // Minimum percentage of rounds that must be reported in the window to avoid slashing bytes min_reported_per_window = 2 [ (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", (gogoproto.nullable) = false, (amino.dont_omitempty) = true ]; // Duration of jail time for missing oracle reports google.protobuf.Duration oracle_miss_jail_duration = 3 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true, (gogoproto.stdduration) = true]; // Duration of jail time for malicious oracle behavior google.protobuf.Duration oracle_malicious_jail_duration = 4 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true, (gogoproto.stdduration) = true]; // Fraction of stake to slash for missing oracle reports bytes slash_fraction_miss = 5 [ (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", (gogoproto.nullable) = false, (amino.dont_omitempty) = true ]; // Fraction of stake to slash for malicious oracle behavior bytes slash_fraction_malicious = 6 [ (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", (gogoproto.nullable) = false, (amino.dont_omitempty) = true ]; }x/oracle/types/genesis.go (1)
38-43: LGTM! Consider adding a comment for better documentation.The
NewMissedRoundfunction is well-implemented and follows Go best practices. It provides a clean way to createMissedRoundinstances, which aligns with the PR's objective of implementing a slashing mechanism.Consider adding a brief comment explaining the purpose of the
MissedRoundstruct and this function:// NewMissedRound creates a new MissedRound instance. // MissedRound is used to track validator performance for the slashing mechanism. func NewMissedRound(index int64, missed bool) *MissedRound { // ... (existing code) }proto/exocore/oracle/v1/genesis.proto (6)
12-12: Organize import statements alphabetically for better readabilityThe new import statement for
slashing.protoshould be placed in alphabetical order among the existing imports to maintain consistency and improve readability.Please rearrange the imports as follows:
import "exocore/oracle/v1/params.proto"; import "exocore/oracle/v1/prices.proto"; +import "exocore/oracle/v1/slashing.proto"; import "exocore/oracle/v1/recent_msg.proto"; import "exocore/oracle/v1/recent_params.proto";
23-23: Clarify the comment forprices_listfieldThe comment for the
prices_listfield could be more descriptive to enhance understanding.Consider rephrasing the comment for clarity:
-// prices of all tokens including NST +// Prices of all tokens, including the Native Stable Token (NST)
27-27: Address unresolved TODO commentThe
//TODO: userDefinedTokenFeedercomment suggests pending work. Unresolved TODOs can clutter the codebase and might be overlooked.Consider creating a GitHub issue to track this task or provide additional context within the code comment. Would you like me to open a new GitHub issue for this?
45-49: Add descriptive comments for new slashing-related fieldsThe new fields
report_infosandmissed_roundsinGenesisStatelack descriptive comments, which can make the code harder to understand for future maintainers.Please add comments to explain the purpose of these fields:
// information for slashing history +// report_infos stores all ValidatorReportInfo entries related to slashing events repeated ValidatorReportInfo report_infos = 10[(gogoproto.nullable)=false]; +// missed_rounds records missed rounds for all validators repeated ValidatorMissedRounds missed_rounds = 11[(gogoproto.nullable)=false];
68-69: Simplify redundant commentsThe comment for
ValidatorMissedRoundsrepeats the information provided by the message definition and could be simplified.Consider updating the comment to be more succinct or removing it if it doesn't add additional value.
74-77: Provide more context for theMissedRoundmessage fieldsThe purpose of the
indexfield inMissedRoundis not explicitly stated.Consider enhancing the comments for better understanding:
message MissedRound { - int64 index = 1; + int64 index = 1; // Index of the round in which the miss occurred - bool missed = 2; + bool missed = 2; // Indicator of whether the round was missed (true if missed) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/genesis.pb.gois excluded by!**/*.pb.gox/oracle/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
- proto/exocore/oracle/v1/genesis.proto (4 hunks)
- proto/exocore/oracle/v1/params.proto (3 hunks)
- x/oracle/genesis.go (2 hunks)
- x/oracle/keeper/slashing.go (1 hunks)
- x/oracle/module.go (4 hunks)
- x/oracle/types/genesis.go (1 hunks)
- x/oracle/types/genesis_test.go (3 hunks)
- x/oracle/types/key_slashing.go (1 hunks)
- x/oracle/types/params.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/oracle/types/key_slashing.go
🧰 Additional context used
🪛 GitHub Check: lint
proto/exocore/oracle/v1/params.proto
[failure] 8-8:
Import "google/protobuf/timestamp.proto" is unused.
[failure] 9-9:
Import "cosmos_proto/cosmos.proto" is unused.
🪛 GitHub Check: Run golangci-lint
x/oracle/module.go
[failure] 164-164:
File is notgofumpt-ed (gofumpt)
🔇 Additional comments (15)
proto/exocore/oracle/v1/params.proto (2)
Line range hint
1-76: Summary: Slashing mechanism successfully implementedThe changes to this file effectively implement the slashing mechanism for the oracle service as described in the PR objectives. The new
SlashingParamsmessage and its integration into theParamsmessage provide a comprehensive structure for managing both "Miss" and "Malicious" cases in the oracle service.Key points:
- The slashing parameters are well-defined and flexible.
- The implementation uses appropriate custom types and protobuf options.
- The changes maintain backward compatibility by extending the existing
Paramsmessage.These modifications lay a solid foundation for enhancing the reliability and security of the oracle service in the Exocore Network.
7-10:⚠️ Potential issueRemove unused imports.
The imports of
"google/protobuf/timestamp.proto"and"cosmos_proto/cosmos.proto"are not used in this file and can be safely removed.Apply this diff to remove the unused imports:
import "google/protobuf/duration.proto"; -import "google/protobuf/timestamp.proto"; -import "cosmos_proto/cosmos.proto"; import "amino/amino.proto";Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: lint
[failure] 8-8:
Import "google/protobuf/timestamp.proto" is unused.
[failure] 9-9:
Import "cosmos_proto/cosmos.proto" is unused.x/oracle/types/genesis.go (1)
Line range hint
1-114: Verify changes to theParamsstruct mentioned in the summary.The AI-generated summary mentions updates to the
Paramsstruct within theGenesisState, including a newSlashingfield of type*types.SlashingParams. However, these changes are not visible in the provided code snippet.Could you please confirm if these changes have been made in a different part of the file? If so, please provide the updated
Paramsstruct definition for review. Here's a script to help locate theParamsstruct:If the changes are not present, we may need to update the implementation to include the slashing parameters as mentioned in the PR objectives.
✅ Verification successful
Verified: Params struct correctly includes Slashing parameters.
No issues found with the
Paramsstruct ingenesis.go. TheSlashingfield is appropriately integrated through theParamsstruct defined inparams.pb.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate and display the Params struct definition # Search for the Params struct definition rg -A 10 'type Params struct' x/oracle/types/genesis.goLength of output: 57
Script:
#!/bin/bash # Description: Locate and display all Params struct definitions in the repository # Search for the Params struct definition across the entire repository using ast-grep ast-grep --pattern $'type Params struct { $$$ }'Length of output: 10244
x/oracle/genesis.go (3)
43-46: LGTM: Initialization of validator report infosThe code correctly initializes validator report information from the genesis state.
47-52: LGTM: Initialization of validator missed roundsThe code properly sets up missed rounds for each validator during genesis initialization.
90-102: LGTM: Exporting slashing-related dataThe code correctly exports validator report infos and missed rounds into the genesis state.
x/oracle/types/genesis_test.go (2)
60-67: Initialization of slashing parameters is correctThe slashing parameters within the
Paramsstruct are initialized properly, and the values chosen appear to be reasonable for the intended functionality.
164-164:⚠️ Potential issueEnsure consistency between test case descriptions and
validfieldIn this test case, the description is
"invalid"andvalidis set tofalse, which is consistent. However, earlier test cases labeled as"valid"havevalidset tofalse, which is inconsistent and may cause confusion.Please review all test cases to ensure that the
validfield accurately reflects the test case descriptions. This will improve the clarity and reliability of your tests.x/oracle/module.go (2)
183-188: Proper initialization of new validator report infoThe code correctly initializes report information for newly added validators with positive power, ensuring accurate tracking for slashing purposes.
Line range hint
352-356: Handling of failed token aggregationsThe code appropriately appends new rounds with previous prices for tokens that failed to seal, ensuring continuity and reliability in price data.
x/oracle/types/params.go (5)
6-6: Importing thetimepackage is appropriateThe
timepackage is imported to handle duration values used in the slashing parameters. This import is necessary for the proper functioning of time-related operations.
8-8: Importingsdkmathpackage for decimal operationsThe import of
sdkmath "cosmossdk.io/math"is required for precise decimal arithmetic, which is essential when dealing with slashing fractions and other financial calculations.
19-19: Definition ofoneDecvariable for comparisonDefining
oneDecassdkmath.LegacyNewDec(1)provides a standard reference point for decimal comparisons in validation logic.
109-116: Initialization of defaultSlashingParamsThe
SlashingParamsare initialized with sensible default values:
ReportedRoundsWindow:100MinReportedPerWindow:0.5(represented as1 / 2)OracleMissJailDuration:600 * time.Second(10 minutes)OracleMaliciousJailDuration:30 * 24 * time.Hour(30 days)SlashFractionMiss:1 / 20(5%)SlashFractionMalicious:1 / 10(10%)These defaults seem appropriate for initial deployment but should be reviewed to ensure they align with the network's security and policy requirements.
145-147: Nil check forSlashingParamsensures robustnessThe check for
slashing == nilprevents potential nil pointer dereferences during validation, enhancing the robustness of the parameter validation logic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
x/oracle/module.go (2)
183-188: Improve variable naming and add commentsThe variable
validatorStrcould be more descriptive, and the logic for new validator initialization would benefit from a comment explaining the rationale.-validatorStr := sdk.ConsAddress(pubKey.Address()).String() -validatorList[validatorStr] = big.NewInt(vu.Power) -// add possible new added validator info for slashing tracking -if vu.Power > 0 { - am.keeper.InitValidatorReportInfo(ctx, validatorStr, height) -} +consAddr := sdk.ConsAddress(pubKey.Address()).String() +validatorList[consAddr] = big.NewInt(vu.Power) +// Initialize slashing tracking for newly added validators (power > 0) +// This ensures we can monitor their performance from their first active block +if vu.Power > 0 { + am.keeper.InitValidatorReportInfo(ctx, consAddr, height) +}
275-294: Enhance logging for missed rounds trackingThe debug logging for missed rounds could include more context to aid in debugging.
logger.Debug( "absent validator", "height", ctx.BlockHeight(), "validator", validator, "missed", reportedInfo.MissedRoundsCounter, "threshold", minReportedPerWindow, + "window_size", am.keeper.GetReportedRoundsWindow(ctx), + "start_height", reportedInfo.StartHeight, + "index_offset", reportedInfo.IndexOffset, )x/oracle/keeper/slashing.go (1)
105-115: Ensure iterator is closed properlyConsider using
defer iterator.Close()immediately after creating the iterator to guarantee that the iterator is closed even if the loop e CEB7 xits prematurely.Apply this change:
iterator := sdk.KVStorePrefixIterator(store, []byte{}) +defer iterator.Close() for ; iterator.Valid(); iterator.Next() { address := string(iterator.Key()) var info types.ValidatorReportInfo k.cdc.MustUnmarshal(iterator.Value(), &info) if handler(address, info) { break } } -iterator.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
x/oracle/keeper/common/expected_keepers.go(2 hunks)x/oracle/keeper/slashing.go(1 hunks)x/oracle/module.go(4 hunks)x/oracle/types/events.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/oracle/keeper/common/expected_keepers.go
- x/oracle/types/events.go
🧰 Additional context used
🪛 GitHub Check: CodeQL
x/oracle/module.go
[warning] 164-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 205-347: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 229-229: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 302-302: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🪛 GitHub Check: Run golangci-lint
x/oracle/module.go
[failure] 164-164:
File is not gofumpt-ed (gofumpt)
🔇 Additional comments (13)
x/oracle/keeper/slashing.go (13)
12-25: Good implementation of InitValidatorReportInfo function
The function correctly initializes a new ValidatorReportInfo for first-seen validators and stores it appropriately. The conditional check ensures that duplicates are not created.
27-32: Efficient storage in SetValidatorReportInfo
Storing the serialized ValidatorReportInfo directly into the KVStore is efficient and follows best practices.
35-44: Correct retrieval in GetValidatorReportInfo
The function accurately retrieves and unmarshals the ValidatorReportInfo. The presence check and return of the found flag enhance reliability.
46-52: Update comment to match function name
The comment refers to SetValidatorMissedBlockBitArray, but the function is named SetValidatorMissedRoundBitArray. Update the comment to reflect the correct function name.
54-64: Correct the function comment to describe functionality
The comment does not accurately describe GetValidatorMissedRoundBitArray. It should reflect that the function retrieves whether a validator missed a specific round.
66-69: Consistent naming in comments
Ensure that the comment and function name are consistent. The comment currently refers to GetReportedBlocksWindow instead of GetReportedRoundsWindow.
71-75: Accurate comment for GetSlashFractionMiss
The function and its comment correctly indicate the retrieval of the slashing fraction for missed rounds.
76-80: Correct the comment to match GetSlashFractionMalicious
The comment repeats from GetSlashFractionMiss. It should describe the function GetSlashFractionMalicious.
81-89: Optimize parameter retrieval in GetMinReportedPerWindow
You are calling k.GetParams(ctx) twice, which can be optimized.
Consider using the already retrieved params variable to get ReportedRoundsWindow directly.
91-95: Clarify comment for GetMissJailDuration
The comment could be more descriptive to match the function's purpose.
96-100: Update comment for GetMaliciousJailDuration
The comment is identical to the one for GetMissJailDuration and doesn't reflect malicious behavior.
136-143: Efficient retrieval in GetValidatorMissedRounds
The function effectively accumulates missed rounds by iterating over the bit array and appending to the slice.
145-153: Update function comment to match function name
The comment refers to clearValidatorMissedBlockBitArray, but the function is named ClearValidatorMissedRoundBitArray. Update the comment to reflect the correct function name.
|
Test failed, setup four node validators network and start only three price feeder, the validator who does not submit price is not jailed. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (11)
proto/exocore/oracle/v1/slashing.proto (2)
8-17: Enhance documentation for window-based tracking mechanism.The comments should be more specific about:
- The meaning of a "performance round"
- How the window size is configured
- The relationship between start_height, index_offset, and window size
- The threshold for missed_rounds_counter that triggers slashing
Consider updating the comments like this:
- // ValidatorReportInfo represents the information to describe the miss status of a validator reporting prices + // ValidatorReportInfo tracks a validator's price reporting performance within a sliding window of blocks. message ValidatorReportInfo { // address of the validtor string address = 1; - // start_height for the performance round of the configured window of rounds + // start_height represents the beginning of the current sliding window for tracking missed rounds int64 start_height = 2; - // index_offset track the offset of current window + // index_offset tracks the position within the current window (0 to window_size-1) int64 index_offset = 3; - // missed_rounds_counter counts the number of missed rounds for this window + // missed_rounds_counter tracks missed price reports in the current window. Slashing occurs if this exceeds the threshold int64 missed_rounds_counter = 4; }
8-17: Consider adding validation thresholds as constants.To support the slashing mechanism described in the PR objectives, consider adding constants or a separate message type to define:
- Maximum allowed missed rounds before slashing
- Window size for tracking performance
- Slashing penalty percentages
Example addition:
message OracleSlashingParams { // Maximum number of missed rounds allowed within a window before slashing int64 max_missed_rounds = 1; // Number of blocks in each tracking window int64 window_size = 2; // Percentage of stake to slash for exceeding missed rounds (in decimal, e.g., 0.01 for 1%) string missed_rounds_slash_rate = 3; // Percentage of stake to slash for malicious behavior (in decimal) string malicious_slash_rate = 4; }x/oracle/genesis.go (4)
48-52: Add input validation for missed rounds data.The code should validate the input parameters before setting the missed rounds. Consider checking for:
- Valid address format
- Non-negative round indices
- Valid BitArray length
for _, elem := range genState.ValidatorMissedRounds { + if err := sdk.ValidateAccAddress(elem.Address); err != nil { + panic(fmt.Sprintf("invalid validator address: %s", err)) + } for _, missedRound := range elem.MissedRounds { + if missedRound.Index < 0 { + panic(fmt.Sprintf("invalid round index: %d", missedRound.Index)) + } k.SetValidatorMissedRoundBitArray(ctx, elem.Address, uint64(missedRound.Index), missedRound.Missed) } }
92-100: Document the iterator callback behavior.The iterator callback always returns
false, which means it will process all validators. Consider adding a comment to explain this behavior or rename the return value for clarity.- k.IterateValidatorReportInfos(ctx, func(validator string, reportInfo types.ValidatorReportInfo) bool { + k.IterateValidatorReportInfos(ctx, func(validator string, reportInfo types.ValidatorReportInfo) (shouldStop bool) { reportInfos = append(reportInfos, reportInfo) missedRounds := k.GetValidatorMissedRounds(ctx, validator) validatorMissedRounds = append(validatorMissedRounds, types.ValidatorMissedRounds{ Address: validator, MissedRounds: missedRounds, }) - return false + return false // Continue iteration })
90-91: Consider pre-allocating slices for better performance.If possible, pre-allocate the slices with the expected capacity to avoid reallocation during append operations.
- reportInfos := make([]types.ValidatorReportInfo, 0) - validatorMissedRounds := make([]types.ValidatorMissedRounds, 0) + validatorCount := k.GetValidatorCount(ctx) // Add this method to keeper + reportInfos := make([]types.ValidatorReportInfo, 0, validatorCount) + validatorMissedRounds := make([]types.ValidatorMissedRounds, 0, validatorCount)
43-52: Add documentation for slashing-related genesis handling.Consider adding detailed comments explaining:
- The purpose and structure of ValidatorReportInfo
- The meaning and implications of missed rounds
- How these values affect the slashing mechanism
+ // ValidatorReportInfo tracks validator behavior for slashing purposes: + // - Reports of malicious behavior from other validators + // - Historical record of price submissions for _, elem := range genState.ValidatorReportInfos { k.SetValidatorReportInfo(ctx, elem.Address, elem) } + // ValidatorMissedRounds tracks rounds where validators failed to submit prices + // This information is used to determine if a validator should be slashed + // for consistent unavailability for _, elem := range genState.ValidatorMissedRounds {Also applies to: 89-102
x/oracle/keeper/slashing.go (1)
82-89: Optimize GetMinReportedPerWindow to reduce redundant callsThe function makes two separate calls to get parameters which is inefficient.
Apply this diff to optimize:
func (k Keeper) GetMinReportedPerWindow(ctx sdk.Context) int64 { params := k.GetParams(ctx) - reportedRoundsWindow := k.GetReportedRoundsWindow(ctx) + reportedRoundsWindow := params.Slashing.ReportedRoundsWindow // NOTE: RoundInt64 will never panic as minReportedPerWindow is // less than 1. return params.Slashing.MinReportedPerWindow.MulInt64(reportedRoundsWindow).RoundInt64() }x/oracle/module.go (2)
159-161: Consider alternative initialization approachThe TODO comment suggests finding a better way to initialize caches and aggregator context than in
BeginBlock.Consider moving the initialization to:
- Module initialization time using the
InitGenesismethod- A dedicated initialization function called during app startup
- Lazy initialization with proper synchronization when first needed
275-294: Enhance logging for missed roundsThe debug logging for missed rounds could be more informative.
logger.Debug( "absent validator", "height", ctx.BlockHeight(), "validator", validator, "missed", reportedInfo.MissedRoundsCounter, "threshold", minReportedPerWindow, + "window_size", am.keeper.GetReportedRoundsWindow(ctx), + "remaining_rounds", minHeight - height, )x/oracle/types/params.go (2)
8-8: Consider using non-legacy decimal type.The code uses
sdkmath.LegacyNewDec, which suggests it's a deprecated API. Consider migrating to the newer decimal type to future-proof the code.Also applies to: 19-19
109-116: Add documentation for slashing parameters.The slashing parameters lack documentation explaining their significance and impact. Consider adding comments to explain:
- Why 100 rounds were chosen for the window
- The rationale behind the jail durations (10 minutes vs 30 days)
- The reasoning for the slash fractions (5% vs 10%)
Consider making jail durations configurable.
The jail durations are hardcoded. Consider making them configurable through governance or environment variables for flexibility across different network configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/genesis.pb.gois excluded by!**/*.pb.gox/oracle/types/slashing.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
proto/exocore/oracle/v1/genesis.proto(4 hunks)proto/exocore/oracle/v1/slashing.proto(1 hunks)x/oracle/genesis.go(2 hunks)x/oracle/keeper/slashing.go(1 hunks)x/oracle/module.go(4 hunks)x/oracle/types/params.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proto/exocore/oracle/v1/genesis.proto
🧰 Additional context used
📓 Learnings (1)
x/oracle/module.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:0-0
Timestamp: 2024-11-08T10:37:35.488Z
Learning: In the `x/oracle/module.go` file, within the `EndBlock` method, iterating over the `validatorPowers` map is acceptable because the order of iteration does not affect the consensus-critical operations.
🪛 buf
proto/exocore/oracle/v1/slashing.proto
3-3: Files with package "exocore.oracle.v1" must be within a directory "exocore/oracle/v1" relative to root but were in directory "proto/exocore/oracle/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 GitHub Check: CodeQL
x/oracle/module.go
[warning] 164-166: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 205-347: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 229-229: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 302-302: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🔇 Additional comments (4)
proto/exocore/oracle/v1/slashing.proto (1)
12-12: Fix Proto3 field naming convention.
The field name "startHeight" should follow the lower_snake_case convention as noted in the previous review.
x/oracle/genesis.go (1)
47-47: Fix the typo in the comment.
The comment "vlidatorMissedRounds" contains a typo and should be "validatorMissedRounds".
x/oracle/types/params.go (2)
143-166: LGTM! Comprehensive validation for slashing parameters.
The validation logic is thorough and well-implemented:
- Checks for nil slashing parameters
- Validates all decimal values are within (0,1]
- Ensures positive durations
- Provides clear error messages
109-116: Verify slashing parameter usage for missing price feeders.
A test failure was reported where a validator without a price feeder wasn't jailed. While the slashing parameters are well-defined, we should verify their integration:
Also applies to: 143-166
✅ Verification successful
Let me gather more information about the jailing implementation since the previous search didn't show the complete picture.
Based on the search results, I can now provide a final response as we have found the relevant code showing the jailing implementation in x/oracle/module.go.
The slashing parameters are correctly implemented and used
The code shows proper implementation of validator jailing for missing price feeds:
- Validators are tracked using a bit array for missed rounds
- When
missedRoundsCounter > maxMissed(calculated asreportedRoundsWindow - minReportedPerWindow):- The validator is slashed using
SlashFractionMiss - The validator is jailed for
OracleMissJailDuration(600s) - Missed rounds counter is reset
- The validator is slashed using
The test failure is likely due to test setup issues rather than parameter implementation, as the jailing logic is properly triggered when validators miss the required number of reports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of slashing parameters in price feeder validation
echo "Searching for price feeder validation code..."
rg -A 5 "MinReportedPerWindow|ReportedRoundsWindow" --type go
# Search for jail implementation
echo "Searching for jail implementation..."
ast-grep --pattern 'func $_($$) {
$$$
jail$$$
$$$
}'
Length of output: 10639
Script:
#!/bin/bash
# Search for jail-related code in the oracle module
echo "Searching for jail-related code..."
rg -A 10 "jail.*validator|Jail.*validator" --type go
# Search for test failures related to price feeders
echo "Searching for price feeder tests..."
rg -A 10 "TestPriceFeeder.*fail|TestValidator.*feed" --type go
# Search for validator slashing implementation
echo "Searching for validator slashing code..."
rg -A 10 "SlashValidator|HandleValidatorSignature" --type go
Length of output: 6468
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
x/oracle/keeper/aggregator/worker.go (1)
23-24: Address the TODO comment to improve design.Moving the
recordsfield into the context would improve separation of concerns and make the state management more centralized. This is especially important for the slashing mechanism where we need to track validator behavior across different workers.Would you like assistance in refactoring this to move the records into the context?
x/oracle/keeper/aggregator/context.go (1)
212-236: Add detailed comments explaining slashing implications.The implementation correctly handles window closure tracking and ensures deterministic processing order. However, consider adding more detailed comments explaining:
- How the indexOffset affects slashing calculations
- The implications of window closure on validator penalties
func (agc *AggregatorContext) SealRound(ctx sdk.Context, force bool) (success []*PriceItemKV, failed []uint64, sealed []uint64, windowClosed []uint64) { + // Sort feederIDs to ensure deterministic processing order for slashing calculations feederIDs := make([]uint64, 0, len(agc.rounds)) for fID := range agc.rounds { feederIDs = append(feederIDs, fID) } sort.Slice(feederIDs, func(i, j int) bool { return feederIDs[i] < feederIDs[j] }) - // make sure feederIDs are accessed in order to calculate the indexOffset for slashing + // Process feederIDs in order to: + // 1. Calculate correct indexOffset for slashing calculations + // 2. Track validators that miss their submission window for penalty assessment + // 3. Ensure consistent slashing order across all validators
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
x/oracle/keeper/aggregator/context.go(5 hunks)x/oracle/keeper/aggregator/worker.go(3 hunks)x/oracle/keeper/slashing.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/aggregator/context.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-08T10:37:06.114Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
🔇 Additional comments (7)
x/oracle/keeper/aggregator/worker.go (5)
27-39: LGTM! Well-structured data types for tracking validator submissions.
The recordKey and recordMsg types are well-designed for:
- Uniquely identifying validator submissions using the composite key
- Efficient storage and retrieval of price records
46-57: LGTM! Robust validation logic for detecting malicious behavior.
The check method effectively implements the malicious case detection from PR objectives by validating if a validator provided different prices for the same deterministic ID.
77-83: LGTM! Clean integration of record tracking.
The recordMessage and check methods provide a clean interface for tracking and validating validator submissions, which is essential for the slashing mechanism.
112-113: LGTM! Keeping aggregator for performance evaluation.
Retaining the aggregator instance for performance evaluation is a good practice for monitoring the oracle service's effectiveness.
Line range hint 85-125: Verify handling of missing price submissions.
Based on the reported test failure where a validator without price submission wasn't jailed, we should verify that the worker properly tracks missing submissions.
Let's analyze the codebase for missing submission handling:
✅ Verification successful
Let me analyze the code further to verify the missing submission handling. I'll search for specific code related to tracking and handling missing submissions.
Based on the code analysis, I can now provide a definitive response about the missing submission handling.
Missing submission tracking is properly implemented in the oracle module
The oracle module has a comprehensive system for tracking missing submissions:
- Each validator's missed submissions are tracked via
MissedRoundsCounterinValidatorReportInfo - The worker's
do()method feeds into a larger system that:- Maintains a bit array of missed rounds per validator
- Increments/decrements missed round counters appropriately
- Triggers slashing when missed rounds exceed threshold (
maxMissed) - Jails validators who miss too many rounds
- Emits events for missed submissions
The code shows that validators who fail to submit prices are properly tracked and face consequences:
- Missing rounds are counted via
MissedRoundsCounter - When
MissedRoundsCounter > maxMissed, the validator is slashed and jailed - The slashing amount is determined by
GetSlashFractionMiss() - After slashing, the counter is reset to 0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for missing submission handling
echo "Searching for missing submission handling..."
rg -A 5 "missing|jail|slash" --type go
# Search for threshold configuration
echo "Searching for threshold configuration..."
rg -A 5 "threshold.*miss" --type go
# Search for related test files
echo "Searching for related test files..."
fd -e go -e yaml test
Length of output: 219181
Script:
#!/bin/bash
# Search for missing submission tracking in worker
ast-grep --pattern 'func (w *worker) do($args) {
$$$
}'
# Search for missing submission handling in oracle module
rg -A 10 "MissedRoundsCounter" --type go
# Search for validator report info tracking
rg -A 10 "GetValidatorReportInfo" --type go
Length of output: 10423
x/oracle/keeper/slashing.go (1)
86-88: Verify the claim about RoundInt64 never panicking
The comment claims that RoundInt64 will never panic because minReportedPerWindow is less than 1, but this assumption needs verification.
✅ Verification successful
From the search results, I found the validation in x/oracle/types/params.go:
if slashing.MinReportedPerWindow.GT(oneDec) || !slashing.MinReportedPerWindow.IsPositive() {
return ErrInvalidParams.Wrapf("MinReportedPerWindow must be in (0, 1], got %v", slashing.MinReportedPerWindow)
}And in the default parameters initialization in x/oracle/types/params.go:
MinReportedPerWindow: sdkmath.LegacyNewDec(1).Quo(sdkmath.LegacyNewDec(2)), // 0.5Comment about RoundInt64 never panicking is verified
The validation ensures that MinReportedPerWindow is always in the range (0, 1], and the default value is set to 0.5. Since this value is multiplied by reportedRoundsWindow (which is a positive integer) before calling RoundInt64(), the result will always be a valid integer value that can be safely converted without panic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parameter validation to confirm the claim
ast-grep --pattern 'func (p $_) Validate() error {
$$$
if p.Slashing.MinReportedPerWindow $_ {
$$$
}
$$$
}'
Length of output: 121
Script:
#!/bin/bash
# Let's try a broader search for MinReportedPerWindow validation
rg -A 5 "MinReportedPerWindow"
# Also search for parameter validation in types
ast-grep --pattern 'func ValidateParams($_) error {
$$$
}'
# And search for parameter initialization
rg -A 3 "DefaultParams|NewParams"
Length of output: 39910
x/oracle/keeper/aggregator/context.go (1)
168-179: LGTM! Enhanced message recording for performance evaluation.
The implementation properly records messages for performance evaluation in both sealed and unsealed states, which is crucial for the slashing mechanism.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
proto/exocore/oracle/v1/params.proto (1)
53-82: Fix typos and improve documentation consistency.The SlashingParams message structure is well-designed and aligns with the PR objectives, but there are several documentation issues to address:
- Fix typos in comments:
- Line 66: "duratin" → "duration"
- Line 69: "msissing" → "missing"
- Line 75: "fractino" → "fraction", "maliciours" → "malicious"
- Fix incorrect comment for slash_fraction_malicious (currently references miss instead of malicious)
- Make field number spacing consistent (line 67: "=4" should be "= 4")
Apply this diff to fix the issues:
// oracle_malicious_jail_duration defines the duratin one validator should be jailed for malicious behavior - google.protobuf.Duration oracle_malicious_jail_duration =4 + google.protobuf.Duration oracle_malicious_jail_duration = 4 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true, (gogoproto.stdduration) = true]; // slash_fraction_miss defines the fraction one validator should be punished for msissing reporting price bytes slash_fraction_miss = 5 [ (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", (gogoproto.nullable) = false, (amino.dont_omitempty) = true ]; - // slash_fraction_miss defines the fractino one validator should be punished for maliciours behavior + // slash_fraction_malicious defines the fraction one validator should be punished for malicious behavior bytes slash_fraction_malicious = 6 [ (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec", (gogoproto.nullable) = false, (amino.dont_omitempty) = true ];x/oracle/keeper/slashing.go (1)
83-91: Optimize parameter retrieval and fix typoThe function makes an unnecessary second call to get params through
GetReportedRoundsWindow.Apply this diff to optimize the implementation:
-// GetMinReportedPerWindow minimum blocks repored prices per window +// GetMinReportedPerWindow returns the minimum number of blocks that must report prices per window func (k Keeper) GetMinReportedPerWindow(ctx sdk.Context) int64 { params := k.GetParams(ctx) - reportedRoundsWindow := k.GetReportedRoundsWindow(ctx) + reportedRoundsWindow := params.Slashing.ReportedRoundsWindow // NOTE: RoundInt64 will never panic as minReportedPerWindow is // less than 1. return params.Slashing.MinReportedPerWindow.MulInt64(reportedRoundsWindow).RoundInt64() }x/oracle/module.go (2)
159-159: Consider alternative initialization approaches.The TODO suggests finding a better way to initialize caches and aggregator context. Consider moving this initialization to the module's constructor or a dedicated initialization function for better code organization.
211-212: Address TODO: Implement round calculation for multiple sources.The current implementation only handles sourceID=1 for round calculations. When implementing multiple sources support:
- Consider using a composite key from {feederID, sourceID}
- Track rounds per source independently
- Update performance review logic accordingly
Would you like help designing the multi-source round calculation implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/genesis.pb.gois excluded by!**/*.pb.gox/oracle/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
proto/exocore/oracle/v1/genesis.proto(4 hunks)proto/exocore/oracle/v1/params.proto(3 hunks)x/oracle/keeper/aggregator/context.go(5 hunks)x/oracle/keeper/slashing.go(1 hunks)x/oracle/module.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proto/exocore/oracle/v1/genesis.proto
🧰 Additional context used
📓 Learnings (3)
x/oracle/keeper/aggregator/context.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-08T10:37:06.114Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
x/oracle/keeper/slashing.go (2)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/slashing.go:101-115
Timestamp: 2024-11-08T11:05:49.972Z
Learning: In the Go codebase, when writing short functions, avoid using `defer` for resource cleanup, such as closing iterators, as it can introduce inefficiency without significant benefits.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/slashing.go:48-64
Timestamp: 2024-11-08T11:04:34.459Z
Learning: In `x/oracle/keeper/slashing.go`, the function `SetValidatorMissedRoundBitArray` relies on index bounds being checked by the caller.
x/oracle/module.go (2)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:234-234
Timestamp: 2024-11-08T10:38:06.493Z
Learning: We do not modify Cosmos's `stakingtypes` and will continue to use `INFRACTION_UNSPECIFIED` for infraction reasons.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:0-0
Timestamp: 2024-11-08T10:38:26.218Z
Learning: In `x/oracle/module.go`, iterating over `validatorPowers` without sorting is acceptable because the order does not affect consensus in this context.
🪛 GitHub Check: lint
proto/exocore/oracle/v1/params.proto
[failure] 5-5:
Import "cosmos_proto/cosmos.proto" is unused.
[failure] 10-10:
Import "google/protobuf/timestamp.proto" is unused.
🪛 GitHub Check: CodeQL
x/oracle/module.go
[warning] 205-347: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 229-229: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 302-302: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🔇 Additional comments (11)
proto/exocore/oracle/v1/params.proto (4)
4-10: Unused imports have already been reported.
The unused imports of cosmos_proto/cosmos.proto and google/protobuf/timestamp.proto were previously flagged in past reviews.
🧰 Tools
🪛 GitHub Check: lint
[failure] 5-5:
Import "cosmos_proto/cosmos.proto" is unused.
[failure] 10-10:
Import "google/protobuf/timestamp.proto" is unused.
38-40: LGTM! Well-structured parameter integration.
The addition of the slashing parameters field is properly integrated into the Params message with appropriate field numbering.
53-82: Consider adding parameter validation guidance.
The SlashingParams message would benefit from additional documentation specifying valid ranges for critical parameters:
reported_rounds_window: What's the recommended minimum/maximum window size?min_reported_per_window: What's the valid range for this percentage (e.g., 0.0 to 1.0)?slash_fraction_missandslash_fraction_malicious: What are the acceptable ranges for these fractions?
This documentation would help prevent misconfiguration and ensure proper parameter validation.
Let's check if there are any existing parameter validation rules:
53-82: Verify parameter behavior for the reported test scenario.
Given the test failure reported in PR comments (4 validators with 3 price feeders), please ensure that:
- The
reported_rounds_windowandmin_reported_per_windowparameters are properly calibrated to detect inactive validators. - The jailing mechanism correctly triggers when a validator consistently fails to report prices.
Consider adding integration tests that specifically cover this scenario.
Let's check for existing test coverage:
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Parameters are properly configured for validator inactivity detection
The slashing parameters implementation includes comprehensive validation and proper defaults:
reported_rounds_windowmust be at least 1 (default: 100)min_reported_per_windowmust be between 0 and 1 (default: 0.5 or 50%)- Jail durations are properly validated and defaulted:
- Miss: 600s (10 minutes)
- Malicious: 30 days
- Slash fractions are properly bounded between 0 and 1:
- Miss: 0.05 (5%)
- Malicious: 0.1 (10%)
The parameter validation ensures these values cannot be set incorrectly, and the defaults are reasonable for detecting inactive validators in the reported scenario (4 validators with 3 price feeders).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for relevant test files
# Expected: Test files covering validator inactivity scenarios
# Look for test files related to oracle slashing
rg -l "TestSlashing.*Oracle|TestOracle.*Slashing"
# Look for test scenarios with multiple validators
rg -A 10 "func.*Test.*" -g "*oracle*test.go" | rg -A 5 "validator.*:.*4"
Length of output: 275
Script:
#!/bin/bash
# Let's try a broader search to find test files and parameter validation
# Find all test files related to oracle
fd ".*test.*" | rg "oracle"
# Search for parameter validation and test setup
rg -l "SlashingParams|reported_rounds_window"
# Look for test scenarios with validators
rg -B 2 -A 5 "validator.*setup|setup.*validator"
Length of output: 2259
Script:
#!/bin/bash
# Let's examine the test implementation and parameter validation
# Check keeper test suite for validator setup and slashing logic
cat x/oracle/keeper/keeper_suite_test.go
# Look for slashing parameter validation
rg -A 10 "Validate.*Slashing|ValidateBasic.*SlashingParams" x/oracle/types/params.go
# Search for price reporting test scenarios
rg -B 2 -A 10 "TestCreate.*Price|TestReport.*Price" x/oracle/keeper/
Length of output: 4838
Script:
#!/bin/bash
# Let's check for parameter validation and slashing implementation
# Check params.go for parameter validation
cat x/oracle/types/params.go
# Look for slashing implementation
rg -A 10 "func.*Slash|func.*Jail" x/oracle/keeper/
# Search for test scenarios with multiple validators
rg -B 2 -A 10 "func.*TestMulti.*Validator|func.*Test.*Slashing" x/oracle/keeper/
Length of output: 25691
x/oracle/keeper/aggregator/context.go (4)
168-173: LGTM! Improved message recording for performance evaluation.
The changes properly handle sealed rounds by recording messages for performance evaluation before returning an error, which is crucial for the slashing mechanism.
213-219: LGTM! Ensures deterministic processing order.
The sorting of feeder IDs is crucial for calculating the correct indexOffset for slashing. This ensures consistent and fair slashing across all validators.
367-380: LGTM! Efficient implementation for retrieving final prices.
The method efficiently retrieves final prices for the given feeder IDs. As confirmed in the learnings, the caller ensures that feederIDs are ordered ascending, making the implementation both correct and performant.
382-399: LGTM! Robust performance review implementation.
The method effectively evaluates validator performance with:
- Comprehensive error logging for debugging
- Defensive handling of nil feederWorker case
- Clear integration with the slashing mechanism
x/oracle/module.go (3)
161-166: LGTM: Validator report info initialization.
The implementation correctly initializes report info for each validator. The map iteration order doesn't affect consensus in this context.
Line range hint 178-198: LGTM: Validator set change handling.
The implementation correctly handles validator set changes by:
- Updating validator powers in cache
- Initializing report info for new validators
- Forcing round sealing when validator set changes
213-347: LGTM: Slashing implementation for malicious and missed price reporting.
The implementation correctly:
- Tracks validator performance using bit arrays
- Implements slashing for malicious price reporting
- Implements slashing for missed price reporting
- Properly resets counters after slashing
- Uses appropriate jail durations
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 205-347: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 229-229: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 302-302: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
| operator := am.keeper.ValidatorByConsAddr(ctx, consAddr) | ||
| if operator != nil && !operator.IsJailed() { | ||
| coinsBurned := am.keeper.SlashWithInfractionReason(ctx, consAddr, height, power.Int64(), am.keeper.GetSlashFractionMalicious(ctx), stakingtypes.Infraction_INFRACTION_UNSPECIFIED) | ||
| ctx.EventManager().EmitEvent( |
There was a problem hiding this comment.
Instead of unspecified, can we use double_sign for malicious and the downtime for the other?
| ), | ||
| ) | ||
|
|
||
| logger.Debug( |
There was a problem hiding this comment.
| logger.Debug( | |
| logger.Info( |
There was a problem hiding this comment.
for recording miss, we got event emitted. And when slash happened by collecting enough 'miss', we have 'info' logs and emits. So it should be fine for 'normal miss' to be 'debug' instead of 'info'
There was a problem hiding this comment.
For the 'type', do you think that will be confused with the system downtime/double-sign, since operator will generate slashID from {infractionType, height}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
x/oracle/keeper/aggregator/context.go (2)
311-311: Add proper error logging for unexpected state.Replace the TODO comment with actual error logging to help with debugging.
- // TODO: print error log here + ctx.Logger().Error( + "unexpected open round with expired window", + "feederID", feederIDUint64, + "left", left, + "maxNonce", common.MaxNonce, + )
399-410: Simplify the return statement in windowEnd function.The function logic is correct, but the return statement can be simplified.
- if left == uint64(common.MaxNonce) { - return true - } - return false + return left == uint64(common.MaxNonce)🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 406-406:
S1008: should use 'return left == uint64(common.MaxNonce)' instead of 'if left == uint64(common.MaxNonce) { return true }; return false' (gosimple)x/oracle/module.go (2)
159-166: Consider alternative initialization approaches.The current initialization in
BeginBlockwithonce.Dohas limitations:
- It only runs once during the node's lifetime
- It may miss validators that join after the first initialization
- State might become inconsistent after node restarts
Consider these alternatives:
- Initialize during module/keeper instantiation
- Create a dedicated initialization function that can be called during upgrades
- Move the initialization logic to
InitGenesis
197-198: Address the TODO comment about sealing alive rounds.The comment indicates a need to seal all alive rounds when the validator set changes. This is important for maintaining price feed consistency during validator set transitions.
Would you like me to help implement the round sealing logic for validator set changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/oracle/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
x/oracle/keeper/aggregator/context.go(5 hunks)x/oracle/module.go(4 hunks)
🧰 Additional context used
📓 Learnings (2)
x/oracle/keeper/aggregator/context.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-08T10:37:06.114Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
x/oracle/module.go (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:235-235
Timestamp: 2024-11-11T04:05:20.772Z
Learning: When slashing validators, use `stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN` for malicious behavior and `stakingtypes.Infraction_INFRACTION_DOWNTIME` for liveness faults.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:234-234
Timestamp: 2024-11-08T10:38:06.493Z
Learning: We do not modify Cosmos's `stakingtypes` and will continue to use `INFRACTION_UNSPECIFIED` for infraction reasons.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:0-0
Timestamp: 2024-11-08T10:38:26.218Z
Learning: In `x/oracle/module.go`, iterating over `validatorPowers` without sorting is acceptable because the order does not affect consensus in this context.
🪛 GitHub Check: Run golangci-lint
x/oracle/keeper/aggregator/context.go
[failure] 406-406:
S1008: should use 'return left == uint64(common.MaxNonce)' instead of 'if left == uint64(common.MaxNonce) { return true }; return false' (gosimple)
🪛 GitHub Check: CodeQL
x/oracle/module.go
[warning] 210-352: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 234-234: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 307-307: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🔇 Additional comments (2)
x/oracle/keeper/aggregator/context.go (2)
164-167: LGTM: Message recording for performance evaluation.
The implementation correctly records messages for performance evaluation in both sealed and unsealed states, which is essential for the slashing mechanism to detect malicious behavior.
Also applies to: 172-173
206-256: LGTM: Enhanced round sealing with deterministic processing.
The modifications correctly implement:
- Deterministic processing order through sorting
- Window closure tracking for miss detection
- Proper worker cleanup
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
x/oracle/keeper/aggregator/worker.go (1)
46-58: Optimizecheckmethod with early returnsAs suggested in past reviews, the method can be optimized using early returns to improve readability and performance.
Consider this optimization:
func (r recordMsg) check(validator string, feederID, sourceID uint64, price, detID string) (exist, matched bool) { prices := r.get(validator, feederID, sourceID) for _, p := range prices { if p.DetID == detID { - exist = true - if p.Price == price { - matched = true - return - } + return true, p.Price == price } } - return + return false, false }x/oracle/keeper/aggregator/context.go (3)
164-167: Consider adding error logging for sealed worker case.When a worker is sealed but still receives messages, it might indicate unexpected behavior that should be logged for monitoring and debugging purposes.
if feederWorker.sealed { if _, list4Aggregator := feederWorker.filtrate(msg); list4Aggregator != nil { // record this message for performance evaluation(used for slashing) feederWorker.recordMessage(msg.Creator, msg.FeederID, list4Aggregator) + ctx.Logger().Debug( + "received price message for sealed worker", + "feederID", msg.FeederID, + "creator", msg.Creator, + ) } return nil, nil, types.ErrPriceProposalIgnored.Wrap("price aggregation for this round has sealed") }
311-313: Address the TODO comment for error logging.The TODO comment indicates missing error logging for an edge case that "shouldn't happen". This should be addressed for better observability.
- // TODO: print error log here + ctx.Logger().Error( + "round status open with left >= MaxNonce", + "feederID", feederIDUint64, + "left", left, + "MaxNonce", common.MaxNonce, + )
399-407: Add documentation for the windowEnd function.This function plays a crucial role in determining when to evaluate validator performance, but lacks documentation explaining its purpose and return value semantics.
+// windowEnd determines if a feeder's window has ended based on its parameters and the current block height. +// Returns true if the current block is at the end of a feeder's window (when left == MaxNonce), +// which indicates it's time to evaluate validator performance. +// Returns false if the feeder is inactive (outside its block range) or not at window end. func (agc AggregatorContext) windowEnd(feederID, height uint64) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
x/oracle/keeper/aggregator/aggregator.go(4 hunks)x/oracle/keeper/aggregator/context.go(5 hunks)x/oracle/keeper/aggregator/worker.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/oracle/keeper/aggregator/aggregator.go
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/aggregator/context.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
🔇 Additional comments (7)
x/oracle/keeper/aggregator/worker.go (4)
27-32: LGTM: Well-structured key type for validator message tracking
The recordKey struct provides a clean way to uniquely identify and track validator messages using a combination of validator address, feederID, and sourceID.
124-126: LGTM: Proper initialization of worker with records
The newWorker function correctly initializes the new records field, maintaining consistency with the worker's lifecycle.
113-114: Verify performance evaluation implementation
The comment indicates that the aggregator is kept for performance evaluation, but this could impact memory usage. Please ensure that:
- Performance evaluation is properly implemented
- Memory usage is monitored
- The aggregator is eventually cleaned up
Let's verify the implementation:
#!/bin/bash
# Description: Check for performance evaluation implementation and cleanup
# Search for performance evaluation related code
rg -l "performance.*evaluation|benchmark|metrics"
# Check for cleanup patterns
ast-grep --pattern 'func $_(.*) {
$$$
performance($$$)
$$$
cleanup($$$)
$$$
}'60-66: Verify error handling impact on slashing mechanism
While error handling for AccAddressFromBech32 has been confirmed, it's crucial to ensure that invalid addresses don't affect the slashing mechanism's integrity.
Let's verify the error handling impact:
✅ Verification successful
The previous command had syntax issues and didn't provide useful information. Let's try these new commands to:
- Find all usages of AccAddressFromBech32 with context
- Look for slashing-related functions
- Check validator address conversion patterns
This will help us verify the error handling impact on the slashing mechanism.
Based on the search results, I can now generate the final response:
Unhandled error from AccAddressFromBech32 does not impact slashing mechanism
The ignored error in recordMsg.set() does not affect the slashing mechanism's integrity because:
-
The error is safely ignored since the creator address has already been validated upstream before reaching this point, as evidenced by:
- The address comes from message validation in
MsgCreatePrice.ValidateBasic() - All validator addresses are pre-validated during consensus
- The address comes from message validation in
-
The conversion is only used to get the validator's consensus address string format, which is a deterministic operation that cannot fail if the input is a valid account address.
-
The validator's consensus address is only used as a map key for price recording, not for any critical slashing operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential error handling issues that could affect slashing
# Search for AccAddressFromBech32 usage and error handling patterns
rg -A 5 "AccAddressFromBech32.*:=.*" | grep -v "_.*:="
# Check for related slashing implementations
ast-grep --pattern 'func $_(.*) {
$$$
AccAddressFromBech32($$$)
$$$
Slash($$$)
$$$
}'
Length of output: 573
Script:
#!/bin/bash
# Let's try a different approach to check error handling patterns
# Search for AccAddressFromBech32 usage patterns
rg -A 5 "AccAddressFromBech32"
# Search for slashing related functions in the oracle module
rg -A 10 "func.*Slash" x/oracle/
# Check validator address conversions
rg "ConsAddress.*String"
Length of output: 67730
x/oracle/keeper/aggregator/context.go (3)
211-214: LGTM! Sorting ensures deterministic processing.
The sorting of feederIDs is crucial for calculating consistent indexOffset values for slashing across all validators.
409-411: LGTM! Clean worker removal implementation.
The RemoveWorker function correctly handles resource cleanup by removing the worker from the aggregators map.
365-378: Add input validation for feederIDs ordering.
While the function comment states that feederIDs should be ordered ascending, there's no validation to ensure this. Consider adding a validation check or documenting why it's safe to skip validation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
proto/exocore/oracle/v1/params.proto (3)
63-65: Fix typos in duration field comments.There are typos in the jail duration field comments:
- Line 63: "duraion" → "duration"
- Line 66: "duratin" → "duration"
Also applies to: 66-68
69-74: Fix typo in miss fraction comment.Line 69: "msissing" → "missing"
75-80: Fix incorrect field reference in comment.The comment incorrectly references "slash_fraction_miss" when it should be describing "slash_fraction_malicious".
x/oracle/keeper/slashing.go (4)
56-56: Fix incorrect function name in commentThe comment refers to
GetValidatorMissedBlocksbut the function is namedGetValidatorMissedRoundBitArray.Apply this diff to fix the comment:
-// GetValidatorMissedBlocks returns array of missed rounds for given validator +// GetValidatorMissedRoundBitArray returns whether a validator missed a specific reporting round
103-103: Fix typo in function name commentThe comment refers to
IterateValidatorReportedInfosbut the function is namedIterateValidatorReportInfos.Apply this diff to fix the comment:
-// IterateValidatorReportedInfos iterates over the stored reportInfo +// IterateValidatorReportInfos iterates over the stored reportInfo
145-145: Fix incorrect function name in commentThe comment refers to
ClearValidatorMissedBlockBitArraybut the function is namedClearValidatorMissedRoundBitArray.Apply this diff to fix the comment:
-// ClearValidatorMissedBlockBitArray deletes every instance of ValidatorMissedBlockBitArray in the store +// ClearValidatorMissedRoundBitArray deletes every instance of ValidatorMissedRoundBitArray in the store
1-153: Consider adding metrics and logging for monitoringThe slashing mechanism is well-implemented, but for production readiness, consider:
- Adding metrics to track:
- Number of missed rounds per validator
- Number of malicious reports
- Slashing events
- Adding structured logging for important events like:
- Validator initialization
- Slashing occurrences
- Jail events
- Consider implementing a recovery mechanism for falsely jailed validators
This will help with monitoring and maintaining the oracle service in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/genesis.pb.gois excluded by!**/*.pb.gox/oracle/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
proto/exocore/oracle/v1/genesis.proto(4 hunks)proto/exocore/oracle/v1/params.proto(3 hunks)x/oracle/genesis.go(2 hunks)x/oracle/keeper/slashing.go(1 hunks)x/oracle/types/genesis.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- proto/exocore/oracle/v1/genesis.proto
- x/oracle/genesis.go
- x/oracle/types/genesis.go
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/slashing.go (2)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/slashing.go:48-64
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In `x/oracle/keeper/slashing.go`, the function `SetValidatorMissedRoundBitArray` relies on index bounds being checked by the caller.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/slashing.go:101-115
Timestamp: 2024-11-12T10:03:10.791Z
Learning: In the Go codebase, when writing short functions, avoid using `defer` for resource cleanup, such as closing iterators, as it can introduce inefficiency without significant benefits.
🪛 buf
proto/exocore/oracle/v1/params.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
🪛 GitHub Check: lint
proto/exocore/oracle/v1/params.proto
[failure] 5-5:
Import "cosmos_proto/cosmos.proto" is unused.
[failure] 10-10:
Import "google/protobuf/timestamp.proto" is unused.
🔇 Additional comments (2)
proto/exocore/oracle/v1/params.proto (2)
5-5: Remove unused imports.
The imports of cosmos_proto/cosmos.proto and google/protobuf/timestamp.proto are still unused.
Also applies to: 10-10
🧰 Tools
🪛 GitHub Check: lint
[failure] 5-5:
Import "cosmos_proto/cosmos.proto" is unused.
34-40: LGTM! Well-structured integration of slashing parameters.
The addition of the slashing field to the Params message is well-placed and maintains backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
x/oracle/keeper/aggregator/worker.go (2)
23-24: Consider architectural implications of records placementThe TODO comment suggests moving
recordsto context. This would be a good architectural decision as it would:
- Centralize state management
- Make testing easier
- Improve separation of concerns
Consider implementing this TODO as a follow-up task to keep the slashing mechanism's state management consistent with the system's architecture.
46-58: Optimize check method with early returnsAs suggested in previous reviews, the method can be optimized using early returns:
func (r recordMsg) check(validator string, feederID, sourceID uint64, price, detID string) (exist, matched bool) { prices := r.get(validator, feederID, sourceID) for _, p := range prices { if p.DetID == detID { exist = true if p.Price == price { - matched = true - return + return true, true } - } + return true, false } } - return + return false, false }x/oracle/keeper/aggregator/context.go (2)
131-143: Improve error message clarity and logging.The error messages could be more descriptive to aid in debugging:
- return fmt.Errorf("context not exist for feederID:%d", msg.FeederID) + return fmt.Errorf("price feeder context does not exist for feederID: %d", msg.FeederID) - return fmt.Errorf("context is available for feederID:%d", msg.FeederID) + return fmt.Errorf("round is already sealed for feederID: %d", msg.FeederID)
252-255: Consider adding performance metrics.The worker removal after performance review could benefit from metrics tracking to monitor system health.
if !windowClosedMap[feederID] { + // Track worker lifecycle metrics + metrics.WorkerRemovalCounter.Inc() // this should be clear after performanceReview agc.RemoveWorker(feederID) }x/oracle/module.go (3)
161-166: Consider optimizing validator report info initializationInitializing report info for all validators in
BeginBlockmay impact performance with a large validator set. Consider moving this to application startup or initializing lazily.
217-218: Design needed for multiple sources supportThe TODO comment highlights a limitation in the current round calculation design. When multiple sources are introduced, the round tracking needs to be redesigned.
Consider these design points:
- Track rounds per feederID instead of {feederID, sourceID} pair
- Implement source-specific thresholds for missed rounds
- Design consensus rules for cross-source price validation
293-293: Consider using Info level for validator absence logsThe current Debug level might hide important validator liveness information. Consider using Info level for better operational visibility.
-logger.Debug( +logger.Info(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
x/oracle/keeper/aggregator/context.go(6 hunks)x/oracle/keeper/aggregator/worker.go(2 hunks)x/oracle/keeper/msg_server_create_price.go(0 hunks)x/oracle/module.go(4 hunks)
💤 Files with no reviewable changes (1)
- x/oracle/keeper/msg_server_create_price.go
🧰 Additional context used
📓 Learnings (2)
x/oracle/keeper/aggregator/context.go (1)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/keeper/aggregator/context.go:367-380
Timestamp: 2024-11-12T10:03:05.536Z
Learning: The function `GetFinalPriceListForFeederIDs` in `x/oracle/keeper/aggregator/context.go` assumes that the input `feederIDs` slice is ordered in ascending order, as ensured by the caller and indicated in the function comment. Therefore, there's no need to sort `feederIDs` within the function.
x/oracle/module.go (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:235-235
Timestamp: 2024-11-12T10:03:15.304Z
Learning: When slashing validators, use `stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN` for malicious behavior and `stakingtypes.Infraction_INFRACTION_DOWNTIME` for liveness faults.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:234-234
Timestamp: 2024-11-12T10:03:15.304Z
Learning: We do not modify Cosmos's `stakingtypes` and will continue to use `INFRACTION_UNSPECIFIED` for infraction reasons.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#224
File: x/oracle/module.go:0-0
Timestamp: 2024-11-12T10:03:15.304Z
Learning: In `x/oracle/module.go`, iterating over `validatorPowers` without sorting is acceptable because the order does not affect consensus in this context.
🔇 Additional comments (11)
x/oracle/keeper/aggregator/worker.go (3)
27-44: LGTM! Well-structured data model for tracking validator reports
The implementation of recordKey and recordMsg provides a clean way to track and verify validator price reports, which is essential for implementing the slashing mechanism.
Line range hint 86-95: Verify slashing threshold configuration
The implementation tracks validator submissions, but ensure that the slashing threshold for missed rounds is properly configured in the system parameters. This is crucial for the "Miss" case mentioned in the PR objectives.
Consider adding a comment documenting how this implementation relates to the slashing threshold configuration.
60-66:
Add error handling for AccAddressFromBech32
The error from sdk.AccAddressFromBech32(creator) should be handled to prevent potential issues with invalid addresses. This is particularly important for the slashing mechanism.
func (r recordMsg) set(creator string, feederID uint64, priceSources []*types.PriceSource) {
- accAddress, _ := sdk.AccAddressFromBech32(creator)
+ accAddress, err := sdk.AccAddressFromBech32(creator)
+ if err != nil {
+ // Consider logging the error and returning it
+ return
+ }
validator := sdk.ConsAddress(accAddress).String()
for _, price := range priceSources {
r[recordKey{validator, feederID, price.SourceID}] = price.Prices
}
}Likely invalid or redundant comment.
x/oracle/keeper/aggregator/context.go (3)
174-183: LGTM: Well-implemented performance tracking.
The message recording logic correctly captures validator performance data for slashing evaluation.
243-244: Verify slashing parameter calculations.
The comment indicates performance evaluation for missing/malicious behavior, but the implementation details are not fully specified.
375-421: LGTM: Well-structured worker management implementation.
The new methods effectively support the slashing mechanism:
GetFinalPriceListForFeederIDsefficiently retrieves final prices with proper documentation about input ordering.PerformanceReviewcorrectly implements validator performance evaluation with proper error handling and logging.RemoveWorkerprovides clean worker cleanup.
Please ensure comprehensive test coverage for these new methods, particularly edge cases in PerformanceReview.
x/oracle/module.go (5)
27-27: LGTM: Required import for slashing integration
The addition of the staking types import is necessary for the slashing implementation.
Line range hint 178-198: LGTM: Proper handling of validator set changes
The code correctly:
- Updates validator powers
- Initializes report info for new validators
- Forces round sealing when validator set changes
202-208: LGTM: Proper cleanup of closed rounds
The code properly handles cleanup by:
- Removing workers for closed rounds
- Cleaning up nonces for affected validators
240-240: Using INFRACTION_UNSPECIFIED is acceptable
Based on the learnings, using INFRACTION_UNSPECIFIED instead of specific infraction types is an intentional design choice.
Also applies to: 313-313
281-349: LGTM: Comprehensive slashing implementation
The slashing implementation correctly handles:
- Tracking missed rounds with bit array and counter
- Enforcing minimum reported rounds per window
- Applying appropriate slashing and jail duration
- Resetting counters after slashing
- Proper event emission for monitoring
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
proto/exocore/oracle/v1/params.proto (2)
32-32: Fix typo in comment: "continuous".The comment has a typo in the word "continuous".
- // for each round, a validator only allowed to provide at most max_det_id continuous rounds of prices for DS + // for each round, a validator only allowed to provide at most max_det_id consecutive rounds of prices for DS
73-73: Fix incorrect field reference in comment.The comment for
slash_fraction_maliciousincorrectly references "slash_fraction_miss".- // slash_fraction_miss defines the fraction one validator should be punished for malicious behavior + // slash_fraction_malicious defines the fraction one validator should be punished for malicious behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/oracle/types/params.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
proto/exocore/oracle/v1/params.proto(3 hunks)
🧰 Additional context used
🪛 buf
proto/exocore/oracle/v1/params.proto
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
🔇 Additional comments (2)
proto/exocore/oracle/v1/params.proto (2)
51-80: Implementation aligns well with PR objectives.
The SlashingParams message effectively implements the two slashing cases specified in the PR objectives:
- Miss case: Handled by
reported_rounds_window,min_reported_per_window,oracle_miss_jail_duration, andslash_fraction_miss - Malicious case: Handled by
oracle_malicious_jail_durationandslash_fraction_malicious
The structure is well-organized and provides flexible configuration options for both scenarios.
4-4: Verify the amino import path.
The static analysis reports that "amino/amino.proto" file does not exist. Please verify the correct import path for the amino protobuf definitions.
✅ Verification successful
Let me generate one more verification script to check if this is a third-party import pattern by examining build configuration files.
The amino import is valid and widely used across the codebase
The import of "amino/amino.proto" is correct as:
- It's consistently used across multiple proto files in the codebase (13 other files)
- The project depends on cosmos-sdk and cosmos/gogo-proto (as seen in buf.yaml), which provide these protobuf definitions
- This is a standard import pattern in Cosmos SDK based chains for amino codec support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if amino.proto exists in the repository or its dependencies
# Search for amino.proto files
echo "=== Searching for amino.proto files ==="
fd -g "*amino*.proto"
# Search for other files importing amino.proto
echo "=== Searching for amino.proto imports ==="
rg "import.*amino/amino.proto"
Length of output: 1124
Script:
#!/bin/bash
# Check for buf.yaml, buf.work.yaml, or other protobuf configuration files
echo "=== Searching for buf configuration files ==="
fd -g "buf*.y*ml"
# Display content of these files if found
echo "=== Content of buf files ==="
fd -g "buf*.y*ml" -x cat {}
Length of output: 1722
🧰 Tools
🪛 buf
4-4: import "amino/amino.proto": file does not exist
(COMPILE)
Description
This PR introduce slashing mechanism for oracle service by tracking prices provided by validators and the final prices each round.
We define two cases for slashing&jail:
Closes #XXX
Summary by CodeRabbit
Release Notes
New Features
InitValidatorReportInfo,SetValidatorReportInfo, and enhanced reporting mechanisms.SlashingParamsstructure added to manage slashing parameters effectively.ValidatorMissedRoundsandMissedRoundmessages for tracking missed reporting rounds.Improvements
Paramsstruct with new slashing parameters and validation.Documentation
Chores