tendermint: GetEvents: return events in order #5117
Conversation
mitjat
force-pushed
the
mitjat/getevents-order
branch
from
3912da5
to
497e1ca
Compare
| tmEvents := append(results.BeginBlockEvents, results.EndBlockEvents...) | ||
| tmEvents := results.BeginBlockEvents | ||
| for _, txResults := range results.TxsResults { | ||
| tmEvents = append(tmEvents, txResults.Events...) | ||
| } | ||
| tmEvents = append(tmEvents, results.EndBlockEvents...) |
There was a problem hiding this comment.
Looks unrelated, but might as well change this too, for consistency.
This covers all the occurrences of EndBlockEvents in the codebase.
Codecov Report
@@ Coverage Diff @@
## master #5117 +/- ##
==========================================
+ Coverage 66.85% 67.07% +0.22%
==========================================
Files 497 497
Lines 53159 53160 +1
==========================================
+ Hits 35540 35658 +118
+ Misses 13280 13186 -94
+ Partials 4339 4316 -23
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
mitjat
requested review from
kostko,
Yawning,
peterjgilbert,
pro-wh and
ptrus
as code owners
mitjat
force-pushed
the
mitjat/getevents-order
branch
from
497e1ca
to
6423bfd
Compare
.changelog/5117.breaking.md
Outdated
| @@ -0,0 +1,4 @@ | |||
| go/tendermint: Change order of events returned from GetEvents() | |||
There was a problem hiding this comment.
I guess under a strict definition this could be considered a breaking change as it modifies the order in API responses. However, this is not a consensus breaking change (no consensus issues in case some nodes apply this change) and could be considered a bugfix. That would be useful if we want to backport this to 22.2.x branch (iirc the release process forbids breaking changes in point releases).
Since oasis-indexer relies on this change, I would recommend backporting this to stable/22.2.x and making a new release, as otherwise the nodes used by the indexer would need a custom-built version of oasis-node.
There was a problem hiding this comment.
Thanks!
I meanwhile already fixed the indexer so it does some heuristic reordering of events. So it does not depend on this change. I was hoping that once this PR goes in, I could remove the reordering on the indexer side, but I guess we'll also have to process archive data from Cobalt sooner or later, and I'm not sure it's worth backporting this change all the way there.
Still, it's nice to have flexibility. Good to know this is not consensus-breaking. Relabeling as bugfix.
mitjat
force-pushed
the
mitjat/getevents-order
branch
from
6423bfd
to
07c003e
Compare
Currently consensus events (e.g. in various GetEvents RPCs) are returned in the following order:
This PR switches all GetEvents RPCs to the more natural order:
The motivation comes from oasis-indexer, where processing events in the order they were returned before this PR can lead to violated assumptions (e.g. "no account can ever have negative balance").