imp: add flat fee for queries to VerifyMembership
#5927
Conversation
WalkthroughThe update introduces a new gas consumption logic in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
charleenfei
requested review from
colin-axner,
AdityaSripal and
chatton
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| return nil, status.Error(codes.FailedPrecondition, errorsmod.Wrapf(types.ErrClientNotActive, "cannot verify membership using client (%s) with status %s", req.ClientId, clientStatus).Error()) | ||
| } | ||
|
|
||
| // consume flat gas fee for proof verification queries | ||
| ctx.GasMeter().ConsumeGas( | ||
| 3*ctx.KVGasConfig().ReadCostPerByte*uint64(len(req.Proof)), | ||
| "verify membership query", | ||
| ) | ||
|
|
||
| if err := clientState.VerifyMembership(cachedCtx, k.ClientStore(cachedCtx, req.ClientId), k.cdc, req.ProofHeight, req.TimeDelay, req.BlockDelay, req.Proof, req.MerklePath, req.Value); err != nil { | ||
| k.Logger(ctx).Debug("proof verification failed", "key", req.MerklePath, "error", err) | ||
| return &types.QueryVerifyMembershipResponse{ |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [372-395]
The addition of a flat gas fee consumption mechanism based on the length of req.Proof in the VerifyMembership function is a significant change aimed at preventing potential abuse scenarios. However, there are a few points to consider for refinement:
-
Gas Consumption Calculation: The formula
3*ctx.KVGasConfig().ReadCostPerByte*uint64(len(req.Proof))is used to calculate the gas consumed. It's crucial to ensure that the multiplier3and the usage ofReadCostPerByteaccurately reflect the intended gas consumption model for proof verification. If the constants and multipliers are derived from empirical data or specific reasoning, documenting this within the code or in the PR description would enhance clarity and maintainability. -
Error Handling and Logging: The function correctly handles errors and logs the failure of proof verification. However, it might be beneficial to also log the gas consumption for successful and unsuccessful queries to aid in debugging and monitoring the system's behavior under different load conditions.
-
Performance Considerations: While the focus is on security and preventing abuse, it's also important to consider the performance implications of the new gas consumption logic, especially in high-throughput scenarios. Ensuring that the gas metering does not introduce significant overhead or latency in the
VerifyMembershipquery processing is crucial. Performance testing or benchmarking results, if available, should be discussed in the PR to validate the impact of these changes. -
Documentation and Comments: Adding more detailed comments explaining the rationale behind the chosen gas consumption formula and any potential edge cases or limitations of the current implementation would be helpful. This documentation can assist future maintainers in understanding the context and reasoning behind these changes.
Overall, the implementation aligns with the PR's objectives to introduce a flat fee for proof verification queries, enhancing the system's resilience against potential abuse. Further refinement and documentation could improve the clarity, maintainability, and performance of this feature.
…rifymembership-rpc
|
| @@ -386,6 +387,12 @@ func (k Keeper) VerifyMembership(c context.Context, req *types.QueryVerifyMember | |||
| return nil, status.Error(codes.FailedPrecondition, errorsmod.Wrapf(types.ErrClientNotActive, "cannot verify membership using client (%s) with status %s", req.ClientId, clientStatus).Error()) | |||
| } | |||
|
|
|||
| // consume flat gas fee for proof verification queries | |||
There was a problem hiding this comment.
do we want to mention the reasoning here? (protection against clients recursively calling each other)
There was a problem hiding this comment.
Personally I think its fine to just state that we're consuming a flat fee for proof verification, as its cpu intensive and not based around IOPS, which is the main gas consumption factor in the cosmos sdk.
Pretty sure all VerifyMembership calls within the client state will have to look up a root to prove against based on the provided height, so there would be gas consumed on the cacheCtx for that store access which should also protection against recursive client calls, but this just adds an extra bit of gas consumption which is definitely no harm and the right call imo!
There was a problem hiding this comment.
but wouldn't that gas on the cached context only be consumed when function returns, i.e in defer? (which, in the recursive case, is what doesn't happen)
I don't want to appear waaay too nitty though 😅, fine w/o it if you agree!
There was a problem hiding this comment.
You're 100% correct! Great point!
Happy to add a note if others agree too, maybe something like:
| // consume flat gas fee for proof verification queries | |
| // consume flat gas fee for proof verification queries. | |
| // NOTE: consuming gas prior to method invocation also provides protection against recursive calls reaching stack overflow |
There was a problem hiding this comment.
great suggestion! ill update this, thanks for the reviews and reasonings @damiannolan @DimitrisJim
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- modules/core/02-client/keeper/grpc_query.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/core/02-client/keeper/grpc_query.go
…rifymembership-rpc
…rifymembership-rpc
|
charleenfei
deleted the
charly/issue#5902-consume-flat-gas-fee-on-verifymembership-rpc
branch
Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 9aa7151)
Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 9aa7151) Co-authored-by: Charly <[email protected]>
Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 9aa7151) # Conflicts: # modules/core/02-client/keeper/grpc_query.go
Co-authored-by: Damian Nolan <[email protected]> (cherry picked from commit 9aa7151) # Conflicts: # modules/core/02-client/keeper/grpc_query.go Co-authored-by: Charly <[email protected]> Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>
Description
see cosmos/cosmos-sdk#9995 for similar flat gas consumption fee logic.
closes: #5902
i elected to consume the gas outside of the defer func as we are not operating on the cached context in this case, and simply consuming gas for the read on consensus state from the store...but open to other opinions if people have any!
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit