coderabbitai[bot]
commented
3 months ago Walkthrough
The recent update introduces a new configuration parameter max-event-size to manage the maximum size of events stored in the server. This feature involves updates to configuration files, functions, and logic in various parts of the application, ensuring oversized event attributes are appropriately handled and restricted by the defined limit.
Changes
| File | Change Summary |
|---|---|
CHANGELOG.md, .../config/toml.go |
Added max-event-size to configuration files for controlling event size. |
baseapp/abci.go |
Enhanced DeliverTx to check and replace event attributes exceeding sdk.MaxEventSize. |
server/config/config.go |
Updated BaseConfig struct and related functions to incorporate MaxEventSize field. |
types/events.go |
Introduced a new global variable MaxEventSize with an initial value of 0. |
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 responsePoem
Out in the fields, the code does change,
With limits set, events arrange,
Size constraints to keep things neat,
A milestone in tasks complete.🐇✨
Configured bytes, the rabbit's feat,
In toml files, success we greet!
Recent review details
**Configuration used: CodeRabbit UI** **Review profile: CHILL**Commits
Files that changed from the base of the PR and between df7a4784e24d21bbabb2c8542f68c3b5b62cbab0 and 9d78cb4da1aa00f03584c4b9b0f269b24b8a2ac3.Files selected for processing (5)
* CHANGELOG.md (1 hunks) * baseapp/abci.go (1 hunks) * server/config/config.go (3 hunks) * server/config/toml.go (2 hunks) * types/events.go (1 hunks)Files skipped from review due to trivial changes (1)
* types/events.goAdditional context used
LanguageTool
CHANGELOG.md
[uncategorized] ~148-~148: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ...itive to the `genesis migrate` command. However in v0.50+, adding custom migrations to ... --- [grammar] ~153-~153: Did you mean “combining”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO) Context: ...s/cosmos-sdk/pull/17284) Properly allow to combine depinject-enabled modules and non-depin... --- [grammar] ~156-~156: Probably a preposition is missing after ‘try’. (ATD_VERBS_TO_COLLOCATION) Context: ...om/cosmos/cosmos-sdk/pull/17220) Do not try validate `msgURL` as web URL in `draft-proposal`... --- [misspelling] ~201-~201: Did you mean the phrasal verb “clean up” instead of the noun ‘cleanup’? (CLEAN_UP) Context: ...` method to `BaseApp` for custom app to cleanup resource in graceful shutdown. ### Bug... --- [uncategorized] ~213-~213: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: .... Chains will not notice this breaking change as this package contains testing utilit... --- [style] ~220-~220: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM) Context: ...' pubkey check in the signature handler in order to work with ICS. * (deps) [#15957](https:... --- [uncategorized] ~223-~223: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ...w be set. Note, when querying against a node it must be re-synced in order to be abl... --- [style] ~223-~223: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM) Context: ...ing against a node it must be re-synced in order to be able to automatically populate the b... --- [grammar] ~272-~272: The verb ‘helps’ is used with an infinitive. (ADVISE_VBG) Context: ...elper func in x/auth module which helps adding accounts to genesis state. * (x/authz) ... --- [style] ~281-~281: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM) Context: ...stead of `x/param`. Legacy types remain in order to facilitate parameter migration from the... --- [style] ~292-~292: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’. (HYPHENATED_LY_ADVERB_ADJECTIVE) Context: ...t path (i.e. the OS path) matches their fully-qualified package name. For example, proto files ... --- [style] ~293-~293: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM) Context: ...ow use deliverState for the first block in order to access changes made in InitChain. * (x/... --- [uncategorized] ~304-~304: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ...ims`. These takes an extra genesisState argument which is the default state of the app. ... --- [misspelling] ~315-~315: This word is normally spelled as one. (EN_COMPOUNDS_SUB_MODULE) Context: ...le` API to support module addresses and sub-module addresses in a backward compatible way.... --- [uncategorized] ~315-~315: If this is a compound adjective that modifies the following noun, use a hyphen. (EN_COMPOUND_ADJECTIVE_INTERNAL) Context: ...addresses and sub-module addresses in a backward compatible way. * (snapshots) [#14608](https://git... --- [duplication] ~324-~324: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE) Context: .../cosmos/cosmos-sdk/pull/14163) Refactor `(coins Coins) Validate()` to avoid unnecessary map. ... --- [uncategorized] ~340-~340: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ...dParams` from the `AppModuleSimulation` interface which is no longer needed. * (ci) [#128... --- [misspelling] ~359-~359: This word is normally spelled as one. (EN_COMPOUNDS_BOILER_PLATE) Context: ...appOptions(appopts)` function to reduce boiler plate in root.go. ### State Machine Breakin... --- [uncategorized] ~379-~379: “It’s” is the contracted form of “it is”. Did you mean to use the possessive determiner “its”? (AI_HYDRA_LEO_SCY_IT_S) Context: ...o self-managed parameters and deprecate it's usage of `x/params`. * (x/distribution)... --- [uncategorized] ~382-~382: “It’s” is the contracted form of “it is”. Did you mean to use the possessive determiner “its”? (AI_HYDRA_LEO_SCY_IT_S) Context: ...o self-managed parameters and deprecate it's usage of `x/params`. * (x/staking) [#12... --- [uncategorized] ~383-~383: “It’s” is the contracted form of “it is”. Did you mean to use the possessive determiner “its”? (AI_HYDRA_LEO_SCY_IT_S) Context: ...o self-managed parameters and deprecate it's usage of `x/params`. * (x/bank) [#11859... --- [uncategorized] ~392-~392: A comma may be missing after the conjunctive/linking adverb ‘Additionally’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA) Context: ...tents` in `simulation.SimulationState`. Additionally it adds a `ProposalMsgs` field to `simu... --- [uncategorized] ~431-~431: “It’s” is the contracted form of “it is”. Did you mean to use the possessive determiner “its”? (AI_HYDRA_LEO_SCY_IT_S) Context: ...o self-managed parameters and deprecate it's usage of `x/params`. * (x/bank) [#12630... --- [typographical] ~461-~461: Usually, there’s no comma before “when”. (IF_NO_COMMA) Context: ...dled error for vesting account transfers, when total vesting amount exceeds total bala... --- [misspelling] ~464-~464: This word is normally spelled as one. (EN_COMPOUNDS_DEAD_LOCK) Context: ...com/cosmos/cosmos-sdk/pull/13116) Fix a dead-lock in the `Group-TotalWeight` `x/group` in...
Markdownlint
CHANGELOG.md
148-148: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 291-291: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 312-312: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 315-315: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 397-397: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 398-398: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 399-399: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 400-400: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 401-401: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 402-402: Expected: 2; Actual: 4 (MD007, ul-indent) Unordered list indentation --- 251-251: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 359-359: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 371-371: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 447-447: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 448-448: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 459-459: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 471-471: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces) Trailing spaces --- 420-420: null (MD038, no-space-in-code) Spaces inside code span elements
Additional comments not posted (5)
server/config/toml.go (1)
`99-101`: The addition of the `max-event-size` configuration parameter is well-documented and correctly implemented in the TOML configuration template.server/config/config.go (3)
`111-114`: The `MaxEventSize` field has been correctly added to the `BaseConfig` struct with appropriate struct tags and documentation. --- `323-323`: Setting the default value for `MaxEventSize` to 1MB in the `DefaultConfig` function is appropriate and aligns with the intended functionality of limiting event sizes. --- `389-389`: Correctly assigning the `MaxEventSize` from the `BaseConfig` to `sdk.MaxEventSize` in the `GetConfig` function ensures that the configuration is effectively utilized throughout the SDK.CHANGELOG.md (1)
`48-48`: The changelog entry for the new `max-event-size` configuration is clear and concise.
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.