Fix/predicate gas consumption #756
Conversation
WalkthroughThe pull request introduces modifications primarily in the Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 🔇 Files ignored due to path filters (5)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #756 +/- ##
==========================================
+ Coverage 54.49% 54.86% +0.37%
==========================================
Files 73 73
Lines 2905 2938 +33
==========================================
+ Hits 1583 1612 +29
- Misses 1228 1230 +2
- Partials 94 96 +2
|
ccamel
force-pushed
the
fix/predicate-gas-consumption
branch
from
a3077ca
to
fd0df01
Compare
This update enhances the gas metering mechanism by leveraging VM hooks to accurately track gas consumption for both built-in predicates and user-defined predicates.
ccamel
force-pushed
the
fix/predicate-gas-consumption
branch
from
fd0df01
to
cb8c3cd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
x/logic/interpreter/interpreter.go (2)
Line range hint
31-40: LGTM: Streamlined predicate registration with minor suggestionThe simplification of
WithPredicateby removing thehookparameter aligns well with the PR objectives. This change makes the function more focused on its primary task of registering a predicate.Consider updating the error message to be more specific:
- return fmt.Errorf("error registering predicate '%s': %w", predicate, err) + return fmt.Errorf("failed to register predicate '%s': %w", predicate, err)This minor change makes the error message slightly more precise and actionable.
Line range hint
20-91: Overall: Excellent refactoring that improves code structureThe changes in this file successfully achieve the PR objectives:
- Simplification of predicate registration by removing hook-related parameters from
WithPredicatesandWithPredicate.- Introduction of a separate
WithHooksfunction for efficient hook management.These changes result in a cleaner, more modular code structure that separates concerns between predicate registration and hook management. The refactoring improves maintainability and aligns well with the goal of streamlining the codebase while maintaining functionality.
Consider documenting these architectural changes in a comment at the top of the file or in the package documentation. This will help future developers understand the design decisions and the separation of predicate registration and hook management.
README.md (2)
123-123: Approved: Enhanced chain-upgrade command descriptionThe addition of information about passing a proposal JSON file using the PROPOSAL variable is a valuable update. This change provides users with more flexibility in initiating chain upgrades.
Consider adding a brief example of how to use the PROPOSAL variable for clarity, such as:
chain-upgrade FROM_VERSION TO_VERSION [PROPOSAL=path/to/proposal.json]This would make it even easier for users to understand how to utilize this new option.
133-133: Approved: New doc-predicate command addedThe addition of the
doc-predicatecommand for generating markdown documentation for all predicates (module logic) is a valuable enhancement. This new feature will greatly assist developers and users in understanding the system's logic, improving overall transparency and ease of use.To further improve the usefulness of this command, consider adding a brief description of where the generated documentation will be saved or how it can be accessed. For example:
doc-predicate Generate markdown documentation for all the predicates (module logic) and save it to docs/predicates/This additional information would help users locate and utilize the generated documentation more easily.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
🔇 Files ignored due to path filters (5)
docs/predicate/bech32_address_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/consult_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/current_output_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/open_4.mdis excluded by!docs/predicate/**/*.mdgo.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
- README.md (1 hunks)
- go.mod (1 hunks)
- x/logic/interpreter/instrument.go (0 hunks)
- x/logic/interpreter/interpreter.go (3 hunks)
- x/logic/interpreter/registry.go (2 hunks)
- x/logic/keeper/features/bech32_address_2.feature (2 hunks)
- x/logic/keeper/features/consult_1.feature (2 hunks)
- x/logic/keeper/features/current_output_1.feature (5 hunks)
- x/logic/keeper/features/open_4.feature (3 hunks)
- x/logic/keeper/grpc_query_ask_test.go (3 hunks)
- x/logic/keeper/interpreter.go (3 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- x/logic/interpreter/instrument.go
🧰 Additional context used
GitHub Check: codecov/patch
x/logic/keeper/interpreter.go
[warning] 140-140: x/logic/keeper/interpreter.go#L140
Added line #L140 was not covered by tests
[warning] 171-171: x/logic/keeper/interpreter.go#L171
Added line #L171 was not covered by tests
[warning] 185-186: x/logic/keeper/interpreter.go#L185-L186
Added lines #L185 - L186 were not covered by tests
🔇 Additional comments not posted (27)
x/logic/interpreter/interpreter.go (2)
20-28: LGTM: Simplified predicate registrationThe removal of the
hookparameter fromWithPredicatesaligns well with the PR objectives. This change simplifies the predicate registration process and is consistently applied throughout the function. The separation of hook handling improves the overall structure of the code.
83-91: LGTM: Well-implemented hook managementThe new
WithHooksfunction is a great addition that aligns perfectly with the PR objectives. It provides a clean and efficient way to add multiple hooks to the interpreter using theengine.CompositeHook. The implementation is concise and focused, making it easy to understand and maintain.This change effectively separates hook management from predicate registration, improving the overall structure of the code.
x/logic/keeper/features/current_output_1.feature (6)
31-31: Gas usage update aligns with PR objectives.The increase in
gas_usedfrom 4240 to 4241 is consistent with the PR's goal of implementing a gas meter hook. This slight increase reflects the additional gas consumption for metering the execution of user-defined predicates, as mentioned in the PR objectives.
69-69: Gas usage increase proportional to output length.The
gas_usedvalue increase from 4274 to 4276 is consistent with the PR objectives and the previous change. The larger increment (2 units) compared to the single-character scenario reflects the additional operations involved in writing a longer string, demonstrating that the new gas meter hook is functioning as expected.
107-107: Gas usage increase consistent despite output truncation.The
gas_usedvalue increase from 4240 to 4242 is consistent with the PR objectives and previous changes. It's noteworthy that the gas usage increased by 2 units even though the output is truncated, correctly reflecting that the full string is processed by the gas meter hook regardless of the output limit.
179-179: Substantial gas usage increase for large, unlimited output.The
gas_usedvalue increase from 4525 to 4721 (196 units) is significantly larger than in previous scenarios, which is expected given the much larger amount of text being processed without output limits. This change demonstrates that the new gas meter hook is effectively accounting for all operations, even in scenarios with unlimited output. The proportional increase in gas usage for a much larger output is consistent with the PR objectives and indicates that the gas metering is functioning as intended for varying output sizes.
Line range hint
1-185: Summary: Gas usage updates align with PR objectives across all scenarios.The changes in this feature file consistently demonstrate increased gas usage across various scenarios, which aligns well with the PR objectives of implementing a new gas meter hook. The varying magnitudes of these increases reflect the hook's sensitivity to different types of operations, output sizes, and character encodings. This comprehensive update to the expected gas usage values ensures that the test suite accurately reflects the new gas metering behavior, contributing to the overall goal of improving security and efficiency in predicate execution.
148-148: Significant gas usage increase for UTF-8 character handling.The
gas_usedvalue increase from 4254 to 4263 (9 units) is notably larger than in previous scenarios. While this aligns with the PR objectives and likely reflects the additional complexity of handling UTF-8 characters, particularly emojis, it might be worth verifying that this increase is proportionate to the actual computational cost.To ensure the gas usage increase for UTF-8 characters is appropriate, please run the following script:
This script will help us compare the gas usage for ASCII vs UTF-8 characters and examine any specific gas calculation logic for character handling.
x/logic/keeper/features/consult_1.feature (3)
37-37: LGTM: Gas usage increase aligns with PR objectives.The increase in
gas_usedfrom 4142 to 4143 is consistent with the PR's goal of expanding gas consumption tracking to include user-defined predicates. This minimal increase (1 unit) reflects the expected slight increase in overall gas consumption mentioned in the PR objectives.
93-93: LGTM: Consistent gas usage increase.The increase in
gas_usedfrom 4141 to 4142 is consistent with the previous change and aligns with the PR's objective of expanding gas consumption tracking. This minimal increase (1 unit) maintains the expected slight increase in overall gas consumption across different scenarios.
Line range hint
170-170: Verify: Unchanged gas usage in the third scenario.The gas usage values have been updated in the first two scenarios, but the third scenario's
gas_usedvalue remains unchanged at 4141. Could you please verify if this is intentional or if it should also be updated to maintain consistency across all scenarios?x/logic/interpreter/registry.go (2)
134-139: LGTM: NewIsRegisteredfunction looks good.The
IsRegisteredfunction is a well-implemented addition that aligns with the PR objectives. It provides a clean way to check if a predicate is registered, which could be useful for the mentioned whitelist/blacklist functionality.
144-144: Clarification needed: Removal ofhookparameter and its impact on gas meteringThe
Registerfunction has been simplified by removing thehookparameter. However, this change seems to contradict the PR objectives, which mention introducing VM hooks for gas metering, particularly for user-defined predicates.Could you please clarify:
- How will gas metering be implemented for user-defined predicates without the
hookparameter?- Does this change impact the ability to track gas consumption during predicate execution, as mentioned in the PR objectives?
- Is there an alternative mechanism in place to associate hooks with predicates for gas metering purposes?
To help verify the impact of this change, could you run the following script to check for any other occurrences of hook-related functionality in the codebase?
Also applies to: 156-156, 158-158, 160-160, 162-162, 164-164, 166-166, 168-168, 170-170, 172-172
x/logic/keeper/features/open_4.feature (4)
46-46: Gas usage increase aligns with PR objectives.The increase in
gas_usedfrom 4146 to 4153 is consistent with the PR objectives, which mention a slight increase in overall gas consumption due to the inclusion of additional predicates in the metering process. This change of 7 gas units is relatively small and aligns with the expected "slight increase".
89-89: Gas usage increase consistent with PR objectives.The increase in
gas_usedfrom 4142 to 4144 is in line with the PR objectives. This change of 2 gas units is minimal and consistent with the expected slight increase in gas consumption due to the inclusion of additional predicates in the metering process.
130-130: Gas usage increase consistent with PR objectives and previous scenario.The increase in
gas_usedfrom 4142 to 4144 is consistent with both the PR objectives and the previous scenario. This 2 gas unit increase is minimal and aligns with the expected slight increase in gas consumption. The consistency between this scenario and the previous one is logical, as they perform similar operations.
46-46: Summary: Gas usage increases align with PR objectives across scenarios.The changes in this file consistently show small increases in gas usage across different scenarios:
- "Open a resource for reading": +7 gas units
- "Open an existing resource and read its content": +2 gas units
- "Open an existing resource and read its content (base64-encoded)": +2 gas units
These small, consistent increases align well with the PR objectives, which mention a slight increase in overall gas consumption due to the inclusion of additional predicates in the metering process. The changes preserve the original test coverage and intentions of the scenarios while reflecting the improved gas metering system.
Also applies to: 89-89, 130-130
README.md (1)
123-133: Overall improvement in documentation and usabilityThe changes made to the README.md file significantly enhance the documentation and usability of the system. The updated
chain-upgradecommand description and the newdoc-predicatecommand both contribute to better understanding and utilization of the system's features, particularly in relation to predicate execution and chain upgrades.These improvements align well with the PR objectives of addressing predicate execution and gas consumption tracking. They demonstrate a commitment to maintaining clear and up-to-date documentation, which is crucial for the project's adoption and community engagement.
x/logic/keeper/features/bech32_address_2.feature (3)
112-112: Approved: Gas usage increase aligns with PR objectivesThe increase in
gas_usedfrom 4140 to 4141 is consistent with the PR objectives. This slight increase (1 unit) in gas consumption is expected due to the inclusion of additional predicates in the new gas metering process. The change doesn't affect the logic or functionality of the test scenario.
133-133: Approved: Consistent gas usage increase across scenariosThe increase in
gas_usedfrom 4140 to 4141 in this scenario is consistent with the previous change. This uniformity across different scenarios (success and not success cases) supports the validity of the new gas metering implementation. The change doesn't affect the logic or functionality of the test scenario.
Line range hint
1-379: Summary: Gas usage updates consistent with PR objectivesThe two changes in this file (lines 112 and 133) consistently update the
gas_usedvalue from 4140 to 4141 across different scenarios. These changes align with the PR objectives of implementing a new gas metering process that includes additional predicates.To ensure consistency across the codebase, please run the following script to check if similar updates have been made in other feature files:
This will help verify that the gas usage updates have been applied consistently across all relevant scenarios in the codebase.
✅ Verification successful
Summary: Gas usage updates are consistent across all feature files
All relevant feature files have been correctly updated to
gas_used: 4141, ensuring consistency with the PR objectives for the new gas metering implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for gas_used updates in other feature files # Search for gas_used changes in feature files echo "Checking for gas_used changes in feature files:" rg --type gherkin -C 2 'gas_used: 4141' x/logic/keeper/features/Length of output: 203
Script:
#!/bin/bash # Description: Check for gas_used updates in other feature files # Search for 'gas_used: 4141' in all .feature files within the specified directory echo "Checking for gas_used: 4141 in .feature files:" rg -g '*.feature' 'gas_used: 4141' x/logic/keeper/features/Length of output: 561
go.mod (1)
79-79: Approved: Updated Prolog interpreter dependency. Verify compatibility.The update to the custom fork of the Prolog interpreter (from
v1.0.0tov1.0.1-0.20240924120526-53584b2b5c0b) aligns with the PR objectives of utilizing the VM hook mechanism. This change likely introduces the necessary features for implementing the gas meter and whitelist/blacklist hooks.To ensure the stability and compatibility of this update, please run the following verification steps:
Please review the output of these commands to ensure that:
- The changes in the Prolog interpreter align with your expectations and requirements.
- The new version is being used correctly throughout the codebase.
If you need any assistance interpreting the results or making further adjustments, please let me know.
✅ Verification successful
Verified: Prolog interpreter dependency updated correctly.
The dependency update successfully replaces
github.com/ichiban/prologwithgit.luolix.top/axone-protocol/prologat versionv1.0.1-0.20240924120526-53584b2b5c0b, aligning with the PR objectives to implement VM hook support. All imports using the original path are correctly redirected to the forked version via the replace directive, ensuring compatibility and stability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changes introduced by the new Prolog interpreter version # Test: Check for any breaking changes or new features in the Prolog interpreter echo "Checking for changes in the Prolog interpreter:" gh api repos/axone-protocol/prolog/compare/v1.0.0...53584b2b5c0b --jq '.commits[].commit.message' # Test: Verify that the new version is being used correctly in the codebase echo "Verifying usage of the new Prolog interpreter version:" rg --type go "github.com/ichiban/prolog"Length of output: 3922
x/logic/keeper/interpreter.go (3)
105-109: Ensure the order of hooks does not affect functionality.In the
WithHooksoption, multiple hooks are added:interpreter.WithHooks( whitelistBlacklistHookFn(whitelistPredicates, blacklistPredicates), gasMeterHookFn(sdkctx, params.GetGasPolicy()), ),Confirm that the order in which
whitelistBlacklistHookFnandgasMeterHookFnare applied does not introduce unintended side effects, as the execution order of hooks may influence the interpreter's behavior.
105-109: Overall enhancement of modularity and readability.The refactoring of hooks into separate functions
whitelistBlacklistHookFnandgasMeterHookFnimproves the modularity and clarity of the code. This separation of concerns makes the codebase more maintainable and easier to extend in the future.
12-12: Consider the necessity of introducing theorderedmapdependency.The addition of
orderedmapfromgit.luolix.top/wk8/go-ordered-map/v2introduces a new external dependency. Evaluate if this is essential for maintaining the order of predicates or if a standard Go map suffices, considering that Go maps do not guarantee order but are more lightweight.To verify the usage of
orderedmap, you can check all instances where it's utilized:✅ Verification successful
The
orderedmapdependency is essential for maintaining order in both the interpreter and registry components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `orderedmap` in the codebase. # Test: Search for `orderedmap` usage. Expect: Only necessary usages. rg --type go 'orderedmap'Length of output: 649
x/logic/keeper/grpc_query_ask_test.go (2)
169-174: Test case for recursive predicate handles out-of-gas error correctlyThe test case for
recursionOfDeathappropriately checks that a recursive predicate exceeding the gas limit results in the expected out-of-gas error. The error message accurately reflects the gas usage exceeding the limit.
175-180: Backtracking gas limit test case is correctly implementedThe test case for
backtrackOfDeatheffectively tests gas consumption during backtracking. The expected error message confirms that the gas limit enforcement works as intended when the limit is exceeded.
| if !interpreter.IsRegistered(predicate) { | ||
| return defaultCost | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the lookupCost function for unregistered predicates.
In the lookupCost function:
if !interpreter.IsRegistered(predicate) {
return defaultCost
}Consider whether unregistered predicates should default to the defaultCost or if a different cost should be applied. Additionally, evaluate if caching the costs of predicates could improve performance, especially if lookupCost is called frequently during execution.
There was a problem hiding this comment.
That’s exactly the intended behavior. Unregistered predicates are user-defined, and a default gas consumption needs to be applied to them.
However, it looks like we’re using a hard-coded constant when we should actually be referencing the module’s default_predicate_cost configuration value instead. It should be fixed.
There was a problem hiding this comment.
@ccamel, you're absolutely right. The lookupCost function should reference the module's default_predicate_cost configuration value instead of using the hard-coded defaultCost. This ensures that the gas cost for unregistered (user-defined) predicates is configurable as per the module's parameters.
You can modify the lookupCost function to use the gasPolicy.DefaultPredicateCost value:
func lookupCost(predicate string, costs []types.PredicateCost, gasPolicy types.GasPolicy) uint64 {
defaultCost := nonNilNorZeroOrDefaultUint64(gasPolicy.DefaultPredicateCost, defaultPredicateCost)
if !interpreter.IsRegistered(predicate) {
return defaultCost
}
for _, c := range costs {
if prolog2.PredicateMatches(predicate)(c.Predicate) {
return nonNilNorZeroOrDefaultUint64(c.Cost, defaultCost)
}
}
return defaultCost
}This way, defaultCost will reflect the configuration value specified in gasPolicy.DefaultPredicateCost.
Don't forget to update the function signature and any calls to lookupCost to include gasPolicy:
func gasMeterHookFn(ctx context.Context, gasPolicy types.GasPolicy) engine.HookFunc {
// ...
return func(opcode engine.Opcode, operand engine.Term, env *engine.Env) (err error) {
// ...
cost := lookupCost(predicate, gasPolicy.PredicateCosts, gasPolicy)
// ...
}
}✏️ Learnings added
Learnt from: ccamel
PR: axone-protocol/axoned#756
File: x/logic/keeper/interpreter.go:197-200
Timestamp: 2024-09-25T07:11:25.645Z
Learning: Unregistered predicates are user-defined and should have a default gas consumption applied, referencing the module's `default_predicate_cost` configuration value instead of a hard-coded constant.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
If you don't mind, I don’t want to address this issue in this PR, as I want to keep it iso-functionality for now. Could I ask you to open a ticket for this? We’ll handle it in a future update. Thanks!
There was a problem hiding this comment.
@ccamel, I've created a new issue as per your request: Use configuration value for default gas consumption in 'lookupCost' function. We'll handle it in a future update.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue as per the user's request
gh issue create \
--title "Use configuration value for default gas consumption in 'lookupCost' function" \
--body "As requested by @ccamel, we need to update the \`lookupCost\` function to reference the module's \`default_predicate_cost\` configuration value instead of using a hard-coded constant. This change will ensure that the gas cost for unregistered (user-defined) predicates is configurable according to the module's parameters.
PR URL: https://github.com/axone-protocol/axoned/pull/756
Comment URL: https://github.com/axone-protocol/axoned/pull/756#discussion_r1774589991
Requested by: @ccamel" \
--assignee ccamel
Length of output: 683
🧠 Learnings used
Learnt from: ccamel
PR: axone-protocol/axoned#756
File: x/logic/keeper/interpreter.go:197-200
Timestamp: 2024-09-25T07:11:26.077Z
Learning: Unregistered predicates are user-defined and should have a default gas consumption applied, referencing the module's `default_predicate_cost` configuration value instead of a hard-coded constant.
There was a problem hiding this comment.
Awesome! Thanks.
Just one more thing—since you pointed out the performance issue with the lookupCost function, could you open a ticket to assess potential performance improvements for it? Thanks.
There was a problem hiding this comment.
@ccamel, I've created a new issue to assess potential performance improvements for the lookupCost function as per your request: Assess potential performance improvements for 'lookupCost' function. We'll handle it in a future update.
(_/)
(•_•)
/ >🎉
🔗 Analysis chain
@ccamel, I've created a new issue to assess potential performance improvements for the lookupCost function as per your request: Assess potential performance improvements for 'lookupCost' function. We'll handle it in a future update.
(\_/)
(•_•)
/ >🎉
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue as per the user's request
gh issue create \
--title "Assess potential performance improvements for 'lookupCost' function" \
--body "As requested by @ccamel, we need to assess potential performance improvements for the \`lookupCost\` function. Caching the costs of predicates could improve performance, especially if \`lookupCost\` is called frequently during execution.
PR URL: https://github.com/axone-protocol/axoned/pull/756
Comment URL: https://github.com/axone-protocol/axoned/pull/756#discussion_r1774589991
Requested by: @ccamel" \
--assignee ccamel
Length of output: 583
ccamel
force-pushed
the
fix/predicate-gas-consumption
branch
from
cb8c3cd
to
a1aa708
Compare
Fixes and improves the tracking of gas consumption during predicate execution by using the VM hook mechanism introduced by axone-protocol/prolog#8.
Mainly 2 VM hooks have been added.
gas meter hook
Previously, only built-in predicates were tracked, while user-defined predicates were not, leading to a significant security risk that could result in uncontrolled memory growth or infinite execution without termination (see tests). By using a VM hook for gas metring, execution can be more effectively monitored and halted when gas consumption is exceeded.
Note that this results in a slight increase in gas consumption, as newly counted predicates are now included in the metering (see the adjustments made to the gas meters in the tests).
whitelist / blacklist hook
The built-in predicate whitelist/blacklist mechanism has been reimplemented using a dedicated VM hook, maintaining the same functionality as before. This greatly simplifies the code by removing the old approach of decorating built-in predicates.
Summary by CodeRabbit
New Features
chain-upgradecommand to include the ability to pass a proposal JSON file.doc-predicatefor generating markdown documentation for predicates.Bug Fixes
gas_usedvalues in multiple feature files to reflect accurate metrics.Documentation
README.mdto clarify command usage and removed obsolete commands (proto-formatandproto-build).Tests