feat(server/v2/stf): delayed marshalling of typed event #22684
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the event handling and configuration management within the CometBFT server. Key changes include renaming fields related to event indexing, adding new configuration options for managing ABCI events, and updating method signatures to utilize these configurations. The changes aim to enhance the clarity and control over how events are processed and indexed, reflecting a shift towards a more structured and flexible event management system. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
github-actions
bot
added
C:Confix
julienrbrt
requested review from
akhilkumarpilli,
a team,
facundomedica,
sontrinh16,
testinginprod and
kocubinski
as code owners
github-prbot
assigned akhilkumarpilli, facundomedica, kocubinski, sontrinh16 and testinginprod
|
@julienrbrt your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
server/v2/stf/core_event_service.go (1)
40-53: Consider adding validation and documentation.While the implementation is correct, consider these improvements:
- Add validation for nil message
- Document the delayed marshalling behavior in the method comment
// Emit emits an typed event that is defined in the protobuf file. -// In the future these events will be added to consensus. +// The event data is marshalled lazily when the Data() function is called, +// improving performance by avoiding unnecessary marshalling operations. func (em *eventManager) Emit(tev transaction.Msg) error { + if tev == nil { + return fmt.Errorf("cannot emit nil event") + } + event := event.Event{server/v2/cometbft/config.go (1)
45-48: Consider adding validation for event stringsWhile the implementation looks good, consider adding validation for the
IndexABCIEventsentries to ensure they follow the expected format of{eventType}.{attributeKey}. This would help catch configuration errors early.Example validation function:
func validateEventString(event string) error { parts := strings.Split(event, ".") if len(parts) != 2 { return fmt.Errorf("event must be in format {eventType}.{attributeKey}, got %s", event) } if parts[0] == "" || parts[1] == "" { return fmt.Errorf("both eventType and attributeKey must be non-empty") } return nil }tools/confix/data/v2-app.toml (3)
24-28: Enhance documentation for event indexing configurationThe event indexing configuration options are crucial for the new delayed marshalling feature, but the documentation could be more detailed:
- Add examples of event types in the
index-abci-eventsdescription- Explain the performance implications of
disable-abci-events- Document the relationship between these two settings
-# index-abci-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index. If empty, all events will be indexed. +# index-abci-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index. +# If empty, all events will be indexed. Example: ["message.sender", "transfer.recipient"] index-abci-events = [] -# disable-abci-events disables the ABCI event indexing. It is useful when relying on the indexer for event indexing. +# disable-abci-events disables ABCI event marshalling, improving performance when using the indexer for event querying. +# Note: When enabled, events will only be available through the indexer and not directly via ABCI responses. disable-abci-events = false
11-20: Review security implications of default network bindingsThe default CometBFT RPC server address is bound to localhost, which is secure. However, consider:
- Adding a warning about security implications of changing the bind address
- Documenting the relationship between
standalonemode and other settings-# address defines the CometBFT RPC server address to bind to. +# address defines the CometBFT RPC server address to bind to. +# WARNING: Binding to 0.0.0.0 or public addresses may expose the RPC server to public networks. +# It's recommended to use localhost or private addresses in production. address = 'tcp://127.0.0.1:26658' # standalone starts the application without the CometBFT node. The node should be started separately. +# When enabled, certain RPC server settings like 'address' and 'transport' may not apply. standalone = false
135-140: Improve telemetry configuration examplesThe global labels configuration would benefit from more comprehensive examples showing different use cases:
# GlobalLabels defines a global set of name/value label tuples applied to all metrics emitted using the wrapper functions defined in telemetry package. -# Example: -# [["chain_id", "cosmoshub-1"]] +# Examples: +# [ +# ["chain_id", "cosmoshub-1"], +# ["network", "mainnet"], +# ["version", "v1.0.0"] +# ] global-labels = []server/v2/cometbft/abci.go (1)
108-113: Consider error handling optimization.While the conditional handling of ABCI events is good, we can optimize the code by avoiding the empty slice allocation when events are disabled.
-events := make([]abci.Event, 0) +var events []abci.Event if !c.cfg.AppTomlConfig.DisableABCIEvents { + events = make([]abci.Event, 0) events, err = intoABCIEvents(resp.Events, c.indexedABCIEvents) if err != nil { return nil, err } }server/v2/cometbft/utils.go (4)
76-77: Avoid grouping parameters of the same type; specify types for each parameter.For clarity and to comply with the Uber Go Style Guide, please specify the type for each parameter individually. Grouping parameters can make the code harder to read and understand.
Apply this diff to improve parameter declarations:
func finalizeBlockResponse( in *server.BlockResponse, cp *cmtproto.ConsensusParams, appHash []byte, indexSet map[string]struct{}, - disableABCIEvents, - debug bool, + disableABCIEvents bool, + debug bool, ) (*abci.FinalizeBlockResponse, error) {
119-123: Avoid grouping parameters of the same type; specify types for each parameter.Similarly, in the
intoABCITxResultsfunction, specify the type for each parameter individually to enhance readability.Apply this diff:
func intoABCITxResults( results []server.TxResult, indexSet map[string]struct{}, - disableABCIEvents, debug bool, + disableABCIEvents bool, + debug bool, ) ([]*abci.ExecTxResult, error) {
157-157: Update error message to reflect the operation performed.The error message should accurately describe the failed operation. Since
e.Data()retrieves event data, consider updating the message.Apply this diff:
- return nil, fmt.Errorf("failed to marshal event data: %w", err) + return nil, fmt.Errorf("failed to retrieve event data: %w", err)
175-179: Avoid redeclaring 'err' variable; reuse the existing 'err' variable.The variable
erris already declared in an outer scope. Redeclaring it here is unnecessary and could lead to confusion.Apply this diff to eliminate the redundant declaration:
} else { - var err error attrs, err = e.Attributes() if err != nil { return nil, err }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
server/v2/cometbft/abci.go(4 hunks)server/v2/cometbft/config.go(1 hunks)server/v2/cometbft/query.go(1 hunks)server/v2/cometbft/server.go(3 hunks)server/v2/cometbft/utils.go(6 hunks)server/v2/stf/core_event_service.go(1 hunks)tools/confix/data/v2-app.toml(1 hunks)tools/confix/migrations.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/stf/core_event_service.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tools/confix/migrations.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
server/v2/stf/core_event_service.go (1)
40-50: LGTM! Efficient implementation of delayed marshalling.
The implementation correctly delays JSON marshalling by using a closure in the Data field, which aligns with the PR objectives for improving performance. The marshalling will only occur when the Data() function is actually called.
server/v2/cometbft/query.go (1)
64-64: LGTM! Verify consistent renaming across codebase.
The variable rename from indexedEvents to indexedABCIEvents maintains consistency with the broader refactoring of event-related naming conventions.
Let's verify that the renaming has been consistently applied across the codebase:
✅ Verification successful
Variable renaming has been consistently applied
The verification confirms that:
- No instances of the old
indexedEventsvariable name remain in the codebase - The new
indexedABCIEventsnaming is consistently used across all relevant files:- server/v2/cometbft/server.go
- server/v2/cometbft/query.go
- server/v2/cometbft/abci.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old variable name
# and confirm the new naming convention is used consistently
# Check for any remaining instances of old name
echo "Checking for any remaining instances of 'indexedEvents'..."
rg "indexedEvents" --type go
# Verify new name usage
echo "Verifying usage of new name 'indexedABCIEvents'..."
rg "indexedABCIEvents" --type go
Length of output: 995
server/v2/cometbft/config.go (3)
30-31: LGTM! Default values are well-chosen
The initialization of new fields follows Go best practices with safe default values:
- Empty slice for IndexABCIEvents allows immediate use without nil checks
- False for DisableABCIEvents maintains backward compatibility
36-42: LGTM! Well-documented configuration fields
The configuration fields are properly defined with:
- Clear and descriptive comments explaining each field's purpose
- Consistent tag formatting
- Appropriate types for each configuration value
47-48: Verify consistent usage across the codebase
Let's verify that these configuration changes are properly handled across all relevant files.
✅ Verification successful
Configuration changes are properly integrated and handled
The verification shows that the new configuration fields (IndexABCIEvents and DisableABCIEvents) are consistently implemented across the codebase:
- The
DisableABCIEventsflag is properly checked in ABCI event processing IndexABCIEventsis correctly used during server initialization to build the indexed events map- The
intoABCIEventsutility function properly handles both the indexing set and disabled state - Old
IndexEventsreferences exist only in configuration templates and migration tools, which is expected for backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new configuration fields
# Check for proper handling of DisableABCIEvents
echo "Checking DisableABCIEvents usage:"
rg -A 5 "DisableABCIEvents"
# Check for proper handling of IndexABCIEvents
echo "Checking IndexABCIEvents usage:"
rg -A 5 "IndexABCIEvents"
# Check for any remaining references to the old IndexEvents
echo "Checking for old IndexEvents references:"
rg "IndexEvents"
Length of output: 6316
Script:
#!/bin/bash
# Let's check for any potential conflicts or inconsistencies in event handling
# Check for any references to event indexing in the ABCI implementation
echo "Checking ABCI event handling implementation:"
rg -A 5 "intoABCIEvents"
# Check the server initialization to see how these configs are used
echo "Checking server initialization:"
ast-grep --pattern 'func New[A-Za-z]*Server'
# Check if there are any migration or conversion utilities
echo "Checking migration utilities:"
fd -t f confix
Length of output: 1932
server/v2/cometbft/server.go (2)
37-37: LGTM: Import statement follows Go conventions
The import statement is correctly placed and follows the standard Go import grouping conventions.
188-188: LGTM: Consistent field naming in consensus struct
The field rename maintains consistency with the earlier variable rename and better describes its ABCI-specific purpose.
server/v2/cometbft/abci.go (2)
65-67: LGTM: Field renaming improves clarity.
The renaming of indexedEvents to indexedABCIEvents better reflects its specific purpose for ABCI event handling.
564-564: Verify event processing behavior.
The addition of DisableABCIEvents configuration to finalizeBlockResponse affects event processing. Ensure that all consumers of these events (like indexers or event subscribers) are aware of this change and can handle disabled events gracefully.
✅ Verification successful
Let me gather more information about the finalizeBlockResponse function and its event handling.
Event processing behavior is properly handled with the new configuration
The DisableABCIEvents configuration is safely implemented in the finalizeBlockResponse function with proper fallback behavior:
- Events are only disabled when explicitly configured via
DisableABCIEvents - The implementation properly handles both block events and transaction results
- Event indexing is preserved through
indexedABCIEventsmap even when events are disabled - All event consumers work through the standard ABCI interfaces which already handle nil/empty events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for event consumers in the codebase
echo "Searching for event consumers..."
rg -A 5 "Events.*\[\]abci\.Event"
# Search for indexer implementations
echo "Searching for indexer implementations..."
rg -A 5 "type.*Indexer.*interface"
Length of output: 7881
Script:
#!/bin/bash
# Search for finalizeBlockResponse implementation
echo "Searching for finalizeBlockResponse implementation..."
ast-grep --pattern 'func finalizeBlockResponse($$$)'
# Search for DisableABCIEvents usage
echo "Searching for DisableABCIEvents usage..."
rg -A 5 "DisableABCIEvents"
# Search for event indexing related code
echo "Searching for event indexing code..."
rg -A 5 "indexedABCIEvents"
Length of output: 6546
server/v2/cometbft/utils.go (2)
5-10: LGTM on the new imports.
The added imports are appropriately grouped and ordered according to Go style guidelines.
200-202: LGTM on the refactored event handling.
The use of intoABCIEvents improves consistency and reduces code duplication.
tools/confix/data/v2-app.toml
Outdated
|
|
||
| # State storage database type. Currently we support: "sqlite", "pebble" and "rocksdb" | ||
| ss-type = 'sqlite' | ||
|
|
||
| # State commitment database type. Currently we support: "iavl" and "iavl-v2" | ||
| sc-type = 'iavl' | ||
|
|
||
| # Pruning options for state storage | ||
| [store.options.ss-pruning-option] | ||
|
|
||
| # Number of recent heights to keep on disk. | ||
| keep-recent = 2 | ||
|
|
||
| # Height interval at which pruned heights are removed from disk. | ||
| interval = 100 | ||
|
|
||
| # Pruning options for state commitment | ||
| [store.options.sc-pruning-option] | ||
|
|
||
| # Number of recent heights to keep on disk. | ||
| keep-recent = 2 | ||
|
|
||
| # Height interval at which pruned heights are removed from disk. | ||
| interval = 100 | ||
|
|
||
| [store.options.iavl-config] | ||
|
|
||
| # CacheSize set the size of the iavl tree cache. | ||
| cache-size = 100000 | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the findings, I can now generate the final response.
Storage configuration validation needs to be implemented
The codebase currently lacks validation for the storage configuration settings in tools/confix/data/v2-app.toml. While there is pruning validation in store/pruning/types/options.go, the store configuration (store/v2/root/config.go and store/v2/root/factory.go) doesn't implement validation for:
- Compatibility between
ss-typeandsc-type - IAVL cache size limits
- Relationship between pruning settings
Action items:
- Implement
Validate()method forroot.Configandroot.Optionsstructs - Add validation for compatible storage type combinations
- Add reasonable bounds for IAVL cache size
- Ensure pruning validation is properly integrated with the store config
🔗 Analysis chain
Add validation rules and clarify storage configuration
The storage configuration is complex and could benefit from additional validation rules and clarification:
- Document the implications of different storage combinations
- Add validation rules for pruning intervals
- Clarify the relationship between state storage and state commitment
Consider implementing validation for these settings in the application configuration parser to ensure:
- Pruning interval is greater than keep-recent
- Compatible combinations of ss-type and sc-type are used
- Reasonable cache-size values based on available system memory
Let me gather more information about the validation logic specifically for storage configuration.
Let me search for pruning validation logic to complete our verification.
Let me try a different approach to find validation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's existing validation logic for these settings
rg -A 5 "ValidateBasic|Validate" | grep -i "store"
Length of output: 8959
Script:
#!/bin/bash
# Search for storage configuration validation
rg -A 10 "store.*config|storage.*config|ss-type|sc-type"
# Search for IAVL configuration validation
ast-grep --pattern 'func $_($_) error {
$$$
cache-size
$$$
}'
Length of output: 30902
Script:
#!/bin/bash
# Search for pruning validation in store package
ast-grep --pattern 'func $_($_) error {
$$$
pruning
$$$
}'
# Search for store validation logic
rg -A 5 "ValidateBasic|Validate.*store" store/
Length of output: 115
Script:
#!/bin/bash
# Search for store config validation in root package
rg -A 10 "type Config struct|type Options struct" store/v2/root/
rg -A 10 "func.*Config.*Validate|func.*Options.*Validate" store/v2/root/
# Search for pruning validation
rg -A 10 "type PruningOption|type Pruning" store/
rg -A 10 "func.*Pruning.*Validate" store/
Length of output: 6733
| @@ -41,7 +41,7 @@ type v2KeyChangesMap map[string][]string | |||
| var v2KeyChanges = v2KeyChangesMap{ | |||
| "minimum-gas-prices": []string{"server.minimum-gas-prices"}, | |||
| "min-retain-blocks": []string{"comet.min-retain-blocks"}, | |||
| "index-events": []string{"comet.index-events"}, | |||
| "index-events": []string{"comet.index-abci-events"}, | |||
There was a problem hiding this comment.
💡 Codebase verification
Configuration key migration needs additional changes
The verification revealed that the configuration key change from "index-events" to "comet.index-abci-events" requires additional attention:
- The migration mapping in
tools/confix/migrations.gois correct, but there are several files that need to be updated:- All test configuration files in
tools/confix/data/v*.tomlstill use the oldindex-eventskey server/config/config.goandserver/config/config_test.gostill reference the old key in structs and testsserver/start.godefines a flag constantFlagIndexEventswith the old name
- All test configuration files in
The configuration migration tool correctly maps the old key to the new one, but the codebase itself needs to be updated to consistently use the new naming convention.
🔗 Analysis chain
LGTM! Configuration key update aligns with event handling changes.
The renaming from "comet.index-events" to "comet.index-abci-events" is consistent with the broader changes in event handling mentioned in the PR objectives.
Let's verify the impact on existing configurations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing usage of the old configuration key
# This helps ensure we haven't missed any references to the old key
# Search for any remaining references to the old key
rg -l "index-events" --type toml
# Search for any documentation or comments mentioning the old key
rg -l "index-events" --type md
Length of output: 241
Script:
#!/bin/bash
# Let's check the actual content of these files to understand the context better
# Check the changelog entry
echo "=== CHANGELOG.md ==="
rg "index-events" CHANGELOG.md -C 2
# Check the configuration files
echo -e "\n=== Configuration Files ==="
for file in tools/confix/data/v*.toml; do
echo -e "\n--- $file ---"
rg "index-events" "$file" -C 1
done
# Check if there are any other references in Go files
echo -e "\n=== Go Files ==="
rg "index-events" --type go -C 2
Length of output: 6084
| indexedABCIEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexABCIEvents)) | ||
| for _, e := range srv.config.AppTomlConfig.IndexABCIEvents { | ||
| indexedABCIEvents[e] = struct{}{} |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent event indexing variable names across the codebase
The codebase shows two different naming conventions being used:
indexEventsinbaseapp/directoryindexedABCIEventsinserver/v2/cometbft/directory
Both variables serve the same purpose of indexing ABCI events, but the inconsistent naming could lead to confusion. Consider:
- Unifying the naming convention across the codebase
- Using
indexedABCIEventsconsistently as it's more descriptive and explicit about its ABCI-specific purpose
🔗 Analysis chain
LGTM: Efficient map initialization with pre-allocated capacity
The map initialization with pre-allocated capacity is a good performance optimization. The renamed variable better reflects its ABCI-specific purpose.
Let's verify the consistency of this change across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of indexedABCIEvents across the codebase
# and ensure no old references to indexEvents remain
# Check for any remaining references to the old name
rg "indexEvents" --type go
# Check the usage pattern of the new name
rg "indexedABCIEvents" --type go -A 2 -B 2
Length of output: 2687
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
server/v2/stf/core_event_service.go (1)
40-50: Implementation looks good with room for improvementThe delayed marshalling implementation through a closure aligns well with the performance objectives. However, consider these improvements:
- The closure could be extracted into a named function for better reusability and testing
- Consider adding context timeout for the marshalling operation to prevent potential blocking
Here's a suggested refactoring:
func (em *eventManager) Emit(tev transaction.Msg) error { event := event.Event{ Type: gogoproto.MessageName(tev), - Data: func() (json.RawMessage, error) { - buf := new(bytes.Buffer) - jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: true, AnyResolver: nil} - if err := jm.Marshal(buf, tev); err != nil { - return nil, err - } - - return buf.Bytes(), nil - }, + Data: marshalEventData(tev), } } +func marshalEventData(msg gogoproto.Message) func() (json.RawMessage, error) { + return func() (json.RawMessage, error) { + buf := new(bytes.Buffer) + jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: true, AnyResolver: nil} + if err := jm.Marshal(buf, msg); err != nil { + return nil, err + } + return buf.Bytes(), nil + } +}server/v2/cometbft/config.go (1)
48-50: Consider adding validation for event formatThe IndexABCIEvents field accepts events in the form
{eventType}.{attributeKey}. Consider adding validation to ensure the correct format is provided.Example validation function:
func validateEventFormat(events []string) error { for _, event := range events { if !strings.Contains(event, ".") { return fmt.Errorf("invalid event format %q: must be in the form {eventType}.{attributeKey}", event) } } return nil }server/v2/cometbft/utils.go (4)
80-89: HandleDisableABCIEventsconfiguration flag correctlyThe conditional block checks
if !cfg.DisableABCIEventsto determine whether to process events. Using a negative configuration flag can be prone to errors and reduce code clarity.Consider renaming
DisableABCIEventstoEnableABCIEventsfor positive logic:- if !cfg.DisableABCIEvents { + if cfg.EnableABCIEvents {And adjust the configuration struct accordingly.
132-136: HandleDisableABCIEventsin transaction results processingSimilar to the previous comment, the
intoABCITxResultsfunction checksif !cfg.DisableABCIEvents. For consistency and clarity, consider applying the same positive logic refactoring here.Apply this diff:
- if !cfg.DisableABCIEvents { + if cfg.EnableABCIEvents {
151-151: Avoid double negatives by renamingindexNonevariableUsing double negatives like
!indexNonecan make the code harder to understand. Consider renamingindexNonetoindexEventsorenableIndexingto reflect positive logic, improving readability.Apply this diff to rename the variable:
-func intoABCIEvents(events []event.Event, indexSet map[string]struct{}, indexNone bool) ([]abci.Event, error) { +func intoABCIEvents(events []event.Event, indexSet map[string]struct{}, indexEvents bool) ([]abci.Event, error) { ... - Index: !indexNone && (index || indexAll), + Index: indexEvents && (index || indexAll),Ensure that all function calls to
intoABCIEventsare updated accordingly.Also applies to: 195-195
158-166: Handle JSON unmarshalling errors gracefullyIn the event data processing, errors from
json.Unmarshalare returned immediately. Consider providing more context in the error messages to aid in debugging.Enhance error messages as follows:
- return nil, fmt.Errorf("failed to unmarshal event data: %w", err) + return nil, fmt.Errorf("failed to unmarshal event data for event type '%s': %w", e.Type, err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
server/v2/cometbft/abci.go(4 hunks)server/v2/cometbft/config.go(1 hunks)server/v2/cometbft/query.go(1 hunks)server/v2/cometbft/server.go(2 hunks)server/v2/cometbft/utils.go(4 hunks)server/v2/stf/core_event_service.go(1 hunks)tools/confix/data/v2-app.toml(1 hunks)tools/confix/migrations.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/stf/core_event_service.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tools/confix/migrations.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (18)
server/v2/stf/core_event_service.go (1)
53-53: Consider handling Data() function errors
The error from the Data closure is never checked when the event is used. Consider implementing error handling where the event data is consumed.
Let's verify how the event data is consumed:
server/v2/cometbft/query.go (1)
64-68: LGTM! Verify the intoABCISimulationResponse implementation.
The changes align well with the PR objectives for implementing delayed marshalling of typed events. The new parameters allow for configurable event indexing behavior.
Let's verify the implementation of intoABCISimulationResponse:
✅ Verification successful
Implementation verified and consistent across codebase
The changes to intoABCISimulationResponse are correctly implemented and the new parameters are consistently used throughout the codebase:
- The function is properly defined in
server/v2/cometbft/utils.gowith matching parameters - All calls to this function, including the one under review, pass the correct indexing parameters
- The underlying
intoABCIEventsfunction is also consistently implemented and used with the same indexing parameters across different contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of intoABCISimulationResponse function
# and its usage of the new parameters.
# Search for the function definition
ast-grep --pattern 'func intoABCISimulationResponse($_) {
$$$
}'
# Search for other usages of this function to ensure consistent parameter passing
rg 'intoABCISimulationResponse\(' -A 3
Length of output: 685
Script:
#!/bin/bash
# Let's verify the implementation of intoABCIEvents function and its usage
# since it's called within intoABCISimulationResponse
# Search for the intoABCIEvents function definition
ast-grep --pattern 'func intoABCIEvents($_) {
$$$
}'
# Search for all usages of intoABCIEvents
rg 'intoABCIEvents\(' -A 3
Length of output: 1366
server/v2/cometbft/config.go (2)
30-32: LGTM: Default configuration initialization is correct
The default values are appropriately set:
- Empty slice for IndexABCIEvents using make()
- Conservative defaults for disable flags (false)
46-50: Verify event format parsing implementation
Let's verify how these event strings are parsed and validated in the codebase.
✅ Verification successful
Let's search for more specific patterns related to event validation and processing.
Let's search for specific event format validation and parsing code in the indexer package since that's where the event indexing logic would likely be implemented.
Let's search for the actual event format validation and processing logic in the CometBFT code.
Based on the code analysis, I can now provide a final response about the event format validation:
Event format validation is properly implemented
The event format {eventType}.{attributeKey} is correctly handled in the CometBFT integration:
- Events are processed by
intoABCIEventsfunction which validates and transforms events - The indexing format is enforced by constructing the index key as
fmt.Sprintf("%s.%s", e.Type, attr.Key) - The config supports both selective indexing through
IndexABCIEventslist and full indexing when the list is empty - Event indexing can be disabled via
DisableIndexABCIEventsflag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for event format parsing and validation
rg -A 5 "index-abci-events" --type go
ast-grep --pattern 'func $_(events []string) $_ {
$$$
}'
Length of output: 1727
Script:
#!/bin/bash
# Search for event-related validation or processing code
rg -A 10 "type.*Event" --type go
rg -A 10 "func.*Event" --type go
# Search for any validation related to event format
ast-grep --pattern 'if $_ := strings.Split($event, "."); $_ {
$$$
}'
# Look for event-related constants or types
rg "EventType|AttributeKey" --type go
Length of output: 101557
Script:
#!/bin/bash
# Search for event format validation in indexer package
rg -A 10 "index.*events" indexer/
rg -A 10 "validateEvent" indexer/
# Look for event format parsing in CometBFT config
rg -A 10 "IndexABCIEvents" server/v2/cometbft/
# Search for event format validation tests
rg -A 10 "TestEvent.*Format" --type go
Length of output: 6231
Script:
#!/bin/bash
# Search for event format validation in CometBFT code
rg -A 10 "intoABCIEvents" server/v2/cometbft/
rg -A 10 "indexSet" server/v2/cometbft/
# Look for event format parsing functions
ast-grep --pattern 'func $_(events []string) $_ {
$$$
strings.Split($_, ".")
$$$
}'
# Search for event format related tests
rg -A 10 "test.*event.*format" --type go -i
Length of output: 7893
tools/confix/data/v2-app.toml (5)
11-14: Verify RPC server binding address security
The default RPC server address is correctly bound to localhost (127.0.0.1), which is secure. However, users might change this to a public address without understanding the security implications.
Consider adding a warning comment about security implications of exposing the RPC server to public interfaces.
55-62: Validate gRPC message size limits
The max message size settings could lead to resource exhaustion if set too high. While the defaults are reasonable:
- max-recv-msg-size: 10MB
- max-send-msg-size: math.MaxInt32
Consider adding validation to prevent unreasonably large values and document the resource implications of these settings.
117-149: Secure telemetry endpoint configuration
The telemetry configuration exposes metrics endpoints that could potentially leak sensitive information. Key considerations:
- The metrics server address is bound to localhost by default (secure)
- Prometheus retention time needs bounds
- Statsd and Datadog endpoints need secure configuration
Consider:
- Adding validation for retention time to prevent excessive resource usage
- Adding security warnings about exposing metrics endpoints
- Documenting secure configuration practices for third-party metrics services
24-31:
Validate event indexing configuration consistency
The three event-related configurations (index-abci-events, disable-index-abci-events, disable-abci-events) might have conflicting states. For example, having both indexing enabled and disabled simultaneously.
Consider implementing validation to ensure these settings are mutually exclusive and adding documentation about their interaction.
82-112:
Storage configuration validation needs to be implemented
The storage configuration settings require validation for:
- Compatibility between
ss-typeandsc-type - IAVL cache size limits
- Relationship between pruning settings
Run the following script to check for existing validation:
#!/bin/bash
# Search for storage configuration validation
rg -A 10 "func.*Validate" store/v2/root/tools/confix/migrations.go (1)
44-44: LGTM! Configuration key update aligns with event handling changes.
The renaming from "index-events" to "comet.index-abci-events" is consistent with the broader changes in event handling mentioned in the PR objectives.
Let's verify if the previously identified files have been updated:
#!/bin/bash
# Description: Check for any remaining references to the old configuration key
# Expected: No occurrences of the old key in test configs and server code
# Check test configuration files
echo "=== Test Configuration Files ==="
fd -e toml . tools/confix/data --exec grep -l "index-events" {} \;
# Check server code
echo -e "\n=== Server Code ==="
rg "index-events" server/ --type goserver/v2/cometbft/server.go (2)
125-127: LGTM: Efficient map initialization with pre-allocated capacity
The map initialization with pre-allocated capacity is a good performance optimization. The renamed variable better reflects its ABCI-specific purpose.
The naming inconsistency issue mentioned in the previous review still applies. Consider unifying the naming convention across the codebase.
188-188: LGTM: Consistent usage of renamed variable
The assignment maintains consistency with the renamed indexedABCIEvents variable, supporting the improved event handling model.
server/v2/cometbft/abci.go (3)
67-67: LGTM: Field rename improves clarity.
The rename from indexedEvents to indexedABCIEvents better reflects its specific purpose for ABCI event handling.
568-574: Verify configuration usage in finalizeBlockResponse.
The function now uses both indexedABCIEvents and AppTomlConfig for event handling. Ensure that all configuration options are properly respected.
✅ Verification successful
Configuration is properly handled in finalizeBlockResponse
The implementation in server/v2/cometbft/utils.go shows that both indexedABCIEvents and AppTomlConfig are correctly used:
DisableABCIEventsflag controls whether events are processed at all- When events are enabled,
indexSet(indexedABCIEvents) andDisableIndexABCIEventsare properly passed tointoABCIEventsfor event handling
The configuration usage is consistent with the intended behavior and properly respects all configuration options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the implementation of finalizeBlockResponse to verify config usage
ast-grep --pattern 'func finalizeBlockResponse($$$) {
$$$
}'
Length of output: 64
Script:
#!/bin/bash
# Let's try to find the finalizeBlockResponse function using ripgrep
rg -A 20 "func finalizeBlockResponse"
# Also search for any usage of AppTomlConfig and indexedABCIEvents
rg "AppTomlConfig|indexedABCIEvents" -A 5
Length of output: 14393
108-117: Verify error handling in event conversion.
The event handling logic has been updated to respect the DisableABCIEvents configuration. However, the error from intoABCIEvents is returned directly, which could expose internal details.
Consider wrapping the error:
- return nil, err
+ return nil, fmt.Errorf("failed to convert events: %w", err)server/v2/cometbft/utils.go (3)
5-5: Ensure Go version compatibility for maps and slices packages
The maps and slices packages were introduced in Go 1.18. Please verify that the project's minimum Go version is 1.18 or higher to ensure compatibility.
Also applies to: 8-8, 10-10
76-76: Confirm consistent handling of configuration parameters
The functions finalizeBlockResponse and intoABCITxResults have been updated to accept cfg *AppTomlConfig instead of a debug boolean. Please ensure that all function calls to these functions are updated accordingly throughout the codebase to prevent mismatches.
Also applies to: 122-122
144-144: Verify that cfg.Trace corresponds to the debug parameter
In the call to responseExecTxResultWithEvents, cfg.Trace is passed as the debug parameter. Please ensure that cfg.Trace correctly represents the intended debugging or tracing behavior in this context.
|
|
||
| # minimum-gas-prices defines the price which a validator is willing to accept for processing a transaction. A transaction's fees must meet the minimum of any denomination specified in this config (e.g. 0.25token1;0.0001token2). | ||
| minimum-gas-prices = '0stake' |
There was a problem hiding this comment.
Review minimum gas price security implications
The default minimum gas price of '0stake' could expose the network to spam attacks.
Consider:
- Adding a warning comment about spam attack risks with zero gas prices
- Recommending a non-zero default value for production environments
julienrbrt
added
the
backport/v0.52.x
Description
Closes: #21312
As discussed yesterday in indexer sync, improve typed event performance in state machine by not marshalling/unmarshalling their data.
Stf:
Set event marshalling in Data()
Cometbft server:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Release Notes
New Features
index-abci-events,disable-index-abci-events, anddisable-abci-events.Bug Fixes
Refactor
Emitmethod.Chores