Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: max-event-size config to limit size of events stored #614

Closed
wants to merge 8 commits into from
Closed

feat!: max-event-size config to limit size of events stored #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d792e23
max event size config
czarcas7ic Jun 17, 2024
8a92ab5
change location
czarcas7ic Jun 17, 2024
1ac9925
add prints
czarcas7ic Jun 17, 2024
7bc6ee1
fix
czarcas7ic Jun 17, 2024
57a2d27
clean up
czarcas7ic Jun 17, 2024
67e8dff
clean
czarcas7ic Jun 17, 2024
59c579b
lint
czarcas7ic Jun 18, 2024
9d78cb4
add changelog
czarcas7ic Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

## v24

## [v0.47.5-v25-osmo-2](https://github.com/osmosis-labs/cosmos-sdk/releases/tag/v0.47.5-v25-osmo-1)

* (server) [#614](https://github.com/osmosis-labs/cosmos-sdk/pull/614) Add `max-event-size` config to app.toml to limit the size of events stored.

## [v0.47.5-v24-osmo-6](https://github.com/osmosis-labs/cosmos-sdk/releases/tag/v0.47.5-v24-osmo-6)

* (perf) [#601](https://github.com/osmosis-labs/cosmos-sdk/pull/601) Don't run ValidateBasic on recheck
Expand Down
12 changes: 12 additions & 0 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,18 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliv
return sdkerrors.ResponseDeliverTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, sdk.MarkEventsToIndex(anteEvents, app.indexEvents), app.trace)
}

// 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"
}
}
}
}

Comment on lines +415 to +426
Copy link

@coderabbitai coderabbitai bot Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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"
}
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

return abci.ResponseDeliverTx{
GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints?
GasUsed: int64(gInfo.GasUsed), // TODO: Should type accept unsigned ints?
Expand Down
6 changes: 6 additions & 0 deletions server/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ type BaseConfig struct {
// AppDBBackend defines the type of Database to use for the application and snapshots databases.
// An empty string indicates that the Tendermint config's DBBackend value should be used.
AppDBBackend string `mapstructure:"app-db-backend"`

// MaxEventSize defines the maximum size of an attribute (key + value) of an event that will be stored in the block results.
// If an attribute is larger than this size, it will be replaced with a placeholder in the block results.
MaxEventSize int `mapstructure:"max-event-size" json:"max-event-size"`
}

// APIConfig defines the API listener configuration.
Expand Down Expand Up @@ -316,6 +320,7 @@ func DefaultConfig() *Config {
IAVLFastNodeModuleWhitelist: []string{"lockup"},
IAVLLazyLoading: false,
AppDBBackend: "",
MaxEventSize: 1000000, // 1MB
},
Telemetry: telemetry.Config{
Enabled: false,
Expand Down Expand Up @@ -381,6 +386,7 @@ func GetConfig(v *viper.Viper) (Config, error) {
if err := v.Unmarshal(conf); err != nil {
return Config{}, fmt.Errorf("error extracting app config: %w", err)
}
sdk.MaxEventSize = conf.BaseConfig.MaxEventSize
return *conf, nil
}

Expand Down
6 changes: 5 additions & 1 deletion server/config/toml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ index-events = [{{ range .BaseConfig.IndexEvents }}{{ printf "%q, " . }}{{end}}]
# IavlCacheSize set the size of the iavl tree cache (in number of nodes).
iavl-cache-size = {{ .BaseConfig.IAVLCacheSize }}

# IAVLDisableFastNode enables or disables the fast node feature of IAVL.
# IAVLDisableFastNode enables or disables the fast node feature of IAVL.
# Default is false.
iavl-disable-fastnode = {{ .BaseConfig.IAVLDisableFastNode }}

Expand All @@ -96,6 +96,10 @@ iavl-lazy-loading = {{ .BaseConfig.IAVLLazyLoading }}
# The fallback is the db_backend value set in Tendermint's config.toml.
app-db-backend = "{{ .BaseConfig.AppDBBackend }}"

# Maximum event size in bytes that is stored in the block results. 0 means no limit.
# If an attribute of an event is larger than the max size, the attribute will be replaced with a placeholder.
max-event-size = {{ .BaseConfig.MaxEventSize }}

###############################################################################
### Telemetry Configuration ###
###############################################################################
Expand Down
2 changes: 2 additions & 0 deletions types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
)

var MaxEventSize = 0

// ----------------------------------------------------------------------------
// Event Manager
// ----------------------------------------------------------------------------
Expand Down
Loading