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

perf(ante): Make the redundancy ante-handler check not run recvpacket middleware #6248

Closed
wants to merge 3 commits into from
Closed

perf(ante): Make the redundancy ante-handler check not run recvpacket middleware #6248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f7bc1e
Make the ante handler not run RecvPacket middleware
ValarDragon May 1, 2024
079a3ea
Remove log messages
ValarDragon May 1, 2024
6029be0
Update comment
ValarDragon May 1, 2024
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
4 changes: 2 additions & 2 deletions modules/core/ante/ante.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
for _, m := range tx.GetMsgs() {
switch msg := m.(type) {
case *channeltypes.MsgRecvPacket:
response, err := rrd.k.RecvPacket(ctx, msg)
redundant, err := rrd.k.RecvPacketCheckNonRedundant(ctx, msg)
if err != nil {
return ctx, err
}
if response.Result == channeltypes.NOOP {
if redundant {
Comment on lines +33 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The verification process has confirmed the presence and implementation of the RecvPacketCheckNonRedundant function within the modules/core/keeper/msg_server.go file. The function is designed to handle errors appropriately, as evidenced by the detailed error handling and conditional logic present in the function's implementation.

However, the verification process did not find any test cases specifically targeting the RecvPacketCheckNonRedundant function. This absence of direct test coverage could pose risks, especially considering the function's role in handling packet redundancy checks, which are critical for the system's integrity and performance.

Given this situation, it is recommended to implement dedicated test cases to ensure the robustness and correctness of the RecvPacketCheckNonRedundant function under various scenarios, including error conditions and edge cases.

Action Points:

  • Implement test cases for RecvPacketCheckNonRedundant to cover various scenarios and error conditions.
  • Review the existing test suite to ensure that indirect coverage, if any, is sufficient until specific tests are developed.
Analysis chain

Integration of RecvPacketCheckNonRedundant looks good, but ensure thorough testing of error scenarios.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the `RecvPacketCheckNonRedundant` function is properly integrated and handles errors correctly.

# Search for the function definition and usage in the codebase to ensure it's correctly implemented and called.
ast-grep --lang go --pattern $'func (k *Keeper) RecvPacketCheckNonRedundant($_, $_) ($_,$_)' modules/core/keeper/msg_server.go

# Search for test cases covering this function.
rg --files-with-matches --type go $'RecvPacketCheckNonRedundant' | xargs rg 'func Test.*RecvPacketCheckNonRedundant'

Length of output: 2667

redundancies++
}
packetMsgs++
Expand Down
40 changes: 40 additions & 0 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,46 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack
return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil
}

// RecvPacketCheckNonRedundant checks that a MsgRecvPacket is:
// 1) Valid
// 2) Not a duplicate of a packet already received
// It is only intended to be ran on CheckTx.
func (k *Keeper) RecvPacketCheckNonRedundant(goCtx context.Context, msg *channeltypes.MsgRecvPacket) (bool, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

// Lookup module by channel capability
module, capability, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel)
if err != nil {
return false, errorsmod.Wrap(err, "could not retrieve module from port-id")
}

// Retrieve callbacks from router
_, ok := k.PortKeeper.Route(module)
if !ok {
return false, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)
}

// Perform TAO verification
//
// If the packet was already received, perform a no-op
// Use a cached context to prevent accidental state changes
cacheCtx, writeFn := ctx.CacheContext()
// TODO: Update RecvPacket to skip MT inclusion checks on Recheck.
// Its slightly involved because right now every client is responsible for checking that the
// height exists on the client, rather than the channel keeper.
err = k.ChannelKeeper.RecvPacket(cacheCtx, capability, msg.Packet, msg.ProofCommitment, msg.ProofHeight)

switch err {
case nil:
writeFn()
case channeltypes.ErrNoOpMsg:
return true, nil
default:
return false, errorsmod.Wrap(err, "receive packet verification failed")
}
return false, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect return value in RecvPacketCheckNonRedundant.

- return false, nil
+ return true, nil

This function should return true, nil when no error occurs and the packet is non-redundant, as per the intended logic described in the comments and switch-case handling.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
return false, nil
return true, nil

}

// Timeout defines a rpc handler method for MsgTimeout.
func (k *Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*channeltypes.MsgTimeoutResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)
Expand Down
Loading