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

chore: rm event emission after context caching #2662

Merged
merged 9 commits into from Nov 10, 2022
Merged

chore: rm event emission after context caching #2662

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b83b7c3
rm event emission after context caching
charleenfei Nov 2, 2022
75e21ff
Merge branch 'main' into charly/2646_event_emission
charleenfei Nov 2, 2022
3d21f5e
Merge branch 'main' into charly/2646_event_emission
charleenfei Nov 3, 2022
d3a1487
update event emissions
charleenfei Nov 9, 2022
f3565a7
Merge branch 'main' into charly/2646_event_emission
charleenfei Nov 9, 2022
632e094
rm unnecessary event emission
charleenfei Nov 9, 2022
5f4e1fb
Merge branch 'charly/2646_event_emission' of github.com:cosmos/ibc-go…
charleenfei Nov 9, 2022
591946a
fix pr comments
charleenfei Nov 10, 2022
1ceea32
Merge branch 'main' into charly/2646_event_emission
charleenfei Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion modules/apps/27-interchain-accounts/host/keeper/relay.go
View file
Open in desktop
Expand Up @@ -74,7 +74,6 @@ func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel str
}

// NOTE: The context returned by CacheContext() creates a new EventManager, so events must be correctly propagated back to the current context
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
Copy link
Contributor

Choose a reason for hiding this comment

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

Note the safety here is that the cached context is being used to cache execution of all msg executions. We use another cache context for each individual message execution (which emits events using result events since message handler will construct a new event manager)
ie

cacheCtx, writeCacheCtx:= ctx.CacheContext()
for msg := rangs msgs {
    cacheCtxInner, writeCacheCtxInner:= cacheCtx.CacheContext()
    
    res, err := handler(msg) // does not set emitted events in cacheCtxInner
    if err != nil {
       // all changes discarded
    }
    
    emitEvents(res.Events()) // emit events otherwise not written
    writeCacheContextInner() 
}

writeCacheCtx() // writes events

Copy link
Member

Choose a reason for hiding this comment

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

++

writeCache()

txResponse, err := proto.Marshal(txMsgData)
Expand Down
12 changes: 0 additions & 12 deletions modules/apps/29-fee/keeper/escrow.go
View file
Open in desktop
Expand Up @@ -74,9 +74,6 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe
k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee)
}

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

// write the cache
writeFn()

Expand Down Expand Up @@ -130,9 +127,6 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee)
}

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

// write the cache
writeFn()

Expand Down Expand Up @@ -177,9 +171,6 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.Ac

// write the cache
writeFn()

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
}

// RefundFeesOnChannelClosure will refund all fees associated with the given port and channel identifiers.
Expand Down Expand Up @@ -228,9 +219,6 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st
}
}

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

// write the cache
writeFn()

Expand Down
33 changes: 18 additions & 15 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Expand Up @@ -395,15 +395,16 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke
cacheCtx, writeFn := ctx.CacheContext()
err = k.ChannelKeeper.RecvPacket(cacheCtx, cap, msg.Packet, msg.ProofCommitment, msg.ProofHeight)

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

switch err {
case nil:
writeFn()
case channeltypes.ErrNoOpMsg:
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// no-ops do not need event emission as they will be ignored.
return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil
default:
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
// no-ops do not need event emission as they will be ignored.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return nil, sdkerrors.Wrap(err, "receive packet verification failed")
}

Expand All @@ -412,12 +413,13 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke
// Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful.
cacheCtx, writeFn = ctx.CacheContext()
ack := cbs.OnRecvPacket(cacheCtx, msg.Packet, relayer)
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
// Events from callback are emitted regardless of acknowledgement success
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
if ack == nil || ack.Success() {
// write application state changes for asynchronous and successful acknowledgements
writeFn()
} else {
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
// Events should still be emitted from failed acks and asynchronous acks
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Events should still be emitted from failed acks and asynchronous acks
// events are emitted for asynchronous and failed acknowledgements

ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
}

// Set packet acknowledgement only if the acknowledgement is not nil.
Expand Down Expand Up @@ -473,15 +475,16 @@ func (k Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*c
cacheCtx, writeFn := ctx.CacheContext()
err = k.ChannelKeeper.TimeoutPacket(cacheCtx, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv)

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

switch err {
case nil:
writeFn()
case channeltypes.ErrNoOpMsg:
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
// no-ops do not need event emission as they will be ignored.
return &channeltypes.MsgTimeoutResponse{Result: channeltypes.NOOP}, nil
default:
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
// no-ops do not need event emission as they will be ignored.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return nil, sdkerrors.Wrap(err, "timeout packet verification failed")
}

Expand Down Expand Up @@ -541,15 +544,16 @@ func (k Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTimeo
cacheCtx, writeFn := ctx.CacheContext()
err = k.ChannelKeeper.TimeoutOnClose(cacheCtx, cap, msg.Packet, msg.ProofUnreceived, msg.ProofClose, msg.ProofHeight, msg.NextSequenceRecv)

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

switch err {
case nil:
writeFn()
case channeltypes.ErrNoOpMsg:
// no-ops do not need event emission as they will be ignored.
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
return &channeltypes.MsgTimeoutOnCloseResponse{Result: channeltypes.NOOP}, nil
default:
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
// no-ops do not need event emission as they will be ignored.
return nil, sdkerrors.Wrap(err, "timeout on close packet verification failed")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -612,15 +616,14 @@ func (k Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAckn
cacheCtx, writeFn := ctx.CacheContext()
err = k.ChannelKeeper.AcknowledgePacket(cacheCtx, cap, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight)

// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())

switch err {
case nil:
writeFn()
case channeltypes.ErrNoOpMsg:
// no-ops do not need event emission as they will be ignored.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return &channeltypes.MsgAcknowledgementResponse{Result: channeltypes.NOOP}, nil
default:
// no-ops do not need event emission as they will be ignored.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return nil, sdkerrors.Wrap(err, "acknowledge packet verification failed")
}

Expand Down