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

Jump to bottom

explicitly propagate events from cached contexts to original context in delaymsg endblocker #393

Merged
merged 5 commits into from
Sep 29, 2023
Merged

explicitly propagate events from cached contexts to original context in delaymsg endblocker #393

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e9519a
explicitly propagate events from cached contexts to original context …
tqin7 Sep 27, 2023
ff59964
add test case that fails before code change and succeeds after
tqin7 Sep 28, 2023
ddc064f
Merge branch 'main' into tq/core-608
tqin7 Sep 28, 2023
17215a5
update comments
tqin7 Sep 28, 2023
fd1758b
Merge branch 'main' into tq/core-608
tqin7 Sep 29, 2023
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
19 changes: 17 additions & 2 deletions protocol/x/delaymsg/keeper/dispatch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ func DispatchMessagesForBlock(k types.DelayMsgKeeper, ctx sdk.Context) {
return
}

// Maintain a list of events emitted by all delayed messages executed in this block.
// As each delayed message is executed in a cached context, their emitted events need
tqin7 marked this conversation as resolved.
Show resolved Hide resolved
// to be explicitly propagated to the current context.
// Note: events in EndBlocker can be found in `end_block_events` in response from
// `/block_results` endpoint.
var events sdk.Events

// Execute all delayed messages scheduled for this block and delete them from the store.
for _, id := range blockMessageIds.Ids {
delayedMsg, found := k.GetMessage(ctx, id)
Expand All @@ -33,13 +40,21 @@ func DispatchMessagesForBlock(k types.DelayMsgKeeper, ctx sdk.Context) {

if err = abci.RunCached(ctx, func(ctx sdk.Context) error {
handler := k.Router().Handler(msg)
_, err := handler(ctx, msg)
return err
res, err := handler(ctx, msg)
if err != nil {
return err
}
// Append events emitted in cached context to `events`.
tqin7 marked this conversation as resolved.
Show resolved Hide resolved
events = append(events, res.GetEvents()...)
return nil
}); err != nil {
k.Logger(ctx).Error("failed to execute delayed message with id %v: %v", id, err)
}
}

// Propagate events emitted in cached contexts to current context.
ctx.EventManager().EmitEvents(events)
Copy link
Contributor

@teddyding teddyding Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We should wrap this event emitting logic in RunCached util, if possible. Does the axelar example util work?

Copy link
Contributor

@clemire clemire Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you double-check that such a change doesn't cause events to be emitted twice? I think the writeCache method does emit events, so I'm wondering if the returned events for this transfer did not get added to the cachedCtx event manager, and that's why they were missing. In that case, handling this from RunCached may not be the solution.

Copy link

Choose a reason for hiding this comment

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

There were a lot of issues due to sdk message handler creating new EventManager. The emitted events could be picked up from the result like in: cosmos/cosmos-sdk#13389
or just propagate events like axelar did and @teddyding is suggesting.I believe Axelar as well had issues with losing events with their initial RunCached implementation: axelarnetwork/axelar-core#1439.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I emit events in RunCached like axelar example, they don't show up any more. The reason is that message handlers create a new event manager and thus the events aren't present in ctx.EventManager() in this line (The cosmos issue that @dalmirel linked also mentioned the same issue)

I think for x/delaymsg's use case, we have to emit events that are part of handler response


for _, id := range blockMessageIds.Ids {
if err := k.DeleteMessage(ctx, id); err != nil {
k.Logger(ctx).Error("failed to delete delayed message: %w", err)
Expand Down