rpc: enforce max_events limit for ForwardingHistory #10512
Conversation
Summary of ChangesHello @fresh3nough, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a crucial validation step for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of enforcing a max_events limit for the ForwardingHistory RPC, preventing potential gRPC message size errors. The introduction of the maxForwardingEvents constant and its validation within the ForwardingHistory method are correctly implemented. Comprehensive unit tests have been added to cover various scenarios, ensuring the robustness of the new validation logic.
rpcserver_test.go
Outdated
| } else if tt.maxEvents != 0 && tt.maxEvents <= maxForwardingEvents { | ||
| // For valid non-zero values, we expect a different | ||
| // error (htlcSwitch nil), not the max events error. | ||
| if err != nil { | ||
| require.NotContains(t, err.Error(), | ||
| "exceeds maximum allowed") | ||
| } | ||
| } |
There was a problem hiding this comment.
This else if condition is a bit complex. It might be clearer to separate the tt.maxEvents != 0 and tt.maxEvents <= maxForwardingEvents checks into distinct if blocks or simplify the condition if possible. For instance, if tt.maxEvents == 0, the numEvents defaults to 100, which is within limit, so it should not expect an error. This could be handled as a separate if tt.maxEvents == 0 case.
} else if tt.maxEvents == 0 {
// For zero maxEvents, it defaults to 100, which is within
// the limit, so we expect no max events error.
if err != nil {
require.NotContains(t, err.Error(),
"exceeds maximum allowed")
}
} else if tt.maxEvents <= maxForwardingEvents {
// For valid non-zero values, we expect a different
// error (htlcSwitch nil), not the max events error.
if err != nil {
require.NotContains(t, err.Error(),
"exceeds maximum allowed")
}
}
|
Perhaps better to consolidate, or help to improve/review: #10503 ? |
The API documentation states that ForwardingHistory responses can only contain 50k records, but this limit was not actually enforced in the code. This change adds validation to return an error when a client requests more than the maximum allowed events, rather than silently capping or allowing potentially oversized responses. This implementation uses the existing channeldb.MaxResponseEvents constant to avoid code duplication and ensure the limit is defined in a single place. Comprehensive unit tests cover various scenarios including valid values, boundary conditions, and exceeding the limit. This change consolidates the approaches from PR lightningnetwork#10503 and lightningnetwork#10512, combining the error-returning validation with the use of the existing channeldb constant. Closes lightningnetwork#10496 Co-Authored-By: Warp <agent@warp.dev>
fresh3nough
force-pushed
the
cody/fix/10496
branch
from
78d18d8 to
d1f91ad
Compare
|
I've consolidated this PR with #10503, combining the best approaches from both:
The updated implementation has been pushed to this branch. This approach provides better error handling while maintaining code consistency with the existing codebase. |
2 participants
Fixes #10496
The ForwardingHistory RPC documentation states that the maximum number of events that can be returned is 50,000 to avoid exceeding the 4 MiB gRPC message size limit. However, this limit was not actually enforced in the code, allowing users to request arbitrarily large numbers of events which could cause gRPC message size errors.
This commit:
Change Description
Description of change / link to associated issue.
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.