feat!: max-event-size config to limit size of events stored
#614
Conversation
czarcas7ic
changed the title
max-event-size config to limit size of events stored
WalkthroughThe recent update introduces a new configuration parameter Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AppConfig as App Config
participant BaseApp as BaseApp
participant SDK
User->>AppConfig: Set max-event-size in app.toml
AppConfig->>SDK: Initialize MaxEventSize
BaseApp->>SDK: DeliverTx request
SDK->>SDK: Check event attribute size
alt Event size exceeds MaxEventSize
SDK->>SDK: Replace with default behavior
else Event size is within limit
SDK->>SDK: Process normally
end
SDK->>BaseApp: Return response
BaseApp->>User: DeliverTx response
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review due to trivial changes (1)
Additional context usedLanguageTool
Markdownlint
Additional comments not posted (5)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // If the application has set a maximum event size, check the size of each event attribute. | ||
| // If the size of any attribute exceeds the maximum, set the attribute value to a placeholder. | ||
| if sdk.MaxEventSize > 0 { | ||
| for i, event := range result.Events { | ||
| for j, attr := range event.Attributes { | ||
| if len([]byte(attr.Key))+len([]byte(attr.Value)) > sdk.MaxEventSize { | ||
| result.Events[i].Attributes[j].Value = "evt val too large, inc max-event-size in config.toml" | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider optimizing the event size checking logic for efficiency.
The current implementation checks each attribute's size individually against sdk.MaxEventSize. This could be optimized by calculating the total size of all attributes in an event first, and then checking if the total exceeds sdk.MaxEventSize. This would reduce the number of comparisons and potentially improve performance, especially for events with many attributes.
- if sdk.MaxEventSize > 0 {
- for i, event := range result.Events {
- for j, attr := range event.Attributes {
- if len([]byte(attr.Key))+len([]byte(attr.Value)) > sdk.MaxEventSize {
- result.Events[i].Attributes[j].Value = "evt val too large, inc max-event-size in config.toml"
- }
- }
- }
- }
+ if sdk.MaxEventSize > 0 {
+ for i, event := range result.Events {
+ totalSize := 0
+ for _, attr := range event.Attributes {
+ totalSize += len([]byte(attr.Key)) + len([]byte(attr.Value))
+ }
+ if totalSize > sdk.MaxEventSize {
+ for j := range event.Attributes {
+ result.Events[i].Attributes[j].Value = "evt val too large, inc max-event-size in config.toml"
+ }
+ }
+ }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If the application has set a maximum event size, check the size of each event attribute. | |
| // If the size of any attribute exceeds the maximum, set the attribute value to a placeholder. | |
| if sdk.MaxEventSize > 0 { | |
| for i, event := range result.Events { | |
| for j, attr := range event.Attributes { | |
| if len([]byte(attr.Key))+len([]byte(attr.Value)) > sdk.MaxEventSize { | |
| result.Events[i].Attributes[j].Value = "evt val too large, inc max-event-size in config.toml" | |
| } | |
| } | |
| } | |
| } | |
| // If the application has set a maximum event size, check the size of each event attribute. | |
| // If the size of any attribute exceeds the maximum, set the attribute value to a placeholder. | |
| if sdk.MaxEventSize > 0 { | |
| for i, event := range result.Events { | |
| totalSize := 0 | |
| for _, attr := range event.Attributes { | |
| totalSize += len([]byte(attr.Key)) + len([]byte(attr.Value)) | |
| } | |
| if totalSize > sdk.MaxEventSize { | |
| for j := range event.Attributes { | |
| result.Events[i].Attributes[j].Value = "evt val too large, inc max-event-size in config.toml" | |
| } | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
There are instances where we want the smaller attributes of an event but remove the larger instances, and this would not work in that way
There was a problem hiding this comment.
@czarcas7ic, thank you for your feedback. I understand your requirement for a more nuanced approach where only the attributes that exceed the sdk.MaxEventSize limit are replaced, while smaller ones are preserved.
Here's a revised approach that checks each attribute individually and replaces only those that exceed the maximum allowed size:
// If the application has set a maximum event size, check the size of each attribute individually.
// If the size of an attribute exceeds the maximum, set the attribute value to a placeholder.
if sdk.MaxEventSize > 0 {
for i, event := range result.Events {
for j, attr := range event.Attributes {
if len([]byte(attr.Key))+len([]byte(attr.Value)) > sdk.MaxEventSize {
result.Events[i].Attributes[j].Value = "evt val too large, inc max-event-size in config.toml"
}
}
}
}This implementation ensures that only the attributes which are too large are affected, and it aligns with your need to selectively manage event attributes based on their size. Please let me know if this adjustment suits your scenario, or if further modifications are required!
Description
Closes: #XXXX
We have recently had issues with massive IBC events being persisted in block results, resulting in slowed down block syncing / consensus. Even after we upgrade to IBC v8, it's likely a matter of time before some other massive events exploit occurs that will bog down nodes once again. We should allow nodes to specify how large events they want to store, and if an event is greater than this size, a placeholder is set.
This PR introduces a
max-event-sizeconfig to theapp.toml. If an event attribute's size is greater than this value, it gets replaced by a placeholder. I think this implies a client breaking change since the expectation of events is now changed, but if it's not considered breaking than I will remove the "!" from the title.