Limit the recursion for msg gatekeeper authz messages?

[rootulp]

An alternative option to the one Callum proposed is:

func (mgk MsgVersioningGateKeeper) hasInvalidMsg(ctx sdk.Context, acceptedMsgs map[string]struct{}, msgs []sdk.Msg) error {
    for _, msg := range msgs {
        // Recursively check for invalid messages in nested authz messages.
        if execMsg, ok := msg.(*authz.MsgExec); ok {
            nestedMsgs, err := execMsg.GetMessages()
            if err != nil {
                return err
            }
+           if err = mgk.hasNestedMsgExec(ctx, nestedMsgs); err != nil {
+               return err
+           }
            if err = mgk.hasInvalidMsg(ctx, acceptedMsgs, nestedMsgs); err != nil {
                return err
            }
        }

        msgTypeURL := sdk.MsgTypeURL(msg)
        _, exists := acceptedMsgs[msgTypeURL]
        if !exists {
            return sdkerrors.ErrNotSupported.Wrapf("message type %s is not supported in version %d", msgTypeURL, ctx.BlockHeader().Version.App)
        }
    }

    return nil
}

+func (mgk MsgVersioningGateKeeper) hasNestedMsgExec(_ sdk.Context, nestedMsgs []sdk.Msg) error {
+   for _, nestedMsg := range nestedMsgs {
+       if _, ok := nestedMsg.(*authz.MsgExec); ok {
+           return sdkerrors.ErrNotSupported.Wrapf("nested MsgExec is not allowed in MsgExec")
+       }
+   }

    return nil
}

but my concern with adding this is that even though a MsgExec inside a MsgExec seems like a strange use case, what if it's actually desirable for certain scenarios.

I don't see a major risk from not adding this constraint because if a user crafts a deeply nested MsgExec wrapper (i.e. MsgExec(MsgExec(MsgExec(...))) then the transaction will be rejected based on tx size.

_Originally posted by @rootulp in https://github.com/celestiaorg/celestia-app/pull/3555#discussion_r1636764149_

[rootulp]

The alternative approach would be what @cmwaters originally suggested which is to remove authz.MsgExec from the list of acceptedMsgs.

[cmwaters]

but my concern with adding this is that even though a MsgExec inside a MsgExec seems like a strange use case, what if it's actually desirable for certain scenarios.

I think it's sufficient to leave it as is (with the assumption that there is no valid use case) and only make an adjustment if a user presents a valid use case to modify it

[rootulp]

Just to clarify are you in favor of:

Option A. allowing nested authz messages (the current implementation) Option B. enforcing some upper limit on recursion for authz messages (a.k.a something like https://github.com/celestiaorg/celestia-app/pull/3590 or split out into it's own ante handler).

[cmwaters]

Option A.

Sorry for the confusion. I actually changed my mind on this. I think if we want to prevent nesting it should be it's own antehandler. For now, we should leave this ante handler to allow nesting

[rootulp]

Thanks for clarifying. Do you think adding a new ante handler should block v2?

[rootulp]

Closing this as won't do and created a new issue b/c we don't have clarity yet: https://github.com/celestiaorg/celestia-app/issues/3609

[cmwaters]

Do you think adding a new ante handler should block v2?

Personally, no. And as has been discussed, it should probably be a CIP