-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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(server/v2/stf): delayed marshalling of typed event #22684
Changes from 6 commits
2c942e7
816183f
32c8f63
9331c67
b086719
d5302e2
5a873c5
b47560b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,9 +122,9 @@ func New[T transaction.Tx]( | |
} | ||
} | ||
|
||
indexEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexEvents)) | ||
for _, e := range srv.config.AppTomlConfig.IndexEvents { | ||
indexEvents[e] = struct{}{} | ||
indexedABCIEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexABCIEvents)) | ||
for _, e := range srv.config.AppTomlConfig.IndexABCIEvents { | ||
indexedABCIEvents[e] = struct{}{} | ||
Comment on lines
+125
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Inconsistent event indexing variable names across the codebase The codebase shows two different naming conventions being used:
Both variables serve the same purpose of indexing ABCI events, but the inconsistent naming could lead to confusion. Consider:
🔗 Analysis chainLGTM: 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 executedThe 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 |
||
} | ||
|
||
ss := store.GetStateStorage().(snapshots.StorageSnapshotter) | ||
|
@@ -185,7 +185,7 @@ func New[T transaction.Tx]( | |
checkTxHandler: srv.serverOptions.CheckTxHandler, | ||
extendVote: srv.serverOptions.ExtendVoteHandler, | ||
chainID: chainID, | ||
indexedEvents: indexEvents, | ||
indexedABCIEvents: indexedABCIEvents, | ||
initialHeight: 0, | ||
queryHandlersMap: queryHandlers, | ||
getProtoRegistry: sync.OnceValues(gogoproto.MergedRegistry), | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -70,16 +70,23 @@ func finalizeBlockResponse( | |||||||||
cp *cmtproto.ConsensusParams, | ||||||||||
appHash []byte, | ||||||||||
indexSet map[string]struct{}, | ||||||||||
debug bool, | ||||||||||
cfg *AppTomlConfig, | ||||||||||
) (*abci.FinalizeBlockResponse, error) { | ||||||||||
allEvents := append(in.BeginBlockEvents, in.EndBlockEvents...) | ||||||||||
|
||||||||||
events, err := intoABCIEvents(allEvents, indexSet) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
events := make([]abci.Event, 0) | ||||||||||
|
||||||||||
if !cfg.DisableABCIEvents { | ||||||||||
var err error | ||||||||||
events, err = intoABCIEvents( | ||||||||||
append(in.BeginBlockEvents, in.EndBlockEvents...), | ||||||||||
indexSet, | ||||||||||
cfg.DisableIndexABCIEvents, | ||||||||||
) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
txResults, err := intoABCITxResults(in.TxResults, indexSet, debug) | ||||||||||
txResults, err := intoABCITxResults(in.TxResults, indexSet, cfg) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
|
@@ -91,6 +98,7 @@ func finalizeBlockResponse( | |||||||||
AppHash: appHash, | ||||||||||
ConsensusParamUpdates: cp, | ||||||||||
} | ||||||||||
|
||||||||||
return resp, nil | ||||||||||
} | ||||||||||
|
||||||||||
|
@@ -108,72 +116,67 @@ func intoABCIValidatorUpdates(updates []appmodulev2.ValidatorUpdate) []abci.Vali | |||||||||
return valsetUpdates | ||||||||||
} | ||||||||||
|
||||||||||
func intoABCITxResults(results []server.TxResult, indexSet map[string]struct{}, debug bool) ([]*abci.ExecTxResult, error) { | ||||||||||
func intoABCITxResults( | ||||||||||
results []server.TxResult, | ||||||||||
indexSet map[string]struct{}, | ||||||||||
cfg *AppTomlConfig, | ||||||||||
) ([]*abci.ExecTxResult, error) { | ||||||||||
res := make([]*abci.ExecTxResult, len(results)) | ||||||||||
for i := range results { | ||||||||||
events, err := intoABCIEvents(results[i].Events, indexSet) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
var err error | ||||||||||
events := make([]abci.Event, 0) | ||||||||||
|
||||||||||
if !cfg.DisableABCIEvents { | ||||||||||
events, err = intoABCIEvents(results[i].Events, indexSet, cfg.DisableIndexABCIEvents) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
res[i] = responseExecTxResultWithEvents( | ||||||||||
results[i].Error, | ||||||||||
results[i].GasWanted, | ||||||||||
results[i].GasUsed, | ||||||||||
events, | ||||||||||
debug, | ||||||||||
cfg.Trace, | ||||||||||
) | ||||||||||
} | ||||||||||
|
||||||||||
return res, nil | ||||||||||
} | ||||||||||
|
||||||||||
func intoABCIEvents(events []event.Event, indexSet map[string]struct{}) ([]abci.Event, error) { | ||||||||||
func intoABCIEvents(events []event.Event, indexSet map[string]struct{}, indexNone bool) ([]abci.Event, error) { | ||||||||||
indexAll := len(indexSet) == 0 | ||||||||||
abciEvents := make([]abci.Event, len(events)) | ||||||||||
for i, e := range events { | ||||||||||
attributes, err := e.Attributes() | ||||||||||
attrs := make([]event.Attribute, 0) | ||||||||||
var err error | ||||||||||
attrs, err = e.Attributes() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix ineffectual assignment to The initial assignment to Apply this fix: - attrs := make([]event.Attribute, 0)
- var err error
- attrs, err = e.Attributes()
+ attrs, err := e.Attributes() 📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (1.62.2)152-152: ineffectual assignment to attrs (ineffassign) |
||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
|
||||||||||
abciEvents[i] = abci.Event{ | ||||||||||
Type: e.Type, | ||||||||||
Attributes: make([]abci.EventAttribute, len(attributes)), | ||||||||||
Attributes: make([]abci.EventAttribute, len(attrs)), | ||||||||||
} | ||||||||||
|
||||||||||
for j, attr := range attributes { | ||||||||||
for j, attr := range attrs { | ||||||||||
_, index := indexSet[fmt.Sprintf("%s.%s", e.Type, attr.Key)] | ||||||||||
abciEvents[i].Attributes[j] = abci.EventAttribute{ | ||||||||||
Key: attr.Key, | ||||||||||
Value: attr.Value, | ||||||||||
Index: index || indexAll, | ||||||||||
Index: !indexNone && (index || indexAll), | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
return abciEvents, nil | ||||||||||
} | ||||||||||
|
||||||||||
func intoABCISimulationResponse(txRes server.TxResult, indexSet map[string]struct{}) ([]byte, error) { | ||||||||||
indexAll := len(indexSet) == 0 | ||||||||||
abciEvents := make([]abci.Event, len(txRes.Events)) | ||||||||||
for i, e := range txRes.Events { | ||||||||||
attributes, err := e.Attributes() | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
abciEvents[i] = abci.Event{ | ||||||||||
Type: e.Type, | ||||||||||
Attributes: make([]abci.EventAttribute, len(attributes)), | ||||||||||
} | ||||||||||
|
||||||||||
for j, attr := range attributes { | ||||||||||
_, index := indexSet[fmt.Sprintf("%s.%s", e.Type, attr.Key)] | ||||||||||
abciEvents[i].Attributes[j] = abci.EventAttribute{ | ||||||||||
Key: attr.Key, | ||||||||||
Value: attr.Value, | ||||||||||
Index: index || indexAll, | ||||||||||
} | ||||||||||
} | ||||||||||
func intoABCISimulationResponse(txRes server.TxResult, indexSet map[string]struct{}, indexNone bool) ([]byte, error) { | ||||||||||
abciEvents, err := intoABCIEvents(txRes.Events, indexSet, indexNone) | ||||||||||
if err != nil { | ||||||||||
return nil, err | ||||||||||
} | ||||||||||
|
||||||||||
msgResponses := make([]*gogoany.Any, len(txRes.Resp)) | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont get why the first one is needed? in both cases when its true it doesnt emit events for comet but one still includes it in the finalize block response? why is that needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, well I didn't know if it was needed so I added it if someone was relying on that use case (block streaming, and doing manual indexing, but still while not willing comet to do it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, i see what you mean. lets make this one value. The second one. if events dont want to be indexed then the node operator should set
tx_index:
tonull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thought about that too, but learned that toml doesn't support nil/null/none value, so that was our only option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its used here https://github.com/cometbft/cometbft/blob/main/config/config.toml.tpl#L548.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but there
null
is simply a string that is probably parsed by comet and do some logic (EDIT: looks like anything other than kv and psql will endup asnull
-> https://github.com/cometbft/cometbft/blob/e2c3fd400f52aecf50c78ec0a3bda73ebb5ff76b/state/indexer/block/indexer.go#L66-L67, so an empty string or foo would have worked too)Our field is a string array, and we cannot have it a string array and a string.
If we have it as an
any
in our config, then everytime you parse it, you need to check if it is an array or a string.IMHO the bool is a tad nicer.